Skip to content

Commit c668956

Browse files
committed
Close the omitted-toolName fail-open in the spill URI sandbox
An omitted toolName used to keep the legacy skip, so a future or out-of-tree caller that forgets the argument would silently skip the deny. The parameter is now required and the verdict denies when no tool identity reaches it. Pin the behavior with an omitted-name test, a gate-level authorize test, and an allowOutside execution test.
1 parent ac77922 commit c668956

3 files changed

Lines changed: 122 additions & 16 deletions

File tree

src/permission/gate.test.ts

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -475,3 +475,38 @@ describe("grant-mismatch asks carry the guard reason as a notice (CL-6824)", ()
475475
expect(seen).toHaveLength(0);
476476
});
477477
});
478+
479+
// Spill URI sandbox (CL-6727): the permission gate denies a non-reader
480+
// virtual ref at authorize time, mirroring the execution-time middleware
481+
// deny, while the exempted reader is not denied.
482+
describe("spill URI sandbox at authorize time (CL-6727)", () => {
483+
const cwd = mkdtempSync(join(tmpdir(), "gate-spill-uri-"));
484+
const gate = createPermissionGate({
485+
approvals: [],
486+
interactive: false,
487+
skipPermissions: false,
488+
reactorGated: false,
489+
cwd,
490+
});
491+
492+
test("grep + tool-output:/// is denied at authorize", async () => {
493+
const verdict = await gate.authorizeCall({
494+
id: "spill-grep",
495+
name: "grep",
496+
arguments: { pattern: "foo", path: "tool-output:///abc123" },
497+
});
498+
expect(verdict.effect).toBe("deny");
499+
expect(verdict.effect === "deny" ? verdict.reason : "").toMatch(
500+
/tool-output/,
501+
);
502+
});
503+
504+
test("read_file + the same tool-output:/// URI is not denied", async () => {
505+
const verdict = await gate.authorizeCall({
506+
id: "spill-read",
507+
name: "read_file",
508+
arguments: { path: "tool-output:///abc123" },
509+
});
510+
expect(verdict.effect).not.toBe("deny");
511+
});
512+
});

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

Lines changed: 68 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -342,15 +342,24 @@ describe("pathEscapePlugin", () => {
342342
pathEscapeBlockReason(
343343
{ options: { path: "../secret.txt" } },
344344
"/project",
345+
() => [],
346+
"read_file",
345347
),
346348
).toMatch(/escapes working directory/);
347349
expect(
348-
pathEscapeBlockReason({ filepath: "../secret.txt" }, "/project"),
350+
pathEscapeBlockReason(
351+
{ filepath: "../secret.txt" },
352+
"/project",
353+
() => [],
354+
"read_file",
355+
),
349356
).toMatch(/escapes working directory/);
350357
expect(
351358
pathEscapeBlockReason(
352359
{ paths: ["src/index.ts", "../secret.txt"] },
353360
"/project",
361+
() => [],
362+
"read_file",
354363
),
355364
).toMatch(/escapes working directory/);
356365
});
@@ -369,6 +378,8 @@ describe("pathEscapePlugin", () => {
369378
pathEscapeBlockReason(
370379
{ options: { command: "../secret.txt" } },
371380
"/project",
381+
() => [],
382+
"custom_tool",
372383
),
373384
).toBeUndefined();
374385
});
@@ -386,7 +397,12 @@ describe("pathEscapePlugin", () => {
386397
expect(result.isError).toBe(true);
387398
expect(result.content).toMatch(/escapes working directory/);
388399
expect(
389-
pathEscapeBlockReason({ [key]: "../secret.txt" }, "/project"),
400+
pathEscapeBlockReason(
401+
{ [key]: "../secret.txt" },
402+
"/project",
403+
() => [],
404+
"read_file",
405+
),
390406
).toMatch(/escapes working directory/);
391407
}
392408
});
@@ -406,7 +422,9 @@ describe("pathEscapePlugin", () => {
406422
);
407423
expect(result.isError).not.toBe(true);
408424
expect(seen()).toEqual(args);
409-
expect(pathEscapeBlockReason(args, "/project")).toBeUndefined();
425+
expect(
426+
pathEscapeBlockReason(args, "/project", () => [], "custom_tool"),
427+
).toBeUndefined();
410428
});
411429

412430
test("normalizePathArguments shares the plugin rewrite identity", () => {
@@ -513,5 +531,52 @@ describe("pathEscapePlugin", () => {
513531
);
514532
expect(blocked.isError).toBe(true);
515533
});
534+
535+
test("omitted toolName fails closed on virtual refs", () => {
536+
const omitted = undefined as unknown as string;
537+
expect(
538+
pathEscapeBlockReason(
539+
{ path: "tool-output:///abc123" },
540+
"/project",
541+
() => [],
542+
omitted,
543+
),
544+
).toMatch(/tool-output/);
545+
expect(
546+
pathEscapeBlockReason(
547+
{ path: "archive:///occ-abc" },
548+
"/project",
549+
() => [],
550+
omitted,
551+
),
552+
).toMatch(/archive/);
553+
});
554+
555+
test("allowOutside still denies a non-reader virtual ref at execution", async () => {
556+
const plugin = pathEscapePlugin("/project", () => [], {
557+
allowOutside: true,
558+
});
559+
const handler = plugin.middleware
560+
? plugin.middleware(nextHandler)
561+
: nextHandler;
562+
const spill = await handler(
563+
makeCall("grep", {
564+
pattern: "foo",
565+
path: "tool-output:///abc123",
566+
}),
567+
new AbortController().signal,
568+
);
569+
expect(spill.isError).toBe(true);
570+
expect(spill.content).toMatch(/tool-output/);
571+
const archive = await handler(
572+
makeCall("write_file", {
573+
path: "archive:///occ-abc",
574+
content: "hi",
575+
}),
576+
new AbortController().signal,
577+
);
578+
expect(archive.isError).toBe(true);
579+
expect(archive.content).toMatch(/archive/);
580+
});
516581
});
517582
});

src/plugins/path-escape-plugin.ts

Lines changed: 19 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ function escapeArgs(
5151
cwd: string,
5252
rootsProvider: RootsProvider,
5353
allowOutside: boolean,
54-
toolName?: string,
54+
toolName: string,
5555
): Record<string, unknown> {
5656
if (!allowOutside) {
5757
const reason = pathEscapeBlockReason(args, cwd, rootsProvider, toolName);
@@ -72,8 +72,8 @@ function escapeValue(
7272
cwd: string,
7373
rootsProvider: RootsProvider,
7474
allowOutside: boolean,
75-
key?: string,
76-
toolName?: string,
75+
key: string | undefined,
76+
toolName: string,
7777
): unknown {
7878
if (typeof value === "string") {
7979
return key !== undefined && looksLikePath(key)
@@ -154,24 +154,30 @@ const TOOL_OUTPUT_URI_TOOL = "read_file";
154154
const ARCHIVE_URI_TOOLS = new Set(["read_file", "grep", "search_files"]);
155155

156156
// "skip" when this tool may receive the virtual ref, a block message when it
157-
// may not, undefined when the value is an ordinary filesystem path. An
158-
// omitted toolName keeps the legacy skip so direct callers that predate the
159-
// parameter see no behavior change; the middleware and the permission gate
160-
// always pass a name.
157+
// may not, undefined when the value is an ordinary filesystem path. An omitted
158+
// toolName denies rather than skips: both production callers (the middleware
159+
// and the permission gate) always pass a name, so an omission is a caller bug
160+
// and must fail closed instead of silently skipping the deny.
161161
function virtualRefVerdict(
162162
value: string,
163163
toolName: string | undefined,
164164
): "skip" | string | undefined {
165165
if (isToolOutputLike(value)) {
166-
if (toolName === undefined || toolName === TOOL_OUTPUT_URI_TOOL) {
166+
if (toolName === TOOL_OUTPUT_URI_TOOL) {
167167
return "skip";
168168
}
169+
if (toolName === undefined) {
170+
return `cannot use a tool-output:// URI without a tool identity: ${value}. Use read_file with that URI to read the spilled output instead.`;
171+
}
169172
return `cannot ${toolName} a tool-output:// URI: ${value}. Use read_file with that URI to read the spilled output instead.`;
170173
}
171174
if (isArchiveLike(value)) {
172-
if (toolName === undefined || ARCHIVE_URI_TOOLS.has(toolName)) {
175+
if (toolName !== undefined && ARCHIVE_URI_TOOLS.has(toolName)) {
173176
return "skip";
174177
}
178+
if (toolName === undefined) {
179+
return `cannot use an archive:/// ref without a tool identity: ${value}. Only read_file, grep, and search_files accept archive:/// refs.`;
180+
}
175181
return `cannot ${toolName} an archive:/// ref: ${value}. Only read_file, grep, and search_files accept archive:/// refs.`;
176182
}
177183
return undefined;
@@ -184,7 +190,7 @@ export function pathEscapeBlockReason(
184190
args: Record<string, unknown>,
185191
cwd: string,
186192
rootsProvider: RootsProvider = () => [],
187-
toolName?: string,
193+
toolName: string,
188194
): string | undefined {
189195
return blockReasonFor(args, cwd, rootsProvider, undefined, toolName);
190196
}
@@ -232,8 +238,8 @@ function blockReasonFor(
232238
value: unknown,
233239
cwd: string,
234240
rootsProvider: RootsProvider,
235-
key?: string,
236-
toolName?: string,
241+
key: string | undefined,
242+
toolName: string,
237243
): string | undefined {
238244
if (typeof value === "string") {
239245
if (key === undefined || !looksLikePath(key)) return undefined;
@@ -272,7 +278,7 @@ function sanitizePath(
272278
cwd: string,
273279
rootsProvider: RootsProvider,
274280
allowOutside: boolean,
275-
toolName?: string,
281+
toolName: string,
276282
): string {
277283
const verdict = virtualRefVerdict(value, toolName);
278284
if (verdict === "skip") {

0 commit comments

Comments
 (0)