From 34ec2aaa4ecc87571a3315e1e00344288a69c754 Mon Sep 17 00:00:00 2001 From: Charles Packer Date: Thu, 26 Feb 2026 22:40:09 -0800 Subject: [PATCH] fix(plan): merge scoped path relaxation with quote-aware shell parsing (#1188) --- src/permissions/mode.ts | 6 +- src/permissions/readOnlyShell.ts | 142 +++++++++++++++++--- src/tests/permissions-mode.test.ts | 47 +++++++ src/tests/permissions/readOnlyShell.test.ts | 40 ++++++ 4 files changed, 215 insertions(+), 20 deletions(-) diff --git a/src/permissions/mode.ts b/src/permissions/mode.ts index 52d0d3c..f71177f 100644 --- a/src/permissions/mode.ts +++ b/src/permissions/mode.ts @@ -203,6 +203,7 @@ class PermissionModeManager { "Grep", "NotebookRead", "TodoWrite", + "TaskOutput", // Plan mode tools (must allow exit!) "ExitPlanMode", "exit_plan_mode", @@ -326,7 +327,10 @@ class PermissionModeManager { ]; if (shellTools.includes(toolName)) { const command = toolArgs?.command as string | string[] | undefined; - if (command && isReadOnlyShellCommand(command)) { + if ( + command && + isReadOnlyShellCommand(command, { allowExternalPaths: true }) + ) { return "allow"; } } diff --git a/src/permissions/readOnlyShell.ts b/src/permissions/readOnlyShell.ts index 4d224ea..2532e12 100644 --- a/src/permissions/readOnlyShell.ts +++ b/src/permissions/readOnlyShell.ts @@ -112,11 +112,117 @@ const SAFE_GH_COMMANDS: Record | null> = { }; // Operators that are always dangerous (file redirects, command substitution) -// Note: &&, ||, ; are handled by splitting and checking each segment -const DANGEROUS_OPERATOR_PATTERN = /(>>|>|\$\(|`)/; +// Segment separators are split quote-aware by splitShellSegments(). +const DANGEROUS_REDIRECT_OPERATORS = [">>", ">"]; + +type ReadOnlyShellOptions = { + /** + * When true, treat absolute paths and `..` traversal as read-only. + * Default false keeps path sandboxing for generic auto-approval. + */ + allowExternalPaths?: boolean; +}; + +/** + * Split a shell command into segments on unquoted separators: |, &&, ||, ; + * Returns null if dangerous operators are found: + * - redirects (>, >>) outside quotes + * - command substitution ($(), backticks) outside single quotes + */ +function splitShellSegments(input: string): string[] | null { + const segments: string[] = []; + let current = ""; + let i = 0; + let quote: "single" | "double" | null = null; + + while (i < input.length) { + const ch = input[i]; + + if (quote === "single") { + current += ch; + if (ch === "'") { + quote = null; + } + i += 1; + continue; + } + + if (quote === "double") { + if (ch === "\\" && i + 1 < input.length) { + current += input.slice(i, i + 2); + i += 2; + continue; + } + + // Command substitution still evaluates inside double quotes. + if (ch === "`" || input.startsWith("$(", i)) { + return null; + } + + current += ch; + if (ch === '"') { + quote = null; + } + i += 1; + continue; + } + + if (ch === "'") { + quote = "single"; + current += ch; + i += 1; + continue; + } + if (ch === '"') { + quote = "double"; + current += ch; + i += 1; + continue; + } + + if (DANGEROUS_REDIRECT_OPERATORS.some((op) => input.startsWith(op, i))) { + return null; + } + if (ch === "`" || input.startsWith("$(", i)) { + return null; + } + + if (input.startsWith("&&", i)) { + segments.push(current); + current = ""; + i += 2; + continue; + } + if (input.startsWith("||", i)) { + segments.push(current); + current = ""; + i += 2; + continue; + } + if (ch === ";") { + segments.push(current); + current = ""; + i += 1; + continue; + } + if (ch === "|") { + segments.push(current); + current = ""; + i += 1; + continue; + } + + current += ch; + i += 1; + } + + segments.push(current); + return segments.map((segment) => segment.trim()).filter(Boolean); +} export function isReadOnlyShellCommand( command: string | string[] | undefined | null, + options: ReadOnlyShellOptions = {}, ): boolean { if (!command) { return false; @@ -133,9 +239,9 @@ export function isReadOnlyShellCommand( if (!nested) { return false; } - return isReadOnlyShellCommand(nested); + return isReadOnlyShellCommand(nested, options); } - return isReadOnlyShellCommand(joined); + return isReadOnlyShellCommand(joined, options); } const trimmed = command.trim(); @@ -143,23 +249,13 @@ export function isReadOnlyShellCommand( return false; } - if (DANGEROUS_OPERATOR_PATTERN.test(trimmed)) { - return false; - } - - // Split on command separators: |, &&, ||, ; - // Each segment must be safe for the whole command to be safe - const segments = trimmed - .split(/\||&&|\|\||;/) - .map((segment) => segment.trim()) - .filter(Boolean); - - if (segments.length === 0) { + const segments = splitShellSegments(trimmed); + if (!segments || segments.length === 0) { return false; } for (const segment of segments) { - if (!isSafeSegment(segment)) { + if (!isSafeSegment(segment, options)) { return false; } } @@ -167,7 +263,11 @@ export function isReadOnlyShellCommand( return true; } -function isSafeSegment(segment: string): boolean { +function isSafeSegment( + segment: string, + options: ReadOnlyShellOptions, +): boolean { + const { allowExternalPaths = false } = options; const tokens = tokenize(segment); if (tokens.length === 0) { return false; @@ -182,10 +282,14 @@ function isSafeSegment(segment: string): boolean { if (!nested) { return false; } - return isReadOnlyShellCommand(stripQuotes(nested)); + return isReadOnlyShellCommand(stripQuotes(nested), options); } if (ALWAYS_SAFE_COMMANDS.has(command)) { + if (allowExternalPaths) { + return true; + } + // `cd` is read-only, but it should still respect path restrictions so // `cd / && cat relative/path` cannot bypass path checks on later segments. if (command === "cd") { diff --git a/src/tests/permissions-mode.test.ts b/src/tests/permissions-mode.test.ts index 1557d8f..9daaadb 100644 --- a/src/tests/permissions-mode.test.ts +++ b/src/tests/permissions-mode.test.ts @@ -483,6 +483,24 @@ test("plan mode - allows read-only Bash commands", () => { ); expect(chainedResult.decision).toBe("allow"); + // absolute path reads should be allowed in plan mode + const absolutePathResult = checkPermission( + "Bash", + { command: "ls -la /Users/test/.letta/plans" }, + permissions, + "/Users/test/project", + ); + expect(absolutePathResult.decision).toBe("allow"); + + // traversal reads should be allowed in plan mode + const traversalResult = checkPermission( + "Bash", + { command: "cat ../../README.md" }, + permissions, + "/Users/test/project", + ); + expect(traversalResult.decision).toBe("allow"); + // cd && dangerous command should still be denied const cdDangerousResult = checkPermission( "Bash", @@ -491,6 +509,35 @@ test("plan mode - allows read-only Bash commands", () => { "/Users/test/project", ); expect(cdDangerousResult.decision).toBe("deny"); + + // quoted pipes in regex patterns should be allowed + const quotedPipeResult = checkPermission( + "Bash", + { command: 'rg -n "foo|bar|baz" src/permissions' }, + permissions, + "/Users/test/project", + ); + expect(quotedPipeResult.decision).toBe("allow"); +}); + +test("plan mode - allows TaskOutput", () => { + permissionMode.setMode("plan"); + + const permissions: PermissionRules = { + allow: [], + deny: [], + ask: [], + }; + + const result = checkPermission( + "TaskOutput", + { task_id: "task_123", block: false }, + permissions, + "/Users/test/project", + ); + + expect(result.decision).toBe("allow"); + expect(result.matchedRule).toBe("plan mode"); }); test("plan mode - denies WebFetch", () => { diff --git a/src/tests/permissions/readOnlyShell.test.ts b/src/tests/permissions/readOnlyShell.test.ts index 7a99eff..8d18c37 100644 --- a/src/tests/permissions/readOnlyShell.test.ts +++ b/src/tests/permissions/readOnlyShell.test.ts @@ -6,6 +6,33 @@ import { } from "../../permissions/readOnlyShell"; describe("isReadOnlyShellCommand", () => { + describe("path restrictions", () => { + test("blocks external paths by default", () => { + expect(isReadOnlyShellCommand("cat /etc/passwd")).toBe(false); + expect(isReadOnlyShellCommand("head -n 20 ../../../.ssh/id_rsa")).toBe( + false, + ); + }); + + test("allows external paths when explicitly enabled", () => { + expect( + isReadOnlyShellCommand("cat /etc/passwd", { + allowExternalPaths: true, + }), + ).toBe(true); + expect( + isReadOnlyShellCommand("head -n 20 ../../../.ssh/id_rsa", { + allowExternalPaths: true, + }), + ).toBe(true); + expect( + isReadOnlyShellCommand("cd / && cat etc/passwd", { + allowExternalPaths: true, + }), + ).toBe(true); + }); + }); + describe("always safe commands", () => { test("allows cat", () => { expect(isReadOnlyShellCommand("cat file.txt")).toBe(true); @@ -166,6 +193,12 @@ describe("isReadOnlyShellCommand", () => { expect(isReadOnlyShellCommand("ls -la | grep txt | wc -l")).toBe(true); }); + test("allows pipe characters inside quoted args", () => { + expect(isReadOnlyShellCommand('rg -n "foo|bar|baz" apps/core')).toBe( + true, + ); + }); + test("blocks pipes with unsafe commands", () => { expect(isReadOnlyShellCommand("cat file | rm")).toBe(false); expect(isReadOnlyShellCommand("echo test | bash")).toBe(false); @@ -187,6 +220,13 @@ describe("isReadOnlyShellCommand", () => { test("blocks command substitution", () => { expect(isReadOnlyShellCommand("echo $(rm file)")).toBe(false); expect(isReadOnlyShellCommand("echo `rm file`")).toBe(false); + expect(isReadOnlyShellCommand('echo "$(rm file)"')).toBe(false); + expect(isReadOnlyShellCommand('echo "`rm file`"')).toBe(false); + }); + + test("allows literal redirect text inside quotes", () => { + expect(isReadOnlyShellCommand('echo "a > b"')).toBe(true); + expect(isReadOnlyShellCommand("echo 'a >> b'")).toBe(true); }); });