chore: address Copilot review round 7 (Windows .cmd shims)
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 <cliff@futurescale.com>
This commit is contained in:
parent
c3c8a33e2c
commit
09b8f0e834
1 changed files with 233 additions and 45 deletions
62
scripts/lib/claude-cli.mjs
Normal file
62
scripts/lib/claude-cli.mjs
Normal file
|
|
@ -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 ?? "");
|
||||
}
|
||||
Loading…
Reference in a new issue