From f1f507a45d64c04513afac3e9b525a7a471767ab Mon Sep 17 00:00:00 2001
From: Charles Packer <packercharles@gmail.com>
Date: Mon, 1 Dec 2025 22:10:35 -0800
Subject: [PATCH] feat: loosen read permissions on shell cmd (#144)

---
 src/permissions/checker.ts                  |  28 +++
 src/permissions/readOnlyShell.ts            | 186 ++++++++++++++++++++
 src/tests/permissions-checker.test.ts       |   8 +-
 src/tests/permissions-cli.test.ts           |  12 +-
 src/tests/permissions-mode.test.ts          |   4 +-
 src/tests/permissions.test.ts               |   8 +-
 src/tests/permissions/readOnlyShell.test.ts | 179 +++++++++++++++++++
 7 files changed, 409 insertions(+), 16 deletions(-)
 create mode 100644 src/permissions/readOnlyShell.ts
 create mode 100644 src/tests/permissions/readOnlyShell.test.ts

diff --git a/src/permissions/checker.ts b/src/permissions/checker.ts
index c48fc19..d277a5e 100644
--- a/src/permissions/checker.ts
+++ b/src/permissions/checker.ts
@@ -9,6 +9,7 @@ import {
   matchesToolPattern,
 } from "./matcher";
 import { permissionMode } from "./mode";
+import { isReadOnlyShellCommand } from "./readOnlyShell";
 import { sessionPermissions } from "./session";
 import type {
   PermissionCheckResult,
@@ -20,6 +21,15 @@ import type {
  * Tools that don't require approval within working directory
  */
 const WORKING_DIRECTORY_TOOLS = ["Read", "Glob", "Grep"];
+const READ_ONLY_SHELL_TOOLS = new Set([
+  "Bash",
+  "shell",
+  "Shell",
+  "shell_command",
+  "ShellCommand",
+  "run_shell_command",
+  "RunShellCommand",
+]);
 
 /**
  * Check permission for a tool execution.
@@ -110,6 +120,16 @@ export function checkPermission(
     };
   }
 
+  if (READ_ONLY_SHELL_TOOLS.has(toolName)) {
+    const shellCommand = extractShellCommand(toolArgs);
+    if (shellCommand && isReadOnlyShellCommand(shellCommand)) {
+      return {
+        decision: "allow",
+        reason: "Read-only shell command",
+      };
+    }
+  }
+
   // After checking CLI overrides, check if Read/Glob/Grep within working directory
   if (WORKING_DIRECTORY_TOOLS.includes(toolName)) {
     const filePath = extractFilePath(toolArgs);
@@ -258,6 +278,14 @@ function buildPermissionQuery(toolName: string, toolArgs: ToolArgs): string {
   }
 }
 
+function extractShellCommand(toolArgs: ToolArgs): string | string[] | null {
+  const command = toolArgs.command;
+  if (typeof command === "string" || Array.isArray(command)) {
+    return command;
+  }
+  return null;
+}
+
 /**
  * Check if query matches a permission pattern
  */
diff --git a/src/permissions/readOnlyShell.ts b/src/permissions/readOnlyShell.ts
new file mode 100644
index 0000000..f9648e6
--- /dev/null
+++ b/src/permissions/readOnlyShell.ts
@@ -0,0 +1,186 @@
+const ALWAYS_SAFE_COMMANDS = new Set([
+  "cat",
+  "head",
+  "tail",
+  "less",
+  "more",
+  "grep",
+  "rg",
+  "ag",
+  "ack",
+  "fgrep",
+  "egrep",
+  "ls",
+  "tree",
+  "file",
+  "stat",
+  "du",
+  "df",
+  "wc",
+  "diff",
+  "cmp",
+  "comm",
+  "cut",
+  "tr",
+  "nl",
+  "column",
+  "fold",
+  "pwd",
+  "whoami",
+  "hostname",
+  "date",
+  "uname",
+  "uptime",
+  "id",
+  "echo",
+  "printf",
+  "env",
+  "printenv",
+  "which",
+  "whereis",
+  "type",
+  "basename",
+  "dirname",
+  "realpath",
+  "readlink",
+  "jq",
+  "yq",
+  "strings",
+  "xxd",
+  "hexdump",
+]);
+
+const SAFE_GIT_SUBCOMMANDS = new Set([
+  "status",
+  "diff",
+  "log",
+  "show",
+  "branch",
+  "tag",
+  "remote",
+]);
+
+const DANGEROUS_OPERATOR_PATTERN = /(>>|>|&&|\|\||;|\$\(|`)/;
+
+export function isReadOnlyShellCommand(
+  command: string | string[] | undefined | null,
+): boolean {
+  if (!command) {
+    return false;
+  }
+
+  if (Array.isArray(command)) {
+    if (command.length === 0) {
+      return false;
+    }
+    const joined = command.join(" ");
+    const [executable, ...rest] = command;
+    if (executable && isShellExecutor(executable)) {
+      const nested = extractDashCArgument(rest);
+      if (!nested) {
+        return false;
+      }
+      return isReadOnlyShellCommand(nested);
+    }
+    return isReadOnlyShellCommand(joined);
+  }
+
+  const trimmed = command.trim();
+  if (!trimmed) {
+    return false;
+  }
+
+  if (DANGEROUS_OPERATOR_PATTERN.test(trimmed)) {
+    return false;
+  }
+
+  const segments = trimmed
+    .split("|")
+    .map((segment) => segment.trim())
+    .filter(Boolean);
+
+  if (segments.length === 0) {
+    return false;
+  }
+
+  for (const segment of segments) {
+    if (!isSafeSegment(segment)) {
+      return false;
+    }
+  }
+
+  return true;
+}
+
+function isSafeSegment(segment: string): boolean {
+  const tokens = tokenize(segment);
+  if (tokens.length === 0) {
+    return false;
+  }
+
+  const command = tokens[0];
+  if (!command) {
+    return false;
+  }
+  if (isShellExecutor(command)) {
+    const nested = extractDashCArgument(tokens.slice(1));
+    if (!nested) {
+      return false;
+    }
+    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 (command === "find") {
+      return !/-delete|\s-exec\b/.test(segment);
+    }
+    if (command === "sort") {
+      return !/\s-o\b/.test(segment);
+    }
+    return false;
+  }
+
+  return true;
+}
+
+function isShellExecutor(command: string): boolean {
+  return command === "bash" || command === "sh";
+}
+
+function tokenize(segment: string): string[] {
+  const matches = segment.match(/(?:[^\s"']+|"[^"]*"|'[^']*')+/g);
+  if (!matches) {
+    return [];
+  }
+  return matches.map((token) => stripQuotes(token));
+}
+
+function stripQuotes(value: string): string {
+  if (
+    (value.startsWith('"') && value.endsWith('"')) ||
+    (value.startsWith("'") && value.endsWith("'"))
+  ) {
+    return value.slice(1, -1);
+  }
+  return value;
+}
+
+function extractDashCArgument(tokens: string[]): string | undefined {
+  for (let i = 0; i < tokens.length; i += 1) {
+    const token = tokens[i];
+    if (!token) {
+      continue;
+    }
+    if (token === "-c" || token === "-lc" || /^-[a-zA-Z]*c$/.test(token)) {
+      return tokens[i + 1];
+    }
+  }
+  return undefined;
+}
diff --git a/src/tests/permissions-checker.test.ts b/src/tests/permissions-checker.test.ts
index 2ea17c3..dbe1084 100644
--- a/src/tests/permissions-checker.test.ts
+++ b/src/tests/permissions-checker.test.ts
@@ -232,20 +232,20 @@ test("Allow rule for file outside working directory", () => {
 
 test("Allow rule for Bash command", () => {
   const permissions: PermissionRules = {
-    allow: ["Bash(git diff:*)"],
+    allow: ["Bash(npm run:*)"],
     deny: [],
     ask: [],
   };
 
   const result = checkPermission(
     "Bash",
-    { command: "git diff HEAD" },
+    { command: "npm run build" },
     permissions,
     "/Users/test/project",
   );
 
   expect(result.decision).toBe("allow");
-  expect(result.matchedRule).toBe("Bash(git diff:*)");
+  expect(result.matchedRule).toBe("Bash(npm run:*)");
 });
 
 test("Allow exact Bash command", () => {
@@ -330,7 +330,7 @@ test("Read defaults to allow", () => {
 test("Bash defaults to ask", () => {
   const result = checkPermission(
     "Bash",
-    { command: "ls -la" },
+    { command: "curl http://example.com" }, // Use non-read-only command
     { allow: [], deny: [], ask: [] },
     "/Users/test/project",
   );
diff --git a/src/tests/permissions-cli.test.ts b/src/tests/permissions-cli.test.ts
index d60e2c9..eb4242f 100644
--- a/src/tests/permissions-cli.test.ts
+++ b/src/tests/permissions-cli.test.ts
@@ -374,7 +374,7 @@ test("Precedence: CLI allowedTools > settings allow", () => {
   cliPermissions.setAllowedTools("Bash(npm install)");
 
   const permissions: PermissionRules = {
-    allow: ["Bash(git:*)"],
+    allow: ["Bash(docker:*)"],
     deny: [],
     ask: [],
   };
@@ -389,13 +389,13 @@ test("Precedence: CLI allowedTools > settings allow", () => {
   expect(npmResult.decision).toBe("allow");
   expect(npmResult.matchedRule).toBe("Bash(npm install) (CLI)");
 
-  // Settings should match for git
-  const gitResult = checkPermission(
+  // Settings should match for docker (non-read-only command)
+  const dockerResult = checkPermission(
     "Bash",
-    { command: "git status" },
+    { command: "docker build ." },
     permissions,
     "/Users/test/project",
   );
-  expect(gitResult.decision).toBe("allow");
-  expect(gitResult.matchedRule).toBe("Bash(git:*)");
+  expect(dockerResult.decision).toBe("allow");
+  expect(dockerResult.matchedRule).toBe("Bash(docker:*)");
 });
diff --git a/src/tests/permissions-mode.test.ts b/src/tests/permissions-mode.test.ts
index 6b6207f..d14c3df 100644
--- a/src/tests/permissions-mode.test.ts
+++ b/src/tests/permissions-mode.test.ts
@@ -23,7 +23,7 @@ test("default mode - no overrides", () => {
 
   const result = checkPermission(
     "Bash",
-    { command: "ls" },
+    { command: "curl http://example.com" }, // Use non-read-only command
     permissions,
     "/Users/test/project",
   );
@@ -161,7 +161,7 @@ test("acceptEdits mode - does NOT allow Bash", () => {
 
   const result = checkPermission(
     "Bash",
-    { command: "ls" },
+    { command: "curl http://example.com" }, // Use non-read-only command
     permissions,
     "/Users/test/project",
   );
diff --git a/src/tests/permissions.test.ts b/src/tests/permissions.test.ts
index be2a494..1b3c054 100644
--- a/src/tests/permissions.test.ts
+++ b/src/tests/permissions.test.ts
@@ -32,7 +32,7 @@ test("Read outside working directory requires approval", () => {
 test("Bash commands require approval by default", () => {
   const result = checkPermission(
     "Bash",
-    { command: "ls -la" },
+    { command: "curl http://example.com" }, // Use non-read-only command
     { allow: [], deny: [], ask: [] },
     "/Users/test/project",
   );
@@ -42,20 +42,20 @@ test("Bash commands require approval by default", () => {
 
 test("Allow rule matches Bash prefix pattern", () => {
   const permissions: PermissionRules = {
-    allow: ["Bash(git diff:*)"],
+    allow: ["Bash(npm run:*)"],
     deny: [],
     ask: [],
   };
 
   const result = checkPermission(
     "Bash",
-    { command: "git diff HEAD" },
+    { command: "npm run build" },
     permissions,
     "/Users/test/project",
   );
 
   expect(result.decision).toBe("allow");
-  expect(result.matchedRule).toBe("Bash(git diff:*)");
+  expect(result.matchedRule).toBe("Bash(npm run:*)");
 });
 
 test("Deny rule blocks file access", () => {
diff --git a/src/tests/permissions/readOnlyShell.test.ts b/src/tests/permissions/readOnlyShell.test.ts
new file mode 100644
index 0000000..ca198c1
--- /dev/null
+++ b/src/tests/permissions/readOnlyShell.test.ts
@@ -0,0 +1,179 @@
+import { describe, expect, test } from "bun:test";
+import { isReadOnlyShellCommand } from "../../permissions/readOnlyShell";
+
+describe("isReadOnlyShellCommand", () => {
+  describe("always safe commands", () => {
+    test("allows cat", () => {
+      expect(isReadOnlyShellCommand("cat file.txt")).toBe(true);
+    });
+
+    test("allows grep", () => {
+      expect(isReadOnlyShellCommand("grep -r 'pattern' .")).toBe(true);
+    });
+
+    test("allows ls", () => {
+      expect(isReadOnlyShellCommand("ls -la")).toBe(true);
+    });
+
+    test("allows head/tail", () => {
+      expect(isReadOnlyShellCommand("head -n 10 file.txt")).toBe(true);
+      expect(isReadOnlyShellCommand("tail -f log.txt")).toBe(true);
+    });
+
+    test("allows wc", () => {
+      expect(isReadOnlyShellCommand("wc -l file.txt")).toBe(true);
+    });
+
+    test("allows diff", () => {
+      expect(isReadOnlyShellCommand("diff file1.txt file2.txt")).toBe(true);
+    });
+
+    test("allows jq", () => {
+      expect(isReadOnlyShellCommand("jq '.foo' file.json")).toBe(true);
+    });
+
+    test("allows pwd, whoami, date, etc", () => {
+      expect(isReadOnlyShellCommand("pwd")).toBe(true);
+      expect(isReadOnlyShellCommand("whoami")).toBe(true);
+      expect(isReadOnlyShellCommand("date")).toBe(true);
+      expect(isReadOnlyShellCommand("hostname")).toBe(true);
+    });
+  });
+
+  describe("git commands", () => {
+    test("allows read-only git commands", () => {
+      expect(isReadOnlyShellCommand("git status")).toBe(true);
+      expect(isReadOnlyShellCommand("git diff")).toBe(true);
+      expect(isReadOnlyShellCommand("git log")).toBe(true);
+      expect(isReadOnlyShellCommand("git show HEAD")).toBe(true);
+      expect(isReadOnlyShellCommand("git branch -a")).toBe(true);
+    });
+
+    test("blocks write git commands", () => {
+      expect(isReadOnlyShellCommand("git push")).toBe(false);
+      expect(isReadOnlyShellCommand("git commit -m 'msg'")).toBe(false);
+      expect(isReadOnlyShellCommand("git reset --hard")).toBe(false);
+      expect(isReadOnlyShellCommand("git checkout branch")).toBe(false);
+    });
+
+    test("blocks bare git", () => {
+      expect(isReadOnlyShellCommand("git")).toBe(false);
+    });
+  });
+
+  describe("find command", () => {
+    test("allows safe find", () => {
+      expect(isReadOnlyShellCommand("find . -name '*.js'")).toBe(true);
+      expect(isReadOnlyShellCommand("find /tmp -type f")).toBe(true);
+    });
+
+    test("blocks find with -delete", () => {
+      expect(isReadOnlyShellCommand("find . -name '*.tmp' -delete")).toBe(
+        false,
+      );
+    });
+
+    test("blocks find with -exec", () => {
+      expect(isReadOnlyShellCommand("find . -exec rm {} \\;")).toBe(false);
+    });
+  });
+
+  describe("sort command", () => {
+    test("allows safe sort", () => {
+      expect(isReadOnlyShellCommand("sort file.txt")).toBe(true);
+      expect(isReadOnlyShellCommand("sort -n numbers.txt")).toBe(true);
+    });
+
+    test("blocks sort with -o (output to file)", () => {
+      expect(isReadOnlyShellCommand("sort -o output.txt input.txt")).toBe(
+        false,
+      );
+    });
+  });
+
+  describe("pipes", () => {
+    test("allows safe pipes", () => {
+      expect(isReadOnlyShellCommand("cat file | grep pattern")).toBe(true);
+      expect(isReadOnlyShellCommand("grep foo | head -10")).toBe(true);
+      expect(isReadOnlyShellCommand("ls -la | grep txt | wc -l")).toBe(true);
+    });
+
+    test("blocks pipes with unsafe commands", () => {
+      expect(isReadOnlyShellCommand("cat file | rm")).toBe(false);
+      expect(isReadOnlyShellCommand("echo test | bash")).toBe(false);
+    });
+  });
+
+  describe("dangerous operators", () => {
+    test("blocks output redirection", () => {
+      expect(isReadOnlyShellCommand("cat file > output.txt")).toBe(false);
+      expect(isReadOnlyShellCommand("cat file >> output.txt")).toBe(false);
+    });
+
+    test("blocks command chaining", () => {
+      expect(isReadOnlyShellCommand("ls && rm file")).toBe(false);
+      expect(isReadOnlyShellCommand("ls || rm file")).toBe(false);
+      expect(isReadOnlyShellCommand("ls; rm file")).toBe(false);
+    });
+
+    test("blocks command substitution", () => {
+      expect(isReadOnlyShellCommand("echo $(rm file)")).toBe(false);
+      expect(isReadOnlyShellCommand("echo `rm file`")).toBe(false);
+    });
+  });
+
+  describe("bash -c handling", () => {
+    test("allows bash -c with safe command", () => {
+      expect(isReadOnlyShellCommand("bash -c 'cat file.txt'")).toBe(true);
+      expect(isReadOnlyShellCommand("sh -c 'grep pattern file'")).toBe(true);
+    });
+
+    test("allows bash -lc with safe command", () => {
+      expect(isReadOnlyShellCommand("bash -lc cat package.json")).toBe(true);
+    });
+
+    test("blocks bash -c with unsafe command", () => {
+      expect(isReadOnlyShellCommand("bash -c 'rm file'")).toBe(false);
+      expect(isReadOnlyShellCommand("sh -c 'echo foo > file'")).toBe(false);
+    });
+
+    test("blocks bare bash/sh", () => {
+      expect(isReadOnlyShellCommand("bash")).toBe(false);
+      expect(isReadOnlyShellCommand("bash script.sh")).toBe(false);
+    });
+  });
+
+  describe("array commands", () => {
+    test("handles array format", () => {
+      expect(isReadOnlyShellCommand(["cat", "file.txt"])).toBe(true);
+      expect(isReadOnlyShellCommand(["rm", "file.txt"])).toBe(false);
+    });
+
+    test("handles bash -c in array format", () => {
+      expect(isReadOnlyShellCommand(["bash", "-c", "cat file"])).toBe(true);
+      expect(isReadOnlyShellCommand(["bash", "-lc", "cat file"])).toBe(true);
+      expect(isReadOnlyShellCommand(["bash", "-c", "rm file"])).toBe(false);
+    });
+  });
+
+  describe("edge cases", () => {
+    test("handles empty/null input", () => {
+      expect(isReadOnlyShellCommand("")).toBe(false);
+      expect(isReadOnlyShellCommand(null)).toBe(false);
+      expect(isReadOnlyShellCommand(undefined)).toBe(false);
+      expect(isReadOnlyShellCommand([])).toBe(false);
+    });
+
+    test("handles whitespace", () => {
+      expect(isReadOnlyShellCommand("   cat file.txt   ")).toBe(true);
+      expect(isReadOnlyShellCommand("  ")).toBe(false);
+    });
+
+    test("blocks unknown commands", () => {
+      expect(isReadOnlyShellCommand("rm file")).toBe(false);
+      expect(isReadOnlyShellCommand("mv a b")).toBe(false);
+      expect(isReadOnlyShellCommand("chmod 755 file")).toBe(false);
+      expect(isReadOnlyShellCommand("curl http://example.com")).toBe(false);
+    });
+  });
+});