chore(audit): weekly cloud audit — 4 SQL read-only bypasses + 4 scanner false-clean masquerades

[tzone85]

Weekly cloud audit — 2026-07-20

Fan-out audit across concurrency, silent failures, event-sourcing wiring, and security, then five loop-until-dry rounds with every candidate adversarially verified before fixing (rounds 4 and 5 both returned no findings → two consecutive dry passes → done; round 4 additionally verified the SQL findings against a live PostgreSQL 16 instance). Concurrency and event-sourcing wiring came back clean. Two real defect classes surfaced, each with several siblings — 8 verified fixes total, all in two files.

Class A — internal/sqlsafety/sql_safety.go: read-only SQL gate bypasses

vxd db sql (and the sql_query_returns QA criterion) run operator SQL under BEGIN READ ONLY, which does not block side-effecting function calls (pg_terminate_backend, pg_read_file, lo_import, …) — so those are rejected statically by ContainsDeniedFunction, which strips comments/strings then regex-matches <fn>(. The classifier's string/identifier lexer had gaps that let a denied call reach the DB without --write. Each was confirmed empirically (ValidateSQL(query, false, nil) returned nil).

• stripSQLCommentsAndStrings — double-quoted identifiers. The stripper left "..." verbatim. Postgres folds an unquoted identifier to lowercase, so "pg_terminate_backend" names the same built-in — but the quote between the name and ( defeated the regex. SELECT "pg_terminate_backend"(pid) / SELECT "pg_read_file"('/etc/passwd') classified read-only. Fix: unwrap double-quoted identifiers to bare inner text.
• stripSQLCommentsAndStrings — E'…' escape strings. Single-quoted handling assumed only '' escaping; Postgres escape strings let \' escape a quote, so the stripper mis-tracked the boundary and swallowed a call after the literal. SELECT E'\'' || pg_read_file('/etc/passwd') (a single valid statement) bypassed. Fix: detect E'/e' at a token boundary and consume backslash escapes.
• stripSQLCommentsAndStrings — dollar-quoted strings. No $tag$…$tag$ handling; a dollar-quoted literal with an odd number of apostrophes desynced the single-quote tracker and hid the trailing call. SELECT $$'$$, pg_read_file('/etc/passwd') bypassed. Fix: consume dollar-quoted strings before the single-quote branch (tag follows identifier rules, so $1 params are not delimiters).
• ValidateSQL / hasUnicodeEscapeIdentifier — U&"…" Unicode-escape identifiers. A U&"\XXXX…" identifier can name a function, but its escapes decode only server-side, so the regex (seeing raw \XXXX) never matches the decoded name. SELECT U&"\0070\0067_\0072\0065\0061\0064_\0066\0069\006C\0065"('/etc/passwd') (decodes to pg_read_file) bypassed. Decoding arbitrary escapes is error-prone, so the gate now rejects U&"…" identifiers in read-only mode (--write opts out). Only the identifier form is flagged — U&'…' is a string literal (its content never executes) and is already stripped correctly.

The classifier now understands the full Postgres string/identifier lexical set: '…' (incl. E'…'), "…" (incl. U&"…"), $tag$…$tag$, and U&'…'. stripSQLCommentsAndStrings was refactored into lexSQL. Documented assumption (unchanged, in-code): standard_conforming_strings=on, the Postgres default and the postgres:16 devdb default; with it off the mismatch direction is over-rejection, never concealment.

Class B — internal/security/scanners.go: failed scans masquerading as clean

RunScanners sorts each tool into ran (clean), skipped (tool absent), or failed (coverage lost). A parser returning (nil, nil) for a run that couldn't actually scan lands in ran, so the security gate — and security.require_scanners strict mode — believe the tool provided coverage it never did. The JSON scanners whose failure output is non-conforming already fail closed via json.Unmarshal; four parsers had a fail-open path:

• parseNpmAudit — npm's valid-JSON error envelope ({"error":{"code":"ENOLOCK",…}}, e.g. no lockfile) decoded to a nil vuln map → clean. Fix: surface the error field as a failure.
• parseSemgrep — decoded only results, ignoring the top-level errors array; a scan-level failure (e.g. --config auto can't load rules offline) emits results:[] + an error-level entry → clean. Fix: fail when results is empty and a level:"error" entry is present (a scan that produced findings is never flipped).
• parseGovulncheck — text-mode parser with no failure detection; an unreachable vuln DB (offline host) produced zero Vulnerability # lines → clean. Fix: detect the govulncheck: fatal line and fail when no findings were parsed.
• parseGosec — decoded only Issues, ignoring the "Golang errors" map; a compile/package-load failure emits valid JSON with empty Issues → clean. Fix: fail when Issues is empty and Golang errors are present.

All five parsers (parseGosec, parseGitleaks, parseNpmAudit, parseSemgrep, parseGovulncheck) now surface a failed run as failed, upholding the module's stated invariant that "a failed scan must never masquerade as a clean one."

Every fix restores already-documented behavior (denylist doc comment; RunScanners "must never masquerade" comment), so no CLI/config/doc-coverage changes are required. Regression tests were added for all eight.

Verification

Run with the project's required toolchain (GOTOOLCHAIN=go1.26.5); golangci-lint and govulncheck were re-installed against go1.26.5 (the pre-installed builds were compiled with go1.25 and refuse a go1.26 target).

• go build ./... — OK
• go vet ./... — OK
• golangci-lint run ./... (config default: standard) — 0 issues
• go test ./internal/sqlsafety/... ./internal/security/... — PASS (the two changed packages, incl. all new regression tests)
• Doc-coverage (TestDocCoverage_*) — PASS

Pre-existing, environment-only failures (present on unmodified main, unrelated to this diff): the cloud runner executes the suite as root (uid 0) and without the gh CLI, breaking tests that assert on filesystem-permission or gh-required failure paths — internal/cli TestApproveStory_*/TestApproveAll_*/TestRunApprove_* (no gh → "gh CLI is required" short-circuit), internal/engine TestWriteLockFile_InvalidPath + TestWriteMetadata_InvalidDir (root MkdirAll succeeds under /), internal/figma TestResolveToken_UnreadableFileIsDistinguished (root reads through chmod 0o000), internal/repolearn TestCountFilesInDir_NonexistentDir (polluted by /nonexistent/path the root engine test creates). Verified by stashing this diff and re-running on clean main — an identical 10-test set fails there. Not touched.

govulncheck / make vuln: could not run — the environment's egress policy denies vuln.go.dev (403 on CONNECT), so the vulnerability DB is unreachable. Not retried, per the proxy's report-don't-route-around rule. go build/vet/test served as the fallback gate. (This same unreachable-DB condition is exactly the failure the parseGovulncheck fix above surfaces instead of hiding.)

🤖 Generated with Claude Code

https://claude.ai/code/session_01U2huFn4mXqnCNn1jZCZKWi