From c0d1780911919b2ec2d5846cd89c4d0450fe5965 Mon Sep 17 00:00:00 2001 From: ConsultingFuture4200 Date: Sat, 25 Jul 2026 09:59:27 -0700 Subject: [PATCH] fix(agent): stop git_diff and git_commit arguments reaching the shell gitDiff interpolated the model-supplied path into `git diff -- ""` and gitCommit interpolated the message into `git commit -m ""`. Both strings are executed via `sh -c`, and a POSIX shell still expands `$(...)` and backticks inside double quotes, so either argument could run arbitrary commands. JSON.stringify escapes `"` and `\` but not `$` or backticks, so it is not a shell-quoting function. git_diff is offered as a read-only tool the user can grant "always allow this session", so this turned a standing approval for reading diffs into one for arbitrary execution, with no further prompt. Both values now go through a shared quoting helper in command-sandbox.ts, which is platform-aware: single quotes for POSIX shells, double quotes for cmd.exe (where `'` is not a quote character at all, and where there is no `$(...)` or backtick expansion to defend against). Quoting Windows arguments POSIX-style would have split ordinary paths and messages on their spaces. --- app/src/agent-tools.test.ts | 26 ++++++++++++++++++++++++++ app/src/agent-tools.ts | 8 +++++--- app/src/command-sandbox.test.ts | 23 ++++++++++++++++++++++- app/src/command-sandbox.ts | 29 ++++++++++++++++++++++------- 4 files changed, 75 insertions(+), 11 deletions(-) diff --git a/app/src/agent-tools.test.ts b/app/src/agent-tools.test.ts index cbb4031..3247868 100644 --- a/app/src/agent-tools.test.ts +++ b/app/src/agent-tools.test.ts @@ -596,6 +596,32 @@ describe("agent-tools", () => { expect(log).toContain("initial commit"); }); + // `git diff -- ""` and `git commit -m ""` are assembled + // into a string that ends up at `sh -c`. Double quotes do not stop the + // shell from expanding `$(...)`, so a model-supplied path or message + // used to be able to run arbitrary commands — including when the user + // had granted git_diff "always allow" as a read-only tool. + it("git_diff does not let a path argument reach the shell", async () => { + const marker = path.join(workspace, "diff-injection-marker"); + await gitDiff(workspace, false, `.$(touch ${marker})`); + expect(fs.existsSync(marker)).toBe(false); + }); + + it("git_commit does not let a commit message reach the shell", async () => { + const marker = path.join(workspace, "commit-injection-marker"); + await gitCommit(workspace, `initial $(touch ${marker})`); + expect(fs.existsSync(marker)).toBe(false); + }); + + it("git_commit preserves a message containing shell metacharacters", async () => { + const message = "fix: handle $HOME and `backticks` and 'quotes' and \"doubles\""; + await gitCommit(workspace, message); + const log = await gitLog(workspace, 5); + // The whole subject, not just its prefix — quoting that drops or + // mangles part of the message is as wrong as quoting that executes it. + expect(log).toContain(message); + }); + it("git_log returns nothing unusual with no commits yet", async () => { const output = await gitLog(workspace); expect(output).toContain("Exit code:"); diff --git a/app/src/agent-tools.ts b/app/src/agent-tools.ts index 94e8a08..d8b29f2 100644 --- a/app/src/agent-tools.ts +++ b/app/src/agent-tools.ts @@ -8,7 +8,7 @@ import type { ToolDefinition } from "./providers/types"; import { getAccountToken } from "./accounts"; import { capturePageScreenshot } from "./browser-capture"; import { killProcessTree } from "./process-tree"; -import { applySandbox } from "./command-sandbox"; +import { applySandbox, shellQuote } from "./command-sandbox"; import { monitorProcess } from "./resource-monitor"; import * as settingsStore from "./settings-store"; import { resolveSafePath } from "./workspace-path"; @@ -1028,7 +1028,7 @@ export function gitStatus(workspaceRoot: string): Promise { } export function gitDiff(workspaceRoot: string, staged = false, relativePath?: string): Promise { - const target = relativePath ? ` -- "${relativePath}"` : ""; + const target = relativePath ? ` -- ${shellQuote(relativePath)}` : ""; return gitCommand(workspaceRoot, `diff${staged ? " --staged" : ""}${target}`); } @@ -1038,7 +1038,9 @@ export function gitLog(workspaceRoot: string, count = 10): Promise { export async function gitCommit(workspaceRoot: string, message: string): Promise { await gitCommand(workspaceRoot, "add -A"); - return gitCommand(workspaceRoot, `commit -m ${JSON.stringify(message)}`); + // JSON.stringify escapes `"` and `\` but not `$` or backticks, and the + // result is handed to `sh -c` — so it is not a shell-quoting function. + return gitCommand(workspaceRoot, `commit -m ${shellQuote(message)}`); } const WEB_FETCH_TIMEOUT_MS = 15_000; diff --git a/app/src/command-sandbox.test.ts b/app/src/command-sandbox.test.ts index 1a1a8de..e8766e5 100644 --- a/app/src/command-sandbox.test.ts +++ b/app/src/command-sandbox.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from "vitest"; -import { detectSandboxCapabilities, wrapCommand, applySandbox } from "./command-sandbox"; +import { detectSandboxCapabilities, wrapCommand, applySandbox, shellQuote } from "./command-sandbox"; const has = (available: string[]) => (cmd: string) => available.includes(cmd); @@ -81,6 +81,27 @@ describe("wrapCommand", () => { }); }); +describe("shellQuote", () => { + it("single-quotes for POSIX shells so substitutions stay inert", () => { + expect(shellQuote("simple.txt", "linux")).toBe("'simple.txt'"); + // The whole point: a POSIX shell expands these inside double quotes. + expect(shellQuote("$(id)", "linux")).toBe("'$(id)'"); + expect(shellQuote("`id`", "linux")).toBe("'`id`'"); + expect(shellQuote("it's", "linux")).toBe("'it'\\''s'"); + expect(shellQuote("", "linux")).toBe("''"); + }); + + // cmd.exe does not treat ' as a quote character, so POSIX quoting there + // would split ordinary arguments on their spaces instead of protecting + // them. It has no $(...) or backtick substitution to defend against. + it("double-quotes for cmd.exe, where single quotes are not quoting", () => { + expect(shellQuote("a message with spaces", "win32")).toBe('"a message with spaces"'); + expect(shellQuote('say "hi"', "win32")).toBe('"say ""hi"""'); + expect(shellQuote("a & b", "win32")).toBe('"a & b"'); + expect(shellQuote("", "win32")).toBe('""'); + }); +}); + describe("applySandbox", () => { it("returns the command unchanged when no sandbox mechanism is available", () => { expect(applySandbox("echo hi", { workspaceRoot: "/ws", allowNetwork: false }, "win32", has(["bwrap"]))).toBe("echo hi"); diff --git a/app/src/command-sandbox.ts b/app/src/command-sandbox.ts index e604ca2..64ea858 100644 --- a/app/src/command-sandbox.ts +++ b/app/src/command-sandbox.ts @@ -149,12 +149,27 @@ function buildMacSandboxProfile(workspaceRoot: string, allowNetwork: boolean): s ].join("\n"); } -// POSIX shell single-quoting: wraps in '...', escaping any embedded single -// quote as '\''. Used to fold a wrapped {command, args} back into the single -// shell-command string that `child_process.exec`/`spawn(..., {shell:true})` -// expect, without needing to change how the rest of agent-tools.ts invokes -// commands. -function shellQuote(arg: string): string { +// Quotes a single argument so the shell that will run it treats it as one +// literal value. Used both to fold a wrapped {command, args} back into the +// single shell-command string that `child_process.exec`/`spawn(..., +// {shell:true})` expect, and by agent-tools.ts when it builds a fixed command +// around a *value* the model supplied (a path for `git diff`, a message for +// `git commit`). +// +// The two shells need different treatment, and getting this wrong in either +// direction is a bug: +// +// - POSIX `sh`: single quotes, with an embedded quote written as '\''. +// Double quotes would not be enough, because `$(...)` and backticks are +// still expanded inside them. +// - Windows `cmd.exe`: single quotes are not quote characters at all, so +// POSIX quoting there would corrupt ordinary arguments rather than protect +// them. Double quotes are the right tool: `&`, `|`, `<` and `>` are +// literal inside them, and `$(...)`/backticks mean nothing to cmd.exe. +// An embedded double quote is written as "" — the convention both cmd.exe +// and the argv parser of the program being launched understand. +export function shellQuote(arg: string, platform: NodeJS.Platform = process.platform): string { + if (platform === "win32") return `"${arg.replace(/"/g, '""')}"`; return `'${arg.replace(/'/g, `'\\''`)}'`; } @@ -171,5 +186,5 @@ export function applySandbox( ): string { const wrapped = wrapCommand(command, opts, platform, hasCommand); if (!wrapped) return command; - return [wrapped.command, ...wrapped.args].map(shellQuote).join(" "); + return [wrapped.command, ...wrapped.args].map((arg) => shellQuote(arg, platform)).join(" "); }