fix: permissions shell rule normalization (#1189)

src/permissions/readOnlyShell.ts

@@ -112,112 +112,15 @@ const SAFE_GH_COMMANDS: Record<string, Set<string> | null> = {
 };
 
 // Operators that are always dangerous (file redirects, command substitution)
-// Segment separators are split quote-aware by splitShellSegments().
-const DANGEROUS_REDIRECT_OPERATORS = [">>", ">"];
+// Note: &&, ||, ; are handled by splitting and checking each segment
+const DANGEROUS_OPERATOR_PATTERN = /(>>|>|\$\(|`)/;
 
-type ReadOnlyShellOptions = {
+export interface ReadOnlyShellOptions {
   /**
-   * When true, treat absolute paths and `..` traversal as read-only.
-   * Default false keeps path sandboxing for generic auto-approval.
+   * Allow absolute/home/traversal path arguments for read-only commands.
+   * Used in plan mode where read-only shell should not be restricted to cwd-relative paths.
    */
   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(
@@ -249,8 +152,18 @@ export function isReadOnlyShellCommand(
     return false;
   }
 
-  const segments = splitShellSegments(trimmed);
-  if (!segments || segments.length === 0) {
+  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) {
     return false;
   }
 
@@ -267,7 +180,6 @@ function isSafeSegment(
   segment: string,
   options: ReadOnlyShellOptions,
 ): boolean {
-  const { allowExternalPaths = false } = options;
   const tokens = tokenize(segment);
   if (tokens.length === 0) {
     return false;
@@ -286,21 +198,40 @@ function isSafeSegment(
   }
 
   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") {
+      if (options.allowExternalPaths) {
+        return true;
+      }
       return !tokens.slice(1).some((t) => hasAbsoluteOrTraversalPathArg(t));
     }
 
     // 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));
+    const hasExternalPath =
+      !options.allowExternalPaths &&
+      tokens.slice(1).some((t) => hasAbsoluteOrTraversalPathArg(t));
+
+    if (hasExternalPath) {
+      return false;
+    }
+    return true;
+  }
+
+  if (command === "sed") {
+    // sed is read-only unless in-place edit flags are used.
+    const usesInPlace = tokens.some(
+      (token) =>
+        token === "-i" || token.startsWith("-i") || token === "--in-place",
+    );
+    if (usesInPlace) {
+      return false;
+    }
+
+    const hasExternalPath =
+      !options.allowExternalPaths &&
+      tokens.slice(1).some((t) => hasAbsoluteOrTraversalPathArg(t));
 
     if (hasExternalPath) {
       return false;