Merge pull request #2 from ConsultingFuture4200/fix/git-tool-shell-injection

fix(agent): stop git_diff and git_commit arguments reaching the shell

Author: voidstackloop

app/src/agent-tools.test.ts

@@ -596,6 +596,32 @@ describe("agent-tools", () => {
             expect(log).toContain("initial commit");
         });
 
+        // `git diff -- "<path>"` and `git commit -m "<message>"` 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:");

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<string> {
 }
 
 export function gitDiff(workspaceRoot: string, staged = false, relativePath?: string): Promise<string> {
-    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<string> {
 
 export async function gitCommit(workspaceRoot: string, message: string): Promise<string> {
     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;

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");

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(" ");
 }