Follow-up from review of #109.
The two new patterns in scripts/secret-scan.mjs (storage-state-cookies, storage-state-origins) detect a Playwright storageState() dump by co-occurrence: a shape indicator ("cookies":[ + httpOnly/sameSite, or "origins":[ + "localStorage":[) and SUBSTANTIAL_VALUE (a "value": "<16+ chars>" field), each checked with an independent .test(body) call against the entire file, not scoped to the same object/array.
That means the three conditions don't need to be near each other, or even related, to combine into a hit. Reproduced against scanText() directly with three unrelated JSON fragments concatenated in one string — a "cookies" feature-flag array (nothing to do with sessions), an unrelated field elsewhere named httpOnly, and an unrelated "value" field elsewhere with a 16+ char string (e.g. a hash, UUID, or cache key) — and it fires storage-state-cookies even though no actual storageState-shaped object exists anywhere in the file.
Why not blocking: the repo-wide test this PR adds (tests/unit/secret-scan.test.ts, "the whole repo stays clean") confirms nothing currently committed trips this, and the failure mode is safe-direction — a false positive blocks CI on an unrelated PR rather than letting a real secret through. It's also the only pattern in the file built this way; every other entry is a single contiguous regex, which is inherently scoped.
Suggested fix: bound the distance between the shape indicator and SUBSTANTIAL_VALUE instead of searching the whole body independently — e.g. extract a bounded window around each "cookies":[/"origins":[ match (a few hundred chars, generous enough for a real cookie/localStorage entry) and only test SUBSTANTIAL_VALUE plus the companion key within that window. A bounded quantifier avoids both the false positive and the catastrophic-backtracking concern the PR's docstring already raises about a lazy [\s\S]{0,N}? bridge — the difference is bounding the search window, not writing one combined regex.
Not currently exercised by any test — tests/unit/secret-scan.test.ts's negative cases are all real repo documents, not synthetic unrelated-content collisions. Worth adding a case like the repro above once the fix lands, asserting three unrelated matches spread across a file do not combine into a hit.
Follow-up from review of #109.
The two new patterns in
scripts/secret-scan.mjs(storage-state-cookies,storage-state-origins) detect a PlaywrightstorageState()dump by co-occurrence: a shape indicator ("cookies":[+httpOnly/sameSite, or"origins":[+"localStorage":[) andSUBSTANTIAL_VALUE(a"value": "<16+ chars>"field), each checked with an independent.test(body)call against the entire file, not scoped to the same object/array.That means the three conditions don't need to be near each other, or even related, to combine into a hit. Reproduced against
scanText()directly with three unrelated JSON fragments concatenated in one string — a"cookies"feature-flag array (nothing to do with sessions), an unrelated field elsewhere namedhttpOnly, and an unrelated"value"field elsewhere with a 16+ char string (e.g. a hash, UUID, or cache key) — and it firesstorage-state-cookieseven though no actual storageState-shaped object exists anywhere in the file.Why not blocking: the repo-wide test this PR adds (
tests/unit/secret-scan.test.ts, "the whole repo stays clean") confirms nothing currently committed trips this, and the failure mode is safe-direction — a false positive blocks CI on an unrelated PR rather than letting a real secret through. It's also the only pattern in the file built this way; every other entry is a single contiguous regex, which is inherently scoped.Suggested fix: bound the distance between the shape indicator and
SUBSTANTIAL_VALUEinstead of searching the whole body independently — e.g. extract a bounded window around each"cookies":[/"origins":[match (a few hundred chars, generous enough for a real cookie/localStorage entry) and only testSUBSTANTIAL_VALUEplus the companion key within that window. A bounded quantifier avoids both the false positive and the catastrophic-backtracking concern the PR's docstring already raises about a lazy[\s\S]{0,N}?bridge — the difference is bounding the search window, not writing one combined regex.Not currently exercised by any test —
tests/unit/secret-scan.test.ts's negative cases are all real repo documents, not synthetic unrelated-content collisions. Worth adding a case like the repro above once the fix lands, asserting three unrelated matches spread across a file do not combine into a hit.