diff --git a/src/permissions/readOnlyShell.ts b/src/permissions/readOnlyShell.ts index 1f11f39..4d224ea 100644 --- a/src/permissions/readOnlyShell.ts +++ b/src/permissions/readOnlyShell.ts @@ -185,61 +185,76 @@ function isSafeSegment(segment: string): boolean { return isReadOnlyShellCommand(stripQuotes(nested)); } - if (!ALWAYS_SAFE_COMMANDS.has(command)) { - if (command === "git") { - const subcommand = tokens[1]; - if (!subcommand) { - return false; - } - return SAFE_GIT_SUBCOMMANDS.has(subcommand); + if (ALWAYS_SAFE_COMMANDS.has(command)) { + // `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") { + return !tokens.slice(1).some((t) => hasAbsoluteOrTraversalPathArg(t)); } - if (command === "gh") { - const category = tokens[1]; - if (!category) { - return false; - } - if (!(category in SAFE_GH_COMMANDS)) { - return false; - } - const allowedActions = SAFE_GH_COMMANDS[category]; - // null means any action is allowed (e.g., gh search, gh api, gh status) - if (allowedActions === null) { - return true; - } - // undefined means category not in map (shouldn't happen after 'in' check) - if (allowedActions === undefined) { - return false; - } - const action = tokens[2]; - if (!action) { - return false; - } - return allowedActions.has(action); + + // For other "always safe" commands, ensure they don't read sensitive files + // outside the allowed directories. + const hasExternalPath = tokens + .slice(1) + .some((t) => hasAbsoluteOrTraversalPathArg(t)); + + if (hasExternalPath) { + return false; } - if (command === "letta") { - const group = tokens[1]; - if (!group) { - return false; - } - if (!(group in SAFE_LETTA_COMMANDS)) { - return false; - } - const action = tokens[2]; - if (!action) { - return false; - } - return SAFE_LETTA_COMMANDS[group]?.has(action) ?? false; - } - if (command === "find") { - return !/-delete|\s-exec\b/.test(segment); - } - if (command === "sort") { - return !/\s-o\b/.test(segment); - } - return false; + return true; } - return true; + if (command === "git") { + const subcommand = tokens[1]; + if (!subcommand) { + return false; + } + return SAFE_GIT_SUBCOMMANDS.has(subcommand); + } + if (command === "gh") { + const category = tokens[1]; + if (!category) { + return false; + } + if (!(category in SAFE_GH_COMMANDS)) { + return false; + } + const allowedActions = SAFE_GH_COMMANDS[category]; + // null means any action is allowed (e.g., gh search, gh api, gh status) + if (allowedActions === null) { + return true; + } + // undefined means category not in map (shouldn't happen after 'in' check) + if (allowedActions === undefined) { + return false; + } + const action = tokens[2]; + if (!action) { + return false; + } + return allowedActions.has(action); + } + if (command === "letta") { + const group = tokens[1]; + if (!group) { + return false; + } + if (!(group in SAFE_LETTA_COMMANDS)) { + return false; + } + const action = tokens[2]; + if (!action) { + return false; + } + return SAFE_LETTA_COMMANDS[group]?.has(action) ?? false; + } + if (command === "find") { + return !/-delete|\s-exec\b/.test(segment); + } + if (command === "sort") { + return !/\s-o\b/.test(segment); + } + return false; } function isShellExecutor(command: string): boolean { @@ -277,6 +292,42 @@ function extractDashCArgument(tokens: string[]): string | undefined { return undefined; } +function isAbsolutePathArg(value: string): boolean { + if (!value) { + return false; + } + + // POSIX absolute paths + if (value.startsWith("/")) { + return true; + } + + // Windows absolute paths (drive letter and UNC) + return /^[a-zA-Z]:[\\/]/.test(value) || value.startsWith("\\\\"); +} + +function isHomeAnchoredPathArg(value: string): boolean { + if (!value) { + return false; + } + + return ( + value.startsWith("~/") || + value.startsWith("$HOME/") || + value.startsWith("%USERPROFILE%\\") || + value.startsWith("%USERPROFILE%/") + ); +} + +function hasAbsoluteOrTraversalPathArg(value: string): boolean { + if (isAbsolutePathArg(value) || isHomeAnchoredPathArg(value)) { + return true; + } + + // Path traversal segments only + return /(^|[\\/])\.\.([\\/]|$)/.test(value); +} + /** * Build the set of allowed memory directory prefixes for the current agent. * Includes: @@ -422,9 +473,34 @@ export function isMemoryDirCommand( // OR if all path-like arguments point to the memory dir if (cwd && isUnderMemoryDir(cwd, prefixes)) { // We're operating within the memory dir + const tokens = tokenize(part); + + const currentCwd = cwd; + if (!currentCwd) { + return false; + } + + // Even if we're in the memory dir, we must ensure the command doesn't + // escape it via absolute paths or parent directory references. + const hasExternalPath = tokens.some((t) => { + if (isAbsolutePathArg(t) || isHomeAnchoredPathArg(t)) { + return !isUnderMemoryDir(t, prefixes); + } + + if (hasAbsoluteOrTraversalPathArg(t)) { + const resolved = expandPath(resolve(expandPath(currentCwd), t)); + return !isUnderMemoryDir(resolved, prefixes); + } + + return false; + }); + + if (hasExternalPath) { + return false; + } + if (!MEMORY_DIR_APPROVE_ALL) { // Strict mode: validate command type - const tokens = tokenize(part); const cmd = tokens[0]; if (!cmd || !SAFE_MEMORY_DIR_COMMANDS.has(cmd)) { return false; diff --git a/src/tests/permissions-mode.test.ts b/src/tests/permissions-mode.test.ts index d0e838b..f973d5a 100644 --- a/src/tests/permissions-mode.test.ts +++ b/src/tests/permissions-mode.test.ts @@ -346,7 +346,7 @@ test("plan mode - allows read-only Bash commands", () => { // cd && git should be allowed (common CLI pattern) const cdGitResult = checkPermission( "Bash", - { command: "cd /some/path && git status" }, + { command: "cd src && git status" }, permissions, "/Users/test/project", ); @@ -355,7 +355,7 @@ test("plan mode - allows read-only Bash commands", () => { // cd && git show should be allowed const cdGitShowResult = checkPermission( "Bash", - { command: "cd /some/path && git show abc123" }, + { command: "cd src && git show abc123" }, permissions, "/Users/test/project", ); @@ -373,7 +373,7 @@ test("plan mode - allows read-only Bash commands", () => { // cd && dangerous command should still be denied const cdDangerousResult = checkPermission( "Bash", - { command: "cd /some/path && npm install" }, + { command: "cd src && npm install" }, permissions, "/Users/test/project", ); diff --git a/src/tests/permissions/readOnlyShell.test.ts b/src/tests/permissions/readOnlyShell.test.ts index fb31091..7a99eff 100644 --- a/src/tests/permissions/readOnlyShell.test.ts +++ b/src/tests/permissions/readOnlyShell.test.ts @@ -237,6 +237,10 @@ describe("isReadOnlyShellCommand", () => { expect(isReadOnlyShellCommand(" ")).toBe(false); }); + test("allows relative cd chaining with read-only git", () => { + expect(isReadOnlyShellCommand("cd src && git status")).toBe(true); + }); + test("blocks unknown commands", () => { expect(isReadOnlyShellCommand("rm file")).toBe(false); expect(isReadOnlyShellCommand("mv a b")).toBe(false); diff --git a/src/tests/permissions_security.test.ts b/src/tests/permissions_security.test.ts new file mode 100644 index 0000000..a196e6d --- /dev/null +++ b/src/tests/permissions_security.test.ts @@ -0,0 +1,46 @@ +import { expect, test } from "bun:test"; +import { homedir } from "node:os"; +import { resolve } from "node:path"; +import { + isMemoryDirCommand, + isReadOnlyShellCommand, +} from "../permissions/readOnlyShell"; + +test("FIX: isReadOnlyShellCommand should not auto-approve reading sensitive files", () => { + // These used to return true, now should return false + expect(isReadOnlyShellCommand("cat /etc/passwd")).toBe(false); + expect(isReadOnlyShellCommand("grep secret /etc/shadow")).toBe(false); + expect(isReadOnlyShellCommand("head -n 20 ../../../.ssh/id_rsa")).toBe(false); + expect( + isReadOnlyShellCommand("cat C:\\Windows\\System32\\drivers\\etc\\hosts"), + ).toBe(false); + + // Normal safe commands should still work + expect(isReadOnlyShellCommand("ls")).toBe(true); + expect(isReadOnlyShellCommand("cat README.md")).toBe(true); + + // Ensure `cd` cannot be used to bypass path checks via later relative reads + expect(isReadOnlyShellCommand("cd / && cat etc/passwd")).toBe(false); + expect(isReadOnlyShellCommand("cd C:\\ && type Windows\\win.ini")).toBe( + false, + ); +}); + +test("FIX: isMemoryDirCommand should not allow command injection via cd bypass", () => { + const agentId = "agent123"; + const home = homedir(); + const memoryDir = resolve(home, ".letta", "agents", agentId, "memory"); + + // This command starts with cd to memory dir, then tries to delete root + const dangerousCommand = `cd ${memoryDir} && rm -rf /`; + const dangerousWindowsCommand = `cd ${memoryDir} && type C:\\Windows\\win.ini`; + + expect(isMemoryDirCommand(dangerousCommand, agentId)).toBe(false); + expect(isMemoryDirCommand(dangerousWindowsCommand, agentId)).toBe(false); + + // Safe commands in memory dir should still work + expect(isMemoryDirCommand(`cd ${memoryDir} && ls`, agentId)).toBe(true); + expect(isMemoryDirCommand(`cd ${memoryDir} && git status`, agentId)).toBe( + true, + ); +});