diff --git a/src/cli/handlers/account.test.ts b/src/cli/handlers/account.test.ts index d39a5f33293..8642df27ea4 100644 --- a/src/cli/handlers/account.test.ts +++ b/src/cli/handlers/account.test.ts @@ -49,7 +49,11 @@ vi.mock('../../shared/node-cli-command-resolution', () => ({ import { ACCOUNT_HANDLERS } from './account' import type { HandlerContext } from '../dispatch' import type { RuntimeClient } from '../runtime-client' -import { getCmdExePath } from '../../shared/windows-batch-spawn' +import { + getCmdExePath, + WINDOWS_BATCH_UNSAFE_ARGUMENTS_ERROR, + WINDOWS_BATCH_UNSAFE_CHARACTERS_LABEL +} from '../../shared/windows-batch-spawn' import { ACCOUNT_IMPORT_RUNTIME_CAPABILITY } from '../../shared/protocol-version' function successfulChild(): EventEmitter { @@ -168,6 +172,35 @@ describe('account CLI handlers', () => { ) }) + it('launches a Windows shim installed under Program Files (x86)', async () => { + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + const shim = 'C:\\Program Files (x86)\\nodejs\\codex.cmd' + resolveCliCommandMock.mockReturnValue(shim) + + await ACCOUNT_HANDLERS['account add'](context('codex')) + + expect(spawnMock).toHaveBeenCalledWith( + getCmdExePath(), + ['/d', '/c', shim, 'login', '--device-auth'], + expect.anything() + ) + }) + + it('explains an unspawnable Windows shim path instead of leaking the error sentinel', async () => { + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + resolveCliCommandMock.mockReturnValue('C:\\Users\\A&B\\codex.cmd') + + const error = await ACCOUNT_HANDLERS['account add'](context('codex')).catch( + (thrown: unknown) => thrown + ) + + expect(spawnMock).not.toHaveBeenCalled() + const message = error instanceof Error ? error.message : String(error) + expect(message).not.toBe(WINDOWS_BATCH_UNSAFE_ARGUMENTS_ERROR) + expect(message).toContain('C:\\Users\\A&B\\codex.cmd') + expect(message).toContain(WINDOWS_BATCH_UNSAFE_CHARACTERS_LABEL) + }) + it('adds version-manager Node paths to the login child environment', async () => { const nodeBin = '/home/test/.nvm/versions/node/v22.0.0/bin' getVersionManagerBinPathsMock.mockReturnValue([nodeBin]) diff --git a/src/cli/handlers/account.ts b/src/cli/handlers/account.ts index 0ad19aa240d..fd2c7b20402 100644 --- a/src/cli/handlers/account.ts +++ b/src/cli/handlers/account.ts @@ -16,7 +16,11 @@ import { getVersionManagerBinPaths, resolveCliCommand } from '../../shared/node-cli-command-resolution' -import { getSpawnArgsForWindows } from '../../shared/windows-batch-spawn' +import { + getSpawnArgsForWindows, + UnsafeWindowsBatchArgumentsError, + WINDOWS_BATCH_UNSAFE_CHARACTERS_LABEL +} from '../../shared/windows-batch-spawn' import { ACCOUNT_IMPORT_RUNTIME_CAPABILITY } from '../../shared/protocol-version' import type { RuntimeStatus } from '../../shared/runtime-types' import type { ClaudeRateLimitAccountsState, CodexRateLimitAccountsState } from '../../shared/types' @@ -86,7 +90,25 @@ async function runAgentLoginInTerminal( ): Promise { await new Promise((resolvePromise, rejectPromise) => { const resolvedCommand = resolveCliCommand(command) - const { spawnCmd, spawnArgs } = getSpawnArgsForWindows(resolvedCommand, args) + let spawnCmd: string + let spawnArgs: string[] + try { + ;({ spawnCmd, spawnArgs } = getSpawnArgsForWindows(resolvedCommand, args)) + } catch (error) { + // Why: the bare sentinel message reaches the user verbatim otherwise, with + // nothing naming the path or the characters that made it unspawnable. + rejectPromise( + error instanceof UnsafeWindowsBatchArgumentsError + ? new RuntimeClientError( + 'invalid_environment', + `Cannot run \`${command}\` from "${resolvedCommand}": the path contains characters ` + + `cmd.exe would reinterpret. Install it somewhere without ` + + `${WINDOWS_BATCH_UNSAFE_CHARACTERS_LABEL} in the path.` + ) + : error + ) + return + } const env = addAgentNodePaths({ ...stripElectronRunAsNode(process.env), ...extraEnv }) const child = spawn(spawnCmd, spawnArgs, { // Why: JSON mode reserves stdout for the response envelope while keeping diff --git a/src/cli/handlers/skills.ts b/src/cli/handlers/skills.ts index 7c911ea1751..4e8db5dbdae 100644 --- a/src/cli/handlers/skills.ts +++ b/src/cli/handlers/skills.ts @@ -12,7 +12,8 @@ import { } from '../../shared/tui-agent-detection-commands' import { getSpawnArgsForWindows, - UnsafeWindowsBatchArgumentsError + UnsafeWindowsBatchArgumentsError, + WINDOWS_BATCH_UNSAFE_CHARACTERS_LABEL } from '../../shared/windows-batch-spawn' import { isSkillsCliAgentKeyShaped, toSkillsCliAgentKeys } from '../../shared/skills-cli-agent-keys' import { @@ -134,7 +135,7 @@ function runNpxSkills(args: string[]): Promise { new RuntimeClientError( 'invalid_environment', `Cannot run npx from "${resolved}": the path contains characters cmd.exe would ` + - 'reinterpret. Install Node.js somewhere without & | < > ^ " % ! in the path.' + `reinterpret. Install Node.js somewhere without ${WINDOWS_BATCH_UNSAFE_CHARACTERS_LABEL} in the path.` ) ) return diff --git a/src/cli/skills.test.ts b/src/cli/skills.test.ts index 11caa30d704..c7fbb6b1b7e 100644 --- a/src/cli/skills.test.ts +++ b/src/cli/skills.test.ts @@ -2,6 +2,7 @@ import { EventEmitter } from 'node:events' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { delimiter } from 'node:path' import type * as CodexCliCommandModule from '../shared/node-cli-command-resolution' +import { WINDOWS_BATCH_UNSAFE_CHARACTERS_LABEL } from '../shared/windows-batch-spawn' const { detectCommandsMock, @@ -689,6 +690,26 @@ describe('orca skills CLI', () => { expect(spawnMock).not.toHaveBeenCalled() expect(process.exitCode).toBe(1) expect(String(errorSpy.mock.calls[0]?.[0])).toContain('cmd.exe would reinterpret') + // Why: remediation advice that names the wrong characters is unactionable. + expect(String(errorSpy.mock.calls[0]?.[0])).toContain(WINDOWS_BATCH_UNSAFE_CHARACTERS_LABEL) + }) + + it('runs npx from a Program Files (x86) install instead of refusing it', async () => { + const child = createFakeChild() + spawnMock.mockReturnValue(child) + vi.spyOn(process, 'platform', 'get').mockReturnValue('win32') + vi.stubEnv('ComSpec', 'C:\\Windows\\System32\\cmd.exe') + const npx = 'C:\\Program Files (x86)\\nodejs\\npx.cmd' + resolveCliCommandMock.mockReturnValue(npx) + vi.spyOn(process.stderr, 'write').mockImplementation(() => true) + + const resultPromise = main(['skills', 'install', '--skill', 'alpha'], '/tmp/repo') + await vi.waitFor(() => expect(spawnMock).toHaveBeenCalled()) + child.emit('exit', 0, null) + await resultPromise + + expect(spawnMock.mock.calls[0]?.[0]).toBe('C:\\Windows\\System32\\cmd.exe') + expect(spawnMock.mock.calls[0]?.[1]?.slice(0, 3)).toEqual(['/d', '/c', npx]) }) it('never puts the current directory on the child PATH when npx is unresolvable', async () => { diff --git a/src/main/skills/skill-update-run.test.ts b/src/main/skills/skill-update-run.test.ts index 0aeb31bf32e..7cc86402c9c 100644 --- a/src/main/skills/skill-update-run.test.ts +++ b/src/main/skills/skill-update-run.test.ts @@ -1,6 +1,10 @@ import { EventEmitter } from 'node:events' import { describe, expect, it, vi } from 'vitest' import type { SkillUpdateRun } from '../../shared/skill-freshness' +import { + getSpawnArgsForWindows, + WINDOWS_BATCH_UNSAFE_CHARACTERS_LABEL +} from '../../shared/windows-batch-spawn' import { CANCEL_RELEASE_TIMEOUT_MS, SkillUpdateRunner } from './skill-update-run' class FakeChild extends EventEmitter { @@ -293,6 +297,24 @@ describe('SkillUpdateRunner', () => { expect(run.state).toBe('error') expect(run.state === 'error' && run.failedNames).toEqual(['orca-cli']) expect(states.at(-1)?.state).toBe('error') + // Why: the message used to name a character set the guard no longer matches. + expect(run.state === 'error' && run.message).toContain(WINDOWS_BATCH_UNSAFE_CHARACTERS_LABEL) + }) + + it('spawns npx from a Program Files (x86) install instead of refusing it', () => { + const platform = vi.spyOn(process, 'platform', 'get').mockReturnValue('win32') + try { + const npx = 'C:\\Program Files (x86)\\nodejs\\npx.cmd' + const { runner, spawnCalls } = makeRunner({ + resolveCommand: () => npx, + buildSpawnArgs: getSpawnArgsForWindows + }) + + expect(runner.start(['orca-cli']).started).toBe(true) + expect(spawnCalls[0]?.args.slice(0, 3)).toEqual(['/d', '/c', npx]) + } finally { + platform.mockRestore() + } }) it('coalesces progress frames into one push instead of one per chunk', async () => { diff --git a/src/main/skills/skill-update-run.ts b/src/main/skills/skill-update-run.ts index 9083b24e43f..71365dd12c4 100644 --- a/src/main/skills/skill-update-run.ts +++ b/src/main/skills/skill-update-run.ts @@ -6,7 +6,7 @@ import { } from '../../shared/skill-freshness' import { resolveCliCommand } from '../codex-cli/command' import { killWithDescendantSweep } from '../pty-descendant-termination' -import { getSpawnArgsForWindows } from '../win32-utils' +import { getSpawnArgsForWindows, WINDOWS_BATCH_UNSAFE_CHARACTERS_LABEL } from '../win32-utils' // Why: `skills update` prints ANSI colour and \r + erase-line progress. We show // this log verbatim to the user but never parse it — `update` has no --json @@ -101,9 +101,8 @@ export class SkillUpdateRunner { ;({ spawnCmd, spawnArgs } = buildSpawnArgs(npxCommand, npxArgs)) } catch { // Why: the names are already canonical here, so this is the cmd.exe rail - // rejecting the resolved npx *path* — a profile directory containing `&`, - // `%` or `!` is enough. Publishing the failure keeps the dialog honest; - // returning a bare `started: false` would leave the button dead and silent. + // rejecting the resolved npx *path*. Publishing the failure keeps the dialog + // honest; a bare `started: false` would leave the button dead and silent. this.runToken += 1 this.settling = false this.publish({ @@ -112,7 +111,9 @@ export class SkillUpdateRunner { finishedAt: this.deps.now(), output: '', failedNames: canonicalNames, - message: `Could not run ${npxCommand} safely from this location.` + message: + `Could not run ${npxCommand} safely from this location: its path contains one of ` + + `${WINDOWS_BATCH_UNSAFE_CHARACTERS_LABEL}, which cmd.exe would reinterpret.` }) return { started: false, reason: 'unsafe-command-path' } } diff --git a/src/main/win32-utils.test.ts b/src/main/win32-utils.test.ts index 3f719e5ae34..4159d7d89b4 100644 --- a/src/main/win32-utils.test.ts +++ b/src/main/win32-utils.test.ts @@ -8,7 +8,8 @@ import { getSpawnArgsForWindows, isPermissionError, isWindowsBatchScript, - resolveWindowsCommand + resolveWindowsCommand, + WINDOWS_BATCH_UNSAFE_CHARACTERS_LABEL } from './win32-utils' function withPlatform(platform: NodeJS.Platform, fn: () => T): T { @@ -119,7 +120,7 @@ describe('getSpawnArgsForWindows', () => { it('rejects unsafe args for .cmd scripts on win32', () => { withPlatform('win32', () => { - for (const argument of ['hello & goodbye', 'close)', '(open']) { + for (const argument of ['hello & goodbye', 'a | b', 'x > y', '%PATH%', 'a\nb']) { expect(() => getSpawnArgsForWindows('C:\\tools\\agent.cmd', [argument])).toThrow( 'UNSAFE_WINDOWS_BATCH_ARGUMENTS' ) @@ -129,11 +130,9 @@ describe('getSpawnArgsForWindows', () => { it('rejects unsafe command paths for .cmd scripts on win32', () => { withPlatform('win32', () => { - for (const command of ['C:\\bad&path\\agent.cmd', 'C:\\bad(path\\agent.cmd']) { - expect(() => getSpawnArgsForWindows(command, ['login'])).toThrow( - 'UNSAFE_WINDOWS_BATCH_ARGUMENTS' - ) - } + expect(() => getSpawnArgsForWindows('C:\\bad&path\\agent.cmd', ['login'])).toThrow( + 'UNSAFE_WINDOWS_BATCH_ARGUMENTS' + ) }) }) @@ -144,6 +143,48 @@ describe('getSpawnArgsForWindows', () => { ).toEqual(['/d', '/c', 'C:\\tools\\agent.cmd', 'package,name;version']) }) }) + + // Why: parentheses only group commands and cannot chain one without a separator + // the guard already rejects, so paren-bearing paths must stay spawnable. + it('spawns .cmd shims under Program Files (x86) and paren-bearing worktrees', () => { + withPlatform('win32', () => { + const npx = 'C:\\Program Files (x86)\\nodejs\\npx.cmd' + expect(getSpawnArgsForWindows(npx, ['C:\\dev\\app (fork)'])).toEqual({ + spawnCmd: getCmdExePath(), + spawnArgs: ['/d', '/c', npx, 'C:\\dev\\app (fork)'] + }) + expect(getSpawnArgsForWindows('C:\\tools\\agent.cmd', ['close)', '(open']).spawnArgs).toEqual( + ['/d', '/c', 'C:\\tools\\agent.cmd', 'close)', '(open'] + ) + }) + }) + + it('still rejects a paren-wrapped command chain', () => { + withPlatform('win32', () => { + expect(() => getSpawnArgsForWindows('C:\\tools\\agent.cmd', ['(x & calc.exe)'])).toThrow( + 'UNSAFE_WINDOWS_BATCH_ARGUMENTS' + ) + }) + }) + + it('rejects exactly the characters the message advertises', () => { + withPlatform('win32', () => { + const advertised = WINDOWS_BATCH_UNSAFE_CHARACTERS_LABEL.split(' ') + expect(advertised.length).toBeGreaterThan(0) + for (const character of advertised) { + expect(() => getSpawnArgsForWindows('C:\\tools\\agent.cmd', [`a${character}b`])).toThrow( + 'UNSAFE_WINDOWS_BATCH_ARGUMENTS' + ) + } + // Why: anything not advertised must pass, or the message misleads the user. + for (const character of ['(', ')', ',', ';', '@', '#', '$', "'", '~', '=']) { + expect(advertised).not.toContain(character) + expect(() => + getSpawnArgsForWindows('C:\\tools\\agent.cmd', [`a${character}b`]) + ).not.toThrow() + } + }) + }) }) describe('resolveWindowsCommand', () => { diff --git a/src/main/win32-utils.ts b/src/main/win32-utils.ts index 21f3a75853d..c1031c7c55e 100644 --- a/src/main/win32-utils.ts +++ b/src/main/win32-utils.ts @@ -7,6 +7,7 @@ export { getSpawnArgsForWindows, isWindowsBatchScript, WINDOWS_BATCH_UNSAFE_ARGUMENTS_ERROR, + WINDOWS_BATCH_UNSAFE_CHARACTERS_LABEL, UnsafeWindowsBatchArgumentsError } from '../shared/windows-batch-spawn' diff --git a/src/shared/windows-batch-spawn.ts b/src/shared/windows-batch-spawn.ts index cd20dff5c79..4afddccae73 100644 --- a/src/shared/windows-batch-spawn.ts +++ b/src/shared/windows-batch-spawn.ts @@ -21,8 +21,22 @@ export class UnsafeWindowsBatchArgumentsError extends Error { } } +// Why: cmd.exe re-parses the command line, and these are the characters that can +// start a new command or expand a variable out of an otherwise inert argument. +// `(`/`)` are deliberately absent: they only group commands, and grouping cannot +// chain anything without one of the separators below, so rejecting them merely +// broke every `C:\Program Files (x86)\...` shim and paren-bearing worktree path. +const WINDOWS_BATCH_UNSAFE_CHARACTERS = ['&', '|', '<', '>', '^', '"', '%', '!'] as const + +/** The rejected characters, spelled for error messages so they cannot drift from the guard. */ +export const WINDOWS_BATCH_UNSAFE_CHARACTERS_LABEL = WINDOWS_BATCH_UNSAFE_CHARACTERS.join(' ') + +const UNSAFE_WINDOWS_BATCH_SYNTAX = new RegExp( + `[${WINDOWS_BATCH_UNSAFE_CHARACTERS.map((character) => character.replace(/[\\^\]-]/, '\\$&')).join('')}\\r\\n]` +) + function hasUnsafeWindowsBatchSyntax(value: string): boolean { - return /[&|<>^"%!()\r\n]/.test(value) + return UNSAFE_WINDOWS_BATCH_SYNTAX.test(value) } export function getSpawnArgsForWindows(