fix(redact): narrow control-split join guard to line-crossing spans - #80987
Merged
kshitijk4poor merged 2 commits intoAug 7, 2026
Merged
Conversation
Post-merge review of aecb9ca found the join guard over-broad: skipping the join whenever ANY fragment self-matches _PREFIX_RE reopened a leak for non-newline splits — sk-<15 chars>ESC<25 chars> masked only the self-matching head and left the 25-char tail in cleartext (fully masked before the guard; main never masked this shape at all, so the merged state was still >= main, but the salvage's own coverage regressed). Skip the join only when the span crosses a line boundary (\n / \r) — that is the shape where adjacent legitimate text gets swallowed (ghp_<token>-then-'button [ref=e3]' annotation bug). ESC/zero-width controls never legitimately separate a token from prose, so joining there is safe and restores full-tail masking. Both legs mutation-checked: reverting to the unconditional skip fails the new tail-mask test; removing the guard fails the annotation test.
Review follow-ups on the guard: state the accepted \n-residual in the comment, reuse the span local in the next condition instead of re-slicing, and short-circuit the substring checks before the regex.
kshitijk4poor
enabled auto-merge (rebase)
August 7, 2026 12:16
Parker-Fawcett
pushed a commit
to Parker-Fawcett/hermes-agent
that referenced
this pull request
Aug 22, 2026
…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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Narrows the control-split join guard added in #80965 (aecb9ca) so a token split by ESC/zero-width bytes is fully masked again — the guard as merged skipped the join whenever any fragment self-matched
_PREFIX_RE, which left the non-matching tail of such a split in cleartext.Root cause
The guard existed to stop a real bug (a complete token at end-of-line joining with the next text line and the mask swallowing "button [ref=e3]"). But its condition was broader than the bug: it also fired for non-newline splits where the head fragment alone was long enough to self-match, so
sk-<15 chars>\x1b<25 chars>masked only the head and leaked the 25-char tail. (Upstream main never masked this shape at all — the merged state was still strictly >= main — but the salvage's own split coverage regressed.)Fix
Skip the join only when the candidate span crosses a line boundary (
\n/\r) — the only shape where adjacent legitimate text can be swallowed. ESC/zero-width controls never legitimately separate a token from prose, so joining there is safe.Validation
sk-<head≥10>\x1b<tail>ghp_<token>\nbutton [ref=e3]annotationtests/agent/test_redact.py+tests/tools/test_browser_secret_exfil.py: 113 passedFound by the post-merge final-stack review of my own follow-up commit on #80965. Refs #77484.