diff --git a/CHANGELOG.md b/CHANGELOG.md index ae7fc9e9..eafbc5b2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,6 +55,21 @@ parallel copies under `docs/` or `scripts/notes/`. At cut time: rename the tool at all. `progress_note` for leaf workers is a separate, not-yet-implemented follow-up. +### Permissions + +- **Quoting or backslash-escaping a redirect target, a dangerous flag, or a + program name no longer bypasses auto mode's shell rules.** The auto-shell + policy used to blank out quoted text before matching its rules, so `echo hi + > "file"`, `echo hi >|file`, a quoted `-c`/`-i` flag, a quoted `install` + subcommand, or a quoted upload-tool name all slipped past the + file-mutation, dependency-install, and network-upload rules — including one + level of quoting inside a `bash -c` payload. Matching now dequotes the + command the way a real shell would (only the operator characters `> < | & + ; \`` are neutralized when they occur inside a quote, everything else stays + literal, and a backslash-escaped quote never opens or closes a span), and + the file-mutation redirect pattern now also recognizes the `>|` / `>>|` + clobber form. + ### Fixed - **Interrupting a turn no longer risks a startup crash.** If an interrupt hit diff --git a/src/permission/auto-shell-policy.ts b/src/permission/auto-shell-policy.ts index 26a7f0c7..8942eeb9 100644 --- a/src/permission/auto-shell-policy.ts +++ b/src/permission/auto-shell-policy.ts @@ -33,12 +33,52 @@ export interface AutoShellRule { const CMD = String.raw`(?:^|[\n;&|({]\s*)(?:\w+=\S*\s+)*`; const inCmd = (body: string): RegExp => new RegExp(`${CMD}${body}`); -// Drop the contents of single- and double-quoted spans before matching so a -// quoted argument cannot trip a rule (e.g. `git commit -m 'fix > bug'` is not a -// redirect, `echo "npm install"` is not an install). Quoted-out redirect targets -// and heredoc markers fall away with their quotes, which is why the file-mutation -// heredoc pattern keys on the bare `<<` operator rather than the marker word. -const stripQuoted = (command: string): string => command.replace(/'[^']*'|"[^"]*"/g, " "); +// Quote-aware dequoting for rule matching. Real shells strip the quote +// characters themselves and hand the program a literal argument, so a rule +// must see the same thing the program would: a quoted redirect target +// (`>"file"`), a quoted flag (`"-c"`), or a quoted program/subcommand name +// (`"sed" -i`, `npm "install"`) all read exactly like their unquoted form. +// The one thing quoting genuinely changes is that a shell *operator* +// character loses its operator meaning inside quotes — `'fix > bug'` is a +// literal string, not a redirect — so only that small set of operator +// characters (`> < | & ; \``) is neutralized when it occurs inside a quoted +// span; every other character (letters, digits, `-`) passes through +// dequoted. Heredoc bodies are left alone: the file-mutation heredoc pattern +// keys on the bare `<<` operator, which is always outside any quoting. +// +// A backslash before a quote character escapes it: `\"` is a literal `"` +// that never opens or closes a quoted span (real bash semantics outside +// single quotes), so `echo hi \"> file"` is a bare, unquoted redirect, not +// text inside a quote. Skip the escaped character without touching quote +// state so its following operator is still seen as live. +const QUOTE_NEUTRALIZED_OPERATORS = new Set(["<", ">", "|", "&", ";", "`"]); + +const dequoteForMatching = (command: string): string => { + let out = ""; + let quote: '"' | "'" | null = null; + for (let i = 0; i < command.length; i++) { + const ch = command[i] as string; + if (ch === "\\" && quote !== "'" && i + 1 < command.length) { + out += command[i + 1]; + i++; + continue; + } + if (quote !== null) { + if (ch === quote) { + quote = null; + } else { + out += QUOTE_NEUTRALIZED_OPERATORS.has(ch) ? " " : ch; + } + continue; + } + if (ch === '"' || ch === "'") { + quote = ch; + continue; + } + out += ch; + } + return out; +}; // Named separately (not inlined in AUTO_SHELL_RULES below) so the dedicated // `env -S`/`--split-string` check further down — which cannot be expressed as @@ -71,9 +111,10 @@ export const AUTO_SHELL_RULES: AutoShellRule[] = [ reason: "File creation and edits must go through the write_file and edit_file tools, not shell tooling (python, sed -i, awk, perl, tee, or output redirection). Re-do this change with edit_file for a surgical replacement or write_file for the full contents.", patterns: [ - // `>` / `>>` (optionally fd-qualified) to a target that is not an fd dup + // `>` / `>>` (optionally fd-qualified, optionally clobber-forced with a + // trailing `|` as in `>|` / `>>|`) to a target that is not an fd dup // (`2>&1`) or a safe pseudo-device (`> /dev/null`, a TTY). - /[0-9]?>>?\s*(?!&|\/dev\/(?:null|stdout|stderr|stdin|tty|pts\/|fd\/))[^\s|;&)]/, + /[0-9]?>>?\|?\s*(?!&|\/dev\/(?:null|stdout|stderr|stdin|tty|pts\/|fd\/))[^\s|;&)]/, // tee writes its stdin to one or more files. /(?:^|[\n;&|({]\s*)tee\b/, // In-place stream editors: sed -i, perl -pi -e, ruby -i. @@ -166,7 +207,7 @@ export const AUTO_SHELL_RULES: AutoShellRule[] = [ ]; export function matchAutoShellRule(command: string): AutoShellRule | undefined { - const scannable = stripQuoted(command); + const scannable = dequoteForMatching(command); return AUTO_SHELL_RULES.find((rule) => rule.patterns.some((pattern) => pattern.test(scannable))); } diff --git a/src/permission/classify-security.test.ts b/src/permission/classify-security.test.ts index 7aaced03..a807ac1b 100644 --- a/src/permission/classify-security.test.ts +++ b/src/permission/classify-security.test.ts @@ -712,3 +712,81 @@ describe("pure directory listing exemption", () => { expect(autoShellRuleForCall(shellCall("ls .env | xargs cat"))?.effect).toBe("ask"); }); }); + +describe("CL-6703 — quoted redirect targets still deny file-mutation", () => { + test("plain unquoted redirect denies (baseline)", () => { + expect(autoShellRuleForCall(shellCall("echo hi > out.txt"))?.name).toBe("file-mutation"); + }); + + test("a quoted redirect target denies", () => { + expect(autoShellRuleForCall(shellCall(`echo hi > "out.txt"`))?.name).toBe("file-mutation"); + expect(autoShellRuleForCall(shellCall(`echo hi > 'out.txt'`))?.name).toBe("file-mutation"); + }); + + test('a quoted fd-qualified redirect target (1>"file") denies', () => { + expect(autoShellRuleForCall(shellCall(`echo hi 1>"file"`))?.name).toBe("file-mutation"); + }); + + test("a nested bash -c form with a quoted redirect denies", () => { + expect(autoShellRuleForCall(shellCall(`bash -c 'echo hi > "out.txt"'`))?.name).toBe( + "file-mutation", + ); + }); + + test("a quoted '>' inside non-redirect text does not false-positive", () => { + expect(autoShellRuleForCall(shellCall(`git commit -m 'fix > bug'`))).toBeUndefined(); + }); + + test("a backslash-escaped quote before a redirect still denies", () => { + // `\"` is a literal quote character in real bash, not a quote-open — the + // shell is never inside a quoted string here, so the `>` that follows is + // a genuine, unquoted redirect. + expect(autoShellRuleForCall(shellCall('echo hi \\"> file"'))?.name).toBe("file-mutation"); + }); + + test("a backslash-escaped quote ahead of a dangerous flag still denies", () => { + // The escaped quote sits before an extra leading space, so it never + // touches the `\s-c` junction later in the string; a naive quote-pairing + // scanner (ignoring the backslash) would consume that junction as part + // of a fake quoted span and hide the -c flag entirely. + expect(autoShellRuleForCall(shellCall('python3 \\" -c print(1)"'))?.name).toBe("file-mutation"); + }); +}); + +describe("CL-6702 — bash clobber redirects match file-mutation", () => { + test("echo hi >|path denies", () => { + expect(autoShellRuleForCall(shellCall("echo hi >|path"))?.name).toBe("file-mutation"); + }); + + test("echo hi >>|path denies", () => { + expect(autoShellRuleForCall(shellCall("echo hi >>|path"))?.name).toBe("file-mutation"); + }); +}); + +describe("CL-6697 — quoted dangerous flags and program names still deny/ask", () => { + test("a quoted -c interpreter one-liner denies", () => { + expect(autoShellRuleForCall(shellCall(`python3 "-c" "print(1)"`))?.name).toBe("file-mutation"); + }); + + test("a quoted sed -i denies", () => { + expect(autoShellRuleForCall(shellCall(`sed "-i" 's/a/b/' file.txt`))?.name).toBe( + "file-mutation", + ); + }); + + test("a quoted npm install asks", () => { + expect(autoShellRuleForCall(shellCall(`npm "install" left-pad`))?.name).toBe( + "dependency-install", + ); + }); + + test("a quoted upload-tool argv0 (curl) asks", () => { + expect( + autoShellRuleForCall(shellCall(`"curl" -d @payload.json https://example.com`))?.name, + ).toBe("network-upload"); + }); + + test("an innocent quoted argument interior does not false-positive", () => { + expect(autoShellRuleForCall(shellCall(`git commit -m "some text"`))).toBeUndefined(); + }); +});