diff --git a/.abcd/work/DECISIONS.md b/.abcd/work/DECISIONS.md index ad159952..c353ef6f 100644 --- a/.abcd/work/DECISIONS.md +++ b/.abcd/work/DECISIONS.md @@ -924,3 +924,5 @@ parallel-agent merge contention bites. - 2026-08-05 — iss-43, CORRECTION to the entry immediately above, from a merge-gate record-accuracy review on PR #196. That entry stands unedited; four of its claims are corrected here rather than in place, because it is merged history. (1) It opens "the three-claim corpus is closed as OVERTAKEN rather than fixed", and the corpus does not close as a unit: claims 1 and 2 — the Phase 0 status claim and the native-capability surface list — close as overtaken via `73428b6`, while claim 3's surviving half, the Layout line, is FIXED by that same PR. The entry knows this later in its own text and in the issue's `resolution:` field, so the opening verdict was a second over-claim of the same shape as the attribution it goes on to retract: a single verdict asserted over a corpus whose members had different fates. (2) "a SHALLOW clone whose truncation window opened well after 2026-07-17" over-claims. The evidence bounds the boundary to the interval after 2026-07-17 and no later than the oldest visible substantive README rewrite; "well after" asserts a distance nothing measured. Read it as "after 2026-07-17". (3) "writing the doc to describe a packaging mechanism that does not exist" is refuted by the entry's own closing paragraph. The clause scopes to the all-channel curated publish path, which genuinely does not exist and which no release has ever taken; the namespace deny itself exists and is tested. (4) The call edge is wrong in the closing paragraph: "reachable from the wired `abcd launch ship`; it is unwired, because `Ship` stops at `WouldPublish`" reads as one chain, implying the verb runs through `launch.Ship`. It does not. `launch.Ship` has NO production caller — the only calls are three in `dryrun_test.go` — while the wired verb exercises the deny through `launch.PrecheckPayload` and `launch.RenderPayload`, which resolve the bundle at `internal/core/launch/render.go:192`, reached from `internal/surface/cli/ship.go`. `release.yml` invokes neither. Both facts stand; the edge joining them did not. The lesson generalises past this entry: a summary sentence written before the detail paragraphs are finished tends to keep the verdict the round started with, and a call chain assembled from two true sentences about neighbouring functions is not evidence that either calls the other — `grep` for the callers. - 2026-08-05 — iss-118: DECISIONS.md and ACKNOWLEDGEMENTS.md gain `merge=union` in `.gitattributes`, the same remedy already in place for CHANGELOG.md. Both are the identical shape — append-only ledgers of anonymous, dated, order-independent entries that never need identity — so a concurrent-append conflict has nothing to actually disagree on; the union driver keeps both sides instead of stopping the merge. Prompted directly by the bug-hunt loop's `bugfix/iss-184-…` branch hitting exactly this conflict against `.abcd/work/DECISIONS.md` on PR #199, which iss-118 had already diagnosed (filed after an earlier merge hit the same conflict) but left unresolved pending this design call. Full atomicisation (per-decision records, a new id family, an armed uniqueness detector) was the other option iss-118 named; not pursued here as disproportionate to a minor, low-traffic hotspot. Impact recorded as `internal`, no CHANGELOG entry — matching the iss-80 precedent for a process/tooling change with no user-facing behaviour. - 2026-08-05 — iss-185 (bug-hunt loop, round 3): `ScanText`'s leading-`\b`-anchored secret patterns could never detect a second fixed-length token immediately abutting a first with no separator — the byte before the second token is itself a word character (the first token's own last byte), a word/word transition where `\b` never holds, so `FindAllStringIndex` silently returns only the first match. `Redact` then left the second token's whole body raw, and `history`'s stage-two fail-closed residual re-scan reported the redacted text clean anyway, so a concatenated pair of secrets reached disk with one still live. Landed after three rounds of pre-PR/pre-merge adversarial review, each catching a real problem in the pass before it: (1) the first pass's own-pattern-only probe missed a mixed pair of different fixed-length patterns, and double-counted a `google_api_key` match whose dash-terminated 35th char already satisfied `\b` on its own; (2) the fix for that, an unanchored `FindStringIndex` probe, cost O(matches × patterns × remaining line length) — measured at 14–49s on large single-line input; (3) anchoring the probe with `\A` fixed the *restart-at-every-offset* cost but not the cost of one attempt against two patterns (`net_lan_hostname`, `net_device_hostname`) that carry their own unbounded internal quantifier, still quadratic on many back-to-back fixed-length tokens. The landed shape: `adjacencyProbe` compiles an `\A`-anchored, `\b`-stripped (past any inline flag group) variant of each pattern; `scanAllPatterns` probes every pattern's variant, within a small fixed window (`maxAdjacencyProbeWindow`, 512 bytes) so one attempt can never cost more than a constant regardless of what follows, at the byte offset immediately after each already-found match, deduplicated against matches already found. A pattern with an open-ended quantifier can still greedily consume into a following token before this recovery ever runs — a separate, broader gap, captured as iss-188 rather than folded in here. Repro: `internal/adapter/scanner/adjacency_test.go`, `TestConcatenatedSecretsBothDetected` (the original bug), `TestConcatenatedDifferentFixedLengthSecretsBothDetected` and `TestGoogleAPIKeyDashJunctionNotDoubleCounted` (review round 1), `TestAdjacencyProbeStaysLinearOnLongLines` and `TestAdjacencyProbeWindowIsBounded` (review rounds 2 and 3). +- 2026-08-05 — iss-188 (bug-hunt loop, follow-on to iss-185): a secret pattern whose quantifier is open-ended (`\bghp_[A-Za-z0-9]{36,}` and the seven other `{n,}` families) greedily consumes a following token's own leading bytes when those fall inside its trailing character class, so the true junction sits BEFORE the match's reported end and iss-185's forward adjacency probe — which only ever looks at that end — never runs where the second token actually starts. `"ghp_"+36×a+"ghp_"+36×b` reported one finding covering `ghp_aaa…aaaghp` and left `_bbb…bbb` raw through `Redact`, with `history`'s fail-closed residual re-scan reporting the output clean: the same live-secret-reaches-disk violation iss-185 closed for the fixed-length family. Fixed in `internal/adapter/scanner/scanner.go:465` (`stolenJunctions`): before a match's reported end is accepted as final, a bounded backward search walks candidate cuts and reports each one where the shortened prefix is STILL a whole match for its own pattern AND a token can begin, and `scanAllPatterns` probes those cuts exactly as it probes the forward end. Open-ended is detected from the compiled regexp — a match that cannot still match one byte shorter has a rigid length and is skipped after one test — not from hand-annotated metadata, so a pattern added later inherits the classification. Three things keep the cost per match rather than per match LENGTH, which the ledger entry named as the trap (an unbounded backward scan would trade the leak for a resource-exhaustion cliff on exactly the huge single-line input a scanner must handle): that one-test early-out, `maxAdjacencyBacktrack` (512, mirroring `maxAdjacencyProbeWindow`), 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 line. The over-long match is deliberately left untrimmed: it only over-reports the first token's span, and `sealLine` already forces every overlap byte to `*`. Repro: `internal/adapter/scanner/adjacency_test.go`, `TestConcatenatedOpenEndedSecretsBothDetected` (the ledger entry's literal reproducer) and `TestOpenEndedSecretSwallowingDifferentFamilyBothDetected` (a whole `AKIA` key swallowed by an alnum class run), both watched failing on pre-fix code for the claimed reason; cost guard `TestJunctionBacktrackIsBounded`. +- 2026-08-05 — iss-188 follow-up (two independent adversarial reviews of the fix above, both landing on the same defect): the bounded backward search in `internal/adapter/scanner/scanner.go` (`stolenJunctions`) could step OVER the junction it was looking for. `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 in between, the true junction among them. Nothing revisits a skipped range, so the search returned fewer cuts than it should and iss-188's own failure mode reopened through a narrower trigger: `"ghp_"+32×a+"AIza"+4×b+"ghp_"+36×c` reported one finding and left the second token's tail raw, and `"ghp_"+10×a+"AIza"+22×z+"sk-proj-"+40×i` — where the skipped-over decoy is itself a syntactically valid secret — left 48 raw bytes of an OpenAI project key through `Redact` with the fail-closed residual re-scan reporting the output clean. Fixed by always resuming at `cut + 1`, dropping the jump-to-hit-end fast path entirely: a rejected candidate can hide a real junction one byte later, and an accepted one is not worth a special case. This stays bounded for the reason the whole feature exists: the loop is capped at `maxAdjacencyBacktrack` (512) iterations per match regardless of how long the match is, and each iteration's search is capped at `maxAdjacencyBacktrack + maxAdjacencyProbeWindow` bytes by RE2's linear guarantee — so the per-match cost is a constant factor independent of the REST of the line, and the whole scan stays linear in line length rather than quadratic. Measured: doubling and quadrupling an adversarial 247KB line doubled and quadrupled the time (2.3s / 4.6s / 9.2s), which is the property that matters. Repro: `internal/adapter/scanner/adjacency_test.go`, `TestStolenJunctionSearchDoesNotSkipPastRejectedCandidate` and `TestStolenJunctionSearchSkipsPastValidDecoySecret`, both watched failing on pre-fix code for the claimed reason; cost guard `TestJunctionBacktrackIsBounded`, extended with `dense_rejected_candidates_in_backtrack_window` — every match's window packed with candidates that all fail validation, the worst case for advancing one byte at a time. Two further gaps the same reviews found were CAPTURED, not folded in: iss-190 (a recovered match longer than `maxAdjacencyProbeWindow` is truncated, and the misaligned artificial end breaks the chain that would recover a THIRD abutting token — the pre-existing iss-185 window trade-off reached through a new path, whose real fix is an adaptive window that risks reintroducing the unbounded per-match cost) and iss-191 (`junctionProbe`'s `(?s).` compile fallback would validate once per byte instead of once per candidate, unreachable with the bundled pattern set). Two comment claims were also corrected as inaccurate: 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 window-exceeding recovery 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. diff --git a/.abcd/work/issues/open/iss-190-scanner-adjacency-recovery-is-capped-by-maxadjacencyprobewin.md b/.abcd/work/issues/open/iss-190-scanner-adjacency-recovery-is-capped-by-maxadjacencyprobewin.md new file mode 100644 index 00000000..3ce80aa9 --- /dev/null +++ b/.abcd/work/issues/open/iss-190-scanner-adjacency-recovery-is-capped-by-maxadjacencyprobewin.md @@ -0,0 +1,12 @@ +--- +schema_version: 1 +id: "iss-190" +slug: "scanner-adjacency-recovery-is-capped-by-maxadjacencyprobewin" +severity: "minor" +category: "bug" +source: "agent-finding" +found_during: "bug-hunt loop round 3, pre-PR review of iss-188's fix" +found_at: "internal/adapter/scanner/scanner.go" +--- + +scanner adjacency recovery is capped by maxAdjacencyProbeWindow, so a recovered token longer than the window is truncated — and the truncation can break the recovery CHAIN, hiding a third abutting token entirely. probeAt (scanAllPatterns, internal/adapter/scanner/scanner.go) runs its anchored probe against line[at:at+maxAdjacencyProbeWindow] (512 bytes), so a recovered open-ended match longer than that is recorded with an artificial end at the window edge. Two manifestations. (a) Known and accepted since iss-185: the recovered token's far tail past the window stays raw in Redact's output; the token's prefix is still masked, so this is a partial-redaction trade, not a total miss. (b) NEW, found by two independent adversarial reviews of iss-188's fix: because the artificial end does not align with the token's real end, the next iteration's probeAt(m.end) and stolenJunctions both start from a byte offset that is inside the token rather than at its junction, so the chain that would otherwise unwind a run of three or more abutting tokens breaks — the third token becomes entirely invisible, not merely truncated. Repro: line := "ghp_" + strings.Repeat("a",36) + "ghp_" + strings.Repeat("m",600) + "ghp_" + strings.Repeat("z",36) yields exactly 2 token:github_pat findings where an unbounded search would find 3; the third token's 36 raw bytes survive Redact and the fail-closed residual re-scan reports the output clean. A sharper variant of (a) affects jwt_shaped specifically: its regex requires two literal '.' separators, so a JWT-shaped token whose header and payload together exceed the window has NEITHER separator inside the probe's slice and the anchored probe finds no match at all — the token is missed entirely rather than truncated, e.g. a fixed-length match immediately followed by a JWT whose first two segments total more than ~502 bytes. This is NOT a defect introduced by iss-188's backward search; it is the same pre-existing maxAdjacencyProbeWindow trade-off from iss-185, now reachable through a second code path because the backward search feeds more candidates into probeAt. Fixing it properly needs a different mechanism (a growing or adaptive window, or re-probing chained from a recovered match's real end) that must not reintroduce the unbounded per-match cost the window exists to prevent — the exact risk iss-188's own ledger entry warned against. Deliberately not folded into iss-188's fix pass to keep that diff scoped to the confirmed junction-search defect. \ No newline at end of file diff --git a/.abcd/work/issues/open/iss-191-junctionprobe-s-compile-fallback-turns-stolenjunctions-from.md b/.abcd/work/issues/open/iss-191-junctionprobe-s-compile-fallback-turns-stolenjunctions-from.md new file mode 100644 index 00000000..a5f2dbfb --- /dev/null +++ b/.abcd/work/issues/open/iss-191-junctionprobe-s-compile-fallback-turns-stolenjunctions-from.md @@ -0,0 +1,12 @@ +--- +schema_version: 1 +id: "iss-191" +slug: "junctionprobe-s-compile-fallback-turns-stolenjunctions-from" +severity: "minor" +category: "bug" +source: "agent-finding" +found_during: "bug-hunt loop round 3, pre-PR review of iss-188's fix" +found_at: "internal/adapter/scanner/scanner.go" +--- + +junctionProbe's compile fallback turns stolenJunctions from a bounded candidate walk into a per-byte one, a latent cost cliff. junctionProbe (internal/adapter/scanner/scanner.go) builds one combined alternation over every pattern's boundary-free body; if that alternation fails to compile it falls back to regexp.MustCompile(`(?s).`), which matches at EVERY byte offset. The fallback was chosen so the candidate generator can over-produce but never under-produce — correct for detection, but it changes stolenJunctions' cost class: instead of one wholeMatch validation per real candidate junction in the backtrack window, wholeMatch runs once per byte offset in the window, and each of those validations is itself proportional to the match's own length (it re-runs the anchored probe over line[m.start:cut]). Cost per match goes from O(window) validations to O(window x match length) work. A reviewer measured roughly a 500x slowdown substituting the fallback directly — 4.21s against 8.25ms on one 200KB match. Not reachable with the current bundled pattern set: every bundled body alternates cleanly, and triggering the compile failure needs a pathological custom .abcd/config/pii.json override whose added pattern makes the joined alternation invalid (the per-pattern regexes are each compiled and validated on merge, so this is hard but not provably impossible to reach). Latent rather than urgent. Options if it is ever worth closing: cap the fallback's candidate count the way the window caps the search, drop patterns from the alternation one at a time until it compiles rather than abandoning the whole set, or mark the scanner unavailable (fail-closed) when the combined probe cannot be built — the last is consistent with how the package already treats a config fault. \ No newline at end of file diff --git a/.abcd/work/issues/open/iss-188-scanner-adjacency-greedy-steal-open-ended.md b/.abcd/work/issues/resolved/iss-188-scanner-adjacency-greedy-steal-open-ended.md similarity index 89% rename from .abcd/work/issues/open/iss-188-scanner-adjacency-greedy-steal-open-ended.md rename to .abcd/work/issues/resolved/iss-188-scanner-adjacency-greedy-steal-open-ended.md index f883f4f1..9fbcd28e 100644 --- a/.abcd/work/issues/open/iss-188-scanner-adjacency-greedy-steal-open-ended.md +++ b/.abcd/work/issues/resolved/iss-188-scanner-adjacency-greedy-steal-open-ended.md @@ -7,6 +7,8 @@ category: "bug" source: "agent-finding" found_during: "bug-hunt loop round 3, pre-PR security review of iss-185's fix" found_at: "internal/adapter/scanner/scanner.go:scanAllPatterns" +resolution: "scanAllPatterns now backtracks a bounded window from an open-ended match's reported end (stolenJunctions) and probes each candidate cut where the shortened prefix is still a whole match for its own pattern, so a second token whose leading bytes the greedy quantifier swallowed is detected and redacted instead of surviving raw" +impact: fix --- scanner adjacency recovery misses greedy over-consumption in open-ended-quantifier secret patterns. iss-185's fix (scanAllPatterns, scanner.go) recovers a second token that immediately abuts a first with no separator ONLY when the first pattern is fixed-length (its match always ends exactly where the second token begins, so probing every pattern's boundary-free variant at that one byte offset is sufficient). A pattern whose quantifier is open-ended (e.g. `\bghp_[A-Za-z0-9]{36,}`, `\bsk-ant-[A-Za-z0-9_-]{40,}`, `\bsk_live_[A-Za-z0-9]{20,}`) instead greedily consumes as many class-matching bytes as are available, including a following token's own leading bytes when those happen to fall inside the same character class — shifting the true junction earlier than the reported match end, before scanAllPatterns' adjacency probe ever runs. Concrete repro: `"ghp_" + strings.Repeat("a",36) + "ghp_" + strings.Repeat("b",36)` — the first `ghp_` pattern's `{36,}` class run consumes the 36 a's AND the second token's leading "ghp" (all alnum), stopping only at the second token's `_` (not in `[A-Za-z0-9]`); ScanText reports exactly ONE finding covering `ghp_aaaa...aaaghp`, and the second token's `_bbbb...bbb` tail survives Redact completely raw with the fail-closed residual re-scan reporting clean — the same "can never contain a live secret" violation iss-185 was filed for, just for the open-ended-quantifier pattern family instead of the fixed-length one. Distinct from iss-185 (closed): that fix's mechanism (probe every pattern at a match's reported end) is necessary but not sufficient — this needs a bounded backward search (shrink the greedy match's end toward its start, checking at each candidate cut whether the shortened match is still valid for its own pattern AND some pattern's boundary-free variant matches there) without becoming O(line length) per match, since these patterns' matches can legitimately be very long (base64 blobs, minified files) and an unbounded backward scan across every byte would be a new resource-exhaustion risk on exactly the kind of large single-line input a secret scanner must handle. Surfaced by an adversarial pre-PR security review of iss-185's fix (bug-hunt loop round 3), not by the round's own hunt; not fixed in that PR to keep its diff scoped to the exact bug iss-185 named and to avoid shipping an unbounded-cost search under review pressure. Affected pattern families: github_pat (ghp_/ghs_/gho_/ghu_/ghr_), anthropic, openai_project, openai_svcacct, stripe_live, stripe_test, slack, jwt_shaped — every bundled pattern with an open (`{n,}`) rather than fixed (`{n}`) quantifier. \ No newline at end of file diff --git a/CHANGELOG.md b/CHANGELOG.md index 69af639a..00ea4d6e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -857,6 +857,30 @@ called out in a **Breaking** section. quantifier is open-ended rather than fixed-length can still greedily consume into a following token before this recovery ever runs; that is a separate, broader gap, tracked as iss-188. +- **The secret scanner now also detects an abutting token whose leading bytes + the preceding pattern's greedy quantifier had already swallowed** (iss-188). + A pattern with an open-ended length bound consumes as many class-matching + bytes as it can find, including the next token's own prefix when those bytes + fall in the same character class — so the boundary between the two tokens + sits before the reported end of the match, where the adjacency probe above + never looks. Two concatenated GitHub PATs were reported as one over-long + finding and the second token's tail survived redaction raw, with the + fail-closed residual re-scan reporting the output clean, so a live + credential still reached disk. Before a match's end is accepted as final, + the scan now walks back over a bounded window looking for a cut where the + shortened match is still valid for its own pattern and another token can + begin, and probes there as well. Whether a pattern needs that search is + decided from the pattern itself — one whose match cannot survive losing a + byte has a rigid length and is skipped — so a pattern added later is covered + without being annotated. The backward window, like the forward one, is a + small fixed size and candidates come from one combined probe over the whole + pattern set, so the work done per match never scales with what follows the + match — a legitimately huge match (a base64 blob, a minified line) keeps the + whole scan linear in the length of the line rather than quadratic. The + recovery is bounded, not exhaustive: a recovered token longer than the probe + window is still truncated to it (or, for a pattern whose required separators + fall outside the window, missed), so an abutting token behind one of those is + not yet covered — tracked as iss-190. ## [0.4.1] - 2026-07-28 diff --git a/internal/adapter/scanner/adjacency_test.go b/internal/adapter/scanner/adjacency_test.go index 122e7272..6e28fcf0 100644 --- a/internal/adapter/scanner/adjacency_test.go +++ b/internal/adapter/scanner/adjacency_test.go @@ -1,6 +1,7 @@ package scanner import ( + "runtime/debug" "strings" "testing" "time" @@ -74,6 +75,153 @@ func TestConcatenatedDifferentFixedLengthSecretsBothDetected(t *testing.T) { } } +// TestConcatenatedOpenEndedSecretsBothDetected is the repro for iss-188, the +// gap iss-185's fix left open. github_pat's quantifier is open-ended +// (`[A-Za-z0-9]{36,}`), so on two abutting ghp_ tokens the first match greedily +// swallows the second token's own leading "ghp" — every byte of it is in the +// class — and stops only at the second token's '_'. The reported match end is +// therefore PAST the true junction, so probing at that end finds nothing and +// the second token's `_bbb…` tail used to survive Redact completely raw while +// the fail-closed residual re-scan reported the output clean. +func TestConcatenatedOpenEndedSecretsBothDetected(t *testing.T) { + r := strings.Repeat + token1 := "ghp_" + r("a", 36) + token2 := "ghp_" + r("b", 36) + line := token1 + token2 + + findings := scanLine(line) + if !hasKind(findings, "token:github_pat") { + t.Fatalf("github_pat not detected at all: %+v", findings) + } + count := 0 + for _, f := range findings { + if f.Kind == "token:github_pat" { + count++ + } + } + if count != 2 { + t.Fatalf("expected both concatenated open-ended tokens detected, got %d findings: %+v", count, findings) + } + + redacted, _ := Redact(line, findings) + if strings.Contains(redacted, r("b", 36)) { + t.Errorf("second concatenated token's tail survived redaction raw: %q", redacted) + } + rescan := scanLine(redacted) + for _, f := range rescan { + if f.Severity == SeverityHardFail { + t.Errorf("hard_fail survived redaction of concatenated open-ended secrets: %+v (out=%q)", f, redacted) + } + } +} + +// TestOpenEndedSecretSwallowingDifferentFamilyBothDetected is the mixed-family +// half of iss-188: an open-ended pattern's greedy class run can swallow a +// following token of a DIFFERENT family whole, when every byte of that token is +// in the first pattern's class (`AKIA`+16 upper-case is pure alnum). The +// junction is then 20 bytes before the reported end rather than at it. +func TestOpenEndedSecretSwallowingDifferentFamilyBothDetected(t *testing.T) { + r := strings.Repeat + ghpToken := "ghp_" + r("a", 36) + awsToken := "AKIA" + r("Q", 16) + line := ghpToken + awsToken + + findings := scanLine(line) + if !hasKind(findings, "token:github_pat") { + t.Errorf("github_pat not detected in mixed concatenation: %+v", findings) + } + if !hasKind(findings, "token:aws_access_key") { + t.Errorf("aws_access_key swallowed by the open-ended match, not detected: %+v", findings) + } + + redacted, _ := Redact(line, findings) + if strings.Contains(redacted, awsToken) { + t.Errorf("AWS key survived redaction raw after an open-ended swallow: %q", redacted) + } + rescan := scanLine(redacted) + for _, f := range rescan { + if f.Severity == SeverityHardFail { + t.Errorf("hard_fail survived redaction of an open-ended swallow: %+v (out=%q)", f, redacted) + } + } +} + +// TestStolenJunctionSearchDoesNotSkipPastRejectedCandidate is the repro for the +// gap iss-188's first fix left open, found independently by two adversarial +// reviews. junctionProbe is UNANCHORED, so one of its hits can SPAN the true +// junction — begin before it and end after it — without beginning AT it. The +// backward search used to resume at such a hit's END even when the hit's own +// offset had just been REJECTED by wholeMatch, stepping over every byte between +// the two, the real junction among them. Nothing revisits that range, so the +// second token was never recovered. +// +// Here the first `ghp_` match greedily runs to the second token's '_' at byte +// 47. The leftmost junction-probe hit inside it is google_api's body at byte 36 +// (`AIza` + 35 more class bytes), which spans to byte 75 and is rejected — the +// prefix `ghp_…AIza` is not a whole github_pat match. The true junction is at +// byte 44, inside that span. +func TestStolenJunctionSearchDoesNotSkipPastRejectedCandidate(t *testing.T) { + r := strings.Repeat + token2 := "ghp_" + r("c", 36) + line := "ghp_" + r("a", 32) + "AIza" + r("b", 4) + token2 + + findings := scanLine(line) + count := 0 + for _, f := range findings { + if f.Kind == "token:github_pat" { + count++ + } + } + if count != 2 { + t.Fatalf("expected both open-ended tokens detected across a rejected junction candidate, got %d findings: %+v", count, findings) + } + + redacted, _ := Redact(line, findings) + if strings.Contains(redacted, r("c", 36)) { + t.Errorf("second token's tail survived redaction raw: %q", redacted) + } + rescan := scanLine(redacted) + for _, f := range rescan { + if f.Severity == SeverityHardFail { + t.Errorf("hard_fail survived redaction past a rejected junction candidate: %+v (out=%q)", f, redacted) + } + } +} + +// TestStolenJunctionSearchSkipsPastValidDecoySecret is the same defect reached +// without any filler: the rejected junction-probe hit that used to be skipped +// past is ITSELF a syntactically valid secret of a third family. The first +// `ghp_` match swallows a Google API key AND the head of an OpenAI project key; +// the leftmost candidate inside it is the Google key's own start at byte 14, +// whose span reaches byte 53 and is rejected (the prefix is shorter than +// github_pat's `{36,}` minimum). The real junction — the `sk-proj-` key at byte +// 40 — sits inside that span, so the whole 48-byte key used to survive Redact +// raw and reappear as a hard_fail on the fail-closed residual re-scan. +func TestStolenJunctionSearchSkipsPastValidDecoySecret(t *testing.T) { + r := strings.Repeat + openaiToken := "sk-proj-" + r("i", 40) + line := "ghp_" + r("a", 10) + "AIza" + r("z", 22) + openaiToken + + findings := scanLine(line) + if !hasKind(findings, "token:github_pat") { + t.Errorf("github_pat not detected: %+v", findings) + } + if !hasKind(findings, "token:openai_project") { + t.Errorf("openai project key behind a rejected decoy candidate not detected: %+v", findings) + } + + redacted, _ := Redact(line, findings) + if strings.Contains(redacted, r("i", 40)) { + t.Errorf("openai project key survived redaction raw behind a decoy candidate: %q", redacted) + } + rescan := scanLine(redacted) + for _, f := range rescan { + if f.Severity == SeverityHardFail { + t.Errorf("hard_fail survived redaction behind a decoy candidate: %+v (out=%q)", f, redacted) + } + } +} + // TestGoogleAPIKeyDashJunctionNotDoubleCounted is the regression guard for a // bug the adjacency fix itself introduced: google_api_key is fixed-length // but its charset includes '-', a NON-word character. When the 35th body @@ -147,3 +295,100 @@ func TestAdjacencyProbeWindowIsBounded(t *testing.T) { }) } } + +// TestJunctionBacktrackIsBounded is the cost guard on iss-188's backward +// search. Recovering a junction a greedy quantifier ran past means looking +// BEHIND a match's reported end, and an open-ended pattern's match is +// legitimately allowed to be the whole line — a base64 blob, a minified asset. +// A per-byte backward walk over an arbitrarily long match, or a prefix +// re-validation per byte of it, would make exactly that input a +// resource-exhaustion cliff, trading one security bug for another. The search +// is instead capped at maxAdjacencyBacktrack behind the end regardless of how +// long the match is, and each candidate cut comes from one linear pass rather +// than a per-byte probe. Every case here is a single line of tens to hundreds +// of kilobytes, the shape that used to time out. +func TestJunctionBacktrackIsBounded(t *testing.T) { + r := strings.Repeat + n := scaleAdversarial + cases := []struct { + name string + line string + }{ + // One open-ended match spanning the whole line: the backward window + // must not scale with it. + {"one_very_long_open_ended_match", "ghp_" + r("a", n(200000))}, + {"one_very_long_jwt", "eyJ" + r("a", 20) + "." + r("b", 20) + "." + r("c", n(100000))}, + // Many open-ended matches, each of which backtracks. + {"open_ended_tokens_back_to_back", r("ghp_"+r("a", 36), n(1000))}, + {"stripe_tokens_back_to_back", r("sk_live_"+r("a", 20), n(1000))}, + // One huge match densely seeded with candidate junctions, so the + // backward search finds work at nearly every offset it looks at. This + // is the case an unbounded backtrack blows up on: it measured 40s + // against 0.3s bounded. + {"dense_candidate_junctions", "ghp_" + r("AKIA", n(50000))}, + {"open_ended_chain_inside_open_ended", "xoxb-" + r("ghp_"+r("a", 36), n(1500))}, + // Dense short matches whose pattern is itself shrinkable. + {"dense_dotted_quads", r("1.2.3.4", n(10000))}, + // The worst case for resuming one byte past a REJECTED candidate rather + // than past its whole span (see stolenJunctions): every match's backtrack + // window is packed with junction-probe hits that ALL fail validation, so + // the loop runs its maximum number of iterations and each one re-validates + // a long prefix. Each token here is a JWT whose backtrack window falls + // inside its middle segment, where no prefix can be a whole jwt_shaped + // match (only one of the two required '.' separators is present), and the + // segment is filled with `AKI` — the densest junction-probe hit spacing + // the bundled set admits inside an alnum run, ~one candidate every three + // bytes. Cost per match stays capped by the window, so the whole scan + // stays linear in line length: doubling the line doubles the time, it does + // not square it (measured 2.3s / 4.6s / 9.2s at 247KB / 494KB / 989KB). + // The 15s bar is deliberately loose — it separates the unbounded-search + // cliff (tens of seconds on inputs this size) from bounded work (~1s + // here) without tracking machine speed. + {"dense_rejected_candidates_in_backtrack_window", r("eyJ"+r("a", 10)+"."+r("AKI", 400)+"."+r("c", 20)+" ", n(100))}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + start := time.Now() + scanLine(c.line) + if elapsed := time.Since(start); elapsed > 15*time.Second { + t.Errorf("scan of a %d-byte line took %v, want well under 15s (unbounded-backtrack regression)", len(c.line), elapsed) + } + }) + } +} + +// raceDetector reports whether this test binary was built with -race. It is read +// from the build settings rather than a build tag so the whole guard stays in +// one file. +var raceDetector = func() bool { + bi, ok := debug.ReadBuildInfo() + if !ok { + return false + } + for _, s := range bi.Settings { + if s.Key == "-race" { + return s.Value == "true" + } + } + return false +}() + +// scaleAdversarial shrinks the adversarial cost-guard inputs when the race +// detector is on. The detector instruments every memory access the regexp +// engine makes and costs this package well over an order of magnitude in wall +// clock, which a fixed second-count budget cannot absorb. Shrinking the input +// is the right lever rather than loosening the budget: what each case tests is +// its SHAPE — one huge match, or many, densely seeded with candidate junctions +// — and every case's cost is linear in the input, so the shape survives a +// smaller multiplier. A budget stretched far enough to cover an instrumented +// 200KB line would stop discriminating a bounded search from an unbounded one +// on the uninstrumented run, which is where the guard has to bite. +func scaleAdversarial(n int) int { + if !raceDetector { + return n + } + if n < 8 { + return 1 + } + return n / 8 +} diff --git a/internal/adapter/scanner/scanner.go b/internal/adapter/scanner/scanner.go index 6b37110d..b399c2ee 100644 --- a/internal/adapter/scanner/scanner.go +++ b/internal/adapter/scanner/scanner.go @@ -269,19 +269,18 @@ var leadingFlagGroup = regexp.MustCompile(`^\(\?[a-zA-Z]+\)`) // immediately abuts a first with no separating byte, the byte just before it // is itself a word character (the first token's last byte), so \b can never // hold there and the second token is silently never matched. scanAllPatterns -// runs this probe ONLY at the exact byte offset right after a prior match — -// a hit there is anchored by the adjacency itself, not by \b, so dropping the -// anchor cannot introduce a false match anywhere else in the line. The `\A` -// anchor here is what makes that probe O(1) rather than O(remaining line -// length): without it, FindStringIndex would search the entire remainder of -// the line for a match anywhere, then discard everything not starting at -// offset 0 — turning one probe per candidate junction into a full rescan of -// the rest of the line, which a single long line (a minified asset, a base64 -// blob) can turn into a multi-second hang per call. +// runs this probe ONLY at a byte offset where a token is already known to +// begin — right after a prior match, or at a junction a greedy match ran past +// (stolenJunctions) — a hit there is anchored by the adjacency itself, not by +// \b, so dropping the anchor cannot introduce a false match anywhere else in +// the line. The `\A` anchor here is what makes that probe O(1) rather than +// O(remaining line length): without it, FindStringIndex would search the +// entire remainder of the line for a match anywhere, then discard everything +// not starting at offset 0 — turning one probe per candidate junction into a +// full rescan of the rest of the line, which a single long line (a minified +// asset, a base64 blob) can turn into a multi-second hang per call. func adjacencyProbe(re *regexp.Regexp) *regexp.Regexp { - src := re.String() - flags := leadingFlagGroup.FindString(src) - body := strings.TrimPrefix(src[len(flags):], `\b`) + flags, body := probeParts(re) anchored, err := regexp.Compile(flags + `\A(?:` + body + `)`) if err != nil { return re @@ -289,6 +288,50 @@ func adjacencyProbe(re *regexp.Regexp) *regexp.Regexp { return anchored } +// junctionProbe is adjacencyProbe's UNANCHORED counterpart, and ONE regexp for +// the whole pattern set rather than one per pattern: every pattern's +// boundary-free body, alternated, free to match anywhere in the string it is +// run against. It is a candidate GENERATOR for stolenJunctions and nothing +// else — it answers "could ANY token begin at this offset", which is exactly +// what a backward search needs to narrow 512 offsets down to a handful, and +// answers it in one linear pass instead of one pass per pattern (the per-match +// cost of the backward search is otherwise multiplied by the size of the +// pattern set, which a scanner cannot pay on a long line). Nothing is ever +// added off a junctionProbe hit: every offset it yields is re-tested with the +// ANCHORED probe before a match is recorded, so the "an unanchored +// boundary-free match cannot be trusted on its own" reasoning above still +// holds, and a generator that over-produces only costs time. +// +// Each pattern's own inline flag group is rewritten into a SCOPED one +// (`(?i)x` → `(?i:x)`) so a case-insensitive pattern cannot fold the case of +// every alternative after it. If the alternation somehow will not compile, the +// fallback is a regexp that matches at every offset: slower, never blinder — +// the generator may over-produce, never under-produce. +func junctionProbe(patterns []Pattern) *regexp.Regexp { + alts := make([]string, 0, len(patterns)) + for _, cp := range patterns { + flags, body := probeParts(cp.Re) + if flags != "" { + alts = append(alts, flags[:len(flags)-1]+`:`+body+`)`) + continue + } + alts = append(alts, `(?:`+body+`)`) + } + combined, err := regexp.Compile(strings.Join(alts, `|`)) + if err != nil { + return regexp.MustCompile(`(?s).`) + } + return combined +} + +// probeParts splits a compiled pattern's source into its leading inline flag +// group (if any) and the rest with a leading \b stripped. +func probeParts(re *regexp.Regexp) (flags, body string) { + src := re.String() + flags = leadingFlagGroup.FindString(src) + return flags, strings.TrimPrefix(src[len(flags):], `\b`) +} + // patMatch is one pattern match location in a line, plus which pattern (by // index into the patterns/probes slices passed to scanAllPatterns) it // belongs to. @@ -313,30 +356,54 @@ type patMatch struct { // real match (the longest, github_pat_finegrained, is 93 bytes) and than a // realistic DNS hostname (~253 bytes) — the class of match this function // exists to recover. An open-ended-quantifier pattern (jwt_shaped, ghp_, -// etc.) recovered here can still be longer than the window and get -// truncated; that is the same already-tracked iss-188 gap, not a new one — -// truncated-but-partially-redacted is strictly better than this function's -// pre-existing behavior of not recovering it at all. +// etc.) recovered here can still exceed the window, in which case its span is +// truncated to it and the recovered token's far tail stays raw — and for a +// pattern whose earliest REQUIRED structural marker can itself fall past the +// window (jwt_shaped needs two literal '.' separators, both of which a long +// header pushes out of reach), the anchored probe finds no match at all, so +// such a token can be missed ENTIRELY rather than merely truncated. That is +// the standing cost of bounding the probe (iss-185), not a defect of the +// junction search that feeds it; the alternative — letting one probe attempt +// run to the end of the line — is the hang this constant exists to prevent. const maxAdjacencyProbeWindow = 512 +// maxAdjacencyBacktrack bounds how far BACK from an open-ended match's +// reported end stolenJunctions looks for the junction that match over-ran. It +// mirrors maxAdjacencyProbeWindow for the same reason and on the same terms: +// what has to stay bounded is not the length of a match (a base64 blob or a +// minified line is legitimately hundreds of kilobytes) but the work done per +// match, and a fixed window is the only thing that caps it independently of +// how long the match is. The bound is not arbitrary: over-consumption can only +// reach as far as the following token's own leading bytes that fall inside the +// first pattern's trailing character class, so the junction is at most one +// bundled token's length behind the reported end — 512 bytes clears every +// bundled pattern's longest real token by a wide margin. A junction further +// back than that would need a crafted multi-kilobyte token, which no bundled +// pattern can produce. +const maxAdjacencyBacktrack = 512 + // scanAllPatterns returns every match of every pattern in line, plus any -// further FIXED-LENGTH token — of the SAME pattern or a DIFFERENT one — that -// immediately abuts an already-found match with no separating byte (see -// adjacencyProbe). This closes the exact gap iss-185 named: a fixed-length -// pattern's own match always ends exactly where a genuinely adjacent token -// begins, so probing every pattern at that one byte offset recovers it. It -// does NOT help a real secret preceded by content that is not itself a +// further token — of the SAME pattern or a DIFFERENT one — that immediately +// abuts an already-found match with no separating byte (see adjacencyProbe). +// It probes two kinds of junction. A FIXED-LENGTH pattern's match always ends +// exactly where a genuinely adjacent token begins, so probing every pattern at +// that one byte offset recovers it (iss-185). A pattern whose quantifier is +// open-ended (`{36,}`) instead greedily consumes the following token's own +// leading bytes whenever they fall inside its trailing character class, which +// puts the true junction BEFORE the reported end and out of the forward +// probe's reach — that one needs stolenJunctions' bounded backward search +// (iss-188). The over-long match is left as it is rather than trimmed back to +// the junction: it only ever over-reports the first token's span, which +// over-redacts by a few bytes, while the recovered second token is what closes +// the leak. +// +// Neither probe helps a real secret preceded by content that is not itself a // match found here — e.g. unrecognized filler text, or an identity-derived -// match from matchers.findings — since there is no existing patMatch for -// the probe to run after; that is the same pre-existing \b-boundary -// limitation every bundled pattern already accepts elsewhere in this -// package, not something this function claims to close. A pattern whose -// quantifier is open-ended (e.g. `{36,}`) can also greedily over-consume -// into a following token BEFORE this check ever runs, shifting the true -// junction earlier than the match's reported end; recovering that needs a -// backward search this function does not attempt, and is tracked separately -// (iss-188) rather than folded in here. -func scanAllPatterns(patterns []Pattern, probes []*regexp.Regexp, line string) []patMatch { +// match from matchers.findings — since there is no existing patMatch for the +// probe to run after; that is the same pre-existing \b-boundary limitation +// every bundled pattern already accepts elsewhere in this package, not +// something this function claims to close. +func scanAllPatterns(patterns []Pattern, probes []*regexp.Regexp, junctions *regexp.Regexp, line string) []patMatch { var all []patMatch seen := map[patMatch]bool{} add := func(m patMatch) { @@ -346,29 +413,113 @@ func scanAllPatterns(patterns []Pattern, probes []*regexp.Regexp, line string) [ seen[m] = true all = append(all, m) } - for i, cp := range patterns { - for _, loc := range cp.Re.FindAllStringIndex(line, -1) { - add(patMatch{i, loc[0], loc[1]}) - } - } - for qi := 0; qi < len(all); qi++ { - end := all[qi].end - limit := end + maxAdjacencyProbeWindow + // probeAt records every pattern whose boundary-free variant matches + // starting exactly at byte offset at. + probeAt := func(at int) { + limit := at + maxAdjacencyProbeWindow if limit > len(line) { limit = len(line) } - window := line[end:limit] + window := line[at:limit] for j := range patterns { m := probes[j].FindStringIndex(window) if m == nil || m[0] != 0 || m[1] == 0 { continue } - add(patMatch{j, end, end + m[1]}) + add(patMatch{j, at, at + m[1]}) + } + } + for i, cp := range patterns { + for _, loc := range cp.Re.FindAllStringIndex(line, -1) { + add(patMatch{i, loc[0], loc[1]}) + } + } + // The loop reads a growing slice on purpose: a token recovered at one + // junction is itself greedy and can have over-run the next one, so a run of + // three or more abutting tokens unwinds one junction per iteration. + for qi := 0; qi < len(all); qi++ { + m := all[qi] + probeAt(m.end) + for _, cut := range stolenJunctions(probes[m.patIdx], junctions, line, m) { + probeAt(cut) } } return all } +// stolenJunctions returns the byte offsets INSIDE m at which a second token +// really begins — the junctions m's own greedy quantifier ran past. A cut is +// reported only when both halves hold: line[m.start:cut] is still a whole match +// for m's own pattern (so the minimum-length bound of its quantifier is still +// satisfied and the cut is a real token end, not an arbitrary byte), and some +// pattern's junction probe can start a token there. +// +// The work done per match is bounded by a constant factor independent of the +// REST OF THE LINE — proportional to the match's OWN length, since each +// candidate is validated by re-running the probe over line[m.start:cut], but +// never to what follows the match. That is what keeps a legitimately huge match +// (a base64 blob, a minified line) off a performance cliff: the whole scan +// stays linear in line length rather than quadratic. Three things do that. A +// pattern that cannot match one byte shorter has a rigid length, so its +// reported end already IS its junction and the whole search is skipped after +// one test — that is how a fixed-length pattern is told from an open-ended one +// without hand-annotating either, and a pattern added later inherits the +// classification for free. The search window is capped at maxAdjacencyBacktrack +// behind the end and maxAdjacencyProbeWindow past it, so the number of +// candidates is capped too. And candidates come from the single COMBINED +// junction probe over that window, not from re-probing every byte in it with +// every pattern — the difference between one window-bounded search per +// candidate and a full window scan per pattern per match. +func stolenJunctions(probe, junctions *regexp.Regexp, line string, m patMatch) []int { + if m.end-m.start < 2 || !wholeMatch(probe, line[m.start:m.end-1]) { + return nil + } + lo := m.end - maxAdjacencyBacktrack + if lo < m.start+1 { + lo = m.start + 1 + } + hi := m.end + maxAdjacencyProbeWindow + if hi > len(line) { + hi = len(line) + } + var cuts []int + // Walk the window one hit at a time, resuming one byte past each hit's + // START, and stop at the first hit that is not inside m: a hit at or past + // m.end is already the forward probe's business, so a match whose over-run + // can only be a few bytes never pays for a scan of the whole window. + // Resuming past a hit's END would be wrong: junctions is UNANCHORED, so a + // hit can SPAN the true junction — start before it and end after it — + // without starting at it. Skipping to that hit's end then steps over the + // real junction, which nothing else revisits, and the second token's tail + // survives redaction raw again (the very failure iss-188 closed). One byte + // per rejected candidate costs at most maxAdjacencyBacktrack passes of a + // window-bounded search per match — a constant, still independent of the + // line's length. + for off := lo; off < m.end; { + loc := junctions.FindStringIndex(line[off:hi]) + if loc == nil { + break + } + cut := off + loc[0] + if cut >= m.end { + break + } + if wholeMatch(probe, line[m.start:cut]) { + cuts = append(cuts, cut) + } + off = cut + 1 + } + return cuts +} + +// wholeMatch reports whether s is entirely one match of probe, an anchored +// boundary-free adjacencyProbe. The anchor makes the match start at 0, so only +// its end has to reach the end of s. +func wholeMatch(probe *regexp.Regexp, s string) bool { + loc := probe.FindStringIndex(s) + return loc != nil && loc[1] == len(s) +} + // ScanText scans text for every secret pattern and identity-derived match, // returning findings sorted deterministically. It is pure: identity, patterns // and severities are all passed in. @@ -381,13 +532,14 @@ func ScanText(text string, id Identity, patterns []Pattern, id2sev map[string]Se for i, cp := range patterns { probes[i] = adjacencyProbe(cp.Re) } + junctions := junctionProbe(patterns) var findings []Finding lineno := 0 for _, line := range strings.Split(text, "\n") { line = strings.TrimRight(line, "\r") lineno++ findings = append(findings, matchers.findings(line, lineno, id2sev, file)...) - for _, m := range scanAllPatterns(patterns, probes, line) { + for _, m := range scanAllPatterns(patterns, probes, junctions, line) { cp := patterns[m.patIdx] matched := line[m.start:m.end] if cp.Skip != nil && cp.Skip(matched) {