Mitigate Windows GH CLI command injection#1874
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
bot
added
size:XS
ApprovabilityVerdict: Needs human review This security fix changes runtime shell execution behavior on Windows. An open review comment raises a medium-severity concern that You can customize Macroscope's approvability policy. Learn more. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| runProcess("gh", input.args, { | ||
| cwd: input.cwd, | ||
| timeoutMs: input.timeoutMs ?? DEFAULT_TIMEOUT_MS, | ||
| shell: false, |
There was a problem hiding this comment.
shell:false breaks gh .cmd shims on Windows
Medium Severity
Setting shell: false for gh invocations on Windows prevents Node.js spawn from executing .cmd/.bat shims. If gh is installed via a package manager that creates .cmd shims (rather than placing gh.exe directly on PATH), the spawn call will fail with ENOENT. The original shell: true on Windows existed precisely to handle this — the openai/codex repo itself has issue #16337 about this exact pattern. While most gh installations use .exe, this is a regression for some Windows setups.
Reviewed by Cursor Bugbot for commit 1fd6244. Configure here.
- Keep `spawn` shell usage limited to Windows defaults - Avoid shell-based command execution on other platforms
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Missing parentheses in nullish coalescing expression
- Added explicit parentheses around
process.platform === "win32"to make the nullish coalescing precedence unambiguous.
- Added explicit parentheses around
Or push these changes by commenting:
@cursor push 05f0d160a1
Preview (05f0d160a1)
diff --git a/apps/server/src/processRunner.ts b/apps/server/src/processRunner.ts
--- a/apps/server/src/processRunner.ts
+++ b/apps/server/src/processRunner.ts
@@ -140,7 +140,7 @@
cwd: options.cwd,
env: options.env,
stdio: "pipe",
- shell: options.shell ?? process.platform === "win32",
+ shell: options.shell ?? (process.platform === "win32"),
});
let stdout = "";You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit ce1b679. Configure here.
| env: options.env, | ||
| stdio: "pipe", | ||
| shell: process.platform === "win32", | ||
| shell: options.shell ?? process.platform === "win32", |
There was a problem hiding this comment.
Missing parentheses in nullish coalescing expression
Low Severity
The expression options.shell ?? process.platform === "win32" relies on === having higher precedence than ?? to evaluate correctly. While functionally equivalent to options.shell ?? (process.platform === "win32"), the PR description itself explicitly includes the parentheses. Adding them would make the intent unambiguous, especially since this is a security-sensitive change preventing command injection. Developers unfamiliar with ?? precedence may misread this as (options.shell ?? process.platform) === "win32".
Reviewed by Cursor Bugbot for commit ce1b679. Configure here.
|
Bugbot Autofix prepared a fix for the issue found in the latest run.
Or push these changes by commenting: Preview (7885e2ba38)diff --git a/apps/server/src/processRunner.ts b/apps/server/src/processRunner.ts
--- a/apps/server/src/processRunner.ts
+++ b/apps/server/src/processRunner.ts
@@ -1,4 +1,6 @@
import { type ChildProcess as ChildProcessHandle, spawn, spawnSync } from "node:child_process";
+import { accessSync } from "node:fs";
+import { delimiter, extname, join } from "node:path";
export interface ProcessRunOptions {
cwd?: string | undefined;
@@ -82,6 +84,45 @@
const DEFAULT_MAX_BUFFER_BYTES = 8 * 1024 * 1024;
/**
+ * On Windows, `.cmd` / `.bat` shims (created by npm, Scoop, etc.) can only be
+ * executed by `spawn` when `shell: true`. When the caller explicitly requests
+ * `shell: false` we still need to honour that for `.exe` binaries, but we must
+ * fall back to `shell: true` when the resolved command is a script shim.
+ */
+function resolveShellOption(command: string, explicit: boolean | undefined): boolean {
+ if (process.platform !== "win32") return explicit ?? false;
+ if (explicit === true) return true;
+
+ if (extname(command)) {
+ const ext = extname(command).toLowerCase();
+ return ext === ".cmd" || ext === ".bat";
+ }
+
+ const pathEnv = process.env["PATH"] ?? process.env["Path"] ?? "";
+ const dirs = pathEnv.split(delimiter).filter(Boolean);
+ for (const dir of dirs) {
+ for (const ext of [".cmd", ".bat"]) {
+ try {
+ accessSync(join(dir, `${command}${ext}`));
+ return true;
+ } catch {
+ // not found, continue
+ }
+ }
+ for (const ext of [".exe", ".com", ""]) {
+ try {
+ accessSync(join(dir, `${command}${ext}`));
+ return false;
+ } catch {
+ // not found, continue
+ }
+ }
+ }
+
+ return explicit ?? false;
+}
+
+/**
* On Windows with `shell: true`, `child.kill()` only terminates the `cmd.exe`
* wrapper, leaving the actual command running. Use `taskkill /T` to kill the
* entire process tree instead.
@@ -140,7 +181,7 @@
cwd: options.cwd,
env: options.env,
stdio: "pipe",
- shell: options.shell ?? (process.platform === "win32"),
+ shell: resolveShellOption(command, options.shell),
});
let stdout = "";You can send follow-ups to the cloud agent here. |
1 participant
Motivation
runProcesshelper enabledshell: process.platform === "win32", which routes commands throughcmd.exeon Windows and allows shell metacharacters in untrustedgharguments (branch names, titles, body file paths) to inject commands.Description
shell?: booleanfield toProcessRunOptionsso callers can explicitly control shell use viaoptions.shell.runProcessto useshell: options.shell ?? (process.platform === "win32")so the previous Windows default is preserved unless a caller overrides it.ghinvocations in the GitHub CLI layer to run withshell: falsesoghreceives arguments directly andcmd.execannot interpret untrusted input.Testing
bun fmtbut it could not run in this environment (bun: command not found) so formatting was not verified automatically.bun lintbut it could not run in this environment (bun: command not found) so lint checks were not executed.bun typecheckbut it could not run in this environment (bun: command not found) so typechecks were not executed.Codex Task
Note
Medium Risk
Touches process execution behavior and security boundaries:
runProcessnow supports overriding shell usage and GitHub CLI calls explicitly disable shell, which could affect Windows-specific invocation semantics if any callers relied on shell parsing.Overview
Mitigates a Windows command-injection vector by adding an optional
shellflag torunProcessand wiring it into thespawnoptions (options.shell ?? (process.platform === "win32")).Updates the GitHub CLI layer (
GitHubCli.ts) to always runghwithshell: false, ensuring untrusted arguments are passed directly to the executable rather than interpreted bycmd.exe.Reviewed by Cursor Bugbot for commit 1fd6244. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Mitigate Windows CLI command injection by disabling shell in GitHub CLI process spawning
shellproperty to theProcessRunOptionsinterface in processRunner.ts, letting callers override the default shell behavior.runProcessto useoptions.shell ?? process.platform === 'win32'so existing callers retain current behavior.shell: falsefor all GitHub CLI invocations in GitHubCli.ts, preventing shell interpolation of arguments on Windows.ghinvocations that relied on shell path resolution, though this is the intended security trade-off.Macroscope summarized ce1b679.