diff --git a/.claude/skills/pre-push-gate/SKILL.md b/.claude/skills/pre-push-gate/SKILL.md index ca679038f..c43fbdde9 100644 --- a/.claude/skills/pre-push-gate/SKILL.md +++ b/.claude/skills/pre-push-gate/SKILL.md @@ -98,6 +98,21 @@ stale install otherwise passes every check and fails later as a behavioral test reporting the *old* dependency's behavior as a product bug (#2494). Don't "fix" that test. +### `verify:install-scripts` + +A dependency with an install script is neither allowed nor denied in the +`allowScripts` of the `package.json` beside that lockfile (#2712) — usually a +dependency you just added or bumped. npm 11.x still **runs** such a script, so +this is a review decision, not a typo: add `"": false` to block it, or +`"@": true` if the package genuinely needs it. Each install +reads only its own manifest, so a package held by several installs needs an +entry in each. A **non-registry** dependency (git, file, remote tarball) is +reported even with no flagged script, since npm may run its `prepare`; clear it +with a key equal to its exact `resolved` spec from the lockfile. A **bundled** +one can't be cleared by any key — the fix is the dependency that bundles it. +Don't reach for `ignore-scripts` — it also disables the root +`postinstall` that installs every client. + ### `local:dco` A commit in `origin/v2/main..HEAD` (minus anything already on `origin/main`) diff --git a/clients/cli/package.json b/clients/cli/package.json index 79ef02d32..5f48bd335 100644 --- a/clients/cli/package.json +++ b/clients/cli/package.json @@ -32,6 +32,10 @@ "format": "prettier --write src __tests__ \"*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}\"", "format:check": "prettier --check src __tests__ \"*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}\"" }, + "allowScripts": { + "esbuild": false, + "fsevents": false + }, "devDependencies": { "@types/express": "^5.0.6", "tsup": "^8.5.0" diff --git a/clients/mcpdo/package.json b/clients/mcpdo/package.json index 58cf251b2..e38c7265f 100644 --- a/clients/mcpdo/package.json +++ b/clients/mcpdo/package.json @@ -27,6 +27,10 @@ "format": "prettier --write src __tests__ scripts \"*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}\"", "format:check": "prettier --check src __tests__ scripts \"*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}\"" }, + "allowScripts": { + "esbuild": false, + "fsevents": false + }, "devDependencies": { "@types/express": "^5.0.6", "tsup": "^8.5.0" diff --git a/clients/tui/package.json b/clients/tui/package.json index 97414d4d5..eba5b4578 100644 --- a/clients/tui/package.json +++ b/clients/tui/package.json @@ -27,6 +27,10 @@ "format": "prettier --write src __tests__ \"*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}\"", "format:check": "prettier --check src __tests__ \"*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}\"" }, + "allowScripts": { + "esbuild": false, + "fsevents": false + }, "dependencies": { "ink": "^6.0.0", "ink-form": "^2.0.2", diff --git a/clients/web/package.json b/clients/web/package.json index 0769b756a..08e07ad01 100644 --- a/clients/web/package.json +++ b/clients/web/package.json @@ -33,6 +33,10 @@ "preview": "vite preview", "storybook": "storybook dev -p 6006" }, + "allowScripts": { + "esbuild": false, + "fsevents": false + }, "dependencies": { "@dnd-kit/core": "^6.3.1", "@dnd-kit/sortable": "^8.0.0", diff --git a/docs/quality-gate.md b/docs/quality-gate.md index 198e71ba0..6d761ff38 100644 --- a/docs/quality-gate.md +++ b/docs/quality-gate.md @@ -34,12 +34,13 @@ That is the readable half, and prose rots. The enforced half is `scripts/lib/wor | Script | What it does | | ----------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `npm run validate` | Runs the seven durable guards first — `verify:install-fresh` (every install's `node_modules` matches its own lockfile, so a checkout that pulled a dependency bump without re-running `npm install` fails in under a second naming it, instead of minutes later as a behavioral test — #2494), `verify:format-coverage` (every tracked source file is format-gated), `verify:skills` (every `.claude/skills` manifest parses and declares its invocation mode; the **model-invoked** ones also carry eval cases — the name-only skills intentionally have none), `verify:typecheck-coverage` (every tracked TS file lands in a tsconfig project), `verify:dep-lockstep` (two tiers: no dependency reaching one `tsc` program from two installs skews across them, and — since #2226 — no **declared** package holds two different top-level versions across installs at all), `verify:test-timeouts` (every Vitest project resolves to a budget somebody chose, both web projects configure Testing Library's `asyncUtilTimeout`, and every project loads the runtime no-retry setup), `verify:action-pins` (every action a credentialed workflow job runs is SHA-pinned — #2484) — then `test:scripts` (the guards' own parser unit tests), then `validate:core` (the shared `core/` `format:check` + `lint` gate — including the `core/react/**` React-hooks block added by #2192, whose plugin is a **root** devDependency because a client-local copy is unreachable from the root: Node resolution walks up, not down), then per client: `format:check` + `lint` + **`typecheck`** (cli/tui/launcher; web typechecks via `tsc -b` inside its `build`) + `build` + fast unit tests. The quick inner-loop check. | +| `npm run validate` | Runs the eight durable guards first — `verify:install-fresh` (every install's `node_modules` matches its own lockfile, so a checkout that pulled a dependency bump without re-running `npm install` fails in under a second naming it, instead of minutes later as a behavioral test — #2494), `verify:install-scripts` (every dependency install script is explicitly allowed or denied in its install's `allowScripts` — #2712), `verify:format-coverage` (every tracked source file is format-gated), `verify:skills` (every `.claude/skills` manifest parses and declares its invocation mode; the **model-invoked** ones also carry eval cases — the name-only skills intentionally have none), `verify:typecheck-coverage` (every tracked TS file lands in a tsconfig project), `verify:dep-lockstep` (two tiers: no dependency reaching one `tsc` program from two installs skews across them, and — since #2226 — no **declared** package holds two different top-level versions across installs at all), `verify:test-timeouts` (every Vitest project resolves to a budget somebody chose, both web projects configure Testing Library's `asyncUtilTimeout`, and every project loads the runtime no-retry setup), `verify:action-pins` (every action a credentialed workflow job runs is SHA-pinned — #2484) — then `test:scripts` (the guards' own parser unit tests), then `validate:core` (the shared `core/` `format:check` + `lint` gate — including the `core/react/**` React-hooks block added by #2192, whose plugin is a **root** devDependency because a client-local copy is unreachable from the root: Node resolution walks up, not down), then per client: `format:check` + `lint` + **`typecheck`** (cli/tui/launcher; web typechecks via `tsc -b` inside its `build`) + `build` + fast unit tests. The quick inner-loop check. | | `npm run coverage` | The **per-file ≥90% gate** (lines/statements/functions/branches) under v8 instrumentation, per client. CI-enforced. For web this also runs the integration project and covers the shared `core/` runtime (including `core/json` and `core/client`). **`maxWorkers` is deliberately unset**, here and in every Vitest project ([#2336](https://github.com/modelcontextprotocol/inspector/issues/2336), PR [#2345](https://github.com/modelcontextprotocol/inspector/pull/2345)): the forks-pool projects inherit Vitest's `max(availableParallelism() - 1, 1)` — 7 on the eight-logical-core machine this team works on — and keeping it is a choice with a measurement, recorded beside `TIMEOUTS` in `vitest.shared.mts`: at the leased baseline, arms interleaved over three rounds, `maxWorkers: 4` cost +14% wall on web `unit` and +39% on `coverage:web` for zero fewer failures (0 across all 18 runs). See [Multi-agent testing](#multi-agent-testing-the-gate-lease) for why a cap was the wrong lever for the contention it was proposed against. Ends with `verify:coverage-scope` (below), which checks that each report measured the file set its client declares. | | `npm run smoke` | End-to-end smokes through the built launcher (`--help` dispatch + prod cli/tui/web — the TUI one runs under a **pseudoterminal** and asserts the Ink app is still running two seconds after its first frame, not merely that it painted one, [#2147](https://github.com/modelcontextprotocol/inspector/issues/2147)), plus three headless-browser smokes: a boot smoke that runs the prod web bundle and asserts a clean first render (no uncaught error — sync exception or unhandled rejection, how a Node built-in reaching the browser bundle manifests), and an **MCP Apps** smoke (`smoke:web:app`) that drives connect → open app → `data-app-status="ready"` against a composable App server, covering the sandbox proxy and UI-protocol bridge, and an **app-rendered elicitation** smoke (`smoke:web:elicit`) that drives one end to end — call the tool, answer inside the sandboxed app, see the app's `ElicitResult` reach the server — and then the same tool against a server that never advertised the capability, which must fall back to the native elicitation form. Those three take their engine from `SMOKE_BROWSER` — see [Supported browsers](#supported-browsers). A fourth, **`smoke:web:tabs`**, drives the core tabs against one server on one browser launch — Tools runs a tool and asserts its `structuredContent` section, Resources reads a resource with templates listed, Prompts fetches one ([#2148](https://github.com/modelcontextprotocol/inspector/issues/2148)) — asserting the `data-*` contract in [the web README](../clients/web/README.md#core-tab-automation-contract) rather than visible copy. It is **Chromium-only**: those tabs are ordinary React and Mantine, so unlike the sandbox they are not engine-sensitive. The Playwright budgets the browser smokes wait under are **named once**, in `scripts/lib/browser-timeouts.mjs` (`BROWSER_TIMEOUTS`: `ui` 30s, `roundTrip` 45s, `nested` 15s, `bestEffort` 5s — [#2323](https://github.com/modelcontextprotocol/inspector/issues/2323)); since [#2333](https://github.com/modelcontextprotocol/inspector/issues/2333) (PR [#2349](https://github.com/modelcontextprotocol/inspector/pull/2349)) the shared flow helpers `deep-link-connect.mjs` and `mcp-app-flow.mjs` default to those constants instead of carrying literals of their own, and the two helper budgets that stay put are **stated at the site with the measurement behind them**: `announced-child.mjs`'s `DEFAULTS` (a 30s readiness ceiling on a 250ms poll — boot-to-announce measured 135ms median through it at load average 15, so a passing start pays one poll interval, not the budget) and `pty.mjs`'s `SCRIPT_PROBE_TIMEOUT_MS` (5s for the `script --version` probe, measured at 1.3ms median). That probe's expiry is now a **failure, not a skip**: a timed-out `spawnSync` reports in the same `error` slot as `ENOENT`, and reading both as "no `script(1)` on PATH" had turned `smoke:tui` into an exit-0 skip on exactly the loaded machine the gate is meant to be believable on. | | `npm run verify:build-gate` | Runs a real `vite build` with a Node built-in forced into the browser graph and asserts the build **fails** via the #1769 gate (which turns Vite's browser-externalization warning into a hard error). Guards against the warning phrasing drifting in a Vite bump and silently disabling the gate. Part of `npm run local:gate` and of GitHub CI. | | `npm run verify:bundle-externals` | Guards the must-not-bundle invariant (#2067): for each tsup-bundled client it reads the **built** `build/` output and fails if any package that must stay external was inlined anyway. Candidates are the union of the client's own `external` array **and the root manifest's `dependencies`** — the latter because #2067 was a *missing* `external` entry, which a self-referential check would have passed. Detection is via esbuild's `// ` module banners, so it covers both shapes — a separate `-HASH.js` chunk (what a dynamically `import()`ed CommonJS package produces) and a statically-imported package folded straight into `index.js`, which emits no chunk at all. A build with no banners fails as such rather than passing clean, so enabling `minify` cannot silently retire the check. `undici` was declared only in the root and `clients/cli` manifests, and tsup auto-externalizes only what the _nearest_ manifest declares, so the web and TUI bundles inlined 1.05MB of it — and CommonJS inlined into an ESM bundle throws `Dynamic require of "assert" is not supported` on first use, from a specifier no user-side install can satisfy. Reads the output rather than the config because those two disagreed for four releases. Part of `npm run local:gate` and of GitHub CI. | | `npm run verify:install-fresh` | Guards against a **stale install** ([#2494](https://github.com/modelcontextprotocol/inspector/issues/2494)). Every other dependency guard reads lockfiles, so none can see `node_modules` lagging the lockfile beside it — the ordinary state of a long-lived checkout after a `git pull` brings in a bump. When the MCP SDK moved 2.0.0 → 2.1.0, such a checkout passed every guard, lint and typecheck and failed only in `transportAuthorizationPrecedence.test.ts`, reporting the old SDK's behavior as a product defect. For the root and every `clients/*` install, compares each `package-lock.json` package entry against the installed copy's own `package.json` version (ground truth, not npm's hidden lockfile), exempting absent `optional`/`devOptional` entries (platform binaries), and fails naming each stale or missing package and `npm install`. Runs first in `validate:guards`; CI installs fresh, so it never fires there. | +| `npm run verify:install-scripts` | Guards against an **unreviewed dependency install script** ([#2712](https://github.com/modelcontextprotocol/inspector/issues/2712)). npm 11.x still runs a script that `allowScripts` neither allows nor denies, so a new dependency's `postinstall` would otherwise run on every install unnoticed. For the root and every `clients/*` install, reads each `package-lock.json` entry flagged `hasInstallScript` and fails unless that install's **own** `package.json` `allowScripts` decides it (`true` or `false`), matched as npm 11.19 matches it: identity from the lockfile's `resolved` registry URL rather than the folder name, and only a bare name or exact versions joined by `||` as keys — npm ignores `^`/`~` ranges. It also reports a **non-registry** dependency (git, file, remote tarball) even when the lockfile does not flag a script, because npm runs `prepare` for those and the lockfile records only preinstall/install/postinstall; such an entry is cleared by a key equal to its exact `resolved` spec. A bundled dependency with a script is always reported, since no key can decide it. Reads the lockfile rather than `npm install-scripts ls` because CI's Node 22 npm 10.x has no such subcommand. `esbuild` and `fsevents` are denied: neither script is needed (platform binaries arrive via `optionalDependencies` / prebuilt). | | `npm run verify:format-coverage` | Parses the `format:check` globs out of every `package.json` (only those reachable from `validate`), enumerates all tracked source files, and **fails** listing any not covered by a glob — the durable guard for the "every first-party source file is format-gated" invariant (#1792). Runs first in `validate`. | | `npm run verify:skills` | Guards `.claude/skills` ([#2163](https://github.com/modelcontextprotocol/inspector/issues/2163)). A skill's body loads only when it is invoked, so one that stops being reachable loses behavior **silently** — and the way that happens is frontmatter Claude Code cannot parse, which loads the body with an *empty* description: `/skill-name` still works, so a manual spot check passes, while nothing is left for the model to match against. This parses each `SKILL.md` the same way, requires an explicit invocation mode on every skill and committed eval cases (positives **and** negatives) on every model-invoked one, and fails when the skill listing exceeds its recorded character budget. Also runs `claude plugin validate` — the authoritative schema — when the installed CLI is **exactly** the pinned version, and otherwise says so and moves on. It runs inside `validate:guards`, which both `validate` (CI) and `local:validate` (the gate's first stage) execute *before* the pinned step, so accepting any other local CLI would let it reject skills that CI accepts and exit the gate before the reproducible check ever ran. Runs in `validate`; because that hand-off is best-effort, the authoritative validator also gets a guaranteed step of its own — see `verify:skills:cli` below. | | `npm run verify:skills:cli` | Runs `claude plugin validate` — the **authoritative** skill schema — as a guaranteed step, in `local:gate` and in CI. `verify:skills` reimplements the parse and *skips* when the CLI is absent, which is right for `validate` (fast, offline, no Claude Code required) but adds up to "never runs". This one resolves the CLI instead: an installed one **only when it matches the pin exactly**, otherwise the pinned package via `npx -y @anthropic-ai/claude-code@…`. Exact, not a floor: accepting a newer local CLI would validate against a different schema than CI's, so the same `local:gate` could disagree across machines — which is the failure a pin exists to prevent. Needs no authentication. Not in `validate` — it can reach the network. | diff --git a/package.json b/package.json index 13d20a0ec..dd496f4ef 100644 --- a/package.json +++ b/package.json @@ -74,10 +74,11 @@ "release:tag": "node scripts/release-tag.mjs", "action:resolve-pin": "node scripts/action-pin-resolve.mjs", "validate": "npm run validate:guards && npm run validate:core && npm run validate:web && npm run validate:cli && npm run validate:mcpdo && npm run validate:tui && npm run validate:launcher", - "validate:guards": "npm run verify:install-fresh && npm run verify:format-coverage && npm run verify:skills && npm run verify:typecheck-coverage && npm run verify:dep-lockstep && npm run verify:test-timeouts && npm run verify:action-pins && npm run test:scripts", + "validate:guards": "npm run verify:install-fresh && npm run verify:install-scripts && npm run verify:format-coverage && npm run verify:skills && npm run verify:typecheck-coverage && npm run verify:dep-lockstep && npm run verify:test-timeouts && npm run verify:action-pins && npm run test:scripts", "verify:format-coverage": "node scripts/verify-format-coverage.mjs", "verify:dep-lockstep": "node scripts/verify-dep-lockstep.mjs", "verify:install-fresh": "node scripts/verify-install-fresh.mjs", + "verify:install-scripts": "node scripts/verify-install-scripts.mjs", "verify:test-timeouts": "node scripts/verify-test-timeouts.mjs", "verify:action-pins": "node scripts/verify-action-pins.mjs", "verify:coverage-scope": "node scripts/verify-coverage-scope.mjs", @@ -149,6 +150,9 @@ "engines": { "node": ">=22.19.0" }, + "allowScripts": { + "fsevents": false + }, "devDependencies": { "@eslint/js": "^10.0.1", "@modelcontextprotocol/server-legacy": "2.3.1", diff --git a/scripts/verify-install-scripts.mjs b/scripts/verify-install-scripts.mjs new file mode 100644 index 000000000..a48e1b7c9 --- /dev/null +++ b/scripts/verify-install-scripts.mjs @@ -0,0 +1,224 @@ +#!/usr/bin/env node +// Guard: no dependency install script is left unreviewed (#2712). +// +// npm 11.19 gates dependency install scripts behind an `allowScripts` policy +// in the installing project's `package.json`, but in 11.x a package that is +// neither allowed nor denied still RUNS its script, with only a warning — only +// an explicit `false` blocks it. npm 12 blocks unreviewed scripts by default, +// but this repo installs with npm 10 (CI's Node 22 bundle) and 11.x, so here a +// new dependency that brings a `postinstall` still runs it on every install, +// silently. +// This guard turns each one into a decision made in review: every package with +// an install script must carry an explicit `true` or `false` in the +// `allowScripts` of the install that holds it. +// +// v2 is not a workspace, and npm reads `allowScripts` only from the manifest of +// the install it is running in, so each install (the root plus every +// `clients/*` with a manifest — the enrolment `verify:install-fresh` uses) is +// checked against its own policy and no other. +// +// It reads the LOCKFILE's `hasInstallScript` rather than shelling out to +// `npm install-scripts ls`, which is what the issue first proposed: that +// subcommand only exists from npm 11.19, and CI's `build` job runs Node 22's +// bundled npm 10.x, where it is absent — so the guard would either fail in CI +// for an unrelated reason or skip there and enforce nothing. The lockfile flag +// is what npm itself records for exactly this question (preinstall, install, +// postinstall, or an implicit `node-gyp rebuild` from a `binding.gyp`), and +// reading it keeps the guard offline and independent of the npm on PATH. The +// `""` entry is the install's own project — the root `postinstall` that +// cascades into every client — which is not a dependency script and is out of +// scope. +// +// Matching mirrors npm 11.19's own (`@npmcli/arborist/lib/script-allowed.js`), +// because a guard that accepts a key npm ignores passes a script npm still +// runs: +// +// - Identity comes from the lockfile's `resolved` registry URL +// (`//-/-.tgz`), never from the +// `node_modules` folder name — an alias puts a package under a name it is +// not, and npm deliberately looks through that. +// - Only three key shapes are honoured for a registry package: a bare name or +// `name@*` (every version), and exact versions joined by `||` +// (`name@1.2.3 || 1.2.4`). npm rejects `^`/`~`/`>=` ranges as policy keys, +// so they are no review here either. +// - A package with no provable registry identity (git, tarball, file) is +// reviewed only by a key equal to its `resolved` spec; otherwise it fails +// closed, as unreviewed. That applies whether or not it is flagged +// `hasInstallScript`: npm also runs `prepare` for a non-registry dependency, +// and the lockfile flag records only preinstall/install/postinstall, so a +// prepare-only one would otherwise pass unseen. (A `link` entry points at +// in-repo source, which is reviewed as source, and is out of scope.) +// - A bundled dependency (`inBundle`) is always reported. npm's matcher +// returns "unreviewed" for one unconditionally, so no `allowScripts` key can +// decide it, and under 11.x an unreviewed script still runs during the +// bundle rebuild. The only remedy is the dependency that bundles it. +// +// A matching key counts as a review whether it allows or denies; a value that +// is neither `true` nor `false` does not count. + +import { existsSync, readdirSync, readFileSync } from "node:fs"; +import path from "node:path"; +import { fileURLToPath, pathToFileURL } from "node:url"; +import semver from "semver"; + +const repoRoot = path.resolve( + path.dirname(fileURLToPath(import.meta.url)), + "..", +); + +/** `node_modules/a/node_modules/@s/b` → `@s/b`, for display only. */ +export function packageNameOf(entryPath) { + const marker = "node_modules/"; + return entryPath.slice(entryPath.lastIndexOf(marker) + marker.length); +} + +/** + * The `{ name, version }` a registry tarball URL proves, or `null` — the same + * parse as npm's `versionFromTgz`, including a registry mounted below the root + * and a scope written as `%2f`. + */ +export function registryIdentity(resolved) { + if (typeof resolved !== "string" || !/^https?:\/\//.test(resolved)) + return null; + const pathname = new URL(resolved).pathname.slice(1); + const parts = pathname.split("/-/"); + if (parts.length < 2) return null; + const base = parts.pop(); + if (base.includes("/") || !base.endsWith(".tgz")) return null; + const segments = parts.pop().split(/\/|%2f/i); + const project = segments.pop(); + const scope = segments.pop(); + if (!project || !base.startsWith(`${project}-`)) return null; + const parsed = semver.parse(base.slice(project.length + 1, -".tgz".length)); + if (!parsed) return null; + const name = scope?.startsWith("@") ? `${scope}/${project}` : project; + return { name, version: parsed.version }; +} + +/** `name` and the spec after it, for a bare or scoped key. */ +function splitKey(key) { + const at = key.indexOf("@", key.startsWith("@") ? 1 : 0); + return at === -1 + ? { name: key, spec: "" } + : { name: key.slice(0, at), spec: key.slice(at + 1).trim() }; +} + +/** One or more exact versions joined by `||` — npm's only versioned key. */ +function isExactVersionDisjunction(spec) { + return spec + .split("||") + .map((p) => p.trim()) + .every((p) => p !== "" && semver.valid(p) !== null); +} + +/** Does this registry-style key cover `identity` the way npm reads it? */ +export function keyMatches(key, identity) { + const { name, spec } = splitKey(key); + if (name !== identity.name) return false; + if (spec === "" || spec === "*") return true; + return ( + isExactVersionDisjunction(spec) && semver.satisfies(identity.version, spec) + ); +} + +/** Has `allowScripts` decided on this lockfile entry? */ +export function isReviewed(allowScripts, entry) { + const identity = registryIdentity(entry.resolved); + return Object.entries(allowScripts ?? {}).some( + ([key, value]) => + typeof value === "boolean" && + (identity ? keyMatches(key, identity) : key === entry.resolved), + ); +} + +/** + * Every lockfile entry with an install script — or, for a non-registry + * entry, possibly a `prepare` — that `allowScripts` has not decided on, as `{ entryPath, name, version, bundled }` — `name` being the + * identity npm matches, or the folder name when none can be proven. A bundled + * entry is always included: no policy key can decide it. + */ +export function unreviewedScripts(lock, allowScripts) { + const found = []; + for (const [entryPath, entry] of Object.entries(lock?.packages ?? {})) { + if (!entryPath.startsWith("node_modules/")) continue; + if (!entry || entry.link) continue; + // A non-registry entry may run `prepare`, which no lockfile flag records. + // Only a `resolved` source says it is one: a bundled entry often carries + // none and never runs `prepare`, and an entry with no `resolved` at all is + // npm's own fallback for a registry dependency. + const mayPrepare = + !entry.inBundle && + typeof entry.resolved === "string" && + registryIdentity(entry.resolved) === null; + if (!entry.hasInstallScript && !mayPrepare) continue; + const bundled = entry.inBundle === true; + if (!bundled && isReviewed(allowScripts, entry)) continue; + const identity = registryIdentity(entry.resolved); + found.push({ + entryPath, + name: identity?.name ?? packageNameOf(entryPath), + version: identity?.version ?? entry.version ?? "", + bundled, + }); + } + return found; +} + +/** The root plus every `clients/*` directory with a `package.json`. */ +function installDirs(root) { + const clients = path.join(root, "clients"); + const dirs = existsSync(clients) + ? readdirSync(clients, { withFileTypes: true }) + .filter((e) => e.isDirectory()) + .map((e) => `clients/${e.name}`) + .filter((rel) => existsSync(path.join(root, rel, "package.json"))) + .sort() + : []; + return ["."].concat(dirs); +} + +export function main(root = repoRoot) { + const problems = []; + let scripted = 0; + const installs = installDirs(root); + for (const rel of installs) { + const dir = path.join(root, rel); + const lockPath = path.join(dir, "package-lock.json"); + // A missing lockfile is `verify:dep-lockstep`'s finding, not this guard's. + if (!existsSync(lockPath)) continue; + const lock = JSON.parse(readFileSync(lockPath, "utf8")); + const manifest = JSON.parse( + readFileSync(path.join(dir, "package.json"), "utf8"), + ); + scripted += Object.entries(lock.packages ?? {}).filter( + ([p, e]) => p.startsWith("node_modules/") && e?.hasInstallScript, + ).length; + for (const u of unreviewedScripts(lock, manifest.allowScripts)) + problems.push( + ` ${rel}: ${u.name}@${u.version} (${u.entryPath})` + + (u.bundled + ? " — bundled: no allowScripts key can decide it; drop or replace the dependency that bundles it" + : ""), + ); + } + if (problems.length > 0) { + console.error( + `verify:install-scripts — ${problems.length} dependency install script(s) are unreviewed:\n` + + problems.join("\n") + + "\n\nnpm 11.x still RUNS an install script that `allowScripts` neither allows nor" + + "\ndenies. Decide each one in the `allowScripts` of the package.json beside that" + + '\nlockfile: `"": false` to block it (a bare name covers every version),' + + '\nor `"@": true` to approve it — npm ignores `^`/`~` ranges here.' + + "\nEach install reads only its own manifest, so a package in several installs" + + "\nneeds an entry in each.", + ); + return 1; + } + console.log( + `verify:install-scripts — OK (${scripted} dependency install script(s) across ${installs.length} installs, all reviewed)`, + ); + return 0; +} + +if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) + process.exit(main()); diff --git a/scripts/verify-install-scripts.test.mjs b/scripts/verify-install-scripts.test.mjs new file mode 100644 index 000000000..b98c40be1 --- /dev/null +++ b/scripts/verify-install-scripts.test.mjs @@ -0,0 +1,265 @@ +// Tests for the install-script review guard. The policy matching and the +// lockfile scan are exercised in memory, and `main` is exercised against a +// throwaway tree on disk, so install discovery, the per-install policy and the exit status are +// covered too. Run via `npm run test:scripts`. + +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import path from "node:path"; +import { + isReviewed, + keyMatches, + main, + packageNameOf, + registryIdentity, + unreviewedScripts, +} from "./verify-install-scripts.mjs"; + +const REGISTRY = "https://registry.npmjs.org"; +const tgz = (name, version) => + `${REGISTRY}/${name}/-/${name.split("/").pop()}-${version}.tgz`; +const dep = (name, version, extra = {}) => ({ + version, + resolved: tgz(name, version), + hasInstallScript: true, + ...extra, +}); + +const lockOf = (packages) => ({ + lockfileVersion: 3, + packages: { "": { name: "x", hasInstallScript: true }, ...packages }, +}); + +test("packageNameOf takes the innermost package, scoped names included", () => { + assert.equal(packageNameOf("node_modules/esbuild"), "esbuild"); + assert.equal( + packageNameOf("node_modules/vite/node_modules/fsevents"), + "fsevents", + ); + assert.equal(packageNameOf("node_modules/a/node_modules/@s/b"), "@s/b"); +}); + +test("registryIdentity reads name and version off the tarball URL", () => { + assert.deepEqual(registryIdentity(tgz("esbuild", "0.28.2")), { + name: "esbuild", + version: "0.28.2", + }); + assert.deepEqual(registryIdentity(`${REGISTRY}/@s/b/-/b-1.0.0.tgz`), { + name: "@s/b", + version: "1.0.0", + }); + assert.deepEqual( + registryIdentity("https://host/mount/@s%2fb/-/b-1.0.0-rc.1.tgz"), + { name: "@s/b", version: "1.0.0-rc.1" }, + ); +}); + +test("registryIdentity proves nothing for a non-registry source", () => { + assert.equal(registryIdentity(undefined), null); + assert.equal(registryIdentity("git+ssh://git@github.com/a/b.git#abc"), null); + assert.equal(registryIdentity("file:../pkg"), null); + assert.equal(registryIdentity("https://host/pkg.tgz"), null); + assert.equal(registryIdentity(`${REGISTRY}/a/-/b-1.0.0.tgz`), null); + assert.equal(registryIdentity(`${REGISTRY}/a/-/a-latest.tgz`), null); + assert.equal(registryIdentity(`${REGISTRY}/a/-/a-1.0.0.zip`), null); +}); + +test("a bare name or name@* reviews every version, allow or deny", () => { + const id = { name: "fsevents", version: "2.3.2" }; + assert.equal(keyMatches("fsevents", id), true); + assert.equal(keyMatches("fsevents@*", id), true); + assert.equal(keyMatches("@s/b", { name: "@s/b", version: "1.0.0" }), true); + assert.equal(isReviewed({ fsevents: true }, dep("fsevents", "9.0.0")), true); +}); + +test("only exact versions joined by || review a version, as npm reads them", () => { + const id = { name: "esbuild", version: "0.28.2" }; + assert.equal(keyMatches("esbuild@0.28.2", id), true); + assert.equal(keyMatches("esbuild@0.28.1 || 0.28.2", id), true); + assert.equal(keyMatches("esbuild@0.28.3", id), false); + // npm rejects comparator ranges as policy keys, so they review nothing. + assert.equal(keyMatches("esbuild@^0.28.0", id), false); + assert.equal(keyMatches("esbuild@>=0.1.0", id), false); + assert.equal(keyMatches("esbuild@0.28.2 ||", id), false); + assert.equal( + keyMatches("@s/b@1.0.0", { name: "@s/b", version: "1.0.0" }), + true, + ); +}); + +test("a non-boolean value or another package is no review", () => { + const entry = dep("esbuild", "1.0.0"); + assert.equal(isReviewed({ esbuild: "maybe" }, entry), false); + assert.equal(isReviewed({ "esbuild-wasm": false }, entry), false); + assert.equal(isReviewed(undefined, entry), false); +}); + +test("an alias is matched by the package it resolves to, not its folder", () => { + // `"esbuild": "npm:naughty@1.0.0"` installs naughty under node_modules/esbuild. + const aliased = dep("naughty", "1.0.0"); + assert.equal(isReviewed({ esbuild: false }, aliased), false); + assert.equal(isReviewed({ naughty: false }, aliased), true); +}); + +test("a non-registry entry is reviewed only by its exact resolved spec", () => { + const git = { + version: "1.0.0", + resolved: "git+ssh://git@github.com/a/b.git#abc", + hasInstallScript: true, + }; + assert.equal(isReviewed({ b: false }, git), false); + assert.equal(isReviewed({ [git.resolved]: false }, git), true); +}); + +test("unreviewedScripts reports only scripted, undecided dependencies", () => { + const lock = lockOf({ + "node_modules/esbuild": dep("esbuild", "0.28.2"), + "node_modules/fsevents": dep("fsevents", "2.3.3"), + "node_modules/vite/node_modules/fsevents": dep("fsevents", "2.3.2"), + "node_modules/aliased": dep("naughty", "1.0.0"), + "node_modules/plain": { version: "1.0.0", resolved: tgz("plain", "1.0.0") }, + }); + // The `""` project entry carries the root postinstall and is never reported. + assert.deepEqual( + unreviewedScripts(lock, { fsevents: false, aliased: false }), + [ + { + entryPath: "node_modules/esbuild", + name: "esbuild", + version: "0.28.2", + bundled: false, + }, + { + entryPath: "node_modules/aliased", + name: "naughty", + version: "1.0.0", + bundled: false, + }, + ], + ); + assert.deepEqual( + unreviewedScripts(lock, { + esbuild: false, + fsevents: false, + naughty: false, + }), + [], + ); + const file = lockOf({ + "node_modules/b": { + version: "1.0.0", + resolved: "file:../b", + hasInstallScript: true, + }, + }); + assert.deepEqual(unreviewedScripts(file, {}), [ + { + entryPath: "node_modules/b", + name: "b", + version: "1.0.0", + bundled: false, + }, + ]); +}); + +test("a non-registry entry needs a decision even without the flag (prepare)", () => { + const git = "git+ssh://git@github.com/a/b.git#abc"; + const lock = lockOf({ + "node_modules/b": { version: "1.0.0", resolved: git }, + "node_modules/linked": { resolved: "../linked", link: true }, + // Scriptless and source-less: neither bundled nor registry-unresolved + // entries can run `prepare`, so neither needs a decision. + "node_modules/b/node_modules/bundled": { version: "1.0.0", inBundle: true }, + "node_modules/unresolved": { version: "1.0.0" }, + }); + assert.deepEqual(unreviewedScripts(lock, {}), [ + { + entryPath: "node_modules/b", + name: "b", + version: "1.0.0", + bundled: false, + }, + ]); + assert.deepEqual(unreviewedScripts(lock, { [git]: false }), []); +}); + +test("a bundled script is always reported: no policy key can decide it", () => { + const lock = lockOf({ + "node_modules/a/node_modules/bundled": dep("bundled", "1.0.0", { + inBundle: true, + }), + }); + assert.deepEqual(unreviewedScripts(lock, { bundled: false }), [ + { + entryPath: "node_modules/a/node_modules/bundled", + name: "bundled", + version: "1.0.0", + bundled: true, + }, + ]); +}); + +function tree(installs) { + const root = mkdtempSync(path.join(tmpdir(), "install-scripts-")); + for (const [rel, { manifest, lock }] of Object.entries(installs)) { + const dir = path.join(root, rel); + mkdirSync(dir, { recursive: true }); + writeFileSync(path.join(dir, "package.json"), JSON.stringify(manifest)); + if (lock) + writeFileSync(path.join(dir, "package-lock.json"), JSON.stringify(lock)); + } + return root; +} + +const silenced = (fn) => { + const { log, error } = console; + console.log = () => {}; + console.error = () => {}; + try { + return fn(); + } finally { + Object.assign(console, { log, error }); + } +}; + +const scripted = lockOf({ "node_modules/esbuild": dep("esbuild", "0.28.2") }); + +test("main passes when every install decides its own scripts", () => { + const root = tree({ + ".": { manifest: { allowScripts: { esbuild: false } }, lock: scripted }, + "clients/a": { + manifest: { allowScripts: { esbuild: false } }, + lock: scripted, + }, + // No lockfile: dep-lockstep's finding, skipped here. + "clients/b": { manifest: {} }, + }); + try { + assert.equal( + silenced(() => main(root)), + 0, + ); + } finally { + rmSync(root, { recursive: true, force: true }); + } +}); + +test("main fails when a client relies on another install's policy", () => { + const root = tree({ + ".": { manifest: { allowScripts: { esbuild: false } }, lock: scripted }, + "clients/a": { manifest: {}, lock: scripted }, + }); + const errors = []; + const { error } = console; + console.error = (msg) => errors.push(msg); + try { + assert.equal(main(root), 1); + assert.match(errors.join("\n"), /clients\/a: esbuild@0\.28\.2/); + assert.doesNotMatch(errors.join("\n"), /^ {2}\.: /m); + } finally { + console.error = error; + rmSync(root, { recursive: true, force: true }); + } +});