From f748f6c6f3daa39cd05b386574f86ee685e91515 Mon Sep 17 00:00:00 2001 From: kartik Date: Sat, 26 Sep 2026 06:08:49 +0530 Subject: [PATCH 1/3] fix(analyzer): honor sqlguard:ignore only inside SQL comments The in-SQL directive only had to follow a comment marker somewhere earlier in the text, and the marker could itself sit inside a string, so SELECT * FROM users WHERE note = '-- sqlguard:ignore' reported nothing. Where a query embeds user input, a value could switch every rule off. parseIgnoreDirective now lexes the SQL and matches the directive only in comment spans, skipping quoted runs and dollar-quoted bodies. Where dialects disagree about literals (backslash escapes, $$) every reading is lexed and a directive counts only if all agree. Comment markers vary too: # is XOR in Postgres and MySQL's -- needs trailing whitespace, so a directive is dropped if the strict markers put it inside a literal. // inside SQL, never a SQL comment, is no longer honored. Closes #66 --- .codeant/review.json | 6 ++ .coderabbit.yaml | 10 +++ .greptile/config.json | 6 ++ .greptile/rules.md | 37 ++++++--- AGENTS.md | 2 +- CHANGELOG.md | 12 +++ analyzer/suppress.go | 156 +++++++++++++++++++++++++++++------ analyzer/suppress_test.go | 74 +++++++++++++++++ website/docs/suppressions.md | 19 ++++- 9 files changed, 278 insertions(+), 44 deletions(-) create mode 100644 analyzer/suppress_test.go diff --git a/.codeant/review.json b/.codeant/review.json index 852398d..f37f518 100644 --- a/.codeant/review.json +++ b/.codeant/review.json @@ -18,6 +18,12 @@ "files": ["analyzer/redact.go", "analyzer/fallback.go", "analyzer/*_test.go"], "scope": ["pr", "ide"] }, + { + "id": "suppression-comment-only", + "description": "An in-SQL sqlguard:ignore directive (-- , /* */ or #) must be honored only inside a comment span, never inside a string literal, quoted identifier or dollar-quoted body. parseIgnoreDirective lexes the SQL and matches ignoreTokenRe only against comment text; matching raw SQL with a regex anchored on a comment marker is not enough, because the marker can itself sit inside a string - a user-supplied value of '-- sqlguard:ignore' switched every rule off (#66). Where a comment starts depends on where each literal ends, so it runs every dialect reading (backslash escapes on/off, $$ as a delimiter or not) and honors a directive only where ALL readings put it in a comment (intersectDirectives): a spurious suppression hides findings, a missed one only reports a finding. Comment markers are dialect-dependent too (# is XOR in Postgres; MySQL's -- needs trailing whitespace), so a directive found in a comment is still dropped if the strict markers put it inside a literal. This is the opposite side from Redact's union, on purpose. Flag a change that matches the directive against raw SQL again, drops a reading, unions the readings, or treats // as a SQL comment. ParseIgnoreComment (Go source) may use the marker-less regex because go/ast has already stripped the //. TestParseIgnoreDirective pins the literal and dialect-ambiguous cases.", + "files": ["analyzer/suppress.go", "analyzer/fallback.go", "analyzer/redact.go", "analyzer/*_test.go"], + "scope": ["pr", "ide"] + }, { "id": "single-analysis-core", "description": "middleware.Guard (Check / CheckLatency / Observe / ResetN1 / Analyzer) is the single analysis core. Every interception point in the driver chain and every out-of-tree integration must route through one Guard; integrations/pgxguard is the reference example. Flag any hand-rolled check or latency logic that bypasses it - that shape silently loses redaction-by-default, fingerprints, the parser seam, file config, N+1, dedup and the analysis cache. Per-query work must stay allocation-light and config-free: enable/disable, severity and settings are resolved once in NewGuard and DefaultWithProfile, never per query.", diff --git a/.coderabbit.yaml b/.coderabbit.yaml index a274867..f3889ad 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -90,6 +90,16 @@ reviews: Flag any change that drops a reading, makes the two lexers disagree, or removes TestRedactNoLeakAcrossDialectAmbiguity / TestIsMultiStatementNeedsBothReadings. + An in-SQL sqlguard:ignore directive counts only inside a comment span: + parseIgnoreDirective lexes the SQL and matches the directive only in + comment text, because a regex anchored on a comment marker also matched + '-- sqlguard:ignore' inside a user-supplied string and switched every + rule off (#66). It runs every dialect reading and honors a directive only + where ALL agree it is in a comment — a spurious suppression hides + findings, a missed one only reports one. # (XOR in Postgres) and a + MySQL -- without trailing whitespace must not reach into a literal. + Flag a change that matches raw + SQL again, drops a reading, unions them, or treats // as a SQL comment. - path: "middleware/**" instructions: >- diff --git a/.greptile/config.json b/.greptile/config.json index 20555f8..fc20403 100644 --- a/.greptile/config.json +++ b/.greptile/config.json @@ -24,6 +24,12 @@ "scope": ["analyzer/redact.go", "analyzer/fallback.go"], "severity": "high" }, + { + "id": "suppression-comment-only", + "rule": "An in-SQL sqlguard:ignore directive (-- , /* */ or #) must be honored only inside a comment span, never inside a string literal, quoted identifier or dollar-quoted body. parseIgnoreDirective lexes the SQL and matches ignoreTokenRe only against comment text; matching raw SQL with a regex anchored on a comment marker is not enough, because the marker can itself sit inside a string - a user-supplied value of '-- sqlguard:ignore' switched every rule off (#66). Where a comment starts depends on where each literal ends, so it runs every dialect reading (backslash escapes on/off, $$ as a delimiter or not) and honors a directive only where ALL readings put it in a comment (intersectDirectives): a spurious suppression hides findings, a missed one only reports a finding. Comment markers are dialect-dependent too (# is XOR in Postgres; MySQL's -- needs trailing whitespace), so a directive found in a comment is still dropped if the strict markers put it inside a literal. This is the opposite side from Redact's union, on purpose. Flag a change that matches the directive against raw SQL again, drops a reading, unions the readings, or treats // as a SQL comment. ParseIgnoreComment (Go source) may use the marker-less regex because go/ast has already stripped the //. TestParseIgnoreDirective pins the literal and dialect-ambiguous cases.", + "scope": ["analyzer/suppress.go", "analyzer/fallback.go", "analyzer/redact.go"], + "severity": "high" + }, { "id": "single-analysis-core", "rule": "middleware.Guard (Check / CheckLatency / Observe / ResetN1 / Analyzer) is the single analysis core. Every interception point in the driver chain and every out-of-tree integration must route through one Guard. Flag any hand-rolled check/latency logic that bypasses it — it silently loses redaction, fingerprints, the parser seam, config, N+1, and dedup.", diff --git a/.greptile/rules.md b/.greptile/rules.md index b9b6311..36ed6e9 100644 --- a/.greptile/rules.md +++ b/.greptile/rules.md @@ -37,19 +37,30 @@ together: `validate()`'s SELECT/WITH-only-by-default policy, never using `!dml` back to `true`; if you touch this, `TestMySQL_ExplainDMLDoesNotMutate` is the test that would catch a real regression. -## Suppression uses two different regexes on purpose - -`analyzer/suppress.go` has two suppression paths that look like they could -share a regex but must not: the in-SQL `-- sqlguard:ignore` / `/* sqlguard: -ignore:rule-a,rule-b */` form is parsed from raw SQL text with a -**marker-anchored** pattern specifically to avoid a string literal like -`'-- sqlguard:ignore'` inside a query being mistaken for a real directive. The -Go-source `// sqlguard:ignore[:rules]` form (`ParseIgnoreComment`) uses a -**separate, marker-less** pattern because `go/ast` has already stripped the -`//` by the time this code sees the comment text. Reusing one regex for the -other either breaks Go-source suppression (the marker-anchored form expects -the `//`/`/* */` delimiters still present) or reopens the string-literal false -positive in SQL. +## An in-SQL suppression directive counts only inside a comment + +`analyzer/suppress.go` honors `-- sqlguard:ignore`, `/* sqlguard:ignore:rule-a, +rule-b */` and `# sqlguard:ignore` in SQL, and `// sqlguard:ignore[:rules]` in +Go source. The SQL form used to be a regex anchored on a preceding comment +marker, documented as keeping string literals out. It did not: the marker can +itself be inside the string, so a user-supplied value of +`'-- sqlguard:ignore'` switched every rule off for that statement (#66). + +`parseIgnoreDirective` now lexes the SQL and matches only comment text, skipping +every quoted run and dollar-quoted body whole. Where a comment starts depends on +where each literal ends, which dialects disagree about (backslash escapes, and +whether `$$` opens a string), so it runs every reading and honors a directive +only where all of them put it in a comment. Comment markers vary too (`#` is XOR in +Postgres; MySQL's `--` needs trailing whitespace), so a directive in a comment is +still dropped if the strict markers put it inside a literal. That is the opposite side from +`Redact`'s union, and deliberately: a spurious suppression hides findings, +while a missed one merely reports a finding. + +Flag a change that matches the directive against raw SQL again, drops a +reading, turns the intersection into a union, or reads `//` as a SQL comment. +`ParseIgnoreComment` may use the marker-less token regex because go/ast has +already reduced its input to comment text. `TestParseIgnoreDirective` holds the +literal and dialect-ambiguous cases. ## Redaction is a security invariant, not a formatting choice diff --git a/AGENTS.md b/AGENTS.md index b539872..4fcb336 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -55,7 +55,7 @@ When changing the public API or Go version, update all nine `go.mod` files and ` **`config` is the only YAML-aware package.** It loads `.sqlguard.yml` (`Load`/`Discover` walks up to the git root), translates it to an `analyzer.Profile`, and exposes `MiddlewareOptions()`/`Middleware()` helpers. It depends on `analyzer` (and `middleware` for the helper); nothing depends on `config`. This keeps `gopkg.in/yaml.v3` out of the `analyzer`/`middleware` import graph for library users who don't opt into file config. Parsing is lenient by default (unknown keys/rules warn); `strict: true` makes them fatal — so a newer config still loads on an older binary. -**Suppression has two layers** (`analyzer/suppress.go`): in-SQL `-- sqlguard:ignore` / `/* sqlguard:ignore:rule-a,rule-b */`, parsed from raw SQL with a **marker-anchored** regex (avoids string-literal false positives), honored at runtime _and_ statically; and Go-source `// sqlguard:ignore[:rules]` via `ParseIgnoreComment`, which uses a **separate marker-less** regex because go/ast already strips the `//`. The scanner (`cmd/sqlguard/scan.go`) applies it against the AST comment map for the call line and the line directly above. +**Suppression has two layers** (`analyzer/suppress.go`): in-SQL `-- sqlguard:ignore` / `/* sqlguard:ignore:rule-a,rule-b */` / `# sqlguard:ignore`, honored at runtime _and_ statically; and Go-source `// sqlguard:ignore[:rules]` via `ParseIgnoreComment`. **An in-SQL directive counts only inside a comment span, never inside a literal.** `parseIgnoreDirective` lexes the SQL (`commentDirective`), skipping every quoted run and dollar-quoted body whole, and matches the marker-less `ignoreTokenRe` only against comment text. A regex anchored on a preceding comment marker is not enough — the marker can itself sit inside a string, so `'-- sqlguard:ignore'` in a user-supplied value switched every rule off (#66). Where a comment starts depends on where each literal ends, so this lexer is the third member of the dual-reading family below, and it takes the strictest side: it varies **both** the backslash and the `$$` reading and honors a directive only where **every** reading puts it in a comment (`intersectDirectives`). A spurious suppression hides findings; a missed one only reports a finding, so ambiguity resolves to not suppressing. Comment markers vary as well — `#` is XOR in Postgres and MySQL's `--` needs trailing whitespace — so a directive found in a comment is dropped if the strict markers put it inside a literal (`lexSQL`'s `strict`). `//` is not a SQL comment and is not honored in SQL. `ParseIgnoreComment` shares `ignoreTokenRe` because go/ast has already stripped the `//`, so its input is comment text by construction. The scanner (`cmd/sqlguard/scan.go`) applies it against the AST comment map for the call line and the line directly above. Pinned by `TestParseIgnoreDirective`. **All entry surfaces share the analyzer/reporter.** In-repo: the runtime `database/sql` driver chain (above), the CLI static scanner (`cmd/sqlguard/scan.go`), and the EXPLAIN-plan analyzer (`explain/`). Out-of-tree integrations are additional runtime entry surfaces and go through the same exported `middleware.Guard` (`pgxguard` hooks `pgx.QueryTracer`/`BatchTracer` for native pgx/pgxpool, which never touches `database/sql`). Findings are `analyzer.Result`s emitted through a `reporter.Reporter` (default `reporter.ConsoleReporter`). diff --git a/CHANGELOG.md b/CHANGELOG.md index ce4bfa6..4420ad1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,17 @@ the same version in lockstep. ### Fixed +- **A `sqlguard:ignore` inside a string literal no longer suppresses + anything** ([#66]). The in-SQL directive only had to follow a comment + marker somewhere earlier in the text, and the marker could itself be inside + a string, so `SELECT * FROM users WHERE note = '-- sqlguard:ignore'` + reported nothing. Where a query embeds user input, that let a value switch + every rule off for the statement. The directive is now matched only inside + comment spans. When dialects disagree about where a literal ends + (backslash escapes, `$$`), it counts only if every reading places it in a + comment, and a `#` (XOR in PostgreSQL) or a MySQL `--` without trailing + whitespace cannot reach into a literal. `//` inside SQL text, which was never a SQL comment, is no longer + honored. - **The dialect parsers no longer drop findings on statements they do not model** ([#81]). `pgparser` and `mysqlparser` cleared every structural field before looking at the AST and refilled them only for @@ -53,6 +64,7 @@ the same version in lockstep. `colord` and `serialize-javascript`. Build-time only — nothing here ships to consumers of the Go modules or to readers of the published site. +[#66]: https://github.com/KARTIKrocks/sqlguard/issues/66 [#81]: https://github.com/KARTIKrocks/sqlguard/issues/81 [#82]: https://github.com/KARTIKrocks/sqlguard/issues/82 diff --git a/analyzer/suppress.go b/analyzer/suppress.go index 5502116..d6c0687 100644 --- a/analyzer/suppress.go +++ b/analyzer/suppress.go @@ -5,43 +5,147 @@ import ( "strings" ) -// ignoreDirectiveRe matches a sqlguard:ignore directive inside a SQL or Go -// comment. The leading comment marker (--, /*, #, //) anchors it so the -// token is honored only in comment context, not when the literal text -// happens to appear inside a string. An optional `:rule-a, rule-b` list -// scopes the suppression to specific rules; without it, all rules are -// suppressed for the statement. -var ignoreDirectiveRe = regexp.MustCompile(`(?i)(?:--|/\*|#|//)[^\n]*?sqlguard:ignore(?::\s*([a-z0-9_,\s-]+))?`) - -// ignoreTokenRe matches the bare directive in text that is already known to -// be a comment (e.g. go/ast comment text with the marker stripped). No -// comment marker is required here because the whole string is comment -// context. +// ignoreTokenRe matches a directive in text already known to be a comment. +// An optional `:rule-a, rule-b` list scopes it; without one, every rule is +// suppressed. var ignoreTokenRe = regexp.MustCompile(`(?i)sqlguard:ignore(?::\s*([a-z0-9_,\s-]+))?`) -// parseIgnoreDirective scans raw SQL for `sqlguard:ignore` directives. -// It returns ignoreAll=true if any directive has no rule list, otherwise a -// set of rule names to suppress. The result is empty when no directive is -// present, so the common path allocates nothing. +// parseIgnoreDirective finds `sqlguard:ignore` directives in the comments of +// raw SQL. Text inside a value must never switch a rule off (#66), and +// dialects disagree about where literals and comments start, so a directive +// counts only if every reading agrees it is not inside a literal. func parseIgnoreDirective(sql string) (ignoreAll bool, ignored map[string]bool) { if !strings.Contains(strings.ToLower(sql), "sqlguard:ignore") { return false, nil } - for _, m := range ignoreDirectiveRe.FindAllStringSubmatch(sql, -1) { - list := strings.TrimSpace(m[1]) - if list == "" { - return true, nil + first := true + for _, backslash := range []bool{false, true} { + if backslash && strings.IndexByte(sql, '\\') < 0 { + continue } - if ignored == nil { - ignored = make(map[string]bool) + for _, dollar := range []bool{true, false} { + if !dollar && strings.IndexByte(sql, '$') < 0 { + continue + } + all, rules := commentDirective(sql, backslash, dollar) + if first { + ignoreAll, ignored, first = all, rules, false + continue + } + ignoreAll, ignored = intersectDirectives(ignoreAll, ignored, all, rules) } - for name := range strings.SplitSeq(list, ",") { - if name = strings.TrimSpace(name); name != "" { - ignored[name] = true + } + return ignoreAll, ignored +} + +// commentDirective collects the directives in sql's comments under one +// literal reading. Comments are found with every marker any dialect accepts; +// a directive is then dropped if the strict markers (no #, MySQL's "-- " that +// needs trailing whitespace) put it inside a literal, as with Postgres' +// "a # b" XOR or MySQL's "1--'x'". +func commentDirective(sql string, backslash, dollar bool) (all bool, rules map[string]bool) { + comments, _ := lexSQL(sql, backslash, dollar, false) + _, literals := lexSQL(sql, backslash, dollar, true) + for _, c := range comments { + for _, m := range ignoreTokenRe.FindAllStringSubmatchIndex(sql[c.lo:c.hi], -1) { + if inSpans(literals, c.lo+m[0]) { + continue + } + list := "" + if m[2] >= 0 { + list = strings.TrimSpace(sql[c.lo+m[2] : c.lo+m[3]]) + } + if list == "" { + return true, nil + } + if rules == nil { + rules = make(map[string]bool) + } + for name := range strings.SplitSeq(list, ",") { + if name = strings.TrimSpace(name); name != "" { + rules[name] = true + } + } + } + } + return false, rules +} + +// lexSQL returns the comment and literal spans of sql under one reading. +// strict limits comment markers to those every dialect agrees on. +func lexSQL(sql string, backslash, dollar, strict bool) (comments, literals []span) { + for i := 0; i < len(sql); { + c := sql[i] + switch { + case c == '\'' || c == '"' || c == '`': + j := scanQuotedRun(sql, i, backslash && c != '`') + literals = append(literals, span{i, j}) + i = j + case c == '$' && dollar: + j, _, ok := scanDollarQuoted(sql, i) + if !ok { + i++ + continue + } + literals = append(literals, span{i, j}) + i = j + case isLineComment(sql, i, strict): + j := skipLineComment(sql, i) + comments = append(comments, span{i, j}) + i = j + case c == '/' && i+1 < len(sql) && sql[i+1] == '*': + j := min(skipBlockComment(sql, i), len(sql)) + comments = append(comments, span{i, j}) + i = j + default: + i++ + } + } + return comments, literals +} + +// isLineComment reports whether a -- or # comment starts at sql[i]. Strictly, +// # is not a comment (Postgres XOR) and -- needs a following space or control +// byte (MySQL). +func isLineComment(sql string, i int, strict bool) bool { + if sql[i] == '#' { + return !strict + } + if sql[i] != '-' || i+1 >= len(sql) || sql[i+1] != '-' { + return false + } + return !strict || i+2 >= len(sql) || sql[i+2] <= ' ' +} + +func inSpans(spans []span, pos int) bool { + for _, s := range spans { + if pos >= s.lo && pos < s.hi { + return true + } + } + return false +} + +// intersectDirectives keeps what both readings suppress. +func intersectDirectives(aAll bool, a map[string]bool, bAll bool, b map[string]bool) (bool, map[string]bool) { + switch { + case aAll && bAll: + return true, nil + case aAll: + return false, b + case bAll: + return false, a + } + var out map[string]bool + for name := range a { + if b[name] { + if out == nil { + out = make(map[string]bool) } + out[name] = true } } - return false, ignored + return false, out } // ParseIgnoreComment parses the text of a single comment for a diff --git a/analyzer/suppress_test.go b/analyzer/suppress_test.go new file mode 100644 index 0000000..8184f1f --- /dev/null +++ b/analyzer/suppress_test.go @@ -0,0 +1,74 @@ +package analyzer + +import ( + "maps" + "slices" + "testing" +) + +// TestParseIgnoreDirective: a directive counts only in a comment, never in a +// literal, even one whose end the dialects disagree about (#66). +func TestParseIgnoreDirective(t *testing.T) { + tests := []struct { + name string + sql string + wantAll bool + wantRules []string + }{ + // Genuine directives. + {"line comment", "SELECT * FROM t -- sqlguard:ignore", true, nil}, + {"block comment scoped", "SELECT * FROM t /* sqlguard:ignore:select-star */", false, []string{"select-star"}}, + {"hash comment", "SELECT * FROM t # sqlguard:ignore", true, nil}, + {"case-insensitive", "SELECT * FROM t -- SQLGuard:Ignore", true, nil}, + {"list with spaces", "SELECT * FROM t -- sqlguard:ignore:select-star, orderby-without-limit", false, []string{"orderby-without-limit", "select-star"}}, + {"after a literal", "SELECT * FROM t WHERE a = 'x' -- sqlguard:ignore", true, nil}, + {"after a literal holding a marker", "SELECT * FROM t WHERE a = '--' -- sqlguard:ignore:select-star", false, []string{"select-star"}}, + {"unterminated block comment", "SELECT * FROM t /* sqlguard:ignore", true, nil}, + {"backslash in an unambiguous literal", `SELECT * FROM t WHERE p = 'C:\dir' -- sqlguard:ignore`, true, nil}, + {"dollar in a literal", "SELECT * FROM t WHERE a = '$$' -- sqlguard:ignore", true, nil}, + {"bind placeholder", "SELECT * FROM t WHERE a = $1 -- sqlguard:ignore", true, nil}, + + // Inside a literal: nothing is suppressed. + {"single-quoted line marker", "SELECT * FROM t WHERE note = '-- sqlguard:ignore'", false, nil}, + {"single-quoted block marker", "SELECT * FROM t WHERE note = '/* sqlguard:ignore */'", false, nil}, + {"single-quoted hash marker", "SELECT * FROM t WHERE note = '# sqlguard:ignore'", false, nil}, + {"bare token in a literal", "SELECT * FROM t WHERE note = 'sqlguard:ignore'", false, nil}, + {"doubled-quote escape", "SELECT * FROM t WHERE note = 'it''s -- sqlguard:ignore'", false, nil}, + {"double-quoted", `SELECT * FROM t WHERE note = "-- sqlguard:ignore"`, false, nil}, + {"backtick identifier", "SELECT `-- sqlguard:ignore` FROM t", false, nil}, + {"dollar-quoted", "SELECT * FROM t WHERE note = $$-- sqlguard:ignore$$", false, nil}, + {"tagged dollar-quoted", "SELECT * FROM t WHERE note = $x$ /* sqlguard:ignore */ $x$", false, nil}, + {"unterminated literal", "SELECT * FROM t WHERE note = 'abc -- sqlguard:ignore", false, nil}, + + // Dialect-ambiguous: only one reading puts the marker in a comment. + {"backslash-escaped quote", `SELECT * FROM t WHERE note = 'x\' -- sqlguard:ignore'`, false, nil}, + {"dollar reading", "SELECT * FROM t WHERE a = $$'$$ -- sqlguard:ignore'", false, nil}, + + // Markers only some dialects accept: a genuine comment still counts, + // but a marker that is an operator elsewhere cannot reach into a literal. + {"hash before a literal", "SELECT * FROM t WHERE flags # 4 = 0 AND note = 'sqlguard:ignore'", false, nil}, + {"hash before a marker literal", "SELECT * FROM t WHERE flags # 4 = 0 AND note = '-- sqlguard:ignore'", false, nil}, + {"mysql double minus", "SELECT * FROM t WHERE a = 1--'-- sqlguard:ignore'", false, nil}, + {"tight double dash", "SELECT * FROM t --sqlguard:ignore", true, nil}, + + // "//" is not a SQL comment in any dialect. + {"double slash", "SELECT * FROM t // sqlguard:ignore", false, nil}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + all, rules := parseIgnoreDirective(tt.sql) + got := slices.Sorted(maps.Keys(rules)) + if all != tt.wantAll || !slices.Equal(got, tt.wantRules) { + t.Errorf("parseIgnoreDirective(%q) = (%v, %v), want (%v, %v)", tt.sql, all, got, tt.wantAll, tt.wantRules) + } + }) + } +} + +// TestIgnoreDirectiveInLiteralDoesNotSuppress is the #66 reproduction. +func TestIgnoreDirectiveInLiteralDoesNotSuppress(t *testing.T) { + q := "SELECT * FROM users WHERE note = '-- sqlguard:ignore'" + if got := filterByRule(Default().Analyze(q), "select-star"); got != 1 { + t.Errorf("select-star reported %d times on %q, want 1", got, q) + } +} diff --git a/website/docs/suppressions.md b/website/docs/suppressions.md index 56f7bee..2d727b5 100644 --- a/website/docs/suppressions.md +++ b/website/docs/suppressions.md @@ -34,10 +34,21 @@ query builder untouched. ### Why it must be in a comment -The directive is matched only when it follows a comment marker. A string -literal that happens to contain the words — say a support ticket body with -`'-- sqlguard:ignore'` in it — does not suppress anything. This is a -deliberate anchoring, not an accident of the regex. +_Changed in 0.6._ The directive is honored only inside a SQL comment. Text +inside a string literal, a quoted identifier or a dollar-quoted body is +never read as one, so a value that happens to contain the words — a support +ticket body with `'-- sqlguard:ignore'` in it, or user input concatenated +into the query — suppresses nothing. Before 0.6 the directive only had to +follow a comment marker, and that marker could itself be inside the string. + +Databases disagree about where some literals end, and where comments +start: whether `\'` escapes a quote, whether `$$` opens a string, whether +`#` is a comment (it is XOR in PostgreSQL), and whether `--` needs a space +after it (it does in MySQL). When the answer decides whether a +directive is in a comment, sqlguard does not suppress. The cost is at most a +finding a genuine but ambiguous directive failed to silence; the alternative +would let text inside a value switch rules off. `//` is not a SQL comment, +so `// sqlguard:ignore` inside SQL text is not honored either. ## In the Go source From bd08614c59e628011671c4b2893cb3df90cbd777 Mon Sep 17 00:00:00 2001 From: kartik Date: Sat, 26 Sep 2026 06:13:10 +0530 Subject: [PATCH 2/3] fix(analyzer): check every comment-marker combination for ignore directives Comparing only "all markers on" against "all markers off" missed MySQL's actual mix, where # is a comment but --x is not. In 1 # '\n--x' -- sqlguard:ignore ' the directive sits inside a MySQL string yet still suppressed every rule. A directive found in a comment is now dropped if any combination of the # and -- readings puts it inside a literal. --- .codeant/review.json | 2 +- .greptile/config.json | 2 +- .greptile/rules.md | 2 +- AGENTS.md | 2 +- analyzer/suppress.go | 46 ++++++++++++++++++++++----------------- analyzer/suppress_test.go | 2 ++ 6 files changed, 32 insertions(+), 24 deletions(-) diff --git a/.codeant/review.json b/.codeant/review.json index f37f518..a66ba64 100644 --- a/.codeant/review.json +++ b/.codeant/review.json @@ -20,7 +20,7 @@ }, { "id": "suppression-comment-only", - "description": "An in-SQL sqlguard:ignore directive (-- , /* */ or #) must be honored only inside a comment span, never inside a string literal, quoted identifier or dollar-quoted body. parseIgnoreDirective lexes the SQL and matches ignoreTokenRe only against comment text; matching raw SQL with a regex anchored on a comment marker is not enough, because the marker can itself sit inside a string - a user-supplied value of '-- sqlguard:ignore' switched every rule off (#66). Where a comment starts depends on where each literal ends, so it runs every dialect reading (backslash escapes on/off, $$ as a delimiter or not) and honors a directive only where ALL readings put it in a comment (intersectDirectives): a spurious suppression hides findings, a missed one only reports a finding. Comment markers are dialect-dependent too (# is XOR in Postgres; MySQL's -- needs trailing whitespace), so a directive found in a comment is still dropped if the strict markers put it inside a literal. This is the opposite side from Redact's union, on purpose. Flag a change that matches the directive against raw SQL again, drops a reading, unions the readings, or treats // as a SQL comment. ParseIgnoreComment (Go source) may use the marker-less regex because go/ast has already stripped the //. TestParseIgnoreDirective pins the literal and dialect-ambiguous cases.", + "description": "An in-SQL sqlguard:ignore directive (-- , /* */ or #) must be honored only inside a comment span, never inside a string literal, quoted identifier or dollar-quoted body. parseIgnoreDirective lexes the SQL and matches ignoreTokenRe only against comment text; matching raw SQL with a regex anchored on a comment marker is not enough, because the marker can itself sit inside a string - a user-supplied value of '-- sqlguard:ignore' switched every rule off (#66). Where a comment starts depends on where each literal ends, so it runs every dialect reading (backslash escapes on/off, $$ as a delimiter or not) and honors a directive only where ALL readings put it in a comment (intersectDirectives): a spurious suppression hides findings, a missed one only reports a finding. Comment markers are dialect-dependent too (# is XOR in Postgres; MySQL's -- needs trailing whitespace), so a directive found in a comment is still dropped if any combination of those markers puts it inside a literal. This is the opposite side from Redact's union, on purpose. Flag a change that matches the directive against raw SQL again, drops a reading, unions the readings, or treats // as a SQL comment. ParseIgnoreComment (Go source) may use the marker-less regex because go/ast has already stripped the //. TestParseIgnoreDirective pins the literal and dialect-ambiguous cases.", "files": ["analyzer/suppress.go", "analyzer/fallback.go", "analyzer/redact.go", "analyzer/*_test.go"], "scope": ["pr", "ide"] }, diff --git a/.greptile/config.json b/.greptile/config.json index fc20403..f76ae00 100644 --- a/.greptile/config.json +++ b/.greptile/config.json @@ -26,7 +26,7 @@ }, { "id": "suppression-comment-only", - "rule": "An in-SQL sqlguard:ignore directive (-- , /* */ or #) must be honored only inside a comment span, never inside a string literal, quoted identifier or dollar-quoted body. parseIgnoreDirective lexes the SQL and matches ignoreTokenRe only against comment text; matching raw SQL with a regex anchored on a comment marker is not enough, because the marker can itself sit inside a string - a user-supplied value of '-- sqlguard:ignore' switched every rule off (#66). Where a comment starts depends on where each literal ends, so it runs every dialect reading (backslash escapes on/off, $$ as a delimiter or not) and honors a directive only where ALL readings put it in a comment (intersectDirectives): a spurious suppression hides findings, a missed one only reports a finding. Comment markers are dialect-dependent too (# is XOR in Postgres; MySQL's -- needs trailing whitespace), so a directive found in a comment is still dropped if the strict markers put it inside a literal. This is the opposite side from Redact's union, on purpose. Flag a change that matches the directive against raw SQL again, drops a reading, unions the readings, or treats // as a SQL comment. ParseIgnoreComment (Go source) may use the marker-less regex because go/ast has already stripped the //. TestParseIgnoreDirective pins the literal and dialect-ambiguous cases.", + "rule": "An in-SQL sqlguard:ignore directive (-- , /* */ or #) must be honored only inside a comment span, never inside a string literal, quoted identifier or dollar-quoted body. parseIgnoreDirective lexes the SQL and matches ignoreTokenRe only against comment text; matching raw SQL with a regex anchored on a comment marker is not enough, because the marker can itself sit inside a string - a user-supplied value of '-- sqlguard:ignore' switched every rule off (#66). Where a comment starts depends on where each literal ends, so it runs every dialect reading (backslash escapes on/off, $$ as a delimiter or not) and honors a directive only where ALL readings put it in a comment (intersectDirectives): a spurious suppression hides findings, a missed one only reports a finding. Comment markers are dialect-dependent too (# is XOR in Postgres; MySQL's -- needs trailing whitespace), so a directive found in a comment is still dropped if any combination of those markers puts it inside a literal. This is the opposite side from Redact's union, on purpose. Flag a change that matches the directive against raw SQL again, drops a reading, unions the readings, or treats // as a SQL comment. ParseIgnoreComment (Go source) may use the marker-less regex because go/ast has already stripped the //. TestParseIgnoreDirective pins the literal and dialect-ambiguous cases.", "scope": ["analyzer/suppress.go", "analyzer/fallback.go", "analyzer/redact.go"], "severity": "high" }, diff --git a/.greptile/rules.md b/.greptile/rules.md index 36ed6e9..b653fb0 100644 --- a/.greptile/rules.md +++ b/.greptile/rules.md @@ -52,7 +52,7 @@ where each literal ends, which dialects disagree about (backslash escapes, and whether `$$` opens a string), so it runs every reading and honors a directive only where all of them put it in a comment. Comment markers vary too (`#` is XOR in Postgres; MySQL's `--` needs trailing whitespace), so a directive in a comment is -still dropped if the strict markers put it inside a literal. That is the opposite side from +still dropped if any combination of those markers puts it inside a literal. That is the opposite side from `Redact`'s union, and deliberately: a spurious suppression hides findings, while a missed one merely reports a finding. diff --git a/AGENTS.md b/AGENTS.md index 4fcb336..4143f6c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -55,7 +55,7 @@ When changing the public API or Go version, update all nine `go.mod` files and ` **`config` is the only YAML-aware package.** It loads `.sqlguard.yml` (`Load`/`Discover` walks up to the git root), translates it to an `analyzer.Profile`, and exposes `MiddlewareOptions()`/`Middleware()` helpers. It depends on `analyzer` (and `middleware` for the helper); nothing depends on `config`. This keeps `gopkg.in/yaml.v3` out of the `analyzer`/`middleware` import graph for library users who don't opt into file config. Parsing is lenient by default (unknown keys/rules warn); `strict: true` makes them fatal — so a newer config still loads on an older binary. -**Suppression has two layers** (`analyzer/suppress.go`): in-SQL `-- sqlguard:ignore` / `/* sqlguard:ignore:rule-a,rule-b */` / `# sqlguard:ignore`, honored at runtime _and_ statically; and Go-source `// sqlguard:ignore[:rules]` via `ParseIgnoreComment`. **An in-SQL directive counts only inside a comment span, never inside a literal.** `parseIgnoreDirective` lexes the SQL (`commentDirective`), skipping every quoted run and dollar-quoted body whole, and matches the marker-less `ignoreTokenRe` only against comment text. A regex anchored on a preceding comment marker is not enough — the marker can itself sit inside a string, so `'-- sqlguard:ignore'` in a user-supplied value switched every rule off (#66). Where a comment starts depends on where each literal ends, so this lexer is the third member of the dual-reading family below, and it takes the strictest side: it varies **both** the backslash and the `$$` reading and honors a directive only where **every** reading puts it in a comment (`intersectDirectives`). A spurious suppression hides findings; a missed one only reports a finding, so ambiguity resolves to not suppressing. Comment markers vary as well — `#` is XOR in Postgres and MySQL's `--` needs trailing whitespace — so a directive found in a comment is dropped if the strict markers put it inside a literal (`lexSQL`'s `strict`). `//` is not a SQL comment and is not honored in SQL. `ParseIgnoreComment` shares `ignoreTokenRe` because go/ast has already stripped the `//`, so its input is comment text by construction. The scanner (`cmd/sqlguard/scan.go`) applies it against the AST comment map for the call line and the line directly above. Pinned by `TestParseIgnoreDirective`. +**Suppression has two layers** (`analyzer/suppress.go`): in-SQL `-- sqlguard:ignore` / `/* sqlguard:ignore:rule-a,rule-b */` / `# sqlguard:ignore`, honored at runtime _and_ statically; and Go-source `// sqlguard:ignore[:rules]` via `ParseIgnoreComment`. **An in-SQL directive counts only inside a comment span, never inside a literal.** `parseIgnoreDirective` lexes the SQL (`commentDirective`), skipping every quoted run and dollar-quoted body whole, and matches the marker-less `ignoreTokenRe` only against comment text. A regex anchored on a preceding comment marker is not enough — the marker can itself sit inside a string, so `'-- sqlguard:ignore'` in a user-supplied value switched every rule off (#66). Where a comment starts depends on where each literal ends, so this lexer is the third member of the dual-reading family below, and it takes the strictest side: it varies **both** the backslash and the `$$` reading and honors a directive only where **every** reading puts it in a comment (`intersectDirectives`). A spurious suppression hides findings; a missed one only reports a finding, so ambiguity resolves to not suppressing. Comment markers vary as well — `#` is XOR in Postgres and MySQL's `--` needs trailing whitespace — so a directive found in a comment is dropped if any combination of those markers puts it inside a literal (`commentDirective`). `//` is not a SQL comment and is not honored in SQL. `ParseIgnoreComment` shares `ignoreTokenRe` because go/ast has already stripped the `//`, so its input is comment text by construction. The scanner (`cmd/sqlguard/scan.go`) applies it against the AST comment map for the call line and the line directly above. Pinned by `TestParseIgnoreDirective`. **All entry surfaces share the analyzer/reporter.** In-repo: the runtime `database/sql` driver chain (above), the CLI static scanner (`cmd/sqlguard/scan.go`), and the EXPLAIN-plan analyzer (`explain/`). Out-of-tree integrations are additional runtime entry surfaces and go through the same exported `middleware.Guard` (`pgxguard` hooks `pgx.QueryTracer`/`BatchTracer` for native pgx/pgxpool, which never touches `database/sql`). Findings are `analyzer.Result`s emitted through a `reporter.Reporter` (default `reporter.ConsoleReporter`). diff --git a/analyzer/suppress.go b/analyzer/suppress.go index d6c0687..71d54da 100644 --- a/analyzer/suppress.go +++ b/analyzer/suppress.go @@ -40,15 +40,21 @@ func parseIgnoreDirective(sql string) (ignoreAll bool, ignored map[string]bool) // commentDirective collects the directives in sql's comments under one // literal reading. Comments are found with every marker any dialect accepts; -// a directive is then dropped if the strict markers (no #, MySQL's "-- " that -// needs trailing whitespace) put it inside a literal, as with Postgres' -// "a # b" XOR or MySQL's "1--'x'". +// a directive is dropped if any combination of the dialect-dependent markers +// (# is XOR in Postgres; MySQL's -- needs trailing whitespace) puts it inside +// a literal. func commentDirective(sql string, backslash, dollar bool) (all bool, rules map[string]bool) { - comments, _ := lexSQL(sql, backslash, dollar, false) - _, literals := lexSQL(sql, backslash, dollar, true) + comments, _ := lexSQL(sql, backslash, dollar, true, true) + literals := make([][]span, 0, 4) + for _, hash := range []bool{true, false} { + for _, looseDash := range []bool{true, false} { + _, lits := lexSQL(sql, backslash, dollar, hash, looseDash) + literals = append(literals, lits) + } + } for _, c := range comments { for _, m := range ignoreTokenRe.FindAllStringSubmatchIndex(sql[c.lo:c.hi], -1) { - if inSpans(literals, c.lo+m[0]) { + if inAnySpans(literals, c.lo+m[0]) { continue } list := "" @@ -71,9 +77,9 @@ func commentDirective(sql string, backslash, dollar bool) (all bool, rules map[s return false, rules } -// lexSQL returns the comment and literal spans of sql under one reading. -// strict limits comment markers to those every dialect agrees on. -func lexSQL(sql string, backslash, dollar, strict bool) (comments, literals []span) { +// lexSQL returns the comment and literal spans of sql under one reading. hash +// makes # a comment; looseDash makes -- one without trailing whitespace. +func lexSQL(sql string, backslash, dollar, hash, looseDash bool) (comments, literals []span) { for i := 0; i < len(sql); { c := sql[i] switch { @@ -89,7 +95,7 @@ func lexSQL(sql string, backslash, dollar, strict bool) (comments, literals []sp } literals = append(literals, span{i, j}) i = j - case isLineComment(sql, i, strict): + case isLineComment(sql, i, hash, looseDash): j := skipLineComment(sql, i) comments = append(comments, span{i, j}) i = j @@ -104,23 +110,23 @@ func lexSQL(sql string, backslash, dollar, strict bool) (comments, literals []sp return comments, literals } -// isLineComment reports whether a -- or # comment starts at sql[i]. Strictly, -// # is not a comment (Postgres XOR) and -- needs a following space or control -// byte (MySQL). -func isLineComment(sql string, i int, strict bool) bool { +// isLineComment reports whether a -- or # comment starts at sql[i]. +func isLineComment(sql string, i int, hash, looseDash bool) bool { if sql[i] == '#' { - return !strict + return hash } if sql[i] != '-' || i+1 >= len(sql) || sql[i+1] != '-' { return false } - return !strict || i+2 >= len(sql) || sql[i+2] <= ' ' + return looseDash || i+2 >= len(sql) || sql[i+2] <= ' ' } -func inSpans(spans []span, pos int) bool { - for _, s := range spans { - if pos >= s.lo && pos < s.hi { - return true +func inAnySpans(readings [][]span, pos int) bool { + for _, spans := range readings { + for _, s := range spans { + if pos >= s.lo && pos < s.hi { + return true + } } } return false diff --git a/analyzer/suppress_test.go b/analyzer/suppress_test.go index 8184f1f..b5ec6f3 100644 --- a/analyzer/suppress_test.go +++ b/analyzer/suppress_test.go @@ -50,6 +50,8 @@ func TestParseIgnoreDirective(t *testing.T) { {"hash before a marker literal", "SELECT * FROM t WHERE flags # 4 = 0 AND note = '-- sqlguard:ignore'", false, nil}, {"mysql double minus", "SELECT * FROM t WHERE a = 1--'-- sqlguard:ignore'", false, nil}, {"tight double dash", "SELECT * FROM t --sqlguard:ignore", true, nil}, + // MySQL's own mix: # is a comment, --x is not, so a string opens after it. + {"mysql marker mix", "SELECT * FROM t WHERE a = 1 # '\n--x' -- sqlguard:ignore '", false, nil}, // "//" is not a SQL comment in any dialect. {"double slash", "SELECT * FROM t // sqlguard:ignore", false, nil}, From d0a4b9ef3faea53037f53dbe8a248ff0b8694c1c Mon Sep 17 00:00:00 2001 From: kartik Date: Sat, 26 Sep 2026 06:17:48 +0530 Subject: [PATCH 3/3] fix(analyzer): check for an ignore directive without allocating parseIgnoreDirective runs on every analyzed query, and its pre-check lowercased the whole query, allocating whenever it held an uppercase byte. containsFold does the same case-insensitive search in place. Also make the reviewer rules say which readings must all agree: the backslash and $$ literal readings. The # and -- markers are handled by dropping a directive a marker combination puts in a literal, so a genuine # sqlguard:ignore still counts. --- .codeant/review.json | 2 +- .coderabbit.yaml | 7 ++++--- .greptile/config.json | 2 +- AGENTS.md | 2 +- analyzer/suppress.go | 13 ++++++++++++- analyzer/suppress_test.go | 7 +++++++ 6 files changed, 26 insertions(+), 7 deletions(-) diff --git a/.codeant/review.json b/.codeant/review.json index a66ba64..90c0c60 100644 --- a/.codeant/review.json +++ b/.codeant/review.json @@ -20,7 +20,7 @@ }, { "id": "suppression-comment-only", - "description": "An in-SQL sqlguard:ignore directive (-- , /* */ or #) must be honored only inside a comment span, never inside a string literal, quoted identifier or dollar-quoted body. parseIgnoreDirective lexes the SQL and matches ignoreTokenRe only against comment text; matching raw SQL with a regex anchored on a comment marker is not enough, because the marker can itself sit inside a string - a user-supplied value of '-- sqlguard:ignore' switched every rule off (#66). Where a comment starts depends on where each literal ends, so it runs every dialect reading (backslash escapes on/off, $$ as a delimiter or not) and honors a directive only where ALL readings put it in a comment (intersectDirectives): a spurious suppression hides findings, a missed one only reports a finding. Comment markers are dialect-dependent too (# is XOR in Postgres; MySQL's -- needs trailing whitespace), so a directive found in a comment is still dropped if any combination of those markers puts it inside a literal. This is the opposite side from Redact's union, on purpose. Flag a change that matches the directive against raw SQL again, drops a reading, unions the readings, or treats // as a SQL comment. ParseIgnoreComment (Go source) may use the marker-less regex because go/ast has already stripped the //. TestParseIgnoreDirective pins the literal and dialect-ambiguous cases.", + "description": "An in-SQL sqlguard:ignore directive (-- , /* */ or #) must be honored only inside a comment span, never inside a string literal, quoted identifier or dollar-quoted body. parseIgnoreDirective lexes the SQL and matches ignoreTokenRe only against comment text; matching raw SQL with a regex anchored on a comment marker is not enough, because the marker can itself sit inside a string - a user-supplied value of '-- sqlguard:ignore' switched every rule off (#66). Where a comment starts depends on where each literal ends, so it runs every dialect reading (backslash escapes on/off, $$ as a delimiter or not) and honors a directive only where ALL of those literal readings put it in a comment (intersectDirectives): a spurious suppression hides findings, a missed one only reports a finding. Comment markers are dialect-dependent too (# is XOR in Postgres; MySQL's -- needs trailing whitespace), so comments are found with every marker (a genuine # sqlguard:ignore counts) and a directive is dropped only if some marker combination puts it inside a literal; do not require # to be a comment in every dialect. This is the opposite side from Redact's union, on purpose. Flag a change that matches the directive against raw SQL again, drops a reading, unions the readings, or treats // as a SQL comment. ParseIgnoreComment (Go source) may use the marker-less regex because go/ast has already stripped the //. TestParseIgnoreDirective pins the literal and dialect-ambiguous cases.", "files": ["analyzer/suppress.go", "analyzer/fallback.go", "analyzer/redact.go", "analyzer/*_test.go"], "scope": ["pr", "ide"] }, diff --git a/.coderabbit.yaml b/.coderabbit.yaml index f3889ad..7c2bc98 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -94,10 +94,11 @@ reviews: parseIgnoreDirective lexes the SQL and matches the directive only in comment text, because a regex anchored on a comment marker also matched '-- sqlguard:ignore' inside a user-supplied string and switched every - rule off (#66). It runs every dialect reading and honors a directive only - where ALL agree it is in a comment — a spurious suppression hides + rule off (#66). It runs every backslash/$$ literal reading and honors a + directive only where ALL agree it is in a comment — a spurious suppression hides findings, a missed one only reports one. # (XOR in Postgres) and a - MySQL -- without trailing whitespace must not reach into a literal. + MySQL -- without trailing whitespace must not reach into a literal, but + a genuine # sqlguard:ignore comment still counts. Flag a change that matches raw SQL again, drops a reading, unions them, or treats // as a SQL comment. diff --git a/.greptile/config.json b/.greptile/config.json index f76ae00..7bb26f4 100644 --- a/.greptile/config.json +++ b/.greptile/config.json @@ -26,7 +26,7 @@ }, { "id": "suppression-comment-only", - "rule": "An in-SQL sqlguard:ignore directive (-- , /* */ or #) must be honored only inside a comment span, never inside a string literal, quoted identifier or dollar-quoted body. parseIgnoreDirective lexes the SQL and matches ignoreTokenRe only against comment text; matching raw SQL with a regex anchored on a comment marker is not enough, because the marker can itself sit inside a string - a user-supplied value of '-- sqlguard:ignore' switched every rule off (#66). Where a comment starts depends on where each literal ends, so it runs every dialect reading (backslash escapes on/off, $$ as a delimiter or not) and honors a directive only where ALL readings put it in a comment (intersectDirectives): a spurious suppression hides findings, a missed one only reports a finding. Comment markers are dialect-dependent too (# is XOR in Postgres; MySQL's -- needs trailing whitespace), so a directive found in a comment is still dropped if any combination of those markers puts it inside a literal. This is the opposite side from Redact's union, on purpose. Flag a change that matches the directive against raw SQL again, drops a reading, unions the readings, or treats // as a SQL comment. ParseIgnoreComment (Go source) may use the marker-less regex because go/ast has already stripped the //. TestParseIgnoreDirective pins the literal and dialect-ambiguous cases.", + "rule": "An in-SQL sqlguard:ignore directive (-- , /* */ or #) must be honored only inside a comment span, never inside a string literal, quoted identifier or dollar-quoted body. parseIgnoreDirective lexes the SQL and matches ignoreTokenRe only against comment text; matching raw SQL with a regex anchored on a comment marker is not enough, because the marker can itself sit inside a string - a user-supplied value of '-- sqlguard:ignore' switched every rule off (#66). Where a comment starts depends on where each literal ends, so it runs every dialect reading (backslash escapes on/off, $$ as a delimiter or not) and honors a directive only where ALL of those literal readings put it in a comment (intersectDirectives): a spurious suppression hides findings, a missed one only reports a finding. Comment markers are dialect-dependent too (# is XOR in Postgres; MySQL's -- needs trailing whitespace), so comments are found with every marker (a genuine # sqlguard:ignore counts) and a directive is dropped only if some marker combination puts it inside a literal; do not require # to be a comment in every dialect. This is the opposite side from Redact's union, on purpose. Flag a change that matches the directive against raw SQL again, drops a reading, unions the readings, or treats // as a SQL comment. ParseIgnoreComment (Go source) may use the marker-less regex because go/ast has already stripped the //. TestParseIgnoreDirective pins the literal and dialect-ambiguous cases.", "scope": ["analyzer/suppress.go", "analyzer/fallback.go", "analyzer/redact.go"], "severity": "high" }, diff --git a/AGENTS.md b/AGENTS.md index 4143f6c..0135f7a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -55,7 +55,7 @@ When changing the public API or Go version, update all nine `go.mod` files and ` **`config` is the only YAML-aware package.** It loads `.sqlguard.yml` (`Load`/`Discover` walks up to the git root), translates it to an `analyzer.Profile`, and exposes `MiddlewareOptions()`/`Middleware()` helpers. It depends on `analyzer` (and `middleware` for the helper); nothing depends on `config`. This keeps `gopkg.in/yaml.v3` out of the `analyzer`/`middleware` import graph for library users who don't opt into file config. Parsing is lenient by default (unknown keys/rules warn); `strict: true` makes them fatal — so a newer config still loads on an older binary. -**Suppression has two layers** (`analyzer/suppress.go`): in-SQL `-- sqlguard:ignore` / `/* sqlguard:ignore:rule-a,rule-b */` / `# sqlguard:ignore`, honored at runtime _and_ statically; and Go-source `// sqlguard:ignore[:rules]` via `ParseIgnoreComment`. **An in-SQL directive counts only inside a comment span, never inside a literal.** `parseIgnoreDirective` lexes the SQL (`commentDirective`), skipping every quoted run and dollar-quoted body whole, and matches the marker-less `ignoreTokenRe` only against comment text. A regex anchored on a preceding comment marker is not enough — the marker can itself sit inside a string, so `'-- sqlguard:ignore'` in a user-supplied value switched every rule off (#66). Where a comment starts depends on where each literal ends, so this lexer is the third member of the dual-reading family below, and it takes the strictest side: it varies **both** the backslash and the `$$` reading and honors a directive only where **every** reading puts it in a comment (`intersectDirectives`). A spurious suppression hides findings; a missed one only reports a finding, so ambiguity resolves to not suppressing. Comment markers vary as well — `#` is XOR in Postgres and MySQL's `--` needs trailing whitespace — so a directive found in a comment is dropped if any combination of those markers puts it inside a literal (`commentDirective`). `//` is not a SQL comment and is not honored in SQL. `ParseIgnoreComment` shares `ignoreTokenRe` because go/ast has already stripped the `//`, so its input is comment text by construction. The scanner (`cmd/sqlguard/scan.go`) applies it against the AST comment map for the call line and the line directly above. Pinned by `TestParseIgnoreDirective`. +**Suppression has two layers** (`analyzer/suppress.go`): in-SQL `-- sqlguard:ignore` / `/* sqlguard:ignore:rule-a,rule-b */` / `# sqlguard:ignore`, honored at runtime _and_ statically; and Go-source `// sqlguard:ignore[:rules]` via `ParseIgnoreComment`. **An in-SQL directive counts only inside a comment span, never inside a literal.** `parseIgnoreDirective` lexes the SQL (`commentDirective`), skipping every quoted run and dollar-quoted body whole, and matches the marker-less `ignoreTokenRe` only against comment text. A regex anchored on a preceding comment marker is not enough — the marker can itself sit inside a string, so `'-- sqlguard:ignore'` in a user-supplied value switched every rule off (#66). Where a comment starts depends on where each literal ends, so this lexer is the third member of the dual-reading family below, and it takes the strictest side: it varies **both** the backslash and the `$$` reading and honors a directive only where **every** such literal reading puts it in a comment (`intersectDirectives`). A spurious suppression hides findings; a missed one only reports a finding, so ambiguity resolves to not suppressing. Comment markers vary as well — `#` is XOR in Postgres and MySQL's `--` needs trailing whitespace — so comments are found with every marker (a genuine `# sqlguard:ignore` counts) and a directive is dropped only if some marker combination puts it inside a literal (`commentDirective`). `//` is not a SQL comment and is not honored in SQL. `ParseIgnoreComment` shares `ignoreTokenRe` because go/ast has already stripped the `//`, so its input is comment text by construction. The scanner (`cmd/sqlguard/scan.go`) applies it against the AST comment map for the call line and the line directly above. Pinned by `TestParseIgnoreDirective`. **All entry surfaces share the analyzer/reporter.** In-repo: the runtime `database/sql` driver chain (above), the CLI static scanner (`cmd/sqlguard/scan.go`), and the EXPLAIN-plan analyzer (`explain/`). Out-of-tree integrations are additional runtime entry surfaces and go through the same exported `middleware.Guard` (`pgxguard` hooks `pgx.QueryTracer`/`BatchTracer` for native pgx/pgxpool, which never touches `database/sql`). Findings are `analyzer.Result`s emitted through a `reporter.Reporter` (default `reporter.ConsoleReporter`). diff --git a/analyzer/suppress.go b/analyzer/suppress.go index 71d54da..68d2f91 100644 --- a/analyzer/suppress.go +++ b/analyzer/suppress.go @@ -15,7 +15,7 @@ var ignoreTokenRe = regexp.MustCompile(`(?i)sqlguard:ignore(?::\s*([a-z0-9_,\s-] // dialects disagree about where literals and comments start, so a directive // counts only if every reading agrees it is not inside a literal. func parseIgnoreDirective(sql string) (ignoreAll bool, ignored map[string]bool) { - if !strings.Contains(strings.ToLower(sql), "sqlguard:ignore") { + if !containsFold(sql, "sqlguard:ignore") { return false, nil } first := true @@ -177,3 +177,14 @@ func ParseIgnoreComment(text string) (all bool, rules map[string]bool, found boo } return false, rules, true } + +// containsFold is a case-insensitive strings.Contains for an ASCII needle that +// does not allocate; it runs on every analyzed query. +func containsFold(s, needle string) bool { + for i := 0; i+len(needle) <= len(s); i++ { + if s[i]|0x20 == needle[0] && strings.EqualFold(s[i:i+len(needle)], needle) { + return true + } + } + return false +} diff --git a/analyzer/suppress_test.go b/analyzer/suppress_test.go index b5ec6f3..f494dd0 100644 --- a/analyzer/suppress_test.go +++ b/analyzer/suppress_test.go @@ -74,3 +74,10 @@ func TestIgnoreDirectiveInLiteralDoesNotSuppress(t *testing.T) { t.Errorf("select-star reported %d times on %q, want 1", got, q) } } + +func TestParseIgnoreDirectiveNoDirectiveDoesNotAllocate(t *testing.T) { + q := "SELECT * FROM users WHERE id = 1 AND name = 'Alice'" + if n := testing.AllocsPerRun(100, func() { parseIgnoreDirective(q) }); n != 0 { + t.Errorf("parseIgnoreDirective allocated %v times on a query without a directive", n) + } +}