Merge pull request #1483 from corbitsdev/cl-10775-drop-path-escape-throw

refactor(plugins): drop the duplicate path-escape throw

Author: TheGreatAxios

src/permission/gate.ts

@@ -719,9 +719,8 @@ export function createPermissionGate(
     if (cachedDenial !== undefined)
       return { kind: "deny", reason: cachedDenial };
 
-    // Path-escape will hard-reject these at execution; deny here rather than
-    // showing Accept for a call that cannot succeed. Workers sandbox against
-    // their own cwd's roots, not the session listing.
+    // Hard-deny workspace escapes here, before allow and before ask.
+    // Workers sandbox against their own cwd's roots, not the session listing.
     const escapeRoots =
       effectiveCwd === resolvedCwd
         ? rootsProvider

src/permission/permission.test.ts

@@ -44,7 +44,10 @@ import {
 import type { Approval, ApprovalOutcome, PermissionRequest } from "./types.js";
 import { initTemporaryGitRepo } from "../../testkit/temporary-git-repo.js";
 import { secretGuardPlugin } from "../plugins/secret-guard-plugin.js";
-import { pathEscapePlugin } from "../plugins/path-escape-plugin.js";
+import {
+  pathEscapePlugin,
+  pathEscapeBlockReason,
+} from "../plugins/path-escape-plugin.js";
 
 const shellCall = (command: string): ToolCall =>
   toolCall("run_shell", { command });
@@ -2668,7 +2671,7 @@ describe("listWorktreeRoots", () => {
     expect(result.isError).not.toBe(true);
   });
 
-  test("pathEscapePlugin still rejects a relative path into a genuinely unrelated directory", async () => {
+  test("the gate denies a relative path into a genuinely unrelated directory", async () => {
     const { repo } = createRepoWithWorktree();
     const roots = await listWorktreeRoots(repo);
     const outside = mkdtempSync(join(tmpdir(), "intercode-unrelated-plugin-"));
@@ -2681,8 +2684,15 @@ describe("listWorktreeRoots", () => {
       toolCall("read_file", { path: relativeToOutside }),
       new AbortController().signal,
     );
-    expect(result.isError).toBe(true);
-    expect(result.content).toMatch(/escapes working directory/);
+    expect(result.isError).not.toBe(true);
+    expect(
+      pathEscapeBlockReason(
+        { path: relativeToOutside },
+        repo,
+        () => roots,
+        "read_file",
+      ),
+    ).toMatch(/escapes working directory/);
   });
 });
 

src/plugins/path-escape-plugin.test.ts

@@ -44,26 +44,31 @@ describe("pathEscapePlugin", () => {
     ["source", "copy", "/etc/passwd"],
     ["filename", "write_file", "../secret.txt"],
   ] as const) {
-    test(`blocks escape via ${key} key (${tool})`, async () => {
+    test(`leaves an outside ${key} unchanged; the gate denies ${tool}`, async () => {
       const plugin = pathEscapePlugin("/project");
-      const handler = pluginHandler(plugin, okHandler);
+      const handler = pluginHandler(plugin, echoArgsHandler);
       const result = await handler(
         makeToolCall(tool, { [key]: value }),
         neverAbort(),
       );
-      expect(result.isError).toBe(true);
+      expect(result.isError).not.toBe(true);
+      const args = JSON.parse(String(result.content)) as Record<string, string>;
+      expect(args[key]).toBe(value);
+      expect(
+        pathEscapeBlockReason({ [key]: value }, "/project", () => [], tool),
+      ).toMatch(/escapes working directory/);
     });
   }
 
-  test("the block message tells the operator the path escapes the working directory", async () => {
-    const plugin = pathEscapePlugin("/project");
-    const handler = pluginHandler(plugin, okHandler);
-    const result = await handler(
-      makeToolCall("read_file", { path: "../secret.txt" }),
-      neverAbort(),
-    );
-    expect(result.isError).toBe(true);
-    expect(result.content).toMatch(/escapes working directory/);
+  test("the gate denial tells the operator the path escapes the working directory", () => {
+    expect(
+      pathEscapeBlockReason(
+        { path: "../secret.txt" },
+        "/project",
+        () => [],
+        "read_file",
+      ),
+    ).toMatch(/escapes working directory/);
   });
 
   test("allows cwd path itself", async () => {
@@ -114,8 +119,17 @@ describe("pathEscapePlugin", () => {
       makeToolCall("read_file", { path: "../other-repo/README.md" }),
       neverAbort(),
     );
-    expect(blocked.isError).toBe(true);
-    expect(blocked.content).toMatch(/escapes working directory/);
+    expect(blocked.isError).not.toBe(true);
+    const blockedArgs = JSON.parse(String(blocked.content)) as { path: string };
+    expect(blockedArgs.path).toBe("../other-repo/README.md");
+    expect(
+      pathEscapeBlockReason(
+        { path: "../other-repo/README.md" },
+        "/project",
+        () => [],
+        "read_file",
+      ),
+    ).toMatch(/escapes working directory/);
 
     allow = true;
     const allowed = await handler(
@@ -204,15 +218,20 @@ describe("pathEscapePlugin", () => {
       return { next, seen: () => seen };
     };
 
-    test("blocks escape in a nested object under a path-like key", async () => {
+    test("passes a nested escape through; the gate denies it", async () => {
+      const args = { options: { path: "../secret.txt" } };
       const plugin = pathEscapePlugin("/project");
-      const handler = pluginHandler(plugin, okHandler);
+      const { next, seen } = captureNext();
+      const handler = pluginHandler(plugin, next);
       const result = await handler(
-        makeToolCall("read_file", { options: { path: "../secret.txt" } }),
+        makeToolCall("read_file", args),
         neverAbort(),
       );
-      expect(result.isError).toBe(true);
-      expect(result.content).toMatch(/escapes working directory/);
+      expect(result.isError).not.toBe(true);
+      expect(seen()).toEqual(args);
+      expect(
+        pathEscapeBlockReason(args, "/project", () => [], "read_file"),
+      ).toMatch(/escapes working directory/);
     });
 
     test("resolves nested in-bounds paths instead of passing them through", async () => {
@@ -229,28 +248,38 @@ describe("pathEscapePlugin", () => {
       });
     });
 
-    test("blocks escape via the filepath spelling", async () => {
+    test("passes an escape via the filepath spelling; the gate denies it", async () => {
+      const args = { filepath: "../secret.txt" };
       const plugin = pathEscapePlugin("/project");
-      const handler = pluginHandler(plugin, okHandler);
+      const { next, seen } = captureNext();
+      const handler = pluginHandler(plugin, next);
       const result = await handler(
-        makeToolCall("read_file", { filepath: "../secret.txt" }),
+        makeToolCall("read_file", args),
         neverAbort(),
       );
-      expect(result.isError).toBe(true);
-      expect(result.content).toMatch(/escapes working directory/);
+      expect(result.isError).not.toBe(true);
+      expect(seen()).toEqual(args);
+      expect(
+        pathEscapeBlockReason(args, "/project", () => [], "read_file"),
+      ).toMatch(/escapes working directory/);
     });
 
-    test("blocks escape in a string array under a path-like key", async () => {
+    test("passes an escape in a string array; the gate denies it", async () => {
+      const args = { paths: ["src/index.ts", "../secret.txt"] };
       const plugin = pathEscapePlugin("/project");
-      const handler = pluginHandler(plugin, okHandler);
+      const { next, seen } = captureNext();
+      const handler = pluginHandler(plugin, next);
       const result = await handler(
-        makeToolCall("read_file", {
-          paths: ["src/index.ts", "../secret.txt"],
-        }),
+        makeToolCall("read_file", args),
         neverAbort(),
       );
-      expect(result.isError).toBe(true);
-      expect(result.content).toMatch(/escapes working directory/);
+      expect(result.isError).not.toBe(true);
+      expect(seen()).toEqual({
+        paths: ["/project/src/index.ts", "../secret.txt"],
+      });
+      expect(
+        pathEscapeBlockReason(args, "/project", () => [], "read_file"),
+      ).toMatch(/escapes working directory/);
     });
 
     test("pathEscapeBlockReason agrees with execution time on nested escapes", async () => {
@@ -302,14 +331,15 @@ describe("pathEscapePlugin", () => {
 
     test("FILE_PATH and file-path match case- and separator-insensitively", async () => {
       const plugin = pathEscapePlugin("/project");
-      const handler = pluginHandler(plugin, okHandler);
+      const { next, seen } = captureNext();
+      const handler = pluginHandler(plugin, next);
       for (const key of ["FILE_PATH", "file-path"]) {
         const result = await handler(
           makeToolCall("read_file", { [key]: "../secret.txt" }),
           neverAbort(),
         );
-        expect(result.isError).toBe(true);
-        expect(result.content).toMatch(/escapes working directory/);
+        expect(result.isError).not.toBe(true);
+        expect(seen()).toEqual({ [key]: "../secret.txt" });
         expect(
           pathEscapeBlockReason(
             { [key]: "../secret.txt" },
@@ -529,13 +559,14 @@ describe("pathEscapePlugin", () => {
       const plugin = pathEscapePlugin(cwd, () => [], {
         trustedPluginRoots: () => [pluginDir],
       });
-      const handler = pluginHandler(plugin, okHandler);
+      const handler = pluginHandler(plugin, echoArgsHandler);
       const result = await handler(
         makeToolCall("write_file", { path: target, content: "x" }),
         neverAbort(),
       );
-      expect(result.isError).toBe(true);
-      expect(result.content).toMatch(/escapes working directory/);
+      expect(result.isError).not.toBe(true);
+      const args = JSON.parse(String(result.content)) as { path: string };
+      expect(args.path).toBe(target);
       expect(
         pathEscapeBlockReason(
           { path: target, content: "x" },

src/plugins/path-escape-plugin.ts

@@ -80,9 +80,9 @@ function escapeArgs(
   toolName: string,
   trustedPluginRoots?: RootsProvider,
 ): Record<string, unknown> {
-  // One fused walk: sanitizePath throws the same deny strings
-  // pathEscapeBlockReason computes, so a surviving call is exactly one the
-  // block-reason leg would have passed.
+  // Virtual-ref denials still throw. Workspace escapes pass through unchanged
+  // when allowOutside is false; the permission gate denies them via
+  // pathEscapeBlockReason before allow and before ask.
   const roots = rootsForEscape(toolName, rootsProvider, trustedPluginRoots);
   return walkPathArgs(args, undefined, (value) =>
     sanitizePath(value, cwd, roots, allowOutside, toolName),
@@ -200,9 +200,9 @@ export function virtualRefVerdict(
   return undefined;
 }
 
-// Same sandbox pathEscapePlugin enforces at execution. The permission gate
-// consults this at authorize time so it can deny instead of asking for a call
-// the plugin will reject after Accept.
+// The permission gate consults this before allow and before ask so an
+// out-of-workspace path is denied instead of prompted. sanitizePath does not
+// throw a second copy of that denial.
 export function pathEscapeBlockReason(
   args: Record<string, unknown>,
   cwd: string,
@@ -297,5 +297,7 @@ function sanitizePath(
     // so later plugins see a stable path, not a relative escape fragment.
     return resolve(cwd, value);
   }
-  throw new Error(`Path escapes working directory: ${value}`);
+  // The gate already hard-denies this escape. Leave the original path so a
+  // later permission check sees the same value the operator sent.
+  return value;
 }