Skip to content

fix(scan): stop generic-bearer flagging test fixtures, and honour .forge/waivers - #48

Merged
teragrid merged 3 commits into
mainfrom
feature/scan-generic-bearer-test-fixtures
Sep 21, 2026
Merged

teragrid merged 3 commits into
mainfrom
feature/scan-generic-bearer-test-fixtures

Conversation

@teragrid

Copy link
Copy Markdown
Owner

This PR addresses:
No existing issue. Found running forge scan security on a real Next.js/Supabase repo, where it printed FORGE-3000 … 56 finding(s) detected; fix before shipping and exited non-zero, every time.

Scope

  • Bug fix
  • New feature (core)
  • New feature (plugin)
  • Documentation / Docs
  • Refactor / Tooling

Checklist

  • Tests added: unit + scanner-level + CLI-level (see below). Honest note: I wrote the code first and the tests after, then mutation-checked the tests (below) rather than watching them fail beforehand.
  • Fixes: #NNN: no issue exists; happy to file one if maintainers want it.
  • Signed-off-by: all three commits use git commit -s.
  • Pre-push checklist: make check not run in full: golangci-lint/govulncheck aren't installed on this machine. Ran instead: gofmt -s, go vet ./..., go build ./..., go test -race on the three affected packages and go test -count=1 ./... on the whole repo (77 packages ok). Please let CI run lint/vuln.

Description

Two problems, one root cause: the only way to get a clean scan on a project with fixtures was to edit the code.

1. generic-bearer flags fixtures and doc placeholders. The rule matches any quoted 16+ char literal assigned to a name containing token/secret/password/api-key. On the repo above that was 56 findings, all placeholders (test_access_token, whsec_placeholder, mock-refresh-token, sbp_your_token_here, YOUR_ACCESS_TOKEN), so the gate was permanently red and a real hit could hide in it.

A value is now recognised as a placeholder structurally (no directory is blanket-skipped): it must be a phrase — 2+ segments split on -/_, each a plain lower/UPPER/Capitalised word with ≤6 trailing digits, or a short number — and either contain a marker word (test, mock, fake, dummy, example, sample, placeholder, changeme, invalid, your, …) or sit in test code.

Still reported, in production and test code: anything opaque — sk_live_…/sk_test_…, real whsec_…, hex, UUIDs, base62, JWT headers, mixed-case or letter/digit blends, single long segments. Only generic-bearer changed: the AWS / sk- / GitHub-token / private-key rules are untouched and still fire in tests. The finding's secret field is byte-identical to before.

2. The waiver registry was never wired in. internal/waiver (DEV-M1-17) loads .forge/waivers/*.yml and is unit-tested, but nothing outside the package called it, so there was no in-tool way to accept one finding. ApplyWaivers now runs in forge scan <family> and in the ship checkpoint's security scan:

  • waived findings leave the result and don't affect count/status/exit code; they are counted in new ScanResult.Waived ("waived" in --json, waived: in text);
  • fail closed: a waiver missing rationale, approved_by or expires_at is an error, not a silent exemption;
  • an expired waiver is never honoured; the finding stays and note names the lapsed waiver;
  • fixed IsWaived returning "expired" at the first expired match even when a valid waiver followed, which made renewing by adding a new entry impossible.

Evidence (real repo, not synthetic)

findings exit
forge 1.10.8 56 1
this branch 3 1
this branch + 2 waiver files (3 findings) 0 (waived: 3) 0

The 3 that remain are what a heuristic should not guess: a documented, public webhook verify token in production code and two camelCase fixtures in one test file. Forge 1.10.8 ignores the waiver files (still exit 1 on the same tree), so a project can commit them before upgrading.

Tests (all pass; whole repo go test ./... = 77 packages ok)

  • placeholder_test.go: 14 real-world placeholders → recognised; 12 real-looking secrets × {src, test path} → still flagged; phrase-in-production-still-flagged; boundaries; isTestPath.
  • scanners_placeholder_test.go (TC-FP-07/08/09): placeholders not reported; real secrets in tests still reported; provider rules unaffected in test files; Secret field unchanged.
  • waivers_test.go: file-scoped and rule-wide waivers, other rule in same file not waived, expiry, renewal, incomplete/blank/malformed waivers, and a CLI test where the exit code flips only because of the waiver.
  • Mutation-checked: disabling the placeholder skip fails TC-FP-07; removing the plain-word check lets Stripe/whsec/UUID/etc. through and fails the negative tests; dropping the ApplyWaivers call fails both CLI tests; reverting the IsWaived fix fails both renewal tests.

Trade-offs for review

  • A passphrase-style literal inside test code (correct-horse-battery-staple in tests/) is now treated as a fixture. In production code it is still reported. That is the deliberate cost of not flagging every fixture; tell me if you'd rather require a marker word everywhere.
  • The marker list and the ≤6-digit limit are judgment calls; both are one-line constants in placeholder.go.

Noticed, not changed here

  • --since <ref> ignores the ref: it calls loadScanBaseline(root, scanner), which reads .forge/scan-history, so results differ between checkouts. Separate bug; happy to look at it in its own PR.
  • Test fixtures under tests/ in this repo contain intentionally broken .go files, so a bare gofmt -l . prints parse errors (unrelated to this change).

🤖 Generated with Claude Code

vietking and others added 3 commits September 21, 2026 14:16
…checkpoint

The waiver registry (internal/waiver, DEV-M1-17) was loaded and unit-tested but
never called by the scanner, so a repository had no in-tool way to accept a
specific finding: the only options were editing the code or a permanently red
gate. ApplyWaivers now runs after confidence assignment in `forge scan <family>`
and in the ship checkpoint's security scan.

- Matching findings are removed from the result and counted in the new
  ScanResult.Waived (`waived` in --json, a `waived:` line in text). They do not
  affect count, status or the exit code.
- Fail closed: a waiver missing rationale, approved_by or expires_at is an error,
  not a silent exemption.
- An expired waiver is never honoured; the finding stays and the result note
  names the lapsed waiver.
- IsWaived returned "expired" at the first expired match even when a valid
  waiver for the same rule and file followed, so a lapsed waiver could not be
  renewed by adding a new entry. A valid match now wins.

Tests cover file-scoped and rule-wide waivers, expiry, renewal, incomplete and
malformed waivers, and the CLI exit code flipping only because of the waiver.
Mutation-checked: dropping the ApplyWaivers call or reverting the IsWaived fix
fails the tests.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: VietKing <likeawind510@gmail.com>
…lders

The built-in generic-bearer heuristic flags any quoted 16+ character literal
assigned to a name containing token/secret/password/api-key. On a real
Next.js/Supabase repo that was 56 findings, every one a placeholder
(test_access_token, whsec_placeholder, mock-refresh-token, sbp_your_token_here),
so `forge scan security` was permanently red and real hits could hide in it.

A value is now recognised as a placeholder structurally, not by silencing
directories: it must be a phrase (2+ segments split on - or _, each a plain
lower/UPPER/Capitalised word with up to six trailing digits, or a short number)
AND either contain a marker word (test, mock, fake, dummy, example, sample,
placeholder, changeme, invalid, your, redacted, xxx, demo, fixture, stub) or sit
in test code. Opaque values are never excused, in production or in tests:
sk_live_/sk_test_ keys, real whsec_, hex, UUIDs, base62, JWT headers, mixed-case
and letter/digit blends, and single long segments. A phrase with no marker is
still reported outside test code. Only generic-bearer changed; the AWS, sk-,
GitHub-token and private-key rules still fire everywhere, including tests. The
finding's Secret field is byte-identical to before.

Measured on the repo that produced the findings: 56 -> 3. The 3 left (a
documented public verify token and two camelCase fixtures) are what waivers are
for, and with three waivers the scan is clean with exit 0.

Tests pin the rule to the real values (TC-FP-07/08/09), include the negative
cases, and were mutation-checked: disabling the skip fails the placeholder test,
and removing the plain-word check makes real secrets slip through and fails the
negative tests.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: VietKing <likeawind510@gmail.com>
…er format

CHANGELOG entry for the two scan fixes and the IsWaived renewal fix; docs/verbs/scan.md
gains the exact placeholder rule with its limits and the .forge/waivers file format.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: VietKing <likeawind510@gmail.com>
@teragrid
teragrid merged commit 4513969 into main Sep 21, 2026
26 checks passed
@teragrid teragrid mentioned this pull request Sep 21, 2026
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants