mirror of
https://github.com/diegosouzapw/OmniRoute.git
synced 2026-09-14 10:52:17 +03:00
fix(guardrails): mask PII inside a tool_result's nested content (#12930)
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.
This commit is contained in:
1
changelog.d/fixes/12930-pii-nested-tool-result.md
Normal file
1
changelog.d/fixes/12930-pii-nested-tool-result.md
Normal file
@@ -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))
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
104
tests/unit/pii-nested-tool-result.test.ts
Normal file
104
tests/unit/pii-nested-tool-result.test.ts
Normal file
@@ -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<string, unknown> | 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);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user