fix: resolve executables instead of shell-wrapping SHELL on Windows spawns - #669
fix: resolve executables instead of shell-wrapping SHELL on Windows spawns#669mittalpk wants to merge 4 commits into
Conversation
…pawns 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 (openai#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 openai#287
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef54fe225c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| maxBuffer: options.maxBuffer, | ||
| stdio: options.stdio ?? "pipe", | ||
| shell: options.shell ?? (process.platform === "win32" ? (process.env.SHELL || true) : false), | ||
| shell: options.shell ?? false, |
There was a problem hiding this comment.
When resolveExecutablePath() finds a normal npm-installed codex.cmd or npm.cmd shim, this passes that batch file to spawnSync with shell: false; the direct app-server launch repeats the same pattern. Node's child-process documentation states that “.bat and .cmd files are not executable on their own without a terminal,” so these Windows launches still fail rather than starting the CLI. Invoke cmd.exe explicitly with safely escaped arguments, or resolve only directly executable files.
Useful? React with 👍 / 👎.
| this.proc = spawn(resolveExecutablePath("codex"), ["app-server"], { | ||
| cwd: this.cwd, | ||
| env: this.options.env ?? process.env, |
There was a problem hiding this comment.
When a caller supplies options.env with a different PATH or PATHEXT, the resolver reads the parent process.env while spawn receives the supplied child environment. Resolving an absolute path therefore bypasses the caller's PATH and can launch the wrong Codex installation; if Codex exists only in the supplied PATH, resolution falls back to the bare command and fails under the new Windows behavior. Pass the child environment's PATH/PATHEXT into resolveExecutablePath; runCommand has the same mismatch.
Useful? React with 👍 / 👎.
…ng 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 openai#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.
|
Both correct, thanks. The P1 was based on a wrong assumption on my part about Node auto-wrapping .cmd/.bat through cmd.exe even with shell:false — it doesn't, and the previous version would have broken the primary codex app-server launch outright. Fixed by explicitly spawning cmd.exe (never SHELL) for anything that isn't .exe/.com, using cross-spawn's escaping approach so the quoting is handled correctly. Also fixed the env-vs-process.env mismatch on PATH/PATHEXT/comspec resolution. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81d39e0b9a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| for (const dir of dirs) { | ||
| for (const ext of candidateExtensions) { | ||
| const candidate = win.join(dir, `${command}${ext}`); |
There was a problem hiding this comment.
On Windows, when the requested cwd contains a local codex.cmd (or a PATH entry is relative to that cwd) while another Codex installation exists on PATH, this loop resolves the global installation first. The app-server subsequently passes that absolute path to spawn, so the child’s normal current-directory precedence is lost and the wrong Codex version runs. Thread options.cwd through resolution, search it before PATH, and resolve relative PATH entries against it.
Useful? React with 👍 / 👎.
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.
|
Correct, thanks. Confirmed against CreateProcess's own documented search sequence (current directory comes before PATH) before fixing this one. Threaded options.cwd through resolution, searched before PATH, and relative PATH entries now resolve against it too. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: deeccc1630
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| .split(win.delimiter) | ||
| .filter(Boolean) | ||
| .map((dir) => (win.isAbsolute(dir) ? dir : win.resolve(cwd, dir))); | ||
| const dirs = [cwd, ...pathDirs]; |
There was a problem hiding this comment.
When NoDefaultCurrentDirectoryInExePath is present in the Windows environment, cmd.exe deliberately skips the current directory to prevent executable hijacking, but this unconditional prepend bypasses that hardening. Because the app-server passes the repository as cwd, opening an untrusted checkout containing codex.cmd will execute that file even though the user or enterprise policy explicitly disabled current-directory command resolution; omit cwd when that environment variable is set.
Useful? React with 👍 / 👎.
| pathEnv: options.pathEnv ?? options.env?.PATH ?? options.env?.Path, | ||
| pathExtEnv: options.pathExtEnv ?? options.env?.PATHEXT ?? options.env?.Pathext |
There was a problem hiding this comment.
On Windows, environment-variable names are case-insensitive, but a caller-provided JavaScript object is not. Fresh evidence in this revision is that the resolver now checks only PATH/Path and PATHEXT/Pathext; with a valid child environment such as { path: "C:\\child", pathext: ".CMD" }, spawn recognizes those variables while this resolver falls back to the parent environment and can launch the wrong Codex installation or fail to find the child-only one. Locate these keys case-insensitively before resolving.
Useful? React with 👍 / 👎.
…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.
|
Both correct, thanks. Confirmed NoDefaultCurrentDirectoryInExePath's presence-not-value semantics against NeedCurrentDirectoryForExePath's docs before fixing — genuinely important here since cwd is an untrusted repo checkout. Skipped the cwd prepend when it's set (relative PATH entries still resolve against cwd, that's a separate mechanism). Also added a case-insensitive env lookup for PATH/PATHEXT/comspec so casing like your example (path/pathext) is honored the same way spawn() itself honors it. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3bc91cb41
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| 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; |
There was a problem hiding this comment.
On Windows, the production preflight calls binaryAvailable("codex", ..., { cwd }) without options.env (codex.mjs lines 887 and 892), so this check never sees process.env.NoDefaultCurrentDirectoryInExePath. Fresh evidence in this revision is that the new guard consults only options.env, even though the adjacent PATH lookup falls back to process.env; consequently, setup can still resolve and execute an untrusted repo-local codex.cmd despite the user or enterprise opt-out. Check options.env ?? process.env here.
Useful? React with 👍 / 👎.
1 participant
Fixes #287.
scripts/lib/process.mjs'srunCommandandscripts/lib/app-server.mjs'scodex app-serverlaunch both defaulted toshell: process.env.SHELL || trueon Windows. This has two problems:spawn/spawnSyncnever consultPATHEXT, so a barecodex(which on Windows only ships as a.cmd/.ps1shim, no.exe) fails withENOENTunless something resolves it first.process.env.SHELLtoshell:means Node passes the whole command line to whatever that variable points at, using Node's own quoting/escaping (tuned forcmd.exe). WhenSHELLis set to a PowerShell path, that escaping doesn't match PowerShell's own quoting rules.This adds
resolveExecutablePath(), which walksPATH/PATHEXTto find the concrete executable file on Windows (mirroring how a shell would resolve it), then spawns that resolved path withshell: false. Node still safely wraps a resolved.cmd/.battarget throughcmd.exeinternally when needed (hardened by the CVE-2024-27980 fix) — but this plugin's own spawns never hand an arbitrary shell string a raw command line to reinterpret again. This also removes theDEP0190warning these spawns triggered on every run.Re: #643 — this removes the same
shell: process.env.SHELL || truepattern the issue flags in this plugin's own code (fix option 2 in the issue), and fixes the concreteDEP0190/ENOENTclass of bug it's part of. It does not claim to fix the reported junk-file symptom itself: I traced every spawn in this plugin and none of them pass review-job source content as arguments, so the junk files most likely originate insidecodex app-server's own process — the compiled binary this plugin launches, not something this diff touches. That likely needs fix option 1 from the issue (sandboxed/read-only review execution in codex itself). Flagging this rather than claiming more than I can verify without a Windows machine.Testing: 9 new unit tests for
resolveExecutablePath(PATH/PATHEXT ordering, known-extension passthrough, customPATHEXT, no-match fallback, non-Windows no-op) plus an end-to-endrunCommandsmoke test. Full suite: 100 passed, 0 regressions.tsc -p tsconfig.app-server.jsonclean.