From 09b8f0e83421e4438ae1abc881ed927b4c9addc4 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 29 Aug 2026 02:32:30 -0400 Subject: [PATCH] chore: address Copilot review round 7 (Windows .cmd shims) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Node has refused shell-free `.cmd`/`.bat` spawns since CVE-2024-27980, and an npm-installed `claude` is a `.cmd` shim on Windows — so every shell-free spawn of it reported ENOENT there, each in a quiet way: `verify:skills` skipped its authoritative hand-off, `verify:skills:cli` downloaded the pinned package it already had (and failed offline), and `skills:eval` said "not on PATH". - New `scripts/lib/claude-cli.mjs` is the one place that decides how `claude` is spawned: `shell` on win32, arguments through the existing `winShellArgs` quoting so `cmd.exe` cannot re-parse them as syntax (#1939). Unit-tested with the platform injected, since a win32-only bug no runner here executes is exactly the kind that ships. - `verify-skills`, `verify-skills-cli` and `skill-eval` all route their probes and spawns through it. - skill-eval sends the prompt on **stdin** rather than in argv. `claude -p` reads a piped prompt (verified end to end), which sidesteps `cmd.exe` metacharacter re-parsing entirely rather than relying on quoting a model-visible string, and keeps the prompt out of the process table. Signed-off-by: cliffhall --- scripts/lib/claude-cli.mjs | 62 ++++++++++++++++++++++++ scripts/lib/claude-cli.test.mjs | 76 ++++++++++++++++++++++++++++++ scripts/skill-eval.mjs | 36 +++++++++----- scripts/skill-eval.test.mjs | 47 ++++++++++++++++++ scripts/verify-skills-cli.mjs | 17 ++++--- scripts/verify-skills-cli.test.mjs | 17 +++---- scripts/verify-skills.mjs | 23 ++++----- 7 files changed, 233 insertions(+), 45 deletions(-) create mode 100644 scripts/lib/claude-cli.mjs create mode 100644 scripts/lib/claude-cli.test.mjs diff --git a/scripts/lib/claude-cli.mjs b/scripts/lib/claude-cli.mjs new file mode 100644 index 00000000..eda39761 --- /dev/null +++ b/scripts/lib/claude-cli.mjs @@ -0,0 +1,62 @@ +// Spawning the `claude` CLI from the root scripts (#2163, Copilot). +// +// On Windows an npm-installed `claude` is a `.cmd` shim, and Node has refused +// shell-free `.cmd`/`.bat` spawns since the CVE-2024-27980 hardening — so a +// plain `spawnSync("claude", …)` reports ENOENT there even though the CLI is +// installed. Every consumer of that would then be wrong in a *quiet* way: +// `verify:skills` would skip its authoritative hand-off, `verify:skills:cli` +// would download the pinned package it already has (and fail offline), and +// `skills:eval` would report "not on PATH". +// +// The shell that fixes it introduces the #1939 problem — `cmd.exe` re-parses a +// space-joined string, so any argument holding a metacharacter becomes syntax — +// so arguments go through the same `winShellArgs` quoting the npm/npx call +// sites use. This is deliberately the ONLY place that decides either question. + +import { spawnSync } from "node:child_process"; +import { winShellArgs } from "./win-shell-args.mjs"; + +/** + * `spawnSync` arguments for a `claude` invocation, correct on every platform. + * + * @param {string[]} args + * @param {object} [options] Passed through to `spawnSync`. + * @param {string} [platform] Defaults to the current platform; injectable for tests. + * @returns {{ command: string, args: string[], options: object }} + */ +export function claudeSpawnArgs( + args, + options = {}, + platform = process.platform, +) { + return { + command: "claude", + args: winShellArgs(args, platform), + options: { ...options, shell: platform === "win32" }, + }; +} + +/** + * Read the installed CLI's version, or null when there is no usable one. + * + * "Usable" deliberately includes parsing the output: a `claude` that answers + * `--version` with something unrecognizable cannot be trusted to be the pinned + * one either, and the callers all treat null as "use the pinned package". + * + * @param {(text: string) => number[] | null} parseVersion + * @param {{ spawn?: typeof spawnSync, platform?: string }} [io] + * @returns {number[] | null} + */ +export function probeClaudeVersion( + parseVersion, + { spawn = spawnSync, platform } = {}, +) { + const { command, args, options } = claudeSpawnArgs( + ["--version"], + { encoding: "utf8" }, + platform, + ); + const res = spawn(command, args, options); + if (res.error || res.status !== 0) return null; + return parseVersion(res.stdout ?? ""); +} diff --git a/scripts/lib/claude-cli.test.mjs b/scripts/lib/claude-cli.test.mjs new file mode 100644 index 00000000..066fa58c --- /dev/null +++ b/scripts/lib/claude-cli.test.mjs @@ -0,0 +1,76 @@ +// Tests for the one place that decides how `claude` is spawned (#2163, Copilot). +// +// The behavior under test only differs ON Windows, and these run on whatever +// the contributor has — so the platform is injected rather than sniffed. A +// win32-only bug that no CI runner here executes is precisely the kind that +// ships. + +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { claudeSpawnArgs, probeClaudeVersion } from "./claude-cli.mjs"; + +test("spawns without a shell off Windows", () => { + const { command, args, options } = claudeSpawnArgs( + ["plugin", "validate", ".claude/skills"], + { cwd: "/repo" }, + "linux", + ); + assert.equal(command, "claude"); + assert.deepEqual(args, ["plugin", "validate", ".claude/skills"]); + assert.equal(options.shell, false); + assert.equal(options.cwd, "/repo"); +}); + +test("uses a shell on Windows, where the CLI is a .cmd shim", () => { + // Node has refused shell-free `.cmd` spawns since CVE-2024-27980, so without + // this the CLI reads as absent even when it is installed. + const { options } = claudeSpawnArgs(["--version"], {}, "win32"); + assert.equal(options.shell, true); +}); + +test("quotes arguments that cmd.exe would otherwise re-parse", () => { + const { args } = claudeSpawnArgs(["-p", "does it? (yes & no)"], {}, "win32"); + assert.deepEqual(args, ["-p", '"does it? (yes & no)"']); + // The same arguments are untouched off Windows. + assert.deepEqual( + claudeSpawnArgs(["-p", "does it? (yes & no)"], {}, "linux").args, + ["-p", "does it? (yes & no)"], + ); +}); + +test("probeClaudeVersion returns the parsed version on success", () => { + const version = probeClaudeVersion( + (t) => (t.trim() === "2.1.250" ? [2, 1, 250] : null), + { + spawn: () => ({ status: 0, stdout: "2.1.250" }), + platform: "linux", + }, + ); + assert.deepEqual(version, [2, 1, 250]); +}); + +test("probeClaudeVersion returns null for every unusable outcome", () => { + for (const res of [ + { error: new Error("ENOENT") }, + { status: 1, stdout: "" }, + { status: 0, stdout: "something else" }, + ]) { + assert.equal( + probeClaudeVersion(() => null, { spawn: () => res, platform: "linux" }), + null, + ); + } +}); + +test("probeClaudeVersion asks for the shell on Windows", () => { + let seen; + probeClaudeVersion(() => [2, 1, 250], { + spawn: (_c, _a, options) => { + seen = options; + return { status: 0, stdout: "2.1.250" }; + }, + platform: "win32", + }); + assert.equal(seen.shell, true); + assert.equal(seen.encoding, "utf8"); +}); diff --git a/scripts/skill-eval.mjs b/scripts/skill-eval.mjs index 8ad3de7c..ad0b75c6 100755 --- a/scripts/skill-eval.mjs +++ b/scripts/skill-eval.mjs @@ -29,11 +29,16 @@ // npm run skills:eval -- testing # one skill's cases // RUNS=5 THRESHOLD=0.8 npm run skills:eval -import { spawn, spawnSync } from "node:child_process"; +import { spawn } from "node:child_process"; import { readFileSync, existsSync, readdirSync, statSync } from "node:fs"; import path from "node:path"; import { fileURLToPath } from "node:url"; -import { parseSkill, validateEvalCases } from "./lib/skill-manifest.mjs"; +import { claudeSpawnArgs, probeClaudeVersion } from "./lib/claude-cli.mjs"; +import { + parseClaudeVersion, + parseSkill, + validateEvalCases, +} from "./lib/skill-manifest.mjs"; const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); const SKILLS_DIR = path.join(ROOT, ".claude", "skills"); @@ -217,13 +222,20 @@ export function sampleHit(expect, invoked, ours = null) { * @param {{ spawnFn?: typeof spawn, cwd?: string }} [opts] * @returns {Promise>} */ -export function runPrompt(prompt, { spawnFn = spawn, cwd = ROOT } = {}) { +export function runPrompt( + prompt, + { spawnFn = spawn, cwd = ROOT, platform = process.platform } = {}, +) { return new Promise((resolve, reject) => { - const p = spawnFn( - "claude", + // The prompt goes in on STDIN, not in argv. `claude -p` with piped stdin + // reads the prompt from it (verified), and that removes two problems at + // once: on Windows the CLI is a `.cmd` shim, so it can only be started + // through a shell, and `cmd.exe` would re-parse any prompt containing a + // metacharacter as syntax (Copilot). It also keeps the prompt out of the + // process table. + const { command, args, options } = claudeSpawnArgs( [ "-p", - prompt, "--output-format", "stream-json", "--verbose", @@ -234,8 +246,10 @@ export function runPrompt(prompt, { spawnFn = spawn, cwd = ROOT } = {}) { "--disallowedTools", "Bash,Write,Edit,NotebookEdit", ], - { cwd, stdio: ["ignore", "pipe", "inherit"] }, + { cwd, stdio: ["pipe", "pipe", "inherit"] }, + platform, ); + const p = spawnFn(command, args, options); let buf = ""; const invoked = new Set(); @@ -255,12 +269,10 @@ export function runPrompt(prompt, { spawnFn = spawn, cwd = ROOT } = {}) { } resolve(invoked); }); + p.stdin?.end(prompt); }); } -const hit = (invoked, skill) => - [...invoked].some((payload) => payload.includes(skill)); - async function pool(items, n, fn) { const out = new Array(items.length); let i = 0; @@ -276,9 +288,9 @@ async function pool(items, n, fn) { } async function main() { - if (spawnSync("claude", ["--version"], { stdio: "ignore" }).error) { + if (probeClaudeVersion(parseClaudeVersion) === null) { console.error( - "skills:eval — the `claude` CLI is not on PATH. This eval needs it.", + "skills:eval — no usable `claude` CLI on PATH. This eval needs one.", ); process.exit(1); } diff --git a/scripts/skill-eval.test.mjs b/scripts/skill-eval.test.mjs index 9933b939..5acb710b 100644 --- a/scripts/skill-eval.test.mjs +++ b/scripts/skill-eval.test.mjs @@ -105,6 +105,9 @@ function fakeSpawn({ chunks = [], code = 0, error = null }) { return () => { const child = new EventEmitter(); child.stdout = new EventEmitter(); + // The prompt is written to stdin rather than passed in argv, so the + // stand-in needs one or `runPrompt` throws before the child can close. + child.stdin = { end: () => {} }; queueMicrotask(() => { if (error) { child.emit("error", error); @@ -215,3 +218,47 @@ test("a negative case ignores skills that are not this repo's", () => { // With no repo set, any invocation still fails a negative case. assert.equal(sampleHit(null, foreign), false); }); + +test("runPrompt sends the prompt on stdin, never in argv", () => { + // On Windows the CLI is a `.cmd` shim and can only start through a shell, + // where `cmd.exe` re-parses any metacharacter in an argument as syntax. Off + // Windows it also keeps the prompt out of the process table. + const prompt = "does it? (yes & no)"; + let seen; + let written = null; + runPrompt(prompt, { + spawnFn: (command, args, options) => { + seen = { command, args, options }; + const c = new EventEmitter(); + c.stdout = new EventEmitter(); + c.stdin = { + end: (text) => { + written = text; + }, + }; + queueMicrotask(() => c.emit("close", 0)); + return c; + }, + platform: "linux", + }).catch(() => {}); + assert.equal(seen.command, "claude"); + assert.ok(!seen.args.includes(prompt), "prompt must not appear in argv"); + assert.deepEqual(seen.options.stdio, ["pipe", "pipe", "inherit"]); + assert.equal(written, prompt); +}); + +test("runPrompt asks for a shell on Windows", () => { + let seen; + runPrompt("p", { + spawnFn: (_c, _a, options) => { + seen = options; + const c = new EventEmitter(); + c.stdout = new EventEmitter(); + c.stdin = { end: () => {} }; + queueMicrotask(() => c.emit("close", 0)); + return c; + }, + platform: "win32", + }).catch(() => {}); + assert.equal(seen.shell, true); +}); diff --git a/scripts/verify-skills-cli.mjs b/scripts/verify-skills-cli.mjs index d01f86e0..7bfb85cf 100644 --- a/scripts/verify-skills-cli.mjs +++ b/scripts/verify-skills-cli.mjs @@ -22,6 +22,8 @@ import { spawnSync } from "node:child_process"; import path from "node:path"; import { fileURLToPath } from "node:url"; +import { probeClaudeVersion } from "./lib/claude-cli.mjs"; +import { winShellArgs } from "./lib/win-shell-args.mjs"; import { compareVersions, parseClaudeVersion, @@ -83,32 +85,29 @@ export function validatorCommand({ version }) { * propagation of a rejected validator would leave `test:scripts` green while * this gate quietly stopped gating (Copilot). * - * @param {{ probe?: () => {error?: Error, status?: number|null, stdout?: string}, + * @param {{ probe?: () => number[] | null, * spawn?: (cmd: string, args: string[], opts: object) => {error?: Error, status?: number|null}, * log?: (msg: string) => void, error?: (msg: string) => void }} [io] * @returns {number} Process exit code. */ export function runValidator(io = {}) { const { - probe = () => spawnSync("claude", ["--version"], { encoding: "utf8" }), + probe = () => probeClaudeVersion(parseClaudeVersion), spawn = (cmd, args, opts) => spawnSync(cmd, args, opts), log = console.log, error = console.error, + platform = process.platform, } = io; - const probed = probe(); - const version = - probed.error || probed.status !== 0 - ? null - : parseClaudeVersion(probed.stdout ?? ""); + const version = probe(); const { command, args, via } = validatorCommand({ version }); log(`verify:skills:cli — validating ${SKILLS_DIR} via ${via}…`); - const res = spawn(command, args, { + const res = spawn(command, winShellArgs(args, platform), { cwd: ROOT, stdio: "inherit", - shell: process.platform === "win32", + shell: platform === "win32", }); if (res.error) { error( diff --git a/scripts/verify-skills-cli.test.mjs b/scripts/verify-skills-cli.test.mjs index ba48caef..816a8196 100644 --- a/scripts/verify-skills-cli.test.mjs +++ b/scripts/verify-skills-cli.test.mjs @@ -72,7 +72,7 @@ function harness({ probe, spawnResult }) { return { code, spawned, logs, errors }; } -const okProbe = { status: 0, stdout: `${PINNED_CLI_VERSION} (Claude Code)\n` }; +const okProbe = PINNED_CLI_VERSION.split(".").map(Number); test("runs the local CLI and succeeds when the validator accepts", () => { const r = harness({ probe: okProbe, spawnResult: { status: 0 } }); @@ -83,16 +83,11 @@ test("runs the local CLI and succeeds when the validator accepts", () => { }); test("treats an unusable probe as no CLI at all", () => { - // A `claude` that fails `--version` cannot be trusted to have the subcommand. - for (const probe of [ - { error: new Error("ENOENT") }, - { status: 1, stdout: "" }, - { status: 0, stdout: "not a version" }, - ]) { - const r = harness({ probe, spawnResult: { status: 0 } }); - assert.equal(r.code, 0); - assert.equal(r.spawned[0].command, "npx"); - } + // `probeClaudeVersion` already collapses ENOENT, a nonzero `--version`, and + // unparseable output into null — this asserts what runValidator does with it. + const r = harness({ probe: null, spawnResult: { status: 0 } }); + assert.equal(r.code, 0); + assert.equal(r.spawned[0].command, "npx"); }); test("fails when the validator rejects the skills", () => { diff --git a/scripts/verify-skills.mjs b/scripts/verify-skills.mjs index 3e189db4..7151975e 100755 --- a/scripts/verify-skills.mjs +++ b/scripts/verify-skills.mjs @@ -30,6 +30,7 @@ import { readFileSync, readdirSync, existsSync, statSync } from "node:fs"; import { spawnSync } from "node:child_process"; import path from "node:path"; import { fileURLToPath } from "node:url"; +import { claudeSpawnArgs, probeClaudeVersion } from "./lib/claude-cli.mjs"; import { compareVersions, parseClaudeVersion, @@ -146,29 +147,25 @@ function main(argv = process.argv.slice(2)) { // never reaching the reproducible step (Copilot). Accepting merely "new enough" // would reintroduce exactly the cross-machine schema drift the pin removes. // Anything else is left to `verify:skills:cli`, which fetches the pin. - const probe = spawnSync("claude", ["--version"], { encoding: "utf8" }); - if (probe.error || probe.status !== 0) { + const version = probeClaudeVersion(parseClaudeVersion); + if (version === null) { console.log( "verify:skills — no usable `claude` CLI here; `verify:skills:cli` runs the pinned validator.", ); return; } - const version = parseClaudeVersion(probe.stdout ?? ""); - if ( - version === null || - compareVersions(version, PINNED_VERSION_PARTS) !== 0 - ) { + if (compareVersions(version, PINNED_VERSION_PARTS) !== 0) { console.log( - `verify:skills — local \`claude\` is ${version?.join(".") ?? "(unreadable)"}, not the pinned ` + + `verify:skills — local \`claude\` is ${version.join(".")}, not the pinned ` + `${PINNED_CLI_VERSION}; \`verify:skills:cli\` runs the pinned validator.`, ); return; } - const res = spawnSync("claude", ["plugin", "validate", ".claude/skills"], { - cwd: ROOT, - stdio: "inherit", - }); - if (res.status !== 0) { + const { command, args, options } = claudeSpawnArgs( + ["plugin", "validate", ".claude/skills"], + { cwd: ROOT, stdio: "inherit" }, + ); + if (spawnSync(command, args, options).status !== 0) { console.error("\nverify:skills — `claude plugin validate` failed."); process.exit(1); }