From 8805fa8e12cb641c993945c65cb6e9c074ba31aa Mon Sep 17 00:00:00 2001 From: Nguyen Quang Trung Date: Sat, 8 Aug 2026 18:24:09 +0700 Subject: [PATCH 1/2] fix(scan): stop RLS scanner flagging REVOKE/GRANT and CTE reads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- internal/cli/cmdscan/scan.go | 132 +++++++++++++++++++++----- internal/cli/cmdscan/scanners_test.go | 130 +++++++++++++++++++++++++ 2 files changed, 237 insertions(+), 25 deletions(-) diff --git a/internal/cli/cmdscan/scan.go b/internal/cli/cmdscan/scan.go index b9b3b93..ef98f62 100644 --- a/internal/cli/cmdscan/scan.go +++ b/internal/cli/cmdscan/scan.go @@ -785,28 +785,71 @@ func scanWithBuiltinPatterns(root string) []Finding { }) } +var ( + // rlsCreateTablePattern flags a CREATE TABLE with no tenant column. + rlsCreateTablePattern = regexp.MustCompile(`(?i)create\s+table\s+\w+\s*\(`) + + // rlsSelectFromPattern matches a terminal SELECT and captures the relation + // it reads from, so the source can be checked against the file's CTEs. + rlsSelectFromPattern = regexp.MustCompile(`(?i)select\s+.+\s+from\s+(\w+)\s*;`) + + // rlsGrantRevokePattern matches a privilege statement. `REVOKE SELECT ON + // t FROM role;` contains both SELECT and FROM but is not a query — it is + // the opposite, a statement that takes read access away. Flagging it + // inverted the scanner's meaning: hardening was reported as exposure. + rlsGrantRevokePattern = regexp.MustCompile(`(?i)^\s*(?:grant|revoke)\b`) + + // rlsCTEPattern matches a common-table-expression definition — `WITH name + // AS (` or a continuation `, name AS (`. A SELECT reading a CTE (e.g. + // `SELECT count(*) INTO v FROM inserted;` after `WITH inserted AS ( + // INSERT ... RETURNING ... )`) reads rows the same statement just wrote; + // tenant scoping belongs on the writing arm, not on this read. + rlsCTEPattern = regexp.MustCompile(`(?i)(?:\bwith\b|,)\s+(\w+)\s+as\s*\(`) +) + // RunRLS scans for missing Row-Level-Security in SQL/migration files. +// +// Scanning is file-aware rather than line-local: CTE names are collected from +// the whole file first so that a SELECT reading a CTE is not mistaken for an +// unscoped read of a physical table. func RunRLS(root string) (*ScanResult, error) { res := &ScanResult{} - rules := []struct { - Name string - Pattern *regexp.Regexp - }{ - // CREATE TABLE without subsequent ENABLE ROW LEVEL SECURITY is a signal, - // but we keep it line-local: flag any CREATE TABLE missing tenant column. - {"missing-tenant-col-create-table", regexp.MustCompile(`(?i)create\s+table\s+\w+\s*\(`)}, - {"select-without-where-tenant", regexp.MustCompile(`(?i)select\s+.+\s+from\s+\w+\s*;`)}, - } - res.Findings = scanFilesExt(root, []string{".sql", ".pgsql"}, func(rel string, line int, text string) []Finding { + res.Findings = scanFilesWhole(root, []string{".sql", ".pgsql"}, func(rel, content string) []Finding { + // Pass 1 — every CTE name defined anywhere in this file. + cteNames := map[string]bool{} + for _, m := range rlsCTEPattern.FindAllStringSubmatch(content, -1) { + cteNames[strings.ToLower(m[1])] = true + } + + // Pass 2 — line-local rule evaluation, now with file context. var out []Finding - for _, r := range rules { - if r.Pattern.MatchString(text) && - !strings.Contains(strings.ToLower(text), "tenant") && - !strings.Contains(strings.ToLower(text), "workspace") { + for i, text := range strings.Split(content, "\n") { + text = strings.TrimRight(text, "\r") + lower := strings.ToLower(text) + // A line that already names the tenant column is scoped by + // construction; this predates the file-aware pass and is kept. + if strings.Contains(lower, "tenant") || strings.Contains(lower, "workspace") { + continue + } + add := func(rule string) { out = append(out, Finding{ - File: rel, Line: line, Rule: r.Name, Match: truncate(text, 100), Secret: "", + File: rel, Line: i + 1, Rule: rule, Match: truncate(text, 100), Secret: "", }) } + if rlsCreateTablePattern.MatchString(text) { + add("missing-tenant-col-create-table") + } + if m := rlsSelectFromPattern.FindStringSubmatch(text); m != nil { + // GRANT/REVOKE name SELECT as a privilege, not as a query. + if rlsGrantRevokePattern.MatchString(text) { + continue + } + // Reading a CTE defined in this file is not an unscoped read. + if cteNames[strings.ToLower(m[1])] { + continue + } + add("select-without-where-tenant") + } } return out }) @@ -919,6 +962,54 @@ func scanFiles(root string, fn func(rel string, line int, text string) []Finding return scanFilesExt(root, nil, fn) } +// skipScanDir reports whether a directory should not be descended into: +// version-control, package trees, generated build output, tool-managed +// directories, and test fixture trees — scanning them produces only noise +// (fixtures contain intentional examples). +func skipScanDir(name string) bool { + switch name { + case ".git", "node_modules", "vendor", ".forge", + ".next", ".nuxt", ".svelte-kit", + "dist", "build", "out", "output", + "coverage", ".nyc_output", ".cache", "tmp", ".tmp", + "fixtures", "testdata", ".playwright-mcp": + return true + } + return false +} + +// scanFilesWhole is like scanFilesExt but hands fn the entire file content +// instead of one line at a time. Use it for rules that need context beyond the +// current line — e.g. resolving whether a SELECT's source is a CTE defined +// earlier in the same file. fn is responsible for reporting 1-based line +// numbers. +func scanFilesWhole(root string, exts []string, fn func(rel, content string) []Finding) []Finding { + var out []Finding + _ = filepath.WalkDir(root, func(p string, d fs.DirEntry, err error) error { + if err != nil { + return nil + } + if d.IsDir() { + if skipScanDir(d.Name()) { + return filepath.SkipDir + } + return nil + } + if !matchesExts(d.Name(), exts) { + return nil + } + rel, _ := filepath.Rel(root, p) + rel = filepath.ToSlash(rel) + b, err := os.ReadFile(p) + if err != nil { + return nil + } + out = append(out, fn(rel, string(b))...) + return nil + }) + return out +} + // scanFilesExt is like scanFiles but restricts to files matching one of exts // (suffix match). Empty/nil exts means all text files. func scanFilesExt(root string, exts []string, fn func(rel string, line int, text string) []Finding) []Finding { @@ -928,16 +1019,7 @@ func scanFilesExt(root string, exts []string, fn func(rel string, line int, text return nil } if d.IsDir() { - name := d.Name() - // Skip version-control, package trees, generated build output, - // tool-managed directories, and test fixture trees — scanning - // them produces only noise (fixtures contain intentional examples). - switch name { - case ".git", "node_modules", "vendor", ".forge", - ".next", ".nuxt", ".svelte-kit", - "dist", "build", "out", "output", - "coverage", ".nyc_output", ".cache", "tmp", ".tmp", - "fixtures", "testdata", ".playwright-mcp": + if skipScanDir(d.Name()) { return filepath.SkipDir } return nil diff --git a/internal/cli/cmdscan/scanners_test.go b/internal/cli/cmdscan/scanners_test.go index 46cf7af..244a3e0 100644 --- a/internal/cli/cmdscan/scanners_test.go +++ b/internal/cli/cmdscan/scanners_test.go @@ -1035,3 +1035,133 @@ func TestRunPromptInjection_ReadmeDocExamples_NoFinding(t *testing.T) { } } } + +// TC-SCAN-RLS-03 (regression): `REVOKE SELECT ON t FROM role;` must NOT be +// flagged. The old line-local regex matched the SELECT/FROM keywords inside a +// privilege statement, so the scanner reported a hardening statement — one +// that REMOVES read access — as an unscoped read. Reported by the +// ai-marketing-platform repo, where two lockdown migrations were flagged. +func TestRunRLS_RevokeSelectNotFlagged(t *testing.T) { + t.Parallel() + root := t.TempDir() + writeFile(t, root, "lockdown.sql", "REVOKE SELECT ON auth.users FROM authenticated;\n") + res, err := RunRLS(root) + if err != nil { + t.Fatalf("RunRLS: %v", err) + } + if res.Count != 0 { + t.Fatalf("REVOKE SELECT is a privilege statement, not a query; got: %+v", res.Findings) + } +} + +// TC-SCAN-RLS-04 (regression): GRANT SELECT is the same shape as TC-03. +func TestRunRLS_GrantSelectNotFlagged(t *testing.T) { + t.Parallel() + root := t.TempDir() + writeFile(t, root, "grants.sql", "GRANT SELECT ON public.plans TO anon;\n") + res, err := RunRLS(root) + if err != nil { + t.Fatalf("RunRLS: %v", err) + } + if res.Count != 0 { + t.Fatalf("GRANT SELECT is a privilege statement, not a query; got: %+v", res.Findings) + } +} + +// TC-SCAN-RLS-05 (regression): a SELECT reading a CTE that the same statement +// just populated is not an unscoped read of a physical table — tenant scoping +// belongs on the writing arm. Requires file-level context, which the old +// line-local scanner did not have. +func TestRunRLS_CTESourceNotFlagged(t *testing.T) { + t.Parallel() + root := t.TempDir() + writeFile(t, root, "heal.sql", ` +WITH inserted AS ( + INSERT INTO memberships (user_id, role) + SELECT u.id, 'member' FROM candidates u + RETURNING 1 +) +SELECT count(*) INTO v_count FROM inserted; +`) + res, err := RunRLS(root) + if err != nil { + t.Fatalf("RunRLS: %v", err) + } + for _, f := range res.Findings { + if f.Rule == "select-without-where-tenant" && strings.Contains(f.Match, "FROM inserted") { + t.Fatalf("reading the CTE `inserted` should not be flagged; got: %+v", res.Findings) + } + } +} + +// TC-SCAN-RLS-06 (regression): a CTE declared as a continuation (`, name AS (`) +// rather than after WITH is recognised too. +func TestRunRLS_ContinuationCTENotFlagged(t *testing.T) { + t.Parallel() + root := t.TempDir() + writeFile(t, root, "view.sql", ` +WITH eligible AS ( + SELECT id FROM contacts WHERE opted_in +), +ad_metrics AS ( + SELECT spend, clicks FROM raw_metrics +) +SELECT * FROM ad_metrics; +`) + res, err := RunRLS(root) + if err != nil { + t.Fatalf("RunRLS: %v", err) + } + for _, f := range res.Findings { + if strings.Contains(f.Match, "FROM ad_metrics;") { + t.Fatalf("reading the continuation CTE `ad_metrics` should not be flagged; got: %+v", res.Findings) + } + } +} + +// TC-SCAN-RLS-07 (false-positive guard for the fix itself): the CTE exemption +// must not blanket-silence the rule. A real unscoped read of a physical table +// in a file that ALSO contains a CTE is still a finding. +func TestRunRLS_PhysicalTableStillFlaggedAlongsideCTE(t *testing.T) { + t.Parallel() + root := t.TempDir() + writeFile(t, root, "mixed.sql", ` +WITH inserted AS ( + INSERT INTO audit (msg) VALUES ('x') RETURNING 1 +) +SELECT count(*) INTO v FROM inserted; + +SELECT email, password_hash FROM users; +`) + res, err := RunRLS(root) + if err != nil { + t.Fatalf("RunRLS: %v", err) + } + found := false + for _, f := range res.Findings { + if f.Rule == "select-without-where-tenant" && strings.Contains(f.Match, "FROM users;") { + found = true + } + } + if !found { + t.Fatalf("unscoped read of physical table `users` must still be flagged; got: %+v", res.Findings) + } +} + +// TC-SCAN-RLS-08 (boundary): line numbers stay 1-based and correct after the +// switch from line-streaming to whole-file scanning. +func TestRunRLS_ReportsCorrectLineNumber(t *testing.T) { + t.Parallel() + root := t.TempDir() + writeFile(t, root, "lines.sql", "-- header\n-- second\nSELECT id FROM users;\n") + res, err := RunRLS(root) + if err != nil { + t.Fatalf("RunRLS: %v", err) + } + if len(res.Findings) != 1 { + t.Fatalf("expected exactly 1 finding, got: %+v", res.Findings) + } + if res.Findings[0].Line != 3 { + t.Fatalf("expected line 3, got %d", res.Findings[0].Line) + } +} From 3ee07f0390c57b44192c7ab77b7f441de153fdaf Mon Sep 17 00:00:00 2001 From: Nguyen Quang Trung Date: Sat, 8 Aug 2026 18:36:46 +0700 Subject: [PATCH 2/2] fix(qa): waive strict-testing gate in QA-24/QA-26 scratch pipeline runs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- scripts/forge-qa-real.sh | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/scripts/forge-qa-real.sh b/scripts/forge-qa-real.sh index e1b8b2d..84e0c0c 100644 --- a/scripts/forge-qa-real.sh +++ b/scripts/forge-qa-real.sh @@ -430,7 +430,18 @@ fi qa_run "QA-23 ship status (empty project, exit 0)" 0 "$FORGE_BIN" ship status # QA-24 Full pipeline via --json bypasses interactive gate; checkpoints key present -SHIP_FULL_OUT=$("$FORGE_BIN" ship --dry-run --no-branch --json "qa-smoke" 2>&1) && SHIP_FULL_EXIT=0 || SHIP_FULL_EXIT=$? +# +# --no-strict-testing is required, not incidental: the QA scratch project comes +# from `forge init --minimal`, so it has no go.mod / package.json / +# pyproject.toml, no cmd/mcp/, and 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 legitimately fails this project and the whole +# pipeline exits 1 — the gate working as designed, not a regression. These two +# scenarios assert that --json bypasses the *interactive* gate and emits the +# documented keys; they are not a test of whether an empty scratch directory +# can satisfy a four-stage testing audit. Waiving the gate is the documented +# escape hatch for exactly this case. +SHIP_FULL_OUT=$("$FORGE_BIN" ship --dry-run --no-branch --no-strict-testing --json "qa-smoke" 2>&1) && SHIP_FULL_EXIT=0 || SHIP_FULL_EXIT=$? if [[ "$SHIP_FULL_EXIT" -eq 0 ]] && echo "$SHIP_FULL_OUT" | grep -q '"checkpoints"'; then qa_pass "QA-24 ship --dry-run --no-branch --json (exit 0, checkpoints key present)" else @@ -441,7 +452,8 @@ fi qa_run "QA-25 ship spec --dry-run (single checkpoint, exit 0)" 0 "$FORGE_BIN" ship spec --dry-run "qa-spec-test" # QA-26 --json output contains dry_run field -SHIP_JSON_OUT=$("$FORGE_BIN" ship --dry-run --json "qa-json-test" 2>&1) && SHIP_JSON_EXIT=0 || SHIP_JSON_EXIT=$? +# --no-strict-testing for the same reason as QA-24 above. +SHIP_JSON_OUT=$("$FORGE_BIN" ship --dry-run --no-strict-testing --json "qa-json-test" 2>&1) && SHIP_JSON_EXIT=0 || SHIP_JSON_EXIT=$? if [[ "$SHIP_JSON_EXIT" -eq 0 ]] && echo "$SHIP_JSON_OUT" | grep -q '"dry_run"'; then qa_pass "QA-26 ship --dry-run --json (dry_run field present in output)" else