diff --git a/internal/security/scanners.go b/internal/security/scanners.go index dab6f38..f54a53b 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" @@ -179,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" @@ -248,10 +260,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 := "" @@ -279,6 +317,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 +339,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 @@ -311,10 +371,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 @@ -338,7 +409,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 b1e8031..55723c3 100644 --- a/internal/security/scanners_test.go +++ b/internal/security/scanners_test.go @@ -4,6 +4,7 @@ import ( "os" "path/filepath" "sort" + "strings" "testing" ) @@ -99,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..."}, @@ -142,6 +169,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": { @@ -176,6 +230,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 === @@ -205,3 +298,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 fa1c374..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) } @@ -177,14 +180,89 @@ 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 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. This +// 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 // 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 { + 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 @@ -207,10 +285,48 @@ func stripSQLCommentsAndStrings(s string) string { } continue } - // Single-quoted string: '...' with '' as escaped quote + // 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' + // 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 @@ -223,8 +339,33 @@ 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. + // 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] == '"' { + if i+1 < len(s) && s[i+1] == '"' { + i += 2 + continue + } + i++ + break + } + b.WriteByte(s[i]) + i++ + } + continue + } 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 3847c6c..ac1ab87 100644 --- a/internal/sqlsafety/sql_safety_test.go +++ b/internal/sqlsafety/sql_safety_test.go @@ -136,6 +136,21 @@ 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"}, + // 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"}, + // 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 { @@ -168,6 +183,20 @@ 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)`}, + // 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)`}, + // 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) { @@ -190,6 +219,17 @@ 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`}, + // 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) { @@ -238,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) + } +}