fix(redact): strip complete ANSI CSI sequences before control-split masking - #81121
fix(redact): strip complete ANSI CSI sequences before control-split masking#81121Parker-Fawcett wants to merge 4 commits into
Conversation
alt-glitch
added
type/security
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
The issue premise is confirmed on current main, and this is the only candidate head I tested that closes both the CSI-glue leak and the mixed ESC+newline remainder leak. One blocking shadow-map bug remains: _strip_shadow() can classify arbitrary live text as stripped CSI noise and delete it from output. The seven relevant regression methods pass against this head, current-main merge-tree is clean, and the adversarial case below still fails.
There is also an attribution-gate problem: the commit author email Parkerscottfawcett@gmail.com is neither an auto-resolving +<id>@users.noreply.github.com address nor mapped under contributors/emails/ / the legacy map. Add the contributor mapping in a follow-up commit; no history rewrite is needed.
| out_chars = [] | ||
| keep = bytearray(len(text)) | ||
| orig_idx = [] | ||
| rem = _ANSI_CSI_SEQ_RE.search |
There was a problem hiding this comment.
Blocking: this must be _ANSI_CSI_SEQ_RE.match, not .search. At an ESC that does not begin CSI, search(text, i) can find a later CSI and then lines 593–595 mark everything from the earlier ESC through that later sequence as stripped noise. The keep validation consequently blesses arbitrary live bytes, and a prefix split around that region causes the redactor to erase them. Reproduced at this head:
text = 'sk-' + 'a'*5 + '\x1bNOT_TOKEN_TEXT\x1b[31m' + 'b'*10
redact_sensitive_text(text, force=True)
# 'sk-aaa...bbbb' — NOT_TOKEN_TEXT is deletedUsing .match(text, i) strips a CSI sequence only when it starts at the current ESC; otherwise the existing bare-control path handles that ESC. Add this exact non-CSI-ESC-before-later-CSI case as a regression test and assert NOT_TOKEN_TEXT survives.
Attribution gate for NousResearch#81121.
|
Thanks for the review — repro confirmed at head, and both findings are addressed in two pushed commits. On the shadow-map bug: applied the prescribed Commit 6180503 therefore adds a second gate. Worth noting: the same repro also mangles text on current Attribution: added Suite: |
…asking A vendor-prefixed token wrapped in ANSI color codes (\x1b[32msk-…\x1b[0m) leaked ENTIRELY: the split-join pass stripped only the bare ESC byte, leaving [32m glued to the token head. The literal 'm' then defeated _PREFIX_RE's (?<![A-Za-z0-9_-]) lookbehind in both the shadow copy and the original, so the full token survived redaction. Strip complete CSI sequences (mirroring tools/ansi_strip.py) instead of the bare byte so the token realigns in the shadow copy; track stripped bytes in a per-index keep map so the orig-span validity check still accepts only token-body + noise bytes. Also closes the ESC+newline residual leak: a span carrying BOTH a line boundary and an escaped body (sk-<head>\x1b<mid>\n<…>) used to skip the join wholesale (the line-boundary guard from NousResearch#80987) and leak every byte after the self-matching head. The join now clips to the first line segment instead, masking the escaped middle while leaving later lines untouched. Closes NousResearch#81012.
At an ESC that does not begin a CSI sequence, _strip_shadow() used _ANSI_CSI_SEQ_RE.search(text, i), which jumps ahead to a LATER sequence and marks everything from that ESC through its end as strippable noise. keep[] then blessed arbitrary live bytes and the greedy control-split join deleted them (``sk-aaaaa\x1bNOT_TOKEN_TEXT\x1b[31m...`` erased NOT_TOKEN_TEXT). Strip a complete CSI only when it starts AT the ESC (.match), classify each stripped byte by kind (bare control vs CSI-sequence byte), and refuse join spans whose stripped regions mix kinds: live text between an orphan ESC and a later color sequence is not provably part of one smuggled token. Kind-uniform spans keep both smuggling shapes covered (repeated bare-control splits NousResearch#77484, color-glued fragments NousResearch#81012). Adds the non-CSI-ESC-before-later-CSI regression asserting the live text survives.
Attribution gate for NousResearch#81121.
037da82 to
e667973
Compare
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Exact-head rereview — one remaining redaction bypass
Reviewed exact head e6679736311d54284c1b0234698266cb01eee842 against current main@530028c213ae9eed5d7f1a826451e0edf24a11d2.
The two findings from my prior review are substantively addressed:
_strip_shadow()now uses_ANSI_CSI_SEQ_RE.match(text, i), so an orphan ESC no longer causes a later CSI search hit to reclassify the intervening bytes as stripped noise;- the requested
NOT_TOKEN_TEXTregression is present and the contributor email mapping is now committed.
One blocking fail-open remains in the replacement kinds != 3 gate.
Blocking — mixing a CSI sequence with one bare control disables masking
The new join deliberately rejects every candidate span containing both stripped-byte classes: 1 for a bare control and 2 for a complete CSI sequence. But those classes are both attacker-controlled token-splitting encodings. Their coexistence is not evidence that the candidate is unrelated prose.
Exact reproduction on this head:
text = "sk-" + "a" * 5 + "\x1b[31m" + "b" * 5 + "\u200b" + "c" * 10
result = redact_sensitive_text(text, force=True)
assert result == text # currently passes: the credential survives unchangedThe shadow is the valid prefix-shaped token sk-aaaaabbbbbcccccccccc, so _PREFIX_RE finds it. The original span contains one complete CSI sequence and one zero-width character, making kinds == 3; the new gate discards the match. The ordinary contiguous-prefix pass cannot cross either inserted sequence, so no later layer masks it.
This reopens the exact threat model the function exists to close: a secret can be smuggled through logs by using two supported control encodings instead of one. The same bypass applies to the other prefix families because the rejection is downstream of the shared _PREFIX_RE match.
Required repair
Mixed control encodings must remain fail-closed. Preserve the unrelated-live-text regression through a segmentation/back-map rule that identifies which original bytes a candidate owns; do not treat diversity of stripped-noise kinds as proof that the candidate is safe to emit. Add a production-path regression with one complete CSI sequence plus one bare/zero-width split inside the same credential and prove the reconstructed token and its body fragments do not survive.
The PR remains open, non-draft, mergeable, three commits ahead / four behind current main; the four main-side commits do not touch the three changed files. Exact-head CI 32604914244, Docker 32604913826, and Nix 32604913797 are all action_required with no hosted jobs executed. No merge recommendation until this mixed-kind bypass is closed and fresh exact-head evidence exists.
…asked The kinds!=3 gate over-rejected: a credential split by one complete CSI sequence PLUS one bare/zero-width control (both attacker-chosen encodings) disabled masking entirely, reopening the exact smuggling path the function exists to close. The data-loss hazard is narrower: an ORPHAN ESC (a bare \x1b that began no CSI) coexisting with a formed CSI marks mangled/truncated terminal output, where live text between the two regions is not provably part of one smuggled token. Only that combination now refuses the join; each splitter alone stays bridgeable and mixed non-ESC encodings remain fail-closed-masked.
|
Fixed in bcdd84a. You're right that kind-diversity was the wrong invariant — both stripped classes are attacker-chosen splitting encodings, so their coexistence is not evidence of unrelated prose. The gate now targets the actual data-loss signature: an orphan ESC (bare Everything else is fail-closed-masked again:
Suite: |
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Exact-head rereview — two fail-open paths remain
Reviewed exact head bcdd84af31cc6638c232fa96e3bb9c733e79386e against current main@4621a2d699daeaa92efb93dae9db076308cbe823.
The latest commit does close the exact CSI+ZWSP/NUL witness from my prior review: non-ESC controls no longer make the join fail open, and test_mixed_csi_and_bare_control_split_masked covers both shapes. The earlier .match shadow-map repair, NOT_TOKEN_TEXT preservation regression, and contributor mapping also remain present.
Two blocking bypasses remain.
Blocking — orphan ESC + CSI is still an attacker-selectable emit path
The replacement gate only narrows the same invalid invariant:
not (has_orphan_esc and has_csi)Both conditions are controlled by the text being redacted. A bare ESC is already a supported token splitter under #77484; combining it with a formed CSI sequence does not prove that the live bytes are unrelated prose.
Exact reproduction on this head:
text = "sk-" + "a" * 5 + "\x1b[31m" + "b" * 5 + "\x1b" + "c" * 10
result = redact_sensitive_text(text, force=True)
assert result == text # currently passes: the credential survives unchangedThe shadow is sk-aaaaabbbbbcccccccccc, so _PREFIX_RE finds a valid credential. No original fragment beginning with sk- reaches the ten-character body floor. The new gate sees both has_csi and has_orphan_esc, discards the only reconstructed match, and the ordinary contiguous pass has nothing to mask.
This is the prior mixed-encoding bypass narrowed from “CSI + any bare control” to “CSI + bare ESC”; it still does not implement the requested ownership/segmentation rule. The NOT_TOKEN_TEXT witness cannot safely authorize unconditional raw emission for every span with the same two attacker-chosen separators.
Required repair: no splitter combination may itself grant permission to emit the candidate unchanged. Segment or otherwise prove ownership of the live bytes so NOT_TOKEN_TEXT survives while a credential split by CSI + orphan ESC remains masked. Add the exact production-path regression above and assert the reconstructed token and each body fragment are absent.
Blocking — the 8-bit CSI form still bypasses the shadow entirely
The new matcher covers only the 7-bit ESC [ form. _CONTROL_CHARS_RE excludes C1 bytes, so a standard 8-bit CSI (\x9b) does not even enter _strip_shadow(). The repository's own tools/ansi_strip.py explicitly recognizes this ECMA-48 form.
Exact reproduction:
text = "\x9b31m" + "sk-" + "a" * 25 + "\x9b0m"
result = redact_sensitive_text(text, force=True)
assert result == text # currently passes: the credential survives unchanged_mask_control_split_tokens() takes its no-control fast path, while the literal m immediately before sk- defeats _PREFIX_RE's negative lookbehind in the original text—the same glue mechanism as #81012.
Required repair: mirror the 8-bit CSI branch already present in tools/ansi_strip.py, make the fast path recognize it, and add a regression proving a \x9b...m-wrapped credential is masked.
The PR is open, non-draft, and mergeable, four commits ahead / 64 behind current main; those main-side commits do not touch the three PR files. Exact-head CI 32621934842, Docker 32621934376, and Nix 32621934343 are all action_required with zero jobs created. No merge recommendation until both fail-open paths are closed and fresh exact-head evidence exists.
3 participants
Fixes #81012
What & why
_mask_control_split_tokensstripped only the bare ESC byte when buildingits shadow copy. For a token wrapped in ANSI color codes
(
\x1b[32msk-…\x1b[0m— no spaces, the real leak shape), the CSI parameterbytes
[32mstayed glued to the token head. The literalmis alphanumeric,so it defeats
_PREFIX_RE's(?<![A-Za-z0-9_-])lookbehind in both theshadow copy and the original, and the whole secret survives redaction
verbatim. The split-join pass never fires because the token is
not-contiguous-but-also-not-
_PREFIX_RE-matchable.The fix strips complete CSI sequences (mirroring the CSI leg of
tools/ansi_strip.py) instead of the bare ESC byte, so the token realigns inthe shadow. A per-index
keepmap marks every stripped byte (CSI member bytesand bare control chars) so the maskable-span check still accepts only
token-body + stripped-noise characters when validating the original span.
It also closes the second gap from the issue via per-line-segment clipping: a
span carrying BOTH a line boundary and an escaped body
(
sk-<head>\x1b<mid>\n<…>) used to skip the join wholesale under the#80987 line-boundary guard and leak every byte after the self-matching head.
The join now clips
end_origand the shadowbodyto the first line segment(
bisect_leftover the index map), so the escaped same-line middle ismasked while later lines stay untouched.
How to test
New regression tests:
TestControlCharSplitTokensgainsansi_wrapped_*,newline_esc_split_remainder_masked, andansi_glued_head_and_tail_both_masked.Platform
Tested on macOS (Python 3.11). Change is pure string/regex logic in
agent/redact.py; no OS-touching code,os.*, or shell — no Windowsfootguns.
tests/agent/test_redact.py(102 ✓) andtests/tools/test_terminal_output_transform_hook.py(5 ✓) pass viascripts/run_tests.sh.Related
Closes #81012. Prior art: #77484 (control-split masking), #80987 (line-boundary
guard), #80465 / #80965 (redaction emission gaps).
Note on overlap: #81060 and #81083 independently addressed the CSI-strip half.
#81083's per-line fallback still leaks the same-line ESC remainder
(
sk-<head>\x1b<mid>\n…→ the\x1b<mid>stays in cleartext, verified on itshead). This PR's per-line-segment clipping masks that middle for real.