fix(permissions): harden shell auto-approval path checks (#972)

Co-authored-by: RinZ27 <222222878+RinZ27@users.noreply.github.com>
Co-authored-by: Letta <noreply@letta.com>

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;

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

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

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,
+  );
+});