From 4ce8f4e012ba7b033f8a62ffb778a7a97dff253f Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 20 Jul 2026 04:22:41 +0000 Subject: [PATCH 1/5] fix(sqlsafety+security): close SQL denylist quoted-identifier bypass and npm-audit false-clean MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two verified defects from the weekly cloud audit: sqlsafety: the read-only SQL gate's side-effecting-function denylist (ContainsDeniedFunction) matched `[\s]*\(`, but stripSQLCommentsAndStrings never unwrapped double-quoted identifiers. Postgres folds an unquoted identifier to lowercase, so `"pg_terminate_backend"` names the same built-in as `pg_terminate_backend`, yet the interposed quote defeated the regex. `vxd db sql 'SELECT "pg_terminate_backend"(pid) FROM pg_stat_activity'` (and `"pg_read_file"('/etc/passwd')`) classified as read-only and ran without --write — exactly the cross-user disruption the denylist exists to block, and READ ONLY transactions do not stop these calls. The classifier now unwraps double-quoted identifiers to bare inner text so the true call shape is seen; this also hardens ClassifyQuery/IsMultiStatement which share the strip pass. security: parseNpmAudit unmarshalled only {vulnerabilities}, so npm's valid-JSON error envelope ({"error":{"code":"ENOLOCK",...}}, emitted when the audit cannot run — e.g. a package.json with no lockfile) parsed as a clean, empty scan. RunScanners then counted it in `ran` (coverage OK) instead of `failed` (coverage lost), so the dependency-CVE scan silently no-ops while the security gate — and security.require_scanners strict mode — believe the tool ran. parseNpmAudit now surfaces the error envelope as a failure. Both restore already-documented behavior (no doc/config change). Regression tests added: quoted-identifier denied/allowed cases + strip-pass unwrap; npm-audit ENOLOCK/code-only error envelopes. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01U2huFn4mXqnCNn1jZCZKWi --- internal/security/scanners.go | 23 +++++++++++++++ internal/security/scanners_test.go | 40 +++++++++++++++++++++++++++ internal/sqlsafety/sql_safety.go | 32 ++++++++++++++++++++- internal/sqlsafety/sql_safety_test.go | 17 ++++++++++++ 4 files changed, 111 insertions(+), 1 deletion(-) diff --git a/internal/security/scanners.go b/internal/security/scanners.go index dab6f38..ead82bc 100644 --- a/internal/security/scanners.go +++ b/internal/security/scanners.go @@ -5,6 +5,7 @@ import ( "bytes" "context" "encoding/json" + "fmt" "log" "os/exec" "path/filepath" @@ -279,6 +280,18 @@ func parseSemgrep(out []byte, repoDir string) ([]Finding, error) { func parseNpmAudit(out []byte) ([]Finding, error) { var doc struct { + // npm emits a valid-JSON error envelope (e.g. {"error":{"code": + // "ENOLOCK",...}}) when the audit cannot actually run — most commonly + // a package.json with no lockfile. That envelope unmarshals cleanly + // into a nil Vulnerabilities map, so without inspecting Error the scan + // would look like a clean pass and be counted in RunScanners' `ran` + // list — exactly the "failed scan masquerading as clean" that the + // `failed` list exists to prevent (and that require_scanners strict + // mode relies on). Surface it as an error so coverage loss is visible. + Error *struct { + Code string `json:"code"` + Summary string `json:"summary"` + } `json:"error"` Vulnerabilities map[string]struct { Name string `json:"name"` Severity string `json:"severity"` @@ -289,6 +302,16 @@ func parseNpmAudit(out []byte) ([]Finding, error) { if err := json.Unmarshal(out, &doc); err != nil { return nil, err } + if doc.Error != nil { + msg := doc.Error.Summary + if msg == "" { + msg = doc.Error.Code + } + if msg == "" { + msg = "npm audit reported an error with no detail" + } + return nil, fmt.Errorf("npm audit error: %s", msg) + } findings := make([]Finding, 0, len(doc.Vulnerabilities)) for pkg, v := range doc.Vulnerabilities { name := v.Name diff --git a/internal/security/scanners_test.go b/internal/security/scanners_test.go index b1e8031..1471a89 100644 --- a/internal/security/scanners_test.go +++ b/internal/security/scanners_test.go @@ -4,6 +4,7 @@ import ( "os" "path/filepath" "sort" + "strings" "testing" ) @@ -176,6 +177,45 @@ func TestParseNpmAudit(t *testing.T) { } } +// TestParseNpmAudit_ErrorEnvelope pins that npm's valid-JSON error envelope +// (emitted when the audit cannot run — e.g. no lockfile) is surfaced as an +// error rather than swallowed as a clean, empty scan. Otherwise RunScanners +// counts it in `ran` (clean coverage) instead of `failed` (coverage lost), +// letting a dependency-CVE scan silently no-op while the security gate — and +// require_scanners strict mode — believe the tool ran. +func TestParseNpmAudit_ErrorEnvelope(t *testing.T) { + cases := []struct { + name string + out string + want string + }{ + { + name: "ENOLOCK no lockfile", + out: `{"error":{"code":"ENOLOCK","summary":"This command requires an existing lockfile."}}`, + want: "This command requires an existing lockfile", + }, + { + name: "error with only code", + out: `{"error":{"code":"EAUDITNOPJSON"}}`, + want: "EAUDITNOPJSON", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := parseNpmAudit([]byte(tc.out)) + if err == nil { + t.Fatalf("parseNpmAudit(%s) = nil error, want failure surfaced; findings=%+v", tc.out, got) + } + if got != nil { + t.Errorf("expected no findings on error envelope, got %+v", got) + } + if !strings.Contains(err.Error(), tc.want) { + t.Errorf("error %q does not mention %q", err.Error(), tc.want) + } + }) + } +} + func TestParseGovulncheck(t *testing.T) { // govulncheck text output (the human format): we extract called vulns. out := []byte(`=== Symbol Results === diff --git a/internal/sqlsafety/sql_safety.go b/internal/sqlsafety/sql_safety.go index fa1c374..bf3ba91 100644 --- a/internal/sqlsafety/sql_safety.go +++ b/internal/sqlsafety/sql_safety.go @@ -178,10 +178,22 @@ func ValidateSQL(query string, writeFlag bool, extraDeny []string) error { } // stripSQLCommentsAndStrings removes -- line comments, /* */ block -// comments, and string literals (single-quoted) from the input. This +// comments, and string literals (single-quoted) from the input, and +// unwraps double-quoted identifiers to their bare inner text. This // neutralises ambushes like `SELECT 1; /* */ DROP TABLE foo` that would // fool a naive substring check. The output is suitable ONLY for classifier // use — it is not a valid SQL string. +// +// Double-quoted identifiers are unwrapped (the surrounding quotes dropped, +// `""` collapsed to nothing) rather than left verbatim: Postgres folds an +// unquoted identifier to lowercase, so `"pg_read_file"` names the *same* +// built-in as `pg_read_file`, yet the interposed quote between the name and +// its `(` used to defeat the denylist regex — a call like +// `SELECT "pg_terminate_backend"(pid)` slipped past ContainsDeniedFunction +// and ran under a read-only transaction that does not block it. Collapsing +// `"pg_read_file"(` to `pg_read_file(` makes the classifier see the true +// call. Inner text is stripped of quotes only; no denied function name +// contains a literal quote, so dropping the `""` escape body is safe here. func stripSQLCommentsAndStrings(s string) string { var b strings.Builder b.Grow(len(s)) @@ -223,6 +235,24 @@ func stripSQLCommentsAndStrings(s string) string { } continue } + // Double-quoted identifier: "..." with "" as escaped quote. Unwrap + // to the bare inner text so a quoted denied-function name is caught. + if s[i] == '"' { + i++ + for i < len(s) { + if s[i] == '"' { + if i+1 < len(s) && s[i+1] == '"' { + i += 2 + continue + } + i++ + break + } + b.WriteByte(s[i]) + i++ + } + continue + } b.WriteByte(s[i]) i++ } diff --git a/internal/sqlsafety/sql_safety_test.go b/internal/sqlsafety/sql_safety_test.go index 3847c6c..55030d6 100644 --- a/internal/sqlsafety/sql_safety_test.go +++ b/internal/sqlsafety/sql_safety_test.go @@ -136,6 +136,12 @@ func TestStripSQLCommentsAndStrings(t *testing.T) { {"SELECT 1 /* unterminated", "SELECT 1 "}, // Unterminated string — strip to EOF rather than panic. {"SELECT '''unterminated", "SELECT "}, + // Double-quoted identifiers unwrap to bare inner text so the + // classifier sees the true call shape. + {`SELECT "pg_read_file"('x')`, "SELECT pg_read_file()"}, + {`SELECT "a""b"`, "SELECT ab"}, + // Unterminated identifier — strip to EOF rather than panic. + {`SELECT "unterminated`, "SELECT unterminated"}, } for _, tt := range cases { if got := stripSQLCommentsAndStrings(tt.in); got != tt.want { @@ -168,6 +174,12 @@ func TestSQLSafety_FunctionDenylist(t *testing.T) { {"schema-qualified", "SELECT pg_catalog.pg_terminate_backend(1)"}, {"comment ambush reassembles", "SELECT pg_terminate/**/_backend(1)"}, {"nested in expression", "SELECT 1 WHERE pg_terminate_backend(pid) IS NOT NULL"}, + // Double-quoted identifiers name the same lowercase built-in in + // Postgres; the quotes must not smuggle a call past the denylist. + {"quoted identifier call", `SELECT "pg_terminate_backend"(1234)`}, + {"quoted identifier read_file", `SELECT "pg_read_file"('/etc/passwd')`}, + {"schema-qualified quoted", `SELECT pg_catalog."pg_read_file"('postgresql.conf')`}, + {"quoted whitespace before paren", `SELECT "pg_terminate_backend" (1)`}, } for _, tt := range denied { t.Run("denied/"+tt.name, func(t *testing.T) { @@ -190,6 +202,11 @@ func TestSQLSafety_FunctionDenylist(t *testing.T) { {"quoted string mention", "SELECT * FROM logs WHERE msg = 'pg_terminate_backend(1)'"}, {"comment mention", "SELECT 1 -- pg_terminate_backend(1)"}, {"plain select", "SELECT id, name FROM users"}, + // Quoted identifiers that are NOT a denied call must still pass: + // a quoted column that merely shares the name (no following paren), + // and an ordinary quoted column with a space. + {"quoted column named like function, no call", `SELECT "pg_terminate_backend" FROM audit_log`}, + {"quoted column with space", `SELECT "user id", name FROM users`}, } for _, tt := range allowed { t.Run("allowed/"+tt.name, func(t *testing.T) { From 5ef64e723653be096db6e15cb17ae8e64f8fab05 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 20 Jul 2026 04:29:08 +0000 Subject: [PATCH 2/5] fix(sqlsafety+security): close E'-string denylist bypass and semgrep false-clean (round 2 siblings) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two verified siblings of the round-1 fixes, found in the same two files: sqlsafety: stripSQLCommentsAndStrings modelled single-quoted strings with only ''-doubling escape semantics. Postgres *escape strings* (E'...'/e'...') let a backslash escape a quote, so `\'` does NOT close the string. The stripper mis-tracked the boundary and over-consumed, swallowing a denied call that followed a literal: `vxd db sql "SELECT E'\'' || pg_read_file('/etc/passwd')"` — a single valid statement — classified read-only and ran without --write (confirmed: ValidateSQL returned nil). The stripper now detects escape-string mode (E'/e' at a token boundary) and consumes backslash escapes so the boundary is correct; the trailing pg_read_file(/pg_terminate_backend( call is then caught. security: parseSemgrep decoded only "results" and ignored the top-level "errors" array. On a scan-level failure (e.g. `--config auto` cannot load rules on an offline host) semgrep emits valid JSON with results:[] + an error-level entry and exits non-zero, so parseSemgrep returned ([], nil) — counted in RunScanners' `ran` (clean) list, not `failed`, hiding total coverage loss from the gate and require_scanners strict mode (same masquerade class as the round-1 npm-audit fix). It now returns an error when results are empty AND a fatal error-level entry is present; a scan that produced findings is never flipped to failed by a coexisting non-fatal warning. Both restore already-documented behavior (no doc/config change). Regression tests added: E-string denied/allowed + strip-pass cases; semgrep scan-error envelope + results-plus-warning stays-clean. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01U2huFn4mXqnCNn1jZCZKWi --- internal/security/scanners.go | 26 ++++++++++++++++++++++++++ internal/security/scanners_test.go | 27 +++++++++++++++++++++++++++ internal/sqlsafety/sql_safety.go | 27 ++++++++++++++++++++++++++- internal/sqlsafety/sql_safety_test.go | 10 ++++++++++ 4 files changed, 89 insertions(+), 1 deletion(-) diff --git a/internal/security/scanners.go b/internal/security/scanners.go index ead82bc..da9c8e9 100644 --- a/internal/security/scanners.go +++ b/internal/security/scanners.go @@ -249,10 +249,36 @@ func parseSemgrep(out []byte, repoDir string) ([]Finding, error) { } `json:"metadata"` } `json:"extra"` } `json:"results"` + // semgrep always emits a top-level "errors" array alongside "results". + // On a scan-level failure (e.g. `--config auto` cannot load rules on an + // offline dispatch host) it emits valid JSON with results:[] and an + // error-level entry, and exits non-zero. Decoding only results would + // return ([], nil) — counted as a clean `ran` scan rather than `failed`, + // hiding total coverage loss from the gate and require_scanners strict + // mode. Inspect errors so that masquerade is surfaced. + Errors []struct { + Level string `json:"level"` + Message string `json:"message"` + } `json:"errors"` } if err := json.Unmarshal(out, &doc); err != nil { return nil, err } + // Fail closed only when no results were produced AND a fatal error is + // present: a scan that returned findings clearly wasn't a total loss, so + // non-fatal per-file warnings that coexist with results never flip a + // successful scan to failed. + if len(doc.Results) == 0 { + for _, e := range doc.Errors { + if strings.EqualFold(e.Level, "error") { + msg := e.Message + if msg == "" { + msg = "semgrep reported a scan error with no results" + } + return nil, fmt.Errorf("semgrep scan error: %s", msg) + } + } + } findings := make([]Finding, 0, len(doc.Results)) for _, r := range doc.Results { cwe := "" diff --git a/internal/security/scanners_test.go b/internal/security/scanners_test.go index 1471a89..cd0eede 100644 --- a/internal/security/scanners_test.go +++ b/internal/security/scanners_test.go @@ -143,6 +143,33 @@ func TestParseSemgrep(t *testing.T) { } } +// TestParseSemgrep_ScanErrorEnvelope pins that a scan-level failure (valid +// JSON, empty results, an error-level entry — e.g. rules failed to load on an +// offline host) is surfaced as a failure, not swallowed as a clean empty scan. +// Otherwise RunScanners counts it in `ran` (clean) instead of `failed` +// (coverage lost), and require_scanners strict mode would not block. +func TestParseSemgrep_ScanErrorEnvelope(t *testing.T) { + out := []byte(`{"results":[],"errors":[{"level":"error","message":"unable to load config: registry unreachable"}]}`) + got, err := parseSemgrep(out, "/repo") + if err == nil { + t.Fatalf("parseSemgrep = nil error, want scan failure surfaced; findings=%+v", got) + } + if !strings.Contains(err.Error(), "unable to load config") { + t.Errorf("error %q does not name the scan error", err.Error()) + } + + // A scan that produced findings is NOT flipped to failed by a coexisting + // non-fatal (warn-level) error entry — coverage clearly wasn't lost. + okOut := []byte(`{"results":[{"check_id":"r","path":"/repo/x.go","start":{"line":1},"extra":{"message":"m","severity":"ERROR","metadata":{}}}],"errors":[{"level":"warn","message":"partial parse"}]}`) + fs, err := parseSemgrep(okOut, "/repo") + if err != nil { + t.Fatalf("parseSemgrep with results + warn = %v, want nil", err) + } + if len(fs) != 1 { + t.Fatalf("expected 1 finding, got %d", len(fs)) + } +} + func TestParseNpmAudit(t *testing.T) { out := []byte(`{ "vulnerabilities": { diff --git a/internal/sqlsafety/sql_safety.go b/internal/sqlsafety/sql_safety.go index bf3ba91..ce50756 100644 --- a/internal/sqlsafety/sql_safety.go +++ b/internal/sqlsafety/sql_safety.go @@ -177,6 +177,16 @@ func ValidateSQL(query string, writeFlag bool, extraDeny []string) error { } } +// isIdentChar reports whether b can appear in a SQL identifier. Used to +// decide whether an E/e immediately before a quote is a standalone +// escape-string introducer (E'...') rather than the tail of an identifier. +func isIdentChar(b byte) bool { + return b == '_' || + (b >= 'a' && b <= 'z') || + (b >= 'A' && b <= 'Z') || + (b >= '0' && b <= '9') +} + // stripSQLCommentsAndStrings removes -- line comments, /* */ block // comments, and string literals (single-quoted) from the input, and // unwraps double-quoted identifiers to their bare inner text. This @@ -219,10 +229,25 @@ func stripSQLCommentsAndStrings(s string) string { } continue } - // Single-quoted string: '...' with '' as escaped quote + // Single-quoted string: '...'. A normal string uses '' as its only + // quote escape — standard_conforming_strings=on (the modern Postgres + // default) treats a backslash literally. A string introduced by E'/e' + // at a token boundary is an *escape string*: there a backslash escapes + // the next character, so `\'` does NOT close the string. Missing that + // mis-tracks the string boundary and lets a denied call following the + // literal be swallowed and hidden — e.g. + // `SELECT E'\'' || pg_read_file('x')` classified read-only despite the + // real pg_read_file call. Detect escape mode and consume backslash + // escapes so the boundary is tracked correctly. if s[i] == '\'' { + escapeString := i >= 1 && (s[i-1] == 'E' || s[i-1] == 'e') && + (i == 1 || !isIdentChar(s[i-2])) i++ for i < len(s) { + if escapeString && s[i] == '\\' { + i += 2 // skip the backslash and the char it escapes + continue + } if s[i] == '\'' { if i+1 < len(s) && s[i+1] == '\'' { i += 2 diff --git a/internal/sqlsafety/sql_safety_test.go b/internal/sqlsafety/sql_safety_test.go index 55030d6..3721a04 100644 --- a/internal/sqlsafety/sql_safety_test.go +++ b/internal/sqlsafety/sql_safety_test.go @@ -142,6 +142,10 @@ func TestStripSQLCommentsAndStrings(t *testing.T) { {`SELECT "a""b"`, "SELECT ab"}, // Unterminated identifier — strip to EOF rather than panic. {`SELECT "unterminated`, "SELECT unterminated"}, + // Escape string: \' is an escaped quote, not the closing quote, so + // the boundary is tracked and the trailing call survives stripping. + {`SELECT E'\'' || pg_read_file('x')`, "SELECT E || pg_read_file()"}, + {`SELECT e'\\'`, "SELECT e"}, } for _, tt := range cases { if got := stripSQLCommentsAndStrings(tt.in); got != tt.want { @@ -180,6 +184,10 @@ func TestSQLSafety_FunctionDenylist(t *testing.T) { {"quoted identifier read_file", `SELECT "pg_read_file"('/etc/passwd')`}, {"schema-qualified quoted", `SELECT pg_catalog."pg_read_file"('postgresql.conf')`}, {"quoted whitespace before paren", `SELECT "pg_terminate_backend" (1)`}, + // Escape strings (E'...') let a backslash escape a quote; the boundary + // must be tracked so a call after the literal is not hidden. + {"e-string escaped quote hides call", `SELECT E'\'' || pg_read_file('/etc/passwd')`}, + {"e-string lowercase", `SELECT e'\'' || pg_terminate_backend(1)`}, } for _, tt := range denied { t.Run("denied/"+tt.name, func(t *testing.T) { @@ -207,6 +215,8 @@ func TestSQLSafety_FunctionDenylist(t *testing.T) { // and an ordinary quoted column with a space. {"quoted column named like function, no call", `SELECT "pg_terminate_backend" FROM audit_log`}, {"quoted column with space", `SELECT "user id", name FROM users`}, + // A legitimate escape string that is NOT a denied call still passes. + {"e-string plain value", `SELECT E'O\'Brien' AS name`}, } for _, tt := range allowed { t.Run("allowed/"+tt.name, func(t *testing.T) { From 5cc749475c46b71bc6571293a22feb867104fe2e Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 20 Jul 2026 04:36:45 +0000 Subject: [PATCH 3/5] fix(sqlsafety+security): close dollar-quote SQL bypass and govulncheck false-clean (round 3 siblings) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third pair of verified siblings in the same two files: sqlsafety: stripSQLCommentsAndStrings had no dollar-quote ($tag$...$tag$) handling. A dollar-quoted literal containing an ODD number of apostrophes desynced the single-quote tracker, swallowing a denied call that followed the literal: `vxd db sql "SELECT $$'$$, pg_read_file('/etc/passwd')"` — a single valid statement whose pg_read_file call is real executable code outside any string — classified read-only and ran without --write (confirmed: ValidateSQL returned nil). The stripper now consumes dollar-quoted strings before the single-quote branch (tag follows identifier rules, so positional params like $1 are not treated as delimiters), fixing the desync. security: parseGovulncheck runs in text mode, so — unlike the JSON scanners whose failure output fails json.Unmarshal and routes to `failed` — a run that could not execute (e.g. the vulnerability DB unreachable on an offline host) produced zero findings and returned (nil, nil), counted in RunScanners' `ran` (clean) list. Same masquerade class as the npm-audit/semgrep fixes. It now detects govulncheck's "govulncheck: " fatal line and returns an error when no findings were parsed; a clean no-vuln run is never flipped to failed. The complete Postgres string/identifier lexical set is now handled by the classifier: '...' (incl. E'...' escape), "..." identifiers, and $tag$...$tag$. Both restore already-documented behavior (no doc/config change). Regression tests added for both. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01U2huFn4mXqnCNn1jZCZKWi --- internal/security/scanners.go | 21 ++++++++- internal/security/scanners_test.go | 26 ++++++++++++ internal/sqlsafety/sql_safety.go | 61 ++++++++++++++++++++++++--- internal/sqlsafety/sql_safety_test.go | 13 ++++++ 4 files changed, 115 insertions(+), 6 deletions(-) diff --git a/internal/security/scanners.go b/internal/security/scanners.go index da9c8e9..58255d1 100644 --- a/internal/security/scanners.go +++ b/internal/security/scanners.go @@ -360,10 +360,21 @@ func parseNpmAudit(out []byte) ([]Finding, error) { func parseGovulncheck(out []byte) ([]Finding, error) { var findings []Finding + // govulncheck runs in text mode, so — unlike the JSON scanners — a failure + // produces no unmarshal error to route it to `failed`. Its fatal errors + // print as "govulncheck: " (e.g. the vulnerability DB is unreachable + // on an offline dispatch host). Without detecting that, a scan that never + // ran returns zero findings and is counted as a clean `ran` scan, hiding + // total coverage loss from the gate and require_scanners strict mode — the + // same masquerade the JSON parsers guard against. + var scanErr string sc := bufio.NewScanner(bytes.NewReader(out)) sc.Buffer(make([]byte, 0, 64*1024), 1024*1024) for sc.Scan() { line := strings.TrimSpace(sc.Text()) + if strings.HasPrefix(line, "govulncheck:") && scanErr == "" { + scanErr = strings.TrimSpace(strings.TrimPrefix(line, "govulncheck:")) + } // Lines look like: "Vulnerability #1: GO-2024-1234" if !strings.HasPrefix(line, "Vulnerability #") { continue @@ -387,7 +398,15 @@ func parseGovulncheck(out []byte) ([]Finding, error) { Source: "scanner", }) } - return findings, sc.Err() + if err := sc.Err(); err != nil { + return nil, err + } + // A fatal error with no vulnerability findings means the scan did not run + // — surface it as a failure rather than a clean empty result. + if len(findings) == 0 && scanErr != "" { + return nil, fmt.Errorf("govulncheck error: %s", scanErr) + } + return findings, nil } // Run executes the scanner against repoDir and returns parsed findings. A diff --git a/internal/security/scanners_test.go b/internal/security/scanners_test.go index cd0eede..d4d6c58 100644 --- a/internal/security/scanners_test.go +++ b/internal/security/scanners_test.go @@ -272,3 +272,29 @@ Vulnerability #2: GO-2023-5678 t.Errorf("dependency CVE should be high, got %v", got[0].Severity) } } + +// TestParseGovulncheck_FatalErrorSurfaced pins that a govulncheck run that +// could not execute (e.g. the vulnerability DB is unreachable on an offline +// host) is surfaced as a failure, not swallowed as a clean, zero-finding scan. +// Text mode has no unmarshal error to route the failure to `failed`, so the +// "govulncheck: " fatal line is the signal. +func TestParseGovulncheck_FatalErrorSurfaced(t *testing.T) { + out := []byte("govulncheck: loading vulnerability database: Get \"https://vuln.go.dev/index/db.json\": dial tcp: lookup vuln.go.dev: no such host\n") + got, err := parseGovulncheck(out) + if err == nil { + t.Fatalf("parseGovulncheck = nil error, want fatal surfaced; findings=%+v", got) + } + if !strings.Contains(err.Error(), "vulnerability database") { + t.Errorf("error %q does not name the fatal cause", err.Error()) + } + + // A clean run (no vulns, no fatal line) is NOT flipped to an error. + clean := []byte("=== Symbol Results ===\n\nNo vulnerabilities found.\n") + fs, err := parseGovulncheck(clean) + if err != nil { + t.Fatalf("parseGovulncheck(clean) = %v, want nil", err) + } + if len(fs) != 0 { + t.Errorf("expected 0 findings on a clean scan, got %d", len(fs)) + } +} diff --git a/internal/sqlsafety/sql_safety.go b/internal/sqlsafety/sql_safety.go index ce50756..c56b3b0 100644 --- a/internal/sqlsafety/sql_safety.go +++ b/internal/sqlsafety/sql_safety.go @@ -177,18 +177,46 @@ func ValidateSQL(query string, writeFlag bool, extraDeny []string) error { } } +// isIdentStart reports whether b may begin a SQL identifier (letter or +// underscore, not a digit) — used to reject positional parameters like $1 as +// dollar-quote tags. +func isIdentStart(b byte) bool { + return b == '_' || (b >= 'a' && b <= 'z') || (b >= 'A' && b <= 'Z') +} + // isIdentChar reports whether b can appear in a SQL identifier. Used to // decide whether an E/e immediately before a quote is a standalone // escape-string introducer (E'...') rather than the tail of an identifier. func isIdentChar(b byte) bool { - return b == '_' || - (b >= 'a' && b <= 'z') || - (b >= 'A' && b <= 'Z') || - (b >= '0' && b <= '9') + return isIdentStart(b) || (b >= '0' && b <= '9') +} + +// dollarQuoteTag reports whether a dollar-quote opening delimiter begins at +// s[start] (which must be '$'), and if so returns the full delimiter token +// (e.g. "$$" or "$tag$"). The tag between the dollar signs, if present, +// follows unquoted-identifier rules (first char a letter or underscore), so a +// positional parameter like $1 is correctly rejected as a non-delimiter. +func dollarQuoteTag(s string, start int) (string, bool) { + j := start + 1 + for j < len(s) && s[j] != '$' { + if j == start+1 { + if !isIdentStart(s[j]) { + return "", false + } + } else if !isIdentChar(s[j]) { + return "", false + } + j++ + } + if j >= len(s) || s[j] != '$' { + return "", false + } + return s[start : j+1], true } // stripSQLCommentsAndStrings removes -- line comments, /* */ block -// comments, and string literals (single-quoted) from the input, and +// comments, string literals (single-quoted, including E'...' escape +// strings), and dollar-quoted strings ($tag$...$tag$) from the input, and // unwraps double-quoted identifiers to their bare inner text. This // neutralises ambushes like `SELECT 1; /* */ DROP TABLE foo` that would // fool a naive substring check. The output is suitable ONLY for classifier @@ -229,6 +257,29 @@ func stripSQLCommentsAndStrings(s string) string { } continue } + // Dollar-quoted string: $tag$ ... $tag$ (tag optional, so $$ ... $$). + // Handled before the single-quote branch because an odd number of + // apostrophes *inside* a dollar-quoted literal would otherwise desync + // the single-quote tracker and swallow a denied call that follows the + // literal — e.g. `SELECT $$'$$, pg_read_file('x')` classified read-only + // despite the real pg_read_file call outside any string. The tag + // follows identifier rules (letter/underscore first), so positional + // params like $1 are NOT treated as a delimiter. + if s[i] == '$' { + if tag, ok := dollarQuoteTag(s, i); ok { + closeIdx := strings.Index(s[i+len(tag):], tag) + if closeIdx < 0 { + i = len(s) // unterminated — consume to EOF + } else { + i = i + len(tag) + closeIdx + len(tag) + } + continue + } + // Not a dollar-quote delimiter ($1 param, lone $): ordinary char. + b.WriteByte(s[i]) + i++ + continue + } // Single-quoted string: '...'. A normal string uses '' as its only // quote escape — standard_conforming_strings=on (the modern Postgres // default) treats a backslash literally. A string introduced by E'/e' diff --git a/internal/sqlsafety/sql_safety_test.go b/internal/sqlsafety/sql_safety_test.go index 3721a04..7172b40 100644 --- a/internal/sqlsafety/sql_safety_test.go +++ b/internal/sqlsafety/sql_safety_test.go @@ -146,6 +146,11 @@ func TestStripSQLCommentsAndStrings(t *testing.T) { // the boundary is tracked and the trailing call survives stripping. {`SELECT E'\'' || pg_read_file('x')`, "SELECT E || pg_read_file()"}, {`SELECT e'\\'`, "SELECT e"}, + // Dollar-quoted strings are dropped; an embedded apostrophe does not + // desync the tracker, so a trailing call survives. + {`SELECT $$'$$, pg_read_file('x')`, "SELECT , pg_read_file()"}, + {`SELECT $t$a'b$t$ FROM x`, "SELECT FROM x"}, + {"SELECT n = $1", "SELECT n = $1"}, } for _, tt := range cases { if got := stripSQLCommentsAndStrings(tt.in); got != tt.want { @@ -188,6 +193,10 @@ func TestSQLSafety_FunctionDenylist(t *testing.T) { // must be tracked so a call after the literal is not hidden. {"e-string escaped quote hides call", `SELECT E'\'' || pg_read_file('/etc/passwd')`}, {"e-string lowercase", `SELECT e'\'' || pg_terminate_backend(1)`}, + // Dollar-quoted strings with an odd apostrophe inside must not desync + // the single-quote tracker and hide the trailing call. + {"dollar-quote odd apostrophe hides call", `SELECT $$'$$, pg_read_file('/etc/passwd')`}, + {"tagged dollar-quote hides call", `SELECT $tag$'$tag$, pg_terminate_backend(1)`}, } for _, tt := range denied { t.Run("denied/"+tt.name, func(t *testing.T) { @@ -217,6 +226,10 @@ func TestSQLSafety_FunctionDenylist(t *testing.T) { {"quoted column with space", `SELECT "user id", name FROM users`}, // A legitimate escape string that is NOT a denied call still passes. {"e-string plain value", `SELECT E'O\'Brien' AS name`}, + // Dollar-quoted literal with no trailing denied call passes; a + // positional parameter ($1) is not a dollar-quote delimiter. + {"dollar-quote plain value", `SELECT $$hello world$$ AS greeting`}, + {"positional parameter not a delimiter", `SELECT id FROM t WHERE n = $1`}, } for _, tt := range allowed { t.Run("allowed/"+tt.name, func(t *testing.T) { From 98766287917304146cc517e619466b806061490e Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 20 Jul 2026 04:41:14 +0000 Subject: [PATCH 4/5] =?UTF-8?q?fix(sqlsafety):=20reject=20Unicode-escape?= =?UTF-8?q?=20identifiers=20(U&"=E2=80=A6")=20in=20read-only=20mode?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fourth SQL-lexer sibling. A Postgres Unicode-escape identifier U&"\XXXX…" can name a function to call, but its \XXXX escapes decode only server-side — the denylist regex sees the raw escapes, so it never recognises the decoded name. `vxd db sql "SELECT U&\"\0070\0067_\0072\0065\0061\0064_\0066\0069\006C\0065\"('/etc/passwd')"` (the escapes decode to pg_read_file) classified read-only and ran without --write (confirmed: ValidateSQL returned nil), and READ ONLY does not block pg_read_file. Rather than attempt to decode arbitrary \XXXX / custom-UESCAPE sequences, the read-only gate now rejects U&"…" identifiers outright (re-run with --write if intentional). Only the identifier form is flagged: U&'…' is a string literal whose content never executes and is already stripped correctly. The stripper is refactored into lexSQL, which returns the stripped text plus a flag for a U&"…" identifier seen outside comments/strings; stripSQLCommentsAndStrings and the new hasUnicodeEscapeIdentifier are thin wrappers. This completes the Postgres string/identifier lexical set the classifier understands: '…' (incl. E'…'), "…" (incl. U&"…"), $tag$…$tag$, U&'…'. Regression tests added (rejected identifier forms, allowed U&'…' string / plain quoted identifier / u-prefixed name, --write opt-out). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01U2huFn4mXqnCNn1jZCZKWi --- internal/sqlsafety/sql_safety.go | 37 ++++++++++++++++++++++++- internal/sqlsafety/sql_safety_test.go | 40 +++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 1 deletion(-) diff --git a/internal/sqlsafety/sql_safety.go b/internal/sqlsafety/sql_safety.go index c56b3b0..7d7d7fd 100644 --- a/internal/sqlsafety/sql_safety.go +++ b/internal/sqlsafety/sql_safety.go @@ -164,6 +164,9 @@ func ValidateSQL(query string, writeFlag bool, extraDeny []string) error { if writeFlag { return nil } + if hasUnicodeEscapeIdentifier(query) { + return fmt.Errorf("query uses a Unicode-escape identifier (U&\"…\"), whose decoded name cannot be safety-checked; re-run with --write if this is intentional") + } if fn, hit := ContainsDeniedFunction(query, extraDeny); hit { return fmt.Errorf("query calls side-effecting function %s(), which READ ONLY transactions do not block; re-run with --write if this is intentional", fn) } @@ -233,8 +236,33 @@ func dollarQuoteTag(s string, start int) (string, bool) { // call. Inner text is stripped of quotes only; no denied function name // contains a literal quote, so dropping the `""` escape body is safe here. func stripSQLCommentsAndStrings(s string) string { + stripped, _ := lexSQL(s) + return stripped +} + +// hasUnicodeEscapeIdentifier reports whether the query contains a +// Postgres Unicode-escape *identifier* (U&"…") outside of comments and +// string literals. Such an identifier can name a function to call while its +// \XXXX escapes decode only server-side, so the denylist regex — which sees +// the raw escapes — cannot recognise the decoded name (e.g. +// `SELECT U&"\0070\0067_..."('/etc/passwd')` calls pg_read_file). The +// read-only gate rejects these rather than attempt to decode arbitrary +// escapes; an operator who truly needs one re-runs with --write. Only the +// identifier form is flagged: U&'…' is a string literal (its content never +// executes) and is already stripped correctly. +func hasUnicodeEscapeIdentifier(query string) bool { + _, u := lexSQL(query) + return u +} + +// lexSQL walks the query once, returning it with -- / block comments, +// string literals (single-quoted incl. E'…' escape strings, and +// dollar-quoted) removed and double-quoted identifiers unwrapped, plus a +// flag reporting whether a Unicode-escape identifier (U&"…") was seen. +func lexSQL(s string) (string, bool) { var b strings.Builder b.Grow(len(s)) + unicodeEscapeIdent := false i := 0 for i < len(s) { // Line comment: -- ... \n @@ -313,7 +341,14 @@ func stripSQLCommentsAndStrings(s string) string { } // Double-quoted identifier: "..." with "" as escaped quote. Unwrap // to the bare inner text so a quoted denied-function name is caught. + // A U&"…" / u&"…" prefix marks a Unicode-escape identifier whose + // \XXXX escapes decode only server-side — flag it so the read-only + // gate can reject it (the raw escapes can't be matched by the regex). if s[i] == '"' { + if i >= 2 && s[i-1] == '&' && (s[i-2] == 'U' || s[i-2] == 'u') && + (i == 2 || !isIdentChar(s[i-3])) { + unicodeEscapeIdent = true + } i++ for i < len(s) { if s[i] == '"' { @@ -332,5 +367,5 @@ func stripSQLCommentsAndStrings(s string) string { b.WriteByte(s[i]) i++ } - return b.String() + return b.String(), unicodeEscapeIdent } diff --git a/internal/sqlsafety/sql_safety_test.go b/internal/sqlsafety/sql_safety_test.go index 7172b40..ac1ab87 100644 --- a/internal/sqlsafety/sql_safety_test.go +++ b/internal/sqlsafety/sql_safety_test.go @@ -278,3 +278,43 @@ func TestSQLSafety_DenylistExtraConfig(t *testing.T) { t.Errorf("ContainsDeniedFunction = (%q, %v), want (audit_purge, true)", fn, hit) } } + +// TestSQLSafety_UnicodeEscapeIdentifier pins that a Postgres Unicode-escape +// identifier (U&"…") — whose \XXXX escapes decode to a function name only +// server-side, so the denylist regex can't see the real name — is rejected in +// read-only mode. The escapes below decode to pg_read_file. A U&'…' *string* +// (content never executes) and a plain double-quoted identifier are NOT +// flagged, and --write bypasses the check. +func TestSQLSafety_UnicodeEscapeIdentifier(t *testing.T) { + rejected := []string{ + `SELECT U&"\0070\0067_\0072\0065\0061\0064_\0066\0069\006C\0065"('/etc/passwd')`, + `SELECT u&"\0070\0067"()`, + `SELECT id FROM U&"\0074"`, // decoded table name, still refused read-only + } + for _, q := range rejected { + err := ValidateSQL(q, false, nil) + if err == nil { + t.Errorf("ValidateSQL(%q, false) = nil, want Unicode-escape rejection", q) + continue + } + if !strings.Contains(err.Error(), "Unicode-escape") { + t.Errorf("error %v does not name the Unicode-escape cause", err) + } + } + + allowed := []string{ + `SELECT U&'\0041' AS a`, // U&'…' is a string literal — safe + `SELECT "pg_read_file" FROM audit`, // plain quoted identifier, no U& + `SELECT amount FROM u_and_others`, // identifier merely starting u... + } + for _, q := range allowed { + if err := ValidateSQL(q, false, nil); err != nil { + t.Errorf("ValidateSQL(%q, false) = %v, want nil", q, err) + } + } + + // --write opts out of the safety check. + if err := ValidateSQL(`SELECT U&"\0070\0067"()`, true, nil); err != nil { + t.Errorf("ValidateSQL with --write = %v, want nil", err) + } +} From 4e7b7b4fdedcb9b7a65586a5aaef9a378a4040ba Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 20 Jul 2026 04:47:06 +0000 Subject: [PATCH 5/5] fix(security): fail closed when gosec reports package-load errors with no issues MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completes the fail-closed invariant across all five scanner parsers. gosec reports compile/package-load failures in its "Golang errors" map while still emitting valid JSON with an empty Issues list, so parseGosec (which decoded only Issues) returned a clean empty scan for a run that could not analyse the code — the same "failed scan masquerading as clean" the npm-audit/semgrep/ govulncheck fixes close. parseGosec now returns an error when Issues is empty AND Golang errors are present; a scan that produced issues is never flipped to failed by a coexisting per-file parse error. With this, parseGosec, parseGitleaks, parseNpmAudit, parseSemgrep and parseGovulncheck all surface a failed run as `failed` (coverage lost) rather than counting it in RunScanners' clean `ran` list. Regression test added. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01U2huFn4mXqnCNn1jZCZKWi --- internal/security/scanners.go | 11 +++++++++++ internal/security/scanners_test.go | 26 ++++++++++++++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/internal/security/scanners.go b/internal/security/scanners.go index 58255d1..f54a53b 100644 --- a/internal/security/scanners.go +++ b/internal/security/scanners.go @@ -180,10 +180,21 @@ func parseGosec(out []byte, repoDir string) ([]Finding, error) { ID string `json:"id"` } `json:"cwe"` } `json:"Issues"` + // gosec reports compile/package-load failures in "Golang errors" + // (filename → errors) while still emitting valid JSON with an empty + // Issues list. Decoding only Issues would return a clean empty scan + // for a run that could not actually analyse the code — the same + // masquerade the other parsers guard against. Fail closed only when + // no issues were produced, so a scan that found issues is never + // flipped to failed by a per-file parse error alongside real results. + GolangErrors map[string]json.RawMessage `json:"Golang errors"` } if err := json.Unmarshal(out, &doc); err != nil { return nil, err } + if len(doc.Issues) == 0 && len(doc.GolangErrors) > 0 { + return nil, fmt.Errorf("gosec reported %d package-load error(s) and no issues; scan coverage lost", len(doc.GolangErrors)) + } findings := make([]Finding, 0, len(doc.Issues)) for _, i := range doc.Issues { line, _ := strconv.Atoi(strings.SplitN(i.Line, "-", 2)[0]) // gosec may emit "12-14" diff --git a/internal/security/scanners_test.go b/internal/security/scanners_test.go index d4d6c58..55723c3 100644 --- a/internal/security/scanners_test.go +++ b/internal/security/scanners_test.go @@ -100,6 +100,32 @@ func TestParseGosec(t *testing.T) { } } +// TestParseGosec_PackageLoadErrorSurfaced pins that a gosec run that could not +// analyse the code (compile/package-load failure → valid JSON with empty +// Issues but a populated "Golang errors" map) is surfaced as a failure rather +// than swallowed as a clean, zero-issue scan. A scan that produced issues +// alongside a per-file error is NOT flipped to failed. +func TestParseGosec_PackageLoadErrorSurfaced(t *testing.T) { + failed := []byte(`{"Issues":[],"Golang errors":{"/repo/broken.go":[{"line":1,"column":1,"error":"expected declaration"}]},"Stats":{"files":0,"lines":0}}`) + got, err := parseGosec(failed, "/repo") + if err == nil { + t.Fatalf("parseGosec = nil error, want package-load failure surfaced; findings=%+v", got) + } + if !strings.Contains(err.Error(), "coverage lost") { + t.Errorf("error %q does not signal coverage loss", err.Error()) + } + + // Issues present alongside a Golang error → still a successful scan. + withResults := []byte(`{"Issues":[{"severity":"HIGH","cwe":{"id":"798"},"rule_id":"G101","details":"d","file":"/repo/a.go","line":"1"}],"Golang errors":{"/repo/b.go":[{"line":1,"column":1,"error":"x"}]}}`) + fs, err := parseGosec(withResults, "/repo") + if err != nil { + t.Fatalf("parseGosec with issues + error = %v, want nil", err) + } + if len(fs) != 1 { + t.Fatalf("expected 1 finding, got %d", len(fs)) + } +} + func TestParseGitleaks(t *testing.T) { out := []byte(`[ {"Description":"AWS Access Key","File":"config/prod.env","StartLine":3,"RuleID":"aws-access-token","Secret":"AKIAXXXXXXXX","Match":"AKIA..."},