fix: the scanner recovers a secret an open-ended pattern's greedy quantifier swallowed (iss-188) - #202
Merged
Conversation
…edy quantifier swallowed (iss-188)
A secret pattern with an open-ended length bound (`\bghp_[A-Za-z0-9]{36,}`
and the seven other `{n,}` families) greedily consumes as many
class-matching bytes as it can find, including a following token's own
leading bytes when those fall in the same character class. This puts the
true junction between the two tokens before the match's reported end,
where iss-185's forward adjacency probe never looks: two concatenated
GitHub PATs were reported as one over-long finding, and the second
token's tail survived Redact raw, with the fail-closed residual re-scan
reporting the output clean — a live credential still reaching disk.
Before a match's reported end is accepted as final, scanAllPatterns now
walks a bounded window backward (stolenJunctions) looking for a cut where
the shortened prefix is still a whole match for its own pattern and a
token can begin there — probing that cut exactly as it already probes the
forward end. Whether a pattern needs this search is decided from the
pattern itself (a match that cannot survive losing one byte has a rigid
length and is skipped after one test), not from hand-annotated metadata,
so a pattern added later is covered without changes to patterns.go.
Cost stays bounded per match, not per match length: the one-test
early-out, a maxAdjacencyBacktrack window mirroring the existing forward
maxAdjacencyProbeWindow (both 512 bytes), and a single combined
junctionProbe alternation over the whole pattern set that generates
candidate cuts in one linear pass instead of one pass per pattern —
measured 40s unbounded against 0.3s bounded on a 200KB adversarial line.
New tests: TestConcatenatedOpenEndedSecretsBothDetected (the ledger
entry's literal repro), TestOpenEndedSecretSwallowingDifferentFamilyBothDetected
(a whole AKIA key swallowed by an alnum class run), TestJunctionBacktrackIsBounded
(cost guard). All watched fail on pre-fix code for the claimed reason, pass after.
Resolves iss-188 via `abcd capture resolve`.
Assisted-by: Claude:claude-opus-5
…ing for (iss-188) Two independent adversarial reviews of 41c62ba landed on the same remaining defect in stolenJunctions' backward search, and it reopened the exact leak iss-188 was filed to close. junctionProbe is UNANCHORED, so one of its hits can SPAN a real junction — begin before it and end after it — without beginning at it. The loop nevertheless resumed at that hit's END (`off += loc[1]`) even when the hit's own offset had just been REJECTED by wholeMatch, skipping every byte between the rejected candidate and the far end of its span. Nothing revisits a skipped range, so the true junction inside it was never tested and the search returned fewer cuts than it should: "ghp_"+32×a+"AIza"+4×b+"ghp_"+36×c → 1 finding, second token's tail raw "ghp_"+10×a+"AIza"+22×z+"sk-proj-"+40×i → 1 finding, 48 raw bytes of an OpenAI project key survive Redact In both, the skipped-over candidate is a google_api_key body — in the second case a syntactically valid secret in its own right, no filler bytes at all — and the fail-closed residual re-scan reports the redacted output clean, so a live credential still reaches disk. The fix drops the jump-to-hit-end fast path entirely and always resumes at `cut + 1`. A rejected candidate can hide a real junction one byte later and an accepted one is no different in that respect, so neither earns a special case, and one byte is the minimum advance that cannot skip a hit start. This stays bounded, which is the property the whole feature exists to protect. The loop is capped at maxAdjacencyBacktrack (512) iterations per match however long the match is, and each iteration's search is capped at maxAdjacencyBacktrack + maxAdjacencyProbeWindow bytes by RE2's linear-in-input guarantee — so the work per match is a constant factor independent of the REST of the line, and the whole scan stays linear in line length rather than quadratic. Measured on an adversarial line whose every match window is packed with candidates that all fail validation: 2.3s / 4.6s / 9.2s at 247KB / 494KB / 989KB — doubling the line doubles the time. The added cost over the previous (incorrect) jump is mostly productive: on the dense_candidate_junctions guard the scan now reports 125 findings where it reported 26. Both repros are added to adjacency_test.go and were watched failing on the pre-fix code for the claimed reason; TestJunctionBacktrackIsBounded gains dense_rejected_candidates_in_backtrack_window, the worst case for advancing one byte at a time. That guard's inputs now scale down under the race detector, whose ~15x instrumentation overhead a fixed wall-clock budget cannot absorb without losing its ability to discriminate on the uninstrumented run. Two comment claims are corrected as inaccurate, both flagged by the reviews: cost is bounded independently of the rest of the line but IS proportional to the match's own length, since wholeMatch re-runs the probe over the prefix; and a recovery that exceeds maxAdjacencyProbeWindow can miss a token ENTIRELY, not merely truncate it, when the pattern's required structural markers (jwt_shaped's two '.' separators) both fall outside the window. Two further gaps the reviews found are captured, not folded in, because neither is new to this change — both are the pre-existing iss-185 window trade-off reached through a new path, and closing them needs a different mechanism that must not reintroduce the unbounded per-match cost the window exists to prevent: iss-190 — a recovered match longer than maxAdjacencyProbeWindow is truncated, and the misaligned artificial end breaks the chain that would otherwise recover a THIRD abutting token iss-191 — junctionProbe's `(?s).` compile fallback would validate once per byte instead of once per candidate (unreachable with the bundled pattern set) Assisted-by: Claude:claude-opus-5
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
2 participants
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.
Fixes iss-188 (critical), surfaced during a pre-PR security review of iss-185's fix (bug-hunt loop round 3) and independently verified with failing tests.
The bug
internal/adapter/scanner's adjacency recovery (iss-185) closes the gap where a second secret token immediately abuts a first one with no separator — but only for FIXED-LENGTH patterns, since a fixed-length match's reported end is always the real junction.Patterns with an OPEN-ENDED quantifier (
\bghp_[A-Za-z0-9]{36,}and seven other{n,}families ininternal/adapter/scanner/patterns.go) instead greedily consume as many class-matching bytes as are available — including a following token's own leading bytes, when those fall inside the same character class. This shifts the true junction earlier than the match's reportedend, so the existing forward-only adjacency probe never runs where the second token actually starts.Concrete repro:
"ghp_" + strings.Repeat("a",36) + "ghp_" + strings.Repeat("b",36)reported exactly ONE finding coveringghp_aaa…aaaghp, and the second token's_bbb…bbbtail survivedRedactcompletely raw — with the fail-closed residual re-scan reporting the output clean. A live secret's tail reaching disk undetected, not merely a missed classification.The fix
internal/adapter/scanner/scanner.go: before a match's reportedendis accepted as final,stolenJunctionswalks a bounded window backward looking for a cut where the shortened prefix is still a whole match for its own pattern AND a token can begin there —scanAllPatternsthen probes that cut exactly as it already probes the forward end (adjacencyProbe). Whether a pattern needs this search is decided from the pattern itself (a match that can't survive losing one byte has a rigid length and is skipped after one test) rather than hand-annotated metadata, so a pattern added later is covered automatically.Two commits:
stolenJunctions,junctionProbe(one combined regexp alternation over the whole pattern set, used only as a cheap candidate generator — every candidate is re-verified with the existing anchored probe before being trusted), andmaxAdjacencyBacktrack(512 bytes, mirroring the existingmaxAdjacencyProbeWindow) to keep cost bounded per match rather than per line length.cut + 1; stays bounded at O(window²) — a constant independent of line length. Also corrects two comments that overclaimed the bound's guarantees.Both reviews' concrete repros are now regression tests:
TestStolenJunctionSearchDoesNotSkipPastRejectedCandidate,TestStolenJunctionSearchSkipsPastValidDecoySecret, plus the originalTestConcatenatedOpenEndedSecretsBothDetectedandTestOpenEndedSecretSwallowingDifferentFamilyBothDetected. All watched failing on pre-fix code for the claimed reason, passing after. Cost-bound guardTestJunctionBacktrackIsBoundedgained an adversarial case for the resume-logic fix specifically (dense_rejected_candidates_in_backtrack_window).Not fixed here — captured to the ledger instead
Both reviews independently identified a separate, pre-existing limitation, now reachable through a new path:
maxAdjacencyProbeWindow(the 512-byte forward-probe window, from iss-185) can truncate a recovered open-ended match, and when it does, the artificial truncated end doesn't align with the token's real end — breaking the recovery chain for a third abutting token, which becomes invisible entirely rather than merely truncated. Properly fixing this needs a different mechanism (an adaptive window, or re-probing chained from a match's real end) that risks reintroducing the unbounded-cost trap this feature exists to avoid if rushed. Captured as iss-190. A related, lower-severity latent cost cliff injunctionProbe's regex-compile fallback path (unreachable with the bundled pattern set, needs a pathological config override) is captured as iss-191.Evidence
internal/adapter/scanner/scanner.go—stolenJunctions,junctionProbe,wholeMatch,probeAt, updatedscanAllPatternsinternal/adapter/scanner/adjacency_test.go— new/updated tests listed above.abcd/work/issues/resolved/iss-188-scanner-adjacency-greedy-steal-open-ended.md— resolved viaabcd capture resolve.abcd/work/issues/open/iss-190-…md,.abcd/work/issues/open/iss-191-…md— new captures.abcd/work/DECISIONS.md,CHANGELOG.md— updatedmake preflightgreen;go test -race ./internal/...green;gofmt -l .clean.