From c93552139cff38cb668270502b0d72a4a65c5222 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 17 Aug 2026 00:18:40 -0400 Subject: [PATCH] fix(scripts): keep the resolver local, complete the cmd.exe quoting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review round 3, all three real: - `require.resolve.paths()` also appends Node's GLOBAL FOLDERS ($HOME/.node_modules, $HOME/.node_libraries, $PREFIX/lib/node) and any NODE_PATH entries, so round 2's lookup could satisfy a missing repo install from a GLOBALLY installed typescript/vite — measuring the programs with the wrong compiler instead of producing the actionable "run npm install" this helper promises. Filter to the node_modules directories on fromDir's own ancestor chain. - The quoting predicate missed characters cmd.exe still parses, notably parentheses (`C:\Temp(1)`), and quoting cannot suppress `%VAR%` at all — expansion happens before quotes are processed. Extracted to lib/win-shell-args.mjs with a broad predicate and a hard error on `%` rather than a silently different path. `platform` is a parameter so the Windows behavior is exercised on the POSIX machines that run the suite. - AGENTS.md claimed pack-and-verify spawns via `process.execPath`; it does not, and cannot — its children are `npm` and the installed `.bin` shim. State the exception and why. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01387r3MotGk52hrkfjzHFsN Signed-off-by: cliffhall --- AGENTS.md | 13 +++--- scripts/lib/resolve-node-bin.mjs | 33 ++++++++++++++- scripts/lib/resolve-node-bin.test.mjs | 49 +++++++++++++++++++++- scripts/lib/win-shell-args.mjs | 58 +++++++++++++++++++++++++++ scripts/lib/win-shell-args.test.mjs | 54 +++++++++++++++++++++++++ scripts/pack-and-verify.mjs | 21 ++++------ 6 files changed, 207 insertions(+), 21 deletions(-) create mode 100644 scripts/lib/win-shell-args.mjs create mode 100644 scripts/lib/win-shell-args.test.mjs diff --git a/AGENTS.md b/AGENTS.md index 4d901292..74b403d3 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -129,11 +129,14 @@ v2/main/ │ # (tsc-program.mjs is the `tsc --listFilesOnly` │ # measurement both coverage guards read a program │ # through — #1965; resolve-node-bin.mjs resolves a -│ # package's bin through its own package.json so the -│ # verify/smoke/pack scripts spawn it with -│ # `process.execPath` instead of the `npx`/`npm` -│ # `.cmd` shims Node refuses to spawn shell-free on -│ # Windows — #1939). Prettier-gated via +│ # package's bin through its own package.json, so the +│ # verify/smoke scripts spawn it with `process.execPath` +│ # rather than the `npx` `.cmd` shim Node refuses to +│ # spawn shell-free on Windows. pack-and-verify.mjs is +│ # the exception — its children are `npm` itself and the +│ # installed `.bin` shim, neither resolvable that way, so +│ # it keeps a Windows shell and quotes its generated +│ # paths via win-shell-args.mjs — #1939). Prettier-gated via │ # `format:check:scripts`; its own pure parsers are │ # unit-tested by `npm run test:scripts` (node --test). ├── specification/ # Build specification diff --git a/scripts/lib/resolve-node-bin.mjs b/scripts/lib/resolve-node-bin.mjs index 33d223ae..8caf9f04 100644 --- a/scripts/lib/resolve-node-bin.mjs +++ b/scripts/lib/resolve-node-bin.mjs @@ -69,17 +69,48 @@ export function resolveNodeBin(pkg, binName, fromDir) { * installed and perfectly spawnable — a false "not installed" that would send * someone to `npm install` for a package already on disk. `resolve.paths()` * gives the search dirs without consulting `exports` at all. + * + * Its list is then filtered to the `node_modules` directories on `fromDir`'s + * own ancestor chain, because `resolve.paths()` also appends Node's GLOBAL + * FOLDERS (`$HOME/.node_modules`, `$HOME/.node_libraries`, `$PREFIX/lib/node`) + * and any `NODE_PATH` entries. Accepting those would quietly break the + * guarantee this module exists to keep — that the spawned tsc/vite is the + * REPO-PINNED one, exactly as `npx --no-install` promised. A missing repo + * install would then find a globally installed TypeScript and measure the + * programs with the wrong compiler, instead of failing with the actionable + * "run npm install" this helper is supposed to produce. */ function resolveManifest(pkg, fromDir) { const require = createRequire(path.join(fromDir, "package.json")); // `resolve.paths` returns null only for builtins, which have no bin to find. - for (const dir of require.resolve.paths(pkg) ?? []) { + for (const dir of localSearchDirs( + require.resolve.paths(pkg) ?? [], + fromDir, + )) { const candidate = path.join(dir, ...pkg.split("/"), "package.json"); if (existsSync(candidate)) return candidate; } throw new Error(`cannot find ${pkg} from ${fromDir}`); } +/** + * The subset of `resolve.paths()` that is a `node_modules` directory sitting + * directly under `fromDir` or one of its ancestors — i.e. the local walk, with + * Node's global folders and `NODE_PATH` dropped. Exported for its own test. + */ +export function localSearchDirs(dirs, fromDir) { + const ancestors = new Set(); + for (let dir = path.resolve(fromDir); ; dir = path.dirname(dir)) { + ancestors.add(dir); + if (dir === path.dirname(dir)) break; + } + return dirs.filter( + (dir) => + path.basename(dir) === "node_modules" && + ancestors.has(path.dirname(path.resolve(dir))), + ); +} + /** `@scope/name` → `name`; an unscoped name is returned unchanged. */ function unscopedName(name) { return name.startsWith("@") ? name.slice(name.indexOf("/") + 1) : name; diff --git a/scripts/lib/resolve-node-bin.test.mjs b/scripts/lib/resolve-node-bin.test.mjs index e62a72ed..febcc749 100644 --- a/scripts/lib/resolve-node-bin.test.mjs +++ b/scripts/lib/resolve-node-bin.test.mjs @@ -17,7 +17,7 @@ import { import { tmpdir } from "node:os"; import path from "node:path"; import { fileURLToPath } from "node:url"; -import { resolveNodeBin } from "./resolve-node-bin.mjs"; +import { localSearchDirs, resolveNodeBin } from "./resolve-node-bin.mjs"; const repoRoot = path.resolve( path.dirname(fileURLToPath(import.meta.url)), @@ -148,6 +148,53 @@ test("resolves a package whose `exports` hides ./package.json", () => { } }); +test("localSearchDirs keeps only node_modules on fromDir's ancestor chain", () => { + // `require.resolve.paths()` appends Node's GLOBAL FOLDERS and any NODE_PATH + // entries. Honouring those would break the guarantee this module exists for + // — that the spawned tsc/vite is the REPO-PINNED one — by letting a globally + // installed TypeScript stand in for a missing repo install, measuring the + // programs with the wrong compiler instead of failing actionably. + const from = path.join(path.sep, "repo", "clients", "web"); + const kept = [ + path.join(from, "node_modules"), + path.join(path.sep, "repo", "clients", "node_modules"), + path.join(path.sep, "repo", "node_modules"), + path.join(path.sep, "node_modules"), + ]; + const dropped = [ + path.join(path.sep, "home", "dev", ".node_modules"), + path.join(path.sep, "home", "dev", ".node_libraries"), + path.join(path.sep, "usr", "local", "lib", "node"), + // A NODE_PATH entry off the chain — a real node_modules, just not ours. + path.join(path.sep, "opt", "shared", "node_modules"), + ]; + assert.deepEqual(localSearchDirs([...kept, ...dropped], from), kept); +}); + +test("a package visible only outside the ancestor chain is not resolved", () => { + // The end-to-end form of the above: `ghostpkg` exists on disk, but under a + // sibling tree rather than an ancestor of `fromDir`. + const outside = fixtureDir( + { name: "elsewherepkg", bin: { elsewhere: "bin/e.js" } }, + { createBinFile: true }, + ); + const consumer = mkdtempSync(path.join(tmpdir(), "resolve-node-bin-from-")); + try { + writeFileSync( + path.join(consumer, "package.json"), + JSON.stringify({ name: "consumer" }), + "utf8", + ); + assert.throws( + () => resolveNodeBin("elsewherepkg", "elsewhere", consumer), + /cannot find elsewherepkg/, + ); + } finally { + rmSync(outside, { recursive: true, force: true }); + rmSync(consumer, { recursive: true, force: true }); + } +}); + test("accepts a scoped package's string-form bin under its unscoped name", () => { const dir = fixtureDir( { name: "@scope/tool", bin: "bin/tool.js" }, diff --git a/scripts/lib/win-shell-args.mjs b/scripts/lib/win-shell-args.mjs new file mode 100644 index 00000000..2e653bdf --- /dev/null +++ b/scripts/lib/win-shell-args.mjs @@ -0,0 +1,58 @@ +// Quoting for the few spawns that still need a Windows shell (#1939). +// +// Most call sites avoid the shell entirely — `resolve-node-bin.mjs` finds a +// package's JS entry and runs it with `process.execPath`. That is not available +// for `npm`/`npx` themselves: they are not resolvable from `node_modules`, and +// locating npm's own CLI relative to `process.execPath` differs across system, +// nvm, and volta installs — trading a quoting bug for a resolution bug. So the +// `npm` calls keep `shell: true`, which on Windows is the ONLY way to start the +// `.cmd` shim at all (Node has refused shell-free `.cmd`/`.bat` spawns since the +// CVE-2024-27980 hardening). +// +// With a shell, Node hands `cmd.exe` one space-joined string rather than an argv +// array, so any argument holding a space or a `cmd.exe` metacharacter is +// re-parsed as syntax. That is not hypothetical for these callers: every path +// they pass is generated under `tmpdir()`, which on Windows sits beneath the +// user profile — `C:\Users\First Last\AppData\Local\Temp` splits in two, and a +// profile name like `C:\Temp(1)` is read as grouping syntax. + +/** + * `cmd.exe` characters that make an argument parse as syntax rather than text. + * Deliberately broad — parentheses group, `^` escapes, `!` expands under + * delayed expansion, and a bare space splits. Anything not plainly inert is + * quoted; over-quoting is free, since `cmd.exe` and the CRT argument parser + * behind it both strip the quotes back off before the child sees them. + */ +const NEEDS_QUOTING = /[\s&|<>^()!"'`,;=[\]]/; + +/** + * A single argument, safe to pass through `cmd.exe`. + * + * Throws on `%`, which quoting genuinely cannot handle: `cmd.exe` expands + * `%NAME%` *before* it processes quotes, so `"%TEMP%"` is rewritten inside the + * quotes and the child receives a different path than we passed. There is no + * escape for it outside a batch file. A `%` in one of these generated temp + * paths is far-fetched, but silently running against the wrong directory is the + * exact class of failure this work exists to remove — so say so instead. + */ +export function quoteWinArg(arg) { + if (arg.includes("%")) { + throw new Error( + `cannot safely pass an argument containing "%" through cmd.exe (it is ` + + `expanded before quotes are processed): ${arg}`, + ); + } + if (!NEEDS_QUOTING.test(arg)) return arg; + // A literal `"` inside a quoted string is doubled, per the CRT parser. + return `"${arg.replace(/"/g, '""')}"`; +} + +/** + * `args`, quoted for `cmd.exe` when spawning with `shell: true` on Windows and + * returned untouched everywhere else — POSIX shells are not in play here, since + * these call sites only set `shell` on win32. + */ +export function winShellArgs(args, platform = process.platform) { + if (platform !== "win32") return args; + return args.map(quoteWinArg); +} diff --git a/scripts/lib/win-shell-args.test.mjs b/scripts/lib/win-shell-args.test.mjs new file mode 100644 index 00000000..7a1a40fa --- /dev/null +++ b/scripts/lib/win-shell-args.test.mjs @@ -0,0 +1,54 @@ +// Tests for `win-shell-args.mjs` (#1939). The platform is a parameter rather +// than a read of `process.platform`, so the Windows behaviour is exercised on +// the Linux/macOS machines that actually run this suite — otherwise every +// assertion below would be skipped exactly where the bug lives. + +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { quoteWinArg, winShellArgs } from "./win-shell-args.mjs"; + +test("passes args through untouched off Windows", () => { + const args = ["pack", "--pack-destination", "/tmp/dir with spaces"]; + assert.deepEqual(winShellArgs(args, "linux"), args); + assert.deepEqual(winShellArgs(args, "darwin"), args); +}); + +test("quotes a path with a space (the user-profile tmpdir case)", () => { + assert.equal( + quoteWinArg("C:\\Users\\First Last\\AppData\\Local\\Temp"), + '"C:\\Users\\First Last\\AppData\\Local\\Temp"', + ); +}); + +test("quotes cmd.exe metacharacters, parentheses included", () => { + // Parentheses group in cmd.exe, so `C:\Temp(1)` is syntax, not a path — the + // case a space-only predicate misses. + for (const meta of ["(", ")", "&", "|", "<", ">", "^", "!", ";", ",", "="]) { + const arg = `C:\\Temp${meta}1`; + assert.equal(quoteWinArg(arg), `"${arg}"`, `unquoted: ${arg}`); + } +}); + +test("leaves an inert argument unquoted", () => { + for (const arg of ["pack", "--json", "C:\\Users\\dev\\AppData\\Temp"]) { + assert.equal(quoteWinArg(arg), arg); + } +}); + +test("doubles an embedded quote", () => { + assert.equal(quoteWinArg('a"b c'), '"a""b c"'); +}); + +test("throws on `%` rather than silently mis-expanding it", () => { + // cmd.exe expands %NAME% before it processes quotes, so quoting cannot save + // this one — the child would receive a different path than we passed. + assert.throws(() => quoteWinArg("C:\\%TEMP%\\pack"), /cannot safely pass/); + assert.throws(() => winShellArgs(["ok", "%X%"], "win32"), /cannot safely/); +}); + +test("winShellArgs maps every element on win32", () => { + assert.deepEqual( + winShellArgs(["pack", "--pack-destination", "C:\\a b\\c"], "win32"), + ["pack", "--pack-destination", '"C:\\a b\\c"'], + ); +}); diff --git a/scripts/pack-and-verify.mjs b/scripts/pack-and-verify.mjs index 7a0deedf..1a1f8f2a 100644 --- a/scripts/pack-and-verify.mjs +++ b/scripts/pack-and-verify.mjs @@ -44,6 +44,7 @@ import { tmpdir } from "node:os"; import { join, resolve } from "node:path"; import { setTimeout as delay } from "node:timers/promises"; import { hasExited, removeSafe, stopChild } from "./lib/child-cleanup.mjs"; +import { winShellArgs } from "./lib/win-shell-args.mjs"; const repoRoot = resolve(import.meta.dirname, ".."); const testServer = join( @@ -98,21 +99,13 @@ function step(message) { console.log(`\npack:verify — ${message}`); } -// Windows needs `shell: true` to start the `npm`/`npx` `.cmd` shims at all -// (#1939), and with a shell Node hands the argv to `cmd.exe` as ONE -// space-joined string — so an argument holding a path with a space is split -// into two. That is not hypothetical here: every generated path in this file -// lives under `tmpdir()`, which on Windows sits beneath the user profile and -// so contains a space whenever the account name does. Quote each argument -// that needs it; `cmd.exe` (and the CRT parser behind it) strips the quotes -// back off, so the child sees exactly the argument we passed. +// This script keeps `shell: true` on Windows because its children are `npm` and +// `npx` — the two commands `resolve-node-bin.mjs` deliberately cannot replace +// (see the note at the top of `lib/win-shell-args.mjs`) — plus the installed +// `.bin` shim. The shell then re-parses the argv, so every generated path we +// pass has to be quoted for `cmd.exe`. const WIN_SHELL = process.platform === "win32"; -function shellArgs(args) { - if (!WIN_SHELL) return args; - return args.map((arg) => - /[\s&|<>^"]/.test(arg) ? `"${arg.replace(/"/g, '""')}"` : arg, - ); -} +const shellArgs = (args) => winShellArgs(args); /** Run a command to completion, inheriting stdio. Returns the exit status. */ function runInherit(command, args, cwd = repoRoot) {