Skip to content

Commit b3e1b98

Browse files
committed
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.
1 parent 4907144 commit b3e1b98

4 files changed

Lines changed: 94 additions & 15 deletions

File tree

‎src/agent/apply-patch-tool.test.ts‎

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,15 @@
1-
import { mkdtemp, readFile, writeFile, stat } from "node:fs/promises";
1+
import { mkdtemp, readFile, stat, symlink, writeFile } from "node:fs/promises";
22
import { tmpdir } from "node:os";
33
import { join } from "node:path";
44
import { expect, test } from "bun:test";
55

66
import { createApplyPatchTool } from "./apply-patch-tool.js";
77

88
async function run(cwd: string, input: string): Promise<string> {
9-
const tool = createApplyPatchTool(cwd);
9+
const tool = createApplyPatchTool(cwd, {
10+
allowOutside: () => false,
11+
rootsProvider: () => [],
12+
});
1013
if (tool.kind !== "string") throw new Error("expected a string tool");
1114
return tool.handler({ input }, new AbortController().signal);
1215
}
@@ -65,3 +68,25 @@ test("rejects paths that escape the workspace", async () => {
6568
);
6669
expect(out).toContain("escapes the workspace");
6770
});
71+
72+
test("refuses to write a secret file", async () => {
73+
const cwd = await mkdtemp(join(tmpdir(), "apply-patch-"));
74+
const out = await run(
75+
cwd,
76+
"*** Begin Patch\n*** Add File: .env\n+TOKEN=x\n*** End Patch",
77+
);
78+
expect(out).toContain("sensitive file blocked");
79+
await expect(stat(join(cwd, ".env"))).rejects.toThrow();
80+
});
81+
82+
test("refuses to follow a workspace symlink out of the workspace", async () => {
83+
const cwd = await mkdtemp(join(tmpdir(), "apply-patch-"));
84+
const outside = await mkdtemp(join(tmpdir(), "apply-patch-outside-"));
85+
await symlink(outside, join(cwd, "link"));
86+
const out = await run(
87+
cwd,
88+
"*** Begin Patch\n*** Add File: link/pwned.txt\n+x\n*** End Patch",
89+
);
90+
expect(out).toContain("escapes the workspace");
91+
await expect(stat(join(outside, "pwned.txt"))).rejects.toThrow();
92+
});

‎src/agent/apply-patch-tool.ts‎

Lines changed: 49 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,16 @@
11
import { mkdir, readFile, rm, writeFile } from "node:fs/promises";
2-
import { dirname, isAbsolute, relative, resolve } from "node:path";
2+
import { dirname, resolve } from "node:path";
33
import { stringTool } from "@intx/agent";
44
import type { AgentTool } from "@intx/agent";
55
import type { ToolDefinition } from "@intx/types/runtime";
66
import { type } from "arktype";
77

8+
import { resolveWorkspacePath } from "../permission/path-restriction.js";
9+
import type { RootsProvider } from "../permission/worktree-roots.js";
10+
import {
11+
createExtraDeniedPathMatcher,
12+
isSensitivePathResolved,
13+
} from "../plugins/secret-guard-plugin.js";
814
import {
915
applyUpdateHunks,
1016
CodexApplyPatchError,
@@ -31,28 +37,52 @@ type Planned =
3137
| { kind: "write"; path: string; content: string; label: string }
3238
| { kind: "remove"; path: string; label: string };
3339

34-
function contained(cwd: string, path: string): string {
40+
export interface ApplyPatchGuard {
41+
allowOutside: () => boolean;
42+
rootsProvider: RootsProvider;
43+
extraDeniedPaths?: readonly string[];
44+
}
45+
46+
// apply_patch is mounted outside the posix plugin stack, so it re-enforces the
47+
// secret-guard denylist and the realpath workspace bound itself. A lexical
48+
// check alone would let a symlink inside the workspace lead out of it.
49+
function guardedPath(
50+
cwd: string,
51+
path: string,
52+
guard: ApplyPatchGuard,
53+
isExtraDenied: (value: string) => boolean,
54+
): string {
3555
const abs = resolve(cwd, path);
36-
const rel = relative(cwd, abs);
37-
if (rel === "" || rel.startsWith("..") || isAbsolute(rel)) {
56+
if (isSensitivePathResolved(abs) || isExtraDenied(abs)) {
57+
throw new CodexApplyPatchError(
58+
`Access to sensitive file blocked by policy: ${path}`,
59+
);
60+
}
61+
if (
62+
!guard.allowOutside() &&
63+
resolveWorkspacePath(cwd, abs, guard.rootsProvider) === undefined
64+
) {
3865
throw new CodexApplyPatchError(`path escapes the workspace: ${path}`);
3966
}
4067
return abs;
4168
}
4269

43-
async function plan(cwd: string, op: PatchOp): Promise<Planned[]> {
70+
async function plan(
71+
op: PatchOp,
72+
contained: (path: string) => string,
73+
): Promise<Planned[]> {
4474
if (op.type === "add") {
45-
const path = contained(cwd, op.path);
75+
const path = contained(op.path);
4676
return [
4777
{ kind: "write", path, content: op.content, label: `A ${op.path}` },
4878
];
4979
}
5080
if (op.type === "delete") {
5181
return [
52-
{ kind: "remove", path: contained(cwd, op.path), label: `D ${op.path}` },
82+
{ kind: "remove", path: contained(op.path), label: `D ${op.path}` },
5383
];
5484
}
55-
const source = contained(cwd, op.path);
85+
const source = contained(op.path);
5686
let original: string;
5787
try {
5888
original = await readFile(source, "utf8");
@@ -66,7 +96,7 @@ async function plan(cwd: string, op: PatchOp): Promise<Planned[]> {
6696
return [
6797
{
6898
kind: "write",
69-
path: contained(cwd, op.moveTo),
99+
path: contained(op.moveTo),
70100
content,
71101
label: `M ${op.moveTo}`,
72102
},
@@ -78,7 +108,15 @@ async function plan(cwd: string, op: PatchOp): Promise<Planned[]> {
78108
* Plans every op before touching disk so a bad hunk in the last file cannot
79109
* leave the earlier files half-patched.
80110
*/
81-
export function createApplyPatchTool(cwd: string): AgentTool {
111+
export function createApplyPatchTool(
112+
cwd: string,
113+
guard: ApplyPatchGuard,
114+
): AgentTool {
115+
const isExtraDenied = createExtraDeniedPathMatcher(
116+
guard.extraDeniedPaths ?? [],
117+
);
118+
const contained = (path: string): string =>
119+
guardedPath(cwd, path, guard, isExtraDenied);
82120
return stringTool({
83121
definition: applyPatchDefinition,
84122
handler: async (rawArgs: Record<string, unknown>): Promise<string> => {
@@ -89,7 +127,7 @@ export function createApplyPatchTool(cwd: string): AgentTool {
89127
try {
90128
const patch = parseCodexApplyPatch(parsedArgs.input);
91129
const steps: Planned[] = [];
92-
for (const op of patch.ops) steps.push(...(await plan(cwd, op)));
130+
for (const op of patch.ops) steps.push(...(await plan(op, contained)));
93131
for (const step of steps) {
94132
if (step.kind === "write") {
95133
await mkdir(dirname(step.path), { recursive: true });

‎src/agent/tools.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -662,7 +662,13 @@ export async function createAgentToolset(
662662
rootsProvider: createWorktreeRootsProvider(cwd),
663663
}),
664664
createUseSkillTool(cwd, skillDirs, args.telemetry),
665-
createApplyPatchTool(cwd),
665+
createApplyPatchTool(cwd, {
666+
allowOutside: () => permissionGate.getSkipPermissions(),
667+
rootsProvider: createWorktreeRootsProvider(cwd),
668+
...(args.secretGuardExtraDeniedPaths !== undefined
669+
? { extraDeniedPaths: args.secretGuardExtraDeniedPaths }
670+
: {}),
671+
}),
666672
builtinExaEnabled
667673
? createExaMCPWebFetchTool({ connect: waitForBuiltinExaConnection })
668674
: createWebFetchTool(),

‎src/subagent/run.ts‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@ import {
6868
import { normalizeToolDefinitionsForProvider } from "../agent/tool-schema-normalize.js";
6969
import { canonicalToolName } from "../agent/canonical-tool-name.js";
7070
import { createApplyPatchTool } from "../agent/apply-patch-tool.js";
71+
import { createWorktreeRootsProvider } from "../permission/worktree-roots.js";
7172
import {
7273
checkMountedRequiresTools,
7374
formatCapabilityUnavailable,
@@ -801,7 +802,16 @@ async function runSubAgentInner(
801802
),
802803
)
803804
) {
804-
tools = [...tools, createApplyPatchTool(params.cwd)];
805+
tools = [
806+
...tools,
807+
createApplyPatchTool(params.cwd, {
808+
allowOutside: () => permissionGate.getSkipPermissions(),
809+
rootsProvider: createWorktreeRootsProvider(params.cwd),
810+
...(params.secretGuardExtraDeniedPaths !== undefined
811+
? { extraDeniedPaths: params.secretGuardExtraDeniedPaths }
812+
: {}),
813+
}),
814+
];
805815
}
806816
hostCommandsMounted = tools.some(
807817
(tool) => canonicalToolName(tool.definition.name) === "run_shell",

0 commit comments

Comments
 (0)