diff --git a/AGENTS.md b/AGENTS.md index 21eb737..e8a3f5b 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 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. **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. diff --git a/CHANGELOG.md b/CHANGELOG.md index dfe8143..1f95152 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,60 @@ 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). +- **`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. 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 + +- **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 + 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 + ## [0.4.0] - 2026-09-25 ### Changed diff --git a/analyzer/analyzer_test.go b/analyzer/analyzer_test.go index 76419c8..e3373ad 100644 --- a/analyzer/analyzer_test.go +++ b/analyzer/analyzer_test.go @@ -119,6 +119,34 @@ 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}, + // 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}, + {"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 a5e1411..eaf9af4 100644 --- a/analyzer/fallback.go +++ b/analyzer/fallback.go @@ -37,7 +37,30 @@ 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`) + // 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`) + // 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 @@ -142,11 +165,24 @@ 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 + // 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 head := fbInsertHeadRe.FindStringIndex(sanitized); { + case head != nil: + rest = sanitized[head[1]:] + default: + // 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[loc[1]:] } - rest := sanitized[loc[1]:] data := fbInsertDataRe.FindStringIndex(rest) if data == nil { return true // no recognizable data clause — don't flag @@ -448,6 +484,9 @@ func parenDepthBefore(s string, idx int) int { func detectKind(sanitized string) StmtKind { m := fbLeadKindRe.FindStringSubmatch(sanitized) if m == nil { + if fbLeadInsertLikeRe.MatchString(sanitized) { + return StmtInsert + } return StmtOther } switch strings.ToUpper(m[1]) { @@ -472,6 +511,9 @@ func detectKind(sanitized string) StmtKind { return StmtDelete } } + if fbInsertLikeWordRe.MatchString(sanitized) { + return StmtInsert + } return StmtSelect } 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 abb1271..ec61731 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" @@ -149,3 +150,122 @@ 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). +// +// 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 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)", + // 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)", + // 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)'", + "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()) + 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("%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 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.Contains(up, "REPLACE") || strings.Contains(up, "UPSERT") { + return true + } + // 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{} { + out := make(map[string]struct{}, len(rs)) + for _, r := range rs { + out[r.RuleName] = 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.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..629d560 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" @@ -53,6 +54,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 +143,123 @@ 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). +// +// 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)", + // 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", + "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()) + 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("%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 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.Contains(up, "REPLACE") || strings.Contains(up, "UPSERT") { + return true + } + // 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{} { + out := make(map[string]struct{}, len(rs)) + for _, r := range rs { + out[r.RuleName] = 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 102688f..0d5906d 100644 --- a/website/docs/parsers.md +++ b/website/docs/parsers.md @@ -76,6 +76,25 @@ 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. +_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, 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 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..f9f8996 100644 --- a/website/docs/rules.md +++ b/website/docs/rules.md @@ -177,6 +177,17 @@ Same as above for `UPDATE`. to the table's column order. Adding, dropping or reordering a column silently shifts every value. +_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 +`INSERT INTO t DEFAULT VALUES`, which writes no caller-supplied values at +all. + > **Fix:** Specify columns explicitly: `INSERT INTO table (col1, col2) VALUES (…)`. ### `select-without-limit`