fix(analyzer): honor sqlguard:ignore only inside SQL comments

.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 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"]
+    },
     {
       "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.",

.coderabbit.yaml

@@ -90,6 +90,17 @@ 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 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, 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.
 
     - path: "middleware/**"
       instructions: >-

.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 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"
+    },
     {
       "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.",

.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 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.
+
+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
 

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** 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`).
 

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
 

analyzer/suppress.go

@@ -5,43 +5,153 @@ 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") {
+	if !containsFold(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
+		}
+		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)
+		}
+	}
+	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 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, 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 inAnySpans(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. 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 {
+		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, hash, looseDash):
+			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++
 		}
-		if ignored == nil {
-			ignored = make(map[string]bool)
+	}
+	return comments, literals
+}
+
+// isLineComment reports whether a -- or # comment starts at sql[i].
+func isLineComment(sql string, i int, hash, looseDash bool) bool {
+	if sql[i] == '#' {
+		return hash
+	}
+	if sql[i] != '-' || i+1 >= len(sql) || sql[i+1] != '-' {
+		return false
+	}
+	return looseDash || i+2 >= len(sql) || sql[i+2] <= ' '
+}
+
+func inAnySpans(readings [][]span, pos int) bool {
+	for _, spans := range readings {
+		for _, s := range spans {
+			if pos >= s.lo && pos < s.hi {
+				return true
+			}
 		}
-		for name := range strings.SplitSeq(list, ",") {
-			if name = strings.TrimSpace(name); name != "" {
-				ignored[name] = 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
@@ -67,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
+}