Grep results bypass the secret scrub entirely (CL-5717)

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<ToolResult> => ({
+        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<typeof mw> => 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<ToolResult> => ({
+        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<typeof mw> => 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]*/);
+  });
 });

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,
   ];
 }

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,

src/plugins/ripgrep-plugin.ts

@@ -1,15 +1,13 @@
 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,
   runBoundedSearchFiles,
   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);