Merge remote-tracking branch origin/main (PRs #2, #5, #6)

Author: voidstackloop

app/src/agent-tools.test.ts

@@ -678,6 +678,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);
 
@@ -104,6 +104,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

@@ -158,12 +158,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, `'\\''`)}'`;
 }
 
@@ -180,5 +195,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(" ");
 }

app/src/json-store.test.ts

@@ -34,6 +34,34 @@ describe("json-store", () => {
         expect(entries).toEqual(["data.json"]);
     });
 
+    // These files hold API keys and conversation history; the default umask
+    // would leave them readable by other accounts on the machine.
+    it.skipIf(process.platform === "win32")("writes owner-only files", () => {
+        writeJson(file, { token: "value" });
+        expect(fs.statSync(file).mode & 0o777).toBe(0o600);
+    });
+
+    it.skipIf(process.platform === "win32")("keeps the file owner-only on rewrite", () => {
+        writeJson(file, { token: "first" });
+        writeJson(file, { token: "second" });
+        expect(fs.statSync(file).mode & 0o777).toBe(0o600);
+    });
+
+    it.skipIf(process.platform === "win32")("stays owner-only when a stale temp file exists", () => {
+        const stale = `${file}.tmp-${process.pid}`;
+        fs.writeFileSync(stale, "leftover", { mode: 0o666 });
+        writeJson(file, { token: "value" });
+        expect(fs.statSync(file).mode & 0o777).toBe(0o600);
+    });
+
+    // A file written by an older build is only ever read if its contents never
+    // change, so tightening on write alone would never reach it.
+    it.skipIf(process.platform === "win32")("tightens an existing world-readable file on read", () => {
+        fs.writeFileSync(file, JSON.stringify({ token: "value" }), { mode: 0o644 });
+        expect(readJson(file, {})).toEqual({ token: "value" });
+        expect(fs.statSync(file).mode & 0o777).toBe(0o600);
+    });
+
     it("backs up and falls back to the default when the file is corrupted", () => {
         fs.writeFileSync(file, "{ not valid json");
         const result = readJson(file, { safe: true });

app/src/json-store.ts

@@ -30,6 +30,8 @@ export function readJson<T>(filePath: string, fallback: T): T {
         return fallback;
     }
 
+    restrictExistingPermissions(filePath);
+
     try {
         return JSON.parse(raw) as T;
     } catch (err) {
@@ -43,9 +45,48 @@ export function readJson<T>(filePath: string, fallback: T): T {
     }
 }
 
+// These files hold provider API keys (secrets.json) and full conversation
+// history. Written with the process umask they land as 0644 — or 0664 under
+// the umask 002 several distributions ship — leaving their contents readable
+// to anything that reaches them: another account on a shared machine, a
+// backup or sync tool, an archive unpacked somewhere else. The userData
+// directory is usually restrictive enough to cover that on a single-user
+// desktop, but a stored credential shouldn't depend on its parent directory's
+// mode.
+const PRIVATE_FILE_MODE = 0o600;
+
+// Files written before this existed keep the mode they were created with, and
+// writeJson alone would never reach them: a key set once and never changed is
+// only ever read. So the mode is also tightened on first read, once per path
+// per run to keep it off the hot path.
+const permissionsChecked = new Set<string>();
+
+function restrictExistingPermissions(filePath: string): void {
+    if (process.platform === "win32" || permissionsChecked.has(filePath)) return;
+    permissionsChecked.add(filePath);
+    try {
+        if ((fs.statSync(filePath).mode & 0o777) !== PRIVATE_FILE_MODE) {
+            fs.chmodSync(filePath, PRIVATE_FILE_MODE);
+        }
+    } catch (err) {
+        // Best effort — unusual ownership or an exotic filesystem must not
+        // stop the app from reading its own data.
+        logger.error(`Failed to restrict permissions on ${filePath}: ${(err as Error).message}`);
+    }
+}
+
 export function writeJson(filePath: string, data: unknown): void {
     fs.mkdirSync(path.dirname(filePath), { recursive: true });
     const tmpPath = `${filePath}.tmp-${process.pid}`;
-    fs.writeFileSync(tmpPath, JSON.stringify(data, null, 2));
+    // The mode goes on the temp file, because the rename below replaces the
+    // destination inode and takes the temp file's mode with it. Chmod'ing the
+    // destination afterwards would leave a window where the contents are
+    // readable, and would be undone by the next write.
+    //
+    // Removed first so writeFileSync always creates the file and so always
+    // applies `mode` — it ignores the option for a path that already exists,
+    // and an interrupted earlier write can leave one behind under this pid.
+    fs.rmSync(tmpPath, { force: true });
+    fs.writeFileSync(tmpPath, JSON.stringify(data, null, 2), { mode: PRIVATE_FILE_MODE });
     fs.renameSync(tmpPath, filePath);
 }

frontend/src/lib/tool-approval.test.ts

@@ -0,0 +1,37 @@
+import { describe, it, expect } from "vitest";
+import { canAlwaysAllow } from "./tool-approval";
+
+describe("canAlwaysAllow", () => {
+    it("allows a standing grant for workspace-local reads", () => {
+        expect(canAlwaysAllow("read_file")).toBe(true);
+        expect(canAlwaysAllow("list_dir")).toBe(true);
+        expect(canAlwaysAllow("git_diff")).toBe(true);
+        expect(canAlwaysAllow("find_symbol_references")).toBe(true);
+    });
+
+    // A standing grant on these would let content the model reads steer an
+    // outbound request on every later turn with no prompt shown.
+    it("never allows a standing grant for tools that reach the network", () => {
+        expect(canAlwaysAllow("fetch_url")).toBe(false);
+        expect(canAlwaysAllow("web_search")).toBe(false);
+        expect(canAlwaysAllow("github_read_file")).toBe(false);
+        expect(canAlwaysAllow("github_list_repositories")).toBe(false);
+        expect(canAlwaysAllow("github_repository_tree")).toBe(false);
+    });
+
+    // Not read-only in either sense: it fetches a model-chosen URL and writes
+    // a PNG into the workspace.
+    it("never allows a standing grant for capture_page_screenshot", () => {
+        expect(canAlwaysAllow("capture_page_screenshot")).toBe(false);
+    });
+
+    it("never allows a standing grant for tools with side effects", () => {
+        for (const tool of ["write_file", "run_command", "run_code", "apply_patch", "delete_path", "git_commit", "http_request", "write_to_terminal"]) {
+            expect(canAlwaysAllow(tool)).toBe(false);
+        }
+    });
+
+    it("defaults to denying an unrecognised tool", () => {
+        expect(canAlwaysAllow("some_future_tool")).toBe(false);
+    });
+});

frontend/src/lib/tool-approval.ts

@@ -0,0 +1,40 @@
+// Which agent tools may be granted "always allow for this session".
+//
+// The bar for that grant is not "doesn't write to the workspace" — it's "the
+// user has nothing to lose by never being asked again". Those differ: a tool
+// can leave the workspace untouched and still reach the network or the disk.
+//
+// This matters because the model's inputs are not trusted. Agent mode reads
+// file contents and web pages, and instructions embedded in that content can
+// steer subsequent tool calls. The per-call approval prompt is what stops that
+// from becoming unattended action, so anything with a side effect the user
+// would want to see keeps its prompt. Notably that excludes:
+//
+//  - web_search, fetch_url and the github_* tools, which each send a request
+//    to a destination the model chooses — a standing grant on those is an
+//    unattended outbound channel for whatever the model has already read.
+//  - capture_page_screenshot, which fetches a model-chosen URL *and* writes a
+//    PNG into the workspace, so it is not read-only in either sense.
+//
+// Listing what qualifies, rather than what doesn't, keeps this default-deny: a
+// tool added to AGENT_TOOLS without being classified here needs per-call
+// approval until someone decides otherwise.
+export const AUTO_APPROVABLE_TOOLS = new Set([
+    "read_file",
+    "find_files",
+    "file_info",
+    "list_dir",
+    "search_files",
+    "git_status",
+    "git_diff",
+    "git_log",
+    "read_notes",
+    "get_background_output",
+    "list_background_commands",
+    "find_symbol_references",
+    "read_terminal_output",
+]);
+
+export function canAlwaysAllow(toolName: string): boolean {
+    return AUTO_APPROVABLE_TOOLS.has(toolName);
+}

frontend/src/pages/Chat.tsx

@@ -73,6 +73,7 @@ import { speakText, stopSpeaking } from "@/lib/tts";
 import { computeLineDiff } from "@/lib/diff";
 import { useToast } from "@/components/toast";
 import { isTransientError } from "@/lib/transient-errors";
+import { canAlwaysAllow } from "@/lib/tool-approval";
 import {
     COMPACTION_BUDGET_TOKENS,
     COMPACTION_KEEP_RECENT,
@@ -118,36 +119,12 @@ const RAG_THRESHOLD_CHARS = 20_000;
 // the most recent window renders by default, with older ones revealed a
 // window at a time on request rather than all at once.
 const RENDER_WINDOW_SIZE = 60;
-// Read-only tools are safe to let the model call repeatedly without a fresh
-// click each time — write_file and run_command always require explicit
-// per-call approval since they have real, potentially irreversible effects.
+
 interface PlanStep {
     text: string;
     done: boolean;
 }
 
-const READ_ONLY_TOOLS = new Set([
-    "read_file",
-    "find_files",
-    "file_info",
-    "list_dir",
-    "search_files",
-    "git_status",
-    "git_diff",
-    "git_log",
-    "web_search",
-    "fetch_url",
-    "read_notes",
-    "github_list_repositories",
-    "github_repository_tree",
-    "github_read_file",
-    "get_background_output",
-    "list_background_commands",
-    "capture_page_screenshot",
-    "find_symbol_references",
-    "read_terminal_output",
-]);
-
 // Vision models can already reason over any attached image — these just save
 // re-typing a good prompt for the common "I attached a diagram/wireframe"
 // case. Selecting one fills the composer; the user can still edit before sending.
@@ -2258,7 +2235,7 @@ export default function Chat() {
                                     >
                                         <X className="size-3.5" /> {t.deny}
                                     </Button>
-                                    {READ_ONLY_TOOLS.has(call.name) && (
+                                    {canAlwaysAllow(call.name) && (
                                         <Button
                                             size="sm"
                                             variant="ghost"