From 39185bd65558e8ad552b7c214aec93e5116d9188 Mon Sep 17 00:00:00 2001 From: kartik Date: Fri, 25 Sep 2026 13:36:04 +0530 Subject: [PATCH 1/4] fix: stop pgparser flagging INSERT ... DEFAULT VALUES (#68) pgparser blanks the eight structural fields before the AST switch and refills InsertColumnsListed from len(n.Columns) alone. DEFAULT VALUES names no columns and needs none -- it inserts no caller-supplied data, so there is no column order for a schema change to shift -- which the zero-dependency fallback handles explicitly. The AST path discarded that and then set Exact = true over it, so opting into the exact parser made insert-without-columns strictly worse than the default, inverting the trade-off parsers.md describes. The grammar encodes the form as an absent row source, so read it back the same way. tree's own Insert.DefaultValues dereferences Rows unguarded, hence the local helper. Pin the class of bug rather than the instance: both parser modules now run a corpus through the dialect grammar and the fallback and assert the grammar never reports a rule the fallback does not. Opting into a real parser may only remove findings. That test immediately found a second instance. vitess parses MySQL/SQLite REPLACE into the same node as INSERT, so mysqlparser reported insert-without-columns on "REPLACE INTO t VALUES (1)" while the fallback read it as an unrecognized kind and said nothing. Here the grammar is the correct one -- REPLACE binds positionally exactly as INSERT does and carries the same risk -- so teach the fallback the statement instead of silencing the parser. INTO is required in the pattern so REPLACE(str, a, b) is never read as a statement; the INTO-less dialect form is one insertColumnsListed already declines to flag. Both fixes verified by reverting them and watching the parity test fail. Note the REPLACE half needs core and mysqlparser released in lockstep: under GOWORK=off that module builds against the core its go.mod requires (v0.4.0), which predates the fallback change, so the REPLACE case fails there until the release pins the pair. CI compiles satellites against this tree via go.work and is unaffected. The broader half of #68 -- the reset-then-partially-refill shape that leaves non-DML AST nodes with Exact = true over blanked structural fields -- is deliberately left open. It only produces false negatives today, so the parity test passes over it, and fixing it properly means deciding what Exact should claim when the fallback's heuristics survive. --- CHANGELOG.md | 29 ++++++++++ analyzer/analyzer_test.go | 8 +++ analyzer/fallback.go | 12 +++- parsers/mysqlparser/mysqlparser_test.go | 72 +++++++++++++++++++++++ parsers/pgparser/pgparser.go | 11 +++- parsers/pgparser/pgparser_test.go | 77 +++++++++++++++++++++++++ website/docs/parsers.md | 9 +++ website/docs/rules.md | 8 ++- 8 files changed, 223 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dfe8143..415c326 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,35 @@ the same version in lockstep. ## [Unreleased] +### Fixed + +- **`pgparser` no longer reports `insert-without-columns` on + `INSERT INTO t DEFAULT VALUES`** ([#68]). The grammar encodes that form as + an absent row source, and the parser refilled `InsertColumnsListed` from + `len(Columns)` alone after blanking it — so the AST path dropped the + fallback's explicit handling ("DEFAULT VALUES inserts no data") and then + marked the result `Exact`. Opting into the exact parser made this rule + strictly worse than the zero-dependency default, inverting the trade-off + documented in [SQL Parsers](https://kartikrocks.github.io/sqlguard/docs/parsers). +- **`REPLACE INTO t VALUES (…)` is now flagged by `insert-without-columns`.** + MySQL/SQLite's `REPLACE` binds positionally exactly as `INSERT` does and + carries the same column-order risk, and a real MySQL grammar parses it into + the same AST node — so `mysqlparser` already reported it while the + zero-dependency fallback read it as an unrecognized statement kind and said + nothing. `REPLACE INTO t (a) VALUES (…)` is not flagged, and the + `REPLACE(str, from, to)` string function is never mistaken for a statement. + +### Added + +- **Parser parity is pinned by a test.** `pgparser` and `mysqlparser` each run + a corpus through both the dialect grammar and the fallback and assert the + grammar never reports a rule the fallback does not + (`TestParser_NeverAddsFindingTheFallbackDoesNot`). Opting into a real parser + can only remove findings, which is the direction the docs promise; both bugs + above are instances of the same shape, and only one had been noticed. + +[#68]: https://github.com/KARTIKrocks/sqlguard/issues/68 + ## [0.4.0] - 2026-09-25 ### Changed diff --git a/analyzer/analyzer_test.go b/analyzer/analyzer_test.go index 76419c8..93713cc 100644 --- a/analyzer/analyzer_test.go +++ b/analyzer/analyzer_test.go @@ -119,6 +119,14 @@ func TestCheckInsertWithoutColumns(t *testing.T) { {"mysql set form", "INSERT INTO users SET name = 'alice', email = 'a@test.com'", false}, {"default values", "INSERT INTO users DEFAULT VALUES", false}, {"cte insert no columns", "WITH s AS (SELECT 1) INSERT INTO users SELECT * FROM s", true}, + // REPLACE is MySQL/SQLite's insert-or-overwrite; positionally it is an + // INSERT and carries the same column-order risk. A real MySQL grammar + // parses it into the same AST node, so reading it as StmtOther here + // made the rule fire only for callers who opted into mysqlparser. + {"replace no columns", "REPLACE INTO users VALUES ('alice')", true}, + {"replace with columns", "REPLACE INTO users (name) VALUES ('alice')", false}, + // REPLACE(str, from, to) is a string function, not a statement. + {"replace function is not a statement", "SELECT REPLACE(name, 'a', 'b') FROM users", false}, } for _, tt := range tests { diff --git a/analyzer/fallback.go b/analyzer/fallback.go index a5e1411..285bc6d 100644 --- a/analyzer/fallback.go +++ b/analyzer/fallback.go @@ -37,7 +37,14 @@ var ( // singular VALUE; SELECT/WITH/TABLE cover INSERT ... SELECT and friends. fbInsertDataRe = regexp.MustCompile(`(?i)\b(VALUES?|SELECT|WITH|TABLE|SET|DEFAULT)\b`) fbLeadKindRe = regexp.MustCompile(`(?i)^\s*\(*\s*(SELECT|INSERT|UPDATE|DELETE|WITH)\b`) - fbDMLWordRe = regexp.MustCompile(`(?i)\b(INSERT|UPDATE|DELETE)\b`) + // fbReplaceIntoRe recognizes MySQL/SQLite's REPLACE, which carries the same + // positional column-order risk as INSERT and is the same AST node to a real + // MySQL grammar. INTO is required here although the dialects make it + // optional: it disambiguates the statement from the REPLACE(str, a, b) + // function, and the INTO-less form is one insertColumnsListed already + // declines to flag (it looks for the span after INTO). + fbReplaceIntoRe = regexp.MustCompile(`(?i)^\s*\(*\s*REPLACE\s+INTO\b`) + fbDMLWordRe = regexp.MustCompile(`(?i)\b(INSERT|UPDATE|DELETE)\b`) // fbWhereRegionEndRe marks the first clause keyword that ends the WHERE // region, so a function in ORDER BY / GROUP BY / HAVING isn't read as a @@ -448,6 +455,9 @@ func parenDepthBefore(s string, idx int) int { func detectKind(sanitized string) StmtKind { m := fbLeadKindRe.FindStringSubmatch(sanitized) if m == nil { + if fbReplaceIntoRe.MatchString(sanitized) { + return StmtInsert + } return StmtOther } switch strings.ToUpper(m[1]) { diff --git a/parsers/mysqlparser/mysqlparser_test.go b/parsers/mysqlparser/mysqlparser_test.go index abb1271..d57e644 100644 --- a/parsers/mysqlparser/mysqlparser_test.go +++ b/parsers/mysqlparser/mysqlparser_test.go @@ -149,3 +149,75 @@ func TestParser_IntegratesWithAnalyzer(t *testing.T) { t.Errorf("expected update-without-where (WHERE only in comment), got %+v", got) } } + +// TestParser_NeverAddsFindingTheFallbackDoesNot pins the direction of the +// trade-off website/docs/parsers.md promises: opting into the real grammar +// removes findings the heuristics guessed wrong, and never adds one the +// zero-dependency default would not have produced. A statement kind the +// grammar understands but the heuristics did not is the shape that breaks it — +// vitess parses REPLACE into the same node as INSERT, so it was reported +// against a statement the fallback read as StmtOther (#68). +// +// The REPLACE cases need the fallback's matching REPLACE handling, which is a +// core change landing in the same release as this test. Under GOWORK=off this +// module compiles against the core its go.mod requires — v0.4.0, which +// predates it — so REPLACE_INTO_t_VALUES fails there until the lockstep tag +// pins the core to the release carrying both. That failure is the go.work +// mechanism reporting a real version skew, not a flake: the pinned pair passes. +func TestParser_NeverAddsFindingTheFallbackDoesNot(t *testing.T) { + // Fallback-only findings are expected and allowed: they are the false + // positives the grammar exists to drop. + corpus := []string{ + "INSERT INTO t SET a = 1", + "INSERT INTO t (a, b) VALUES (1, 2)", + "INSERT INTO t VALUES (1, 2)", + "INSERT INTO t (a) SELECT x FROM u", + "INSERT INTO t (a) VALUES (1) ON DUPLICATE KEY UPDATE a = 2", + "REPLACE INTO t (a) VALUES (1)", + "REPLACE INTO t VALUES (1)", + "SELECT * FROM users", + "SELECT id FROM users WHERE id = 1", + "SELECT id FROM users ORDER BY id", + "SELECT DISTINCT id FROM users LIMIT 10", + "SELECT 1", + "DELETE FROM t", + "DELETE FROM t WHERE id = 1", + "UPDATE t SET a = 1", + "UPDATE t SET a = 1 WHERE id = 2", + "ALTER TABLE t ADD COLUMN c INT NOT NULL", + "CREATE TABLE t (id INT)", + "DROP TABLE t", + "TRUNCATE TABLE t", + "BEGIN", + "COMMIT", + "SET autocommit = 1", + "SELECT a FROM t LIMIT 5000, 10", + "SELECT a FROM t, u", + "SELECT a FROM t WHERE a IN (1, 2, 3)", + "SELECT a FROM t WHERE name LIKE '%abc%'", + "SELECT `a` FROM `t`", + "(SELECT a FROM t) UNION (SELECT b FROM u)", + } + + fallback := analyzer.Default() + exact := analyzer.Default().WithParser(New()) + + for _, sql := range corpus { + t.Run(sql, func(t *testing.T) { + base := ruleSet(fallback.Analyze(sql)) + for name := range ruleSet(exact.Analyze(sql)) { + if _, ok := base[name]; !ok { + t.Errorf("mysqlparser reports %q, the fallback does not", name) + } + } + }) + } +} + +func ruleSet(rs []analyzer.Result) map[string]struct{} { + out := make(map[string]struct{}, len(rs)) + for _, r := range rs { + out[r.RuleName] = struct{}{} + } + return out +} diff --git a/parsers/pgparser/pgparser.go b/parsers/pgparser/pgparser.go index b2af944..64f0992 100644 --- a/parsers/pgparser/pgparser.go +++ b/parsers/pgparser/pgparser.go @@ -87,13 +87,22 @@ func (p *Parser) Parse(sql string) (*analyzer.Statement, error) { st.OffsetValue = offsetValue(n.Limit) case *tree.Insert: st.Kind = analyzer.StmtInsert - st.InsertColumnsListed = len(n.Columns) > 0 + st.InsertColumnsListed = len(n.Columns) > 0 || defaultValues(n) } st.Exact = true return st, nil } +// defaultValues reports whether an INSERT is the "DEFAULT VALUES" form, which +// the grammar encodes as an absent row source. It names no columns and needs +// none — it inserts no data, so there is no positional column-order risk for +// insert-without-columns to warn about, matching the FallbackParser. tree's own +// Insert.DefaultValues dereferences Rows unguarded, hence the nil check here. +func defaultValues(n *tree.Insert) bool { + return n.Rows == nil || n.Rows.Select == nil +} + // fillSelectBody unwraps the inner SelectStatement of a *tree.Select. func fillSelectBody(st *analyzer.Statement, sel tree.SelectStatement) { switch c := sel.(type) { diff --git a/parsers/pgparser/pgparser_test.go b/parsers/pgparser/pgparser_test.go index b2e51b2..b44fe50 100644 --- a/parsers/pgparser/pgparser_test.go +++ b/parsers/pgparser/pgparser_test.go @@ -53,6 +53,13 @@ func TestParser_ExactStructuralFacts(t *testing.T) { sql: "INSERT INTO users VALUES ('a')", want: analyzer.Statement{Kind: analyzer.StmtInsert, InsertColumnsListed: false, Exact: true}, }, + { + // DEFAULT VALUES names no columns and needs none: it inserts no + // data, so there is no column order for a schema change to shift. + name: "insert default values counts as listed", + sql: "INSERT INTO users DEFAULT VALUES", + want: analyzer.Statement{Kind: analyzer.StmtInsert, InsertColumnsListed: true, Exact: true}, + }, { name: "order by without limit", sql: "SELECT id FROM users ORDER BY name", @@ -135,3 +142,73 @@ func TestParser_IntegratesWithAnalyzer(t *testing.T) { t.Errorf("expected no findings for safe query, got %+v", r) } } + +// TestParser_NeverAddsFindingTheFallbackDoesNot pins the direction of the +// trade-off website/docs/parsers.md promises: opting into the real grammar +// removes findings the heuristics guessed wrong, and never adds one the +// zero-dependency default would not have produced. A statement kind the +// grammar understands but the rules were never taught about is the shape that +// breaks it — INSERT ... DEFAULT VALUES did, by refilling +// InsertColumnsListed from len(Columns) alone after blanking it (#68). +func TestParser_NeverAddsFindingTheFallbackDoesNot(t *testing.T) { + // Fallback-only findings are expected and allowed: they are the false + // positives the grammar exists to drop. + corpus := []string{ + "INSERT INTO t DEFAULT VALUES", + "INSERT INTO t (a, b) VALUES (1, 2)", + "INSERT INTO t VALUES (1, 2)", + "INSERT INTO t (a) SELECT x FROM u", + "WITH c AS (SELECT 1) INSERT INTO t (a) SELECT n FROM c", + "SELECT * FROM users", + "SELECT id FROM users WHERE id = 1", + "SELECT id FROM users ORDER BY id", + "SELECT DISTINCT id FROM users LIMIT 10", + "SELECT 1", + "DELETE FROM t", + "DELETE FROM t WHERE id = 1", + "UPDATE t SET a = 1", + "UPDATE t SET a = 1 WHERE id = 2", + "ALTER TABLE t ADD COLUMN c INT NOT NULL", + "ALTER TABLE t ADD COLUMN c INT NOT NULL DEFAULT 0", + "CREATE TABLE t (id INT)", + "CREATE INDEX idx ON t (a)", + "DROP TABLE t", + "TRUNCATE TABLE t", + "BEGIN", + "COMMIT", + "ROLLBACK", + "SET search_path = public", + "SELECT a FROM t ORDER BY a OFFSET 5000", + "SELECT a FROM t, u WHERE t.id = u.id", + "SELECT a FROM t, u", + "SELECT a FROM t WHERE a IN (1, 2, 3)", + "SELECT a FROM t WHERE name LIKE '%abc%'", + "SELECT a FROM t WHERE lower(email) = 'x'", + "(SELECT a FROM t) UNION (SELECT b FROM u)", + "VALUES (1), (2)", + "SELECT t.* FROM t", + "SELECT * FROM t LIMIT 1 OFFSET 2000", + } + + fallback := analyzer.Default() + exact := analyzer.Default().WithParser(New()) + + for _, sql := range corpus { + t.Run(sql, func(t *testing.T) { + base := ruleSet(fallback.Analyze(sql)) + for name := range ruleSet(exact.Analyze(sql)) { + if _, ok := base[name]; !ok { + t.Errorf("pgparser reports %q, the fallback does not", name) + } + } + }) + } +} + +func ruleSet(rs []analyzer.Result) map[string]struct{} { + out := make(map[string]struct{}, len(rs)) + for _, r := range rs { + out[r.RuleName] = struct{}{} + } + return out +} diff --git a/website/docs/parsers.md b/website/docs/parsers.md index 102688f..6f77d7a 100644 --- a/website/docs/parsers.md +++ b/website/docs/parsers.md @@ -76,6 +76,15 @@ The first group is the false-positive-prone set; those become exact regardless of the parser, and each field's doc comment says so. This is a documented carve-out, not a gap waiting to be closed. +**A dialect parser only ever removes findings** — _0.5+_. Opting in can +drop a finding the heuristics guessed wrong, but it never reports one the +fallback would not have. Each parser module pins this against a corpus +(`TestParser_NeverAddsFindingTheFallbackDoesNot`), so a rule that reads a +structural field the grammar fills differently from the fallback fails +there rather than reaching you. Before 0.4, `pgparser` reported +`insert-without-columns` on `INSERT INTO t DEFAULT VALUES`, which the +fallback correctly ignores. + ## Degradation on parse failure A real grammar will reject SQL it does not know: dynamic fragments, a diff --git a/website/docs/rules.md b/website/docs/rules.md index 369d82a..247ca5b 100644 --- a/website/docs/rules.md +++ b/website/docs/rules.md @@ -175,7 +175,13 @@ Same as above for `UPDATE`. `INSERT INTO t VALUES (…)` and `INSERT INTO t SELECT …` bind positionally to the table's column order. Adding, dropping or reordering a column -silently shifts every value. +silently shifts every value. MySQL/SQLite's `REPLACE INTO t VALUES (…)` +binds the same way and is flagged too — _0.5+_. + +Forms that name their columns, or have none to name, are not flagged: +MySQL's `INSERT INTO t SET col = …`, and PostgreSQL's +`INSERT INTO t DEFAULT VALUES`, which writes no caller-supplied values at +all. > **Fix:** Specify columns explicitly: `INSERT INTO table (col1, col2) VALUES (…)`. From e5e97b1104be971eb37e9d1d4ae15b94209581cf Mon Sep 17 00:00:00 2001 From: kartik Date: Fri, 25 Sep 2026 13:54:16 +0530 Subject: [PATCH 2/4] fix: cover every row-inserting keyword in insert-without-columns Review of the previous commit found the parity test it added was passing while the invariant it claimed to pin was false, in three more places. The fallback recognized only INSERT and REPLACE INTO as inserting rows. Both grammars recognize more, so each of these was a grammar-only finding -- the exact shape the test exists to catch, missed because the corpus did not contain it: REPLACE t VALUES (1) MySQL makes INTO optional INSERT t VALUES (1) likewise, and pre-existing UPSERT INTO t VALUES (1) accepted by the CockroachDB-derived grammar INTO was required in the pattern to separate the statement from the REPLACE(str, from, to) function. Anchoring the keyword at the start of the statement separates them instead, and requiring a table name after it rejects a spaced REPLACE (a, b, c) call too, so INTO can go back to being optional. insertColumnsListed still prefers INTO when present: it is the only anchor that survives a CTE prefix, where the keyword can appear inside the CTE body. Falling back to the statement head only when INTO is absent keeps the CTE case intact, which a test now pins. Modifier runs (LOW_PRIORITY / DELAYED / HIGH_PRIORITY / IGNORE) sit between the keyword and the table in MySQL and are matched in both. Both corpora gained the shapes that motivated the test, and each fix was verified by reverting it and watching the corresponding rows fail. Also from review: - StmtInsert now covers REPLACE and UPSERT, which reaches a second consumer: explain.validate admits them under --allow-dml rather than refusing them as unrecognized. Documented in explain.md and the changelog; still planned-only and rolled back. - The rule message said "INSERT without explicit column list" for a statement the user did not write. Reworded to "Row-inserting statement". - StmtInsert and InsertColumnsListed doc comments recorded the rationale only in unexported helpers; the exported docs are what a rule author reads, and InsertColumnsListed's "names its target columns" is exactly the reading that produced the DEFAULT VALUES bug. - parsers.md stated "a dialect parser only ever removes findings" as an absolute. It is an invariant over a corpus, not a proof, and it was false when written. Reworded, and the version marker corrected from 0.4 to "0.4 and earlier" -- 0.4 is released and does have the bug. The parity rows that need the core's new keyword support are now skipped when the linked core predates it, instead of failing GOWORK=off until the next lockstep tag. go.work builds -- local dev and CI -- always run them. --- CHANGELOG.md | 34 +++++++++++----- analyzer/analyzer_test.go | 13 ++++++ analyzer/fallback.go | 46 +++++++++++++++------ analyzer/rules.go | 2 +- analyzer/statement.go | 16 ++++++-- parsers/mysqlparser/mysqlparser_test.go | 54 +++++++++++++++++++++---- parsers/pgparser/pgparser_test.go | 44 +++++++++++++++++++- website/docs/explain.md | 9 ++++- website/docs/parsers.md | 19 ++++++--- website/docs/rules.md | 7 +++- 10 files changed, 201 insertions(+), 43 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 415c326..2020266 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,13 +19,17 @@ the same version in lockstep. marked the result `Exact`. Opting into the exact parser made this rule strictly worse than the zero-dependency default, inverting the trade-off documented in [SQL Parsers](https://kartikrocks.github.io/sqlguard/docs/parsers). -- **`REPLACE INTO t VALUES (…)` is now flagged by `insert-without-columns`.** - MySQL/SQLite's `REPLACE` binds positionally exactly as `INSERT` does and - carries the same column-order risk, and a real MySQL grammar parses it into - the same AST node — so `mysqlparser` already reported it while the - zero-dependency fallback read it as an unrecognized statement kind and said - nothing. `REPLACE INTO t (a) VALUES (…)` is not flagged, and the - `REPLACE(str, from, to)` string function is never mistaken for a statement. +- **`insert-without-columns` now covers every keyword that inserts rows + positionally**, not just `INSERT INTO`: MySQL/SQLite's `REPLACE`, the + `UPSERT` the CockroachDB-derived grammar behind `pgparser` accepts, and the + forms that omit MySQL's optional `INTO` (`INSERT t VALUES (…)`, + `REPLACE t VALUES (…)`), including a `LOW_PRIORITY`/`DELAYED`/`IGNORE` + modifier run. All bind by column order and carry the same schema-change + risk, and all are the same AST node to a real grammar — so the dialect + parsers already reported them while the fallback read them as an + unrecognized statement kind and said nothing. `REPLACE(str, from, to)` is + never mistaken for a statement, and a `REPLACE()` call inside a CTE does not + displace the real statement head. ### Added @@ -33,8 +37,20 @@ the same version in lockstep. a corpus through both the dialect grammar and the fallback and assert the grammar never reports a rule the fallback does not (`TestParser_NeverAddsFindingTheFallbackDoesNot`). Opting into a real parser - can only remove findings, which is the direction the docs promise; both bugs - above are instances of the same shape, and only one had been noticed. + should only remove findings, which is the direction the docs promise. It is + an invariant over a corpus rather than a proof — all four bugs above are the + same shape, and only one of them had been noticed. + +### Changed + +- **`StmtInsert` now covers `REPLACE` and `UPSERT`**, which reaches + `sqlguard explain`: both were previously refused as unrecognized statements + and are now admitted under `--allow-dml`, planned and rolled back like any + other DML. On a server where the keyword is not valid, the server's syntax + error replaces sqlguard's refusal. +- **`insert-without-columns` message reworded** from "INSERT without explicit + column list" to "Row-inserting statement without an explicit column list", + since it no longer fires only on `INSERT`. [#68]: https://github.com/KARTIKrocks/sqlguard/issues/68 diff --git a/analyzer/analyzer_test.go b/analyzer/analyzer_test.go index 93713cc..8c651f2 100644 --- a/analyzer/analyzer_test.go +++ b/analyzer/analyzer_test.go @@ -125,8 +125,21 @@ func TestCheckInsertWithoutColumns(t *testing.T) { // made the rule fire only for callers who opted into mysqlparser. {"replace no columns", "REPLACE INTO users VALUES ('alice')", true}, {"replace with columns", "REPLACE INTO users (name) VALUES ('alice')", false}, + // INTO is optional in MySQL for both keywords, and a modifier run may + // sit between the keyword and the table. + {"replace without into", "REPLACE users VALUES ('alice')", true}, + {"replace without into with columns", "REPLACE users (name) VALUES ('alice')", false}, + {"insert without into", "INSERT users VALUES ('alice')", true}, + {"insert without into with columns", "INSERT users (name) VALUES ('alice')", false}, + {"insert with modifier", "INSERT LOW_PRIORITY INTO users VALUES ('alice')", true}, + {"replace delayed without into", "REPLACE DELAYED users VALUES ('alice')", true}, + // UPSERT reaches the INSERT node in the grammar behind pgparser. + {"upsert no columns", "UPSERT INTO users VALUES ('alice')", true}, + {"upsert with columns", "UPSERT INTO users (name) VALUES ('alice')", false}, // REPLACE(str, from, to) is a string function, not a statement. {"replace function is not a statement", "SELECT REPLACE(name, 'a', 'b') FROM users", false}, + {"replace function inside an insert", "INSERT INTO users (name) VALUES (REPLACE(x, 'a', 'b'))", false}, + {"cte containing a replace call", "WITH s AS (SELECT REPLACE(a, 'x', 'y') AS n FROM u) INSERT INTO users SELECT n FROM s", true}, } for _, tt := range tests { diff --git a/analyzer/fallback.go b/analyzer/fallback.go index 285bc6d..6f06190 100644 --- a/analyzer/fallback.go +++ b/analyzer/fallback.go @@ -37,14 +37,25 @@ var ( // singular VALUE; SELECT/WITH/TABLE cover INSERT ... SELECT and friends. fbInsertDataRe = regexp.MustCompile(`(?i)\b(VALUES?|SELECT|WITH|TABLE|SET|DEFAULT)\b`) fbLeadKindRe = regexp.MustCompile(`(?i)^\s*\(*\s*(SELECT|INSERT|UPDATE|DELETE|WITH)\b`) - // fbReplaceIntoRe recognizes MySQL/SQLite's REPLACE, which carries the same - // positional column-order risk as INSERT and is the same AST node to a real - // MySQL grammar. INTO is required here although the dialects make it - // optional: it disambiguates the statement from the REPLACE(str, a, b) - // function, and the INTO-less form is one insertColumnsListed already - // declines to flag (it looks for the span after INTO). - fbReplaceIntoRe = regexp.MustCompile(`(?i)^\s*\(*\s*REPLACE\s+INTO\b`) - fbDMLWordRe = regexp.MustCompile(`(?i)\b(INSERT|UPDATE|DELETE)\b`) + // fbInsertModRe is the optional modifier run MySQL allows between the + // statement keyword and the target table. + fbInsertModRe = `(?:\s+(?:LOW_PRIORITY|DELAYED|HIGH_PRIORITY|IGNORE))*` + // fbLeadInsertLikeRe recognizes the statement keywords that are an INSERT + // positionally without being spelled INSERT: MySQL/SQLite's REPLACE and the + // UPSERT accepted by the CockroachDB-derived grammar behind pgparser. Both + // bind by column order and carry the same schema-change risk, and both are + // the same AST node to the real grammars — so leaving them as StmtOther + // here means the rule fires only for callers who opted into a parser. + // INTO is optional (MySQL's is), but a table name must follow, which is + // what separates the statement from the REPLACE(str, from, to) function. + fbLeadInsertLikeRe = regexp.MustCompile(`(?i)^\s*\(*\s*(?:REPLACE|UPSERT)` + fbInsertModRe + `\s+(?:INTO\s+)?[^\s(]`) + // fbInsertHeadRe spans the statement keyword and its modifiers, used to + // find the target table when the optional INTO is absent. Anchored at the + // start, unlike fbIntoRe: without that anchor the keyword could be matched + // inside a CTE body (a REPLACE() call in a WITH clause), and the INTO-less + // forms are MySQL-only, where the CTE-prefixed shape needs INTO anyway. + fbInsertHeadRe = regexp.MustCompile(`(?i)^\s*\(*\s*(?:INSERT|REPLACE|UPSERT)` + fbInsertModRe + `\b`) + fbDMLWordRe = regexp.MustCompile(`(?i)\b(INSERT|UPDATE|DELETE)\b`) // fbWhereRegionEndRe marks the first clause keyword that ends the WHERE // region, so a function in ORDER BY / GROUP BY / HAVING isn't read as a @@ -149,11 +160,20 @@ func (p *FallbackParser) Parse(sql string) (*Statement, error) { // both count as listed (no positional column-order risk to warn about). // Comment-free, literal-blanked input expected; heuristic by contract. func insertColumnsListed(sanitized string) bool { - loc := fbIntoRe.FindStringIndex(sanitized) - if loc == nil { - return true // no INTO found — can't tell, don't flag + // INTO is the tightest anchor and the only one that survives a CTE prefix, + // where the statement keyword itself can appear inside the CTE body. Fall + // back to the statement head for the forms that omit it. + var rest string + switch loc := fbIntoRe.FindStringIndex(sanitized); { + case loc != nil: + rest = sanitized[loc[1]:] + default: + head := fbInsertHeadRe.FindStringIndex(sanitized) + if head == nil { + return true // no recognizable statement head — can't tell, don't flag + } + rest = sanitized[head[1]:] } - rest := sanitized[loc[1]:] data := fbInsertDataRe.FindStringIndex(rest) if data == nil { return true // no recognizable data clause — don't flag @@ -455,7 +475,7 @@ func parenDepthBefore(s string, idx int) int { func detectKind(sanitized string) StmtKind { m := fbLeadKindRe.FindStringSubmatch(sanitized) if m == nil { - if fbReplaceIntoRe.MatchString(sanitized) { + if fbLeadInsertLikeRe.MatchString(sanitized) { return StmtInsert } return StmtOther diff --git a/analyzer/rules.go b/analyzer/rules.go index bc4466c..3d89f3f 100644 --- a/analyzer/rules.go +++ b/analyzer/rules.go @@ -110,7 +110,7 @@ func CheckInsertWithoutColumns(s *Statement) (Result, bool) { return Result{ RuleName: "insert-without-columns", Query: s.Raw, - Message: "INSERT without explicit column list. This breaks if table schema changes.", + Message: "Row-inserting statement without an explicit column list. This breaks if table schema changes.", Suggestion: "Specify columns explicitly: INSERT INTO table (col1, col2) VALUES (...).", }, true } diff --git a/analyzer/statement.go b/analyzer/statement.go index 3b54401..2d4bbdd 100644 --- a/analyzer/statement.go +++ b/analyzer/statement.go @@ -8,7 +8,12 @@ const ( StmtUnknown StmtKind = iota // StmtSelect is a SELECT (or WITH ... SELECT) query. StmtSelect - // StmtInsert is an INSERT statement. + // StmtInsert is an INSERT statement, or one of the dialect keywords that + // inserts rows the same way: MySQL/SQLite's REPLACE and the UPSERT the + // CockroachDB-derived grammar behind pgparser accepts. They bind values by + // column order exactly as INSERT does, so every rule that targets INSERT + // targets them. Note this also makes them DML to explain.validate, which + // accepts them under WithAllowDML rather than refusing them as unrecognized. StmtInsert // StmtUpdate is an UPDATE statement. StmtUpdate @@ -60,8 +65,13 @@ type Statement struct { SelectDistinct bool // InsertColumnsListed reports whether an INSERT names its target columns - // explicitly: INSERT INTO t (a, b) VALUES (...). Only meaningful when - // Kind == StmtInsert. + // explicitly, or has no columns to name. The second case is what keeps + // PostgreSQL's INSERT INTO t DEFAULT VALUES and MySQL's + // INSERT INTO t SET col = ... out of insert-without-columns: neither binds + // caller-supplied values by position, so no schema change can shift them. + // A parser filling this from an AST must account for both — reading it as + // "the column list is non-empty" alone reports DEFAULT VALUES. + // Only meaningful when Kind == StmtInsert. InsertColumnsListed bool // LeadingWildcardLike reports a LIKE pattern beginning with a wildcard diff --git a/parsers/mysqlparser/mysqlparser_test.go b/parsers/mysqlparser/mysqlparser_test.go index d57e644..ea5c8f1 100644 --- a/parsers/mysqlparser/mysqlparser_test.go +++ b/parsers/mysqlparser/mysqlparser_test.go @@ -1,6 +1,7 @@ package mysqlparser import ( + "strings" "testing" "github.com/KARTIKrocks/sqlguard/analyzer" @@ -158,12 +159,9 @@ func TestParser_IntegratesWithAnalyzer(t *testing.T) { // vitess parses REPLACE into the same node as INSERT, so it was reported // against a statement the fallback read as StmtOther (#68). // -// The REPLACE cases need the fallback's matching REPLACE handling, which is a -// core change landing in the same release as this test. Under GOWORK=off this -// module compiles against the core its go.mod requires — v0.4.0, which -// predates it — so REPLACE_INTO_t_VALUES fails there until the lockstep tag -// pins the core to the release carrying both. That failure is the go.work -// mechanism reporting a real version skew, not a flake: the pinned pair passes. +// Some rows below need matching support in the core's fallback parser; see +// fallbackKnowsInsertLikeKeywords for why they are skipped against an older +// core rather than failing. func TestParser_NeverAddsFindingTheFallbackDoesNot(t *testing.T) { // Fallback-only findings are expected and allowed: they are the false // positives the grammar exists to drop. @@ -175,6 +173,20 @@ func TestParser_NeverAddsFindingTheFallbackDoesNot(t *testing.T) { "INSERT INTO t (a) VALUES (1) ON DUPLICATE KEY UPDATE a = 2", "REPLACE INTO t (a) VALUES (1)", "REPLACE INTO t VALUES (1)", + // INTO is optional in MySQL for both keywords, and the modifier run may + // sit between the keyword and the table. These are the shapes an + // INTO-anchored heuristic misses while the grammar still reports them. + "REPLACE t VALUES (1)", + "REPLACE t (a) VALUES (1)", + "INSERT t VALUES (1)", + "INSERT t (a) VALUES (1)", + "INSERT LOW_PRIORITY INTO t VALUES (1)", + "INSERT IGNORE INTO t VALUES (1)", + "REPLACE LOW_PRIORITY INTO t VALUES (1)", + "REPLACE DELAYED t VALUES (1)", + "INSERT INTO t VALUES (1), (2)", + "SELECT REPLACE(name, 'a', 'b') FROM t", + "SELECT * FROM t WHERE a = 'REPLACE INTO x VALUES (1)'", "SELECT * FROM users", "SELECT id FROM users WHERE id = 1", "SELECT id FROM users ORDER BY id", @@ -201,19 +213,34 @@ func TestParser_NeverAddsFindingTheFallbackDoesNot(t *testing.T) { fallback := analyzer.Default() exact := analyzer.Default().WithParser(New()) + coreKnows := fallbackKnowsInsertLikeKeywords() for _, sql := range corpus { t.Run(sql, func(t *testing.T) { + if !coreKnows && needsCoreKeywordSupport(sql) { + t.Skip("linked core predates the fallback's row-inserting keyword support") + } base := ruleSet(fallback.Analyze(sql)) for name := range ruleSet(exact.Analyze(sql)) { if _, ok := base[name]; !ok { - t.Errorf("mysqlparser reports %q, the fallback does not", name) + t.Errorf("%s reports %q, the fallback does not", "mysqlparser", name) } } }) } } +// needsCoreKeywordSupport reports whether a corpus entry's parity depends on +// the core recognizing a row-inserting keyword other than "INSERT INTO". +func needsCoreKeywordSupport(sql string) bool { + up := strings.ToUpper(strings.TrimSpace(sql)) + if strings.HasPrefix(up, "INSERT INTO ") { + return false + } + return strings.HasPrefix(up, "REPLACE") || strings.HasPrefix(up, "UPSERT") || + strings.HasPrefix(up, "INSERT") +} + func ruleSet(rs []analyzer.Result) map[string]struct{} { out := make(map[string]struct{}, len(rs)) for _, r := range rs { @@ -221,3 +248,16 @@ func ruleSet(rs []analyzer.Result) map[string]struct{} { } return out } + +// fallbackKnowsInsertLikeKeywords reports whether the linked core recognizes +// the row-inserting keywords that are not spelled INSERT. That support and +// this test ship in the same release, but this is a separate module: under +// GOWORK=off it builds against the core its go.mod requires, which until the +// next lockstep tag predates the support. Parity against an older core is a +// version skew rather than a parity bug, so the rows that depend on it are +// skipped there instead of failing. go.work (local dev and CI) always builds +// against this tree, where they run. +func fallbackKnowsInsertLikeKeywords() bool { + st, _ := analyzer.NewFallbackParser().Parse("REPLACE INTO t VALUES (1)") + return st.Kind == analyzer.StmtInsert +} diff --git a/parsers/pgparser/pgparser_test.go b/parsers/pgparser/pgparser_test.go index b44fe50..21a852a 100644 --- a/parsers/pgparser/pgparser_test.go +++ b/parsers/pgparser/pgparser_test.go @@ -1,6 +1,7 @@ package pgparser import ( + "strings" "testing" "github.com/KARTIKrocks/sqlguard/analyzer" @@ -150,11 +151,24 @@ func TestParser_IntegratesWithAnalyzer(t *testing.T) { // grammar understands but the rules were never taught about is the shape that // breaks it — INSERT ... DEFAULT VALUES did, by refilling // InsertColumnsListed from len(Columns) alone after blanking it (#68). +// +// Some rows below need matching support in the core's fallback parser; see +// fallbackKnowsInsertLikeKeywords for why they are skipped against an older +// core rather than failing. func TestParser_NeverAddsFindingTheFallbackDoesNot(t *testing.T) { // Fallback-only findings are expected and allowed: they are the false // positives the grammar exists to drop. corpus := []string{ "INSERT INTO t DEFAULT VALUES", + "INSERT INTO t DEFAULT VALUES RETURNING id", + // UPSERT is accepted by the CockroachDB-derived grammar behind this + // parser, so it reaches the INSERT node while a keyword-list heuristic + // would not recognize the statement at all. + "UPSERT INTO t VALUES (1)", + "UPSERT INTO t (a) VALUES (1)", + "INSERT INTO t VALUES (1) RETURNING id", + "INSERT INTO t (a) VALUES (1) ON CONFLICT (a) DO NOTHING", + "SELECT replace(name, 'a', 'b') FROM t", "INSERT INTO t (a, b) VALUES (1, 2)", "INSERT INTO t VALUES (1, 2)", "INSERT INTO t (a) SELECT x FROM u", @@ -192,19 +206,34 @@ func TestParser_NeverAddsFindingTheFallbackDoesNot(t *testing.T) { fallback := analyzer.Default() exact := analyzer.Default().WithParser(New()) + coreKnows := fallbackKnowsInsertLikeKeywords() for _, sql := range corpus { t.Run(sql, func(t *testing.T) { + if !coreKnows && needsCoreKeywordSupport(sql) { + t.Skip("linked core predates the fallback's row-inserting keyword support") + } base := ruleSet(fallback.Analyze(sql)) for name := range ruleSet(exact.Analyze(sql)) { if _, ok := base[name]; !ok { - t.Errorf("pgparser reports %q, the fallback does not", name) + t.Errorf("%s reports %q, the fallback does not", "pgparser", name) } } }) } } +// needsCoreKeywordSupport reports whether a corpus entry's parity depends on +// the core recognizing a row-inserting keyword other than "INSERT INTO". +func needsCoreKeywordSupport(sql string) bool { + up := strings.ToUpper(strings.TrimSpace(sql)) + if strings.HasPrefix(up, "INSERT INTO ") { + return false + } + return strings.HasPrefix(up, "REPLACE") || strings.HasPrefix(up, "UPSERT") || + strings.HasPrefix(up, "INSERT") +} + func ruleSet(rs []analyzer.Result) map[string]struct{} { out := make(map[string]struct{}, len(rs)) for _, r := range rs { @@ -212,3 +241,16 @@ func ruleSet(rs []analyzer.Result) map[string]struct{} { } return out } + +// fallbackKnowsInsertLikeKeywords reports whether the linked core recognizes +// the row-inserting keywords that are not spelled INSERT. That support and +// this test ship in the same release, but this is a separate module: under +// GOWORK=off it builds against the core its go.mod requires, which until the +// next lockstep tag predates the support. Parity against an older core is a +// version skew rather than a parity bug, so the rows that depend on it are +// skipped there instead of failing. go.work (local dev and CI) always builds +// against this tree, where they run. +func fallbackKnowsInsertLikeKeywords() bool { + st, _ := analyzer.NewFallbackParser().Parse("REPLACE INTO t VALUES (1)") + return st.Kind == analyzer.StmtInsert +} diff --git a/website/docs/explain.md b/website/docs/explain.md index 1c2775b..1aaea5a 100644 --- a/website/docs/explain.md +++ b/website/docs/explain.md @@ -32,7 +32,7 @@ sqlguard explain --db "…" --format json "SELECT …" | `--db ` | required | Connection string. Postgres DSNs use pgx's `postgres://` URL or key=value form; MySQL uses `go-sql-driver/mysql` DSN syntax. | | `--dialect postgres\|mysql` | `postgres` | Which planner to talk to. MariaDB works through `mysql`. | | `--format console\|json` | `console` | Output shape. | -| `--allow-dml` | off | Permit `INSERT` / `UPDATE` / `DELETE`. Still planned only, still rolled back. | +| `--allow-dml` | off | Permit `INSERT` / `UPDATE` / `DELETE` (incl. `REPLACE` / `UPSERT` _0.5+_). Still planned only, still rolled back. | | `--config`, `--no-config` | — | Persistent flags. `rules:` config applies — see below. | The whole command runs under a 30-second timeout, including the initial @@ -123,6 +123,13 @@ not rely on parameterization: `--allow-dml`; DDL, `SET`, transaction control and anything unrecognised are always refused. + _Changed in 0.5._ The row-inserting dialect keywords `REPLACE` and + `UPSERT` now classify as `INSERT` rather than as unrecognised, so + `--allow-dml` admits them instead of refusing them outright. They are + planned and rolled back like any other DML. On a server where the + keyword is not valid you now get that server's syntax error in place of + sqlguard's refusal. + _Changed in 0.3._ The separator check previously scanned with a single reading of `$$`, which let some stacked input through. It now counts a `;` as a separator whenever _any_ dialect reading leaves it outside a literal, diff --git a/website/docs/parsers.md b/website/docs/parsers.md index 6f77d7a..450a889 100644 --- a/website/docs/parsers.md +++ b/website/docs/parsers.md @@ -76,14 +76,21 @@ The first group is the false-positive-prone set; those become exact regardless of the parser, and each field's doc comment says so. This is a documented carve-out, not a gap waiting to be closed. -**A dialect parser only ever removes findings** — _0.5+_. Opting in can -drop a finding the heuristics guessed wrong, but it never reports one the -fallback would not have. Each parser module pins this against a corpus +**A dialect parser is meant to remove findings, never add them** — _0.5+_. +Opting in can drop a finding the heuristics guessed wrong; it should not +report one the fallback would not have. Each parser module pins this +direction against a corpus (`TestParser_NeverAddsFindingTheFallbackDoesNot`), so a rule that reads a structural field the grammar fills differently from the fallback fails -there rather than reaching you. Before 0.4, `pgparser` reported -`insert-without-columns` on `INSERT INTO t DEFAULT VALUES`, which the -fallback correctly ignores. +there rather than reaching you. That is a tested invariant over a corpus, +not a proof: a dialect form neither the corpus nor the fallback's keyword +list knows is how it would break again. + +In 0.4 and earlier it did break, twice in the same rule. `pgparser` +reported `insert-without-columns` on `INSERT INTO t DEFAULT VALUES`, and +both grammars reported it on row-inserting keywords the fallback did not +recognize as inserts at all — `REPLACE`, `UPSERT`, and the `INTO`-less +`INSERT t VALUES (…)` MySQL accepts. ## Degradation on parse failure diff --git a/website/docs/rules.md b/website/docs/rules.md index 247ca5b..3d10e24 100644 --- a/website/docs/rules.md +++ b/website/docs/rules.md @@ -175,8 +175,11 @@ Same as above for `UPDATE`. `INSERT INTO t VALUES (…)` and `INSERT INTO t SELECT …` bind positionally to the table's column order. Adding, dropping or reordering a column -silently shifts every value. MySQL/SQLite's `REPLACE INTO t VALUES (…)` -binds the same way and is flagged too — _0.5+_. +silently shifts every value. + +Every keyword that inserts rows this way is flagged, not just `INSERT` — +MySQL/SQLite's `REPLACE`, the `UPSERT` CockroachDB accepts, and the forms +that omit the optional `INTO` (`INSERT t VALUES (…)`) — _0.5+_. Forms that name their columns, or have none to name, are not flagged: MySQL's `INSERT INTO t SET col = …`, and PostgreSQL's From 4867dc55df48ec8a468fecb4e8352010ebb1ec66 Mon Sep 17 00:00:00 2001 From: kartik Date: Fri, 25 Sep 2026 13:59:44 +0530 Subject: [PATCH 3/4] docs: record the parser parity invariant in AGENTS.md The invariant the new parity test pins was only written down in the test and in parsers.md. AGENTS.md is where the architectural invariants that constrain a change live, and this one constrains a specific future change: teaching a dialect parser a statement kind obliges teaching the fallback the same kind, or the grammar starts reporting findings the default never would. Records the pair to check (detectKind and insertColumnsListed), why the INTO anchor is preferred over the statement head, and why a corpus row can skip under GOWORK=off. --- AGENTS.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index 21eb737..a7b4bac 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -45,6 +45,8 @@ When changing the public API or Go version, update all nine `go.mod` files and ` **The analyzer is parser-pluggable.** `analyzer.Analyzer` runs `Rule`s against a normalized, dialect-agnostic `Statement` produced by an `analyzer.Parser` (`analyzer/parser.go`, `statement.go`). The default `FallbackParser` (`fallback.go`) is zero-dependency, strips comments/string literals, and never errors. `analyzer.Analyze` degrades to the FallbackParser if a configured parser errors, so analysis never breaks the caller's query path. Real grammars are supplied via `middleware.WithParser(...)` / `analyzer.Default().WithParser(...)` using the `parsers/*` modules. Rules read the `Statement`, never raw SQL. +**A dialect parser may only remove findings, never add one.** Opting into a real grammar is sold as trading false positives away; a grammar-only finding inverts that, and the reporting surface is identical, so nothing warns. `TestParser_NeverAddsFindingTheFallbackDoesNot` in each `parsers/*` module runs a corpus through both the grammar and the FallbackParser and fails on any rule the grammar reports and the fallback does not. It is an invariant over a corpus, not a proof: the shape that breaks it is a statement the grammar understands and the fallback's keyword list does not, which is exactly how all four instances in #68 arose (`INSERT ... DEFAULT VALUES` under `pgparser`; `REPLACE`, `UPSERT` and the `INTO`-less `INSERT t VALUES (...)` under both). So **when a parser learns a statement kind, the fallback has to learn it too** — `detectKind` and `insertColumnsListed` are the pair to check — and a new structural field read by a rule needs a corpus row here. `insertColumnsListed` prefers the `INTO` anchor when present because it is the only one that survives a CTE prefix, where the statement keyword can appear inside the CTE body; the statement-head anchor is the `INTO`-less fallback, deliberately start-anchored so a `REPLACE()` call in a `WITH` clause cannot displace it. Because the test compares a satellite module against the core's fallback, a corpus row depending on core support the published core lacks is skipped rather than failed, so `GOWORK=off` stays green between lockstep tags. + **Rules self-register; config is resolved once, never per query.** Built-in rules call `analyzer.Register(RuleSpec{...})` from `init()` in `analyzer/rules.go` — a stable name, default severity, and a settings-aware `Factory`. To add a rule, write it and add one `Register` call; **do not** hand-maintain a rule list in `Default()`. Being addressable by name is what makes enable/disable, severity overrides, per-rule `Settings`, and suppressions work uniformly. `analyzer.Profile` (disabled set, `only` whitelist, severity map, per-rule settings) is applied in `DefaultWithProfile` at construction; the per-query `Analyze` path does no config work (it runs on every query through the driver — keep it allocation-light). `analyzer` must stay free of `config`/YAML imports. **Not every registered rule is evaluated by the Analyzer.** A `RuleSpec` with a nil `Factory` registers a name without a rule body — `RuleSpec.Evaluated()` reports the difference, and `DefaultWithProfile` skips those when binding. Seven use this: `slow-query` and `n-plus-one`, which `middleware` derives from latency and fingerprint counts, and `seq-scan` / `high-cost` / `full-table-scan` / `no-index-used` / `filesort`, which `explain` derives from the database's own plan. None can be run against a parsed `Statement`, but all seven are documented as rules, so they register to be **addressable**: `disable`, `only`, `severity` and `settings` must mean the same thing on every surface. Their owners ask `Analyzer.RuleEnabled` / `RuleSeverity` / `RuleSettings` for the resolved decision. **`RuleEnabled` answers `disable:` and `severity: off` but deliberately ignores the `only:` whitelist**, because `only:` selects which rules are evaluated _against a statement_ and none of these seven are: a list written to focus `sqlguard scan` would otherwise switch off latency and N+1 reporting in a running application and blank out `sqlguard explain`, none of which it names, and none of which warns. Switching one off takes naming it, which is why `disable:` reaches every surface and `only:` reaches one (pinned by `TestRuleEnabledIgnoresOnly`, `TestRuleEnabledHonoursDisableInsideAnOnlyList` and `TestApplyProfile_IgnoresOnly`). `Guard` resolves its two once in `NewGuard` (the per-query path stays config-free), and `explain` filters centrally in `applyProfile` rather than at each of the five sites that build a finding, so a plan rule added later cannot forget the check. All seven are registered in `analyzer/rules.go`, **not** in the packages that own them: `config` validates names against `analyzer.RuleNames()` while importing only `analyzer`, so registering `seq-scan` from `explain`'s `init()` would make a config naming it fail for anyone who doesn't link that package. Per-rule tunables live in `rules.settings` for these too (`slow-query.threshold`, `n-plus-one.threshold`/`window`, the latter pair also being what turns N+1 on from a file); an explicit Go option (`WithSlowQueryThreshold`, `WithN1Detection`) outranks the file. `config.checkSettings` validates the whole settings block, because every read path — `Settings.Duration`, `Settings.Int` — falls back to the caller's default rather than failing, so anything it cannot use becomes a silently wrong threshold (or, for `n-plus-one`, detection that never switches on). It checks the key against `ruleSettings`, which is the complete tunable set by design: a key absent from a listed rule is a misspelling, and any key on an unlisted rule is a mistake, since that rule reads no settings. It also checks the value's type, rejects a non-positive `n-plus-one.threshold`, and reports half a paired block. `config.checkScanHasRules` covers the shapes that only became expressible once the seven were registered, and asks `Profile.Skip` — the same question `DefaultWithProfile` asks — rather than testing the list against `EvaluatedRuleNames()`, which was blind to a name `only:` selects and `disable:` or `severity: off` then takes away again. It is gated on `only:` being configured, because disabling every rule without one is a legitimate runtime-only setup. Validation reads values through `Settings.LookupDuration`/`LookupInt`, the accessors `Duration`/`Int` themselves delegate to, so a value the loader accepts can never be one the reader silently replaces with a default. From 2b16f844980995db893383936312141fbacb4492 Mon Sep 17 00:00:00 2001 From: kartik Date: Fri, 25 Sep 2026 14:22:59 +0530 Subject: [PATCH 4/4] fix: anchor the INTO lookup and classify CTE-prefixed insert keywords Two more instances of the same shape, both reproduced before fixing. A column named "into" defeated the rule. The target table was found by scanning for INTO anywhere in the statement, so in a form that omits the optional keyword the backticked identifier matched, the column list was read as the target table, and a statement that does name its columns was reported as having none: INSERT t (`into`) VALUES (1) -> insert-without-columns The statement head is now tried first and INTO only for the CTE-prefixed forms, where the keyword sits mid-statement and can also occur inside the CTE body. Note the ordering is the whole fix: stripping a leading INTO off the span as well changed no result, because that span holds no "(", so it is left out rather than kept as an untested branch. This predates the previous commit for INSERT and was widened to REPLACE by it. A CTE prefix also put UPSERT past the leading-keyword check, leaving "WITH c AS (...) UPSERT INTO t SELECT n FROM c" classified as a SELECT while pgparser read it as an insert -- another grammar-only finding that the corpus did not contain. detectKind now checks the insert-like keywords after a WITH clause too, with the same table-name requirement that keeps a REPLACE() call in a CTE body from being read as the statement. The GOWORK=off skip guard matched on a leading keyword, so it would not have covered the CTE-prefixed row; it now matches the keyword anywhere. Docs, from the same review: - parsers.md attributed every past break to "both grammars". UPSERT is pgparser's, REPLACE and the INTO-less INSERT are mysqlparser's. Split per parser, and "twice" was already stale. - Version markers followed neither form AGENTS.md sanctions: a prose paragraph takes a leading "_Added in X.Y._" / "_Changed in X.Y._", and "_0.5+_" is for a table cell. rules.md also gained the line on previous behaviour that a "Changed" marker requires. explain.md was already correct and is untouched. - AGENTS.md described insertColumnsListed as preferring the INTO anchor, which is now backwards and was the bug; corrected, with why the order matters. --- AGENTS.md | 2 +- CHANGELOG.md | 11 +++++++- analyzer/analyzer_test.go | 7 +++++ analyzer/fallback.go | 34 +++++++++++++++++-------- parsers/mysqlparser/mysqlparser_test.go | 18 +++++++++---- parsers/pgparser/pgparser_test.go | 19 ++++++++++---- website/docs/parsers.md | 21 ++++++++------- website/docs/rules.md | 8 +++--- 8 files changed, 85 insertions(+), 35 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index a7b4bac..e8a3f5b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -45,7 +45,7 @@ When changing the public API or Go version, update all nine `go.mod` files and ` **The analyzer is parser-pluggable.** `analyzer.Analyzer` runs `Rule`s against a normalized, dialect-agnostic `Statement` produced by an `analyzer.Parser` (`analyzer/parser.go`, `statement.go`). The default `FallbackParser` (`fallback.go`) is zero-dependency, strips comments/string literals, and never errors. `analyzer.Analyze` degrades to the FallbackParser if a configured parser errors, so analysis never breaks the caller's query path. Real grammars are supplied via `middleware.WithParser(...)` / `analyzer.Default().WithParser(...)` using the `parsers/*` modules. Rules read the `Statement`, never raw SQL. -**A dialect parser may only remove findings, never add one.** Opting into a real grammar is sold as trading false positives away; a grammar-only finding inverts that, and the reporting surface is identical, so nothing warns. `TestParser_NeverAddsFindingTheFallbackDoesNot` in each `parsers/*` module runs a corpus through both the grammar and the FallbackParser and fails on any rule the grammar reports and the fallback does not. It is an invariant over a corpus, not a proof: the shape that breaks it is a statement the grammar understands and the fallback's keyword list does not, which is exactly how all four instances in #68 arose (`INSERT ... DEFAULT VALUES` under `pgparser`; `REPLACE`, `UPSERT` and the `INTO`-less `INSERT t VALUES (...)` under both). So **when a parser learns a statement kind, the fallback has to learn it too** — `detectKind` and `insertColumnsListed` are the pair to check — and a new structural field read by a rule needs a corpus row here. `insertColumnsListed` prefers the `INTO` anchor when present because it is the only one that survives a CTE prefix, where the statement keyword can appear inside the CTE body; the statement-head anchor is the `INTO`-less fallback, deliberately start-anchored so a `REPLACE()` call in a `WITH` clause cannot displace it. Because the test compares a satellite module against the core's fallback, a corpus row depending on core support the published core lacks is skipped rather than failed, so `GOWORK=off` stays green between lockstep tags. +**A dialect parser may only remove findings, never add one.** Opting into a real grammar is sold as trading false positives away; a grammar-only finding inverts that, and the reporting surface is identical, so nothing warns. `TestParser_NeverAddsFindingTheFallbackDoesNot` in each `parsers/*` module runs a corpus through both the grammar and the FallbackParser and fails on any rule the grammar reports and the fallback does not. It is an invariant over a corpus, not a proof: the shape that breaks it is a statement the grammar understands and the fallback's keyword list does not, which is exactly how every instance in #68 arose (`INSERT ... DEFAULT VALUES` and `UPSERT`, plain and CTE-prefixed, under `pgparser`; `REPLACE` and the `INTO`-less `INSERT t VALUES (...)` under `mysqlparser`). So **when a parser learns a statement kind, the fallback has to learn it too** — `detectKind` and `insertColumnsListed` are the pair to check — and a new structural field read by a rule needs a corpus row here. `detectKind` recognizes the insert-like keywords in two places, leading and after a `WITH` clause, and both require a table name after the keyword so a `REPLACE(str, from, to)` call is never read as the statement. `insertColumnsListed` anchors on the **statement head** whenever it starts the statement and falls back to the `INTO` clause only for the CTE-prefixed forms, where the keyword sits mid-statement and can also occur inside the CTE body. That order matters: searching for `INTO` first matches an identifier named `into` in the column list of a form that omits the keyword, reads the list as the target table, and reports a statement that does name its columns. Because the test compares a satellite module against the core's fallback, a corpus row depending on core support the published core lacks is skipped rather than failed, so `GOWORK=off` stays green between lockstep tags. **Rules self-register; config is resolved once, never per query.** Built-in rules call `analyzer.Register(RuleSpec{...})` from `init()` in `analyzer/rules.go` — a stable name, default severity, and a settings-aware `Factory`. To add a rule, write it and add one `Register` call; **do not** hand-maintain a rule list in `Default()`. Being addressable by name is what makes enable/disable, severity overrides, per-rule `Settings`, and suppressions work uniformly. `analyzer.Profile` (disabled set, `only` whitelist, severity map, per-rule settings) is applied in `DefaultWithProfile` at construction; the per-query `Analyze` path does no config work (it runs on every query through the driver — keep it allocation-light). `analyzer` must stay free of `config`/YAML imports. diff --git a/CHANGELOG.md b/CHANGELOG.md index 2020266..1f95152 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,7 +29,16 @@ the same version in lockstep. parsers already reported them while the fallback read them as an unrecognized statement kind and said nothing. `REPLACE(str, from, to)` is never mistaken for a statement, and a `REPLACE()` call inside a CTE does not - displace the real statement head. + displace the real statement head. A CTE prefix puts the keyword + mid-statement, past the leading-keyword check, so + `WITH c AS (…) UPSERT INTO t SELECT …` is covered too. +- **A column named `into` no longer defeats `insert-without-columns`.** The + target table was found by scanning for `INTO` anywhere in the statement, so + ``INSERT t (`into`) VALUES (1)`` — a form that omits the optional keyword — + had its column list read as the target table and was reported as having no + columns. The statement head is now the anchor whenever it starts the + statement; `INTO` remains the anchor for the CTE-prefixed forms, where the + keyword sits mid-statement. ### Added diff --git a/analyzer/analyzer_test.go b/analyzer/analyzer_test.go index 8c651f2..e3373ad 100644 --- a/analyzer/analyzer_test.go +++ b/analyzer/analyzer_test.go @@ -140,6 +140,13 @@ func TestCheckInsertWithoutColumns(t *testing.T) { {"replace function is not a statement", "SELECT REPLACE(name, 'a', 'b') FROM users", false}, {"replace function inside an insert", "INSERT INTO users (name) VALUES (REPLACE(x, 'a', 'b'))", false}, {"cte containing a replace call", "WITH s AS (SELECT REPLACE(a, 'x', 'y') AS n FROM u) INSERT INTO users SELECT n FROM s", true}, + {"cte upsert no columns", "WITH s AS (SELECT 1 AS n) UPSERT INTO users SELECT n FROM s", true}, + {"cte upsert with columns", "WITH s AS (SELECT 1 AS n) UPSERT INTO users (name) SELECT n FROM s", false}, + // A column named "into" must not be read as the INTO clause, which + // would make the column list look like the target table. + {"column named into without the keyword", "INSERT users (`into`) VALUES ('alice')", false}, + {"replace with a column named into", "REPLACE users (`into`) VALUES ('alice')", false}, + {"column named into with the keyword", "INSERT INTO users (`into`) VALUES ('alice')", false}, } for _, tt := range tests { diff --git a/analyzer/fallback.go b/analyzer/fallback.go index 6f06190..eaf9af4 100644 --- a/analyzer/fallback.go +++ b/analyzer/fallback.go @@ -55,7 +55,12 @@ var ( // inside a CTE body (a REPLACE() call in a WITH clause), and the INTO-less // forms are MySQL-only, where the CTE-prefixed shape needs INTO anyway. fbInsertHeadRe = regexp.MustCompile(`(?i)^\s*\(*\s*(?:INSERT|REPLACE|UPSERT)` + fbInsertModRe + `\b`) - fbDMLWordRe = regexp.MustCompile(`(?i)\b(INSERT|UPDATE|DELETE)\b`) + // fbInsertLikeWordRe is fbLeadInsertLikeRe without the start anchor, for + // the CTE-prefixed forms where the keyword follows the WITH clause. The + // table-name requirement is what keeps a REPLACE(str, from, to) call inside + // a CTE body from being read as the statement. + fbInsertLikeWordRe = regexp.MustCompile(`(?i)\b(?:REPLACE|UPSERT)` + fbInsertModRe + `\s+(?:INTO\s+)?[^\s(]`) + fbDMLWordRe = regexp.MustCompile(`(?i)\b(INSERT|UPDATE|DELETE)\b`) // fbWhereRegionEndRe marks the first clause keyword that ends the WHERE // region, so a function in ORDER BY / GROUP BY / HAVING isn't read as a @@ -160,19 +165,23 @@ func (p *FallbackParser) Parse(sql string) (*Statement, error) { // both count as listed (no positional column-order risk to warn about). // Comment-free, literal-blanked input expected; heuristic by contract. func insertColumnsListed(sanitized string) bool { - // INTO is the tightest anchor and the only one that survives a CTE prefix, - // where the statement keyword itself can appear inside the CTE body. Fall - // back to the statement head for the forms that omit it. + // The statement head is the anchor whenever it starts the statement. Trying + // INTO first instead reads an identifier named "into" — a column name in a + // form that omits the keyword — as the clause, which makes the column list + // look like the target table and reports the columns as unlisted. Whether + // the real INTO stays in the span does not matter: it holds no "(". var rest string - switch loc := fbIntoRe.FindStringIndex(sanitized); { - case loc != nil: - rest = sanitized[loc[1]:] + switch head := fbInsertHeadRe.FindStringIndex(sanitized); { + case head != nil: + rest = sanitized[head[1]:] default: - head := fbInsertHeadRe.FindStringIndex(sanitized) - if head == nil { - return true // no recognizable statement head — can't tell, don't flag + // A CTE prefix puts the keyword mid-statement, where it can also occur + // inside the CTE body, so INTO is the reliable anchor there. + loc := fbIntoRe.FindStringIndex(sanitized) + if loc == nil { + return true // no recognizable anchor — can't tell, don't flag } - rest = sanitized[head[1]:] + rest = sanitized[loc[1]:] } data := fbInsertDataRe.FindStringIndex(rest) if data == nil { @@ -502,6 +511,9 @@ func detectKind(sanitized string) StmtKind { return StmtDelete } } + if fbInsertLikeWordRe.MatchString(sanitized) { + return StmtInsert + } return StmtSelect } return StmtOther diff --git a/parsers/mysqlparser/mysqlparser_test.go b/parsers/mysqlparser/mysqlparser_test.go index ea5c8f1..ec61731 100644 --- a/parsers/mysqlparser/mysqlparser_test.go +++ b/parsers/mysqlparser/mysqlparser_test.go @@ -184,6 +184,10 @@ func TestParser_NeverAddsFindingTheFallbackDoesNot(t *testing.T) { "INSERT IGNORE INTO t VALUES (1)", "REPLACE LOW_PRIORITY INTO t VALUES (1)", "REPLACE DELAYED t VALUES (1)", + // A column named "into" in a statement that omits the INTO keyword: + // scanning for INTO anywhere reads the column list as the target table. + "INSERT t (`into`) VALUES (1)", + "REPLACE t (`into`) VALUES (1)", "INSERT INTO t VALUES (1), (2)", "SELECT REPLACE(name, 'a', 'b') FROM t", "SELECT * FROM t WHERE a = 'REPLACE INTO x VALUES (1)'", @@ -231,14 +235,18 @@ func TestParser_NeverAddsFindingTheFallbackDoesNot(t *testing.T) { } // needsCoreKeywordSupport reports whether a corpus entry's parity depends on -// the core recognizing a row-inserting keyword other than "INSERT INTO". +// the core recognizing a row-inserting keyword other than a leading +// "INSERT INTO". It matches on the keyword appearing anywhere, not just at the +// front, because a CTE prefix puts it mid-statement; an entry that merely +// mentions REPLACE as a function is skipped too, which costs nothing. func needsCoreKeywordSupport(sql string) bool { up := strings.ToUpper(strings.TrimSpace(sql)) - if strings.HasPrefix(up, "INSERT INTO ") { - return false + if strings.Contains(up, "REPLACE") || strings.Contains(up, "UPSERT") { + return true } - return strings.HasPrefix(up, "REPLACE") || strings.HasPrefix(up, "UPSERT") || - strings.HasPrefix(up, "INSERT") + // INSERT with the optional INTO omitted. + i := strings.Index(up, "INSERT") + return i >= 0 && !strings.HasPrefix(strings.TrimSpace(up[i+len("INSERT"):]), "INTO") } func ruleSet(rs []analyzer.Result) map[string]struct{} { diff --git a/parsers/pgparser/pgparser_test.go b/parsers/pgparser/pgparser_test.go index 21a852a..629d560 100644 --- a/parsers/pgparser/pgparser_test.go +++ b/parsers/pgparser/pgparser_test.go @@ -166,6 +166,11 @@ func TestParser_NeverAddsFindingTheFallbackDoesNot(t *testing.T) { // would not recognize the statement at all. "UPSERT INTO t VALUES (1)", "UPSERT INTO t (a) VALUES (1)", + // A CTE prefix puts the keyword mid-statement, past the leading-keyword + // check that classifies the plain form. + "WITH c AS (SELECT 1 AS n) UPSERT INTO t SELECT n FROM c", + "WITH c AS (SELECT 1 AS n) UPSERT INTO t (a) SELECT n FROM c", + "WITH c AS (SELECT replace(a, 'x', 'y') AS n FROM u) SELECT n FROM c", "INSERT INTO t VALUES (1) RETURNING id", "INSERT INTO t (a) VALUES (1) ON CONFLICT (a) DO NOTHING", "SELECT replace(name, 'a', 'b') FROM t", @@ -224,14 +229,18 @@ func TestParser_NeverAddsFindingTheFallbackDoesNot(t *testing.T) { } // needsCoreKeywordSupport reports whether a corpus entry's parity depends on -// the core recognizing a row-inserting keyword other than "INSERT INTO". +// the core recognizing a row-inserting keyword other than a leading +// "INSERT INTO". It matches on the keyword appearing anywhere, not just at the +// front, because a CTE prefix puts it mid-statement; an entry that merely +// mentions REPLACE as a function is skipped too, which costs nothing. func needsCoreKeywordSupport(sql string) bool { up := strings.ToUpper(strings.TrimSpace(sql)) - if strings.HasPrefix(up, "INSERT INTO ") { - return false + if strings.Contains(up, "REPLACE") || strings.Contains(up, "UPSERT") { + return true } - return strings.HasPrefix(up, "REPLACE") || strings.HasPrefix(up, "UPSERT") || - strings.HasPrefix(up, "INSERT") + // INSERT with the optional INTO omitted. + i := strings.Index(up, "INSERT") + return i >= 0 && !strings.HasPrefix(strings.TrimSpace(up[i+len("INSERT"):]), "INTO") } func ruleSet(rs []analyzer.Result) map[string]struct{} { diff --git a/website/docs/parsers.md b/website/docs/parsers.md index 450a889..0d5906d 100644 --- a/website/docs/parsers.md +++ b/website/docs/parsers.md @@ -76,21 +76,24 @@ The first group is the false-positive-prone set; those become exact regardless of the parser, and each field's doc comment says so. This is a documented carve-out, not a gap waiting to be closed. -**A dialect parser is meant to remove findings, never add them** — _0.5+_. -Opting in can drop a finding the heuristics guessed wrong; it should not -report one the fallback would not have. Each parser module pins this -direction against a corpus +_Added in 0.5._ **A dialect parser is meant to remove findings, never add +them.** Opting in can drop a finding the heuristics guessed wrong; it +should not report one the fallback would not have. Each parser module +pins this direction against a corpus (`TestParser_NeverAddsFindingTheFallbackDoesNot`), so a rule that reads a structural field the grammar fills differently from the fallback fails there rather than reaching you. That is a tested invariant over a corpus, not a proof: a dialect form neither the corpus nor the fallback's keyword list knows is how it would break again. -In 0.4 and earlier it did break, twice in the same rule. `pgparser` -reported `insert-without-columns` on `INSERT INTO t DEFAULT VALUES`, and -both grammars reported it on row-inserting keywords the fallback did not -recognize as inserts at all — `REPLACE`, `UPSERT`, and the `INTO`-less -`INSERT t VALUES (…)` MySQL accepts. +In 0.4 and earlier it did break, in one rule, on every statement the +grammar recognised as inserting rows and the fallback did not: + +- `pgparser` reported `insert-without-columns` on + `INSERT INTO t DEFAULT VALUES`, which names no columns and needs none, + and on `UPSERT INTO t VALUES (…)`, plain or behind a CTE. +- `mysqlparser` reported it on `REPLACE INTO t VALUES (…)` and on the + forms that omit MySQL's optional `INTO`, such as `INSERT t VALUES (…)`. ## Degradation on parse failure diff --git a/website/docs/rules.md b/website/docs/rules.md index 3d10e24..f9f8996 100644 --- a/website/docs/rules.md +++ b/website/docs/rules.md @@ -177,9 +177,11 @@ Same as above for `UPDATE`. to the table's column order. Adding, dropping or reordering a column silently shifts every value. -Every keyword that inserts rows this way is flagged, not just `INSERT` — -MySQL/SQLite's `REPLACE`, the `UPSERT` CockroachDB accepts, and the forms -that omit the optional `INTO` (`INSERT t VALUES (…)`) — _0.5+_. +_Changed in 0.5._ Every keyword that inserts rows this way is flagged, not +just `INSERT`: MySQL/SQLite's `REPLACE`, the `UPSERT` CockroachDB accepts, +and the forms that omit the optional `INTO` (`INSERT t VALUES (…)`). +Previously only a leading `INSERT INTO` was recognised, so the others were +reported solely to users who had opted into a dialect parser. Forms that name their columns, or have none to name, are not flagged: MySQL's `INSERT INTO t SET col = …`, and PostgreSQL's