fix(scan): stop the RLS scanner flagging REVOKE/GRANT and CTE reads - #39
Merged
Merged
Conversation
The `select-without-where-tenant` rule matched line-locally with the regex `select\s+.+\s+from\s+\w+\s*;`, which produced two classes of false positive: 1. `REVOKE SELECT ON auth.users FROM authenticated;` matched, because SELECT and FROM both appear in a privilege statement. The scanner reported a statement that *removes* read access as an unscoped read — inverting its own meaning, so hardening looked like exposure. 2. `SELECT count(*) INTO v FROM inserted;` matched, where `inserted` is a CTE the same statement just populated (`WITH inserted AS (INSERT ... RETURNING ...)`). Tenant scoping belongs on the writing arm; the read cannot be scoped and is not a leak. Resolving this needs context beyond the current line, which a line-local scanner does not have. RunRLS now scans whole files: pass 1 collects every CTE name (`WITH x AS (` and continuation `, x AS (`), pass 2 evaluates the rules with that context and skips lines whose leading keyword is GRANT or REVOKE. Adds `scanFilesWhole` alongside `scanFilesExt`, and extracts the shared directory-skip list into `skipScanDir()` rather than duplicating it. Six regression tests, including a false-positive guard for the fix itself (TC-SCAN-RLS-07): a genuine unscoped read of a physical table in a file that also contains a CTE must still be flagged, so the exemption cannot silently become an off switch for the rule. TC-SCAN-RLS-08 pins line numbers, which changed representation when scanning moved off bufio line streaming. Found by dogfooding on the ai-marketing-platform repo, where all 8 findings from `forge scan security` were false positives — 7 of them from this rule. Verified: that repo now reports `findings: 0, clean`, and a real unscoped SELECT still trips the rule. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
QA-24 and QA-26 run the full ship pipeline against a scratch project created by `forge init --minimal` — no go.mod / package.json / pyproject.toml, no cmd/mcp/, no testing-pipeline.md. The four-stage testing gate became BLOCKING by default in 1.8.2 (re-released as 1.9.0), so from that release on QA-Verify correctly fails such a project and the pipeline exits 1. Both scenarios assert exit 0, so both have failed ever since — on a clean `main`, unrelated to any diff being pushed. That left stage [13/13] of the pre-push hook permanently red, which is precisely the state in which a real failure goes unnoticed. The gate is behaving correctly; the expectations were stale. What these two scenarios actually cover is that `--json` bypasses the *interactive* gate and emits the documented `checkpoints` / `dry_run` keys — not whether an empty scratch directory can satisfy a four-stage testing audit. Both now pass --no-strict-testing, the documented waiver for exactly this case, with a comment recording why it is required rather than incidental. Verified: `SHIP_QA_ONLY=1 FORGE_NO_LLM=1 bash scripts/forge-qa-real.sh` goes from 2 of 12 failing to all 12 passing. QA-27 also runs the full pipeline but passes on its own terms and is left untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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.
Why
Dogfooding
forge scan securityon theai-marketing-platformrepo returned8 findings, all 8 false positives — 7 of them from a single rule,
select-without-where-tenant. A permanently-red gate is exactly the state inwhich a real finding goes unnoticed, so this fixes the rule rather than
suppressing the findings.
The two bugs
1.
REVOKE/GRANTmatched as if they were queries.The line-local regex
select\s+.+\s+from\s+\w+\s*;matches this, becauseSELECTandFROMboth appear — as a privilege name and a role separator, notas a query. The scanner was reporting a statement that removes read access
as an unscoped read: hardening reported as exposure, the rule's own meaning
inverted. Two lockdown migrations were flagged this way.
2. Reads of a CTE the same statement just populated.
insertedis not a physical table. Tenant scoping belongs on the writing arm;this read cannot be scoped and leaks nothing. Distinguishing it needs context
beyond the current line, which a line-local scanner does not have.
What changed
RunRLSnow scans whole files instead of streaming lines:WITH x AS (and thecontinuation form
, x AS (leading keyword is
GRANTorREVOKESupporting changes:
scanFilesWholealongsidescanFilesExt, for rules needing file contextskipScanDir()and shared, ratherthan duplicated into the new walker
Tests
Six new cases. TC-07 is the one that matters most — a false-positive guard
for the fix itself:
REVOKE SELECT … FROM role;not flaggedGRANT SELECT … TO role;not flaggedWITHCTE not flagged, x AS (continuation CTE not flaggedbufioline streamingSecond commit — an unrelated red gate found on the way
Pushing this branch was blocked by pre-push stage
[13/13], which was alreadyfailing on a clean
main: QA-24 and QA-26 run the full ship pipelineagainst a
forge init --minimalscratch project (nogo.mod/package.json,no
cmd/mcp/, notesting-pipeline.md) and assert exit 0. The four-stagetesting gate became blocking by default in 1.8.2 / 1.9.0, so QA-Verify has
correctly failed that project ever since — the gate works, the expectations
were stale.
Both scenarios exist to check that
--jsonbypasses the interactive gate andemits the documented
checkpoints/dry_runkeys, not to prove an emptydirectory can pass a four-stage testing audit. They now pass
--no-strict-testing, the documented waiver for this case, with a commentrecording why it is required. QA-27 also runs the full pipeline but passes on
its own terms and is left untouched.
Verification
go test ./internal/cli/cmdscan/— pass, including the 6 new casesgo test ./...— 77 packages, no failuresSHIP_QA_ONLY=1 FORGE_NO_LLM=1 bash scripts/forge-qa-real.sh— 2 of 12failing → all 12 passing
ai-marketing-platform:findings: 0, status: clean(was 8), and a real unscopedSELECTstilltrips the rule
🤖 Generated with Claude Code