fix(redact): stop masking from eating container delimiters - #78379
Conversation
…rch#77472) Same defect class as NousResearch#43083 (_AUTH_HEADER_RE's greedy \S+ pulling in a closing quote), at the sibling patterns that fix did not touch: _ENV_ASSIGN_RE, _CFG_VALUE (_CFG_DOTTED_RE / _CFG_ANCHORED_RE) and _SECRET_HEADER_RE. Each bounds the secret with a "not whitespace" value class, which also admits a quote that closes an *enclosing* document. A value under _mask_token's 18-char floor masks to a bare "***", so that quote is deleted rather than merely displaced. It needs no JSON to bite: on main, `sh -c 'export MY_TOKEN=xyz'` masks to `sh -c 'export MY_TOKEN=***` — an unterminated quote, i.e. shell EOF. This is exactly the corruption NousResearch#43083 declared fixed. Every in-tree consumer that reparses redacted text degrades differently: * agent_runtime_helpers.dump_api_request_debug — json.loads inside `except Exception: return None`, so no request dump lands at all, for precisely the API failures the dump exists to explain. * agent.trace_upload._tool_calls_to_blocks — raises TraceRedactionError and refuses the whole upload. * tools.kanban_tools — `except json.JSONDecodeError: pass` leaves the ORIGINAL unredacted dict bound, so the secret is persisted verbatim. That one is a redaction bypass, not data loss: 3 of 4 sampled inputs stored plaintext on main. Measured on main across 21 secret shapes x 2 serializers: 11 corrupted. Fix in the masking callbacks, not the value classes. _split_trailing_ delimiter() splits a trailing delimiter run off an unquoted capture and re-emits it verbatim after the mask; _mask_preserving_delimiter() adds the balanced-pair case so `x-api-key: "abc"` masks to `x-api-key: "***"` rather than an unbalanced `***"`. Deliberately NOT copying NousResearch#43083's mechanism (excluding quotes from the value class). Implemented and A/B measured it: it truncates at an *interior* quote and re-emits the tail verbatim — PGPASSWORD=p'ass -> PGPASSWORD=***'ass — and cannot cross the escaped \" inside serialized JSON, leaking the whole value. "Real credentials never contain quotes" holds for opaque bearer tokens, not for passwords. Making the redactor leak more is the wrong direction. Known limit, documented in the helper: a truly compact serializer (separators=(",", ":")) leaves no whitespace after the value, so the capture swallows `x","b":1}` with no delimiter-only suffix to split. All three reparsing callers use indent=2 or json.dumps' default ", ", so it is unreachable today. General handling means walking parsed structure instead of running a text redactor across JSON boundaries — a caller-side change, not this helper's. Container-free output is byte-identical to main except where main was already corrupting it (verified over 22 inputs). Two intentional deltas: a secret whose final character is a quote now shows that character, and a quoted header value keeps both quotes. tests/agent/test_redact.py: 79 -> 126 tests. Asserts invariants (redact(serialize(x)) parses; quote counts stay balanced) plus one test per reparsing consumer replicating its real exception handler. Teeth check: 40 of the new tests fail against unmodified main. Refs NousResearch#77472
Review of the parent commit found two defects in it. Both reproduce.
1. Verbatim re-emission disclosed secret bytes. _split_trailing_delimiter
re-emitted whatever run _TRAILING_DELIMITER_RE matched, and that pattern
matched on *shape alone*. A secret whose own tail looked structural was
therefore republished: MY_TOKEN=a'}}}... (33 chars) printed 32 of them,
all of which the bare *** had covered on main. The disclosed alphabet is
only ["'}], so entropy per character is low, but the count was unbounded
and grew with the secret. A redactor that emits more plaintext than
before is a regression, not a fix — and the parent commit described this
as a one-character delta, which was wrong.
2. _YAML_ASSIGN_RE was a missed sibling. Its value class [^\s&]++ absorbs a
trailing quote exactly like the patterns that were fixed; the negative
lookahead only rejects a *leading* quote. So the parent commit's headline
symptom survived on a pattern it claimed was out of the class:
sh -c '\npassword: xyz' still masked to an unterminated quote. Reachable
through gateway.run._redact_approval_command, which redacts the raw
command string with code_file=False, so a multi-line command echoed into
a chat approval prompt hits this pass.
Fix for (1): a delimiter only closes something that was *opened*. The quote
is now a capture group, and the split requires that same character to appear
earlier in the subject (text[:pos]). No opening quote means the run belongs
to the secret and is masked. Chosen over capping the run length or emitting
a synthesized delimiter because it is a correctness test rather than a
magic number, and it fixes the empty-value case as a side effect.
Fix for (2): _redact_yaml now goes through _mask_preserving_delimiter.
Also fixed, exposed by a surviving mutant in the same review: dropping the
match.start() == 0 guard. It skipped the split when the capture was nothing
but the delimiter, so MY_TOKEN= left its container unterminated —
sh -c 'export MY_TOKEN=' and {"c": "MY_TOKEN=", "b": 1} both corrupted.
Measured, this branch vs the parent commit:
MY_TOKEN=a'}}}... 32 bytes re-emitted -> 0
MY_TOKEN=Tr0ub4dor&3'} 2 bytes re-emitted -> 0
sh -c '\npassword: xyz' unterminated -> balanced
sh -c 'export MY_TOKEN=' unterminated -> balanced
{"c": "MY_TOKEN=", "b": 1} unparseable -> round-trips
The container-free delta set against upstream/main is now strictly smaller
than the parent commit's: the one-character disclosure delta is gone
entirely, leaving only two repairs (a quoted curl header keeps its closing
quote; a quoted header value keeps both quotes).
tests/agent/test_redact.py: 126 -> 155. Two new classes.
TestDelimiterSplitNeverDisclosesSecretBytes asserts the property that
matters — for six structural-tail shapes, *no suffix* of the secret appears
in the output — so a partial re-emission fails too, not just a whole-run
one. TestYamlAssignDelimiter covers the fourth pattern. Teeth check: 54 of
155 fail against clean upstream/main.
Refs NousResearch#77472
|
Independent verification of the second commit ( The disclosure channel is closed. For each probe below, I searched the output for any suffix of the secret of length ≥ 2, not just the whole run: So the four rows in the PR's own before/after table hold on the real Worth noting for reviewers, because it changes how the first commit should be read: I had independently flagged Suite at the new head: Two things I did not verify, stated so nobody reads more into this than it supports: I didn't re-run the 54-failures-on-clean- Separately, and this is the honest framing rather than a status complaint: no workflow run has ever executed on this PR. The check-suites sit at The branch merges cleanly into current |
Review of the two earlier commits on this branch found three defects. This replaces their mechanism; the bug being fixed is unchanged (NousResearch#77472). What was wrong ca158df split any trailing run matching ["'][}\],]*$ and re-emitted it verbatim. Shape is not evidence: MY_TOKEN=a'}}}} (33 chars) re-emitted 32 of them, all of which the bare *** had covered. A redactor that emits MORE plaintext than before is a regression, not a fix. 2df7123 required the quote to appear earlier in the subject (quote in text[:pos]). Two further defects: (a) presence is not openness — a prose apostrophe ("it's") re-enabled the leak; (b) text[:pos] is O(pos) per match, so a pass was O(N*L). Measured on values that actually reach the split path (every value ending in a quote), us/match rose 8.4 -> 13.7 from 2k to 16k matches while base held flat at 9.6 -> 9.8. The mechanism now _DocumentStateScanner carries two pieces of state forward across re.sub callbacks, which are monotonic in position, so a pass stays O(L): * which quote is open — escape-aware, so the \" inside serialized JSON does not flip parity; * the stack of containers open — pushed on { and [ while no quote is open, so a brace inside a string is content. A split then requires the captured trailing quote to BE the open one, and the structural run after it is truncated to the prefix that closes the open containers in order (_justified_delimiter_len). That is what bounds disclosure: * "}]}}} — five closers, from json.dumps' defaults on a five-level payload — is fully justified and survives whole; * a'}}}} under a single open { keeps '} and drops the rest; * nothing open: no split at all, mask everything. A separator is accepted only as the final character and only inside a container. The stack bounds closers, but a comma closes nothing, so a run of them is bounded by nothing — the fuzzer caught ',,,, being republished whole. An earlier iteration capped the run at {0,3} instead. The cap is not a property of anything: it rejected the five-character container tail that json.dumps' default separators produce (breaking the repair this PR exists to deliver, 5 consumer tests) while still admitting three characters of a secret's own }}}}. Residual, stated plainly Quote parity is syntactic. An unbalanced quote earlier in the subject — a prose apostrophe, or one inside an earlier secret — inverts parity for everything after it. Two consequences, both no worse than main: * a genuine shell quote after prose is not repaired (main eats it too); * a secret ending in that quote re-emits it: at most 1 + the open depth, drawn from "'}],. In flat prose that is one character. Above the 18-char mask floor the head/tail window dominates and base and fix disclose the same 4-char window. Verification Differential fuzz, 2772 inputs (11 container shapes x 7 prose prefixes x 6 key patterns x 6 secrets), base vs fix: 0 inputs disclose more than the open container stack justifies. Same metric run against the two earlier commits: 321 (ca158df) and 128 (2df7123). Instrumenting the helper over the same corpus: 2054 re-emitted suffixes, 0 containing a non-delimiter byte, 0 longer than 1 + open depth. Payload bytes disclosed via the mask window: 0 inputs where fix shows a longer run than base, 128 where it shows fewer. Of the 462 corpus inputs that are valid JSON before redaction, base reparses 123 and fix reparses 462. Timing, values that all reach the split path, min of 3: matches base v2 fix 2000 19.3ms 16.8ms 21.4ms 4000 39.2ms 40.8ms 43.4ms 8000 78.5ms 93.8ms 88.7ms 16000 157.4ms 218.4ms 175.2ms scaling x2.00 x2.33 x1.98 (per doubling) A 637 KB indent=2 request dump: base 85ms, fix 129ms, reparses under both. Sibling audit, 24 patterns x 4 container shapes: base corrupts 22 cases, fix 1 (MY_TOKEN="quotedshort" inside a double-quoted shell string — byte-identical to base, pre-existing, the input is ambiguous). All 34 changed cases belong to the five target patterns; 0 immune patterns changed. Container-free output stays byte-identical to base on 17 of 22 probe inputs; the 5 deltas are the intended repairs. Mutation-tested: 10 mutations, all 10 caught (neuter parity, treat nothing-open as a match, neuter the stack check, drop the structural run, re-add the old match.start() == 0 guard, neuter escape-awareness, ignore quote state when tracking containers, make the separator non-terminal, re-add the {0,3} cap, re-emit the rejected run). Three of those survived the first run and are the reason this commit also adds odd-escaped-quote and separator-position tests. tests/agent/test_redact.py: 198 -> 259 tests, 83 of which fail against clean upstream/main. Blast radius (84 files touching "redact"): 3012 passed, 2 failed, both pre-existing on clean upstream/main. CI has never run on this PR — fork PRs from first-time contributors are gated (check-suites = action_required). All numbers above are local runs. Refs NousResearch#77472
|
Correcting my own claims: review found defects in both of my earlier commits, and the PR body described a mechanism I no longer ship. Pushed What was wrong
An intermediate iteration I did not push fixed both (real escape-aware quote parity, carried forward incrementally so a pass stays O(L)) but bounded the structural run with a What
|
ca158df8b |
2df71234e |
aad60d98c |
|
|---|---|---|---|
| over the justified bound | 321 | 128 | 0 |
| µs/match, 2k → 16k matches | — | 8.42 → 13.65 | 10.69 → 10.95 (×1.98/doubling) |
Instrumenting the helper over the same corpus: 2,054 re-emitted suffixes, 0 containing a non-delimiter byte, 0 longer than 1 + open depth. Of the 462 corpus inputs that are valid JSON before redaction, main reparses 123 and this reparses 462. Sibling audit (24 patterns × 4 container shapes): main corrupts 22, this corrupts 1 (an ambiguous input, byte-identical to main), and 0 immune patterns changed.
Mutation testing: 10 mutations, 10 caught. Three survived the first run, and that is worth reporting rather than burying — my escaped-quote fixture used an even number of \", which is parity-neutral whether or not the scanner honours the backslash, so it could not catch a scanner that ignores escapes; and there was no test for separator position. Both gaps are covered now.
The residual, stated honestly
Quote parity is syntactic, so an unbalanced quote earlier in the subject inverts it for everything after. Two consequences, both no worse than main, both verified against a clean worktree:
- A genuine shell quote after prose is not repaired —
it's fine, sh -c 'export MY_TOKEN=xyz'still loses its closing quote.maineats it too, so this is an un-repaired case rather than a regression. - A secret ending in that quote re-emits it, bounded by
1 + open container depthand drawn from"'}],. In flat prose that is one character, and only below the 18-char mask floor; above the floor the head/tail window dominates and base and fix disclose the same 4-char window.
That is asserted rather than excused — test_odd_prefix_discloses_at_most_one_quote_beyond_the_baseline pins the output to base or base + the one quote, so it cannot grow silently — and it is in the helper docstring and the PR body.
Tests: 155 → 259, of which 83 fail against clean upstream/main. Blast radius unchanged (84 files, 3012 passed, the same 2 pre-existing failures). CI still has not run on this PR — fork PRs from contributors without a merged PR are gated — so all of the above is local, on this commit.
`_split_trailing_delimiter` compared the captured trailing quote to the
quote open at the match, but never asked whether that quote was ESCAPED.
Inside a serialized JSON string the enclosing quote appears as `\"`, and
the `(\S+)` value classes absorb both bytes, so the split cut between the
backslash and the quote: the backslash went into the secret handed to
`_mask_token` (deleted outright below the 18-char floor, where the mask is
a bare `***`) and the quote was re-emitted BARE. A bare quote terminates
the JSON string early, so a document that reparsed on main no longer did.
in {"content": "sh -c \"export MY_TOKEN=9\" && echo done"}
main {"content": "sh -c \"export MY_TOKEN=*** && echo done"} parses
HEAD~ {"content": "sh -c \"export MY_TOKEN=***" && echo done"} broken
now {"content": "sh -c \"export MY_TOKEN=***\" && echo done"} parses
Reachable through `_ENV_ASSIGN_RE`, `_CFG_DOTTED_RE` and
`_SECRET_HEADER_RE`. It needs whitespace AND more content after the
escaped quote, so the capture terminates exactly at `\"`; when the escaped
quote is the last thing in the string the capture runs past it and the
`$`-anchored search lands on the document's real quote instead, which is
why the earlier 320- and 160-case sweeps (escaped quote always last) and
11,664 valid-JSON probes all reported zero regressions.
The guard keys on backslash parity, not presence: `json.dumps` doubles a
literal backslash, so a value genuinely ending in `\` is serialized `\\`
and leaves an EVEN run before the real delimiter. Only an ODD run means
the last backslash introduces an escape. Verified over literal-backslash
runs of length 0-5 before the quote. Parity upstream is unaffected — the
scanner is already escape-aware, so `open_quote` is legitimately the
enclosing quote; what must not happen is consuming the escape while
republishing its quote. Nothing after an escaped quote can close a
container, so no structural run is justified there and the run is dropped
rather than re-emitted.
Measured on 4800 cases (3 families x lengths 1-40 x quote " and ' x
indent None/2 x last/not-last/nested shapes), importing agent.redact from
each worktree with an asserted `__file__`:
reparse OK main 3882/4800 HEAD~ 4032/4800 HEAD 4800/4800
kanban raw kept main 918 HEAD~ 768 HEAD 0
new full leaks vs main 0; vs HEAD~ 0
96 reparse regressions on the P0 family go to 0 and all 438 of HEAD~'s
improvements are kept. The kanban count is what makes this a blocker
rather than cosmetic: `tools/kanban_tools.py` keeps the ORIGINAL
unredacted dict on `JSONDecodeError`, so every corrupted document is a
redaction bypass that persists the secret verbatim.
Also documents the disclosure bound the docstring understated: splitting
the delimiter shortens the token, which shifts `_mask_token`'s tail window
left, so total disclosure may move by up to `tail` bytes. Measured max 4,
always a subset of the true secret's own last 4 bytes — head6/tail4
applied to the correct token instead of to secret+delimiter, not a new
disclosure class, but the suffix bound alone did not describe it.
Knowingly left: an inner *shell* quote (`sh -c 'export MY_TOKEN=x'`)
nested inside a JSON string is still masked with the value on every tree.
`json.dumps` does not escape `'`, so the quote open at the match is the
JSON string's own and the redactor cannot tell that `'` from content. The
document parses; only the inner shell quote is lost. Unchanged from main.
`_AUTH_HEADER_RE`'s credential class `[^\s\"']+` excludes the quote — the NousResearch#43083 exclusion, which is doing its job — but NOT the backslash that escapes it. Inside a serialized JSON string the capture therefore ends `…secret\`, and masking destroys that escape, leaving the document's own `"` unescaped so the string closes one byte early: in {"content": "curl -H \"Authorization: Bearer k9v3z\""} main {"content": "curl -H \"Authorization: Bearer ***""} broken now {"content": "curl -H \"Authorization: Bearer ***\""} parses Present on main and unaffected by the delimiter split, since the split is never reached: this pattern has no quote in its capture to split on. It corrupted only credentials under the 18-char mask floor, which made it look like a floor bug. It is not — above the floor `_mask_token` keeps a 4-char tail that happened to include the backslash, so the escape survived by accident of the window. Instrumented at the boundary: len=16 captured='…abcdefgh\' mask='***' reparse FAIL len=17 captured='…abcdefghi\' mask='k9v3zq...ghi\' reparse OK Fixed with the same parity rule as the previous commit rather than by widening the value class. Excluding `\` from the class would stop the capture at any backslash and re-emit the rest verbatim — the lossy outcome `test_interior_quote_value_is_fully_masked` already rules out for the sibling patterns. Gated on the scanner: the escape is only split off when the next byte really is the quote left open at that position, so a trailing backslash before whitespace, or in flat text with nothing open, stays part of the masked credential. Measured over the requested grid (Bearer, Basic, bare credential, Proxy-Authorization; lengths 1-40; quote " and '; indent None/2; last/not-last/nested), 2400 authz cases inside a 4800-case corpus: reparse OK main 1920/2400 HEAD~ 1920/2400 HEAD 2400/2400 new full leaks 0 vs main, 0 vs HEAD~ Disclosure delta, measured as the set of secret byte positions visible in the output (min window 3), 378 of 1600 cases disclose one MORE byte than HEAD~, all at credential length >= 18 and all inside the true secret's own last-4 window; 0 cases disclose a byte outside it, and no case gains disclosure while still failing to reparse. The cause is arithmetic, not a new channel: the masker now sees the 18-byte credential instead of `credential + \` (19 bytes), so the tail window covers 4 real bytes rather than 3 plus the stray escape. Same effect the previous commit documents for the delimiter split, one byte instead of four. Below the floor the mask is `***` either way, so lengths 1-17 disclose strictly less than main.
…uote
Third site of the same mechanism, found by sweeping every pattern that
masks a value captured with a quote-permissive class. `_JSON_FIELD_RE`'s
value class `[^"]+` stops at the first `"` even when that quote is
ESCAPED, so a field value that legitimately contains one hands the
trailing backslash to `_mask_token` (deleted below the floor) while the
pass re-emits its own closing quote — which then lands bare:
in {"password": "abc\""}
main {"password": "***""} broken
now {"password": "***\""} parses
Pre-existing on main and on this branch — not introduced by the delimiter
split, which this pattern never reaches (it has its own quote group, so
`_split_trailing_delimiter` returns early). Included here because the
rubric asks for the whole bug class and this is the same escape
destruction at the one site reachable WITHOUT a serialized document nested
inside a string: an ordinary one-level `{"password": ...}` with a quote in
the value. Drop this commit if you would rather ship it separately; the
first two do not depend on it.
Reuses the parity helper rather than making the value class escape-aware.
`(?:[^"\\]|\\.)+` would be the tidier regex but it does not match at all
when the closing quote is missing (truncated/malformed JSON, which a
redactor sees), turning a masked value into an unmasked one — a disclosure
regression on exactly the inputs least able to afford it.
Scope, pinned by a second test: the fix makes the document parseable, it
does not extend the masked span. `[^"]+` still ends the value at the first
quote, so bytes after an interior quote are re-emitted verbatim as they are
on main. The improvement is still strict — an unparseable document makes
`tools/kanban_tools.py` discard the redaction and persist the ORIGINAL
dict, so the whole value leaks, versus the post-quote bytes only.
reparse (30 field cases: 4 value shapes x 3 field names x 2 indents,
lengths 1-40) main 12/30 -> 30/30, no new leaks.
What does this PR do?
redact_sensitive_text()corrupts the document it is redacting. Five patterns bound the secret with a "not whitespace" value class ((\S+), or[^\s&]+?/[^\s&]++for the config and YAML forms), which also admits the quote that closes an enclosing document. Because_mask_tokenmasks anything under its 18-char floor to a bare***, that quote is deleted rather than merely displaced.This is the same defect class as #43083 — "
_AUTH_HEADER_RE's greedy\S+credential class ate a closing quote … turning value corruption into syntax corruption" — at the sibling patterns that fix did not touch:_ENV_ASSIGN_RE,_CFG_DOTTED_RE/_CFG_ANCHORED_RE(via_CFG_VALUE),_SECRET_HEADER_RE, and_YAML_ASSIGN_RE.It needs no JSON to bite. On current
main:Why it matters: three consumers reparse redacted text, and each degrades differently
agent_runtime_helpers.dump_api_request_debugexcept Exception: return Noneagent.trace_upload._tool_calls_to_blocksTraceRedactionErrortools.kanban_toolsexcept json.JSONDecodeError: passThe third is a redaction bypass, not data loss.
Related Issue
Refs #77472 — the "redacted only by regex (
force=Truemakes it a controlled residual)" item in cluster R-DUMP. The file-mode items from that issue are covered separately by #77520 / #77655 / #77717, and its unbounded-growth item by #78395; the "exact-value redaction on every persistence path" ask remains deliberately not implemented, per #43083 (masking a credential in a replayed path poisons the replay — guarded bytests/agent/test_tool_call_arg_no_redaction.py).Type of Change
Changes Made
agent/redact.py_DocumentStateScanner— tracks, for onesub()pass, which quote is open and which containers are open at a position.re.subcalls its callback on non-overlapping matches in increasing positional order, so the state is carried forward incrementally and a whole pass stays O(L). Escape-aware (the\"inside serialized JSON does not flip parity);{/[are only counted while no quote is open, so a brace inside a string is content, not structure._justified_delimiter_len(run, stack)— how much of a trailing}]}}run is consistent with closing the containers actually open there, in order._split_trailing_delimiter(value, *, quote="", open_quote=None, stack=None)— splits a trailing delimiter off an unquoted capture and returns(secret, suffix); the suffix is re-emitted after the mask. Two requirements, both about what is open: the captured trailing quote must be the open one, and the structural run after it is truncated to its justified prefix. The rest is secret and is dropped — not re-emitted, and not handed back to the masker where the head/tail window could republish it._mask_preserving_delimiter(value, *, open_quote=None, stack=None)— for bare-value captures with no quote group of their own. Adds the balanced-pair case sox-api-key: "abc"→x-api-key: "***"instead of an unbalanced***". That branch is reachable only from_SECRET_HEADER_RE;_YAML_ASSIGN_RE's(?!['\"])lookahead rejects a value that starts with a quote. Verified by instrumentation — 0 firings from_redact_yamlover 2,872 inputs._redact_env(shared by_ENV_ASSIGN_RE,_CFG_DOTTED_RE,_CFG_ANCHORED_RE) and the_SECRET_HEADER_REand_YAML_ASSIGN_REpasses. One scanner persub()pass, since each pass runs over a freshly rebuilt subject._escapes_the_quote_at(text, quote_pos)— is the quote at that position backslash-escaped? Keyed on parity, not presence:json.dumpsdoubles a literal backslash, so a value genuinely ending in\serializes to\\and leaves an EVEN run before the real delimiter. Only an ODD run means the last backslash introduces an escape. This is the one rule behind every escape-aware branch in the module, and it is wired into three sites (see the P0 section below).tests/agent/test_redact.py— 73 → 356 tests.The mechanism, and why it is bounded by document state rather than by a cap
The structural bytes a capture absorbs are document bytes — they must be re-emitted or the document loses them. But a secret's random
}}}}run is distinguishable from a document's closing run, because the document's run has to be consistent with what is actually open at that position:xyz"}]}}}{ { { [ {"}]}}}— fully justifiedxyza'}}}}{'}— one closer justifieda+ the rest droppedpw',{',— separator, inside a containerpwpw'}]},A separator is accepted only as the final character and only with a container open. The stack bounds closers, but
,closes nothing, so a run of them is bounded by nothing — my own differential fuzzer caught',,,,being republished whole before this landed.Why not copy #43083's mechanism
The obvious move is to reuse that fix's shape: exclude quotes from the value class (
(\S+)→([^\s\"']+)). I implemented it and measured it head-to-head, then rejected it. It truncates at an interior quote and re-emits the tail verbatim:It also cannot cross the escaped
\"inside serialized JSON, which leaks the whole value and still corrupts the document:#43083's stated rationale — "real credentials never contain
"or'" — holds for the opaque bearer tokens it was written for. It does not hold for passwords, which is whatPGPASSWORD/_CFG_*match. Making a redactor leak more is the wrong direction, so the fix goes in the masking callbacks instead.Review found three defects across my earlier pushes
I had this branch adversarially reviewed before asking for yours, twice. Both earlier commits were wrong, and one of my own claims in this body was wrong. Stating it plainly rather than editing history:
ca158df8b— the split was itself a disclosure channel._TRAILING_DELIMITER_REmatched on shape alone (["'][}\],]*$) and the run was re-emitted verbatim, so a secret whose own tail looked structural got republished:MY_TOKEN=a'}}}…(33 chars) printed 32 of them, all of which the bare***had covered onmain. My first body described this as a one-character delta. That was simply wrong.2df71234e— "present" is not "open", and the check was quadratic. That commit required the quote to appear earlier in the subject (quote in text[:pos]). Two further defects:it's,couldn't) satisfies a membership test, so the leak came straight back — 2,335 of 6,000 fuzz cases.text[:pos]copies a prefix per match, i.e. O(N·L) per pass — 8.7 GB copied on a 1 MB input.aad60d98c(this push) — the cap was not a property of anything. An intermediate iteration replaced presence with real quote parity (correct, kept) but bounded the structural run with a{0,3}cap.json.dumps' default separators on a five-level payload close with"}]}}}— five structural characters — so the cap rejected the genuine container tail and left exactly the corruption this PR exists to remove (5 consumer tests failed), while still admitting three characters of a secret's own}}}}. Replaced with the open-container-stack test above.Measured across the three, same metric, same 2,772-input corpus — inputs where the candidate discloses more than the open-container stack justifies:
ca158df8b2df71234eaad60d98cKnown limitations, stated plainly
1. Quote parity is syntactic, so an unbalanced quote earlier in the subject inverts it. A prose apostrophe, or a quote inside an earlier secret, leaves a quote nominally open for everything after it. Two consequences, both no worse than
main:it's fine, sh -c 'export MY_TOKEN=xyz'still loses its closing quote.maineats it too — verified in a clean worktree — so this is an un-repaired case, not a regression."'}],; in flat prose that is a single character. Above the 18-char mask floormask_secret's head/tail window dominates and base and fix disclose the same 4-char window; the residual only appears below the floor, where base emitted a bare***.This is asserted, not excused:
test_odd_prefix_discloses_at_most_one_quote_beyond_the_baselinepins the output to base or base + the one quote, so the residual cannot grow silently.2. A truly compact serializer (
separators=(",", ":")) leaves no whitespace after the value, so the capture ends in}preceded by non-structural bytes — no delimiter-only suffix to split. That input is still corrupted, and the helper's docstring says so. It is unreachable from the three reparsing callers:dump_api_request_debugpassesindent=2;trace_uploadandkanban_toolsusejson.dumps' default", ". Of the 52separators=(",", ":")sites in the tree, none feed the redactor (wire framing, cache keys, canonical hashing).plugins/platforms/google_chat/adapter.py:1410redacts an un-indented dump but only logs it, never reparses. Handling it generally means walking the parsed structure instead of running a text redactor across JSON boundaries — a caller-side change at three sites, so not here.3. One shape stays unbalanced, identically to base:
sh -c "MY_TOKEN="quotedshort""— the input is genuinely ambiguous about which quote closes what. Byte-identical tomain.Intentional behavior deltas
Container-free output is byte-identical to
mainon 17 of 22 probe inputs; the 5 deltas are the repairs:x-api-key: "quoted"→x-api-key: "***"(wasx-api-key: ***). Matches the contract Passwords get replaced by *** but model reads back its own conversation history and fails on second tool call. #43083 already pins forAuthorization:(assert result.count('"') == 2).MY_TOKEN=averylongsecretvalueinside JSON shows…aluewhere base showed…lue". Same 4-char budget, correct bytes.An adversarial re-audit found a P0 in this PR, and two more sites of the same defect
I had this branch attacked rather than reviewed. It found a regression I introduced, plus two pre-existing siblings of the same mechanism. All three are fixed in
5e0656bdf,971dca686,59bb144ff.The P0 (mine).
_split_trailing_delimitercompared the captured trailing quote to the quote open at the match, but never asked whether that quote was escaped. Inside a serialized JSON string the enclosing quote appears as\", and the(\S+)value classes absorb both bytes — so the split cut between the backslash and the quote. The backslash went into the secret handed to_mask_token(deleted outright below the 18-char floor, where the mask is a bare***) and the quote was re-emitted bare, terminating the string early:Why every sweep in this body missed it. The defect needs whitespace and more content after the escaped quote, so the capture terminates exactly at
\". When the escaped quote is the last thing in the string the capture runs past it and the$-anchored search lands on the document's real quote instead — which is the shape all my earlier corpora generated.json.dumpsof a plain value never produces the failing shape either, so 11,664 valid-JSON probes also reported clean. My corpus was structurally blind to the family, and the fuzz numbers above should be read with that in mind.Why it was a merge blocker, not a cosmetic regression.
tools/kanban_tools.pykeeps the original unredacted dict onJSONDecodeError, so every corrupted document is a redaction bypass that persists the secret verbatim. Measured on 4,800 cases (3 families × lengths 1-40 × quote"and'×indentNone/2 × last / not-last / nested):1be70d635aad60d98c59bb144ffIndependently re-measured on a smaller focused grid (320 P0+authz probes, 120
_JSON_FIELD_REprobes), importingagent.redactfrom each worktree with an asserted__file__: reparse failures 32 → 128 → 0, kanban raw-persisted 32 → 128 → 0, JSON-field failures 48 → 48 → 0.Sibling 1 —
_AUTH_HEADER_RE, pre-existing onmain. Its credential class[^\s\"']+excludes the quote (the #43083 exclusion, doing its job) but not the backslash that escapes it. The capture therefore ends…secret\, masking destroys the escape, and the document's own"is left unescaped. It only corrupted credentials under the mask floor, which made it look like a floor bug — above the floor_mask_token's 4-char tail happened to include the backslash, so the escape survived by accident of the window:Fixed with the same parity rule rather than by widening the value class — excluding
\would stop the capture at any backslash and re-emit the rest verbatim, the lossy outcometest_interior_quote_value_is_fully_maskedalready rules out for the sibling patterns. authz reparse 1920/2400 → 1920/2400 → 2400/2400.Sibling 2 —
_JSON_FIELD_RE, pre-existing onmain.[^"]+stops at the first"even when it is escaped, so this is the one site reachable without a serialized document nested inside a string — an ordinary one-level{"password": "abc\""}corrupts:Scope, pinned by a test: the fix makes the document parseable, it does not extend the masked span —
[^"]+still ends the value at the first quote, so bytes after an interior quote are re-emitted verbatim as onmain. The improvement is still strict, because an unparseable document makeskanban_toolsdiscard the redaction and persist the whole original value. Field cases 12/30 → 30/30, no new leaks. Deliberately not switched to(?:[^"\\]|\\.)+: that regex does not match at all when the closing quote is missing (truncated or malformed JSON, which a redactor does see), turning a masked value into an unmasked one — a disclosure regression on exactly the inputs least able to afford it.Disclosure delta, stated rather than buried. Both escape fixes hand the masker the correct token instead of
token + \, which shifts_mask_token's tail window left by one byte. Measured: 378 of 1600 cases disclose exactly one more byte than the pre-guard revision, all at credential length ≥ 18, all inside the true secret's own last-4 window, 0 outside it, and 0 cases gain disclosure while still failing to reparse. Below the floor both mask to***, so lengths 1-17 disclose strictly less thanmain. This is head6/tail4 applied to the right token, not a new channel — the same effect the delimiter split has, one byte instead of four. The_split_trailing_delimiterdocstring now states this bound; the earlier suffix-only bound (delimiter-only, ≤ 1 + depth) was true but did not describe it.Knowingly left, all unchanged from
main:sh -c 'export MY_TOKEN=x') still loses its'.json.dumpsdoes not escape', so the quote open at the match is the JSON string's own and a text redactor cannot tell that'from content. The document parses; only the inner shell quote is lost._JSON_FIELD_REtruncation (above) — parseable, not fully masked. Pinned bytest_json_field_interior_quote_parses_but_still_truncatesso it is a recorded limit rather than a surprise.x-api-key: <val>'at depth 0 loses its quote to_YAML_ASSIGN_RE→_SECRET_HEADER_REdouble-masking within one call, andapi_key: <secret>inside a JSON string is redacted by no tree (_SECRET_HEADER_NAMESlistsapi-key/apikeybut notapi_key).Cost. The authz pass now builds a scanner: ~4 ms on a synthetic 112 KB payload with 2,000
Authorizationmatches (10.3 → 14.3 ms). Gate-skipped text is unaffected (23.4 → 24.9 ms on a 297 KB secret-free log).How to Test
scripts/run_tests.sh tests/agent/test_redact.py— 356 passed (was 73 at the merge base; an earlier revision of this body said 79, which was wrong — I re-measured at3aeff239b,f5be9236e,4075c8fd5and1be70d635and got 73 every time).Teeth check. Copy the new test file onto clean
upstream/mainand run it: 83 of 259 fail at the pre-guard revision; against the three trees, the escaped-quote class alone gives base37 failed / 33 passed, pre-guard46 failed / 24 passed, guarded70 passed.Differential fuzz, base vs fix, 2,772 inputs (11 container shapes × 7 prose prefixes × 6 key patterns × 6 secret shapes, both quote kinds, both serializers, secrets above and below the mask floor):
ca158df8b: 321,2df71234e: 128).1 + open depth.Mutation testing — 10 mutations, 10 caught: neuter parity; treat nothing-open as a match; neuter the stack check; drop the structural run; re-add the old
match.start() == 0guard; neuter escape-awareness; ignore quote state when tracking containers; make the separator non-terminal; re-add the{0,3}cap; re-emit the rejected run. Three of those survived the first run — the escaped-quote fixture used an even number of\"(parity-neutral either way) and there was no test for separator position. Both gaps are now covered, which is why this push addstest_odd_escaped_quote_count_does_not_flip_parityandtest_separator_is_accepted_only_as_the_final_character.Consumer regression pass:
scripts/run_tests.sh tests/agent/test_redact.py tests/agent/test_trace_upload.py tests/tools/test_kanban_redaction.py tests/agent/test_tool_call_arg_no_redaction.py tests/monitoring/test_export_redaction.py— 275 passed. (test_tool_call_arg_no_redaction.pyis the Passwords get replaced by *** but model reads back its own conversation history and fails on second tool call. #43083 guard: this PR does not re-introduce history redaction.)Blast radius over every redaction-touching test file —
scripts/run_tests.sh $(rg -ln "redact" tests/ | rg -v " 2\.py")— 84 files, 3012 passed, 2 failed; both reproduce identically on cleanupstream/main(test_approval.py::…nonrecursive_verification_artifact_cleanup…,test_api_server.py::…health_detailed_returns_ok).Sibling audit — 24 patterns × 4 container shapes (both serializers + both shell quotings): base corrupts 22 cases, the fix 1 (limitation 3 above, byte-identical to base). That audit was run before the escape work below. It is now out of date in one direction:
_AUTH_HEADER_REand_JSON_FIELD_REare no longer unaffected — both carried the same escape-destruction defect onmainand both are now fixed (see below). The remaining patterns are still byte-for-byte unaffected, each for a stated reason:_TELEGRAM_REand_PRIVATE_KEY_REemit fixed replacement text;_JWT_RE,_SIGNAL_PHONE_REand_PREFIX_REhave value classes that exclude both"and\;_DB_CONNSTR_REand_URL_BARE_TOKEN_REdo admit both, but their captures are bounded by a mandatory following@, so an escape and its quote are always consumed together — that deletes content but never leaves a bare quote, so no syntax corruption.Perf. The scanner is incremental, so a pass is O(L). Measured on the shape that actually reaches the split path (every value ends in a quote, so every match pays the cost), min of 3:
2df71234eA realistic 637 KB
indent=2request dump (2,400 messages, three secret shapes per message): base 85 ms → fix 129 ms, reparses under both. The ~1.1 µs/match constant is the scanner step; the slope is flat.Full CI-parity suite —
scripts/run_tests.sh— 3,093 files, 28,599 passed, 52 failed. None are redaction-related (no failing test name or file matchesredact). I re-ran every failing file against cleanupstream/mainto attribute them: 32 reproduce identically there, and the remaining 20 are all files that do not exist onupstream/main— stale untracked"… 2.py"duplicates in my local tree. Of the three tracked files that differ, two are whole-tree AST scans (test_update_zip_two_phase.py,test_managed_runtime_resolution.py) that fail on a stray untrackedhermes_cli/update_cmd 2.pyin my working copy, not on anything this PR touches, and the third (test_telegram_start_polling_timeout.py) fails on the parent commit2df71234eas well. Nothing in the failure set is attributable to this change.Platforms tested: macOS 27.0 (Darwin 27.0.0, arm64), Python 3.11.15 (venv) — pure-stdlib
reand string handling, no platform-conditional code, no filesystem or path behavior, so Linux / WSL2 / Windows behavior is unchanged. No config keys added, no new env vars, no docs impact.Checklist
Code
Documentation & Housekeeping
cli-config.yaml.example— N/A (no config keys added)CONTRIBUTING.md/AGENTS.md— N/A (no architecture or workflow change)