diff --git a/src/permission/gate.ts b/src/permission/gate.ts index aa26d5f3f..e18345ea9 100644 --- a/src/permission/gate.ts +++ b/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 diff --git a/src/permission/permission.test.ts b/src/permission/permission.test.ts index f9b63a6d4..2b04e559f 100644 --- a/src/permission/permission.test.ts +++ b/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/); }); }); diff --git a/src/plugins/path-escape-plugin.test.ts b/src/plugins/path-escape-plugin.test.ts index ec1cf6137..9fd82261c 100644 --- a/src/plugins/path-escape-plugin.test.ts +++ b/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; + 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" }, diff --git a/src/plugins/path-escape-plugin.ts b/src/plugins/path-escape-plugin.ts index a02a7bcf9..54defbbc7 100644 --- a/src/plugins/path-escape-plugin.ts +++ b/src/plugins/path-escape-plugin.ts @@ -80,9 +80,9 @@ function escapeArgs( toolName: string, trustedPluginRoots?: RootsProvider, ): Record { - // 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, 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; }