From 403a1a697da9008101b4849b013c98a7bead142a Mon Sep 17 00:00:00 2001 From: Nguyen Thanh Dat Date: Fri, 11 Sep 2026 04:13:12 +0700 Subject: [PATCH] fix(guardrails): mask PII inside a tool_result's nested content (#12930) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Boarded with 13 sibling PRs into one worktree off release/v3.8.51 and validated as a set: 132 focused tests pass across all 15 test files in the batch, typecheck:core is clean, check-changelog-integrity reports no lost base bullets, and check-file-size is green. Your PR merged without conflict against its siblings. Thank you — the write-up made this reviewable: measuring the behaviour on the release tip and showing the before/after table meant the defect could be confirmed rather than taken on faith. --- .../fixes/12930-pii-nested-tool-result.md | 1 + src/lib/guardrails/piiMasker.ts | 15 ++- tests/unit/pii-nested-tool-result.test.ts | 104 ++++++++++++++++++ 3 files changed, 116 insertions(+), 4 deletions(-) create mode 100644 changelog.d/fixes/12930-pii-nested-tool-result.md create mode 100644 tests/unit/pii-nested-tool-result.test.ts diff --git a/changelog.d/fixes/12930-pii-nested-tool-result.md b/changelog.d/fixes/12930-pii-nested-tool-result.md new file mode 100644 index 0000000000..2309f2e9f1 --- /dev/null +++ b/changelog.d/fixes/12930-pii-nested-tool-result.md @@ -0,0 +1 @@ +- **fix(guardrails):** mask PII inside a `tool_result`'s nested content array, which the masker walked past while redacting its sibling block ([#12930](https://github.com/diegosouzapw/OmniRoute/pull/12930)) diff --git a/src/lib/guardrails/piiMasker.ts b/src/lib/guardrails/piiMasker.ts index cb3b77f956..249b9e2a0b 100644 --- a/src/lib/guardrails/piiMasker.ts +++ b/src/lib/guardrails/piiMasker.ts @@ -57,11 +57,18 @@ function applyToContentValue( modified ||= result.modified; record.text = result.text; } - if (typeof record.content === "string") { - const result = sanitizeStringValue(record.content); - detections.push(...result.detections); + // Recurse rather than only masking a string `content`. A tool_result + // block carries its payload as an array of parts, which is what every + // agentic client sends back, and the string-only test walked straight + // past it: the outer text block was redacted while the tool output next + // to it reached the provider intact. This is the same call + // sanitizeMessageLikeList already makes one level up, so the two agree + // on how deep masking goes. The payload is a JSON round-trip, so it is + // acyclic and the recursion is bounded by its nesting. + if ("content" in record) { + const result = applyToContentValue(record.content, detections); modified ||= result.modified; - record.content = result.text; + record.content = result.value; } return record; } diff --git a/tests/unit/pii-nested-tool-result.test.ts b/tests/unit/pii-nested-tool-result.test.ts new file mode 100644 index 0000000000..e9bdcbf592 --- /dev/null +++ b/tests/unit/pii-nested-tool-result.test.ts @@ -0,0 +1,104 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +process.env.PII_REDACTION_ENABLED = "true"; + +import { PIIMaskerGuardrail } from "../../src/lib/guardrails/piiMasker"; +import type { GuardrailContext } from "../../src/lib/guardrails/base"; + +const SSN = "123-45-6789"; +const CONTEXT = {} as GuardrailContext; + +const guardrail = new PIIMaskerGuardrail(); + +async function mask(payload: unknown) { + const result = await guardrail.preCall(payload, CONTEXT); + const out = (result as { modifiedPayload?: unknown }).modifiedPayload ?? payload; + return { + out, + serialised: JSON.stringify(out), + meta: result.meta as Record | null, + }; +} + +const userTurn = (content: unknown) => ({ messages: [{ role: "user", content }] }); + +test.describe("PII masking reaches nested content blocks", () => { + // The defect. A tool_result carries its payload as an array of parts, which + // is what every agentic client sends back after running a tool. The masker + // only descended into a `content` that was a string, so it walked past this. + test("a tool_result's array content is masked", async () => { + const { serialised } = await mask( + userTurn([ + { type: "text", text: `visible ${SSN}` }, + { + type: "tool_result", + tool_use_id: "toolu_1", + content: [{ type: "text", text: `tool output ${SSN}` }], + }, + ]) + ); + + assert.ok(!serialised.includes(SSN), `SSN survived: ${serialised}`); + assert.equal(serialised.match(/\[SSN_REDACTED\]/g)?.length, 2); + }); + + test("the sibling block being masked is not enough on its own", async () => { + // Pins what the bug looked like from outside: the payload came back + // `modified: true` with a redaction in it, so nothing downstream could tell + // that a second copy of the same SSN had gone out untouched. + const { out } = await mask( + userTurn([ + { type: "text", text: `visible ${SSN}` }, + { type: "tool_result", content: [{ type: "text", text: `tool output ${SSN}` }] }, + ]) + ); + + const blocks = ( + out as { messages: { content: { text?: string; content?: { text: string }[] }[] }[] } + ).messages[0].content; + assert.equal(blocks[0].text, "visible [SSN_REDACTED]"); + assert.equal(blocks[1].content?.[0].text, "tool output [SSN_REDACTED]"); + }); + + test("nesting deeper than one tool_result is still reached", async () => { + const { serialised } = await mask( + userTurn([ + { + type: "tool_result", + content: [{ type: "tool_result", content: [{ type: "text", text: `deep ${SSN}` }] }], + }, + ]) + ); + + assert.ok(!serialised.includes(SSN), `SSN survived: ${serialised}`); + }); + + // The branch this change replaces, so it cannot be lost silently. + test("a string content on a block is still masked", async () => { + const { serialised } = await mask( + userTurn([{ type: "tool_result", tool_use_id: "toolu_1", content: `tool output ${SSN}` }]) + ); + + assert.ok(!serialised.includes(SSN), `SSN survived: ${serialised}`); + }); + + test("a payload with nothing to mask is passed through unchanged", async () => { + const payload = userTurn([ + { type: "tool_result", content: [{ type: "text", text: "no personal data here" }] }, + ]); + + const result = await guardrail.preCall(payload, CONTEXT); + + assert.equal((result as { modifiedPayload?: unknown }).modifiedPayload, undefined); + }); + + test("the nested detection is counted, not just redacted", async () => { + const { meta } = await mask( + userTurn([{ type: "tool_result", content: [{ type: "text", text: `tool output ${SSN}` }] }]) + ); + + assert.equal(meta?.redacted, true); + assert.equal(meta?.detections, 1); + }); +});