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

* Add a failing test for grep output bypassing the secret scrub

ripgrepPlugin answers grep and search_files without calling next(),
so a secret-shaped string in grep output never reaches
toolResultSecretScrubPlugin, which sits later in the plugin array
(CL-5717). This test proves it: a grep hit on an AWS key and an
OpenAI-style key comes back unredacted through the real posix tool
chain.

* Prepend the secret scrub and result cap so short-circuiting plugins can't skip them

composeMiddleware wraps outer-to-inner in array order, so a plugin
positioned earlier in buildCorePosixToolPlugins still sees a call's
final result even when a later plugin (ripgrepPlugin) answers
directly without invoking its own next(). Move
toolResultSecretScrubPlugin and resultTruncationPlugin to the front
of the array so both are unconditional outer wrappers around the
entire chain, mirroring how vendor/intx-inference/src/assembly.ts
hardcodes its size-cap transform as the first, mandatory element
rather than trusting every middleware author to call next().

This closes CL-5717: grep output (and anything else a future plugin
answers without delegating) is now capped and scrubbed regardless of
where in the chain it short-circuits.

* Delete ripgrepPlugin's own char-cap helper now that the wiring caps unconditionally

bounded() and its six call sites reapplied truncateToolResultContent
by hand because ripgrepPlugin answers grep/search_files without
calling next(), so the old in-chain result-truncation plugin never
saw its output. Now that resultTruncationPlugin (and the secret
scrub) wrap the whole chain unconditionally, this duplicate
application is dead weight — six call sites are six places to forget
a future change to the cap. Deleted rather than left alongside the
new wiring.

* Fix the exploitable prepend order and make the short-circuit test cover the real wiring

Scrub was prepended outermost so it ran on already-truncated content:
a secret straddling the character-cap boundary got cut mid-pattern,
the scrub's regex no longer matched the fragment, and a bare,
unredacted piece of the credential reached the model with no
redaction marker. Truncation now sits outermost (index 0) and the
scrub sits at index 1, so the scrub always sees the full,
untruncated content — truncating already-redacted text loses nothing
sensitive, so this direction is safe in both orders where the
reverse is not. Added a permanent regression test for a secret
straddling the boundary.

Also rewrote the 'short-circuiting plugin still gets capped and
scrubbed' test: it previously hand-composed the scrub and cap
middleware in a hardcoded order, so it passed unchanged against the
pre-fix wiring and gave no protection against a real reordering of
buildCorePosixToolPlugins's output. It now takes the actual array the
builder returns, splices a short-circuiting stand-in into
ripgrepPlugin's own slot, and composes that — so reordering the real
array fails the test.

* Correct the short-circuit test's comment to match what it actually guards

The comment claimed swapping the cap and scrub back would fail this
test. It does not: the secret sits at the front of a 90KB payload,
nowhere near the cap boundary, so cap-then-scrub still leaves the
whole key intact for the scrub to catch. This test guards the
PREPENDED POSITION of both terminal concerns (moving them away from
the front of the array fails it); the RELATIVE order between them is
guarded only by the boundary-straddle test. Leaving the old comment
in place risked a future engineer reading this test as redundant
coverage and deleting the straddle test, silently reopening the
exploit.

Author: TheGreatAxios

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);