fix(security): secret-scan now catches a real storageState() dump

[OM152002]

Closes #100. Follow-up to #37 (SC-03). Branched from main; no overlap with #107 or #108.

CONTRIBUTING.md rule 1 states:

No credentials, cookies, session/storage dumps… Secret-scanning CI fails the build on matches.

That was false for the shape that matters most. A synthetic fixture in Playwright's exact context.storageState() layout matched zero of the eight patterns:

patterns matching a real storageState() shape: 0/8  []

Its keys are name/value/domain/httpOnly/sameSite — so there is no response header for cookie-header to find, and no key spelled sessionid for session-json.

A documented guard that does not exist is worse than a known gap. It is why nobody looks twice at a session dump in a diff, and this check is merge-blocking, so it is the one people trust implicitly.

The discriminator is the value, not the shape

Two new patterns match a "cookies" / "origins" array co-occurring with a cookie-specific key (httpOnly / sameSite, or a nested localStorage array) and a value of 16+ characters.

That last clause is the whole design. docs/privacy/session-custody.md already quotes a real storageState layout verbatim — to document this very gap — and gate/recorder.md discusses the same fields in prose. A shape-only pattern would have broken CI on an existing doc the moment it merged.

Both elide their values ("value":"..."). A genuine dump cannot elide them, because the value is the secret. So the patterns catch session material, not session discussion. 16 characters clears every placeholder in the tree (..., REDACTED, <omitted>) while sitting far below a real session cookie — Grafana's is 32 hex chars, a JWT is hundreds.

Expressed as predicates rather than one regex: the co-occurrence would otherwise need a lazy [\s\S]{0,N}? bridge between two shapes, which is unreadable and a backtracking risk on a large file. Two linear scans and an && are neither.

Both directions, both guard-proven

Deleting the two new patterns:

× catches a synthetic Playwright storageState dump
× catches the origins/localStorage half on its own
× exits non-zero when the CLI is pointed at the fixture
× an elided example stays clean; the same shape with a real value does not

Dropping the value-length requirement (i.e. matching on shape alone):

× does not flag docs/privacy/session-custody.md
× does not flag session-custody.md, which quotes the shape verbatim
× an elided example stays clean; the same shape with a real value does not
× common redaction placeholders stay under the value threshold
× no tracked text file matches, so this suite cannot pass by breaking CI

That last test is the one that matters most for everyone else: it walks the whole tree and asserts nothing already committed matches. Without it, a pattern that breaks CI for every unrelated PR would still let all the positive tests pass.

Two self-referential traps, both hit

Worth recording because they are the character of this particular file:

1. The scanner's own comment tripped cookie-header — I spelled the header literal while explaining why it does not fire on storageState. The existing code already concatenates that string for exactly this reason.
2. The test file tripped the new pattern — it needs a 16+ character value to prove the pattern fires, which made the file itself a hit. Caught by my own repo-wide guard test. Both now assemble their values at runtime.

The fixture does not break the build

tests/fixtures/storage-state.sample is committed with fake values only. Its extension sits outside the set the repo-wide walk reads, so it does not permanently fail CI — and no allowlist was added, because an allowlist is a hole someone can later name a real dump into.

The CLI now accepts explicit paths, scanned regardless of extension, which is how the test proves the patterns fire end to end rather than only through the predicate:

$ npm run secret-scan                                        # clean
$ node scripts/secret-scan.mjs tests/fixtures/storage-state.sample
SECRET SCAN FAILED:
  - tests/fixtures/storage-state.sample (storage-state-cookies)
exit=1

main() is now guarded so importing the module for tests does not kick off a repo walk.

Scope

docs/privacy/session-custody.md's SC-03 gap note is updated to record the hole as closed, with the reasoning for the value-length threshold, since that is the part a future reader will be tempted to "tighten" into a false-positive machine.

This does not claim SC-03 is now enforced. No centralized logging module exists and console output is still scattered console.log/console.error; what changed is that the one automated backstop now covers the one shape it demonstrably missed.

npm run ci            # green — 190 unit, 14 integration, secret-scan clean, lint-docs clean (47 docs)
npm run test:canary   # 13 pass

🤖 Generated with Claude Code

[myselfsiddharth]

Review

Fixes a real gap: scripts/secret-scan.mjs's eight prior patterns matched zero of the fields in Playwright's actual context.storageState() shape (name/value/domain/httpOnly/sameSite), despite CONTRIBUTING.md claiming this class of dump is merge-blocked. Adds two co-occurrence patterns gated on a value-length discriminator (16+ chars), a synthetic fixture, a CLI explicit-path mode to prove end-to-end wiring without permanently tripping the repo-wide walk, and a TOCTOU fix (stat-then-read by path → stat-then-read through one open handle).

What I checked

Read the full diff, then pulled the branch into a worktree and ran tests/unit/secret-scan.test.ts directly — all 16 pass. I also wrote a throwaway repro against the exported scanText() to stress-test the one design choice that seemed riskiest (below); it confirmed the concern.

Strengths

• The value-length discriminator is the right call and well-justified: docs/privacy/session-custody.md and docs/gate/recorder.md both quote/discuss the real shape with elided values, so a shape-only pattern would have broken CI on existing docs the moment it merged. Both directions are tested, including the sharpest case (the doc that quotes the shape verbatim).
• The repo-wide "nothing already committed matches" test is the right guard for a merge-blocking check — it's what stops the positive tests from passing while CI silently breaks for everyone else.
• The TOCTOU fix (open once, stat + read through the same fd) is a genuine, well-motivated improvement, not scope creep — the comment correctly identifies why it matters more here than elsewhere.
• main() guarded behind an invokedDirectly check so importing the module for tests doesn't trigger a repo walk — small but easy to get wrong, done correctly.
• Nice catches documented in the PR body: the scanner's own comment tripping cookie-header, and the test fixture tripping its own pattern. Both fixed by assembling values at runtime rather than allowlisting.

Issue found

The two new patterns aren't proximity-scoped. storage-state-cookies and storage-state-origins each check three conditions — a shape indicator, a companion key, and SUBSTANTIAL_VALUE — as independent .test(body) calls against the entire file, not confined to the same object. I confirmed this fires as a false positive: three unrelated JSON fragments concatenated in one string (a "cookies" feature-flag array unrelated to sessions, an unrelated field elsewhere named httpOnly, and an unrelated "value" field elsewhere that happens to be 16+ characters — e.g. a hash or UUID) trips storage-state-cookies even though no actual storageState object exists anywhere in the file.

This is the one pattern in the file built this way — every other entry is a single contiguous regex, which is inherently scoped by construction. Given the PR's own stated priority ("a false positive here breaks CI for unrelated PRs, so the negative half of the suite is not padding"), the co-occurrence being unscoped works against that goal for inputs the current test suite doesn't cover — all of its negative cases are real, known repo documents, not synthetic unrelated-content collisions.

Filed as #115 with a suggested fix (bound the search window around each shape match instead of scanning the whole body independently) and a note that this doesn't currently trip on anything in the tree, and fails in the safe direction (blocks CI, doesn't let a secret through) — not blocking this PR.

Everything else

No concerns with the CLI explicit-path mode (not used by npm run secret-scan in CI — confirmed CI invokes the script with no args, so this is purely a test/manual-invocation convenience, not a scan-narrowing bypass), the fixture's extension staying outside the walk's textExt filter, or the .env.example / dotenv-file handling, which is unchanged.

---

Verdict: Merge, then resolve #115.

[myselfsiddharth]

@OM152002 Pls address the issue here.