-
Notifications
You must be signed in to change notification settings - Fork 113
fix(windows): route .cmd shims through cmd.exe in dev and perf runners #391
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
f17b136
1808971
a1fe51c
b74d12d
e8f8d2f
7b22021
d30c054
c556368
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -229,8 +229,8 @@ describe('claudeSpawnPlan', () => { | |
| const args = ['plugin', 'install', 'agentsys-core@agentsys']; | ||
|
|
||
| test('spawns a posix or native executable directly', () => { | ||
| expect(claudeSpawnPlan('claude', args)).toEqual({ file: 'claude', args }); | ||
| expect(claudeSpawnPlan('C:\\bin\\claude.exe', args)).toEqual({ file: 'C:\\bin\\claude.exe', args }); | ||
| expect(claudeSpawnPlan('claude', args)).toEqual({ file: 'claude', args, verbatim: false }); | ||
| expect(claudeSpawnPlan('C:\\bin\\claude.exe', args)).toEqual({ file: 'C:\\bin\\claude.exe', args, verbatim: false }); | ||
| }); | ||
|
|
||
| test('routes a batch shim through cmd.exe, which execFileSync cannot spawn', () => { | ||
|
|
@@ -239,7 +239,7 @@ describe('claudeSpawnPlan', () => { | |
| const shim = 'C:\\npm\\claude.cmd'; | ||
| expect(claudeSpawnPlan(shim, args)).toEqual({ | ||
| file: 'cmd.exe', | ||
| args: ['/d', '/s', '/c', '""C:\\npm\\claude.cmd" plugin install agentsys-core@agentsys"'], | ||
| args: ['/d', '/s', '/c', '""C:\\npm\\claude.cmd" "plugin" "install" "agentsys-core@agentsys""'], | ||
| verbatim: true | ||
| }); | ||
| expect(claudeSpawnPlan('C:\\npm\\claude.bat', args).file).toBe('cmd.exe'); | ||
|
|
@@ -257,6 +257,10 @@ describe('claudeSpawnPlan', () => { | |
| expect(() => claudeSpawnPlan('claude.cmd', ['plugin', 'install', 'a|b'])).toThrow(/Refusing to pass/); | ||
| expect(() => claudeSpawnPlan('claude.cmd', ['plugin', 'install', 'a b'])).toThrow(/Refusing to pass/); | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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 |
||
| expect(() => claudeSpawnPlan('claude.cmd', ['plugin', 'install', 'a\n&calc'])).toThrow(/Refusing to pass/); | ||
| }); | ||
|
|
||
| test('passes the same arguments through unchecked when no shell is involved', () => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,7 +20,7 @@ const VERSION = require('../package.json').version; | |
| const PACKAGE_DIR = path.join(__dirname, '..'); | ||
| const discovery = require('../lib/discovery'); | ||
| const transforms = require('../lib/adapter-transforms'); | ||
| const { resolveExecutableForPlatform } = require('../lib/utils/command-parser'); | ||
| const { resolveExecutableForPlatform, planShimSpawn, shimSpawnOptions } = require('../lib/utils/command-parser'); | ||
|
|
||
| // Valid tool names | ||
| const VALID_TOOLS = ['claude', 'opencode', 'codex', 'cursor', 'kiro']; | ||
|
|
@@ -72,7 +72,9 @@ const WINDOWS_DIRECT_EXEC = /\.(exe|com)$/i; | |
| // to execFileSync fails with EINVAL rather than running it. | ||
| const WINDOWS_BATCH_SHIM = /\.(cmd|bat)$/i; | ||
| // Arguments safe to hand to cmd.exe: no whitespace, no shell metacharacters. | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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, /^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 Separately, No objection to keeping |
||
|
|
||
| /** | ||
| * Pick the Claude Code executable from a `where.exe claude` result. | ||
|
|
@@ -100,23 +102,20 @@ function pickClaudeExecutable(platform, whereOutput) { | |
| /** | ||
| * Build the file and argv for one Claude Code invocation. | ||
| * | ||
| * A batch shim cannot be handed to execFileSync at all, so it is launched | ||
| * through cmd.exe. cmd.exe re-parses its command line, so every argument is | ||
| * checked first: callers only ever pass literal subcommands and validated | ||
| * `<plugin>@<marketplace>` ids, so anything else is a bug rather than a string | ||
| * to escape. The quoting mirrors how Node wraps a shell command - /s strips the | ||
| * outer quote pair, leaving the quoted shim path as the first token. | ||
| * planShimSpawn does the cmd.exe routing a batch shim needs. Every argument is | ||
| * checked against a stricter rule than that quoting requires: callers only ever | ||
| * pass literal subcommands and validated `<plugin>@<marketplace>` ids, so | ||
| * anything carrying whitespace or a shell metacharacter is a bug rather than a | ||
| * string to escape. | ||
| */ | ||
| function claudeSpawnPlan(executable, args, comspec) { | ||
| if (!WINDOWS_BATCH_SHIM.test(executable)) { | ||
| return { file: executable, args }; | ||
| } | ||
| const unsafe = args.find(arg => !CMD_SAFE_ARG.test(arg)); | ||
| if (unsafe !== undefined) { | ||
| throw new Error(`Refusing to pass ${JSON.stringify(unsafe)} to cmd.exe`); | ||
| if (WINDOWS_BATCH_SHIM.test(executable)) { | ||
| const unsafe = args.find(arg => !CMD_SAFE_ARG.test(arg)); | ||
| if (unsafe !== undefined) { | ||
| throw new Error(`Refusing to pass ${JSON.stringify(unsafe)} to cmd.exe`); | ||
| } | ||
| } | ||
| const command = [`"${executable}"`, ...args].join(' '); | ||
| return { file: comspec || 'cmd.exe', args: ['/d', '/s', '/c', `"${command}"`], verbatim: true }; | ||
| return planShimSpawn(executable, args, { comspec }); | ||
| } | ||
|
|
||
| let claudeBinCache; | ||
|
|
@@ -144,7 +143,7 @@ function claudeExecutable() { | |
| */ | ||
| function claudeSpawn(args, options) { | ||
| const plan = claudeSpawnPlan(claudeExecutable(), args, process.env.comspec); | ||
| return execFileSync(plan.file, plan.args, plan.verbatim ? { ...options, windowsVerbatimArguments: true } : options); | ||
| return execFileSync(plan.file, plan.args, shimSpawnOptions(plan, options)); | ||
| } | ||
|
|
||
| function copyDirRecursive(src, dest) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,7 +14,7 @@ | |
|
|
||
| const path = require('path'); | ||
| const { execSync, spawnSync } = require('child_process'); | ||
| const { resolveExecutableForPlatform } = require('../lib/utils/command-parser'); | ||
| const { resolveExecutableForPlatform, planShimSpawn, shimSpawnOptions } = require('../lib/utils/command-parser'); | ||
|
|
||
| const VERSION = require('../package.json').version; | ||
| const ROOT_DIR = path.join(__dirname, '..'); | ||
|
|
@@ -284,12 +284,14 @@ const COMMANDS = { | |
| cmdArgs.push(...args); | ||
| } | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an auto review done by revuto.
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. |
||
| const result = spawnSync(plan.file, plan.args, shimSpawnOptions(plan, { | ||
| cwd: ROOT_DIR, | ||
| stdio: 'inherit', | ||
| shell: false, | ||
| windowsHide: true | ||
| }); | ||
| })); | ||
| if (result.error) { | ||
| throw result.error; | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an auto review done by revuto.
This
### Securityentry documents a vulnerability that did not exist, and states an incorrect fact about JavaScript regex semantics.Without the
mflag, JS$matches only at end of Input (ECMAScriptAssertion :: $).core@agentsys\nwas rejected by the base regex:A published
### Securitynote 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:
planShimSpawngenuinely does need the CR/LF refusal, because unlikebin/cli.jsit has no allowlist and would otherwise let'bench\ncalc'through to the cmd.exe command line. Suggest demoting this to### Fixed, scoping it to theplanShimSpawnCR/LF guard plus thequoteForCmdReDoS fix (which is real — CodeQL alert 111), and dropping thebin/cli.jsbypass claim.