diff --git a/src/agent/posix-tool-plugins.test.ts b/src/agent/posix-tool-plugins.test.ts index 2c632e05e..3bfe1e7dd 100644 --- a/src/agent/posix-tool-plugins.test.ts +++ b/src/agent/posix-tool-plugins.test.ts @@ -4,6 +4,7 @@ import { join } from "node:path"; import { tmpdir } from "node:os"; import { createBlobReader } from "@intx/types/runtime"; import { createPosixTools, composeMiddleware } from "@intx/tools-posix"; +import type { ToolPlugin } from "@intx/tools-posix"; import type { ToolCall, ToolResult } from "@intx/types/runtime"; import { createPermissionGate } from "../permission/gate.js"; import { buildCorePosixToolPlugins } from "./posix-tool-plugins.js"; @@ -295,4 +296,141 @@ describe("buildCorePosixToolPlugins", () => { await rm(dir, { recursive: true, force: true }); } }); + + test("a grep result containing a secret-shaped string is redacted before reaching the model (CL-5717)", async () => { + const cwd = await mkdtemp(join(tmpdir(), "ic-posix-grep-scrub-")); + try { + await writeFile( + join(cwd, "leaky.env"), + "AWS_KEY=AKIAABCDEFGHIJKLMNOP\nOPENAI_API_KEY=sk-abcdefghijklmnopqrstuvwxyz123456\n", + ); + + const gate = createPermissionGate({ + approvals: [], + interactive: false, + skipPermissions: true, + cwd, + }); + const runner = createPosixTools({ + cwd, + plugins: buildCorePosixToolPlugins({ cwd, permissionGate: gate }), + }); + + const result = await runner.run( + { id: "grep-1", name: "grep", arguments: { pattern: "AKIA|sk-", path: cwd } }, + new AbortController().signal, + ); + + expect(result.isError).not.toBe(true); + const content = String(result.content); + expect(content).not.toContain("AKIAABCDEFGHIJKLMNOP"); + expect(content).not.toContain("sk-abcdefghijklmnopqrstuvwxyz123456"); + expect(content).toContain("[redacted: looks like a credential]"); + } finally { + await rm(cwd, { recursive: true, force: true }); + } + }); + + test("a plugin that returns without calling next() still gets capped and scrubbed (CL-5717)", async () => { + // Generic, plugin-shape-agnostic version of the grep case above: any + // plugin that answers a scrubbable/truncatable tool directly instead of + // delegating to `next` must still be capped and scrubbed, because it is + // wrapped by the unconditional outer plugins in buildCorePosixToolPlugins. + // This composes the REAL production array from the builder — not a + // hand-picked middleware order — with a short-circuiting stand-in spliced + // in at ripgrepPlugin's own position, so moving both terminal concerns + // away from the front of the real array fails this test. + // + // This guards their PREPENDED POSITION only, not the RELATIVE order + // between the two of them: the secret here sits at the very front of the + // payload, nowhere near the cap boundary, so it survives even under the + // exploitable cap-then-scrub order. The relative order is guarded solely + // by the boundary-straddle test below — do not treat this test as + // redundant with it. + const secretShapedContent = `AKIAABCDEFGHIJKLMNOP\n${"x".repeat(90_000)}`; + const shortCircuitingPlugin: ToolPlugin = { + middleware: () => async (call: ToolCall): Promise => ({ + callId: call.id, + content: secretShapedContent, + }), + }; + + const gate = createPermissionGate({ + approvals: [], + interactive: false, + skipPermissions: true, + cwd: "/tmp", + }); + const plugins = buildCorePosixToolPlugins({ cwd: "/tmp", permissionGate: gate }); + const ripgrepIndex = findMiddlewareIndex(plugins, "no matches for /"); + expect(ripgrepIndex).toBeGreaterThanOrEqual(0); + plugins[ripgrepIndex] = shortCircuitingPlugin; + + const composed = composeMiddleware( + plugins.map((plugin) => plugin.middleware).filter((mw): mw is NonNullable => mw !== undefined), + async (call) => ({ callId: call.id, content: "unreachable: short-circuiting plugin never delegates" }), + ); + + const result = await composed( + { id: "short-1", name: "grep", arguments: {} }, + new AbortController().signal, + ); + + expect(result.isError).not.toBe(true); + const content = String(result.content); + expect(content).not.toContain("AKIAABCDEFGHIJKLMNOP"); + expect(content).toContain("[redacted: looks like a credential]"); + expect(content.length).toBeLessThan(secretShapedContent.length); + expect(content).toContain("[output truncated"); + }); + + test("a secret straddling the character-cap boundary is still fully redacted, not left as a bare fragment (CL-5717)", async () => { + // Regression guard for the exploitable ordering: if truncation ran before + // the scrub, a secret split mid-pattern at the cap boundary would no + // longer match the scrub's regex, and a bare, unredacted fragment of the + // credential would reach the model with no redaction marker at all. + const { MAX_RESULT_CHARS } = await import("../plugins/result-truncation-plugin.js"); + // A newline immediately ahead of the key gives the scrub regex's `\b` a + // real word boundary; the padding length puts the cap boundary partway + // through the 20-char key that follows, so a truncate-then-scrub bug + // would cut the key down to an unmatchable, unredacted fragment. + const padding = `${"x".repeat(MAX_RESULT_CHARS - 10)}\n`; + const straddlingSecret = "AKIAABCDEFGHIJKLMNOP"; // 20 chars, cap lands mid-key + const secretShapedContent = `${padding}${straddlingSecret}`; + const shortCircuitingPlugin: ToolPlugin = { + middleware: () => async (call: ToolCall): Promise => ({ + callId: call.id, + content: secretShapedContent, + }), + }; + + const gate = createPermissionGate({ + approvals: [], + interactive: false, + skipPermissions: true, + cwd: "/tmp", + }); + const plugins = buildCorePosixToolPlugins({ cwd: "/tmp", permissionGate: gate }); + const ripgrepIndex = findMiddlewareIndex(plugins, "no matches for /"); + expect(ripgrepIndex).toBeGreaterThanOrEqual(0); + plugins[ripgrepIndex] = shortCircuitingPlugin; + + const composed = composeMiddleware( + plugins.map((plugin) => plugin.middleware).filter((mw): mw is NonNullable => mw !== undefined), + async (call) => ({ callId: call.id, content: "unreachable: short-circuiting plugin never delegates" }), + ); + + const result = await composed( + { id: "straddle-1", name: "grep", arguments: {} }, + new AbortController().signal, + ); + + // The redaction marker is longer than the key it replaces, so the cap can + // still trim its tail — that's fine, it's already-redacted text. The + // security property under test is narrower: no bare, matchable-or-partial + // fragment of the raw key survives into the result. + const content = String(result.content); + expect(content).not.toContain(straddlingSecret); + expect(content).not.toMatch(/AKIA[0-9A-Z]*/); + }); }); \ No newline at end of file diff --git a/src/agent/posix-tool-plugins.ts b/src/agent/posix-tool-plugins.ts index 597537220..ef6f47a7f 100644 --- a/src/agent/posix-tool-plugins.ts +++ b/src/agent/posix-tool-plugins.ts @@ -35,8 +35,27 @@ export type CorePosixToolPluginsArgs = { }; // Middleware order matches docs/ARCHITECTURE.md: path escape through truncation, -// with shell-guard after permission so blocked commands never spawn. Secret-shaped -// result scrub runs immediately before truncation so credentials are redacted first. +// with shell-guard after permission so blocked commands never spawn. +// +// The secret scrub and the character cap are prepended unconditionally, ahead +// of every other plugin, rather than left in call order. composeMiddleware +// wraps outer-to-inner in array order, so a plugin earlier in this array +// still observes the final result even when a later plugin (ripgrepPlugin, +// notably) answers a call directly without invoking its own `next()` and so +// never reaches whatever sits after it. A mandatory terminal concern like +// redacting a credential cannot depend on every middleware author remembering +// to call `next()` — see vendor/intx-inference/src/assembly.ts's +// sizeCapTransform for the same reasoning upstream. +// +// Truncation must run outermost, ahead of (i.e. after "seeing the result of") +// the scrub — meaning the scrub sits closer to the base handler, at index 1, +// so it runs on the FULL, untruncated content and truncation only trims what +// the scrub already produced. The reverse order is exploitable: a secret +// straddling the character-cap boundary gets cut mid-pattern (e.g. +// `AKIA[0-9A-Z]{16}` losing its tail), the scrub's regex no longer matches +// the fragment, and a bare, unredacted piece of the credential reaches the +// model with no redaction marker. Scrub-then-truncate is always safe, since +// truncating already-redacted text loses nothing sensitive. export function buildCorePosixToolPlugins(args: CorePosixToolPluginsArgs): ToolPlugin[] { const { cwd, @@ -47,6 +66,8 @@ export function buildCorePosixToolPlugins(args: CorePosixToolPluginsArgs): ToolP shellEnv, } = args; return [ + resultTruncationPlugin(), + toolResultSecretScrubPlugin(), pathEscapePlugin(cwd, createWorktreeRootsProvider(cwd)), deleteFilePlugin(cwd), toolOutputUriPlugin(), @@ -65,8 +86,6 @@ export function buildCorePosixToolPlugins(args: CorePosixToolPluginsArgs): ToolP editFileDiagnosticsPlugin(), lspHintPlugin(), createLSPPlugin({ cwd, minSeverity: 1 }), - toolResultSecretScrubPlugin(), - resultTruncationPlugin(), ...extraToolPlugins, ]; } \ No newline at end of file diff --git a/src/plugins/result-truncation-plugin.ts b/src/plugins/result-truncation-plugin.ts index 9f7ec6c29..18d53c246 100644 --- a/src/plugins/result-truncation-plugin.ts +++ b/src/plugins/result-truncation-plugin.ts @@ -9,9 +9,10 @@ export const MAX_RESULT_CHARS = 80_000; // The single primitive for size truncation: callers may pass their own // threshold but never invent their own wording, so a result can never carry // two differently-worded "truncated" notices. Called directly by runners this -// middleware does not wrap — the MCP tool runner (src/mcp/plugin.ts), and -// ripgrep-plugin.ts, which answers grep without calling next and so never -// reaches this middleware despite sitting earlier in the same plugin array. +// middleware does not wrap — the MCP tool runner (src/mcp/plugin.ts). The +// posix chain gets this middleware prepended unconditionally in +// posix-tool-plugins.ts, so plugins like ripgrepPlugin that answer without +// calling next() no longer need to apply the cap themselves. export function truncateToolResultContent( content: string, maxChars: number = MAX_RESULT_CHARS, diff --git a/src/plugins/ripgrep-plugin.ts b/src/plugins/ripgrep-plugin.ts index c0994a150..6e3b8c5e2 100644 --- a/src/plugins/ripgrep-plugin.ts +++ b/src/plugins/ripgrep-plugin.ts @@ -1,7 +1,6 @@ import { statSync } from "node:fs"; import { dirname, basename } from "node:path"; import type { ToolPlugin } from "@intx/tools-posix"; -import type { ToolResult } from "@intx/types/runtime"; import { runBoundedGrep, @@ -9,7 +8,6 @@ import { type BoundedGrepArgs, } from "./bounded-grep-fallback.js"; import { createRgCollector } from "./rg-output.js"; -import { truncateToolResultContent } from "./result-truncation-plugin.js"; import { MAX_OUTPUT_BYTES, runRg, type RgLimits, type SpawnRg } from "./rg-run.js"; // A grep over a large tree with the pure-TypeScript walker enumerates the whole @@ -33,14 +31,6 @@ function capLines(text: string, max: number): string { return `${lines.slice(0, max).join("\n")}\n... (showing first ${max} of ${lines.length}+ matches; narrow path/glob)`; } -// ripgrepPlugin answers grep and search_files without calling next, so the -// result-truncation middleware sitting later in the chain never sees these -// results. The shared primitive is applied here instead, keeping one wording -// for size truncation on a path that would otherwise return uncapped. -function bounded(callId: string, content: string): ToolResult { - return { callId, content: truncateToolResultContent(content) }; -} - // Mirrors read_file's truncate-and-offer behavior: a cap or timeout still // surfaces whatever matches were collected before it fired, instead of // discarding them behind a bare error. `notice` is only set for conditions @@ -114,7 +104,7 @@ export function ripgrepPlugin(cwd: string, limits: RgLimits = {}, spawnChild?: S }; if (glob !== undefined) boundedArgs.glob = glob; const content = await runBoundedGrep(boundedArgs, signal, rgCwd); - return bounded(call.id, boundedContent(content, maxResults, maxBytes)); + return { callId: call.id, content: boundedContent(content, maxResults, maxBytes) }; } catch (err) { return { callId: call.id, @@ -130,9 +120,9 @@ export function ripgrepPlugin(cwd: string, limits: RgLimits = {}, spawnChild?: S return { callId: call.id, content: result.message, isError: true }; } if (result.kind === "partial") { - return bounded(call.id, partialContent(result.stdout, maxResults, result.notice)); + return { callId: call.id, content: partialContent(result.stdout, maxResults, result.notice) }; } - return bounded(call.id, capLines(result.stdout, maxResults)); + return { callId: call.id, content: capLines(result.stdout, maxResults) }; } if (call.name === "search_files") { @@ -150,7 +140,7 @@ export function ripgrepPlugin(cwd: string, limits: RgLimits = {}, spawnChild?: S signal, rgCwd, ); - return bounded(call.id, boundedContent(content, maxResults, maxBytes)); + return { callId: call.id, content: boundedContent(content, maxResults, maxBytes) }; } catch (err) { return { callId: call.id, @@ -166,9 +156,9 @@ export function ripgrepPlugin(cwd: string, limits: RgLimits = {}, spawnChild?: S return { callId: call.id, content: result.message, isError: true }; } if (result.kind === "partial") { - return bounded(call.id, partialContent(result.stdout, maxResults, result.notice)); + return { callId: call.id, content: partialContent(result.stdout, maxResults, result.notice) }; } - return bounded(call.id, capLines(result.stdout, maxResults)); + return { callId: call.id, content: capLines(result.stdout, maxResults) }; } return next(call, signal);