fix: mask sibling secrets by byte span in serialized scan snippets (iss-65) - #30
Merged
Merged
Conversation
…ss-65) A serialized scan finding stores its whole source line and Finding.MarshalJSON masked only the finding's OWN token, so two secrets on one line each leaked the other into the abcd launch --json report (which CI may archive). - sealSnippets (new, in ScanText) masks EVERY finding's span out of the shared line before serialization. Masking is by BYTE SPAN (Column + len(Matched)), not substring: a byte covered by one span shows that token's head/tail fingerprint, a byte in an overlap of two+ spans is forced to '*', an uncovered byte stays raw. Substring masking leaked the non-overlapping tail of a shorter match when a greedy neighbour (e.g. an sk-ant- key running into a JWT) partially overlapped it; byte-span masking closes every overlap topology. O(n+k) per line. - isText trims a dangling partial trailing rune before utf8.Valid, so a valid UTF-8 file whose multibyte rune straddles the 8KB sniff cap is not misread as binary and skipped (the NUL check still runs on the full chunk first). - ScanBundle surfaces an unreadable file in Unscanned instead of dropping it silently, matching binary-file visibility; the zero-coverage sentinel still fails closed. TDD: disjoint, overlapping, straddling, mid-rune, and unreadable cases each watched fail then pass. Reviewed: ruthless SHIP; security BLOCK on the first (substring) fix's overlap leak, re-reviewed PASS on the byte-span fix. Resolves iss-65 in the native ledger (abcd capture resolve). P10 (skip-list coverage floor) deferred: the result-level zero-coverage sentinel already fails closed, and per-fragment breadth capping has no crisp threshold. Assisted-by: Claude:claude-opus-4-8
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
1 participant
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.
Resolves iss-65 (native ledger) — the highest-severity item from the clean-slate adversarial sweep (a
BLOCK).The leak
The secret scanner's findings are serialized into the
abcd launch --jsonreport (which CI may archive). EachFindingstores its whole source line, butFinding.MarshalJSONmasked only the finding's own matched token. So two secrets on one line each leaked the other verbatim into the report — e.g. a minified.envlinegh=ghp_AAAA… other=ghp_BBBB…: finding A's snippet maskedAbut shippedBraw.Fixes (all in
internal/adapter/scanner)sealSnippetsmasks every finding's span out of the shared source line before serialization, by byte span (Column+len(Matched)), not substring. A byte covered by one span shows that token's head/tail fingerprint; a byte in an overlap of two+ spans is forced to*; uncovered bytes stay raw. O(n+k) per line.isTexttrims a dangling partial trailing rune beforeutf8.Valid, so a valid UTF-8 file whose multibyte rune straddles the 8 KB sniff cap is no longer misread as binary and skipped from scanning. The NUL check still runs on the full chunk first.ScanBundlesurfaces an unreadable file inUnscannedinstead of dropping it silently; the zero-coverage sentinel still fails closed.Why byte-span, not substring
The first iteration masked with
strings.ReplaceAll(longest-first). The security-reviewer caught a second BLOCK: when two matches partially overlap (a greedysk-ant-…key running into a followingeyJ…JWT), masking the longer first destroys the shorter's substring, leaving its non-overlapping tail raw. Byte-span masking closes every overlap topology (partial, nested, 3-way, adjacent, duplicate, multibyte — all verified). It also makes the pass O(n+k), resolving a quadratic the ruthless-reviewer had flagged.Tests (TDD, each watched fail→pass)
TestSerializedFindingRedactsSiblingSecret— two disjoint tokens, both directions.TestSerializedFindingRedactsOverlappingSecret— the overlap repro (sk-ant-…-eyJ…).TestIsTextMultibyteRuneAtSniffBoundary— a€straddling byte 8192.TestUnscannedUnreadableRecorded— an unreadable bundle file surfaced.make preflight+make record-lintgreen. Reviewed: ruthless-reviewer SHIP, security-reviewer PASS (after finding + re-verifying the overlap BLOCK).Deferred
P10 (per-repo
pii.jsonskip-list has no per-fragment coverage floor) is document-accepted: the result-level zero-coverage sentinel already fails closed when nothing is scanned, blank/slash fragments are already rejected, and "over-broad" has no crisp threshold.Assisted-by: Claude:claude-opus-4-8