fix(windows): route .cmd shims through cmd.exe in dev and perf runners - #391
Conversation
resolveExecutableForPlatform turns `npm` into `npm.cmd` and `node_modules/.bin/vitest` into `vitest.cmd`, and three call sites handed that straight to spawnSync/execFileSync: bin/dev-cli.js `agentsys-dev test` lib/perf/benchmark-runner.js runBenchmark lib/perf/profiling-runner.js runProfiling Node's src has disallowed direct .bat/.cmd spawning since the CVE-2024-27980 fix (18.20.2 / 20.12.2 / 21.7.3), so all three fail with EINVAL on Windows - the same defect just fixed for the Claude plugin CLI in #390. Extract that fix into a shared planShimSpawn/shimSpawnOptions pair in lib/utils/command-parser.js and use it from all four sites, bin/cli.js included, so the two mechanisms cannot drift apart. These three sites differ from the Claude ones: they carry user-written commands, so rejecting whitespace and metacharacters would break legitimate benchmark commands. Arguments are quoted per token for cmd.exe instead, with trailing backslashes doubled so the closing quote survives the child's argv reparse. A literal " or % is still refused - % is expanded even inside quotes and a " ends the quoting, so neither can be carried faithfully. bin/cli.js keeps its stricter pre-check. The command line is built here rather than delegated to `shell: true`, which concatenates arguments unquoted (Node warns as DEP0190) - that concatenation is the injection CVE-2024-27980 was about.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CodeQL flagged two alerts in the new shim planner on #391. quoteForCmd matched trailing backslashes with /(\\+)$/, which retries the greedy run from every start position and so is quadratic in the length of an all-backslash argument (js/polynomial-redos, high). Count the run instead - linear, and the output is unchanged. Separately, while re-reading the guard #390 added: CMD_SAFE_ARG ended in $, and in JavaScript $ also matches before a trailing newline. So 'core@agentsys\n' passed as safe, and 'a\n&calc' with it. cmd.exe ends its command line at the newline, so the tail was dropped rather than checked - not an execution path, but the guard was not enforcing what it claimed. Anchor with (?![\s\S]), and refuse CR and LF in planShimSpawn for the same reason: neither survives the cmd.exe hop. The remaining alert, js/shell-command-constructed-from-input, is the cmd.exe command line this planner exists to build; the quoting and the "/%/newline refusals are the mitigation.
![revuto-review[bot]](https://avatars.githubusercontent.com/u/55848801?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is an auto review done by revuto.
Reviewed the shared planShimSpawn/shimSpawnOptions extraction and the four call sites. The cmd.exe payload shape, windowsVerbatimArguments plumbing, per-token quoting and trailing-backslash doubling all look right, and the CHANGELOG entry is present. Two gaps in planShimSpawn worth considering, both in the newly added code.
| for (const arg of args) { | ||
| if (typeof arg !== 'string') { | ||
| throw new Error('Spawn argument must be a string'); | ||
| } | ||
| if (arg.includes('\0')) { | ||
| throw new Error('Spawn argument contains invalid null byte'); | ||
| } | ||
| if (arg.includes('"') || arg.includes('%')) { | ||
| throw new Error( | ||
| `Cannot run ${executable} with argument ${JSON.stringify(arg)}: ` + | ||
| 'a literal " or % is not representable through cmd.exe' | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| const command = [executable, ...args].map(quoteForCmd).join(' '); |
There was a problem hiding this comment.
This is an auto review done by revuto.
The "/% refusal is applied to every element of args, but not to executable, even though the executable is interpolated into the same cmd.exe command line one line below ([executable, ...args].map(quoteForCmd)). assertValidToken only rejects non-strings/empty/null-byte.
At the two new call sites the executable comes from a user-written command string (parseCommand(command).executable in benchmark-runner.js:59-60 and profiling-runner.js:34-35), so e.g. a benchmark command rooted at C:\build%TEMP%\.bin\vitest.cmd — or any path containing % — is quoted and handed to cmd.exe, which expands it before the shim path is resolved. That is exactly the "silently mangling" outcome the argument check exists to prevent (the doc comment on line 72-73 states the invariant: "A literal " or % cannot be carried across cmd.exe faithfully ... so those are refused rather than silently mangled"). Hoisting the "/% check to cover executable as well would make the guarantee uniform.
| function planShimSpawn(executable, args = [], options = {}) { | ||
| assertValidToken(executable, 'Executable'); | ||
|
|
||
| if (!/\.(cmd|bat)$/i.test(executable)) { |
There was a problem hiding this comment.
This is an auto review done by revuto.
The shim branch is keyed only on the extension, with no process.platform === 'win32' check — unlike its sibling resolveExecutableForPlatform above, which takes an explicit platform parameter.
Since benchmark-runner.js / profiling-runner.js pass a user-written executable straight through, a command whose executable happens to end in .cmd/.bat on Linux/macOS (a repo-local script, or a profiler buildCommand result) now gets rewritten to cmd.exe /d /s /c ... and fails with ENOENT where it previously spawned directly. Gating the rewrite on the platform (or accepting a platform option like resolveExecutableForPlatform does, so the tests can still exercise the win32 path off-Windows) would keep the change Windows-only, matching the PR's stated scope.
CodeQL's js/shell-command-constructed-from-input fires on the /c payload by construction - cmd.exe accepts one command-line string, so building it is unavoidable once a .cmd shim has to be launched. The per-token quoting and the "/%/newline refusals are the mitigation, which the query cannot model, so suppress it in place with the reasoning next to the code rather than dismissing it out of band.
Both from revuto review on #391. planShimSpawn keyed the rewrite on the .cmd/.bat extension alone. The perf runners pass a user-written executable straight through, so a repo-local build.cmd on Linux - spawnable as it stands - would have been rewritten to a cmd.exe that is not installed, turning a working command into ENOENT. The rewrite now requires win32, with options.platform (and a platform parameter on claudeSpawnPlan) so the win32 path stays testable off Windows. The "/%/newline refusal also covered only the arguments, not the executable, which lands on the same command line: a benchmark rooted at C:\\build%TEMP%\\.bin\\vitest.cmd would have been expanded by cmd.exe before the shim path was resolved - the silent mangling the check exists to prevent. Both now go through one assertCmdPassable.
Default-setup code scanning ignores // codeql[...] comments, so the marker only looked like it did something. Keep the reasoning next to the code and dismiss the alert in the security tab instead.
There was a problem hiding this comment.
This is an auto review done by revuto.
Reviewed the shared planShimSpawn / shimSpawnOptions extraction and all four call sites.
The core of the PR checks out. I verified independently:
quoteForCmd's rewrite from/(\\+)$/to a counted loop is behaviourally identical — exhaustive over all strings of length 0–5 from{\, a, /, ", space, %}(9,331 strings), zero divergence, and'\\'.repeat(5000)produces the expected 10,002-char output linearly. The CodeQL polynomial-ReDoS fix is correct and output-preserving.- The
CMD_UNPASSABLE_ARG = /["%\r\n]/refusal inplanShimSpawnis load-bearing: unlikebin/cli.js,planShimSpawnhas no allowlist, so'bench\ncalc'would otherwise reach the cmd.exe command line. Refusing CR/LF there is a genuine fix. - All four sites correctly thread
verbatimthroughshimSpawnOptions, andbin/cli.jsretains its stricterCMD_SAFE_ARGpre-check, so #390's guarantee is preserved. - No remaining unpatched
execFileSync/spawnSyncsite consumes aresolveExecutableForPlatformresult (grep overlib/,bin/).
One factual problem, flagged inline: the second commit's premise — that JavaScript $ matches before a trailing newline — is not true without the m flag. Per ECMAScript (Assertion :: $), when [[Multiline]] is false, $ matches only at the end of Input; that "match before one trailing newline" behaviour is Python/Perl, not JS. So the $ → (?![\s\S]) change in bin/cli.js is a no-op, and the ### Security CHANGELOG entry describes a vulnerability that never existed.
Verification:
JS /^a$/.test("a\n") -> false Python re.match(r"^a$", "a\n") -> True
JS /^a$/m.test("a\n") -> true (only with the m flag)
exhaustive fuzz, len 0..3 over 31 chars (incl. \n \r \u2028 \u2029 \u0085 \v \f \t, metachars, NUL, astral):
strings tested: 30784
divergences between /^[A-Za-z0-9@._:\\/+-]+$/ and /^[A-Za-z0-9@._:\\/+-]+(?![\s\S])/: 0
The two new "regression" cases at __tests__/cli-args.test.js:262-263 throw identically against the base regex, so they don't pin the claimed fix. Belt-and-braces, \r and \n aren't in the [A-Za-z0-9@._:\\/+-] class either, so a CR/LF anywhere in an argument — not just trailing — was already rejected at base.
The code change is harmless and arguably a readability improvement; the issue is the CHANGELOG ### Security claim and the propagated misconception in the comments.
| const CMD_SAFE_ARG = /^[A-Za-z0-9@._:\\/+-]+$/; | ||
| // Ends with (?![\s\S]) rather than $, which in JavaScript also matches before a | ||
| // trailing newline - so 'plugin\n' would otherwise pass as safe. | ||
| const CMD_SAFE_ARG = /^[A-Za-z0-9@._:\\/+-]+(?![\s\S])/; |
There was a problem hiding this comment.
This is an auto review done by revuto.
This change is a no-op, and the comment justifying it states something that isn't true of JavaScript.
In JS, $ without the m flag matches only at the end of Input — ECMAScript Assertion :: $ is defined as: if [[Multiline]] is false, the assertion succeeds only when the index equals the input length. Matching before a single trailing newline is Python/Perl semantics, not JS:
/^a$/.test('a\n') // false <- JS
/^a$/m.test('a\n') // true <- only with the m flagre.match(r'^a$', 'a\n') # True <- Python doesSo 'plugin\n' never "passed as safe" under the old $. I fuzzed the two forms exhaustively over strings of length 0–3 drawn from 31 characters (the allowlist chars plus \n \r \u2028 \u2029 \u0085 \v \f \t, space, & | < > ^ ( ) " %, NUL, and an astral pair) — 30,784 strings, zero behavioural divergence between /^[A-Za-z0-9@._:\\/+-]+$/ and /^[A-Za-z0-9@._:\\/+-]+(?![\s\S])/.
Separately, \r and \n aren't members of the [A-Za-z0-9@._:\\/+-] class, so a CR or LF anywhere in the argument — not just trailing — already failed the allowlist at base. The guard was double-covered.
No objection to keeping (?![\s\S]) if you prefer an explicit end-of-input assertion, but the comment should not assert the JS $ behaviour it describes. Suggest something like: // Explicit end-of-input assertion; equivalent to $ here since the pattern is not /m.
| ### Security | ||
|
|
||
| - Claude plugin marketplace/install/update/uninstall in `bin/cli.js` now spawn via `execFileSync` with an argv array, so no plugin ID reaches a shell (#388). | ||
| - The cmd.exe argument guard in `bin/cli.js` ended in `$`, which in JavaScript also matches before a trailing newline - so an id like `core@agentsys\n` passed as safe, and whatever followed the newline was dropped by cmd.exe rather than checked. It now asserts end of input. `planShimSpawn` refuses CR and LF in an argument for the same reason. |
There was a problem hiding this comment.
This is an auto review done by revuto.
This ### Security entry documents a vulnerability that did not exist, and states an incorrect fact about JavaScript regex semantics.
ended in
$, which in JavaScript also matches before a trailing newline - so an id likecore@agentsys\npassed as safe
Without the m flag, JS $ matches only at end of Input (ECMAScript Assertion :: $). core@agentsys\n was rejected by the base regex:
"agentsys-core@agentsys\n" /^[A-Za-z0-9@._:\\/+-]+$/ deems safe? false
"a\n&calc" /^[A-Za-z0-9@._:\\/+-]+$/ deems safe? false
A published ### Security note asserting a bypass that was never reachable is worth correcting before release — it pollutes downstream security triage, and readers will take the JS-$ claim at face value.
The second half of the sentence is fine and worth keeping: planShimSpawn genuinely does need the CR/LF refusal, because unlike bin/cli.js it has no allowlist and would otherwise let 'bench\ncalc' through to the cmd.exe command line. Suggest demoting this to ### Fixed, scoping it to the planShimSpawn CR/LF guard plus the quoteForCmd ReDoS fix (which is real — CodeQL alert 111), and dropping the bin/cli.js bypass claim.
| expect(() => claudeSpawnPlan('claude.cmd', ['plugin', 'install', 'a"b'])).toThrow(/Refusing to pass/); | ||
| // JavaScript's $ also matches before a trailing newline, so the guard has to | ||
| // assert end of input or 'id\n' - and 'id\n&calc' - would pass as safe. | ||
| expect(() => claudeSpawnPlan('claude.cmd', ['plugin', 'install', 'agentsys-core@agentsys\n'])).toThrow(/Refusing to pass/); |
There was a problem hiding this comment.
This is an auto review done by revuto.
These two cases don't pin the behaviour the comment describes — they pass identically against the pre-PR regex, so they'd stay green if (?![\s\S]) were reverted to $:
BASE /^[A-Za-z0-9@._:\\/+-]+$/ on "agentsys-core@agentsys\n" -> no match -> claudeSpawnPlan throws
BASE /^[A-Za-z0-9@._:\\/+-]+$/ on "a\n&calc" -> no match -> claudeSpawnPlan throws
The assertions themselves are good additions — CR/LF in a plugin id should be refused, and locking that down is worthwhile. It's the comment on lines 260-261 that's wrong: JS $ does not match before a trailing newline unless the regex carries m. Since the comment explains why the test exists, a future reader will conclude the anchor style is load-bearing when it isn't. Recommend rewording to just state the invariant ("a plugin id containing CR or LF is refused") without the $ rationale.
revuto is right: in JavaScript $ without the m flag matches only at the
end of input - matching before a trailing newline is Python/Perl. Checked
directly: /^a$/.test('a\n') is false, /^a$/m.test('a\n') is true, and
the base CMD_SAFE_ARG rejects 'core@agentsys\n' and 'a\n&calc' already.
CR and LF are not in its character class either, so they were refused
anywhere in an argument, not just trailing.
So the (?![\s\S]) anchor in commit 1808971 fixed nothing. Revert it to $,
drop the CHANGELOG Security entry that described a bypass which never
existed, and reword the test comment to state the invariant it actually
pins - a plugin id carrying CR or LF is refused.
The CR/LF refusal in planShimSpawn stays: it has no allowlist, so
'bench\ncalc' would otherwise reach the cmd.exe command line. The
quoteForCmd ReDoS fix in the same commit also stays - that one was real
(CodeQL 111, now fixed).
There was a problem hiding this comment.
This is an auto review done by revuto.
Reviewed the shared planShimSpawn/shimSpawnOptions extraction and the four call sites at the PR head. The win32 gate, the executable-side "/%/CR/LF refusal, and the non-backtracking trailing-backslash doubling all check out, and the payload shape (cmd.exe /d /s /c "…" + windowsVerbatimArguments) matches what Node builds for a shell command. One gap outside the diff plus one inline note:
A fourth site still hands a .cmd to execFileSync. scripts/bump-version.js:63-67:
const npmCommand = process.platform === 'win32' ? 'npm.cmd' : 'npm';
execFileSync(npmCommand, ['version', newVersion, '--no-git-tag-version'], { … });This is the exact defect the PR describes — a hardcoded npm.cmd passed straight to execFileSync, which fails with EINVAL since the CVE-2024-27980 fix. It is reachable from agentsys-dev bump (bin/dev-cli.js:202), so agentsys-dev bump <version> is broken on Windows in the same way agentsys-dev test was. The PR body enumerates three sites and the CHANGELOG entry says bin/cli.js uses the shared helper "so the two mechanisms cannot drift apart"; leaving bump-version.js on its own hardcoded shim reintroduces exactly that drift. planShimSpawn('npm', args, {}) after resolveExecutableForPlatform('npm') would cover it, and __tests__/bump-version.test.js already mocks child_process so the assertion is cheap.
Note that the earlier review comments on the $-vs-(?![\s\S]) rationale (bin/cli.js:77, CHANGELOG.md:15, __tests__/cli-args.test.js:262) still apply at this head — JS $ without /m matches only at end of input, so the ### Security entry documents a bypass that was not reachable. Not repeating them inline.
| const npmExecutable = resolveExecutableForPlatform('npm'); | ||
| const result = spawnSync(npmExecutable, cmdArgs, { | ||
| // npm resolves to npm.cmd on Windows, which cannot be spawned directly. | ||
| const plan = planShimSpawn(npmExecutable, cmdArgs); |
There was a problem hiding this comment.
This is an auto review done by revuto.
planShimSpawn can now throw from inside this handler — cmdArgs includes everything after agentsys-dev test --, and on win32 an argument containing % or " hits assertCmdPassable (lib/utils/command-parser.js:129). The surrounding catch (err) { return err.status || 1; } (line 299-301) discards the message, so agentsys-dev test -- --grep=%foo% on Windows exits 1 with no output at all, where previously spawnSync ran and reported the child's own failure.
Since this call site was explicitly chosen to quote rather than reject because it carries user-written arguments, the refusal path is user-reachable here and deserves the error text printed (e.g. console.error(err.message) before returning) rather than a bare exit code.
revuto found a fourth site: scripts/bump-version.js hardcoded npm.cmd on
win32 and handed it to execFileSync, so `agentsys-dev bump <version>` was
broken there in the same EINVAL way `agentsys-dev test` was. It goes
through planShimSpawn now, which is the point of having one planner.
Sweeping the rest of scripts/, lib/ and bin/ for the same shape turned up
one more: probeCLI in lib/sources/custom-handler.js. Its tool names cannot
carry a .cmd suffix (isValidToolName rejects dots), but execFileSync
applies no PATHEXT, so a custom source naming npx or pnpm was reported
unavailable on Windows even when installed. It now resolves the shim and
routes it. Everything else spawns git, gh, bash or a downloaded binary -
all real executables on Windows.
Also print the error `agentsys-dev test` used to swallow: planShimSpawn
can refuse an argument passed after `--`, and `catch { return err.status
|| 1 }` turned that into a silent exit 1.
There was a problem hiding this comment.
This is an auto review done by revuto.
Reviewed the shared planShimSpawn / shimSpawnOptions extraction and all six call sites at the PR head.
The mechanism checks out: the win32 gate (command-parser.js:116) keeps a repo-local build.cmd spawnable off Windows, /d /s /c + the ""exe" "arg"" payload matches the shape Node builds for a shell command and is paired with windowsVerbatimArguments at every site (bin/cli.js:145, bin/dev-cli.js:289, scripts/bump-version.js:71, lib/perf/benchmark-runner.js:79, lib/perf/profiling-runner.js:53, lib/sources/custom-handler.js:111), the trailing-backslash run is counted instead of regex-matched, and bin/cli.js keeps its stricter CMD_SAFE_ARG allowlist ahead of the shared planner so #390's guarantee is unchanged. planShimSpawn throwing inside each try means the perf runners still surface a refusal through their existing error wrapper rather than crashing.
One test-portability issue below.
| test('spawns npm directly off Windows', () => { | ||
| const { execFileSync } = require('child_process'); | ||
| expect(main(['3.7.3'])).toBe(0); | ||
| expect(execFileSync).toHaveBeenCalledWith( | ||
| 'npm', | ||
| ['version', '3.7.3', '--no-git-tag-version'], | ||
| expect.not.objectContaining({ windowsVerbatimArguments: true }) | ||
| ); |
There was a problem hiding this comment.
This is an auto review done by revuto.
This case is host-dependent and will fail when the suite runs on Windows — the very platform this PR is fixing.
Unlike its sibling below (which fakes win32 explicitly) and unlike __tests__/perf-benchmark-runner.test.js, which fakes linux for its "off Windows" case, this test leaves process.platform as the host's. On a Windows host resolveExecutableForPlatform('npm') returns npm.cmd (lib/utils/command-parser.js:45-47), so scripts/bump-version.js:69-71 plans the cmd.exe hop and execFileSync is called with 'cmd.exe', not 'npm' — the assertion on line 88 fails.
__tests__/command-parser.test.js:71-72 states the intended convention for exactly this reason ("every shim case names the platform rather than depending on the host running the suite"). Pinning the platform to 'linux' around this main(['3.7.3']) call, the same way the perf test does, makes it deterministic.
(Note tests/sources/custom-handler.test.js's "should probe a plain executable directly off Windows" is fine as written, since gh is not in WINDOWS_CMD_SHIMS and so resolves unchanged on win32 too.)
revuto is right: the case asserted execFileSync('npm', ...) while leaving
process.platform as the host's, so it would fail on Windows - where
resolveExecutableForPlatform returns npm.cmd and the planner takes the
cmd.exe branch. Pin it to linux, matching the convention the other shim
tests follow.
There was a problem hiding this comment.
This is an auto review done by revuto.
Revuto completed the review and found no evidence-backed concerns.
The new Windows CI leg surfaced ten failures in three groups. Six were my own tests from #391 asserting a bare `cmd.exe`. A real Windows host has COMSPEC set - the runner reports C:\Windows\system32\cmd.exe - so planShimSpawn returns that absolute path and the assertions only held on a host where COMSPEC was unset. The platform was already faked in those tests; comspec now is too, and a new case covers the COMSPEC fallback and the bare cmd.exe default that is left when it is unset. Three came from Git's CRLF conversion on Windows checkouts. generate-docs compares generated sections against the file on disk, so with CRLF on disk every section read as stale and `--check` exited 1; the plugins.txt parser kept a trailing \r on each name. .gitattributes pins text checkouts to LF, which is what those byte comparisons assume, and the plugins.txt parse tolerates CRLF so a checkout setting cannot look like a plugin mismatch. The last one asserted that writing to C:\Windows\System32 throws. The runner is elevated, so the write succeeded - and left a file in System32. It now writes through a parent that is a regular file, which no privilege level can turn into a directory.
* ci: run the test job on windows-latest too The Windows-specific code in this repo - where.exe executable resolution, PATHEXT-aware lookups, and the cmd.exe routing that .cmd shims need since the CVE-2024-27980 fix - has never been exercised by CI. Every regression in it so far was found by a user on Windows or by reading the code, which is why three separate PRs were needed to fix one defect class. The matrix adds windows-latest with fail-fast disabled so a Windows-only failure does not hide the Linux result (and vice versa). * fix(windows): make the suite pass on windows-latest The new Windows CI leg surfaced ten failures in three groups. Six were my own tests from #391 asserting a bare `cmd.exe`. A real Windows host has COMSPEC set - the runner reports C:\Windows\system32\cmd.exe - so planShimSpawn returns that absolute path and the assertions only held on a host where COMSPEC was unset. The platform was already faked in those tests; comspec now is too, and a new case covers the COMSPEC fallback and the bare cmd.exe default that is left when it is unset. Three came from Git's CRLF conversion on Windows checkouts. generate-docs compares generated sections against the file on disk, so with CRLF on disk every section read as stale and `--check` exited 1; the plugins.txt parser kept a trailing \r on each name. .gitattributes pins text checkouts to LF, which is what those byte comparisons assume, and the plugins.txt parse tolerates CRLF so a checkout setting cannot look like a plugin mismatch. The last one asserted that writing to C:\Windows\System32 throws. The runner is elevated, so the write succeeded - and left a file in System32. It now writes through a parent that is a regular file, which no privilege level can turn into a directory. * test: keep the plugins.txt parse strict about everything but line endings
Follow-up to #390, which fixed this defect for the Claude plugin CLI only. Four other call sites had it.
The bug
resolveExecutableForPlatformturnsnpmintonpm.cmdandnode_modules/.bin/vitestintovitest.cmd. These sites handed that result straight tospawnSync/execFileSync:bin/dev-cli.jsagentsys-dev testscripts/bump-version.jsagentsys-dev bump <version>(hardcodednpm.cmd)lib/perf/benchmark-runner.jsrunBenchmarklib/perf/profiling-runner.jsrunProfilingNode's
srchas disallowed direct.bat/.cmdspawning since the CVE-2024-27980 fix (18.20.2 / 20.12.2 / 21.7.3 - changelog:src: disallow direct .bat and .cmd file spawning). So on Windows all four fail withEINVALrather than running: every benchmark, every profile, the dev test runner, and the version bump.A fifth site failed the sibling way:
probeCLIinlib/sources/custom-handler.jspasses a bare tool name toexecFileSync, which applies no PATHEXT, so a custom source namingnpx,pnpmoryarnwas reported unavailable on Windows even when installed. Its names cannot carry a.cmdsuffix (isValidToolNamerejects dots), so it needed the shim resolution plus the same hop.Everything else in
scripts/,lib/andbin/spawnsgit,gh,bash, or a downloaded binary - all real executables on Windows - or goes throughexecSync, which already runs a shell.The fix
Extract #390's cmd.exe hop into a shared
planShimSpawn/shimSpawnOptionspair inlib/utils/command-parser.js, and use it from all six sites -bin/cli.jsincluded - so the mechanisms cannot drift apart.planShimSpawnreturns the executable untouched unless the platform is win32 and the name ends in.cmd/.bat; for a shim it returnscmd.exe /d /s /c "<quoted command>"plus averbatimflag, andshimSpawnOptionsfolds that intowindowsVerbatimArgumentsso Node does not re-quote the payload./sstrips the outer quote pair, leaving the quoted shim path as the first token - the same shape Node itself builds for a shell command.The win32 gate matters because the perf runners take a user-written executable: a repo-local
build.cmdon Linux is spawnable as it stands, and rewriting it to acmd.exethat is not installed would turn a working command intoENOENT.options.platform(and aplatformparameter onclaudeSpawnPlan) keeps the win32 path testable off Windows.The command line is built here rather than delegated to
shell: true, which concatenates arguments unquoted (Node warns about it as DEP0190). That concatenation is the injection CVE-2024-27980 was about.Why these sites quote instead of reject
#390 could reject any argument containing whitespace or a shell metacharacter, because its arguments are plugin ids the CLI generates. These carry user-written commands -
npm run bench -- --grep "a b", or a path underC:\Program Files\- so rejection would break them. Arguments are quoted per token instead:& | < > ^ ( )alone, so quoting every token is what stops reinterpretation."C:\dir\"reads as an escaped quote when the child's CRT parses the command line back into argv. Counted in a loop rather than matched with/(\\+)$/, which backtracks per start position (CodeQLjs/polynomial-redos).",%, CR or LF is refused - in the executable as well as the arguments, since both land on the same command line.%VAR%is expanded even inside quotes, a"ends the quoting, and a newline ends the command line. Refusing beats silently mangling;bench\ncalcwould otherwise reach cmd.exe.bin/cli.jskeeps its stricter allowlist, so fix: resolve Claude Code executable from where.exe on Windows #390's guarantee is unchanged.agentsys-dev testnow prints the error it used to swallow, since a refused argument passed after--reaches the user from there.Tests
__tests__/command-parser.test.js- 11 new cases: passthrough for non-shims and for.cmdoff Windows, the cmd.exe payload shape, metacharacter and empty-argument quoting, trailing-backslash doubling (including a 5000-character run), the"/%/CR/LF/null-byte/non-string refusals on both the arguments and the shim path, explicitcomspec, andshimSpawnOptionssettingwindowsVerbatimArgumentsonly for a shim.__tests__/perf-benchmark-runner.test.js,__tests__/perf-profiling-runner.test.js,__tests__/bump-version.test.js,tests/sources/custom-handler.test.js- each asserts the cmd.exe route with the platform faked to win32, and the direct spawn off Windows.__tests__/dev-cli.test.js- asserts thetesthandler cannot bypass the plan.__tests__/cli-args.test.js- fix: resolve Claude Code executable from where.exe on Windows #390's expectations updated for per-token quoting and the platform parameter, plus CR/LF refusal in a plugin id.Full gate green locally:
npm test88 suites / 3567 passed / 39 skipped,npm run validate[OK] All validators passed,expand-templates --checkandgen-adapters --checkboth exit 0.Notes
js/shell-command-constructed-from-inputon the/cpayload by construction - cmd.exe accepts exactly one command-line string, so building it is unavoidable. Dismissed as mitigated (quoting + the refusals); the reasoning is in the code next to the line.bin/cli.js's$anchor let a trailing newline through. That was wrong - JS$without/mmatches only at end of input, which revuto caught - and it is retracted in 7b22021. The CR/LF refusal inplanShimSpawnis still needed: it has no allowlist.ci.ymlhas nowindows-latestleg, and the.bat/.cmdgate is Windows-only native code, so it cannot be reproduced on Linux even with a fakedprocess.platform. The behaviour rests on Node's changelog and its April 2024 security advisory, plus unit tests over the constructed command line. Awindows-latestleg is worth adding separately.