Repository navigation
Conversation
The ai-pii protocol profiles only extracted the plain text member of a content part, so text nested in a document part (source.data of a text source), a file part (a textual base64 file_data) and the arguments of a tool call was never scanned. With REDACT or BLOCK that leaked identifiers to the model provider or the caller. - add PiiContentParts to share content part and tool call extraction between the chat and responses profiles - extract document source.data (text source only) and tool_calls / function_call arguments; extract responses function_call arguments - add PiiTextSlot.ofBase64TextData to decode a textual base64 file_data for scanning and re-encode it after redaction; binary media types, base64 document sources and non textual file_data stay out of scope - update PiiProtocolProfile javadoc, README and the affected tests to reflect that function arguments are now scanned Fixes #8112
Aias00
left a comment
There was a problem hiding this comment.
This closes the gap I reported in #8112, and the approach is right: rather than widening the profiles ad hoc, extraction moved into PiiContentParts and both profiles share it, with the existing overlapping-range merge in PiiScanner doing the dedup. Confirmed correct:
collectDocumentSlotsonly takessource.datawhensource.type == "text", so a base64 document source is genuinely out of scope rather than accidentally decoded.isTextualMediaTyperestrictsfile_datatotext/*andapplication/json, which excludes image and audio parts.dataUrlPrefixrequires the;base64suffix before the comma, so a plaindata:text/plain,hellois not treated as base64.PiiScanner.scanalready skips a null or emptytext()(line 70), so a slot that cannot be read does not NPE downstream.- README,
PiiProtocolProfilejavadoc and the renamedtestToolCallArgumentsAreRedactedInResponseall track the behaviour change, instead of leaving the old "a redaction never touches function arguments" sentence behind.
One item I would like fixed before merge, and four smaller ones.
1. Base64.getDecoder() silently drops a large class of real payloads (should fix)
decodeDataUrl uses Base64.getDecoder(), which is the strict RFC 4648 decoder. It rejects:
- base64 with line breaks — the MIME-style wrapping that many clients and libraries emit, especially for anything a user pasted or that was chunked at 76 characters
- the URL-safe alphabet (
-and_) instead of+and/
Either one throws IllegalArgumentException, which you catch and turn into null, and PiiScanner then treats the slot as empty and continues. So a file_data of data:text/plain;base64,<wrapped base64 containing a phone number> is passed through untouched — even under a fail-closed rule, because nothing about the outcome is treated as a failure. That is the same shape of defect this PR exists to fix: PII leaves unredacted and nothing is logged.
Suggested fix: try Base64.getMimeDecoder() first (it tolerates line breaks and skips other non-alphabet characters), fall back to Base64.getUrlDecoder(), and only then give up. Re-encoding with getEncoder() in replace() is fine for the output side.
2. text() re-decodes on every call, with no size bound
text() calls decodeDataUrl(parent.get(member)) each time it is invoked. PiiScanner calls it at least once per slot, and redact reads the text again. A multi-megabyte file_data is therefore base64-decoded repeatedly per request, allocating a fresh String each time.
The per-direction deadline does not help here: it is enforced by the deadline-aware CharSequence that the detectors walk, but decoding happens before any CharSequence exists. A large enough payload spends its time and memory in decodeDataUrl with nothing bounding it.
Two things worth doing: cache the decoded text per slot for the lifetime of one scan, and cap the payload length you are willing to decode, treating an over-limit payload as an explicit fail-closed rejection (or at minimum a warning) rather than a silent skip.
3. replace() normalises the base64 encoding
Re-encoding with Base64.getEncoder() emits padded, unwrapped base64. If the original was MIME-wrapped or URL-safe, the member changes encoding style after a redaction. It stays a valid data URL, so this is cosmetic rather than a correctness bug, but it does mean the "everything outside the redacted range is preserved" property does not hold for this member. Worth a sentence in the javadoc so it is a documented tradeoff rather than a surprise.
4. OpenAiResponsesProfile reads arguments from any item
PiiTextSlot arguments = PiiTextSlot.ofMember(object, "arguments");This is unconditional in collectItemSlots, while document and file parts are both gated on their type. Any responses item that happens to carry a string arguments member gets scanned as message text. Today only function_call has one, so the practical risk is low, but gating on "function_call".equals(typeOf(object)) would make the rule explicit and keep the three extractions consistent with each other.
5. Minor
addIfPresent is called for the plain text member before the type dispatch, so a document part that also carries a text member yields two slots. Harmless given the overlap merge, just noting it is intentional rather than an oversight.
The test coverage is good — document text, textual and binary file_data, chat tool_calls, responses function_call, and the textless-message regression are all pinned. The testStrictToolCallOnlyResponseRemainsUnchanged → testToolCallArgumentsAreRedactedInResponse rewrite is the right call: keeping the old name while asserting the opposite would have been confusing.
Item 1 is the only one I would hold the merge on; the rest can be follow-ups if you prefer.
- decode file_data with the MIME decoder (tolerates line breaks) and fall back to the URL safe decoder, instead of the strict decoder only - extract Base64TextData util and Base64TextDataSlot, caching the decoded text for the lifetime of one slot - treat an undecodable textual file_data as a malformed extraction shape, so it follows the rule failure policy (fail closed reject / warn) instead of being silently skipped; no extra payload cap is added because the body is already bounded by maxBodySize before parsing - gate responses arguments extraction on type == function_call - document that replace() normalises the base64 encoding and that the text+document double slot is intentional (merged by the overlap merge)
|
Thanks for the thorough review @Aias00! All five items are addressed in 3eca7cf: 1. Robust base64 decoding (blocker) — Decoding moved into a new 2. Decode cost & silent skips — Two changes:
On the length cap: I deliberately did not add a separate decode cap. The whole request body is already bounded by the rule's 3. Encoding normalisation on replace — Documented in the 4. Arguments gating — Responses extraction now gates 5. text + document double slot — Comment added in Verification: |
Aias00
left a comment
There was a problem hiding this comment.
Found a JSON-escaping gap in the new tool-argument scanning path. A targeted PiiScanner regression test, testEscapedEmailInToolArgumentsIsRedacted, confirms that a valid JSON argument value decoding to alice@example.com passes the EMAIL scanner unchanged when @ is encoded as a Unicode escape.
| */ | ||
| static void collectFunctionCallArguments(final JsonObject item, final List<PiiTextSlot> slots) { | ||
| if ("function_call".equals(stringMember(item, "type"))) { | ||
| addIfPresent(slots, PiiTextSlot.ofMember(item, "arguments")); |
There was a problem hiding this comment.
[P2] Scan decoded JSON values inside function arguments
The arguments member is itself serialized JSON. Treating it as an ordinary text slot scans the JSON representation rather than its decoded values, so valid escaping hides PII: arguments containing {"email":"alice\u0040example.com"} decode to alice@example.com when the tool parses them, but the EMAIL detector sees no @ and forwards the value unchanged. The Chat tool_calls and legacy function_call paths above have the same issue.
Please expose decoded string values from the nested argument JSON as slots and serialize the modified argument object back, with explicit handling for malformed arguments. I reproduced this through OpenAiResponsesProfile.responseSlots() and PiiScanner with EMAIL enabled; testEscapedEmailInToolArgumentsIsRedacted fails because outcome.isChanged() is false.
The arguments member is itself serialized JSON, so scanning its raw text missed PII hidden behind escaping (an at-sign encoded as a unicode escape decodes to a real address at the tool). Parse the arguments and expose each decoded string value as a slot, re-serialize the argument document on redaction to preserve siblings and structure, and treat malformed arguments as an unsupported shape so a fail-closed rule rejects them. Covers chat tool_calls, legacy function_call and responses function_call. Addresses the CHANGES_REQUESTED review on #8116.
What this PR does
The
shenyu-plugin-ai-piiprotocol profiles only extracted the plaintextmember of a content part, so text nested in structured parts was never scanned
and
REDACT/BLOCKrules silently let PII through. This PR extends thechat / responses extractors to cover the missed cases.
documentcontent parts: extractsource.datawhensource.type == "text"(a base64 document source is binary and stays out of scope).
filecontent parts: addPiiTextSlot.ofBase64TextData, which decodes atextual base64
file_data(text/*orapplication/json) for scanning andre-encodes it after redaction, keeping the
data:URL header. Binary mediatypes (image / audio) and non textual
file_datayield no slot.tool_calls[].function.arguments, the legacyfunction_call.arguments(chat) and theargumentsof a responsesfunction_callitem as text slots.PiiContentPartshelper; the existing overlapping-range merge in
PiiScanneris reused, so thesame text reached via multiple paths yields a single merged redaction.
This changes the previously documented design ("a redaction never touches ...
function arguments"), so the
PiiProtocolProfilejavadoc, the plugin README andthe affected tests are updated accordingly. The broader "non chat/responses
shapes" item (#4 in the issue, a design topic) is intentionally out of scope.
Related Issue
Fixes #8112
Type of change
How was this tested
PiiProtocolProfilesTest,PiiResponseDecoratorTest)./mvnw test -pl shenyu-plugin/shenyu-plugin-ai/shenyu-plugin-ai-piipassed (82 tests)./mvnw checkstyle:checkpassed (0 violations) andapache-rat:checkcleanfile_data,chat tool_calls and responses function_call arguments
Checklist