From ef54fe225c7130eedf492c938b7ed55718f22d64 Mon Sep 17 00:00:00 2001 From: Praveen Mittal Date: Sat, 22 Aug 2026 22:40:22 +0200 Subject: [PATCH 1/5] fix: resolve executables instead of shell-wrapping SHELL on Windows spawns process.mjs's runCommand and app-server.mjs's codex app-server launch both defaulted to shell: process.env.SHELL || true on Windows. spawn/spawnSync never consult PATHEXT, so a bare codex (which on Windows only ships as a .cmd/.ps1 shim, no .exe) fails with ENOENT unless something resolves it first (#287). Handing process.env.SHELL to shell: means Node passes the whole command line to whatever that variable points at using its own quoting, which doesn't match PowerShell's rules when SHELL points there. Add resolveExecutablePath(), which walks PATH/PATHEXT to find the concrete executable on Windows, then spawn that resolved path with shell: false. Node still safely wraps a resolved .cmd/.bat target through cmd.exe internally when needed (hardened by the CVE-2024-27980 fix), but these spawns never hand an arbitrary shell a raw command line to reinterpret. Also removes the DEP0190 warning these spawns triggered on every run. Fixes #287 --- plugins/codex/scripts/lib/app-server.mjs | 14 +-- plugins/codex/scripts/lib/process.mjs | 66 ++++++++++++- tests/process.test.mjs | 115 ++++++++++++++++++++++- 3 files changed, 186 insertions(+), 9 deletions(-) diff --git a/plugins/codex/scripts/lib/app-server.mjs b/plugins/codex/scripts/lib/app-server.mjs index 72b30a764..9f599d6f7 100644 --- a/plugins/codex/scripts/lib/app-server.mjs +++ b/plugins/codex/scripts/lib/app-server.mjs @@ -14,7 +14,7 @@ import { spawn } from "node:child_process"; import readline from "node:readline"; import { parseBrokerEndpoint } from "./broker-endpoint.mjs"; import { ensureBrokerSession, loadBrokerSession } from "./broker-lifecycle.mjs"; -import { terminateProcessTree } from "./process.mjs"; +import { resolveExecutablePath, terminateProcessTree } from "./process.mjs"; const PLUGIN_MANIFEST_URL = new URL("../../.claude-plugin/plugin.json", import.meta.url); const PLUGIN_MANIFEST = JSON.parse(fs.readFileSync(PLUGIN_MANIFEST_URL, "utf8")); @@ -187,11 +187,11 @@ class SpawnedCodexAppServerClient extends AppServerClientBase { } async initialize() { - this.proc = spawn("codex", ["app-server"], { + this.proc = spawn(resolveExecutablePath("codex"), ["app-server"], { cwd: this.cwd, env: this.options.env ?? process.env, stdio: ["pipe", "pipe", "pipe"], - shell: process.platform === "win32" ? (process.env.SHELL || true) : false, + shell: false, windowsHide: true }); @@ -245,9 +245,11 @@ class SpawnedCodexAppServerClient extends AppServerClientBase { this.proc.stdin.end(); setTimeout(() => { if (this.proc && !this.proc.killed && this.proc.exitCode === null) { - // On Windows with shell: true, the direct child is cmd.exe. - // Use terminateProcessTree to kill the entire tree including - // the grandchild node process. + // On Windows, a resolved .cmd/.bat target still runs through + // cmd.exe as an intermediary (Node wraps those internally even + // with shell: false), so the direct child is cmd.exe, not codex + // itself. Use terminateProcessTree to kill the entire tree + // including the grandchild node process. if (process.platform === "win32") { try { terminateProcessTree(this.proc.pid); diff --git a/plugins/codex/scripts/lib/process.mjs b/plugins/codex/scripts/lib/process.mjs index dd8fc3751..752debaa3 100644 --- a/plugins/codex/scripts/lib/process.mjs +++ b/plugins/codex/scripts/lib/process.mjs @@ -1,15 +1,77 @@ +import fs from "node:fs"; +import path from "node:path"; import { spawnSync } from "node:child_process"; import process from "node:process"; +const DEFAULT_PATHEXT = ".COM;.EXE;.BAT;.CMD"; + +/** + * Resolves `command` to a concrete file path on Windows, so it can be + * spawned with `shell: false` instead of a shell string. `spawn`/`spawnSync` + * never consult `PATHEXT` themselves, so a bare command that only exists as + * an extensionless/`.cmd` shim (e.g. an npm-installed CLI) fails with ENOENT + * unless something else resolves it first (#287) -- but handing + * `process.env.SHELL` to `shell:` as a quick fix means Node hands the whole + * command line to whatever that variable points at, unescaped for that + * shell's own quoting rules. When it happens to be PowerShell, a `>` from + * quoted source text is read as a redirect and creates junk files in the + * repo (#643). Resolving to the literal file sidesteps a caller-supplied + * shell entirely: Node still wraps a resolved `.cmd`/`.bat` target through + * cmd.exe internally when needed (hardened by the CVE-2024-27980 fix), but + * never asks an arbitrary shell to reinterpret a raw command string. + */ +export function resolveExecutablePath(command, options = {}) { + const platform = options.platform ?? process.platform; + if (platform !== "win32") { + return command; + } + + const win = path.win32; + if (win.isAbsolute(command) || command.includes("/") || command.includes("\\")) { + return command; + } + + const existsSync = options.existsSync ?? fs.existsSync; + const pathEnv = options.pathEnv ?? process.env.PATH ?? process.env.Path ?? ""; + const pathExtEnv = options.pathExtEnv ?? process.env.PATHEXT ?? DEFAULT_PATHEXT; + + const dirs = pathEnv.split(win.delimiter).filter(Boolean); + const extensions = pathExtEnv + .split(";") + .map((ext) => ext.trim()) + .filter(Boolean); + + const hasKnownExtension = extensions.some((ext) => command.toLowerCase().endsWith(ext.toLowerCase())); + const candidateExtensions = hasKnownExtension ? [""] : extensions; + + for (const dir of dirs) { + for (const ext of candidateExtensions) { + const candidate = win.join(dir, `${command}${ext}`); + if (existsSync(candidate)) { + return candidate; + } + } + } + + return command; +} + export function runCommand(command, args = [], options = {}) { - const result = spawnSync(command, args, { + const resolvedCommand = resolveExecutablePath(command, { + platform: options.platform, + existsSync: options.existsSync, + pathEnv: options.pathEnv, + pathExtEnv: options.pathExtEnv + }); + + const result = spawnSync(resolvedCommand, args, { cwd: options.cwd, env: options.env, encoding: "utf8", input: options.input, maxBuffer: options.maxBuffer, stdio: options.stdio ?? "pipe", - shell: options.shell ?? (process.platform === "win32" ? (process.env.SHELL || true) : false), + shell: options.shell ?? false, windowsHide: true }); diff --git a/tests/process.test.mjs b/tests/process.test.mjs index 80e0715b0..de816a04f 100644 --- a/tests/process.test.mjs +++ b/tests/process.test.mjs @@ -1,7 +1,8 @@ import test from "node:test"; import assert from "node:assert/strict"; +import process from "node:process"; -import { terminateProcessTree } from "../plugins/codex/scripts/lib/process.mjs"; +import { resolveExecutablePath, runCommand, terminateProcessTree } from "../plugins/codex/scripts/lib/process.mjs"; test("terminateProcessTree uses taskkill on Windows", () => { let captured = null; @@ -53,3 +54,115 @@ test("terminateProcessTree treats missing Windows processes as already stopped", assert.equal(outcome.result.status, 128); assert.match(outcome.result.stdout, /not found/i); }); + +function fakeExistsSync(realPaths) { + // Windows filesystems are case-insensitive, so match the way the real + // fs.existsSync would on a Windows machine. + const set = new Set(realPaths.map((p) => p.toLowerCase())); + return (candidate) => set.has(candidate.toLowerCase()); +} + +test("resolveExecutablePath is a no-op off Windows", () => { + let existsSyncCalled = false; + const resolved = resolveExecutablePath("codex", { + platform: "linux", + existsSync: () => { + existsSyncCalled = true; + return true; + }, + pathEnv: "C:\\tools", + pathExtEnv: ".EXE" + }); + + assert.equal(resolved, "codex"); + assert.equal(existsSyncCalled, false); +}); + +test("resolveExecutablePath leaves an already-absolute or path-qualified command alone", () => { + assert.equal( + resolveExecutablePath("C:\\tools\\codex.cmd", { platform: "win32", existsSync: () => false }), + "C:\\tools\\codex.cmd" + ); + assert.equal( + resolveExecutablePath(".\\codex.cmd", { platform: "win32", existsSync: () => false }), + ".\\codex.cmd" + ); + assert.equal( + resolveExecutablePath("sub/codex.cmd", { platform: "win32", existsSync: () => false }), + "sub/codex.cmd" + ); +}); + +test("resolveExecutablePath finds an npm-style .cmd shim via PATH and PATHEXT, in PATHEXT order", () => { + const resolved = resolveExecutablePath("codex", { + platform: "win32", + pathEnv: "C:\\nothing;C:\\tools", + pathExtEnv: ".COM;.EXE;.BAT;.CMD", + existsSync: fakeExistsSync(["C:\\tools\\codex.cmd"]) + }); + + assert.equal(resolved, "C:\\tools\\codex.CMD"); +}); + +test("resolveExecutablePath prefers an earlier PATH directory over a later one", () => { + const resolved = resolveExecutablePath("git", { + platform: "win32", + pathEnv: "C:\\first;C:\\second", + pathExtEnv: ".EXE", + existsSync: fakeExistsSync(["C:\\first\\git.exe", "C:\\second\\git.exe"]) + }); + + assert.equal(resolved, "C:\\first\\git.EXE"); +}); + +test("resolveExecutablePath prefers an earlier PATHEXT extension over a later one in the same directory", () => { + const resolved = resolveExecutablePath("tool", { + platform: "win32", + pathEnv: "C:\\tools", + pathExtEnv: ".EXE;.CMD", + existsSync: fakeExistsSync(["C:\\tools\\tool.exe", "C:\\tools\\tool.cmd"]) + }); + + assert.equal(resolved, "C:\\tools\\tool.EXE"); +}); + +test("resolveExecutablePath does not append another extension when the command already has a known one", () => { + const resolved = resolveExecutablePath("codex.CMD", { + platform: "win32", + pathEnv: "C:\\tools", + pathExtEnv: ".COM;.EXE;.BAT;.CMD", + existsSync: fakeExistsSync(["C:\\tools\\codex.CMD.exe", "C:\\tools\\codex.CMD"]) + }); + + assert.equal(resolved, "C:\\tools\\codex.CMD"); +}); + +test("resolveExecutablePath respects a custom PATHEXT instead of the built-in default", () => { + const resolved = resolveExecutablePath("tool", { + platform: "win32", + pathEnv: "C:\\tools", + pathExtEnv: ".EXE", + existsSync: fakeExistsSync(["C:\\tools\\tool.cmd"]) + }); + + assert.equal(resolved, "tool"); +}); + +test("resolveExecutablePath falls back to the bare command when nothing on PATH matches", () => { + const resolved = resolveExecutablePath("missing-tool", { + platform: "win32", + pathEnv: "C:\\tools", + pathExtEnv: ".EXE", + existsSync: () => false + }); + + assert.equal(resolved, "missing-tool"); +}); + +test("runCommand still runs a real command end to end", () => { + const result = runCommand(process.execPath, ["--version"]); + + assert.equal(result.error, null); + assert.equal(result.status, 0); + assert.match(result.stdout, /^v\d+\.\d+\.\d+/); +}); From 81d39e0b9af37a871ba3909f76664ad86e8b86f1 Mon Sep 17 00:00:00 2001 From: Praveen Mittal Date: Sat, 22 Aug 2026 23:28:05 +0200 Subject: [PATCH 2/5] fix: spawn cmd.exe explicitly for .cmd/.bat targets instead of assuming shell:false works The previous commit assumed Node wraps a resolved .cmd/.bat target through cmd.exe internally even with shell: false. That's wrong: Node's own docs are explicit that .bat/.cmd files "are not executable on their own without a terminal" and cannot be launched via spawn()/spawnSync() with shell: false at all, resolved path or not. As written, the previous fix broke the primary codex app-server launch outright on a normal Windows install, since codex only ships as a .cmd/.ps1 shim. Port cross-spawn's (MIT, https://github.com/moxystudio/node-cross-spawn) escaping algorithm: anything resolving to .exe/.com still spawns directly with shell: false; anything else explicitly spawns cmd.exe (never process.env.SHELL, which is what caused #643) with the command line escaped and quoted the way cmd.exe itself requires, including the double-escape quirk cross-spawn documents for npm's own node_modules/.bin/*.cmd shims. Also fixes resolveExecutablePath/resolveSpawnInvocation to read PATH/PATHEXT/comspec from a caller-supplied options.env instead of always process.env, so resolution matches the environment the child actually runs in. Both issues found via Codex Review on the PR. --- plugins/codex/scripts/lib/app-server.mjs | 19 ++-- plugins/codex/scripts/lib/process.mjs | 128 +++++++++++++++++++---- tests/process.test.mjs | 119 ++++++++++++++++++++- 3 files changed, 239 insertions(+), 27 deletions(-) diff --git a/plugins/codex/scripts/lib/app-server.mjs b/plugins/codex/scripts/lib/app-server.mjs index 9f599d6f7..565b94bb5 100644 --- a/plugins/codex/scripts/lib/app-server.mjs +++ b/plugins/codex/scripts/lib/app-server.mjs @@ -14,7 +14,7 @@ import { spawn } from "node:child_process"; import readline from "node:readline"; import { parseBrokerEndpoint } from "./broker-endpoint.mjs"; import { ensureBrokerSession, loadBrokerSession } from "./broker-lifecycle.mjs"; -import { resolveExecutablePath, terminateProcessTree } from "./process.mjs"; +import { resolveSpawnInvocation, terminateProcessTree } from "./process.mjs"; const PLUGIN_MANIFEST_URL = new URL("../../.claude-plugin/plugin.json", import.meta.url); const PLUGIN_MANIFEST = JSON.parse(fs.readFileSync(PLUGIN_MANIFEST_URL, "utf8")); @@ -187,11 +187,14 @@ class SpawnedCodexAppServerClient extends AppServerClientBase { } async initialize() { - this.proc = spawn(resolveExecutablePath("codex"), ["app-server"], { + const env = this.options.env ?? process.env; + const invocation = resolveSpawnInvocation("codex", ["app-server"], { env }); + this.proc = spawn(invocation.command, invocation.args, { cwd: this.cwd, - env: this.options.env ?? process.env, + env, stdio: ["pipe", "pipe", "pipe"], shell: false, + windowsVerbatimArguments: invocation.windowsVerbatimArguments, windowsHide: true }); @@ -245,11 +248,11 @@ class SpawnedCodexAppServerClient extends AppServerClientBase { this.proc.stdin.end(); setTimeout(() => { if (this.proc && !this.proc.killed && this.proc.exitCode === null) { - // On Windows, a resolved .cmd/.bat target still runs through - // cmd.exe as an intermediary (Node wraps those internally even - // with shell: false), so the direct child is cmd.exe, not codex - // itself. Use terminateProcessTree to kill the entire tree - // including the grandchild node process. + // On Windows, a .cmd-shimmed target is launched by explicitly + // spawning cmd.exe (see resolveSpawnInvocation()), so the direct + // child here is cmd.exe, not codex itself. Use + // terminateProcessTree to kill the entire tree including the + // grandchild node process. if (process.platform === "win32") { try { terminateProcessTree(this.proc.pid); diff --git a/plugins/codex/scripts/lib/process.mjs b/plugins/codex/scripts/lib/process.mjs index 752debaa3..75afb8625 100644 --- a/plugins/codex/scripts/lib/process.mjs +++ b/plugins/codex/scripts/lib/process.mjs @@ -6,19 +6,12 @@ import process from "node:process"; const DEFAULT_PATHEXT = ".COM;.EXE;.BAT;.CMD"; /** - * Resolves `command` to a concrete file path on Windows, so it can be - * spawned with `shell: false` instead of a shell string. `spawn`/`spawnSync` - * never consult `PATHEXT` themselves, so a bare command that only exists as - * an extensionless/`.cmd` shim (e.g. an npm-installed CLI) fails with ENOENT - * unless something else resolves it first (#287) -- but handing - * `process.env.SHELL` to `shell:` as a quick fix means Node hands the whole - * command line to whatever that variable points at, unescaped for that - * shell's own quoting rules. When it happens to be PowerShell, a `>` from - * quoted source text is read as a redirect and creates junk files in the - * repo (#643). Resolving to the literal file sidesteps a caller-supplied - * shell entirely: Node still wraps a resolved `.cmd`/`.bat` target through - * cmd.exe internally when needed (hardened by the CVE-2024-27980 fix), but - * never asks an arbitrary shell to reinterpret a raw command string. + * Resolves `command` to a concrete file path on Windows, so its extension + * can be inspected to decide how it needs to be spawned (see + * buildSpawnCommand()). `spawn`/`spawnSync` never consult `PATHEXT` + * themselves, so a bare command that only exists as an extensionless/`.cmd` + * shim (e.g. an npm-installed CLI) fails with ENOENT unless something else + * resolves it first (#287). */ export function resolveExecutablePath(command, options = {}) { const platform = options.platform ?? process.platform; @@ -56,15 +49,113 @@ export function resolveExecutablePath(command, options = {}) { return command; } -export function runCommand(command, args = [], options = {}) { +const EXECUTABLE_EXTENSION_REGEXP = /\.(?:com|exe)$/i; +// Matches cross-spawn's own detection of an npm-generated cmd shim, which +// wraps the real command through its own %~dp0-based cmd.exe redirection -- +// meta chars we escape once get interpreted once by that inner layer before +// cmd.exe ever sees them, so they need a second escape pass to survive. +const NPM_CMD_SHIM_REGEXP = /node_modules[\\/].bin[\\/][^\\/]+\.cmd$/i; +// See http://www.robvanderwoude.com/escapechars.php +const CMD_METACHAR_REGEXP = /([()\][%!^"`<>&|;, *?])/g; + +// escapeCmdCommand/escapeCmdArgument are ported from cross-spawn +// (https://github.com/moxystudio/node-cross-spawn, MIT License, Copyright +// (c) 2018 Made With MOXY Lda) -- the standard reference implementation for +// safely invoking cmd.exe on Windows. escapeCmdArgument's backslash/quote +// handling is based on https://qntm.org/cmd, cross-spawn's own cited source. +function escapeCmdCommand(value) { + return value.replace(CMD_METACHAR_REGEXP, "^$1"); +} + +function escapeCmdArgument(value, doubleEscapeMetaChars) { + let arg = String(value); + + // Sequence of backslashes followed by a double quote: double up all the + // backslashes and escape the double quote. + arg = arg.replace(/(?=(\\+?)?)\1"/g, '$1$1\\"'); + // Sequence of backslashes followed by the end of the string (which will + // become a double quote next): double up all the backslashes. + arg = arg.replace(/(?=(\\+?)?)\1$/, "$1$1"); + // All other backslashes occur literally. + + arg = `"${arg}"`; + arg = arg.replace(CMD_METACHAR_REGEXP, "^$1"); + if (doubleEscapeMetaChars) { + arg = arg.replace(CMD_METACHAR_REGEXP, "^$1"); + } + + return arg; +} + +/** + * Given a command already resolved by resolveExecutablePath(), decides how + * it actually needs to be spawned on Windows and returns the + * { command, args, windowsVerbatimArguments } to pass to spawn/spawnSync. + * + * Node's own docs are explicit that `.bat`/`.cmd` files "are not executable + * on their own without a terminal" -- spawn()/spawnSync() with + * shell: false cannot launch them no matter what path is given, resolved + * or not. Anything that isn't `.exe`/`.com` must instead be launched by + * explicitly spawning cmd.exe (never a caller- or environment-supplied + * shell, which is what caused #643) with the command line escaped and + * quoted the way cmd.exe itself requires. + */ +export function buildSpawnCommand(resolvedCommand, args, options = {}) { + const platform = options.platform ?? process.platform; + if (platform !== "win32" || EXECUTABLE_EXTENSION_REGEXP.test(resolvedCommand)) { + return { command: resolvedCommand, args, windowsVerbatimArguments: undefined }; + } + + const needsDoubleEscapeMetaChars = NPM_CMD_SHIM_REGEXP.test(resolvedCommand); + const escapedCommand = escapeCmdCommand(path.win32.normalize(resolvedCommand)); + const escapedArgs = args.map((arg) => escapeCmdArgument(arg, needsDoubleEscapeMetaChars)); + const shellCommand = [escapedCommand, ...escapedArgs].join(" "); + const comspec = options.comspec || "cmd.exe"; + + return { + command: comspec, + args: ["/d", "/s", "/c", `"${shellCommand}"`], + windowsVerbatimArguments: true + }; +} + +/** + * Resolves `command` and decides how to spawn it, in one step. `options.env` + * (the environment the child will actually run in) is consulted for + * PATH/PATHEXT/COMSPEC when given, since resolving against the running + * process's own environment could pick a different executable than the one + * the child would actually see. + */ +export function resolveSpawnInvocation(command, args, options = {}) { + const platform = options.platform ?? process.platform; const resolvedCommand = resolveExecutablePath(command, { - platform: options.platform, + platform, existsSync: options.existsSync, - pathEnv: options.pathEnv, - pathExtEnv: options.pathExtEnv + pathEnv: options.pathEnv ?? options.env?.PATH ?? options.env?.Path, + pathExtEnv: options.pathExtEnv ?? options.env?.PATHEXT ?? options.env?.Pathext }); - const result = spawnSync(resolvedCommand, args, { + return buildSpawnCommand(resolvedCommand, args, { + platform, + comspec: options.comspec ?? options.env?.comspec ?? options.env?.ComSpec + }); +} + +export function runCommand(command, args = [], options = {}) { + let spawnCommand = command; + let spawnArgs = args; + let windowsVerbatimArguments; + + // An explicit `options.shell` asks for direct control over shell + // behavior; anything else goes through the safe, resolved invocation. + if (options.shell === undefined) { + const invocation = resolveSpawnInvocation(command, args, options); + spawnCommand = invocation.command; + spawnArgs = invocation.args; + windowsVerbatimArguments = invocation.windowsVerbatimArguments; + } + + const result = spawnSync(spawnCommand, spawnArgs, { cwd: options.cwd, env: options.env, encoding: "utf8", @@ -72,6 +163,7 @@ export function runCommand(command, args = [], options = {}) { maxBuffer: options.maxBuffer, stdio: options.stdio ?? "pipe", shell: options.shell ?? false, + windowsVerbatimArguments, windowsHide: true }); diff --git a/tests/process.test.mjs b/tests/process.test.mjs index de816a04f..74e661257 100644 --- a/tests/process.test.mjs +++ b/tests/process.test.mjs @@ -2,7 +2,13 @@ import test from "node:test"; import assert from "node:assert/strict"; import process from "node:process"; -import { resolveExecutablePath, runCommand, terminateProcessTree } from "../plugins/codex/scripts/lib/process.mjs"; +import { + buildSpawnCommand, + resolveExecutablePath, + resolveSpawnInvocation, + runCommand, + terminateProcessTree +} from "../plugins/codex/scripts/lib/process.mjs"; test("terminateProcessTree uses taskkill on Windows", () => { let captured = null; @@ -166,3 +172,114 @@ test("runCommand still runs a real command end to end", () => { assert.equal(result.status, 0); assert.match(result.stdout, /^v\d+\.\d+\.\d+/); }); + +// Regression tests for the P1 finding on PR #669: spawn()/spawnSync() with +// shell: false cannot launch a .bat/.cmd file at all, resolved path or not +// (Node's own docs: "`.bat` and `.cmd` files are not executable on their +// own without a terminal"). A resolved path alone is not sufficient -- +// anything that isn't .exe/.com must be launched by explicitly spawning +// cmd.exe with the command line escaped the way cmd.exe itself requires. +test("buildSpawnCommand leaves a resolved .exe target unwrapped", () => { + const result = buildSpawnCommand("C:\\tools\\git.exe", ["--version"], { platform: "win32" }); + + assert.deepEqual(result, { + command: "C:\\tools\\git.exe", + args: ["--version"], + windowsVerbatimArguments: undefined + }); +}); + +test("buildSpawnCommand leaves a resolved .COM target unwrapped (case-insensitive)", () => { + const result = buildSpawnCommand("C:\\tools\\tool.COM", ["x"], { platform: "win32" }); + + assert.equal(result.command, "C:\\tools\\tool.COM"); + assert.equal(result.windowsVerbatimArguments, undefined); +}); + +test("buildSpawnCommand is a no-op off Windows regardless of extension", () => { + const result = buildSpawnCommand("codex.cmd", ["app-server"], { platform: "linux" }); + + assert.deepEqual(result, { + command: "codex.cmd", + args: ["app-server"], + windowsVerbatimArguments: undefined + }); +}); + +test("buildSpawnCommand wraps a resolved .cmd target through cmd.exe with escaped arguments", () => { + const result = buildSpawnCommand("C:\\tools\\codex.cmd", ["app-server"], { + platform: "win32", + comspec: "cmd.exe" + }); + + assert.deepEqual(result, { + command: "cmd.exe", + args: ["/d", "/s", "/c", '"C:\\tools\\codex.cmd ^"app-server^""'], + windowsVerbatimArguments: true + }); +}); + +test("buildSpawnCommand defaults comspec to cmd.exe when not given", () => { + const result = buildSpawnCommand("codex.cmd", ["app-server"], { platform: "win32" }); + + assert.equal(result.command, "cmd.exe"); +}); + +test("buildSpawnCommand double-escapes meta chars for an npm node_modules/.bin cmd shim", () => { + const shimResult = buildSpawnCommand("C:\\proj\\node_modules\\.bin\\codex.cmd", ["--flag=a&b"], { + platform: "win32", + comspec: "cmd.exe" + }); + const plainResult = buildSpawnCommand("C:\\tools\\codex.cmd", ["--flag=a&b"], { + platform: "win32", + comspec: "cmd.exe" + }); + + assert.deepEqual(shimResult.args, ["/d", "/s", "/c", '"C:\\proj\\node_modules\\.bin\\codex.cmd ^^^"--flag=a^^^&b^^^""'],); + assert.deepEqual(plainResult.args, ["/d", "/s", "/c", '"C:\\tools\\codex.cmd ^"--flag=a^&b^""']); +}); + +test("resolveSpawnInvocation resolves the executable and wraps it through cmd.exe in one step", () => { + const invocation = resolveSpawnInvocation("codex", ["app-server"], { + platform: "win32", + pathEnv: "C:\\tools", + pathExtEnv: ".COM;.EXE;.BAT;.CMD", + comspec: "cmd.exe", + existsSync: fakeExistsSync(["C:\\tools\\codex.cmd"]) + }); + + assert.equal(invocation.command, "cmd.exe"); + assert.equal(invocation.windowsVerbatimArguments, true); + assert.equal(invocation.args[3], '"C:\\tools\\codex.CMD ^"app-server^""'); +}); + +test("resolveSpawnInvocation prefers options.env's PATH/PATHEXT/comspec over process.env's", () => { + const invocation = resolveSpawnInvocation("codex", ["app-server"], { + platform: "win32", + env: { + PATH: "C:\\childpath", + PATHEXT: ".EXE", + comspec: "C:\\child\\cmd.exe" + }, + existsSync: fakeExistsSync(["C:\\childpath\\codex.EXE"]) + }); + + assert.equal(invocation.command, "C:\\childpath\\codex.EXE"); + assert.equal(invocation.args[0], "app-server"); + assert.equal(invocation.windowsVerbatimArguments, undefined); +}); + +test("resolveSpawnInvocation uses options.env's comspec when the resolved target needs cmd.exe wrapping", () => { + const invocation = resolveSpawnInvocation("codex", ["app-server"], { + platform: "win32", + env: { + PATH: "C:\\childpath", + PATHEXT: ".CMD", + comspec: "C:\\child\\cmd.exe" + }, + existsSync: fakeExistsSync(["C:\\childpath\\codex.CMD"]) + }); + + assert.equal(invocation.command, "C:\\child\\cmd.exe"); + assert.equal(invocation.windowsVerbatimArguments, true); +}); From deeccc1630ef94474c3a67ed36431a5b50882c03 Mon Sep 17 00:00:00 2001 From: Praveen Mittal Date: Sat, 22 Aug 2026 23:57:40 +0200 Subject: [PATCH 3/5] fix: search cwd before PATH when resolving an executable CreateProcess's own documented search sequence for a bare command name is the loading app's directory, then "the current directory for the parent process", then the system/Windows directories, then PATH. resolveExecutablePath only ever searched PATH, so a cwd-local codex.cmd could be shadowed by an unrelated global installation found on PATH, even though running the same bare command from that directory would find the local one first. Thread options.cwd through resolveExecutablePath/resolveSpawnInvocation, search it before PATH, and resolve relative PATH entries against it. Found via Codex Review on the PR. --- plugins/codex/scripts/lib/app-server.mjs | 2 +- plugins/codex/scripts/lib/process.mjs | 22 +++++++++- tests/process.test.mjs | 55 ++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 3 deletions(-) diff --git a/plugins/codex/scripts/lib/app-server.mjs b/plugins/codex/scripts/lib/app-server.mjs index 565b94bb5..3b9bdb684 100644 --- a/plugins/codex/scripts/lib/app-server.mjs +++ b/plugins/codex/scripts/lib/app-server.mjs @@ -188,7 +188,7 @@ class SpawnedCodexAppServerClient extends AppServerClientBase { async initialize() { const env = this.options.env ?? process.env; - const invocation = resolveSpawnInvocation("codex", ["app-server"], { env }); + const invocation = resolveSpawnInvocation("codex", ["app-server"], { env, cwd: this.cwd }); this.proc = spawn(invocation.command, invocation.args, { cwd: this.cwd, env, diff --git a/plugins/codex/scripts/lib/process.mjs b/plugins/codex/scripts/lib/process.mjs index 75afb8625..76ada56d2 100644 --- a/plugins/codex/scripts/lib/process.mjs +++ b/plugins/codex/scripts/lib/process.mjs @@ -12,6 +12,15 @@ const DEFAULT_PATHEXT = ".COM;.EXE;.BAT;.CMD"; * themselves, so a bare command that only exists as an extensionless/`.cmd` * shim (e.g. an npm-installed CLI) fails with ENOENT unless something else * resolves it first (#287). + * + * Windows' own CreateProcess searches the current directory before PATH + * when given a bare command name (documented search sequence: the loading + * app's directory, then "the current directory for the parent process", + * then the system/Windows directories, then PATH) -- so `cwd` (the + * directory the spawned process will actually run from, matching Node's + * own `spawn`/`spawnSync` `cwd` option) is searched first here too, and + * any relative PATH entry is resolved against it, to match what running + * the same bare command from that directory would actually find. */ export function resolveExecutablePath(command, options = {}) { const platform = options.platform ?? process.platform; @@ -25,10 +34,16 @@ export function resolveExecutablePath(command, options = {}) { } const existsSync = options.existsSync ?? fs.existsSync; + const cwd = options.cwd ?? process.cwd(); const pathEnv = options.pathEnv ?? process.env.PATH ?? process.env.Path ?? ""; const pathExtEnv = options.pathExtEnv ?? process.env.PATHEXT ?? DEFAULT_PATHEXT; - const dirs = pathEnv.split(win.delimiter).filter(Boolean); + const pathDirs = pathEnv + .split(win.delimiter) + .filter(Boolean) + .map((dir) => (win.isAbsolute(dir) ? dir : win.resolve(cwd, dir))); + const dirs = [cwd, ...pathDirs]; + const extensions = pathExtEnv .split(";") .map((ext) => ext.trim()) @@ -124,13 +139,16 @@ export function buildSpawnCommand(resolvedCommand, args, options = {}) { * (the environment the child will actually run in) is consulted for * PATH/PATHEXT/COMSPEC when given, since resolving against the running * process's own environment could pick a different executable than the one - * the child would actually see. + * the child would actually see. `options.cwd` (the directory the child will + * actually run from) is searched before PATH, matching what running the + * same bare command from that directory would find. */ export function resolveSpawnInvocation(command, args, options = {}) { const platform = options.platform ?? process.platform; const resolvedCommand = resolveExecutablePath(command, { platform, existsSync: options.existsSync, + cwd: options.cwd, pathEnv: options.pathEnv ?? options.env?.PATH ?? options.env?.Path, pathExtEnv: options.pathExtEnv ?? options.env?.PATHEXT ?? options.env?.Pathext }); diff --git a/tests/process.test.mjs b/tests/process.test.mjs index 74e661257..9a02a2c16 100644 --- a/tests/process.test.mjs +++ b/tests/process.test.mjs @@ -165,6 +165,61 @@ test("resolveExecutablePath falls back to the bare command when nothing on PATH assert.equal(resolved, "missing-tool"); }); +// Regression tests for the P2 finding on PR #669: CreateProcess's own +// documented search sequence for a bare command name is the loading app's +// directory, then "the current directory for the parent process", then the +// system/Windows directories, then PATH -- so a cwd-local copy should win +// over one on PATH, matching what running the same bare command from that +// directory would find, instead of only ever considering PATH. +test("resolveExecutablePath searches cwd before PATH directories", () => { + const resolved = resolveExecutablePath("codex", { + platform: "win32", + cwd: "C:\\project", + pathEnv: "C:\\tools", + pathExtEnv: ".CMD", + existsSync: fakeExistsSync(["C:\\project\\codex.cmd", "C:\\tools\\codex.cmd"]) + }); + + assert.equal(resolved, "C:\\project\\codex.CMD"); +}); + +test("resolveExecutablePath falls through to PATH when cwd has no match", () => { + const resolved = resolveExecutablePath("codex", { + platform: "win32", + cwd: "C:\\project", + pathEnv: "C:\\tools", + pathExtEnv: ".CMD", + existsSync: fakeExistsSync(["C:\\tools\\codex.cmd"]) + }); + + assert.equal(resolved, "C:\\tools\\codex.CMD"); +}); + +test("resolveExecutablePath resolves a relative PATH entry against cwd", () => { + const resolved = resolveExecutablePath("codex", { + platform: "win32", + cwd: "C:\\project", + pathEnv: "vendor\\bin", + pathExtEnv: ".CMD", + existsSync: fakeExistsSync(["C:\\project\\vendor\\bin\\codex.cmd"]) + }); + + assert.equal(resolved, "C:\\project\\vendor\\bin\\codex.CMD"); +}); + +test("resolveSpawnInvocation threads options.cwd through to prefer a cwd-local executable", () => { + const invocation = resolveSpawnInvocation("codex", ["app-server"], { + platform: "win32", + cwd: "C:\\project", + pathEnv: "C:\\tools", + pathExtEnv: ".CMD", + comspec: "cmd.exe", + existsSync: fakeExistsSync(["C:\\project\\codex.cmd", "C:\\tools\\codex.cmd"]) + }); + + assert.equal(invocation.args[3], '"C:\\project\\codex.CMD ^"app-server^""'); +}); + test("runCommand still runs a real command end to end", () => { const result = runCommand(process.execPath, ["--version"]); From d3bc91cb41594a7572cbd581728a264801ca6618 Mon Sep 17 00:00:00 2001 From: Praveen Mittal Date: Sun, 23 Aug 2026 00:09:34 +0200 Subject: [PATCH 4/5] fix: honor NoDefaultCurrentDirectoryInExePath and read env vars case-insensitively The cwd-first search added in the previous commit unconditionally prepended cwd to the executable search path, but Windows' own NeedCurrentDirectoryForExePath (which cmd.exe and CreateProcess both consult) skips the current directory whenever NoDefaultCurrentDirectoryInExePath is present in the environment -- its mere presence disables the lookup, not its value. This variable exists specifically so a user or enterprise policy can opt out of current-directory executable lookup to stop a malicious file dropped into a working directory (e.g. an untrusted repo checkout, which is exactly what the app-server passes as cwd) from being executed just by resolving a bare command name there. Skip the cwd prepend when it's set; relative PATH entries still resolve against cwd regardless, since that's a separate mechanism unaffected by this variable. Windows environment variable names are also case-insensitive, but a plain JS options.env object is not: the resolver only ever checked PATH/Path and PATHEXT/Pathext, missing any other casing a caller might use even though spawn() itself builds the real (case-insensitive) environment block regardless of casing. Added a case-insensitive lookup helper and switched all PATH/PATHEXT/comspec reads from options.env to use it. Both found via Codex Review on the PR. --- plugins/codex/scripts/lib/process.mjs | 43 ++++++++++++++++--- tests/process.test.mjs | 62 +++++++++++++++++++++++++++ 2 files changed, 100 insertions(+), 5 deletions(-) diff --git a/plugins/codex/scripts/lib/process.mjs b/plugins/codex/scripts/lib/process.mjs index 76ada56d2..b57c1b543 100644 --- a/plugins/codex/scripts/lib/process.mjs +++ b/plugins/codex/scripts/lib/process.mjs @@ -5,6 +5,29 @@ import process from "node:process"; const DEFAULT_PATHEXT = ".COM;.EXE;.BAT;.CMD"; +/** + * Looks up an environment variable by name, case-insensitively. Windows + * environment variable names are case-insensitive, but a plain JS object + * (a caller-supplied `options.env`, as opposed to the running process's own + * `process.env`, which Node already exposes case-insensitively on win32) is + * not -- `spawn` builds the child's real (case-insensitive) environment + * block from it regardless of the casing used, so anything reading that + * same object needs to match by key name, not by one or two guessed + * casings. + */ +function getEnvValue(env, name) { + if (!env) { + return undefined; + } + const lowerName = name.toLowerCase(); + for (const key of Object.keys(env)) { + if (key.toLowerCase() === lowerName) { + return env[key]; + } + } + return undefined; +} + /** * Resolves `command` to a concrete file path on Windows, so its extension * can be inspected to decide how it needs to be spawned (see @@ -20,7 +43,15 @@ const DEFAULT_PATHEXT = ".COM;.EXE;.BAT;.CMD"; * directory the spawned process will actually run from, matching Node's * own `spawn`/`spawnSync` `cwd` option) is searched first here too, and * any relative PATH entry is resolved against it, to match what running - * the same bare command from that directory would actually find. + * the same bare command from that directory would actually find. Unless + * `NoDefaultCurrentDirectoryInExePath` is present in the environment (its + * mere presence disables the lookup, not its value -- this is what + * cmd.exe/CreateProcess themselves check), in which case `cwd` is skipped + * entirely: this variable exists specifically so a user or enterprise + * policy can opt out of current-directory executable lookup to prevent a + * malicious file dropped into a working directory (e.g. an untrusted repo + * checkout) from being executed just by resolving a bare command name + * there. */ export function resolveExecutablePath(command, options = {}) { const platform = options.platform ?? process.platform; @@ -37,12 +68,13 @@ export function resolveExecutablePath(command, options = {}) { const cwd = options.cwd ?? process.cwd(); const pathEnv = options.pathEnv ?? process.env.PATH ?? process.env.Path ?? ""; const pathExtEnv = options.pathExtEnv ?? process.env.PATHEXT ?? DEFAULT_PATHEXT; + const skipCwdLookup = getEnvValue(options.env, "NoDefaultCurrentDirectoryInExePath") !== undefined; const pathDirs = pathEnv .split(win.delimiter) .filter(Boolean) .map((dir) => (win.isAbsolute(dir) ? dir : win.resolve(cwd, dir))); - const dirs = [cwd, ...pathDirs]; + const dirs = skipCwdLookup ? pathDirs : [cwd, ...pathDirs]; const extensions = pathExtEnv .split(";") @@ -149,13 +181,14 @@ export function resolveSpawnInvocation(command, args, options = {}) { platform, existsSync: options.existsSync, cwd: options.cwd, - pathEnv: options.pathEnv ?? options.env?.PATH ?? options.env?.Path, - pathExtEnv: options.pathExtEnv ?? options.env?.PATHEXT ?? options.env?.Pathext + env: options.env, + pathEnv: options.pathEnv ?? getEnvValue(options.env, "PATH"), + pathExtEnv: options.pathExtEnv ?? getEnvValue(options.env, "PATHEXT") }); return buildSpawnCommand(resolvedCommand, args, { platform, - comspec: options.comspec ?? options.env?.comspec ?? options.env?.ComSpec + comspec: options.comspec ?? getEnvValue(options.env, "comspec") }); } diff --git a/tests/process.test.mjs b/tests/process.test.mjs index 9a02a2c16..e8b71d910 100644 --- a/tests/process.test.mjs +++ b/tests/process.test.mjs @@ -220,6 +220,68 @@ test("resolveSpawnInvocation threads options.cwd through to prefer a cwd-local e assert.equal(invocation.args[3], '"C:\\project\\codex.CMD ^"app-server^""'); }); +// Regression tests for the two newest findings on PR #669: cmd.exe/CreateProcess +// respect NoDefaultCurrentDirectoryInExePath (an enterprise/user opt-out of +// current-directory executable lookup, meant to stop a malicious file dropped +// into a working directory -- e.g. an untrusted repo checkout -- from being +// executed just by resolving a bare command name there) by its mere presence +// in the environment, not its value; and Windows environment variable names +// are case-insensitive, so a caller-supplied options.env object needs to be +// read that way too, since spawn() itself doesn't care what casing was used. +test("resolveExecutablePath skips the cwd search when NoDefaultCurrentDirectoryInExePath is present", () => { + const resolved = resolveExecutablePath("codex", { + platform: "win32", + cwd: "C:\\project", + pathEnv: "C:\\tools", + pathExtEnv: ".CMD", + env: { NoDefaultCurrentDirectoryInExePath: "" }, + existsSync: fakeExistsSync(["C:\\project\\codex.cmd", "C:\\tools\\codex.cmd"]) + }); + + assert.equal(resolved, "C:\\tools\\codex.CMD"); +}); + +test("resolveExecutablePath still resolves relative PATH entries against cwd when the opt-out is set", () => { + const resolved = resolveExecutablePath("codex", { + platform: "win32", + cwd: "C:\\project", + pathEnv: "vendor\\bin", + pathExtEnv: ".CMD", + env: { NoDefaultCurrentDirectoryInExePath: "1" }, + existsSync: fakeExistsSync(["C:\\project\\vendor\\bin\\codex.cmd"]) + }); + + assert.equal(resolved, "C:\\project\\vendor\\bin\\codex.CMD"); +}); + +test("resolveExecutablePath searches cwd normally when the opt-out is not set", () => { + const resolved = resolveExecutablePath("codex", { + platform: "win32", + cwd: "C:\\project", + pathEnv: "C:\\tools", + pathExtEnv: ".CMD", + env: {}, + existsSync: fakeExistsSync(["C:\\project\\codex.cmd", "C:\\tools\\codex.cmd"]) + }); + + assert.equal(resolved, "C:\\project\\codex.CMD"); +}); + +test("resolveSpawnInvocation reads PATH/PATHEXT/comspec from options.env case-insensitively", () => { + const invocation = resolveSpawnInvocation("codex", ["app-server"], { + platform: "win32", + env: { + path: "C:\\childpath", + pathext: ".CMD", + COMSPEC: "C:\\child\\cmd.exe" + }, + existsSync: fakeExistsSync(["C:\\childpath\\codex.CMD"]) + }); + + assert.equal(invocation.command, "C:\\child\\cmd.exe"); + assert.equal(invocation.windowsVerbatimArguments, true); +}); + test("runCommand still runs a real command end to end", () => { const result = runCommand(process.execPath, ["--version"]); From 8d94235a95b28f7696e61f91431d0e678781d24e Mon Sep 17 00:00:00 2001 From: Praveen Mittal Date: Mon, 24 Aug 2026 23:52:20 +0200 Subject: [PATCH 5/5] fix: fall back to process.env for the cwd-lookup opt-out check resolveExecutablePath's NoDefaultCurrentDirectoryInExePath check only ever consulted options.env, unlike the adjacent PATH/PATHEXT lookups which already fall back to process.env. The real production preflight (codex.mjs's binaryAvailable("codex", ..., { cwd }) calls) never passes options.env at all, so the opt-out was silently invisible on the one call path where it actually matters -- setup could still resolve and execute an untrusted repo-local codex.cmd despite the user or enterprise opt-out. Also fixes 4 existing tests that unknowingly relied on NoDefaultCurrentDirectoryInExePath being absent from the real environment; some hosts (e.g. WSL, which inherits some Windows env vars via interop) genuinely have it set, so tests exercising cwd-search ordering now pass an explicit empty env to isolate themselves from the ambient environment. Found via Codex Review on the PR. --- plugins/codex/scripts/lib/process.mjs | 2 +- tests/process.test.mjs | 37 +++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/plugins/codex/scripts/lib/process.mjs b/plugins/codex/scripts/lib/process.mjs index b57c1b543..ac0b1c2e3 100644 --- a/plugins/codex/scripts/lib/process.mjs +++ b/plugins/codex/scripts/lib/process.mjs @@ -68,7 +68,7 @@ export function resolveExecutablePath(command, options = {}) { const cwd = options.cwd ?? process.cwd(); const pathEnv = options.pathEnv ?? process.env.PATH ?? process.env.Path ?? ""; const pathExtEnv = options.pathExtEnv ?? process.env.PATHEXT ?? DEFAULT_PATHEXT; - const skipCwdLookup = getEnvValue(options.env, "NoDefaultCurrentDirectoryInExePath") !== undefined; + const skipCwdLookup = getEnvValue(options.env ?? process.env, "NoDefaultCurrentDirectoryInExePath") !== undefined; const pathDirs = pathEnv .split(win.delimiter) diff --git a/tests/process.test.mjs b/tests/process.test.mjs index e8b71d910..c7f357806 100644 --- a/tests/process.test.mjs +++ b/tests/process.test.mjs @@ -177,6 +177,11 @@ test("resolveExecutablePath searches cwd before PATH directories", () => { cwd: "C:\\project", pathEnv: "C:\\tools", pathExtEnv: ".CMD", + // Explicit empty env: this test exercises cwd-search ordering, not the + // NoDefaultCurrentDirectoryInExePath opt-out, so it must not fall + // through to the real process.env -- some hosts (e.g. WSL, which + // inherits some Windows env vars via interop) genuinely have this set. + env: {}, existsSync: fakeExistsSync(["C:\\project\\codex.cmd", "C:\\tools\\codex.cmd"]) }); @@ -189,6 +194,7 @@ test("resolveExecutablePath falls through to PATH when cwd has no match", () => cwd: "C:\\project", pathEnv: "C:\\tools", pathExtEnv: ".CMD", + env: {}, existsSync: fakeExistsSync(["C:\\tools\\codex.cmd"]) }); @@ -201,6 +207,7 @@ test("resolveExecutablePath resolves a relative PATH entry against cwd", () => { cwd: "C:\\project", pathEnv: "vendor\\bin", pathExtEnv: ".CMD", + env: {}, existsSync: fakeExistsSync(["C:\\project\\vendor\\bin\\codex.cmd"]) }); @@ -214,6 +221,8 @@ test("resolveSpawnInvocation threads options.cwd through to prefer a cwd-local e pathEnv: "C:\\tools", pathExtEnv: ".CMD", comspec: "cmd.exe", + // See the note on the resolveExecutablePath cwd-ordering test above. + env: {}, existsSync: fakeExistsSync(["C:\\project\\codex.cmd", "C:\\tools\\codex.cmd"]) }); @@ -241,6 +250,34 @@ test("resolveExecutablePath skips the cwd search when NoDefaultCurrentDirectoryI assert.equal(resolved, "C:\\tools\\codex.CMD"); }); +// Regression test for a 4th Codex Review finding on PR #669: the real +// production preflight (codex.mjs's binaryAvailable("codex", ..., { cwd }) +// calls) never passes options.env at all, so the opt-out check needs to +// fall back to the real process.env the same way the adjacent PATH/PATHEXT +// lookups already do -- checking only options.env silently never sees the +// opt-out on the actual call path that matters. +test("resolveExecutablePath honors NoDefaultCurrentDirectoryInExePath from process.env when options.env is not given", () => { + const previous = process.env.NoDefaultCurrentDirectoryInExePath; + process.env.NoDefaultCurrentDirectoryInExePath = "1"; + try { + const resolved = resolveExecutablePath("codex", { + platform: "win32", + cwd: "C:\\project", + pathEnv: "C:\\tools", + pathExtEnv: ".CMD", + existsSync: fakeExistsSync(["C:\\project\\codex.cmd", "C:\\tools\\codex.cmd"]) + }); + + assert.equal(resolved, "C:\\tools\\codex.CMD"); + } finally { + if (previous === undefined) { + delete process.env.NoDefaultCurrentDirectoryInExePath; + } else { + process.env.NoDefaultCurrentDirectoryInExePath = previous; + } + } +}); + test("resolveExecutablePath still resolves relative PATH entries against cwd when the opt-out is set", () => { const resolved = resolveExecutablePath("codex", { platform: "win32",