From 4f7cf56738a50d6cbc8302c72a68931a9b07cfd6 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Wed, 30 Sep 2026 09:29:54 -0700 Subject: [PATCH] fix(agent): guard apply_patch with secret-guard and realpath bound apply_patch is mounted outside the posix plugin stack, so it skipped the secret-guard denylist and checked the workspace bound lexically only. Every patch path now goes through the denylist and the shared realpath resolver, on both the primary and worker mounts. --- src/agent/apply-patch-tool.test.ts | 29 ++++++++++++++- src/agent/apply-patch-tool.ts | 60 ++++++++++++++++++++++++------ src/agent/tools.ts | 8 +++- src/subagent/run.ts | 12 +++++- 4 files changed, 94 insertions(+), 15 deletions(-) diff --git a/src/agent/apply-patch-tool.test.ts b/src/agent/apply-patch-tool.test.ts index 97a2f6a24..4187ece7d 100644 --- a/src/agent/apply-patch-tool.test.ts +++ b/src/agent/apply-patch-tool.test.ts @@ -1,4 +1,4 @@ -import { mkdtemp, readFile, writeFile, stat } from "node:fs/promises"; +import { mkdtemp, readFile, stat, symlink, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { expect, test } from "bun:test"; @@ -6,7 +6,10 @@ import { expect, test } from "bun:test"; import { createApplyPatchTool } from "./apply-patch-tool.js"; async function run(cwd: string, input: string): Promise { - const tool = createApplyPatchTool(cwd); + const tool = createApplyPatchTool(cwd, { + allowOutside: () => false, + rootsProvider: () => [], + }); if (tool.kind !== "string") throw new Error("expected a string tool"); return tool.handler({ input }, new AbortController().signal); } @@ -65,3 +68,25 @@ test("rejects paths that escape the workspace", async () => { ); expect(out).toContain("escapes the workspace"); }); + +test("refuses to write a secret file", async () => { + const cwd = await mkdtemp(join(tmpdir(), "apply-patch-")); + const out = await run( + cwd, + "*** Begin Patch\n*** Add File: .env\n+TOKEN=x\n*** End Patch", + ); + expect(out).toContain("sensitive file blocked"); + await expect(stat(join(cwd, ".env"))).rejects.toThrow(); +}); + +test("refuses to follow a workspace symlink out of the workspace", async () => { + const cwd = await mkdtemp(join(tmpdir(), "apply-patch-")); + const outside = await mkdtemp(join(tmpdir(), "apply-patch-outside-")); + await symlink(outside, join(cwd, "link")); + const out = await run( + cwd, + "*** Begin Patch\n*** Add File: link/pwned.txt\n+x\n*** End Patch", + ); + expect(out).toContain("escapes the workspace"); + await expect(stat(join(outside, "pwned.txt"))).rejects.toThrow(); +}); diff --git a/src/agent/apply-patch-tool.ts b/src/agent/apply-patch-tool.ts index 4909e4c04..ebeb96a78 100644 --- a/src/agent/apply-patch-tool.ts +++ b/src/agent/apply-patch-tool.ts @@ -1,10 +1,16 @@ import { mkdir, readFile, rm, writeFile } from "node:fs/promises"; -import { dirname, isAbsolute, relative, resolve } from "node:path"; +import { dirname, resolve } from "node:path"; import { stringTool } from "@intx/agent"; import type { AgentTool } from "@intx/agent"; import type { ToolDefinition } from "@intx/types/runtime"; import { type } from "arktype"; +import { resolveWorkspacePath } from "../permission/path-restriction.js"; +import type { RootsProvider } from "../permission/worktree-roots.js"; +import { + createExtraDeniedPathMatcher, + isSensitivePathResolved, +} from "../plugins/secret-guard-plugin.js"; import { applyUpdateHunks, CodexApplyPatchError, @@ -31,28 +37,52 @@ type Planned = | { kind: "write"; path: string; content: string; label: string } | { kind: "remove"; path: string; label: string }; -function contained(cwd: string, path: string): string { +export interface ApplyPatchGuard { + allowOutside: () => boolean; + rootsProvider: RootsProvider; + extraDeniedPaths?: readonly string[]; +} + +// apply_patch is mounted outside the posix plugin stack, so it re-enforces the +// secret-guard denylist and the realpath workspace bound itself. A lexical +// check alone would let a symlink inside the workspace lead out of it. +function guardedPath( + cwd: string, + path: string, + guard: ApplyPatchGuard, + isExtraDenied: (value: string) => boolean, +): string { const abs = resolve(cwd, path); - const rel = relative(cwd, abs); - if (rel === "" || rel.startsWith("..") || isAbsolute(rel)) { + if (isSensitivePathResolved(abs) || isExtraDenied(abs)) { + throw new CodexApplyPatchError( + `Access to sensitive file blocked by policy: ${path}`, + ); + } + if ( + !guard.allowOutside() && + resolveWorkspacePath(cwd, abs, guard.rootsProvider) === undefined + ) { throw new CodexApplyPatchError(`path escapes the workspace: ${path}`); } return abs; } -async function plan(cwd: string, op: PatchOp): Promise { +async function plan( + op: PatchOp, + contained: (path: string) => string, +): Promise { if (op.type === "add") { - const path = contained(cwd, op.path); + const path = contained(op.path); return [ { kind: "write", path, content: op.content, label: `A ${op.path}` }, ]; } if (op.type === "delete") { return [ - { kind: "remove", path: contained(cwd, op.path), label: `D ${op.path}` }, + { kind: "remove", path: contained(op.path), label: `D ${op.path}` }, ]; } - const source = contained(cwd, op.path); + const source = contained(op.path); let original: string; try { original = await readFile(source, "utf8"); @@ -66,7 +96,7 @@ async function plan(cwd: string, op: PatchOp): Promise { return [ { kind: "write", - path: contained(cwd, op.moveTo), + path: contained(op.moveTo), content, label: `M ${op.moveTo}`, }, @@ -78,7 +108,15 @@ async function plan(cwd: string, op: PatchOp): Promise { * Plans every op before touching disk so a bad hunk in the last file cannot * leave the earlier files half-patched. */ -export function createApplyPatchTool(cwd: string): AgentTool { +export function createApplyPatchTool( + cwd: string, + guard: ApplyPatchGuard, +): AgentTool { + const isExtraDenied = createExtraDeniedPathMatcher( + guard.extraDeniedPaths ?? [], + ); + const contained = (path: string): string => + guardedPath(cwd, path, guard, isExtraDenied); return stringTool({ definition: applyPatchDefinition, handler: async (rawArgs: Record): Promise => { @@ -89,7 +127,7 @@ export function createApplyPatchTool(cwd: string): AgentTool { try { const patch = parseCodexApplyPatch(parsedArgs.input); const steps: Planned[] = []; - for (const op of patch.ops) steps.push(...(await plan(cwd, op))); + for (const op of patch.ops) steps.push(...(await plan(op, contained))); for (const step of steps) { if (step.kind === "write") { await mkdir(dirname(step.path), { recursive: true }); diff --git a/src/agent/tools.ts b/src/agent/tools.ts index 7ef4c3607..afeb19ce2 100644 --- a/src/agent/tools.ts +++ b/src/agent/tools.ts @@ -666,7 +666,13 @@ export async function createAgentToolset( rootsProvider: createWorktreeRootsProvider(cwd), }), createUseSkillTool(cwd, skillDirs, args.telemetry), - createApplyPatchTool(cwd), + createApplyPatchTool(cwd, { + allowOutside: () => permissionGate.getSkipPermissions(), + rootsProvider: createWorktreeRootsProvider(cwd), + ...(args.secretGuardExtraDeniedPaths !== undefined + ? { extraDeniedPaths: args.secretGuardExtraDeniedPaths } + : {}), + }), builtinExaEnabled ? createExaMCPWebFetchTool({ connect: waitForBuiltinExaConnection }) : createWebFetchTool(), diff --git a/src/subagent/run.ts b/src/subagent/run.ts index 5f654080d..ff30fb7af 100644 --- a/src/subagent/run.ts +++ b/src/subagent/run.ts @@ -68,6 +68,7 @@ import { import { normalizeToolDefinitionsForProvider } from "../agent/tool-schema-normalize.js"; import { canonicalToolName } from "../agent/canonical-tool-name.js"; import { createApplyPatchTool } from "../agent/apply-patch-tool.js"; +import { createWorktreeRootsProvider } from "../permission/worktree-roots.js"; import { checkMountedRequiresTools, formatCapabilityUnavailable, @@ -811,7 +812,16 @@ async function runSubAgentInner( ), ) ) { - tools = [...tools, createApplyPatchTool(params.cwd)]; + tools = [ + ...tools, + createApplyPatchTool(params.cwd, { + allowOutside: () => permissionGate.getSkipPermissions(), + rootsProvider: createWorktreeRootsProvider(params.cwd), + ...(params.secretGuardExtraDeniedPaths !== undefined + ? { extraDeniedPaths: params.secretGuardExtraDeniedPaths } + : {}), + }), + ]; } hostCommandsMounted = tools.some( (tool) => canonicalToolName(tool.definition.name) === "run_shell",