fix(scan): stop the RLS scanner flagging REVOKE/GRANT and CTE reads

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

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)
+	}
+}

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