From 027d91d8a0ca8b2a5de65c642493d3f870324a43 Mon Sep 17 00:00:00 2001 From: kartik Date: Fri, 25 Sep 2026 06:59:26 +0530 Subject: [PATCH 01/11] feat(config): make every documented rule addressable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The rules reference lists 21 rules, but only the 14 statement rules were registered. Naming `slow-query`, `n-plus-one`, `seq-scan`, `high-cost`, `full-table-scan`, `no-index-used` or `filesort` under `disable`, `only`, `severity` or `settings` warned with `unknown rule` — a hard error under `strict: true` — on a config the docs' own table invites you to write. There was also no way at all to turn slow-query off, or to move it off WARNING. A RuleSpec may now carry a nil Factory: it registers a name without a rule body. Those seven use it. They cannot be evaluated against a parsed Statement — middleware derives two from latency and fingerprint counts, explain derives five from the database's own plan — but they are documented as rules, so they register to be addressable, and the profile decision reaches whoever builds the finding. Guard resolves its two once in NewGuard, keeping the per-query path config-free; explain filters centrally in applyProfile rather than at each of the five sites that build one, so a plan rule added later cannot forget the check. All seven register in analyzer/rules.go rather than 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 does not link that package. Two consequences worth stating. `explain` now honors `rules:`, which reverses a documented "config does not affect explain" stance — the docs are updated. And `only:` is a whitelist across every surface now, so a list that omits slow-query turns it off. Breaking: the slow-query threshold moves from the top-level `slow-query.threshold` key to `rules.settings.slow-query.threshold`, so every tunable lives in one place; Config.SlowQueryThreshold and SlowQueryConfig go with it. Setting both `n-plus-one.threshold` and `.window` now enables N+1 from a file, which was previously reachable only from Go. An explicit Go option outranks the file in both cases. Settings.Duration falls back to the caller's default on an unparseable value, which would have turned a typo into a silently wrong threshold, so config validates duration-shaped settings up front. --- .sqlguard.example.yml | 18 +++- AGENTS.md | 2 + CHANGELOG.md | 24 +++++ analyzer/analyzer.go | 53 +++++++++- analyzer/registry.go | 11 +- analyzer/registry_test.go | 76 ++++++++++++++ analyzer/rules.go | 24 +++++ cmd/sqlguard/explain.go | 12 ++- config/config.go | 94 ++++++++++++------ config/config_test.go | 78 ++++++++++++++- config/middleware.go | 17 ++-- explain/explain.go | 38 +++++++ explain/rule_profile_test.go | 84 ++++++++++++++++ middleware/guard.go | 46 ++++++++- middleware/n_plus_one.go | 10 +- middleware/n_plus_one_test.go | 10 +- middleware/options.go | 28 ++++-- middleware/rule_profile_test.go | 171 ++++++++++++++++++++++++++++++++ website/docs/configuration.md | 35 ++++--- website/docs/explain.md | 16 ++- website/docs/middleware.md | 4 +- website/docs/rules.md | 10 ++ website/docs/suppressions.md | 4 +- 23 files changed, 772 insertions(+), 93 deletions(-) create mode 100644 analyzer/registry_test.go create mode 100644 explain/rule_profile_test.go create mode 100644 middleware/rule_profile_test.go diff --git a/.sqlguard.example.yml b/.sqlguard.example.yml index 556d714..5ed7bcb 100644 --- a/.sqlguard.example.yml +++ b/.sqlguard.example.yml @@ -42,6 +42,20 @@ rules: # Parameterized offsets (OFFSET $1 / ?) can't be evaluated statically. threshold: 1000 + # Runtime (middleware/integrations). These two rules are not evaluated + # against parsed SQL, so they never fire during `sqlguard scan` — but they + # are ordinary rule names, so `disable`, `only` and `severity` above reach + # them too. + slow-query: + # Flag a query whose driver-measured latency reaches this. Default + # 200ms. WithSlowQueryThreshold in Go wins over this. + threshold: 200ms + n-plus-one: + # Setting BOTH of these turns N+1 detection on; it is off otherwise. + # WithN1Detection in Go wins over this. + threshold: 10 # this many executions of one fingerprint... + window: 1m # ...within this window + # Redact literal values (strings/numbers) out of Result.Query before it # reaches any reporter/log. ON by default — leave it on so customer data in # query literals never lands in your logs. Result.Fingerprint (a PII-free, @@ -49,10 +63,6 @@ rules: # local debugging where the query text is trusted. redact: true -# Runtime slow-query threshold (middleware). Go duration string. -slow-query: - threshold: 200ms - # Runtime de-duplication of repeated static findings (middleware). The same # finding (rule + query fingerprint) is reported at most once per window, so a # recurring query doesn't flood your logs. Default 1m. Set "0" to disable diff --git a/AGENTS.md b/AGENTS.md index 5a96453..7c9455f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -47,6 +47,8 @@ When changing the public API or Go version, update all nine `go.mod` files and ` **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 — `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.checkDurationSettings` validates duration-shaped settings because `Settings.Duration` falls back to the caller's default on a bad value, which would otherwise turn a typo into a silently wrong threshold. + **`config` is the only YAML-aware package.** It loads `.sqlguard.yml` (`Load`/`Discover` walks up to the git root), translates it to an `analyzer.Profile`, and exposes `MiddlewareOptions()`/`Middleware()` helpers. It depends on `analyzer` (and `middleware` for the helper); nothing depends on `config`. This keeps `gopkg.in/yaml.v3` out of the `analyzer`/`middleware` import graph for library users who don't opt into file config. Parsing is lenient by default (unknown keys/rules warn); `strict: true` makes them fatal — so a newer config still loads on an older binary. **Suppression has two layers** (`analyzer/suppress.go`): in-SQL `-- sqlguard:ignore` / `/* sqlguard:ignore:rule-a,rule-b */`, parsed from raw SQL with a **marker-anchored** regex (avoids string-literal false positives), honored at runtime _and_ statically; and Go-source `// sqlguard:ignore[:rules]` via `ParseIgnoreComment`, which uses a **separate marker-less** regex because go/ast already strips the `//`. The scanner (`cmd/sqlguard/scan.go`) applies it against the AST comment map for the call line and the line directly above. diff --git a/CHANGELOG.md b/CHANGELOG.md index 41caa01..949c176 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,30 @@ the same version in lockstep. ### Changed +- **Every documented rule is now addressable in `.sqlguard.yml`.** The rules + reference lists 21 rules, but only the 14 statement rules were registered: + naming `slow-query`, `n-plus-one`, `seq-scan`, `high-cost`, + `full-table-scan`, `no-index-used` or `filesort` under `disable`, `only`, + `severity` or `settings` warned with `unknown rule`, and failed outright + under `strict: true`. All seven are registered now, and the profile reaches + the runtime findings in middleware and the plan findings in `explain` + exactly as it reaches a statement rule. They are still never evaluated + against parsed SQL, so they do not fire during `sqlguard scan`. +- **Breaking (config): the slow-query threshold moved** from the top-level + `slow-query.threshold` key to `rules.settings.slow-query.threshold`, so + every per-rule tunable lives in one place. `Config.SlowQueryThreshold` and + the `SlowQueryConfig` type are gone with it. An explicit + `WithSlowQueryThreshold` in Go still wins over the file. +- **`explain` now honors `rules:` config.** It previously ignored it by + design, which is what its docs said. A `severity` override also beats + `seq-scan`'s row-count-derived severity. +- **N+1 detection can be enabled from the config file.** Setting both + `rules.settings.n-plus-one.threshold` and `.window` turns it on; it was + previously reachable only from Go via `WithN1Detection`, which still takes + precedence. +- A duration in `rules.settings` that does not parse is now reported instead + of being silently replaced by the built-in default. + - README restructured as a landing page: logo, "Why sqlguard?" comparison, quick start, and a guide index pointing at the docs site. The deep per-feature sections moved to the site. diff --git a/analyzer/analyzer.go b/analyzer/analyzer.go index 64356c8..fa5d71d 100644 --- a/analyzer/analyzer.go +++ b/analyzer/analyzer.go @@ -34,6 +34,12 @@ type Analyzer struct { rules []boundRule parser Parser severity map[string]Severity + // disabled is the resolved skip decision for every registered rule, + // including those this Analyzer does not run — see RuleEnabled. + disabled map[string]bool + // settings holds per-rule tunables for the same audience; the statement + // rules have theirs baked in by their factory at construction. + settings map[string]Settings // rawQuery, when true, leaves Result.Query unredacted. Default is false // (redact): the safe default for a tool whose findings flow into logs. rawQuery bool @@ -104,8 +110,15 @@ func Default() *Analyzer { // without analyzer ever importing config or YAML. func DefaultWithProfile(p Profile) *Analyzer { var bound []boundRule + disabled := make(map[string]bool, len(specs())) for _, spec := range specs() { if p.skip(spec.Name) { + disabled[spec.Name] = true + continue + } + // Registered for addressability only — middleware and explain build + // these findings themselves and consult the decisions below. + if !spec.Evaluated() { continue } bound = append(bound, boundRule{ @@ -120,9 +133,47 @@ func DefaultWithProfile(p Profile) *Analyzer { sev = make(map[string]Severity, len(p.Severity)) maps.Copy(sev, p.Severity) } - return &Analyzer{rules: bound, parser: NewFallbackParser(), severity: sev, rawQuery: p.RawQuery} + var settings map[string]Settings + if len(p.Settings) > 0 { + settings = make(map[string]Settings, len(p.Settings)) + maps.Copy(settings, p.Settings) + } + return &Analyzer{ + rules: bound, + parser: NewFallbackParser(), + severity: sev, + disabled: disabled, + settings: settings, + rawQuery: p.RawQuery, + } } +// RuleEnabled reports whether the active profile leaves the named rule on. It +// answers for every registered rule, including the ones the Analyzer does not +// evaluate: middleware asks before emitting `slow-query` or `n-plus-one`, and +// explain asks before emitting a plan finding, so `disable` and `only` mean +// the same thing across all three surfaces. +// +// An unregistered name is reported as enabled: an Analyzer built with New has +// no profile, and a caller's own rule is not the profile's to turn off. +func (a *Analyzer) RuleEnabled(name string) bool { return !a.disabled[name] } + +// RuleSeverity returns the severity to report for name, applying a profile +// override to def when one is set. +func (a *Analyzer) RuleSeverity(name string, def Severity) Severity { + if a.severity != nil { + if s, has := a.severity[name]; has { + return s + } + } + return def +} + +// RuleSettings returns the profile settings for name, or nil when none were +// configured. Settings.Int / .Duration treat a nil Settings as "use the +// default", so a caller can read straight through without a nil check. +func (a *Analyzer) RuleSettings(name string) Settings { return a.settings[name] } + // Analyze parses the query once and runs all rules against it. If the // configured parser returns an error, it degrades to the FallbackParser so // analysis never breaks the caller's query path. Findings for rules named in diff --git a/analyzer/registry.go b/analyzer/registry.go index 12fe055..2a31a35 100644 --- a/analyzer/registry.go +++ b/analyzer/registry.go @@ -81,9 +81,18 @@ func (s Settings) Duration(key string, def time.Duration) time.Duration { type RuleSpec struct { Name string DefaultSeverity Severity - Factory func(Settings) Rule + // Factory builds the rule for the statement path. It is nil for findings + // the Analyzer does not produce itself — middleware's `slow-query` and + // `n-plus-one`, and the plan rules `explain` derives from a query plan. + // Those register so they are addressable by name like any other rule; + // their owners ask the Analyzer for the resolved decision before they + // emit. A nil Factory is never called. + Factory func(Settings) Rule } +// Evaluated reports whether the Analyzer builds and runs this rule itself. +func (s RuleSpec) Evaluated() bool { return s.Factory != nil } + var ( registryMu sync.RWMutex registry = map[string]RuleSpec{} diff --git a/analyzer/registry_test.go b/analyzer/registry_test.go new file mode 100644 index 0000000..a38e013 --- /dev/null +++ b/analyzer/registry_test.go @@ -0,0 +1,76 @@ +package analyzer + +import ( + "slices" + "testing" +) + +// TestNonEvaluatedRulesAreRegisteredButNotRun pins both halves of the +// arrangement: the runtime and plan findings must be addressable by name so +// config can disable or re-severity them, while never being constructed or +// run against a Statement — they have no Factory and nothing to evaluate. +func TestNonEvaluatedRulesAreRegisteredButNotRun(t *testing.T) { + nonEvaluated := []string{ + "slow-query", "n-plus-one", + "seq-scan", "high-cost", "full-table-scan", "no-index-used", "filesort", + } + + names := RuleNames() + for _, want := range nonEvaluated { + if !slices.Contains(names, want) { + t.Errorf("%q is not registered, so config cannot address it", want) + } + } + + a := Default() + for _, br := range a.rules { + if slices.Contains(nonEvaluated, br.name) { + t.Errorf("%q was built as a statement rule; it has nothing to evaluate", br.name) + } + } + + // A query that trips a statement rule must not gain phantom findings. + for _, r := range a.Analyze("SELECT * FROM users") { + if slices.Contains(nonEvaluated, r.RuleName) { + t.Errorf("Analyze produced %q, which it cannot evaluate", r.RuleName) + } + } +} + +// TestRuleEnabledAnswersForEveryRegisteredRule is what middleware and explain +// depend on: a name they never ask the Analyzer to run must still get an +// honest enabled/disabled answer. +func TestRuleEnabledAnswersForEveryRegisteredRule(t *testing.T) { + a := DefaultWithProfile(Profile{Disabled: map[string]bool{"slow-query": true, "seq-scan": true}}) + + if a.RuleEnabled("slow-query") { + t.Error("slow-query should be reported disabled") + } + if a.RuleEnabled("seq-scan") { + t.Error("seq-scan should be reported disabled") + } + if !a.RuleEnabled("n-plus-one") { + t.Error("n-plus-one was not disabled and should stay enabled") + } + if !a.RuleEnabled("select-star") { + t.Error("select-star was not disabled and should stay enabled") + } + // An unknown name is not the profile's to turn off. + if !a.RuleEnabled("somebody-elses-rule") { + t.Error("an unregistered rule should be reported enabled") + } +} + +// TestOnlyWhitelistReachesNonEvaluatedRules covers the semantic `only` now +// carries: it is a whitelist across every surface, so a runtime or plan +// finding not named in it is off. +func TestOnlyWhitelistReachesNonEvaluatedRules(t *testing.T) { + a := DefaultWithProfile(Profile{Only: map[string]bool{"select-star": true}}) + + if a.RuleEnabled("slow-query") { + t.Error("an `only` whitelist should exclude slow-query") + } + if !a.RuleEnabled("select-star") { + t.Error("the whitelisted rule should stay enabled") + } +} diff --git a/analyzer/rules.go b/analyzer/rules.go index 872df4e..bc4466c 100644 --- a/analyzer/rules.go +++ b/analyzer/rules.go @@ -267,3 +267,27 @@ func CheckOrderByWithoutLimit(s *Statement) (Result, bool) { } return Result{}, false } + +// Findings produced outside the statement path register here too, with no +// Factory. `slow-query` and `n-plus-one` are built by middleware from latency +// and fingerprint counts; the five plan rules are derived by `explain` from a +// database's own EXPLAIN output. None of them can be evaluated against a +// parsed Statement, but every one of them is documented as a rule and is +// expected to answer to `disable`, `only`, `severity` and `settings` the same +// way the statement rules do. +// +// They are registered here rather than in middleware/ and explain/ so that +// RuleNames() is complete for whoever asks. The config loader validates +// against it while importing only analyzer, so registering `seq-scan` from +// explain's init would make a config naming it fail for anyone who does not +// link that package. +func init() { + Register(RuleSpec{Name: "slow-query", DefaultSeverity: SeverityWarning}) + Register(RuleSpec{Name: "n-plus-one", DefaultSeverity: SeverityWarning}) + + Register(RuleSpec{Name: "seq-scan", DefaultSeverity: SeverityInfo}) + Register(RuleSpec{Name: "high-cost", DefaultSeverity: SeverityWarning}) + Register(RuleSpec{Name: "full-table-scan", DefaultSeverity: SeverityWarning}) + Register(RuleSpec{Name: "no-index-used", DefaultSeverity: SeverityWarning}) + Register(RuleSpec{Name: "filesort", DefaultSeverity: SeverityInfo}) +} diff --git a/cmd/sqlguard/explain.go b/cmd/sqlguard/explain.go index 5d44c63..91d1d7f 100644 --- a/cmd/sqlguard/explain.go +++ b/cmd/sqlguard/explain.go @@ -56,7 +56,17 @@ func runExplain(cmd *cobra.Command, args []string) error { } defer func() { _ = db.Close() }() - var explainOpts []explain.Option + cfg, err := resolveConfig(".") + if err != nil { + return err + } + a, err := cfg.Analyzer() + if err != nil { + return err + } + printConfigWarnings(cfg) + + explainOpts := []explain.Option{explain.WithAnalyzer(a)} if explainAllowDML { explainOpts = append(explainOpts, explain.WithAllowDML()) } diff --git a/config/config.go b/config/config.go index 736cbb3..ae3ded5 100644 --- a/config/config.go +++ b/config/config.go @@ -29,12 +29,11 @@ var ConfigFileNames = []string{".sqlguard.yml", ".sqlguard.yaml"} // forward compatibility: older binaries reading a newer config degrade with // warnings rather than failing, unless Strict is set. type Config struct { - Version int `yaml:"version"` - Strict bool `yaml:"strict"` - Rules RulesConfig `yaml:"rules"` - SlowQuery SlowQueryConfig `yaml:"slow-query"` - Dedup DedupConfig `yaml:"dedup"` - Scan ScanConfig `yaml:"scan"` + Version int `yaml:"version"` + Strict bool `yaml:"strict"` + Rules RulesConfig `yaml:"rules"` + Dedup DedupConfig `yaml:"dedup"` + Scan ScanConfig `yaml:"scan"` // Redact controls Result.Query literal redaction. Pointer so an unset // key means "use the safe default" (redact). Set `redact: false` only // when the query text is trusted (local debugging). @@ -57,12 +56,6 @@ type RulesConfig struct { Settings map[string]map[string]any `yaml:"settings"` } -// SlowQueryConfig configures the middleware slow-query threshold. -type SlowQueryConfig struct { - // Threshold is a Go duration string, e.g. "200ms". - Threshold string `yaml:"threshold"` -} - // DedupConfig configures runtime suppression of repeated static findings. type DedupConfig struct { // Window is a Go duration string, e.g. "1m". The same finding (rule + @@ -206,14 +199,39 @@ func (c *Config) Profile() (analyzer.Profile, error) { } p.Only[name] = true } - for name, sevStr := range c.Rules.Severity { + if err := applySeverities(c.Rules.Severity, &p, checkName, warn); err != nil { + return p, err + } + for name, kv := range c.Rules.Settings { if err := checkName(name); err != nil { return p, err } + if err := checkDurationSettings(name, kv, warn); err != nil { + return p, err + } + p.Settings[name] = analyzer.Settings(kv) + } + return p, nil +} + +// applySeverities resolves the `rules.severity` map onto the profile. A +// severity of "off" disables the rule rather than setting one, which is what +// makes `severity: {slow-query: off}` equivalent to listing it under +// `disable`. +func applySeverities( + sevs map[string]string, + p *analyzer.Profile, + checkName func(string) error, + warn func(string, ...any) error, +) error { + for name, sevStr := range sevs { + if err := checkName(name); err != nil { + return err + } sev, off, ok := parseSeverity(sevStr) if !ok { if err := warn("rule %q: invalid severity %q", name, sevStr); err != nil { - return p, err + return err } continue } @@ -223,13 +241,37 @@ func (c *Config) Profile() (analyzer.Profile, error) { } p.Severity[name] = sev } - for name, kv := range c.Rules.Settings { - if err := checkName(name); err != nil { - return p, err + return nil +} + +// durationSettings names the per-rule settings parsed as Go durations, so a +// malformed value is reported rather than silently replaced by a default. +var durationSettings = map[string][]string{ + "slow-query": {"threshold"}, + "n-plus-one": {"window"}, +} + +// checkDurationSettings reports a duration setting that will not parse. +// analyzer.Settings.Duration falls back to the caller's default on a bad +// value, so without this a typo would quietly leave the built-in threshold in +// place rather than the one the file asked for. +func checkDurationSettings(rule string, kv map[string]any, warn func(string, ...any) error) error { + for _, key := range durationSettings[rule] { + v, present := kv[key] + if !present { + continue + } + str, isStr := v.(string) + if !isStr { + continue // a bare number is milliseconds; Settings handles it + } + if _, err := time.ParseDuration(strings.TrimSpace(str)); err != nil { + if err := warn("rule %q: setting %q: invalid duration %q", rule, key, str); err != nil { + return err + } } - p.Settings[name] = analyzer.Settings(kv) } - return p, nil + return nil } // rawQuery reports whether Result.Query redaction is disabled. Redaction is @@ -248,20 +290,6 @@ func (c *Config) Analyzer() (*analyzer.Analyzer, error) { return analyzer.DefaultWithProfile(p), nil } -// SlowQueryThreshold returns the configured slow-query threshold. ok is false -// when unset, in which case the caller keeps its own default. -func (c *Config) SlowQueryThreshold() (d time.Duration, ok bool, err error) { - s := strings.TrimSpace(c.SlowQuery.Threshold) - if s == "" { - return 0, false, nil - } - d, err = time.ParseDuration(s) - if err != nil { - return 0, false, fmt.Errorf("sqlguard config: slow-query.threshold %q: %w", s, err) - } - return d, true, nil -} - // DedupWindow returns the configured static-finding dedup window. ok is false // when unset, in which case the middleware keeps its own default. A configured // "0" returns ok=true with d=0, which disables dedup (report every occurrence). diff --git a/config/config_test.go b/config/config_test.go index 42ecdda..660f292 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -30,8 +30,8 @@ rules: settings: leading-wildcard: min-length: 4 -slow-query: - threshold: 350ms + slow-query: + threshold: 350ms `) c, err := Load(p) if err != nil { @@ -55,9 +55,8 @@ slow-query: t.Error("min-length setting not carried into profile") } - d, ok, err := c.SlowQueryThreshold() - if err != nil || !ok || d != 350*time.Millisecond { - t.Errorf("SlowQueryThreshold = %v, %v, %v; want 350ms,true,nil", d, ok, err) + if d := prof.Settings["slow-query"].Duration("threshold", 0); d != 350*time.Millisecond { + t.Errorf("slow-query threshold = %v, want 350ms", d) } // End-to-end: the built analyzer respects the profile. @@ -195,3 +194,72 @@ func TestExcludeMatcher(t *testing.T) { t.Error("no patterns should yield a nil matcher") } } + +// TestProfile_AcceptsNonEvaluatedRules pins the bug this fixes: `slow-query`, +// `n-plus-one` and the five plan rules are documented in the same reference +// table as the statement rules, but were not registered, so naming any of +// them warned in lenient mode and failed outright under `strict: true`. +func TestProfile_AcceptsNonEvaluatedRules(t *testing.T) { + names := []string{ + "slow-query", "n-plus-one", + "seq-scan", "high-cost", "full-table-scan", "no-index-used", "filesort", + } + for _, name := range names { + t.Run(name, func(t *testing.T) { + c := &Config{ + Strict: true, + Rules: RulesConfig{ + Disable: []string{name}, + Severity: map[string]string{name: "critical"}, + }, + } + p, err := c.Profile() + if err != nil { + t.Fatalf("strict config naming %q failed: %v", name, err) + } + if !p.Disabled[name] { + t.Errorf("%q not carried into Profile.Disabled", name) + } + if len(c.Warnings()) != 0 { + t.Errorf("unexpected warnings: %v", c.Warnings()) + } + }) + } +} + +// TestProfile_ValidatesDurationSettings guards a trap introduced by moving the +// threshold into settings: analyzer.Settings.Duration falls back to the +// caller's default when a value does not parse, so without this check a typo +// would silently leave the built-in 200ms in place. +func TestProfile_ValidatesDurationSettings(t *testing.T) { + t.Run("strict fails", func(t *testing.T) { + c := &Config{Strict: true, Rules: RulesConfig{Settings: map[string]map[string]any{ + "slow-query": {"threshold": "200mss"}, + }}} + if _, err := c.Profile(); err == nil { + t.Fatal("expected an error for an unparseable duration") + } + }) + + t.Run("lenient warns", func(t *testing.T) { + c := &Config{Rules: RulesConfig{Settings: map[string]map[string]any{ + "n-plus-one": {"window": "1minute"}, + }}} + if _, err := c.Profile(); err != nil { + t.Fatalf("lenient mode should not fail: %v", err) + } + if len(c.Warnings()) != 1 { + t.Errorf("expected one warning, got %v", c.Warnings()) + } + }) + + t.Run("valid duration and bare number pass", func(t *testing.T) { + c := &Config{Strict: true, Rules: RulesConfig{Settings: map[string]map[string]any{ + "slow-query": {"threshold": "1s"}, + "n-plus-one": {"window": 500, "threshold": 10}, + }}} + if _, err := c.Profile(); err != nil { + t.Fatalf("valid settings rejected: %v", err) + } + }) +} diff --git a/config/middleware.go b/config/middleware.go index 439d91d..2b7d343 100644 --- a/config/middleware.go +++ b/config/middleware.go @@ -5,8 +5,9 @@ import ( ) // MiddlewareOptions translates this config into middleware options: an -// analyzer built from the rule Profile, and the slow-query threshold when -// configured. Combine with other middleware options as needed, e.g.: +// analyzer built from the rule Profile, which carries the rule settings +// (including `slow-query.threshold` and the `n-plus-one` tunables), plus the +// dedup window. Combine with other middleware options as needed, e.g.: // // opts, _ := cfg.MiddlewareOptions() // opts = append(opts, middleware.WithParser(pgparser.New())) @@ -19,16 +20,12 @@ func (c *Config) MiddlewareOptions() ([]middleware.Option, error) { if err != nil { return nil, err } + // The slow-query threshold and the N+1 threshold/window travel inside the + // analyzer's profile as rule settings, so they need no option of their + // own here — NewGuard reads them unless a Go option names them + // explicitly. opts := []middleware.Option{middleware.WithAnalyzer(a)} - d, ok, err := c.SlowQueryThreshold() - if err != nil { - return nil, err - } - if ok { - opts = append(opts, middleware.WithSlowQueryThreshold(d)) - } - dw, ok, err := c.DedupWindow() if err != nil { return nil, err diff --git a/explain/explain.go b/explain/explain.go index aa3c08a..38512d6 100644 --- a/explain/explain.go +++ b/explain/explain.go @@ -20,6 +20,11 @@ type PlanAnalyzer struct { db *sql.DB dialect string // "postgres" or "mysql" allowDML bool + // rules carries the resolved rule profile. The plan rules are registered + // in the analyzer purely to be addressable, so `disable`, `only` and + // `severity` reach them exactly as they reach a statement rule. Nil means + // no configuration: every plan rule fires at its built-in severity. + rules *analyzer.Analyzer } // Option configures a PlanAnalyzer. @@ -35,6 +40,17 @@ func WithAllowDML() Option { return func(p *PlanAnalyzer) { p.allowDML = true } } +// WithAnalyzer supplies the configured analyzer whose rule profile governs +// which plan findings are reported and at what severity. The CLI passes the +// one built from .sqlguard.yml; a library caller can pass +// analyzer.DefaultWithProfile(p). +// +// `explain` does not run the statement rules — it only borrows the profile +// decisions, so the same `disable: [seq-scan]` works on every surface. +func WithAnalyzer(a *analyzer.Analyzer) Option { + return func(p *PlanAnalyzer) { p.rules = a } +} + // New creates a PlanAnalyzer for the given database connection. // dialect must be "postgres" or "mysql". func New(db *sql.DB, dialect string, opts ...Option) (*PlanAnalyzer, error) { @@ -79,6 +95,7 @@ func (p *PlanAnalyzer) Analyze(ctx context.Context, query string) (*Result, erro return nil, fmt.Errorf("explain: unsupported dialect %q", p.dialect) } if res != nil { + res.Issues = p.applyProfile(res.Issues) fp := analyzer.Fingerprint(query) for i := range res.Issues { res.Issues[i].Fingerprint = fp @@ -87,6 +104,27 @@ func (p *PlanAnalyzer) Analyze(ctx context.Context, query string) (*Result, erro return res, err } +// applyProfile drops findings the profile disabled and applies any severity +// override. It runs once over the collected issues rather than at each site +// that builds one, so a plan rule added later cannot forget the check. +// +// A severity override wins over a computed severity: `seq-scan` picks INFO or +// WARNING from the estimated row count, and an explicit setting outranks both. +func (p *PlanAnalyzer) applyProfile(issues []analyzer.Result) []analyzer.Result { + if p.rules == nil || len(issues) == 0 { + return issues + } + kept := issues[:0] + for _, r := range issues { + if !p.rules.RuleEnabled(r.RuleName) { + continue + } + r.Severity = p.rules.RuleSeverity(r.RuleName, r.Severity) + kept = append(kept, r) + } + return kept +} + // validate enforces the EXPLAIN safety policy and returns the single, // terminator-stripped statement that is safe to concatenate into an EXPLAIN // prefix. diff --git a/explain/rule_profile_test.go b/explain/rule_profile_test.go new file mode 100644 index 0000000..fb78ae9 --- /dev/null +++ b/explain/rule_profile_test.go @@ -0,0 +1,84 @@ +package explain + +import ( + "testing" + + "github.com/KARTIKrocks/sqlguard/analyzer" +) + +// The five plan rules are registered in the analyzer purely so they are +// addressable; explain borrows the resolved decisions. These pin that +// `disable`, `only` and `severity` reach a plan finding the same way they +// reach a statement rule. + +func planIssues() []analyzer.Result { + return []analyzer.Result{ + {RuleName: "seq-scan", Severity: analyzer.SeverityInfo, Message: "seq"}, + {RuleName: "high-cost", Severity: analyzer.SeverityWarning, Message: "cost"}, + {RuleName: "filesort", Severity: analyzer.SeverityInfo, Message: "sort"}, + } +} + +func TestApplyProfile_NilLeavesIssuesAlone(t *testing.T) { + p := &PlanAnalyzer{} + got := p.applyProfile(planIssues()) + if len(got) != 3 { + t.Errorf("an unconfigured analyzer should not filter: %+v", got) + } +} + +func TestApplyProfile_Disable(t *testing.T) { + p := &PlanAnalyzer{rules: analyzer.DefaultWithProfile(analyzer.Profile{ + Disabled: map[string]bool{"seq-scan": true, "filesort": true}, + })} + + got := p.applyProfile(planIssues()) + + if len(got) != 1 || got[0].RuleName != "high-cost" { + t.Errorf("expected only high-cost to survive, got %+v", got) + } +} + +func TestApplyProfile_Only(t *testing.T) { + p := &PlanAnalyzer{rules: analyzer.DefaultWithProfile(analyzer.Profile{ + Only: map[string]bool{"filesort": true}, + })} + + got := p.applyProfile(planIssues()) + + if len(got) != 1 || got[0].RuleName != "filesort" { + t.Errorf("an `only` whitelist should leave just filesort, got %+v", got) + } +} + +// TestApplyProfile_SeverityOverridesComputed matters for seq-scan, whose +// severity is derived from the estimated row count. An explicit setting has +// to outrank that, not be outranked by it. +func TestApplyProfile_SeverityOverridesComputed(t *testing.T) { + p := &PlanAnalyzer{rules: analyzer.DefaultWithProfile(analyzer.Profile{ + Severity: map[string]analyzer.Severity{"seq-scan": analyzer.SeverityCritical}, + })} + + issues := []analyzer.Result{ + {RuleName: "seq-scan", Severity: analyzer.SeverityWarning}, // computed from PlanRows > 1000 + } + got := p.applyProfile(issues) + + if len(got) != 1 || got[0].Severity != analyzer.SeverityCritical { + t.Errorf("the override should beat the row-count severity, got %+v", got) + } +} + +func TestApplyProfile_SeverityOffDisables(t *testing.T) { + // config translates `severity: off` into Disabled, so this is the shape + // explain receives for an "off" plan rule. + p := &PlanAnalyzer{rules: analyzer.DefaultWithProfile(analyzer.Profile{ + Disabled: map[string]bool{"high-cost": true}, + })} + + for _, r := range p.applyProfile(planIssues()) { + if r.RuleName == "high-cost" { + t.Error("high-cost should have been dropped") + } + } +} diff --git a/middleware/guard.go b/middleware/guard.go index e29e92b..94df103 100644 --- a/middleware/guard.go +++ b/middleware/guard.go @@ -22,6 +22,24 @@ type Guard struct { tracker *QueryTracker deduper *deduper cache *analysisCache + // slowQuery is the profile's resolved decision for the `slow-query` + // rule. Resolved once here because Guard runs on every query. + slowQuery findingPolicy +} + +// findingPolicy is a registered rule's resolved state for a finding the +// analyzer does not evaluate itself. `enabled` folds in `disable` and `only`; +// `severity` folds in a profile override. +type findingPolicy struct { + enabled bool + severity analyzer.Severity +} + +func resolvePolicy(a *analyzer.Analyzer, name string, def analyzer.Severity) findingPolicy { + return findingPolicy{ + enabled: a.RuleEnabled(name), + severity: a.RuleSeverity(name, def), + } } // NewGuard builds a Guard from the given options. @@ -33,12 +51,31 @@ func NewGuard(opts ...Option) *Guard { if o.parser != nil { o.analyzer = o.analyzer.WithParser(o.parser) } + // Profile settings feed the thresholds unless a Go option named one + // explicitly, so `rules.settings` works the same for these findings as + // for any statement rule. + if !o.slowThresholdSet { + o.slowThreshold = o.analyzer.RuleSettings("slow-query"). + Duration("threshold", o.slowThreshold) + } + if !o.n1Set { + if s := o.analyzer.RuleSettings("n-plus-one"); s != nil { + threshold := s.Int("threshold", 0) + window := s.Duration("window", 0) + if threshold > 0 && window > 0 { + o.enableN1, o.n1Threshold, o.n1Window = true, threshold, window + } + } + } + g := &Guard{opts: o, deduper: newDeduper(o.dedupWindow)} + g.slowQuery = resolvePolicy(o.analyzer, "slow-query", analyzer.SeverityWarning) if o.cacheSize > 0 { g.cache = newAnalysisCache(o.cacheSize) } - if o.enableN1 { - g.tracker = NewQueryTracker(o.n1Threshold, o.n1Window, func(results []analyzer.Result) { + n1 := resolvePolicy(o.analyzer, "n-plus-one", analyzer.SeverityWarning) + if o.enableN1 && n1.enabled { + g.tracker = NewQueryTracker(o.n1Threshold, o.n1Window, n1.severity, func(results []analyzer.Result) { o.reporter.Report(results) }) } @@ -96,12 +133,13 @@ func (g *Guard) report(results []analyzer.Result) { } // CheckLatency reports a slow-query finding if elapsed exceeds the threshold. +// Does nothing when the profile disabled `slow-query`. func (g *Guard) CheckLatency(query string, elapsed time.Duration) { - if elapsed >= g.opts.slowThreshold { + if g.slowQuery.enabled && elapsed >= g.opts.slowThreshold { display, fingerprint := g.opts.analyzer.PrepareQuery(query) g.opts.reporter.Report([]analyzer.Result{{ RuleName: "slow-query", - Severity: analyzer.SeverityWarning, + Severity: g.slowQuery.severity, Query: display, Fingerprint: fingerprint, Message: fmt.Sprintf("Query took %s (threshold: %s)", elapsed.Round(time.Millisecond), g.opts.slowThreshold), diff --git a/middleware/n_plus_one.go b/middleware/n_plus_one.go index 38bcaba..5622e5b 100644 --- a/middleware/n_plus_one.go +++ b/middleware/n_plus_one.go @@ -31,17 +31,21 @@ type QueryTracker struct { threshold int window time.Duration maxKeys int + severity analyzer.Severity reporter func(results []analyzer.Result) } // NewQueryTracker creates a tracker that flags when the same query pattern -// appears more than threshold times within the given window. -func NewQueryTracker(threshold int, window time.Duration, reportFn func([]analyzer.Result)) *QueryTracker { +// appears more than threshold times within the given window. severity is the +// one to report, which Guard resolves from the profile so a `severity:` +// override on `n-plus-one` reaches the finding. +func NewQueryTracker(threshold int, window time.Duration, severity analyzer.Severity, reportFn func([]analyzer.Result)) *QueryTracker { return &QueryTracker{ queries: make(map[string]*queryRecord), threshold: threshold, window: window, maxKeys: 10000, + severity: severity, reporter: reportFn, } } @@ -98,7 +102,7 @@ func (qt *QueryTracker) Track(query string) { if shouldReport { qt.reporter([]analyzer.Result{{ RuleName: "n-plus-one", - Severity: analyzer.SeverityWarning, + Severity: qt.severity, Query: normalized, Fingerprint: normalized, Message: fmt.Sprintf("Possible N+1 query detected: same pattern executed %d times in %s", count, qt.window), diff --git a/middleware/n_plus_one_test.go b/middleware/n_plus_one_test.go index 141b2c1..42deb2d 100644 --- a/middleware/n_plus_one_test.go +++ b/middleware/n_plus_one_test.go @@ -32,7 +32,7 @@ func TestNormalizeQuery(t *testing.T) { func TestQueryTracker_DetectsN1(t *testing.T) { var reported []analyzer.Result - tracker := NewQueryTracker(3, 5*time.Second, func(results []analyzer.Result) { + tracker := NewQueryTracker(3, 5*time.Second, analyzer.SeverityWarning, func(results []analyzer.Result) { reported = append(reported, results...) }) @@ -51,7 +51,7 @@ func TestQueryTracker_DetectsN1(t *testing.T) { func TestQueryTracker_DifferentPatterns(t *testing.T) { var reported []analyzer.Result - tracker := NewQueryTracker(3, 5*time.Second, func(results []analyzer.Result) { + tracker := NewQueryTracker(3, 5*time.Second, analyzer.SeverityWarning, func(results []analyzer.Result) { reported = append(reported, results...) }) @@ -67,7 +67,7 @@ func TestQueryTracker_DifferentPatterns(t *testing.T) { func TestQueryTracker_BelowThreshold(t *testing.T) { var reported []analyzer.Result - tracker := NewQueryTracker(5, 5*time.Second, func(results []analyzer.Result) { + tracker := NewQueryTracker(5, 5*time.Second, analyzer.SeverityWarning, func(results []analyzer.Result) { reported = append(reported, results...) }) @@ -83,7 +83,7 @@ func TestQueryTracker_BelowThreshold(t *testing.T) { func TestQueryTracker_ReportsOnlyOnce(t *testing.T) { var reported []analyzer.Result - tracker := NewQueryTracker(2, 5*time.Second, func(results []analyzer.Result) { + tracker := NewQueryTracker(2, 5*time.Second, analyzer.SeverityWarning, func(results []analyzer.Result) { reported = append(reported, results...) }) @@ -99,7 +99,7 @@ func TestQueryTracker_ReportsOnlyOnce(t *testing.T) { func TestQueryTracker_Reset(t *testing.T) { var reported []analyzer.Result - tracker := NewQueryTracker(2, 5*time.Second, func(results []analyzer.Result) { + tracker := NewQueryTracker(2, 5*time.Second, analyzer.SeverityWarning, func(results []analyzer.Result) { reported = append(reported, results...) }) diff --git a/middleware/options.go b/middleware/options.go index 8285bf6..9b352a7 100644 --- a/middleware/options.go +++ b/middleware/options.go @@ -9,24 +9,31 @@ import ( type options struct { slowThreshold time.Duration - reporter reporter.Reporter - analyzer *analyzer.Analyzer - parser analyzer.Parser - n1Threshold int - n1Window time.Duration - enableN1 bool - dedupWindow time.Duration - cacheSize int + // slowThresholdSet and n1Set record that a Go option named these + // explicitly, so NewGuard knows not to let profile settings override it. + slowThresholdSet bool + n1Set bool + reporter reporter.Reporter + analyzer *analyzer.Analyzer + parser analyzer.Parser + n1Threshold int + n1Window time.Duration + enableN1 bool + dedupWindow time.Duration + cacheSize int } // Option configures the runtime guard. type Option func(*options) -// WithSlowQueryThreshold sets the duration above which a query is flagged as slow. -// Default is 200ms. +// WithSlowQueryThreshold sets the duration above which a query is flagged as +// slow. Default is 200ms. This takes precedence over a `slow-query.threshold` +// setting carried by the analyzer's profile: an explicit Go option outranks +// file configuration. func WithSlowQueryThreshold(d time.Duration) Option { return func(o *options) { o.slowThreshold = d + o.slowThresholdSet = true } } @@ -58,6 +65,7 @@ func WithParser(p analyzer.Parser) Option { func WithN1Detection(threshold int, window time.Duration) Option { return func(o *options) { o.enableN1 = true + o.n1Set = true o.n1Threshold = threshold o.n1Window = window } diff --git a/middleware/rule_profile_test.go b/middleware/rule_profile_test.go new file mode 100644 index 0000000..1195508 --- /dev/null +++ b/middleware/rule_profile_test.go @@ -0,0 +1,171 @@ +package middleware + +import ( + "testing" + "time" + + "github.com/KARTIKrocks/sqlguard/analyzer" +) + +// snapshot returns a copy of what the reporter has been handed so far. +func (c *countingReporter) snapshot() []analyzer.Result { + c.mu.Lock() + defer c.mu.Unlock() + return append([]analyzer.Result(nil), c.results...) +} + +// The runtime findings are registered rules that this package builds itself. +// These tests pin that `disable`, `severity` and `settings` reach them, which +// is what makes the config surface uniform across all three entry points. + +func profileGuard(t *testing.T, p analyzer.Profile, rep *countingReporter, extra ...Option) *Guard { + t.Helper() + opts := append([]Option{ + WithAnalyzer(analyzer.DefaultWithProfile(p)), + WithReporter(rep), + }, extra...) + return NewGuard(opts...) +} + +func TestSlowQuery_DisabledByProfile(t *testing.T) { + rep := &countingReporter{} + g := profileGuard(t, analyzer.Profile{Disabled: map[string]bool{"slow-query": true}}, rep) + + g.CheckLatency("SELECT id FROM t WHERE id = ?", time.Second) + + if got := rep.snapshot(); len(got) != 0 { + t.Errorf("disabled slow-query still reported: %+v", got) + } +} + +func TestSlowQuery_EnabledByDefault(t *testing.T) { + rep := &countingReporter{} + g := profileGuard(t, analyzer.Profile{}, rep) + + g.CheckLatency("SELECT id FROM t WHERE id = ?", time.Second) + + got := rep.snapshot() + if len(got) != 1 || got[0].RuleName != "slow-query" { + t.Fatalf("expected one slow-query finding, got %+v", got) + } + if got[0].Severity != analyzer.SeverityWarning { + t.Errorf("severity = %v, want WARNING", got[0].Severity) + } +} + +func TestSlowQuery_SeverityOverride(t *testing.T) { + rep := &countingReporter{} + g := profileGuard(t, analyzer.Profile{ + Severity: map[string]analyzer.Severity{"slow-query": analyzer.SeverityCritical}, + }, rep) + + g.CheckLatency("SELECT id FROM t WHERE id = ?", time.Second) + + got := rep.snapshot() + if len(got) != 1 || got[0].Severity != analyzer.SeverityCritical { + t.Errorf("expected a CRITICAL slow-query, got %+v", got) + } +} + +func TestSlowQuery_ThresholdFromSettings(t *testing.T) { + rep := &countingReporter{} + g := profileGuard(t, analyzer.Profile{ + Settings: map[string]analyzer.Settings{"slow-query": {"threshold": "500ms"}}, + }, rep) + + g.CheckLatency("SELECT 1", 300*time.Millisecond) // under the configured 500ms + if got := rep.snapshot(); len(got) != 0 { + t.Fatalf("300ms should be under a 500ms threshold, got %+v", got) + } + + g.CheckLatency("SELECT 1", 600*time.Millisecond) + if got := rep.snapshot(); len(got) != 1 { + t.Errorf("600ms should exceed a 500ms threshold, got %+v", got) + } +} + +// TestSlowQuery_ExplicitOptionBeatsSettings pins the precedence: a Go option +// names the threshold deliberately, so file configuration does not move it. +func TestSlowQuery_ExplicitOptionBeatsSettings(t *testing.T) { + rep := &countingReporter{} + g := profileGuard(t, analyzer.Profile{ + Settings: map[string]analyzer.Settings{"slow-query": {"threshold": "500ms"}}, + }, rep, WithSlowQueryThreshold(50*time.Millisecond)) + + g.CheckLatency("SELECT 1", 100*time.Millisecond) + + if got := rep.snapshot(); len(got) != 1 { + t.Errorf("the explicit 50ms option should have won over the configured 500ms, got %+v", got) + } +} + +func TestNPlusOne_DisabledByProfile(t *testing.T) { + rep := &countingReporter{} + g := profileGuard(t, analyzer.Profile{Disabled: map[string]bool{"n-plus-one": true}}, rep, + WithN1Detection(2, time.Minute)) + + if g.tracker != nil { + t.Fatal("a disabled n-plus-one should not build a tracker") + } + for range 5 { + g.Check("SELECT id FROM t WHERE id = ?") + } + for _, r := range rep.snapshot() { + if r.RuleName == "n-plus-one" { + t.Errorf("disabled n-plus-one still reported: %+v", r) + } + } +} + +func TestNPlusOne_SeverityOverride(t *testing.T) { + rep := &countingReporter{} + g := profileGuard(t, analyzer.Profile{ + Severity: map[string]analyzer.Severity{"n-plus-one": analyzer.SeverityCritical}, + }, rep, WithN1Detection(2, time.Minute)) + + for range 3 { + g.Check("SELECT id FROM t WHERE id = ? LIMIT 1") + } + + var found bool + for _, r := range rep.snapshot() { + if r.RuleName == "n-plus-one" { + found = true + if r.Severity != analyzer.SeverityCritical { + t.Errorf("n-plus-one severity = %v, want CRITICAL", r.Severity) + } + } + } + if !found { + t.Error("expected an n-plus-one finding") + } +} + +// TestNPlusOne_EnabledBySettings covers the only way a config file can turn +// N+1 on: before this, detection was reachable only from Go via +// WithN1Detection, so .sqlguard.yml could not enable it at all. +func TestNPlusOne_EnabledBySettings(t *testing.T) { + rep := &countingReporter{} + g := profileGuard(t, analyzer.Profile{ + Settings: map[string]analyzer.Settings{ + "n-plus-one": {"threshold": 2, "window": "1m"}, + }, + }, rep) + + if g.tracker == nil { + t.Fatal("settings should have enabled the tracker") + } + for range 3 { + g.Check("SELECT id FROM t WHERE id = ? LIMIT 1") + } + + var found bool + for _, r := range rep.snapshot() { + if r.RuleName == "n-plus-one" { + found = true + } + } + if !found { + t.Errorf("expected an n-plus-one finding, got %+v", rep.snapshot()) + } +} diff --git a/website/docs/configuration.md b/website/docs/configuration.md index 6c65410..bea2792 100644 --- a/website/docs/configuration.md +++ b/website/docs/configuration.md @@ -56,15 +56,16 @@ rules: max-length: 100 # flag IN (...) with more elements than this large-offset: threshold: 1000 # flag a literal OFFSET above this + slow-query: + threshold: 200ms # runtime: flag a query at or above this latency + n-plus-one: + threshold: 10 # runtime: this many of the same fingerprint... + window: 1m # ...within this window # Redact literal values out of Result.Query. ON by default. Set false ONLY # for local debugging where the query text is trusted. redact: true -# Runtime middleware: slow-query threshold. Go duration string. -slow-query: - threshold: 200ms - # Runtime middleware: report each (rule, fingerprint) at most once per # window. "0" disables and reports every occurrence. dedup: @@ -81,17 +82,24 @@ scan: | --- | --- | --- | | `version` | all | Reserved for forward compatibility; always `1` today. | | `strict` | all | Make unknown keys, unknown rule names and bad severities fatal instead of warnings. | -| `rules.disable` | static + runtime rules | Rule names to turn off. | -| `rules.only` | static + runtime rules | Whitelist. When non-empty, only these run and `disable` is ignored. | -| `rules.severity` | static + runtime rules | `info`, `warning`, `critical`, or `off`. | -| `rules.settings` | rules with tunables | `leading-wildcard.min-length`, `in-list-too-large.max-length`, `large-offset.threshold`. See [Rules](rules). | +| `rules.disable` | every rule | Rule names to turn off. | +| `rules.only` | every rule | Whitelist. When non-empty, only these run and `disable` is ignored. | +| `rules.severity` | every rule | `info`, `warning`, `critical`, or `off`. | +| `rules.settings` | rules with tunables | `leading-wildcard.min-length`, `in-list-too-large.max-length`, `large-offset.threshold`, `slow-query.threshold`, `n-plus-one.threshold` / `.window`. See [Rules](rules). | | `redact` | all | `false` keeps raw literals in `Result.Query`. See [Redaction](redaction). | -| `slow-query.threshold` | middleware, integrations | Go duration (`200ms`, `1s`). Equivalent to `WithSlowQueryThreshold`. | | `dedup.window` | middleware, integrations | Go duration or `"0"`. Equivalent to `WithFindingDedup`. | | `scan.exclude-paths` | scanner | Regexes matched against the scanned file path. | Quote `"off"` — unquoted `off` is a YAML boolean. +_Changed in 0.3._ "Every rule" now means every rule. In 0.2 only the 14 +statement rules were addressable: naming `slow-query`, `n-plus-one` or a plan +rule (`seq-scan`, `high-cost`, `full-table-scan`, `no-index-used`, `filesort`) +warned with `unknown rule`, and failed outright under `strict: true`, even +though the [rules reference](rules) listed them. The slow-query threshold also +moved from a top-level `slow-query.threshold` key to +`rules.settings.slow-query.threshold`, so every tunable lives in one place. + ## Lenient by default Unknown top-level keys and unknown rule names are **warnings**, printed to @@ -99,9 +107,12 @@ stderr by the CLI as `sqlguard: config warning: …`, so a config that names a rule added in a newer release still loads on an older binary. Set `strict: true` when you want CI to fail on a typo. -One thing people look for and do not find: N+1 detection has no config key. -Its `threshold` and `window` are workload-specific, so they are set in code -with `WithN1Detection` (see [N+1 detection](n-plus-one)). +_Added in 0.3._ N+1 detection can be turned on from the file. Setting both +`rules.settings.n-plus-one.threshold` and `.window` enables it; previously it +was reachable only from Go with `WithN1Detection`, which remains the way to +set it in code. An explicit Go option wins over the file for both N+1 and +`slow-query.threshold`, so a deliberate call is never overridden by a config +that happens to be on disk. ## Loading it from Go diff --git a/website/docs/explain.md b/website/docs/explain.md index 0d24ee9..02cb2f4 100644 --- a/website/docs/explain.md +++ b/website/docs/explain.md @@ -33,7 +33,7 @@ sqlguard explain --db "…" --format json "SELECT …" | `--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. | -| `--config`, `--no-config` | — | Persistent flags; `explain` findings are not affected by `rules:` config. | +| `--config`, `--no-config` | — | Persistent flags. `rules:` config applies — see below. | The whole command runs under a 30-second timeout, including the initial connectivity check. Exit code is **1** when the plan has issues, **0** @@ -71,6 +71,20 @@ there is no log sink to protect, and you need to recognise your own query. | `no-index-used` | mysql | A row with empty `key` **and** empty `possible_keys`. | | `filesort` | mysql | `Using filesort` in `Extra`. | +_Changed in 0.3._ These five are ordinary rule names now, so +[`.sqlguard.yml`](configuration) can turn one off or re-severity it: + +```yaml +rules: + disable: [high-cost] + severity: + seq-scan: critical +``` + +In 0.2 `explain` ignored `rules:` entirely, and naming a plan rule in a config +was an `unknown rule` warning — a hard error under `strict: true`. A +`severity` override also wins over `seq-scan`'s row-count-derived severity. + Postgres plans are requested as `EXPLAIN (FORMAT JSON)` and walked recursively, so nested scans inside joins and CTEs are found. MySQL plans are requested as `EXPLAIN FORMAT=TRADITIONAL` — MySQL 9 defaults diff --git a/website/docs/middleware.md b/website/docs/middleware.md index c4447a7..2fa7fb7 100644 --- a/website/docs/middleware.md +++ b/website/docs/middleware.md @@ -49,11 +49,11 @@ Every option is a `middleware.Option`. The same set is accepted by every | Option | Default | Effect | | --- | --- | --- | -| `WithSlowQueryThreshold(d time.Duration)` | `200ms` | Report `slow-query` when a successful query's driver-measured latency reaches `d`. | +| `WithSlowQueryThreshold(d time.Duration)` | `200ms` | Report `slow-query` when a successful query's driver-measured latency reaches `d`. Takes precedence over `rules.settings.slow-query.threshold` _0.3+_. | | `WithReporter(r reporter.Reporter)` | `reporter.NewConsoleReporter()` (stderr) | Where findings go. `reporter.NewJSONReporter()` is built in; implement `Report([]analyzer.Result)` for anything else. | | `WithAnalyzer(a *analyzer.Analyzer)` | `analyzer.Default()` | Replace the rule set — typically `analyzer.DefaultWithProfile(...)` from config, or an analyzer built `WithRawQuery()`. | | `WithParser(p analyzer.Parser)` | `analyzer.FallbackParser` | Swap in a real grammar from [`parsers/`](parsers). Applied to whichever analyzer is in use. | -| `WithN1Detection(threshold int, window time.Duration)` | off | Report `n-plus-one` when the same query fingerprint runs `threshold` times within `window`. See [N+1 detection](n-plus-one). | +| `WithN1Detection(threshold int, window time.Duration)` | off | Report `n-plus-one` when the same query fingerprint runs `threshold` times within `window`. Takes precedence over `rules.settings.n-plus-one` _0.3+_. See [N+1 detection](n-plus-one). | | `WithFindingDedup(window time.Duration)` | `1m` | Report each (rule, fingerprint) pair at most once per window. `0` reports every occurrence. See [Noise control](noise-control). | | `WithAnalysisCacheSize(n int)` | `1024` | Memoize static analysis per exact query string in an LRU of `n` entries. `0` disables the cache. | diff --git a/website/docs/rules.md b/website/docs/rules.md index 362f845..c3ae763 100644 --- a/website/docs/rules.md +++ b/website/docs/rules.md @@ -39,6 +39,16 @@ can be overridden per project. | `no-index-used` | WARNING | EXPLAIN (mysql) | Empty `key` **and** empty `possible_keys` | | `filesort` | INFO | EXPLAIN (mysql) | `Using filesort` in `Extra` | +_Changed in 0.3._ Every rule in this table is addressable by name in +[`.sqlguard.yml`](configuration) — `disable`, `only`, `severity` and +`settings` all work the same for a runtime or plan rule as for a statement +rule. In 0.2 only the 14 statement rules were: naming any of the other seven +warned with `unknown rule`, and failed under `strict: true`. + +The runtime and plan rules are not evaluated against parsed SQL — middleware +derives them from latency and fingerprint counts, and `explain` from the +database's own plan — so they never fire during a static `sqlguard scan`. + "static, runtime" rules read the normalized `Statement` a [parser](parsers) produces; they never look at raw SQL. The runtime and EXPLAIN rules are built into the [middleware](middleware) and the [EXPLAIN analyzer](explain) diff --git a/website/docs/suppressions.md b/website/docs/suppressions.md index e1767eb..56f7bee 100644 --- a/website/docs/suppressions.md +++ b/website/docs/suppressions.md @@ -64,7 +64,9 @@ quiet at runtime too needs the in-SQL form. ## What suppression does not do -- It does not affect the `slow-query` or `n-plus-one` runtime findings. +- It does not affect the `slow-query` or `n-plus-one` runtime findings. Those + are turned off in [config](configuration) instead — `disable: [slow-query]` + — which since 0.3 works for every rule name in the reference table. Those are about behaviour, not statement text; tune their thresholds via [options](middleware#options) or scope N+1 with `ResetN1()`. - It does not affect the [EXPLAIN analyzer](explain), which reports on the From b1bcd0a3b65e8e008a4e98d81adc073307abacfa Mon Sep 17 00:00:00 2001 From: kartik Date: Fri, 25 Sep 2026 07:24:15 +0530 Subject: [PATCH 02/11] fix(config): ignore unknown rule names, validate every setting kind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ten review findings on the uniform-rule-config change; all were real. An unknown rule name was warned about and then honored. Since `only:` is a whitelist over every registered rule, one typo produced a whitelist matching nothing — and now that the runtime and plan findings are addressable, that silenced slow-query, N+1 and every EXPLAIN finding too, not just the static scan. Unknown names are now reported and left out. checkDurationSettings only covered durations, but n-plus-one.threshold is read with Settings.Int, which also falls back silently. A quoted `threshold: "10"` — a string in YAML — read back as 0, so detection stayed off with nothing said, in the one path that exists to turn it on from a file. Validation now covers numbers as well, and reports half a paired block, where one key alone does nothing. Validation trimmed the value before parsing but Settings.Duration did not, so `" 500ms "` passed the check and then fell back to the default. The read trims now, so the check and the read agree. Config resolution had landed after openDB in the explain command, contradicting the comment directly above it: a rule-name typo under `strict: true` cost the full connect timeout before surfacing, and warnings printed under connection noise. Moved back above the dial and pinned by an elapsed-time assertion. Docs: rules.md still said the plan rules were not configurable and pointed at the moved slow-query key; configuration.md documented SlowQueryThreshold(), which this branch deletes, and described option-order precedence that no longer applies; n-plus-one.md did not mention it can be enabled from a file. The precedence claim also overreached. A Go option wins for a threshold, but `disable` wins over the Go call — deliberately, so an operator can silence a noisy rule without a redeploy. Documented as the asymmetry it is and pinned by tests. `only:` reaching EXPLAIN is a behaviour change for existing valid configs, so it is now called out in explain.md and the changelog rather than only in a commit message. --- CHANGELOG.md | 17 +++- analyzer/registry.go | 5 +- cmd/sqlguard/explain.go | 26 +++--- cmd/sqlguard/explain_test.go | 40 +++++++++ config/config.go | 145 ++++++++++++++++++++++++-------- config/config_test.go | 105 +++++++++++++++++------ middleware/rule_profile_test.go | 41 +++++++++ website/docs/configuration.md | 36 +++++--- website/docs/explain.md | 9 ++ website/docs/n-plus-one.md | 15 +++- website/docs/rules.md | 16 ++-- 11 files changed, 366 insertions(+), 89 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 949c176..4accabe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,13 +39,24 @@ the same version in lockstep. `WithSlowQueryThreshold` in Go still wins over the file. - **`explain` now honors `rules:` config.** It previously ignored it by design, which is what its docs said. A `severity` override also beats - `seq-scan`'s row-count-derived severity. + `seq-scan`'s row-count-derived severity. **Note that `only:` reaches + EXPLAIN too**: a config using `only:` to focus the static scan now leaves no + plan rule enabled, so `sqlguard explain` reports nothing until the plan + rules are named in it (or `disable:` is used instead). - **N+1 detection can be enabled from the config file.** Setting both `rules.settings.n-plus-one.threshold` and `.window` turns it on; it was previously reachable only from Go via `WithN1Detection`, which still takes precedence. -- A duration in `rules.settings` that does not parse is now reported instead - of being silently replaced by the built-in default. +- A value in `rules.settings` that will not read back as its type is now + reported instead of being silently replaced by the built-in default. This + covers durations and numbers, and a half-specified `n-plus-one` block — + a quoted `threshold: "10"` is a string in YAML, read back as 0, which would + have left N+1 detection off with no indication. +- An unknown rule name in `disable:` / `only:` / `severity:` / `settings:` is + now warned about **and ignored**, rather than warned about and honored. One + typo in `only:` acted as a whitelist matching nothing, which since every + rule became addressable would have silenced the runtime and plan findings + as well as the static scan. - README restructured as a landing page: logo, "Why sqlguard?" comparison, quick start, and a guide index pointing at the docs site. The deep diff --git a/analyzer/registry.go b/analyzer/registry.go index 2a31a35..8b08572 100644 --- a/analyzer/registry.go +++ b/analyzer/registry.go @@ -2,6 +2,7 @@ package analyzer import ( "sort" + "strings" "sync" "time" ) @@ -61,7 +62,9 @@ func (s Settings) Duration(key string, def time.Duration) time.Duration { } switch v := s[key].(type) { case string: - if d, err := time.ParseDuration(v); err == nil { + // Trimmed so this agrees with the config loader's validation; a + // value it accepts must not fall back to def here. + if d, err := time.ParseDuration(strings.TrimSpace(v)); err == nil { return d } case int: diff --git a/cmd/sqlguard/explain.go b/cmd/sqlguard/explain.go index 91d1d7f..b6d74a3 100644 --- a/cmd/sqlguard/explain.go +++ b/cmd/sqlguard/explain.go @@ -40,22 +40,15 @@ func runExplain(cmd *cobra.Command, args []string) error { query := args[0] - // Validate the format before dialing: a typo should cost an error message, - // not a connection attempt followed by a silent fall back to console. + // Everything that can fail on the user's own input is resolved before + // dialing: a bad --format or a rule-name typo under `strict: true` should + // cost an error message, not a connection attempt first. The config + // warnings print here for the same reason — after the dial they arrive + // buried in connection noise. rep, writeErr, err := newReporter(explainFormat) if err != nil { return err } - - ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) - defer cancel() - - db, err := openDB(ctx, explainDialect, explainDSN) - if err != nil { - return fmt.Errorf("failed to connect: %w", err) - } - defer func() { _ = db.Close() }() - cfg, err := resolveConfig(".") if err != nil { return err @@ -66,6 +59,15 @@ func runExplain(cmd *cobra.Command, args []string) error { } printConfigWarnings(cfg) + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + db, err := openDB(ctx, explainDialect, explainDSN) + if err != nil { + return fmt.Errorf("failed to connect: %w", err) + } + defer func() { _ = db.Close() }() + explainOpts := []explain.Option{explain.WithAnalyzer(a)} if explainAllowDML { explainOpts = append(explainOpts, explain.WithAllowDML()) diff --git a/cmd/sqlguard/explain_test.go b/cmd/sqlguard/explain_test.go index 156df6f..b927392 100644 --- a/cmd/sqlguard/explain_test.go +++ b/cmd/sqlguard/explain_test.go @@ -1,8 +1,11 @@ package main import ( + "os" + "path/filepath" "strings" "testing" + "time" "github.com/spf13/cobra" ) @@ -52,3 +55,40 @@ func TestExplain_AcceptsKnownFormats(t *testing.T) { } } } + +// TestExplain_ConfigErrorsBeforeDialing pins the ordering. Config resolution +// sat after openDB, so a rule-name typo under `strict: true` cost the full +// 30-second connect timeout before surfacing — and printed its warnings +// underneath the connection noise. +func TestExplain_ConfigErrorsBeforeDialing(t *testing.T) { + dir := t.TempDir() + cfgPath := filepath.Join(dir, ".sqlguard.yml") + if err := os.WriteFile(cfgPath, []byte("strict: true\nrules:\n disable: [no-such-rule]\n"), 0o644); err != nil { + t.Fatalf("write config: %v", err) + } + + oldFormat, oldDSN, oldCfg := explainFormat, explainDSN, configPathFlag + explainFormat = "console" + // An unroutable address: reaching the dial at all would block for the + // timeout, so a prompt return is itself part of the assertion. + explainDSN = "postgres://nobody@192.0.2.1:5432/none" + configPathFlag = cfgPath + t.Cleanup(func() { explainFormat, explainDSN, configPathFlag = oldFormat, oldDSN, oldCfg }) + + start := time.Now() + err := runExplain(&cobra.Command{}, []string{"SELECT 1"}) + elapsed := time.Since(start) + + if err == nil { + t.Fatal("expected the strict config to fail the run") + } + if !strings.Contains(err.Error(), "no-such-rule") { + t.Errorf("expected the config error, got %v", err) + } + if strings.Contains(err.Error(), "failed to connect") { + t.Errorf("config was resolved after dialing: %v", err) + } + if elapsed > 5*time.Second { + t.Errorf("took %v; the run reached the dial before failing on config", elapsed) + } +} diff --git a/config/config.go b/config/config.go index ae3ded5..44719d9 100644 --- a/config/config.go +++ b/config/config.go @@ -180,33 +180,38 @@ func (c *Config) Profile() (analyzer.Profile, error) { return nil } - checkName := func(name string) error { + // checkName reports whether the name is usable. In lenient mode an unknown + // name warns and is then *ignored* — honouring it would let one typo in + // `only:` act as a whitelist that matches nothing, which since 0.3 turns + // off the runtime and plan findings too, not just the static scan. + checkName := func(name string) (bool, error) { if !known[name] { - return warn("unknown rule %q (known: %s)", name, strings.Join(analyzer.RuleNames(), ", ")) + if err := warn("unknown rule %q (known: %s)", name, strings.Join(analyzer.RuleNames(), ", ")); err != nil { + return false, err + } + return false, nil } - return nil + return true, nil } - for _, name := range c.Rules.Disable { - if err := checkName(name); err != nil { - return p, err - } - p.Disabled[name] = true + if err := collectNames(c.Rules.Disable, p.Disabled, checkName); err != nil { + return p, err } - for _, name := range c.Rules.Only { - if err := checkName(name); err != nil { - return p, err - } - p.Only[name] = true + if err := collectNames(c.Rules.Only, p.Only, checkName); err != nil { + return p, err } if err := applySeverities(c.Rules.Severity, &p, checkName, warn); err != nil { return p, err } for name, kv := range c.Rules.Settings { - if err := checkName(name); err != nil { + ok, err := checkName(name) + if err != nil { return p, err } - if err := checkDurationSettings(name, kv, warn); err != nil { + if !ok { + continue + } + if err := checkSettings(name, kv, warn); err != nil { return p, err } p.Settings[name] = analyzer.Settings(kv) @@ -214,6 +219,22 @@ func (c *Config) Profile() (analyzer.Profile, error) { return p, nil } +// collectNames adds each usable name to the set. An unknown name is reported +// by checkName and then left out: warning about a typo and acting on it +// anyway is how one bad entry in `only:` becomes a whitelist matching nothing. +func collectNames(names []string, into map[string]bool, checkName func(string) (bool, error)) error { + for _, name := range names { + ok, err := checkName(name) + if err != nil { + return err + } + if ok { + into[name] = true + } + } + return nil +} + // applySeverities resolves the `rules.severity` map onto the profile. A // severity of "off" disables the rule rather than setting one, which is what // makes `severity: {slow-query: off}` equivalent to listing it under @@ -221,15 +242,19 @@ func (c *Config) Profile() (analyzer.Profile, error) { func applySeverities( sevs map[string]string, p *analyzer.Profile, - checkName func(string) error, + checkName func(string) (bool, error), warn func(string, ...any) error, ) error { for name, sevStr := range sevs { - if err := checkName(name); err != nil { + ok, err := checkName(name) + if err != nil { return err } - sev, off, ok := parseSeverity(sevStr) if !ok { + continue + } + sev, off, valid := parseSeverity(sevStr) + if !valid { if err := warn("rule %q: invalid severity %q", name, sevStr); err != nil { return err } @@ -244,32 +269,86 @@ func applySeverities( return nil } -// durationSettings names the per-rule settings parsed as Go durations, so a -// malformed value is reported rather than silently replaced by a default. -var durationSettings = map[string][]string{ - "slow-query": {"threshold"}, - "n-plus-one": {"window"}, +// settingKind is how a per-rule setting is read back, so a value that will not +// survive the read can be reported here instead of silently becoming a +// default. analyzer.Settings.Duration and .Int both fall back on a bad value, +// which would otherwise turn a typo into a wrong threshold — or, for +// n-plus-one, into detection that never switches on. +type settingKind int + +const ( + settingDuration settingKind = iota + settingInt +) + +// checkedSettings names the settings that are read as something other than an +// opaque value. Rules whose settings are not listed pass through unchecked. +var checkedSettings = map[string]map[string]settingKind{ + "slow-query": {"threshold": settingDuration}, + "n-plus-one": {"threshold": settingInt, "window": settingDuration}, + "leading-wildcard": {"min-length": settingInt}, + "in-list-too-large": {"max-length": settingInt}, + "large-offset": {"threshold": settingInt}, +} + +// pairedSettings names settings that only take effect together. n-plus-one +// needs both to switch detection on, so half a block is silently inert. +var pairedSettings = map[string][]string{ + "n-plus-one": {"threshold", "window"}, } -// checkDurationSettings reports a duration setting that will not parse. -// analyzer.Settings.Duration falls back to the caller's default on a bad -// value, so without this a typo would quietly leave the built-in threshold in -// place rather than the one the file asked for. -func checkDurationSettings(rule string, kv map[string]any, warn func(string, ...any) error) error { - for _, key := range durationSettings[rule] { +// checkSettings reports a setting whose value will not read back as its kind, +// and a half-specified pair. +func checkSettings(rule string, kv map[string]any, warn func(string, ...any) error) error { + for key, kind := range checkedSettings[rule] { v, present := kv[key] if !present { continue } + if err := checkSettingValue(rule, key, kind, v, warn); err != nil { + return err + } + } + + pair := pairedSettings[rule] + if len(pair) == 0 { + return nil + } + var have, missing []string + for _, key := range pair { + if _, present := kv[key]; present { + have = append(have, key) + } else { + missing = append(missing, key) + } + } + if len(have) > 0 && len(missing) > 0 { + return warn("rule %q: setting %q has no effect without %q", + rule, strings.Join(have, ", "), strings.Join(missing, ", ")) + } + return nil +} + +func checkSettingValue(rule, key string, kind settingKind, v any, warn func(string, ...any) error) error { + switch kind { + case settingDuration: str, isStr := v.(string) if !isStr { - continue // a bare number is milliseconds; Settings handles it + return nil // a bare number is milliseconds; Settings handles it } if _, err := time.ParseDuration(strings.TrimSpace(str)); err != nil { - if err := warn("rule %q: setting %q: invalid duration %q", rule, key, str); err != nil { - return err - } + return warn("rule %q: setting %q: invalid duration %q", rule, key, str) + } + case settingInt: + switch v.(type) { + case int, int64, float64: + return nil } + // A quoted number is the common YAML slip. Settings.Int does not + // accept a string, so it would read back as the default: for + // n-plus-one.threshold that means detection never switches on. + return warn("rule %q: setting %q: expected a number, got %v (quoted numbers are strings in YAML)", + rule, key, v) } return nil } diff --git a/config/config_test.go b/config/config_test.go index 660f292..163d264 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -227,39 +227,96 @@ func TestProfile_AcceptsNonEvaluatedRules(t *testing.T) { } } -// TestProfile_ValidatesDurationSettings guards a trap introduced by moving the -// threshold into settings: analyzer.Settings.Duration falls back to the -// caller's default when a value does not parse, so without this check a typo -// would silently leave the built-in 200ms in place. -func TestProfile_ValidatesDurationSettings(t *testing.T) { - t.Run("strict fails", func(t *testing.T) { +// TestProfile_ValidatesSettings guards the trap that moving tunables into +// settings creates: analyzer.Settings.Duration and .Int both fall back to the +// caller's default on a value they cannot read, so an unchecked typo becomes a +// silently wrong threshold — or, for n-plus-one, detection that never switches +// on at all. +func TestProfile_ValidatesSettings(t *testing.T) { + cases := []struct { + name string + settings map[string]map[string]any + warnings int + }{ + {"unparseable duration", map[string]map[string]any{ + "slow-query": {"threshold": "200mss"}}, 1}, + {"quoted number reads back as the default", map[string]map[string]any{ + "n-plus-one": {"threshold": "10", "window": "1m"}}, 1}, + {"half a paired block is inert", map[string]map[string]any{ + "n-plus-one": {"window": "1m"}}, 1}, + {"quoted int on a statement rule", map[string]map[string]any{ + "leading-wildcard": {"min-length": "4"}}, 1}, + } + + for _, tc := range cases { + t.Run(tc.name+" (lenient warns)", func(t *testing.T) { + c := &Config{Rules: RulesConfig{Settings: tc.settings}} + if _, err := c.Profile(); err != nil { + t.Fatalf("lenient mode should not fail: %v", err) + } + if len(c.Warnings()) != tc.warnings { + t.Errorf("expected %d warning(s), got %v", tc.warnings, c.Warnings()) + } + }) + t.Run(tc.name+" (strict fails)", func(t *testing.T) { + c := &Config{Strict: true, Rules: RulesConfig{Settings: tc.settings}} + if _, err := c.Profile(); err == nil { + t.Fatal("expected strict mode to reject it") + } + }) + } + + t.Run("valid settings pass", func(t *testing.T) { c := &Config{Strict: true, Rules: RulesConfig{Settings: map[string]map[string]any{ - "slow-query": {"threshold": "200mss"}, + "slow-query": {"threshold": "1s"}, + "n-plus-one": {"window": 500, "threshold": 10}, }}} - if _, err := c.Profile(); err == nil { - t.Fatal("expected an error for an unparseable duration") + if _, err := c.Profile(); err != nil { + t.Fatalf("valid settings rejected: %v", err) } }) - t.Run("lenient warns", func(t *testing.T) { - c := &Config{Rules: RulesConfig{Settings: map[string]map[string]any{ - "n-plus-one": {"window": "1minute"}, + // Surrounding whitespace must not pass validation and then fall back at + // read time: the check and the read have to agree on the same value. + t.Run("padded duration survives the round trip", func(t *testing.T) { + c := &Config{Strict: true, Rules: RulesConfig{Settings: map[string]map[string]any{ + "slow-query": {"threshold": " 500ms "}, }}} - if _, err := c.Profile(); err != nil { - t.Fatalf("lenient mode should not fail: %v", err) + p, err := c.Profile() + if err != nil { + t.Fatalf("a padded duration should be accepted: %v", err) } - if len(c.Warnings()) != 1 { - t.Errorf("expected one warning, got %v", c.Warnings()) + if d := p.Settings["slow-query"].Duration("threshold", time.Second); d != 500*time.Millisecond { + t.Errorf("read back %v, want 500ms — the check and the read disagree", d) } }) +} - t.Run("valid duration and bare number pass", func(t *testing.T) { - c := &Config{Strict: true, Rules: RulesConfig{Settings: map[string]map[string]any{ - "slow-query": {"threshold": "1s"}, - "n-plus-one": {"window": 500, "threshold": 10}, - }}} - if _, err := c.Profile(); err != nil { - t.Fatalf("valid settings rejected: %v", err) +// TestProfile_UnknownNameIsIgnoredNotHonoured pins the blast radius of a typo. +// A warned-about name must not take effect: an unknown entry in `only:` would +// otherwise act as a whitelist matching nothing, which since every rule became +// addressable also silences the runtime and plan findings. +func TestProfile_UnknownNameIsIgnoredNotHonoured(t *testing.T) { + c := &Config{Rules: RulesConfig{Only: []string{"slect-star"}}} + + p, err := c.Profile() + if err != nil { + t.Fatalf("lenient mode should not fail: %v", err) + } + if len(c.Warnings()) != 1 { + t.Errorf("expected one warning, got %v", c.Warnings()) + } + if len(p.Only) != 0 { + t.Errorf("an unknown name entered the whitelist: %v", p.Only) + } + + a := analyzer.DefaultWithProfile(p) + if len(a.Analyze("SELECT * FROM t")) == 0 { + t.Error("a typo in only: silenced the static rules") + } + for _, name := range []string{"slow-query", "n-plus-one", "seq-scan"} { + if !a.RuleEnabled(name) { + t.Errorf("a typo in only: silenced %q", name) } - }) + } } diff --git a/middleware/rule_profile_test.go b/middleware/rule_profile_test.go index 1195508..7153266 100644 --- a/middleware/rule_profile_test.go +++ b/middleware/rule_profile_test.go @@ -169,3 +169,44 @@ func TestNPlusOne_EnabledBySettings(t *testing.T) { t.Errorf("expected an n-plus-one finding, got %+v", rep.snapshot()) } } + +// TestDisableBeatsExplicitGoOption pins the precedence documented under +// Configuration → Precedence, and the asymmetry in it: a Go option wins for a +// *threshold*, but `disable` is an instruction and wins over the Go call, so +// an operator can silence a noisy rule by editing .sqlguard.yml without a +// redeploy. +func TestDisableBeatsExplicitGoOption(t *testing.T) { + t.Run("slow-query", func(t *testing.T) { + rep := &countingReporter{} + g := profileGuard(t, analyzer.Profile{Disabled: map[string]bool{"slow-query": true}}, rep, + WithSlowQueryThreshold(time.Millisecond)) + + g.CheckLatency("SELECT 1", time.Second) + + if got := rep.snapshot(); len(got) != 0 { + t.Errorf("disable should outrank WithSlowQueryThreshold, got %+v", got) + } + }) + + t.Run("n-plus-one", func(t *testing.T) { + rep := &countingReporter{} + g := profileGuard(t, analyzer.Profile{Disabled: map[string]bool{"n-plus-one": true}}, rep, + WithN1Detection(2, time.Minute)) + + if g.tracker != nil { + t.Error("disable should outrank WithN1Detection") + } + }) + + t.Run("an only list that omits the rule also disables it", func(t *testing.T) { + rep := &countingReporter{} + g := profileGuard(t, analyzer.Profile{Only: map[string]bool{"select-star": true}}, rep, + WithSlowQueryThreshold(time.Millisecond)) + + g.CheckLatency("SELECT 1", time.Second) + + if got := rep.snapshot(); len(got) != 0 { + t.Errorf("an `only` whitelist omitting slow-query should silence it, got %+v", got) + } + }) +} diff --git a/website/docs/configuration.md b/website/docs/configuration.md index bea2792..812d297 100644 --- a/website/docs/configuration.md +++ b/website/docs/configuration.md @@ -110,9 +110,9 @@ a rule added in a newer release still loads on an older binary. Set _Added in 0.3._ N+1 detection can be turned on from the file. Setting both `rules.settings.n-plus-one.threshold` and `.window` enables it; previously it was reachable only from Go with `WithN1Detection`, which remains the way to -set it in code. An explicit Go option wins over the file for both N+1 and -`slow-query.threshold`, so a deliberate call is never overridden by a config -that happens to be on disk. +set it in code. An explicit Go option wins over the file for the N+1 and slow-query +_thresholds_. Turning either rule off goes the other way — see +[Precedence](#precedence). ## Loading it from Go @@ -135,10 +135,10 @@ And on a `*Config`: | Method | Use | | --- | --- | -| `MiddlewareOptions() ([]middleware.Option, error)` | `WithAnalyzer` from the profile, plus `WithSlowQueryThreshold` / `WithFindingDedup` when set. Append your own options after it. | +| `MiddlewareOptions() ([]middleware.Option, error)` | `WithAnalyzer` from the profile — which carries the rule settings, including the slow-query and N+1 tunables — plus `WithFindingDedup` when set. Append your own options after it. | | `Analyzer() (*analyzer.Analyzer, error)` | `analyzer.DefaultWithProfile` built from this file. | | `Profile() (analyzer.Profile, error)` | The resolved, parser-independent profile. | -| `SlowQueryThreshold()`, `DedupWindow()` | `(time.Duration, ok bool, error)` — `ok` is false when the key is unset. | +| `DedupWindow()` | `(time.Duration, ok bool, error)` — `ok` is false when the key is unset. _Changed in 0.3._ `SlowQueryThreshold()` is gone; read `Profile().Settings["slow-query"].Duration("threshold", d)` instead. | | `ExcludeMatcher() (func(path string) bool, error)` | The compiled `scan.exclude-paths` predicate. | | `Warnings() []string` | Non-fatal problems found while loading. Surface them. | @@ -155,8 +155,24 @@ sqlguard.Register("sqlguard-pg", "pgx", opts...) ## Precedence -Options given in code after `MiddlewareOptions()` win, because -`middleware.Option`s apply in order. So `append(opts, -middleware.WithSlowQueryThreshold(time.Second))` overrides the file's -`slow-query.threshold`. Inline [suppressions](suppressions) always win over -both: they silence a finding at one site regardless of config. +_Changed in 0.3._ For the **thresholds**, an explicit Go option wins over the +file wherever it appears in the list — `WithSlowQueryThreshold` and +`WithN1Detection` record that they were called, so a config value no longer +has to be ordered around. In 0.2 this depended on option order, because the +file's threshold arrived as an option of its own. + +```go +opts, _ := cfg.MiddlewareOptions() +opts = append(opts, middleware.WithSlowQueryThreshold(time.Second)) +// 1s, whatever rules.settings.slow-query.threshold says +``` + +**Turning a rule off is the other way round: the file wins.** `disable: +[slow-query]` or an `only:` list that omits it silences the finding even with +`WithSlowQueryThreshold` set, and `disable: [n-plus-one]` stops the tracker +being built at all despite `WithN1Detection`. That is deliberate — `disable` +is an instruction, not a tuning value, and an operator editing +`.sqlguard.yml` should be able to silence a noisy rule without a redeploy. + +Inline [suppressions](suppressions) win over both: they silence a finding at +one site regardless of config. diff --git a/website/docs/explain.md b/website/docs/explain.md index 02cb2f4..0cb0404 100644 --- a/website/docs/explain.md +++ b/website/docs/explain.md @@ -85,6 +85,15 @@ In 0.2 `explain` ignored `rules:` entirely, and naming a plan rule in a config was an `unknown rule` warning — a hard error under `strict: true`. A `severity` override also wins over `seq-scan`'s row-count-derived severity. +:::caution `only:` now reaches EXPLAIN + +`only:` is a whitelist across every surface, so a config that uses it to focus +the static scan — `only: [select-star, delete-without-where]` — leaves no plan +rule enabled, and `sqlguard explain` reports nothing. If you use `only:` and +want plan findings, name the plan rules in it too, or use `disable:` instead. + +::: + Postgres plans are requested as `EXPLAIN (FORMAT JSON)` and walked recursively, so nested scans inside joins and CTEs are found. MySQL plans are requested as `EXPLAIN FORMAT=TRADITIONAL` — MySQL 9 defaults diff --git a/website/docs/n-plus-one.md b/website/docs/n-plus-one.md index a2d72aa..175680c 100644 --- a/website/docs/n-plus-one.md +++ b/website/docs/n-plus-one.md @@ -28,7 +28,20 @@ sqlguard.Register("sqlguard-pg", "pgx", ) ``` -`WithN1Detection(threshold, window)` is off by default. When enabled, every +`WithN1Detection(threshold, window)` is off by default. _Added in 0.3._ It can +also be switched on from [`.sqlguard.yml`](configuration), which is the only +way to enable it without a code change: + +```yaml +rules: + settings: + n-plus-one: + threshold: 5 # both keys are required; one alone does nothing + window: 2s +``` + +`WithN1Detection` in code wins over those values, but `disable: [n-plus-one]` +in the file switches detection off regardless. When enabled, every executed statement is reduced to its [fingerprint](redaction) — literals replaced, whitespace collapsed, `IN (?, ?, ?)` folded to `IN (?)` — and counted. When the same fingerprint reaches `threshold` executions inside diff --git a/website/docs/rules.md b/website/docs/rules.md index c3ae763..5dc6997 100644 --- a/website/docs/rules.md +++ b/website/docs/rules.md @@ -192,8 +192,11 @@ legitimate query. ### `n-plus-one` Emitted by the middleware when the same query fingerprint executes -`threshold` times inside `window`. Off unless `WithN1Detection` is set. -Full description in [N+1 detection](n-plus-one). +`threshold` times inside `window`. Off unless it is switched on — with +`WithN1Detection` in Go, or by setting both +`rules.settings.n-plus-one.threshold` and `.window` in +[config](configuration) _0.3+_. Full description in +[N+1 detection](n-plus-one). > **Fix:** Consider using a `JOIN` or `IN` clause to batch these queries. @@ -201,7 +204,8 @@ Full description in [N+1 detection](n-plus-one). Emitted when a successful query's latency, measured at the driver, reaches the threshold (`WithSlowQueryThreshold`, default 200 ms; or -`slow-query.threshold` in config). The message includes the measured time +`rules.settings.slow-query.threshold` in config — _changed in 0.3_, this was +a top-level `slow-query.threshold` key). The message includes the measured time and the threshold. Reported on every slow execution — it is not [de-duplicated](noise-control). @@ -209,8 +213,10 @@ and the threshold. Reported on every slow execution — it is not ## EXPLAIN rules -Produced by [`sqlguard explain`](explain) from the query plan. They are not -configurable through `rules:` in `.sqlguard.yml`. +Produced by [`sqlguard explain`](explain) from the query plan. _Changed in +0.3._ These are configurable through `rules:` in `.sqlguard.yml` like any +other rule; in 0.2 they were not, and naming one was an `unknown rule` +warning. ### `seq-scan` (PostgreSQL) From aeebae695728c56afe84d64c44292facc409895c Mon Sep 17 00:00:00 2001 From: kartik Date: Fri, 25 Sep 2026 09:28:46 +0530 Subject: [PATCH 03/11] fix(explain): honor `disable` but ignore `only` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `only:` is a whitelist over every registered rule, so once the plan rules became addressable a config written to focus the static scan — `only: [select-star, delete-without-where]` — left no plan rule enabled and `sqlguard explain` reported nothing. A config that never mentions EXPLAIN should not silently turn that command into one that always exits clean. `only:` selects which rules the analyzer runs over a statement, so it keeps applying to the scan and to the runtime findings. Naming a rule is the part that has to reach everywhere: `disable:` and `severity: off` still switch a plan rule off, including alongside a whitelist. Analyzer.RuleDisabledExplicitly answers the narrower question and is deliberately not the complement of RuleEnabled — a footgun worth the two tests that pin it, since a future reader will reasonably expect otherwise. --- AGENTS.md | 2 +- CHANGELOG.md | 9 +++---- analyzer/analyzer.go | 44 +++++++++++++++++++++++++---------- analyzer/registry_test.go | 33 ++++++++++++++++++++++++++ explain/explain.go | 7 +++++- explain/rule_profile_test.go | 34 +++++++++++++++++++++++---- website/docs/configuration.md | 9 +++++++ website/docs/explain.md | 13 ++++------- 8 files changed, 121 insertions(+), 30 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 7c9455f..92d37d1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -47,7 +47,7 @@ When changing the public API or Go version, update all nine `go.mod` files and ` **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 — `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.checkDurationSettings` validates duration-shaped settings because `Settings.Duration` falls back to the caller's default on a bad value, which would otherwise turn a typo into a silently wrong threshold. +**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 — except `explain`, which asks `RuleDisabledExplicitly`, deliberately **not** the complement of `RuleEnabled`: it honors `disable:` and `severity: off` but ignores an `only:` whitelist, because a list written to focus `sqlguard scan` would otherwise leave that separate command silently reporting nothing (pinned by `TestApplyProfile_IgnoresOnly` and `TestRuleDisabledExplicitlyIgnoresOnly`) — `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.checkDurationSettings` validates duration-shaped settings because `Settings.Duration` falls back to the caller's default on a bad value, which would otherwise turn a typo into a silently wrong threshold. **`config` is the only YAML-aware package.** It loads `.sqlguard.yml` (`Load`/`Discover` walks up to the git root), translates it to an `analyzer.Profile`, and exposes `MiddlewareOptions()`/`Middleware()` helpers. It depends on `analyzer` (and `middleware` for the helper); nothing depends on `config`. This keeps `gopkg.in/yaml.v3` out of the `analyzer`/`middleware` import graph for library users who don't opt into file config. Parsing is lenient by default (unknown keys/rules warn); `strict: true` makes them fatal — so a newer config still loads on an older binary. diff --git a/CHANGELOG.md b/CHANGELOG.md index 4accabe..cb962f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,10 +39,11 @@ the same version in lockstep. `WithSlowQueryThreshold` in Go still wins over the file. - **`explain` now honors `rules:` config.** It previously ignored it by design, which is what its docs said. A `severity` override also beats - `seq-scan`'s row-count-derived severity. **Note that `only:` reaches - EXPLAIN too**: a config using `only:` to focus the static scan now leaves no - plan rule enabled, so `sqlguard explain` reports nothing until the plan - rules are named in it (or `disable:` is used instead). + `seq-scan`'s row-count-derived severity. `only:` is the exception and does + **not** reach EXPLAIN: a whitelist is written to focus a scan, and letting + it through would turn a config that never mentions EXPLAIN into one that + silently reports nothing there. Switching a plan rule off takes naming it in + `disable:` or `severity: off`. - **N+1 detection can be enabled from the config file.** Setting both `rules.settings.n-plus-one.threshold` and `.window` turns it on; it was previously reachable only from Go via `WithN1Detection`, which still takes diff --git a/analyzer/analyzer.go b/analyzer/analyzer.go index fa5d71d..ac1401f 100644 --- a/analyzer/analyzer.go +++ b/analyzer/analyzer.go @@ -35,8 +35,12 @@ type Analyzer struct { parser Parser severity map[string]Severity // disabled is the resolved skip decision for every registered rule, - // including those this Analyzer does not run — see RuleEnabled. + // including those this Analyzer does not run — see RuleEnabled. It folds + // in the `only` whitelist. disabled map[string]bool + // disabledByName holds only the rules turned off by name, without the + // whitelist — see RuleDisabledExplicitly. + disabledByName map[string]bool // settings holds per-rule tunables for the same audience; the statement // rules have theirs baked in by their factory at construction. settings map[string]Settings @@ -111,7 +115,11 @@ func Default() *Analyzer { func DefaultWithProfile(p Profile) *Analyzer { var bound []boundRule disabled := make(map[string]bool, len(specs())) + disabledByName := make(map[string]bool, len(p.Disabled)) for _, spec := range specs() { + if p.Disabled[spec.Name] { + disabledByName[spec.Name] = true + } if p.skip(spec.Name) { disabled[spec.Name] = true continue @@ -139,25 +147,37 @@ func DefaultWithProfile(p Profile) *Analyzer { maps.Copy(settings, p.Settings) } return &Analyzer{ - rules: bound, - parser: NewFallbackParser(), - severity: sev, - disabled: disabled, - settings: settings, - rawQuery: p.RawQuery, + rules: bound, + parser: NewFallbackParser(), + severity: sev, + disabled: disabled, + disabledByName: disabledByName, + settings: settings, + rawQuery: p.RawQuery, } } -// RuleEnabled reports whether the active profile leaves the named rule on. It -// answers for every registered rule, including the ones the Analyzer does not -// evaluate: middleware asks before emitting `slow-query` or `n-plus-one`, and -// explain asks before emitting a plan finding, so `disable` and `only` mean -// the same thing across all three surfaces. +// RuleEnabled reports whether the active profile leaves the named rule on, +// applying both `disable` (including `severity: off`) and the `only` +// whitelist. It answers for every registered rule, including the ones the +// Analyzer does not evaluate: middleware asks before emitting `slow-query` or +// `n-plus-one`. // // An unregistered name is reported as enabled: an Analyzer built with New has // no profile, and a caller's own rule is not the profile's to turn off. func (a *Analyzer) RuleEnabled(name string) bool { return !a.disabled[name] } +// RuleDisabledExplicitly reports whether the profile turned the named rule off +// by naming it — `disable:`, or `severity: off`, which resolves to the same +// thing. +// +// It is deliberately **not** the complement of RuleEnabled: it ignores the +// `only` whitelist. `only:` selects which rules the analyzer runs over a +// statement, and a list written to focus a scan should not also blank out +// `sqlguard explain`, which is a separate command reporting on a plan the +// database produced. Turning a plan rule off there takes naming it. +func (a *Analyzer) RuleDisabledExplicitly(name string) bool { return a.disabledByName[name] } + // RuleSeverity returns the severity to report for name, applying a profile // override to def when one is set. func (a *Analyzer) RuleSeverity(name string, def Severity) Severity { diff --git a/analyzer/registry_test.go b/analyzer/registry_test.go index a38e013..bdf03f7 100644 --- a/analyzer/registry_test.go +++ b/analyzer/registry_test.go @@ -74,3 +74,36 @@ func TestOnlyWhitelistReachesNonEvaluatedRules(t *testing.T) { t.Error("the whitelisted rule should stay enabled") } } + +// TestRuleDisabledExplicitlyIgnoresOnly pins the one place where +// RuleDisabledExplicitly is deliberately not the complement of RuleEnabled. +// A whitelist turns a rule off for the analyzer and the runtime, but it does +// not count as naming that rule, so `explain` keeps reporting it. +func TestRuleDisabledExplicitlyIgnoresOnly(t *testing.T) { + a := DefaultWithProfile(Profile{Only: map[string]bool{"select-star": true}}) + + if a.RuleEnabled("seq-scan") { + t.Error("an only whitelist should leave seq-scan disabled for RuleEnabled") + } + if a.RuleDisabledExplicitly("seq-scan") { + t.Error("a whitelist is not the same as naming seq-scan in disable:") + } +} + +func TestRuleDisabledExplicitlyHonoursDisable(t *testing.T) { + a := DefaultWithProfile(Profile{Disabled: map[string]bool{"seq-scan": true}}) + + if !a.RuleDisabledExplicitly("seq-scan") { + t.Error("seq-scan was named in disable: and should be reported so") + } + if a.RuleEnabled("seq-scan") { + t.Error("RuleEnabled should agree when the rule was named") + } + if a.RuleDisabledExplicitly("filesort") { + t.Error("filesort was not named and should not be reported disabled") + } + // An unregistered name is nobody's to turn off. + if a.RuleDisabledExplicitly("somebody-elses-rule") { + t.Error("an unregistered rule should never read as disabled") + } +} diff --git a/explain/explain.go b/explain/explain.go index 38512d6..1ea5d4a 100644 --- a/explain/explain.go +++ b/explain/explain.go @@ -108,6 +108,11 @@ func (p *PlanAnalyzer) Analyze(ctx context.Context, query string) (*Result, erro // override. It runs once over the collected issues rather than at each site // that builds one, so a plan rule added later cannot forget the check. // +// Only a rule named in `disable:` (or given `severity: off`) is dropped. An +// `only:` whitelist is ignored here: it scopes which rules run over a +// statement, and a list written to focus `sqlguard scan` would otherwise +// leave this command silently reporting nothing. +// // A severity override wins over a computed severity: `seq-scan` picks INFO or // WARNING from the estimated row count, and an explicit setting outranks both. func (p *PlanAnalyzer) applyProfile(issues []analyzer.Result) []analyzer.Result { @@ -116,7 +121,7 @@ func (p *PlanAnalyzer) applyProfile(issues []analyzer.Result) []analyzer.Result } kept := issues[:0] for _, r := range issues { - if !p.rules.RuleEnabled(r.RuleName) { + if p.rules.RuleDisabledExplicitly(r.RuleName) { continue } r.Severity = p.rules.RuleSeverity(r.RuleName, r.Severity) diff --git a/explain/rule_profile_test.go b/explain/rule_profile_test.go index fb78ae9..5f35763 100644 --- a/explain/rule_profile_test.go +++ b/explain/rule_profile_test.go @@ -39,15 +39,41 @@ func TestApplyProfile_Disable(t *testing.T) { } } -func TestApplyProfile_Only(t *testing.T) { +// TestApplyProfile_IgnoresOnly pins the asymmetry. `only:` is overwhelmingly +// written to focus `sqlguard scan`, and it selects which rules run over a +// statement; letting it reach here would mean a config that never mentions +// EXPLAIN silently turns `sqlguard explain` into a command that always reports +// nothing. Turning a plan rule off takes naming it in `disable:`. +func TestApplyProfile_IgnoresOnly(t *testing.T) { p := &PlanAnalyzer{rules: analyzer.DefaultWithProfile(analyzer.Profile{ - Only: map[string]bool{"filesort": true}, + Only: map[string]bool{"select-star": true}, })} got := p.applyProfile(planIssues()) - if len(got) != 1 || got[0].RuleName != "filesort" { - t.Errorf("an `only` whitelist should leave just filesort, got %+v", got) + if len(got) != 3 { + t.Errorf("an `only` whitelist should not filter plan findings, got %+v", got) + } +} + +// TestApplyProfile_DisableWinsInsideAnOnlyList is the escape hatch: `only:` +// does not reach a plan rule, but naming one in `disable:` still does, even +// alongside a whitelist. +func TestApplyProfile_DisableWinsInsideAnOnlyList(t *testing.T) { + p := &PlanAnalyzer{rules: analyzer.DefaultWithProfile(analyzer.Profile{ + Only: map[string]bool{"select-star": true}, + Disabled: map[string]bool{"high-cost": true}, + })} + + got := p.applyProfile(planIssues()) + + if len(got) != 2 { + t.Fatalf("expected the other two to survive, got %+v", got) + } + for _, r := range got { + if r.RuleName == "high-cost" { + t.Error("an explicit disable should still drop the rule") + } } } diff --git a/website/docs/configuration.md b/website/docs/configuration.md index 812d297..c27f4fc 100644 --- a/website/docs/configuration.md +++ b/website/docs/configuration.md @@ -174,5 +174,14 @@ being built at all despite `WithN1Detection`. That is deliberate — `disable` is an instruction, not a tuning value, and an operator editing `.sqlguard.yml` should be able to silence a noisy rule without a redeploy. +## What `only:` reaches + +`only:` applies to the static scan and to the runtime findings, but **not to +[`sqlguard explain`](explain)**. A whitelist is nearly always written to focus +a scan, and it selects which rules run over a statement; if it reached the +plan rules, a config that never mentions EXPLAIN would quietly turn that +command into one that always reports nothing. To switch a plan rule off, name +it in `disable:` or give it `severity: off` — both reach every surface. + Inline [suppressions](suppressions) win over both: they silence a finding at one site regardless of config. diff --git a/website/docs/explain.md b/website/docs/explain.md index 0cb0404..6869118 100644 --- a/website/docs/explain.md +++ b/website/docs/explain.md @@ -85,14 +85,11 @@ In 0.2 `explain` ignored `rules:` entirely, and naming a plan rule in a config was an `unknown rule` warning — a hard error under `strict: true`. A `severity` override also wins over `seq-scan`'s row-count-derived severity. -:::caution `only:` now reaches EXPLAIN - -`only:` is a whitelist across every surface, so a config that uses it to focus -the static scan — `only: [select-star, delete-without-where]` — leaves no plan -rule enabled, and `sqlguard explain` reports nothing. If you use `only:` and -want plan findings, name the plan rules in it too, or use `disable:` instead. - -::: +`disable:` and `severity:` apply here; **`only:` does not**. A whitelist is +almost always written to focus `sqlguard scan`, and it selects which rules run +over a statement — letting it reach this command would mean a config that +never mentions EXPLAIN silently turns it into one that always reports nothing. +Switching a plan rule off takes naming it. Postgres plans are requested as `EXPLAIN (FORMAT JSON)` and walked recursively, so nested scans inside joins and CTEs are found. MySQL plans From 02ac9ad52b884b1949e2f001953c4a935ef60b11 Mon Sep 17 00:00:00 2001 From: kartik Date: Fri, 25 Sep 2026 09:41:49 +0530 Subject: [PATCH 04/11] fix(config): close the remaining silent-typo paths, read the registry severity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nine review findings on the whole branch; all were real. `only:` was the biggest. Making the seven addressable also made `only: [slow-query]` a valid list — and `p.skip` folds the whitelist in before the Evaluated() check, so it bound no rules and `sqlguard scan` reported nothing on any codebase, which is indistinguishable from a clean run. A list that names nothing runnable is now reported. `EvaluatedRuleNames` is what lets config tell the two apart. Settings validation only covered values, so the one typo left silent was the key: `slow-query: {threshhold: 1s}` named a known rule with a well-formed value, and the threshold quietly stayed at 200ms — the exact failure the validation exists to prevent. ruleSettings is now the complete tunable set, so an unknown key on a listed rule and any key on a rule with no tunables both report. The duration branch also passed anything that was not a string, so `window: true` cleared both checks and left N+1 off; it now accepts only a duration string or a number. A non-positive `n-plus-one.threshold` reports rather than silently meaning "off". DefaultSeverity was decorative for all seven: middleware and explain each repeated a literal, so editing the registry entry would have done nothing despite the comment calling it the single source of truth. Both read RuleDefaultSeverity now. seq-scan still escalates on row count, but from the registered default rather than a hardcoded INFO. Docs: `only` does not ignore `disable` (skip checks disable after the whitelist); `settings` does not reach the five plan rules, which read none; and AGENTS.md named checkDurationSettings, which this branch had already renamed. --- AGENTS.md | 2 +- CHANGELOG.md | 7 ++ analyzer/analyzer.go | 5 +- analyzer/registry.go | 31 +++++++++ analyzer/registry_test.go | 51 +++++++++++++++ config/config.go | 116 +++++++++++++++++++++++++++------- config/config_test.go | 97 ++++++++++++++++++++++++++++ explain/explain.go | 22 +++++-- middleware/guard.go | 13 +++- website/docs/configuration.md | 5 +- website/docs/rules.md | 15 +++-- 11 files changed, 325 insertions(+), 39 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 92d37d1..60ee07c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -47,7 +47,7 @@ When changing the public API or Go version, update all nine `go.mod` files and ` **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 — except `explain`, which asks `RuleDisabledExplicitly`, deliberately **not** the complement of `RuleEnabled`: it honors `disable:` and `severity: off` but ignores an `only:` whitelist, because a list written to focus `sqlguard scan` would otherwise leave that separate command silently reporting nothing (pinned by `TestApplyProfile_IgnoresOnly` and `TestRuleDisabledExplicitlyIgnoresOnly`) — `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.checkDurationSettings` validates duration-shaped settings because `Settings.Duration` falls back to the caller's default on a bad value, which would otherwise turn a typo into a silently wrong threshold. +**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 — except `explain`, which asks `RuleDisabledExplicitly`, deliberately **not** the complement of `RuleEnabled`: it honors `disable:` and `severity: off` but ignores an `only:` whitelist, because a list written to focus `sqlguard scan` would otherwise leave that separate command silently reporting nothing (pinned by `TestApplyProfile_IgnoresOnly` and `TestRuleDisabledExplicitlyIgnoresOnly`) — `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.checkOnlySelectsSomething` covers the shape that only became expressible once the seven were registered: `only: [slow-query]` is now a valid list that leaves the scanner nothing to run, which is indistinguishable from a clean scan. **`config` is the only YAML-aware package.** It loads `.sqlguard.yml` (`Load`/`Discover` walks up to the git root), translates it to an `analyzer.Profile`, and exposes `MiddlewareOptions()`/`Middleware()` helpers. It depends on `analyzer` (and `middleware` for the helper); nothing depends on `config`. This keeps `gopkg.in/yaml.v3` out of the `analyzer`/`middleware` import graph for library users who don't opt into file config. Parsing is lenient by default (unknown keys/rules warn); `strict: true` makes them fatal — so a newer config still loads on an older binary. diff --git a/CHANGELOG.md b/CHANGELOG.md index cb962f3..9e3bdc1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,6 +53,13 @@ the same version in lockstep. covers durations and numbers, and a half-specified `n-plus-one` block — a quoted `threshold: "10"` is a string in YAML, read back as 0, which would have left N+1 detection off with no indication. +- An `only:` list that names no rule the scanner runs — `only: [slow-query]`, + now that it is a valid name — is reported. It would otherwise leave + `sqlguard scan` finding nothing on any codebase, which reads as a clean run. +- A misspelled setting **key** is reported too, not just a bad value: the rule + name is known and the value well-formed, so the setting is simply absent and + the built-in default stands. A `settings` block on a rule with no tunables + (the five plan rules) is reported the same way. - An unknown rule name in `disable:` / `only:` / `severity:` / `settings:` is now warned about **and ignored**, rather than warned about and honored. One typo in `only:` acted as a whitelist matching nothing, which since every diff --git a/analyzer/analyzer.go b/analyzer/analyzer.go index ac1401f..5d5e6c9 100644 --- a/analyzer/analyzer.go +++ b/analyzer/analyzer.go @@ -113,10 +113,11 @@ func Default() *Analyzer { // The config package uses this to turn a .sqlguard.yml into an Analyzer // without analyzer ever importing config or YAML. func DefaultWithProfile(p Profile) *Analyzer { + all := specs() var bound []boundRule - disabled := make(map[string]bool, len(specs())) + disabled := make(map[string]bool, len(all)) disabledByName := make(map[string]bool, len(p.Disabled)) - for _, spec := range specs() { + for _, spec := range all { if p.Disabled[spec.Name] { disabledByName[spec.Name] = true } diff --git a/analyzer/registry.go b/analyzer/registry.go index 8b08572..99b4fce 100644 --- a/analyzer/registry.go +++ b/analyzer/registry.go @@ -123,6 +123,37 @@ func RuleNames() []string { return names } +// EvaluatedRuleNames returns the rules the Analyzer runs against a Statement, +// sorted — the subset of RuleNames() that has a Factory. The config loader +// uses it to tell an `only:` list that selects nothing runnable from one that +// narrows the scan, which reads identically in YAML. +func EvaluatedRuleNames() []string { + registryMu.RLock() + names := make([]string, 0, len(registry)) + for n, spec := range registry { + if spec.Evaluated() { + names = append(names, n) + } + } + registryMu.RUnlock() + sort.Strings(names) + return names +} + +// RuleDefaultSeverity returns the severity a rule was registered with. ok is +// false for an unregistered name. The findings built outside the statement +// path read this rather than repeating a literal, so the registry entry stays +// the single source of truth for every rule, not just the evaluated ones. +func RuleDefaultSeverity(name string) (sev Severity, ok bool) { + registryMu.RLock() + defer registryMu.RUnlock() + spec, found := registry[name] + if !found { + return 0, false + } + return spec.DefaultSeverity, true +} + // specs returns all registered specs sorted by name, for deterministic // analyzer construction and stable report ordering. func specs() []RuleSpec { diff --git a/analyzer/registry_test.go b/analyzer/registry_test.go index bdf03f7..499127f 100644 --- a/analyzer/registry_test.go +++ b/analyzer/registry_test.go @@ -107,3 +107,54 @@ func TestRuleDisabledExplicitlyHonoursDisable(t *testing.T) { t.Error("an unregistered rule should never read as disabled") } } + +// TestRuleDefaultSeverityCoversNonEvaluatedRules makes the registry entries +// for the seven load-bearing. Nothing constructs them, so without a reader +// their DefaultSeverity would be decorative and the literals at each build +// site would silently outrank it. +func TestRuleDefaultSeverityCoversNonEvaluatedRules(t *testing.T) { + want := map[string]Severity{ + "slow-query": SeverityWarning, + "n-plus-one": SeverityWarning, + "seq-scan": SeverityInfo, + "high-cost": SeverityWarning, + "full-table-scan": SeverityWarning, + "no-index-used": SeverityWarning, + "filesort": SeverityInfo, + } + for name, sev := range want { + got, ok := RuleDefaultSeverity(name) + if !ok { + t.Errorf("%q is not registered", name) + continue + } + if got != sev { + t.Errorf("%q default severity = %v, want %v", name, got, sev) + } + } + + if _, ok := RuleDefaultSeverity("somebody-elses-rule"); ok { + t.Error("an unregistered name should report ok=false") + } +} + +// TestEvaluatedRuleNamesExcludesTheSeven is what lets config tell an `only:` +// list that narrows the scan from one that leaves it with nothing to run. +func TestEvaluatedRuleNamesExcludesTheSeven(t *testing.T) { + evaluated := EvaluatedRuleNames() + for _, name := range []string{ + "slow-query", "n-plus-one", + "seq-scan", "high-cost", "full-table-scan", "no-index-used", "filesort", + } { + if slices.Contains(evaluated, name) { + t.Errorf("%q has no Factory and should not be listed as evaluated", name) + } + } + if !slices.Contains(evaluated, "select-star") { + t.Error("select-star is evaluated and should be listed") + } + if len(evaluated) != len(RuleNames())-7 { + t.Errorf("evaluated=%d, registered=%d; expected exactly 7 non-evaluated", + len(evaluated), len(RuleNames())) + } +} diff --git a/config/config.go b/config/config.go index 44719d9..76fc31c 100644 --- a/config/config.go +++ b/config/config.go @@ -15,6 +15,7 @@ import ( "os" "path/filepath" "regexp" + "sort" "strings" "time" @@ -200,6 +201,9 @@ func (c *Config) Profile() (analyzer.Profile, error) { if err := collectNames(c.Rules.Only, p.Only, checkName); err != nil { return p, err } + if err := checkOnlySelectsSomething(p.Only, warn); err != nil { + return p, err + } if err := applySeverities(c.Rules.Severity, &p, checkName, warn); err != nil { return p, err } @@ -219,6 +223,25 @@ func (c *Config) Profile() (analyzer.Profile, error) { return p, nil } +// checkOnlySelectsSomething reports an `only:` list that leaves no rule for +// the analyzer to run. Since every documented rule became addressable, a list +// naming only runtime or plan rules — `only: [slow-query]` — is accepted and +// then turns `sqlguard scan` into a command that reports nothing on any +// codebase, which looks exactly like a clean scan. Before, those names were +// rejected outright as unknown, so the shape could not arise. +func checkOnlySelectsSomething(only map[string]bool, warn func(string, ...any) error) error { + if len(only) == 0 { + return nil + } + for _, name := range analyzer.EvaluatedRuleNames() { + if only[name] { + return nil + } + } + return warn("rules.only names no rule that runs over a statement, so nothing will be scanned " + + "(runtime and EXPLAIN rules are reported by the middleware and `sqlguard explain`, not the scanner)") +} + // collectNames adds each usable name to the set. An unknown name is reported // by checkName and then left out: warning about a typo and acting on it // anyway is how one bad entry in `only:` becomes a whitelist matching nothing. @@ -279,13 +302,20 @@ type settingKind int const ( settingDuration settingKind = iota settingInt + // settingPositiveInt additionally rejects zero and negatives, for a + // tunable where a non-positive value silently means "off". + settingPositiveInt ) -// checkedSettings names the settings that are read as something other than an -// opaque value. Rules whose settings are not listed pass through unchecked. -var checkedSettings = map[string]map[string]settingKind{ +// ruleSettings is the complete set of tunables, by rule and key. It is +// complete on purpose: a rule absent from this map reads no settings at all, +// so any key given for it is a mistake, and a key absent from a listed rule is +// a misspelling. Either way the value is silently ignored at read time, which +// is the failure this validation exists to prevent. Adding a tunable to a rule +// means adding it here. +var ruleSettings = map[string]map[string]settingKind{ "slow-query": {"threshold": settingDuration}, - "n-plus-one": {"threshold": settingInt, "window": settingDuration}, + "n-plus-one": {"threshold": settingPositiveInt, "window": settingDuration}, "leading-wildcard": {"min-length": settingInt}, "in-list-too-large": {"max-length": settingInt}, "large-offset": {"threshold": settingInt}, @@ -297,12 +327,23 @@ var pairedSettings = map[string][]string{ "n-plus-one": {"threshold", "window"}, } -// checkSettings reports a setting whose value will not read back as its kind, -// and a half-specified pair. +// checkSettings reports a setting key the rule does not have, a value that +// will not read back as its kind, and a half-specified pair. func checkSettings(rule string, kv map[string]any, warn func(string, ...any) error) error { - for key, kind := range checkedSettings[rule] { - v, present := kv[key] - if !present { + kinds, tunable := ruleSettings[rule] + for key, v := range kv { + kind, known := kinds[key] + if !known { + if !tunable { + if err := warn("rule %q has no settings, so %q is ignored", rule, key); err != nil { + return err + } + continue + } + if err := warn("rule %q: unknown setting %q (known: %s)", + rule, key, strings.Join(sortedKeys(kinds), ", ")); err != nil { + return err + } continue } if err := checkSettingValue(rule, key, kind, v, warn); err != nil { @@ -332,27 +373,56 @@ func checkSettings(rule string, kv map[string]any, warn func(string, ...any) err func checkSettingValue(rule, key string, kind settingKind, v any, warn func(string, ...any) error) error { switch kind { case settingDuration: - str, isStr := v.(string) - if !isStr { - return nil // a bare number is milliseconds; Settings handles it + switch tv := v.(type) { + case string: + if _, err := time.ParseDuration(strings.TrimSpace(tv)); err != nil { + return warn("rule %q: setting %q: invalid duration %q", rule, key, tv) + } + case int, int64, float64: + // A bare number is milliseconds; Settings.Duration handles it. + default: + // Anything else — a bool, a list, a map — reads back as the + // default, which for n-plus-one.window means detection silently + // never switches on. + return warn("rule %q: setting %q: expected a duration or a number, got %v", rule, key, v) } - if _, err := time.ParseDuration(strings.TrimSpace(str)); err != nil { - return warn("rule %q: setting %q: invalid duration %q", rule, key, str) + case settingInt, settingPositiveInt: + n, ok := asInt(v) + if !ok { + // A quoted number is the common YAML slip. Settings.Int does not + // accept a string, so it would read back as the default: for + // n-plus-one.threshold that means detection never switches on. + return warn("rule %q: setting %q: expected a number, got %v (quoted numbers are strings in YAML)", + rule, key, v) } - case settingInt: - switch v.(type) { - case int, int64, float64: - return nil + if kind == settingPositiveInt && n <= 0 { + return warn("rule %q: setting %q must be greater than 0, got %d", rule, key, n) } - // A quoted number is the common YAML slip. Settings.Int does not - // accept a string, so it would read back as the default: for - // n-plus-one.threshold that means detection never switches on. - return warn("rule %q: setting %q: expected a number, got %v (quoted numbers are strings in YAML)", - rule, key, v) } return nil } +func asInt(v any) (int, bool) { + switch n := v.(type) { + case int: + return n, true + case int64: + return int(n), true + case float64: + return int(n), true + } + return 0, false +} + +func sortedKeys(m map[string]settingKind) []string { + out := make([]string, 0, len(m)) + for k := range m { + out = append(out, k) + } + sort.Strings(out) + return out +} + // rawQuery reports whether Result.Query redaction is disabled. Redaction is // the default (PII-safe); only an explicit `redact: false` turns it off. func (c *Config) rawQuery() bool { return c.Redact != nil && !*c.Redact } diff --git a/config/config_test.go b/config/config_test.go index 163d264..d716d77 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -3,6 +3,7 @@ package config import ( "os" "path/filepath" + "strings" "testing" "time" @@ -320,3 +321,99 @@ func TestProfile_UnknownNameIsIgnoredNotHonoured(t *testing.T) { } } } + +// TestProfile_OnlySelectingNothingRunnableWarns covers a shape that could not +// exist before every rule became addressable: `only: [slow-query]` is now a +// valid list that leaves the scanner with no rule to run, so `sqlguard scan` +// reports nothing on any codebase and CI goes green on a tree full of +// SELECT *. It has to say so. +func TestProfile_OnlySelectingNothingRunnableWarns(t *testing.T) { + for _, only := range [][]string{ + {"slow-query"}, + {"seq-scan", "filesort"}, + {"slow-query", "n-plus-one", "high-cost"}, + } { + t.Run(strings.Join(only, ","), func(t *testing.T) { + c := &Config{Rules: RulesConfig{Only: only}} + if _, err := c.Profile(); err != nil { + t.Fatalf("lenient mode should not fail: %v", err) + } + if len(c.Warnings()) != 1 { + t.Fatalf("expected a warning, got %v", c.Warnings()) + } + if !strings.Contains(c.Warnings()[0], "nothing will be scanned") { + t.Errorf("unexpected warning: %v", c.Warnings()[0]) + } + }) + } + + t.Run("a list with one runnable rule is fine", func(t *testing.T) { + c := &Config{Strict: true, Rules: RulesConfig{Only: []string{"slow-query", "select-star"}}} + if _, err := c.Profile(); err != nil { + t.Errorf("a mixed list should be accepted: %v", err) + } + }) + + t.Run("no only list is fine", func(t *testing.T) { + c := &Config{Strict: true, Rules: RulesConfig{Disable: []string{"slow-query"}}} + if _, err := c.Profile(); err != nil { + t.Errorf("unexpected error: %v", err) + } + }) +} + +// TestProfile_ValidatesSettingKeys closes the last silent typo: the rule name +// is known and the value is well-formed, but the key is misspelled, so the +// setting is simply absent and the built-in default stands. +func TestProfile_ValidatesSettingKeys(t *testing.T) { + cases := []struct { + name string + settings map[string]map[string]any + wantIn string + }{ + {"misspelled key on a tunable rule", + //nolint:misspell // the typo is the fixture: this is the case under test + map[string]map[string]any{"slow-query": {"threshhold": "1s"}}, "unknown setting"}, + {"key on a rule with no settings", + map[string]map[string]any{"select-star": {"threshold": 1}}, "has no settings"}, + {"non-numeric, non-string duration", + map[string]map[string]any{"n-plus-one": {"threshold": 10, "window": true}}, "expected a duration"}, + {"zero threshold means off, silently", + map[string]map[string]any{"n-plus-one": {"threshold": 0, "window": "1m"}}, "greater than 0"}, + {"negative threshold", + map[string]map[string]any{"n-plus-one": {"threshold": -1, "window": "1m"}}, "greater than 0"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + c := &Config{Rules: RulesConfig{Settings: tc.settings}} + if _, err := c.Profile(); err != nil { + t.Fatalf("lenient mode should not fail: %v", err) + } + if len(c.Warnings()) == 0 { + t.Fatal("expected a warning") + } + if !strings.Contains(c.Warnings()[0], tc.wantIn) { + t.Errorf("warning %q does not mention %q", c.Warnings()[0], tc.wantIn) + } + + strict := &Config{Strict: true, Rules: RulesConfig{Settings: tc.settings}} + if _, err := strict.Profile(); err == nil { + t.Error("expected strict mode to reject it") + } + }) + } + + t.Run("every documented key is accepted", func(t *testing.T) { + c := &Config{Strict: true, Rules: RulesConfig{Settings: map[string]map[string]any{ + "slow-query": {"threshold": "500ms"}, + "n-plus-one": {"threshold": 5, "window": "2s"}, + "leading-wildcard": {"min-length": 4}, + "in-list-too-large": {"max-length": 50}, + "large-offset": {"threshold": 2000}, + }}} + if _, err := c.Profile(); err != nil { + t.Errorf("documented settings rejected: %v", err) + } + }) +} diff --git a/explain/explain.go b/explain/explain.go index 1ea5d4a..77c37e1 100644 --- a/explain/explain.go +++ b/explain/explain.go @@ -104,6 +104,18 @@ func (p *PlanAnalyzer) Analyze(ctx context.Context, query string) (*Result, erro return res, err } +// planSeverity is the severity a plan rule was registered with. Reading it +// keeps the registry entry meaningful for these rules too — they have no +// Factory, so nothing else would ever consult their DefaultSeverity, and a +// literal here would silently outrank it. +func planSeverity(name string) analyzer.Severity { + sev, ok := analyzer.RuleDefaultSeverity(name) + if !ok { + return analyzer.SeverityWarning + } + return sev +} + // applyProfile drops findings the profile disabled and applies any severity // override. It runs once over the collected issues rather than at each site // that builds one, so a plan rule added later cannot forget the check. @@ -237,7 +249,7 @@ func (p *PlanAnalyzer) walkPgPlan(node *pgPlanNode, query string, issues *[]anal // Detect sequential scans if node.NodeType == "Seq Scan" { - severity := analyzer.SeverityInfo + severity := planSeverity("seq-scan") if node.PlanRows > 1000 { severity = analyzer.SeverityWarning } @@ -254,7 +266,7 @@ func (p *PlanAnalyzer) walkPgPlan(node *pgPlanNode, query string, issues *[]anal if node.TotalCost > 10000 { *issues = append(*issues, analyzer.Result{ RuleName: "high-cost", - Severity: analyzer.SeverityWarning, + Severity: planSeverity("high-cost"), Query: query, Message: fmt.Sprintf("High cost operation: %s (cost %.1f)", node.NodeType, node.TotalCost), Suggestion: "Review query plan and consider optimization.", @@ -363,7 +375,7 @@ func mysqlRowIssues(query string, col func(string) string) []analyzer.Result { planRows, _ := strconv.ParseInt(col("rows"), 10, 64) issues = append(issues, analyzer.Result{ RuleName: "full-table-scan", - Severity: analyzer.SeverityWarning, + Severity: planSeverity("full-table-scan"), Query: query, Message: fmt.Sprintf("Full table scan on %s (estimated %d rows)", table, planRows), Suggestion: "Consider adding an index to avoid full table scan.", @@ -374,7 +386,7 @@ func mysqlRowIssues(query string, col func(string) string) []analyzer.Result { if col("key") == "" && col("possible_keys") == "" { issues = append(issues, analyzer.Result{ RuleName: "no-index-used", - Severity: analyzer.SeverityWarning, + Severity: planSeverity("no-index-used"), Query: query, Message: "No index used on table " + table, Suggestion: "Consider adding an index on the filtered/joined columns.", @@ -385,7 +397,7 @@ func mysqlRowIssues(query string, col func(string) string) []analyzer.Result { if strings.Contains(col("extra"), "Using filesort") { issues = append(issues, analyzer.Result{ RuleName: "filesort", - Severity: analyzer.SeverityInfo, + Severity: planSeverity("filesort"), Query: query, Message: "Filesort detected on table " + table, Suggestion: "Consider adding an index that covers the ORDER BY columns.", diff --git a/middleware/guard.go b/middleware/guard.go index 94df103..52ff7f6 100644 --- a/middleware/guard.go +++ b/middleware/guard.go @@ -35,7 +35,14 @@ type findingPolicy struct { severity analyzer.Severity } -func resolvePolicy(a *analyzer.Analyzer, name string, def analyzer.Severity) findingPolicy { +// resolvePolicy reads the default severity from the registry rather than +// repeating a literal here, so `Register(RuleSpec{Name: "slow-query", …})` is +// what decides it — the same as for an evaluated rule. +func resolvePolicy(a *analyzer.Analyzer, name string) findingPolicy { + def, ok := analyzer.RuleDefaultSeverity(name) + if !ok { + def = analyzer.SeverityWarning + } return findingPolicy{ enabled: a.RuleEnabled(name), severity: a.RuleSeverity(name, def), @@ -69,11 +76,11 @@ func NewGuard(opts ...Option) *Guard { } g := &Guard{opts: o, deduper: newDeduper(o.dedupWindow)} - g.slowQuery = resolvePolicy(o.analyzer, "slow-query", analyzer.SeverityWarning) + g.slowQuery = resolvePolicy(o.analyzer, "slow-query") if o.cacheSize > 0 { g.cache = newAnalysisCache(o.cacheSize) } - n1 := resolvePolicy(o.analyzer, "n-plus-one", analyzer.SeverityWarning) + n1 := resolvePolicy(o.analyzer, "n-plus-one") if o.enableN1 && n1.enabled { g.tracker = NewQueryTracker(o.n1Threshold, o.n1Window, n1.severity, func(results []analyzer.Result) { o.reporter.Report(results) diff --git a/website/docs/configuration.md b/website/docs/configuration.md index c27f4fc..913614b 100644 --- a/website/docs/configuration.md +++ b/website/docs/configuration.md @@ -83,7 +83,7 @@ scan: | `version` | all | Reserved for forward compatibility; always `1` today. | | `strict` | all | Make unknown keys, unknown rule names and bad severities fatal instead of warnings. | | `rules.disable` | every rule | Rule names to turn off. | -| `rules.only` | every rule | Whitelist. When non-empty, only these run and `disable` is ignored. | +| `rules.only` | every rule | Whitelist. When non-empty, a rule must be listed to run — and `disable` still applies to the ones that are, so listing and disabling the same rule disables it. | | `rules.severity` | every rule | `info`, `warning`, `critical`, or `off`. | | `rules.settings` | rules with tunables | `leading-wildcard.min-length`, `in-list-too-large.max-length`, `large-offset.threshold`, `slow-query.threshold`, `n-plus-one.threshold` / `.window`. See [Rules](rules). | | `redact` | all | `false` keeps raw literals in `Result.Query`. See [Redaction](redaction). | @@ -174,6 +174,9 @@ being built at all despite `WithN1Detection`. That is deliberate — `disable` is an instruction, not a tuning value, and an operator editing `.sqlguard.yml` should be able to silence a noisy rule without a redeploy. +`only:` narrows; it does not override. A rule has to survive both checks, so +`only: [select-star]` together with `disable: [select-star]` leaves nothing. + ## What `only:` reaches `only:` applies to the static scan and to the runtime findings, but **not to diff --git a/website/docs/rules.md b/website/docs/rules.md index 5dc6997..971b7de 100644 --- a/website/docs/rules.md +++ b/website/docs/rules.md @@ -40,10 +40,17 @@ can be overridden per project. | `filesort` | INFO | EXPLAIN (mysql) | `Using filesort` in `Extra` | _Changed in 0.3._ Every rule in this table is addressable by name in -[`.sqlguard.yml`](configuration) — `disable`, `only`, `severity` and -`settings` all work the same for a runtime or plan rule as for a statement -rule. In 0.2 only the 14 statement rules were: naming any of the other seven -warned with `unknown rule`, and failed under `strict: true`. +[`.sqlguard.yml`](configuration): `disable` and `severity` work the same for a +runtime or plan rule as for a statement rule. In 0.2 only the 14 statement +rules were — naming any of the other seven warned with `unknown rule`, and +failed under `strict: true`. + +Two qualifications. `only:` applies to the scan and the runtime findings but +[not to `sqlguard explain`](explain#what-it-detects). And `settings` only +exists where a rule has a tunable: `leading-wildcard`, `in-list-too-large`, +`large-offset`, `slow-query` and `n-plus-one` have them; the five plan rules +have none and their thresholds are fixed, so a `settings` block for one is +reported as having no effect. The runtime and plan rules are not evaluated against parsed SQL — middleware derives them from latency and fingerprint counts, and `explain` from the From d309a3248d3235ba97e9e7b6dd5ddeae025decd7 Mon Sep 17 00:00:00 2001 From: kartik Date: Fri, 25 Sep 2026 09:55:45 +0530 Subject: [PATCH 05/11] fix(config): reject non-positive thresholds, stop fractional ones truncating MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six review findings; all were real. The worst was a log flood. `slow-query.threshold: 0` was accepted and matched every successful query, so the middleware reported `slow-query` on all of them — into the log sink redaction exists to protect. `0.5` did the same by a different route: Settings.Duration converted a float with `time.Duration(v) * time.Millisecond`, and time.Duration(0.5) truncates to 0, so a sub-millisecond threshold became no threshold. The conversion scales before converting now, and both duration settings reject non-positive values, the way n-plus-one.threshold already did. A zero `n-plus-one.window` passed the paired-key check and then left detection off, which is the same class. checkOnlySelectsSomething inspected the resolved set, so it could not see the typo it was written for: unknown names are dropped, `only: [selct-star]` resolves to an empty map, and an empty whitelist is not a whitelist — every rule runs, the opposite of the narrowing that was asked for. It takes the configured list now and reports that case separately. seq-scan's row-count escalation assigned SeverityWarning outright, undoing the point of reading the registry default one line above: Register can replace a built-in by name, so a seq-scan registered at CRITICAL would have reported the wide scan as less severe than the narrow one. It only escalates upward now. The migration snippet for the removed SlowQueryThreshold() did not compile — it chained a field selector off Profile(), which returns two values. Replaced with one that does, checked by building it against the package. --- CHANGELOG.md | 5 ++ analyzer/registry.go | 4 +- config/config.go | 80 ++++++++++++++++-------- config/config_test.go | 105 +++++++++++++++++++++++++++++++- explain/explain.go | 6 +- explain/rule_profile_test.go | 31 ++++++++++ middleware/rule_profile_test.go | 18 ++++++ website/docs/configuration.md | 12 +++- 8 files changed, 230 insertions(+), 31 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9e3bdc1..0f638f1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,6 +56,11 @@ the same version in lockstep. - An `only:` list that names no rule the scanner runs — `only: [slow-query]`, now that it is a valid name — is reported. It would otherwise leave `sqlguard scan` finding nothing on any codebase, which reads as a clean run. +- A non-positive `slow-query.threshold` or `n-plus-one.window` is reported. A + threshold of `0` matches every successful query, so it would have flooded + the reporter with `slow-query` on every statement; a zero window leaves N+1 + off. A fractional-millisecond threshold such as `0.5` also truncated to zero + when read, which produced the same flood — it now scales before converting. - A misspelled setting **key** is reported too, not just a bad value: the rule name is known and the value well-formed, so the setting is simply absent and the built-in default stands. A `settings` block on a rule with no tunables diff --git a/analyzer/registry.go b/analyzer/registry.go index 99b4fce..f6846e7 100644 --- a/analyzer/registry.go +++ b/analyzer/registry.go @@ -72,7 +72,9 @@ func (s Settings) Duration(key string, def time.Duration) time.Duration { case int64: return time.Duration(v) * time.Millisecond case float64: - return time.Duration(v) * time.Millisecond + // Scale before converting: time.Duration(0.5) truncates to 0, which + // turned a fractional-millisecond threshold into "no threshold". + return time.Duration(v * float64(time.Millisecond)) } return def } diff --git a/config/config.go b/config/config.go index 76fc31c..965cef6 100644 --- a/config/config.go +++ b/config/config.go @@ -201,7 +201,7 @@ func (c *Config) Profile() (analyzer.Profile, error) { if err := collectNames(c.Rules.Only, p.Only, checkName); err != nil { return p, err } - if err := checkOnlySelectsSomething(p.Only, warn); err != nil { + if err := checkOnlySelectsSomething(c.Rules.Only, p.Only, warn); err != nil { return p, err } if err := applySeverities(c.Rules.Severity, &p, checkName, warn); err != nil { @@ -223,18 +223,29 @@ func (c *Config) Profile() (analyzer.Profile, error) { return p, nil } -// checkOnlySelectsSomething reports an `only:` list that leaves no rule for -// the analyzer to run. Since every documented rule became addressable, a list -// naming only runtime or plan rules — `only: [slow-query]` — is accepted and -// then turns `sqlguard scan` into a command that reports nothing on any -// codebase, which looks exactly like a clean scan. Before, those names were -// rejected outright as unknown, so the shape could not arise. -func checkOnlySelectsSomething(only map[string]bool, warn func(string, ...any) error) error { - if len(only) == 0 { +// checkOnlySelectsSomething reports an `only:` list that does not narrow the +// scan to anything. Two shapes reach here, and they fail in opposite +// directions, so it takes the list as written (`configured`) as well as the +// resolved set. +// +// A list naming only runtime or plan rules — `only: [slow-query]` — is now +// accepted, because every documented rule is addressable, and leaves the +// scanner with no rule to run: `sqlguard scan` reports nothing on any +// codebase, which is indistinguishable from a clean scan. +// +// A list whose names are all unknown — `only: [selct-star]` — resolves to an +// empty whitelist, and an empty whitelist is not a whitelist at all, so +// *every* rule runs. The `unknown rule` warning alone does not say that the +// narrowing the user asked for turned into its opposite. +func checkOnlySelectsSomething(configured []string, resolved map[string]bool, warn func(string, ...any) error) error { + if len(configured) == 0 { return nil } + if len(resolved) == 0 { + return warn("rules.only named no rule that exists, so it selects nothing and every rule runs") + } for _, name := range analyzer.EvaluatedRuleNames() { - if only[name] { + if resolved[name] { return nil } } @@ -302,9 +313,12 @@ type settingKind int const ( settingDuration settingKind = iota settingInt - // settingPositiveInt additionally rejects zero and negatives, for a - // tunable where a non-positive value silently means "off". + // settingPositiveInt and settingPositiveDuration additionally reject zero + // and negatives, for a tunable where a non-positive value is never what + // anyone means: it either silently switches the feature off, or — for a + // latency threshold — matches every query and floods the reporter. settingPositiveInt + settingPositiveDuration ) // ruleSettings is the complete set of tunables, by rule and key. It is @@ -314,8 +328,8 @@ const ( // is the failure this validation exists to prevent. Adding a tunable to a rule // means adding it here. var ruleSettings = map[string]map[string]settingKind{ - "slow-query": {"threshold": settingDuration}, - "n-plus-one": {"threshold": settingPositiveInt, "window": settingDuration}, + "slow-query": {"threshold": settingPositiveDuration}, + "n-plus-one": {"threshold": settingPositiveInt, "window": settingPositiveDuration}, "leading-wildcard": {"min-length": settingInt}, "in-list-too-large": {"max-length": settingInt}, "large-offset": {"threshold": settingInt}, @@ -372,20 +386,16 @@ func checkSettings(rule string, kv map[string]any, warn func(string, ...any) err func checkSettingValue(rule, key string, kind settingKind, v any, warn func(string, ...any) error) error { switch kind { - case settingDuration: - switch tv := v.(type) { - case string: - if _, err := time.ParseDuration(strings.TrimSpace(tv)); err != nil { - return warn("rule %q: setting %q: invalid duration %q", rule, key, tv) - } - case int, int64, float64: - // A bare number is milliseconds; Settings.Duration handles it. - default: - // Anything else — a bool, a list, a map — reads back as the - // default, which for n-plus-one.window means detection silently - // never switches on. + case settingDuration, settingPositiveDuration: + d, ok := asDuration(v) + if !ok { + // A bool, a list or a map reads back as the default, which for + // n-plus-one.window means detection silently never switches on. return warn("rule %q: setting %q: expected a duration or a number, got %v", rule, key, v) } + if kind == settingPositiveDuration && d <= 0 { + return warn("rule %q: setting %q must be greater than 0, got %v", rule, key, v) + } case settingInt, settingPositiveInt: n, ok := asInt(v) if !ok { @@ -402,6 +412,24 @@ func checkSettingValue(rule, key string, kind settingKind, v any, warn func(stri return nil } +// asDuration mirrors analyzer.Settings.Duration, so the check and the read +// agree on both what parses and what it parses to. A value this rejects would +// read back as the caller's default. +func asDuration(v any) (time.Duration, bool) { + switch n := v.(type) { + case string: + d, err := time.ParseDuration(strings.TrimSpace(n)) + return d, err == nil + case int: + return time.Duration(n) * time.Millisecond, true + case int64: + return time.Duration(n) * time.Millisecond, true + case float64: + return time.Duration(n * float64(time.Millisecond)), true + } + return 0, false +} + func asInt(v any) (int, bool) { switch n := v.(type) { case int: diff --git a/config/config_test.go b/config/config_test.go index d716d77..b84b593 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -1,6 +1,7 @@ package config import ( + "fmt" "os" "path/filepath" "strings" @@ -304,8 +305,12 @@ func TestProfile_UnknownNameIsIgnoredNotHonoured(t *testing.T) { if err != nil { t.Fatalf("lenient mode should not fail: %v", err) } - if len(c.Warnings()) != 1 { - t.Errorf("expected one warning, got %v", c.Warnings()) + // Two warnings: the name itself, and what dropping it did to the list. + if len(c.Warnings()) != 2 { + t.Errorf("expected the unknown-name warning and the consequence, got %v", c.Warnings()) + } + if !strings.Contains(strings.Join(c.Warnings(), " "), "unknown rule") { + t.Errorf("the unknown name was not reported: %v", c.Warnings()) } if len(p.Only) != 0 { t.Errorf("an unknown name entered the whitelist: %v", p.Only) @@ -417,3 +422,99 @@ func TestProfile_ValidatesSettingKeys(t *testing.T) { } }) } + +// TestProfile_RejectsNonPositiveDurations covers the worst shape on the +// branch. A slow-query threshold of 0 — or 0.5, which truncated to 0 before +// Settings.Duration was fixed to scale first — matches every successful query, +// so the middleware reports `slow-query` on all of them and floods the log +// sink it exists to protect. +func TestProfile_RejectsNonPositiveDurations(t *testing.T) { + cases := []struct { + name string + rule string + key string + value any + }{ + {"zero threshold flags every query", "slow-query", "threshold", 0}, + {"zero duration string", "slow-query", "threshold", "0s"}, + {"negative", "slow-query", "threshold", "-1s"}, + {"zero window leaves N+1 off", "n-plus-one", "window", "0s"}, + {"negative window", "n-plus-one", "window", "-1m"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + settings := map[string]map[string]any{tc.rule: {tc.key: tc.value}} + if tc.rule == "n-plus-one" { + settings[tc.rule]["threshold"] = 5 + } + + c := &Config{Rules: RulesConfig{Settings: settings}} + if _, err := c.Profile(); err != nil { + t.Fatalf("lenient mode should not fail: %v", err) + } + if len(c.Warnings()) == 0 { + t.Fatal("expected a warning") + } + if !strings.Contains(c.Warnings()[0], "greater than 0") { + t.Errorf("unexpected warning: %v", c.Warnings()[0]) + } + + strict := &Config{Strict: true, Rules: RulesConfig{Settings: settings}} + if _, err := strict.Profile(); err == nil { + t.Error("expected strict mode to reject it") + } + }) + } + + // A sub-millisecond threshold is legitimate. It used to truncate to zero + // — time.Duration(0.5) is 0 — which turned a tight threshold into one + // that matched every query, the opposite of what was asked for. + for _, tc := range []struct { + value any + want time.Duration + }{ + {1.5, 1500 * time.Microsecond}, + {0.5, 500 * time.Microsecond}, + {0.0004, 400 * time.Nanosecond}, + } { + t.Run(fmt.Sprintf("fractional %v round trips", tc.value), func(t *testing.T) { + c := &Config{Strict: true, Rules: RulesConfig{Settings: map[string]map[string]any{ + "slow-query": {"threshold": tc.value}, + }}} + p, err := c.Profile() + if err != nil { + t.Fatalf("%v ms should be accepted: %v", tc.value, err) + } + if d := p.Settings["slow-query"].Duration("threshold", 0); d != tc.want { + t.Errorf("read back %v, want %v — the float conversion truncated", d, tc.want) + } + }) + } +} + +// TestProfile_OnlyResolvingToNothingWarns covers the inverse of the +// selects-nothing case: when every name is unknown the whitelist resolves to +// empty, and an empty whitelist is not a whitelist — every rule runs, which is +// the opposite of what the user asked for. +func TestProfile_OnlyResolvingToNothingWarns(t *testing.T) { + c := &Config{Rules: RulesConfig{Only: []string{"selct-star"}}} + + p, err := c.Profile() + if err != nil { + t.Fatalf("lenient mode should not fail: %v", err) + } + if len(p.Only) != 0 { + t.Fatalf("expected an empty whitelist, got %v", p.Only) + } + + var found bool + for _, w := range c.Warnings() { + if strings.Contains(w, "selects nothing and every rule runs") { + found = true + } + } + if !found { + t.Errorf("the consequence was not reported: %v", c.Warnings()) + } +} diff --git a/explain/explain.go b/explain/explain.go index 77c37e1..c74bd5a 100644 --- a/explain/explain.go +++ b/explain/explain.go @@ -249,8 +249,12 @@ func (p *PlanAnalyzer) walkPgPlan(node *pgPlanNode, query string, issues *[]anal // Detect sequential scans if node.NodeType == "Seq Scan" { + // A wide scan escalates, but only upward: Register can replace a + // built-in by name, so assigning the literal outright would let a + // seq-scan registered at CRITICAL report the >1000-row case as the + // *less* severe of the two. severity := planSeverity("seq-scan") - if node.PlanRows > 1000 { + if node.PlanRows > 1000 && severity < analyzer.SeverityWarning { severity = analyzer.SeverityWarning } *issues = append(*issues, analyzer.Result{ diff --git a/explain/rule_profile_test.go b/explain/rule_profile_test.go index 5f35763..85a0916 100644 --- a/explain/rule_profile_test.go +++ b/explain/rule_profile_test.go @@ -108,3 +108,34 @@ func TestApplyProfile_SeverityOffDisables(t *testing.T) { } } } + +// TestPlanSeverity_EscalationOnlyGoesUp guards against inverting the registry +// default. analyzer.Register can replace a built-in by name, so if seq-scan +// were registered above WARNING, assigning the literal outright would report +// the wide scan as *less* severe than the narrow one. +func TestPlanSeverity_EscalationOnlyGoesUp(t *testing.T) { + // Restore the built-in registration however this test exits. + orig, ok := analyzer.RuleDefaultSeverity("seq-scan") + if !ok { + t.Fatal("seq-scan is not registered") + } + t.Cleanup(func() { + analyzer.Register(analyzer.RuleSpec{Name: "seq-scan", DefaultSeverity: orig}) + }) + + analyzer.Register(analyzer.RuleSpec{Name: "seq-scan", DefaultSeverity: analyzer.SeverityCritical}) + + base := planSeverity("seq-scan") + if base != analyzer.SeverityCritical { + t.Fatalf("planSeverity did not pick up the re-registration: %v", base) + } + + // The escalation branch must not lower it. + wide := base + if wide < analyzer.SeverityWarning { + wide = analyzer.SeverityWarning + } + if wide < base { + t.Errorf("a wide scan reported %v, below the registered %v", wide, base) + } +} diff --git a/middleware/rule_profile_test.go b/middleware/rule_profile_test.go index 7153266..26f041e 100644 --- a/middleware/rule_profile_test.go +++ b/middleware/rule_profile_test.go @@ -210,3 +210,21 @@ func TestDisableBeatsExplicitGoOption(t *testing.T) { } }) } + +// A sub-millisecond threshold must behave as written, not as zero. +func TestSlowQuery_FractionalThreshold(t *testing.T) { + rep := &countingReporter{} + g := profileGuard(t, analyzer.Profile{ + Settings: map[string]analyzer.Settings{"slow-query": {"threshold": 0.5}}, + }, rep) + + g.CheckLatency("SELECT 1", 100*time.Microsecond) // under 500µs + if got := rep.snapshot(); len(got) != 0 { + t.Fatalf("100µs is under a 500µs threshold; a 0 threshold would flag it: %+v", got) + } + + g.CheckLatency("SELECT 1", 900*time.Microsecond) // over 500µs + if got := rep.snapshot(); len(got) != 1 { + t.Errorf("900µs should exceed a 500µs threshold, got %+v", got) + } +} diff --git a/website/docs/configuration.md b/website/docs/configuration.md index 913614b..4135c18 100644 --- a/website/docs/configuration.md +++ b/website/docs/configuration.md @@ -138,10 +138,20 @@ And on a `*Config`: | `MiddlewareOptions() ([]middleware.Option, error)` | `WithAnalyzer` from the profile — which carries the rule settings, including the slow-query and N+1 tunables — plus `WithFindingDedup` when set. Append your own options after it. | | `Analyzer() (*analyzer.Analyzer, error)` | `analyzer.DefaultWithProfile` built from this file. | | `Profile() (analyzer.Profile, error)` | The resolved, parser-independent profile. | -| `DedupWindow()` | `(time.Duration, ok bool, error)` — `ok` is false when the key is unset. _Changed in 0.3._ `SlowQueryThreshold()` is gone; read `Profile().Settings["slow-query"].Duration("threshold", d)` instead. | +| `DedupWindow()` | `(time.Duration, ok bool, error)` — `ok` is false when the key is unset. _Changed in 0.3._ `SlowQueryThreshold()` is gone; take the profile first and read the setting off it (see below). | | `ExcludeMatcher() (func(path string) bool, error)` | The compiled `scan.exclude-paths` predicate. | | `Warnings() []string` | Non-fatal problems found while loading. Surface them. | +The slow-query threshold now lives in the profile with every other tunable: + +```go +p, err := cfg.Profile() +if err != nil { + return err +} +d := p.Settings["slow-query"].Duration("threshold", 200*time.Millisecond) +``` + The common case is one line: ```go From 79d7c2ae9e4e3205a60146251255401dd3647822 Mon Sep 17 00:00:00 2001 From: kartik Date: Fri, 25 Sep 2026 10:02:19 +0530 Subject: [PATCH 06/11] feat(config): scope `only:` to the rules evaluated against a statement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `only:` now narrows the scanner and the statement rules at runtime, and deliberately reaches neither the runtime findings nor the plan rules. It is the same argument that already applied to explain, and it applies harder here: a whitelist is written to focus a scan and names statement rules, so `only: [select-star]` in a repository's config would otherwise switch off latency and N+1 reporting in the running application — without naming either, and without warning. Losing production observability to a line meant for CI is worse than losing a CLI command's output. Collapses the two accessors into one. RuleEnabled and RuleDisabledExplicitly were deliberately not complements, which I called a footgun when adding the second; with `only:` scoped the same way for all seven, one method answers `disable:` and `severity: off` for every finding built outside the statement path, and the Analyzer no longer needs the whitelist-folded map at all. Naming a rule still reaches every surface, so `disable:` and `severity: off` are the way to switch one of these off — including alongside an `only:` list. --- AGENTS.md | 2 +- CHANGELOG.md | 14 ++++--- analyzer/analyzer.go | 61 ++++++++++++++---------------- analyzer/registry_test.go | 66 ++++++++++++++++++--------------- config/middleware_test.go | 43 +++++++++++++++++++++ explain/explain.go | 8 ++-- middleware/rule_profile_test.go | 15 ++++++-- website/docs/configuration.md | 38 +++++++++++++------ website/docs/explain.md | 9 ++--- website/docs/rules.md | 5 ++- 10 files changed, 163 insertions(+), 98 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 60ee07c..4700b92 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -47,7 +47,7 @@ When changing the public API or Go version, update all nine `go.mod` files and ` **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 — except `explain`, which asks `RuleDisabledExplicitly`, deliberately **not** the complement of `RuleEnabled`: it honors `disable:` and `severity: off` but ignores an `only:` whitelist, because a list written to focus `sqlguard scan` would otherwise leave that separate command silently reporting nothing (pinned by `TestApplyProfile_IgnoresOnly` and `TestRuleDisabledExplicitlyIgnoresOnly`) — `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.checkOnlySelectsSomething` covers the shape that only became expressible once the seven were registered: `only: [slow-query]` is now a valid list that leaves the scanner nothing to run, which is indistinguishable from a clean scan. +**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.checkOnlySelectsSomething` covers the shape that only became expressible once the seven were registered: `only: [slow-query]` is now a valid list that leaves the scanner nothing to run, which is indistinguishable from a clean scan. **`config` is the only YAML-aware package.** It loads `.sqlguard.yml` (`Load`/`Discover` walks up to the git root), translates it to an `analyzer.Profile`, and exposes `MiddlewareOptions()`/`Middleware()` helpers. It depends on `analyzer` (and `middleware` for the helper); nothing depends on `config`. This keeps `gopkg.in/yaml.v3` out of the `analyzer`/`middleware` import graph for library users who don't opt into file config. Parsing is lenient by default (unknown keys/rules warn); `strict: true` makes them fatal — so a newer config still loads on an older binary. diff --git a/CHANGELOG.md b/CHANGELOG.md index 0f638f1..2c4dcc1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,11 +39,15 @@ the same version in lockstep. `WithSlowQueryThreshold` in Go still wins over the file. - **`explain` now honors `rules:` config.** It previously ignored it by design, which is what its docs said. A `severity` override also beats - `seq-scan`'s row-count-derived severity. `only:` is the exception and does - **not** reach EXPLAIN: a whitelist is written to focus a scan, and letting - it through would turn a config that never mentions EXPLAIN into one that - silently reports nothing there. Switching a plan rule off takes naming it in - `disable:` or `severity: off`. + `seq-scan`'s row-count-derived severity. +- **`only:` is scoped to the rules evaluated against a statement.** It + narrows the scanner and the statement rules at runtime, and deliberately + does not reach `slow-query`, `n-plus-one` or the five plan rules. A + whitelist is written to focus a scan and names statement rules; if it + reached the rest, `only: [select-star]` in a repository's config would also + switch off latency and N+1 reporting in the running application and make + `sqlguard explain` report nothing, without naming any of them and without + warning. `disable:` and `severity: off` reach every surface. - **N+1 detection can be enabled from the config file.** Setting both `rules.settings.n-plus-one.threshold` and `.window` turns it on; it was previously reachable only from Go via `WithN1Detection`, which still takes diff --git a/analyzer/analyzer.go b/analyzer/analyzer.go index 5d5e6c9..ca1d235 100644 --- a/analyzer/analyzer.go +++ b/analyzer/analyzer.go @@ -34,13 +34,10 @@ type Analyzer struct { rules []boundRule parser Parser severity map[string]Severity - // disabled is the resolved skip decision for every registered rule, - // including those this Analyzer does not run — see RuleEnabled. It folds - // in the `only` whitelist. + // disabled holds the rules turned off by name — `disable:`, or + // `severity: off`. It is what RuleEnabled answers from, and it does not + // fold in the `only` whitelist; see RuleEnabled for why. disabled map[string]bool - // disabledByName holds only the rules turned off by name, without the - // whitelist — see RuleDisabledExplicitly. - disabledByName map[string]bool // settings holds per-rule tunables for the same audience; the statement // rules have theirs baked in by their factory at construction. settings map[string]Settings @@ -115,14 +112,12 @@ func Default() *Analyzer { func DefaultWithProfile(p Profile) *Analyzer { all := specs() var bound []boundRule - disabled := make(map[string]bool, len(all)) - disabledByName := make(map[string]bool, len(p.Disabled)) + disabled := make(map[string]bool, len(p.Disabled)) for _, spec := range all { if p.Disabled[spec.Name] { - disabledByName[spec.Name] = true + disabled[spec.Name] = true } if p.skip(spec.Name) { - disabled[spec.Name] = true continue } // Registered for addressability only — middleware and explain build @@ -148,37 +143,35 @@ func DefaultWithProfile(p Profile) *Analyzer { maps.Copy(settings, p.Settings) } return &Analyzer{ - rules: bound, - parser: NewFallbackParser(), - severity: sev, - disabled: disabled, - disabledByName: disabledByName, - settings: settings, - rawQuery: p.RawQuery, + rules: bound, + parser: NewFallbackParser(), + severity: sev, + disabled: disabled, + settings: settings, + rawQuery: p.RawQuery, } } -// RuleEnabled reports whether the active profile leaves the named rule on, -// applying both `disable` (including `severity: off`) and the `only` -// whitelist. It answers for every registered rule, including the ones the -// Analyzer does not evaluate: middleware asks before emitting `slow-query` or -// `n-plus-one`. +// RuleEnabled reports whether the profile leaves the named rule on, for a +// finding built outside the statement path: middleware's `slow-query` and +// `n-plus-one`, and the plan rules `explain` derives. Those have no Factory, +// so the Analyzer never runs them and their owners ask here instead. +// +// It answers `disable:` and `severity: off`. An `only:` whitelist is +// deliberately **not** consulted: `only:` selects which rules the Analyzer +// evaluates over a statement, and these are not evaluated at all. A list +// written to focus `sqlguard scan` — overwhelmingly what `only:` is for — +// would otherwise switch off slow-query and N+1 in a running application and +// blank out `sqlguard explain`, none of which it mentions. Turning one of +// these off takes naming it. // -// An unregistered name is reported as enabled: an Analyzer built with New has +// For an evaluated rule the whitelist has already been applied: a rule it +// excludes was never bound, so nothing asks this about it. +// +// An unregistered name is reported as enabled. An Analyzer built with New has // no profile, and a caller's own rule is not the profile's to turn off. func (a *Analyzer) RuleEnabled(name string) bool { return !a.disabled[name] } -// RuleDisabledExplicitly reports whether the profile turned the named rule off -// by naming it — `disable:`, or `severity: off`, which resolves to the same -// thing. -// -// It is deliberately **not** the complement of RuleEnabled: it ignores the -// `only` whitelist. `only:` selects which rules the analyzer runs over a -// statement, and a list written to focus a scan should not also blank out -// `sqlguard explain`, which is a separate command reporting on a plan the -// database produced. Turning a plan rule off there takes naming it. -func (a *Analyzer) RuleDisabledExplicitly(name string) bool { return a.disabledByName[name] } - // RuleSeverity returns the severity to report for name, applying a profile // override to def when one is set. func (a *Analyzer) RuleSeverity(name string, def Severity) Severity { diff --git a/analyzer/registry_test.go b/analyzer/registry_test.go index 499127f..a76c574 100644 --- a/analyzer/registry_test.go +++ b/analyzer/registry_test.go @@ -61,50 +61,56 @@ func TestRuleEnabledAnswersForEveryRegisteredRule(t *testing.T) { } } -// TestOnlyWhitelistReachesNonEvaluatedRules covers the semantic `only` now -// carries: it is a whitelist across every surface, so a runtime or plan -// finding not named in it is off. -func TestOnlyWhitelistReachesNonEvaluatedRules(t *testing.T) { +// TestRuleEnabledIgnoresOnly pins the semantic that makes `only:` mean one +// thing everywhere: it selects which rules the Analyzer evaluates over a +// statement, and the seven findings built outside that path are not evaluated, +// so a whitelist written to focus `sqlguard scan` does not reach them. +func TestRuleEnabledIgnoresOnly(t *testing.T) { a := DefaultWithProfile(Profile{Only: map[string]bool{"select-star": true}}) - if a.RuleEnabled("slow-query") { - t.Error("an `only` whitelist should exclude slow-query") + for _, name := range []string{ + "slow-query", "n-plus-one", + "seq-scan", "high-cost", "full-table-scan", "no-index-used", "filesort", + } { + if !a.RuleEnabled(name) { + t.Errorf("an only whitelist should not reach %q", name) + } } - if !a.RuleEnabled("select-star") { - t.Error("the whitelisted rule should stay enabled") + + // The whitelist still does its job for the rules it is about. + if len(a.rules) != 1 || a.rules[0].name != "select-star" { + t.Errorf("expected only select-star bound, got %d rules", len(a.rules)) } } -// TestRuleDisabledExplicitlyIgnoresOnly pins the one place where -// RuleDisabledExplicitly is deliberately not the complement of RuleEnabled. -// A whitelist turns a rule off for the analyzer and the runtime, but it does -// not count as naming that rule, so `explain` keeps reporting it. -func TestRuleDisabledExplicitlyIgnoresOnly(t *testing.T) { - a := DefaultWithProfile(Profile{Only: map[string]bool{"select-star": true}}) +func TestRuleEnabledHonoursDisable(t *testing.T) { + a := DefaultWithProfile(Profile{Disabled: map[string]bool{"seq-scan": true}}) if a.RuleEnabled("seq-scan") { - t.Error("an only whitelist should leave seq-scan disabled for RuleEnabled") + t.Error("seq-scan was named in disable: and should be reported off") } - if a.RuleDisabledExplicitly("seq-scan") { - t.Error("a whitelist is not the same as naming seq-scan in disable:") + if !a.RuleEnabled("filesort") { + t.Error("filesort was not named and should stay on") + } + // An unregistered name is nobody's to turn off. + if !a.RuleEnabled("somebody-elses-rule") { + t.Error("an unregistered rule should be reported enabled") } } -func TestRuleDisabledExplicitlyHonoursDisable(t *testing.T) { - a := DefaultWithProfile(Profile{Disabled: map[string]bool{"seq-scan": true}}) +// TestRuleEnabledHonoursDisableInsideAnOnlyList is the escape hatch: `only:` +// does not reach these findings, but naming one in `disable:` still does. +func TestRuleEnabledHonoursDisableInsideAnOnlyList(t *testing.T) { + a := DefaultWithProfile(Profile{ + Only: map[string]bool{"select-star": true}, + Disabled: map[string]bool{"slow-query": true}, + }) - if !a.RuleDisabledExplicitly("seq-scan") { - t.Error("seq-scan was named in disable: and should be reported so") - } - if a.RuleEnabled("seq-scan") { - t.Error("RuleEnabled should agree when the rule was named") - } - if a.RuleDisabledExplicitly("filesort") { - t.Error("filesort was not named and should not be reported disabled") + if a.RuleEnabled("slow-query") { + t.Error("an explicit disable should still switch slow-query off") } - // An unregistered name is nobody's to turn off. - if a.RuleDisabledExplicitly("somebody-elses-rule") { - t.Error("an unregistered rule should never read as disabled") + if !a.RuleEnabled("n-plus-one") { + t.Error("n-plus-one was not named and should stay on") } } diff --git a/config/middleware_test.go b/config/middleware_test.go index cfe4b9f..2cef9e5 100644 --- a/config/middleware_test.go +++ b/config/middleware_test.go @@ -5,8 +5,10 @@ import ( "path/filepath" "strings" "testing" + "time" "github.com/KARTIKrocks/sqlguard" + "github.com/KARTIKrocks/sqlguard/analyzer" "github.com/KARTIKrocks/sqlguard/middleware" "github.com/KARTIKrocks/sqlguard/reporter" @@ -48,3 +50,44 @@ func TestMiddlewareOptionsAppliesProfile(t *testing.T) { t.Errorf("select-star should be disabled via config, got:\n%s", buf.String()) } } + +// TestOnlyDoesNotSilenceRuntimeFindings goes end to end from the YAML shape a +// repository actually ships: `only:` written to focus the scanner must not +// switch off the running application's latency reporting. The config package +// is where this can be asserted, since middleware cannot import it. +func TestOnlyDoesNotSilenceRuntimeFindings(t *testing.T) { + c := &Config{Rules: RulesConfig{Only: []string{"select-star"}}} + + opts, err := c.MiddlewareOptions() + if err != nil { + t.Fatalf("MiddlewareOptions: %v", err) + } + + var got []analyzer.Result + opts = append(opts, + middleware.WithReporter(reporterFunc(func(rs []analyzer.Result) { got = append(got, rs...) })), + middleware.WithSlowQueryThreshold(time.Millisecond), + ) + g := middleware.NewGuard(opts...) + + g.CheckLatency("SELECT id FROM t WHERE id = ?", time.Second) + + if len(got) != 1 || got[0].RuleName != "slow-query" { + t.Errorf("an `only:` list silenced slow-query in the running app: %+v", got) + } + + // The whitelist still narrows the statement rules it is about. + a, err := c.Analyzer() + if err != nil { + t.Fatalf("Analyzer: %v", err) + } + for _, r := range a.Analyze("DELETE FROM sessions") { + if r.RuleName != "select-star" { + t.Errorf("only: [select-star] let %q through", r.RuleName) + } + } +} + +type reporterFunc func([]analyzer.Result) + +func (f reporterFunc) Report(rs []analyzer.Result) { f(rs) } diff --git a/explain/explain.go b/explain/explain.go index c74bd5a..33a80ea 100644 --- a/explain/explain.go +++ b/explain/explain.go @@ -120,10 +120,8 @@ func planSeverity(name string) analyzer.Severity { // override. It runs once over the collected issues rather than at each site // that builds one, so a plan rule added later cannot forget the check. // -// Only a rule named in `disable:` (or given `severity: off`) is dropped. An -// `only:` whitelist is ignored here: it scopes which rules run over a -// statement, and a list written to focus `sqlguard scan` would otherwise -// leave this command silently reporting nothing. +// Only a rule named in `disable:` (or given `severity: off`) is dropped — +// see Analyzer.RuleEnabled for why an `only:` whitelist does not reach here. // // A severity override wins over a computed severity: `seq-scan` picks INFO or // WARNING from the estimated row count, and an explicit setting outranks both. @@ -133,7 +131,7 @@ func (p *PlanAnalyzer) applyProfile(issues []analyzer.Result) []analyzer.Result } kept := issues[:0] for _, r := range issues { - if p.rules.RuleDisabledExplicitly(r.RuleName) { + if !p.rules.RuleEnabled(r.RuleName) { continue } r.Severity = p.rules.RuleSeverity(r.RuleName, r.Severity) diff --git a/middleware/rule_profile_test.go b/middleware/rule_profile_test.go index 26f041e..8263429 100644 --- a/middleware/rule_profile_test.go +++ b/middleware/rule_profile_test.go @@ -198,15 +198,22 @@ func TestDisableBeatsExplicitGoOption(t *testing.T) { } }) - t.Run("an only list that omits the rule also disables it", func(t *testing.T) { + t.Run("an only list does not reach the runtime findings", func(t *testing.T) { rep := &countingReporter{} g := profileGuard(t, analyzer.Profile{Only: map[string]bool{"select-star": true}}, rep, - WithSlowQueryThreshold(time.Millisecond)) + WithSlowQueryThreshold(time.Millisecond), WithN1Detection(2, time.Minute)) g.CheckLatency("SELECT 1", time.Second) - if got := rep.snapshot(); len(got) != 0 { - t.Errorf("an `only` whitelist omitting slow-query should silence it, got %+v", got) + // `only:` selects which rules run over a statement. slow-query and + // n-plus-one are not evaluated over one, and a list written to focus + // `sqlguard scan` should not switch off a running app's latency and + // N+1 reporting without saying so. + if got := rep.snapshot(); len(got) != 1 || got[0].RuleName != "slow-query" { + t.Errorf("an `only` whitelist should not silence slow-query, got %+v", got) + } + if g.tracker == nil { + t.Error("an `only` whitelist should not stop the N+1 tracker being built") } }) } diff --git a/website/docs/configuration.md b/website/docs/configuration.md index 4135c18..043eb0e 100644 --- a/website/docs/configuration.md +++ b/website/docs/configuration.md @@ -83,7 +83,7 @@ scan: | `version` | all | Reserved for forward compatibility; always `1` today. | | `strict` | all | Make unknown keys, unknown rule names and bad severities fatal instead of warnings. | | `rules.disable` | every rule | Rule names to turn off. | -| `rules.only` | every rule | Whitelist. When non-empty, a rule must be listed to run — and `disable` still applies to the ones that are, so listing and disabling the same rule disables it. | +| `rules.only` | the scanner and the statement rules at runtime | Whitelist over the rules evaluated against a statement. `disable` still applies to the ones listed, so listing and disabling the same rule disables it. It does **not** reach `slow-query`, `n-plus-one` or the plan rules — see below. | | `rules.severity` | every rule | `info`, `warning`, `critical`, or `off`. | | `rules.settings` | rules with tunables | `leading-wildcard.min-length`, `in-list-too-large.max-length`, `large-offset.threshold`, `slow-query.threshold`, `n-plus-one.threshold` / `.window`. See [Rules](rules). | | `redact` | all | `false` keeps raw literals in `Result.Query`. See [Redaction](redaction). | @@ -178,23 +178,37 @@ opts = append(opts, middleware.WithSlowQueryThreshold(time.Second)) ``` **Turning a rule off is the other way round: the file wins.** `disable: -[slow-query]` or an `only:` list that omits it silences the finding even with -`WithSlowQueryThreshold` set, and `disable: [n-plus-one]` stops the tracker -being built at all despite `WithN1Detection`. That is deliberate — `disable` -is an instruction, not a tuning value, and an operator editing -`.sqlguard.yml` should be able to silence a noisy rule without a redeploy. +[slow-query]` silences the finding even with `WithSlowQueryThreshold` set, and +`disable: [n-plus-one]` stops the tracker being built at all despite +`WithN1Detection`. That is deliberate — `disable` is an instruction, not a +tuning value, and an operator editing `.sqlguard.yml` should be able to +silence a noisy rule without a redeploy. `only:` narrows; it does not override. A rule has to survive both checks, so `only: [select-star]` together with `disable: [select-star]` leaves nothing. ## What `only:` reaches -`only:` applies to the static scan and to the runtime findings, but **not to -[`sqlguard explain`](explain)**. A whitelist is nearly always written to focus -a scan, and it selects which rules run over a statement; if it reached the -plan rules, a config that never mentions EXPLAIN would quietly turn that -command into one that always reports nothing. To switch a plan rule off, name -it in `disable:` or give it `severity: off` — both reach every surface. +`only:` selects which rules run **against a statement** — the 14 in the +scanner and at runtime. It does not reach the seven findings that are not +derived from statement text: `slow-query` and `n-plus-one`, which the +middleware computes from latency and repetition, and the five plan rules +[`sqlguard explain`](explain) reads from the database's own plan. + +That is because a whitelist is nearly always written to focus a scan, and it +names statement rules. If it reached the rest, `only: [select-star]` in a +repository's config would also switch off latency and N+1 reporting in the +running application, and make `sqlguard explain` report nothing — none of +which it mentions, and none of which would produce a warning. + +To switch one of those off, name it: `disable:` and `severity: off` reach +every surface. + +```yaml +rules: + only: [select-star] # scanner + runtime statement rules + disable: [slow-query] # and this reaches the middleware too +``` Inline [suppressions](suppressions) win over both: they silence a finding at one site regardless of config. diff --git a/website/docs/explain.md b/website/docs/explain.md index 6869118..ec6f05f 100644 --- a/website/docs/explain.md +++ b/website/docs/explain.md @@ -85,11 +85,10 @@ In 0.2 `explain` ignored `rules:` entirely, and naming a plan rule in a config was an `unknown rule` warning — a hard error under `strict: true`. A `severity` override also wins over `seq-scan`'s row-count-derived severity. -`disable:` and `severity:` apply here; **`only:` does not**. A whitelist is -almost always written to focus `sqlguard scan`, and it selects which rules run -over a statement — letting it reach this command would mean a config that -never mentions EXPLAIN silently turns it into one that always reports nothing. -Switching a plan rule off takes naming it. +`disable:` and `severity:` apply here; **`only:` does not**. A whitelist +selects which rules run over a statement, and a plan rule is not one — see +[what `only:` reaches](configuration#what-only-reaches). Switching a plan rule +off takes naming it. Postgres plans are requested as `EXPLAIN (FORMAT JSON)` and walked recursively, so nested scans inside joins and CTEs are found. MySQL plans diff --git a/website/docs/rules.md b/website/docs/rules.md index 971b7de..369d82a 100644 --- a/website/docs/rules.md +++ b/website/docs/rules.md @@ -45,8 +45,9 @@ runtime or plan rule as for a statement rule. In 0.2 only the 14 statement rules were — naming any of the other seven warned with `unknown rule`, and failed under `strict: true`. -Two qualifications. `only:` applies to the scan and the runtime findings but -[not to `sqlguard explain`](explain#what-it-detects). And `settings` only +Two qualifications. `only:` is a whitelist over the rules evaluated against a +statement, so it reaches neither the runtime findings nor the plan rules — +see [what `only:` reaches](configuration#what-only-reaches). And `settings` only exists where a rule has a tunable: `leading-wildcard`, `in-list-too-large`, `large-offset`, `slow-query` and `n-plus-one` have them; the five plan rules have none and their thresholds are fixed, so a `settings` block for one is From feefe7e64c44bc18d2f5ac8fb157f10ff3b50b77 Mon Sep 17 00:00:00 2001 From: kartik Date: Fri, 25 Sep 2026 12:13:23 +0530 Subject: [PATCH 07/11] docs(changelog): move this branch's entries back under [Unreleased] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The rebase onto the 0.3.0 release put them under `## [0.3.0] - 2026-09-25` without reporting a conflict — the release commit inserted a new `## [Unreleased]` directly above the old heading, so git matched the surrounding context and appended into the section below it. Nothing here shipped in 0.3.0, so leaving them there would have credited a published release with a breaking config change it does not contain. --- CHANGELOG.md | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2c4dcc1..bf7c118 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,18 +9,6 @@ the same version in lockstep. ## [Unreleased] -## [0.3.0] - 2026-09-25 - -### Added - -- **Documentation site** at , built - with Docusaurus from `website/` and deployed from `main` by - `.github/workflows/docs.yml`. Docs are versioned by snapshot (`0.2` is the - first); see `website/VERSIONING.md`. `make lint-docs` lints every Markdown - file in the repo (`.markdownlint-cli2.jsonc`) and is part of `make ci` and - the CI workflow. -- Project logo, favicon and social card under `website/static/img/`. - ### Changed - **Every documented rule is now addressable in `.sqlguard.yml`.** The rules @@ -75,6 +63,20 @@ the same version in lockstep. rule became addressable would have silenced the runtime and plan findings as well as the static scan. +## [0.3.0] - 2026-09-25 + +### Added + +- **Documentation site** at , built + with Docusaurus from `website/` and deployed from `main` by + `.github/workflows/docs.yml`. Docs are versioned by snapshot (`0.2` is the + first); see `website/VERSIONING.md`. `make lint-docs` lints every Markdown + file in the repo (`.markdownlint-cli2.jsonc`) and is part of `make ci` and + the CI workflow. +- Project logo, favicon and social card under `website/static/img/`. + +### Changed + - README restructured as a landing page: logo, "Why sqlguard?" comparison, quick start, and a guide index pointing at the docs site. The deep per-feature sections moved to the site. From 99b2429ad1c615bbda0dac8687a52bc94d52fc2d Mon Sep 17 00:00:00 2001 From: kartik Date: Fri, 25 Sep 2026 12:13:34 +0530 Subject: [PATCH 08/11] fix(test): simplify severity escalation logic in TestPlanSeverity_EscalationOnlyGoesUp --- explain/rule_profile_test.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/explain/rule_profile_test.go b/explain/rule_profile_test.go index 85a0916..cdd7f12 100644 --- a/explain/rule_profile_test.go +++ b/explain/rule_profile_test.go @@ -131,10 +131,7 @@ func TestPlanSeverity_EscalationOnlyGoesUp(t *testing.T) { } // The escalation branch must not lower it. - wide := base - if wide < analyzer.SeverityWarning { - wide = analyzer.SeverityWarning - } + wide := max(base, analyzer.SeverityWarning) if wide < base { t.Errorf("a wide scan reported %v, below the registered %v", wide, base) } From fa8bd40fa9639a742f3ec573070bbb5837c4c9d3 Mon Sep 17 00:00:00 2001 From: kartik Date: Fri, 25 Sep 2026 12:41:51 +0530 Subject: [PATCH 09/11] fix(config): ask the profile whether any rule survives, not the rule list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four findings from the cloud review. checkOnlySelectsSomething tested the `only:` list against EvaluatedRuleNames, which is not the condition it meant to catch. A name the whitelist selects and `disable:` — or `severity: off` — then takes away again left the scanner with nothing to run, silently, even under `strict: true`: exactly the failure the function was added for, reached by a config that reads like an ordinary narrowing. It now asks `Profile.Skip`, the same question DefaultWithProfile asks, so the two cannot disagree. Renamed to checkScanHasRules, since it is no longer only about the list. It stays gated on `only:` being configured. Disabling every statement rule without one is a deliberate setup — sqlguard used purely for its runtime findings — and does not deserve a warning. config's asDuration/asInt duplicated Settings.Duration/Int, which is how a validator and a reader drift into disagreeing about the same value; this branch already fixed one instance of that. Settings now exposes LookupDuration/LookupInt, Duration/Int delegate to them, and the loader validates through the very accessors the rules read with. Both remaining nits: the registry-default fallback was written out twice, now analyzer.RuleDefaultSeverityOr; and NewQueryTracker's new severity parameter is a Go API break that the changelog documented only for the YAML change. --- AGENTS.md | 2 +- CHANGELOG.md | 6 +++ analyzer/analyzer.go | 2 +- analyzer/registry.go | 77 +++++++++++++++++++++++++++----------- config/config.go | 87 ++++++++++++++++++------------------------- config/config_test.go | 43 +++++++++++++++++++++ explain/explain.go | 6 +-- middleware/guard.go | 5 +-- 8 files changed, 145 insertions(+), 83 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 4700b92..21eb737 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -47,7 +47,7 @@ When changing the public API or Go version, update all nine `go.mod` files and ` **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.checkOnlySelectsSomething` covers the shape that only became expressible once the seven were registered: `only: [slow-query]` is now a valid list that leaves the scanner nothing to run, which is indistinguishable from a clean scan. +**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. **`config` is the only YAML-aware package.** It loads `.sqlguard.yml` (`Load`/`Discover` walks up to the git root), translates it to an `analyzer.Profile`, and exposes `MiddlewareOptions()`/`Middleware()` helpers. It depends on `analyzer` (and `middleware` for the helper); nothing depends on `config`. This keeps `gopkg.in/yaml.v3` out of the `analyzer`/`middleware` import graph for library users who don't opt into file config. Parsing is lenient by default (unknown keys/rules warn); `strict: true` makes them fatal — so a newer config still loads on an older binary. diff --git a/CHANGELOG.md b/CHANGELOG.md index bf7c118..76bb065 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,12 @@ the same version in lockstep. the runtime findings in middleware and the plan findings in `explain` exactly as it reaches a statement rule. They are still never evaluated against parsed SQL, so they do not fire during `sqlguard scan`. +- **Breaking (Go API): `middleware.NewQueryTracker` takes a severity.** The + signature is now `NewQueryTracker(threshold, window, severity, reportFn)`. + It is exported, so a caller constructing a tracker directly will not + compile until the argument is added; pass `analyzer.SeverityWarning` for + the previous behaviour. Guard resolves it from the profile, which is what + makes a `severity:` override on `n-plus-one` reach the finding. - **Breaking (config): the slow-query threshold moved** from the top-level `slow-query.threshold` key to `rules.settings.slow-query.threshold`, so every per-rule tunable lives in one place. `Config.SlowQueryThreshold` and diff --git a/analyzer/analyzer.go b/analyzer/analyzer.go index ca1d235..81b0df3 100644 --- a/analyzer/analyzer.go +++ b/analyzer/analyzer.go @@ -117,7 +117,7 @@ func DefaultWithProfile(p Profile) *Analyzer { if p.Disabled[spec.Name] { disabled[spec.Name] = true } - if p.skip(spec.Name) { + if p.Skip(spec.Name) { continue } // Registered for addressability only — middleware and explain build diff --git a/analyzer/registry.go b/analyzer/registry.go index f6846e7..a0f159d 100644 --- a/analyzer/registry.go +++ b/analyzer/registry.go @@ -13,22 +13,33 @@ import ( // can always be constructed even with no settings supplied. type Settings map[string]any -// Int returns the setting as an int, or def if missing or not numeric. -// YAML decodes integers as int and JSON as float64, so both are accepted. -func (s Settings) Int(key string, def int) int { +// LookupInt returns the setting as an int. ok is false when the key is absent +// or the value is one Int cannot use — the same condition under which Int +// falls back to its default. The config loader validates through this so it +// flags exactly what the reader will ignore, instead of keeping a second copy +// of these rules that can drift. +func (s Settings) LookupInt(key string) (int, bool) { if s == nil { - return def + return 0, false } switch v := s[key].(type) { case int: - return v + return v, true case int64: - return int(v) + return int(v), true case float64: - return int(v) - default: - return def + return int(v), true } + return 0, false +} + +// Int returns the setting as an int, or def if missing or not numeric. +// YAML decodes integers as int and JSON as float64, so both are accepted. +func (s Settings) Int(key string, def int) int { + if v, ok := s.LookupInt(key); ok { + return v + } + return def } // Bool returns the setting as a bool, or def if missing or not a bool. @@ -53,28 +64,37 @@ func (s Settings) String(key, def string) string { return def } -// Duration returns the setting parsed as a time.Duration. It accepts a -// duration string ("200ms") or a number interpreted as milliseconds. Returns -// def if missing or unparseable. -func (s Settings) Duration(key string, def time.Duration) time.Duration { +// LookupDuration returns the setting parsed as a time.Duration. It accepts a +// duration string ("200ms") or a number interpreted as milliseconds. ok is +// false when the key is absent or the value is one Duration cannot use — see +// LookupInt for why the config loader validates through this. +func (s Settings) LookupDuration(key string) (time.Duration, bool) { if s == nil { - return def + return 0, false } switch v := s[key].(type) { case string: // Trimmed so this agrees with the config loader's validation; a // value it accepts must not fall back to def here. - if d, err := time.ParseDuration(strings.TrimSpace(v)); err == nil { - return d - } + d, err := time.ParseDuration(strings.TrimSpace(v)) + return d, err == nil case int: - return time.Duration(v) * time.Millisecond + return time.Duration(v) * time.Millisecond, true case int64: - return time.Duration(v) * time.Millisecond + return time.Duration(v) * time.Millisecond, true case float64: // Scale before converting: time.Duration(0.5) truncates to 0, which // turned a fractional-millisecond threshold into "no threshold". - return time.Duration(v * float64(time.Millisecond)) + return time.Duration(v * float64(time.Millisecond)), true + } + return 0, false +} + +// Duration returns the setting parsed as a time.Duration, or def if missing +// or unparseable. +func (s Settings) Duration(key string, def time.Duration) time.Duration { + if d, ok := s.LookupDuration(key); ok { + return d } return def } @@ -156,6 +176,17 @@ func RuleDefaultSeverity(name string) (sev Severity, ok bool) { return spec.DefaultSeverity, true } +// RuleDefaultSeverityOr returns the severity a rule was registered with, or def +// when the name is not registered. It is what the findings built outside the +// statement path use: they cannot reach their RuleSpec any other way, and +// without one helper each of them repeats the same lookup-and-fall-back. +func RuleDefaultSeverityOr(name string, def Severity) Severity { + if sev, ok := RuleDefaultSeverity(name); ok { + return sev + } + return def +} + // specs returns all registered specs sorted by name, for deterministic // analyzer construction and stable report ordering. func specs() []RuleSpec { @@ -188,7 +219,11 @@ type Profile struct { RawQuery bool } -func (p Profile) skip(name string) bool { +// Skip reports whether this profile excludes the named rule from evaluation, +// folding in both the `only` whitelist and the disabled set. It is exported so +// the config loader can ask the same question the Analyzer answers, rather +// than reimplementing the precedence and drifting from it. +func (p Profile) Skip(name string) bool { if len(p.Only) > 0 && !p.Only[name] { return true } diff --git a/config/config.go b/config/config.go index 965cef6..f81d17c 100644 --- a/config/config.go +++ b/config/config.go @@ -201,12 +201,14 @@ func (c *Config) Profile() (analyzer.Profile, error) { if err := collectNames(c.Rules.Only, p.Only, checkName); err != nil { return p, err } - if err := checkOnlySelectsSomething(c.Rules.Only, p.Only, warn); err != nil { + + if err := applySeverities(c.Rules.Severity, &p, checkName, warn); err != nil { return p, err } - if err := applySeverities(c.Rules.Severity, &p, checkName, warn); err != nil { + if err := checkScanHasRules(c.Rules.Only, p, warn); err != nil { return p, err } + for name, kv := range c.Rules.Settings { ok, err := checkName(name) if err != nil { @@ -223,33 +225,40 @@ func (c *Config) Profile() (analyzer.Profile, error) { return p, nil } -// checkOnlySelectsSomething reports an `only:` list that does not narrow the -// scan to anything. Two shapes reach here, and they fail in opposite -// directions, so it takes the list as written (`configured`) as well as the -// resolved set. +// checkScanHasRules reports an `only:` list that leaves the scanner with +// nothing to run. It asks the profile the same question the Analyzer does — +// does any evaluated rule survive Skip — rather than testing the list against +// EvaluatedRuleNames, which was blind to a name that `only:` selects and +// `disable:` (or `severity: off`) then takes away again. +// +// Three shapes reach here: +// +// - `only: [slow-query]` — valid names now that every documented rule is +// addressable, but none of them runs over a statement. +// - `only: [select-star]` with `disable: [select-star]` — the whitelist +// excludes everything else and the disabled set removes the remainder. +// - `only: [selct-star]` — every name unknown, so the whitelist resolves to +// empty; an empty whitelist is not a whitelist, and *every* rule runs, +// which is the opposite of the narrowing that was asked for. // -// A list naming only runtime or plan rules — `only: [slow-query]` — is now -// accepted, because every documented rule is addressable, and leaves the -// scanner with no rule to run: `sqlguard scan` reports nothing on any -// codebase, which is indistinguishable from a clean scan. +// All three report nothing on any codebase, which reads as a clean scan. // -// A list whose names are all unknown — `only: [selct-star]` — resolves to an -// empty whitelist, and an empty whitelist is not a whitelist at all, so -// *every* rule runs. The `unknown rule` warning alone does not say that the -// narrowing the user asked for turned into its opposite. -func checkOnlySelectsSomething(configured []string, resolved map[string]bool, warn func(string, ...any) error) error { - if len(configured) == 0 { +// The check is gated on `only:` being configured. Disabling every rule without +// one is a deliberate act — using sqlguard purely for its runtime findings is +// a legitimate setup — and does not deserve a warning. +func checkScanHasRules(configuredOnly []string, p analyzer.Profile, warn func(string, ...any) error) error { + if len(configuredOnly) == 0 { return nil } - if len(resolved) == 0 { + if len(p.Only) == 0 { return warn("rules.only named no rule that exists, so it selects nothing and every rule runs") } for _, name := range analyzer.EvaluatedRuleNames() { - if resolved[name] { + if !p.Skip(name) { return nil } } - return warn("rules.only names no rule that runs over a statement, so nothing will be scanned " + + return warn("rules.only leaves no rule that runs over a statement, so nothing will be scanned " + "(runtime and EXPLAIN rules are reported by the middleware and `sqlguard explain`, not the scanner)") } @@ -385,9 +394,15 @@ func checkSettings(rule string, kv map[string]any, warn func(string, ...any) err } func checkSettingValue(rule, key string, kind settingKind, v any, warn func(string, ...any) error) error { + // Validate through the same accessors the rules read with, so a value + // accepted here can never be one the reader quietly replaces with its + // default. A second copy of these parsing rules is exactly how the two + // drift apart. + one := analyzer.Settings{key: v} + switch kind { case settingDuration, settingPositiveDuration: - d, ok := asDuration(v) + d, ok := one.LookupDuration(key) if !ok { // A bool, a list or a map reads back as the default, which for // n-plus-one.window means detection silently never switches on. @@ -397,7 +412,7 @@ func checkSettingValue(rule, key string, kind settingKind, v any, warn func(stri return warn("rule %q: setting %q must be greater than 0, got %v", rule, key, v) } case settingInt, settingPositiveInt: - n, ok := asInt(v) + n, ok := one.LookupInt(key) if !ok { // A quoted number is the common YAML slip. Settings.Int does not // accept a string, so it would read back as the default: for @@ -412,36 +427,6 @@ func checkSettingValue(rule, key string, kind settingKind, v any, warn func(stri return nil } -// asDuration mirrors analyzer.Settings.Duration, so the check and the read -// agree on both what parses and what it parses to. A value this rejects would -// read back as the caller's default. -func asDuration(v any) (time.Duration, bool) { - switch n := v.(type) { - case string: - d, err := time.ParseDuration(strings.TrimSpace(n)) - return d, err == nil - case int: - return time.Duration(n) * time.Millisecond, true - case int64: - return time.Duration(n) * time.Millisecond, true - case float64: - return time.Duration(n * float64(time.Millisecond)), true - } - return 0, false -} - -func asInt(v any) (int, bool) { - switch n := v.(type) { - case int: - return n, true - case int64: - return int(n), true - case float64: - return int(n), true - } - return 0, false -} - func sortedKeys(m map[string]settingKind) []string { out := make([]string, 0, len(m)) for k := range m { diff --git a/config/config_test.go b/config/config_test.go index b84b593..efb9679 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -352,6 +352,49 @@ func TestProfile_OnlySelectingNothingRunnableWarns(t *testing.T) { }) } + // A name the whitelist selects and `disable` (or `severity: off`) then + // takes away again leaves the scanner with nothing, but reads like a + // perfectly ordinary narrowing config. Testing the list against the + // registry could not see it; asking the profile whether any rule survives + // can. + t.Run("only and disable cancelling out", func(t *testing.T) { + c := &Config{Rules: RulesConfig{ + Only: []string{"select-star"}, + Disable: []string{"select-star"}, + }} + if _, err := c.Profile(); err != nil { + t.Fatalf("lenient mode should not fail: %v", err) + } + if len(c.Warnings()) != 1 || !strings.Contains(c.Warnings()[0], "nothing will be scanned") { + t.Errorf("expected the nothing-scanned warning, got %v", c.Warnings()) + } + }) + + t.Run("only and severity off cancelling out", func(t *testing.T) { + c := &Config{Rules: RulesConfig{ + Only: []string{"select-star"}, + Severity: map[string]string{"select-star": "off"}, + }} + if _, err := c.Profile(); err != nil { + t.Fatalf("lenient mode should not fail: %v", err) + } + if len(c.Warnings()) != 1 || !strings.Contains(c.Warnings()[0], "nothing will be scanned") { + t.Errorf("expected the nothing-scanned warning, got %v", c.Warnings()) + } + }) + + // Disabling everything without an `only:` list is a deliberate setup — + // using sqlguard purely for its runtime findings — and must stay quiet. + t.Run("disabling every rule without only stays quiet", func(t *testing.T) { + c := &Config{Strict: true, Rules: RulesConfig{Disable: analyzer.EvaluatedRuleNames()}} + if _, err := c.Profile(); err != nil { + t.Fatalf("this is a legitimate config: %v", err) + } + if len(c.Warnings()) != 0 { + t.Errorf("unexpected warning: %v", c.Warnings()) + } + }) + t.Run("a list with one runnable rule is fine", func(t *testing.T) { c := &Config{Strict: true, Rules: RulesConfig{Only: []string{"slow-query", "select-star"}}} if _, err := c.Profile(); err != nil { diff --git a/explain/explain.go b/explain/explain.go index 33a80ea..d1d28cc 100644 --- a/explain/explain.go +++ b/explain/explain.go @@ -109,11 +109,7 @@ func (p *PlanAnalyzer) Analyze(ctx context.Context, query string) (*Result, erro // Factory, so nothing else would ever consult their DefaultSeverity, and a // literal here would silently outrank it. func planSeverity(name string) analyzer.Severity { - sev, ok := analyzer.RuleDefaultSeverity(name) - if !ok { - return analyzer.SeverityWarning - } - return sev + return analyzer.RuleDefaultSeverityOr(name, analyzer.SeverityWarning) } // applyProfile drops findings the profile disabled and applies any severity diff --git a/middleware/guard.go b/middleware/guard.go index 52ff7f6..9bbf5a8 100644 --- a/middleware/guard.go +++ b/middleware/guard.go @@ -39,10 +39,7 @@ type findingPolicy struct { // repeating a literal here, so `Register(RuleSpec{Name: "slow-query", …})` is // what decides it — the same as for an evaluated rule. func resolvePolicy(a *analyzer.Analyzer, name string) findingPolicy { - def, ok := analyzer.RuleDefaultSeverity(name) - if !ok { - def = analyzer.SeverityWarning - } + def := analyzer.RuleDefaultSeverityOr(name, analyzer.SeverityWarning) return findingPolicy{ enabled: a.RuleEnabled(name), severity: a.RuleSeverity(name, def), From 13834a35e890e1d28a110ab3ca31ddf07dcb8907 Mon Sep 17 00:00:00 2001 From: kartik Date: Fri, 25 Sep 2026 12:50:45 +0530 Subject: [PATCH 10/11] style(explain): use max for the seq-scan escalation Follows through on 99b2429, which did the same in the test. The production branch still carried the long form; the behaviour is identical and the builtin says what it means. --- explain/explain.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/explain/explain.go b/explain/explain.go index d1d28cc..2f571c6 100644 --- a/explain/explain.go +++ b/explain/explain.go @@ -248,8 +248,8 @@ func (p *PlanAnalyzer) walkPgPlan(node *pgPlanNode, query string, issues *[]anal // seq-scan registered at CRITICAL report the >1000-row case as the // *less* severe of the two. severity := planSeverity("seq-scan") - if node.PlanRows > 1000 && severity < analyzer.SeverityWarning { - severity = analyzer.SeverityWarning + if node.PlanRows > 1000 { + severity = max(severity, analyzer.SeverityWarning) } *issues = append(*issues, analyzer.Result{ RuleName: "seq-scan", From 6cacb6c5e6617ad364556434001bd03d649a6ee0 Mon Sep 17 00:00:00 2001 From: kartik Date: Fri, 25 Sep 2026 13:13:57 +0530 Subject: [PATCH 11/11] fix(config): drop rejected settings instead of only warning about them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A warning is half an answer in lenient mode, which is the default: the load continues, and the rejected value still reached the reader. `slow-query.threshold: 0` warned and then matched every successful query anyway, flooding the reporter it exists to protect — the earlier fix only held under `strict: true`, where the load stops. checkSettings now returns the settings that survived, and Profile carries that subset. The paired-key check judges what survived too, so a pair whose other half was rejected is reported as inert, which it is. TestPlanSeverity_EscalationOnlyGoesUp asserted on a locally computed max and so could not fail — it exercised the builtin, not walkPgPlan, and would have kept passing if the escalation went back to assigning SeverityWarning outright. It now drives walkPgPlan with a Seq Scan node and asserts on the finding; reverting the fix fails it. Docs: `.sqlguard.example.yml` and the reference YAML in configuration.md both still said `only:` means "disable is ignored", which is false — Profile.Skip checks the disabled set after the whitelist — and neither said the whitelist does not reach the runtime or plan rules. explain.WithAnalyzer is new exported API with no example, so Library use now shows both building a profile in code and taking one from a loaded config; both snippets were compiled. --- .sqlguard.example.yml | 7 +++- CHANGELOG.md | 6 +++- config/config.go | 56 +++++++++++++++++++---------- config/config_test.go | 4 ++- config/middleware_test.go | 43 +++++++++++++++++++++++ explain/rule_profile_test.go | 66 +++++++++++++++++++++++++++-------- middleware/guard.go | 8 +++-- website/docs/configuration.md | 7 +++- website/docs/explain.md | 38 ++++++++++++++++++++ 9 files changed, 197 insertions(+), 38 deletions(-) diff --git a/.sqlguard.example.yml b/.sqlguard.example.yml index 5ed7bcb..139e2b8 100644 --- a/.sqlguard.example.yml +++ b/.sqlguard.example.yml @@ -16,7 +16,12 @@ rules: disable: - orderby-without-limit - # Whitelist mode: when non-empty, ONLY these rules run (disable is ignored). + # Whitelist mode: when non-empty, a rule must be listed to run. It narrows + # the rules evaluated against a statement — the scanner and the runtime + # statement rules — and does NOT reach slow-query, n-plus-one or the EXPLAIN + # plan rules; switch one of those off by naming it in `disable` above. + # `disable` still applies to the rules listed here, so listing and disabling + # the same rule disables it. # only: # - delete-without-where # - update-without-where diff --git a/CHANGELOG.md b/CHANGELOG.md index 76bb065..0c105f2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,7 +47,11 @@ the same version in lockstep. previously reachable only from Go via `WithN1Detection`, which still takes precedence. - A value in `rules.settings` that will not read back as its type is now - reported instead of being silently replaced by the built-in default. This + reported **and dropped**, so the rule falls back to its built-in default + rather than acting on the rejected value. In lenient mode the load continues + after a warning, so reporting alone was half an answer: + `slow-query.threshold: 0` warned and then matched every successful query + anyway, flooding the reporter it exists to protect. This covers durations and numbers, and a half-specified `n-plus-one` block — a quoted `threshold: "10"` is a string in YAML, read back as 0, which would have left N+1 detection off with no indication. diff --git a/config/config.go b/config/config.go index f81d17c..ddae687 100644 --- a/config/config.go +++ b/config/config.go @@ -217,10 +217,13 @@ func (c *Config) Profile() (analyzer.Profile, error) { if !ok { continue } - if err := checkSettings(name, kv, warn); err != nil { + kept, err := checkSettings(name, kv, warn) + if err != nil { return p, err } - p.Settings[name] = analyzer.Settings(kv) + if len(kept) > 0 { + p.Settings[name] = kept + } } return p, nil } @@ -351,49 +354,66 @@ var pairedSettings = map[string][]string{ } // checkSettings reports a setting key the rule does not have, a value that -// will not read back as its kind, and a half-specified pair. -func checkSettings(rule string, kv map[string]any, warn func(string, ...any) error) error { +// will not read back as its kind, and a half-specified pair. It returns the +// settings that survived. +// +// Returning a subset is the point: in lenient mode a warning does not stop the +// load, and carrying a rejected value through to the profile would mean the +// reader still acts on it. `slow-query.threshold: 0` warned and then matched +// every query anyway, flooding the reporter — the warning named the problem +// while the problem still happened. A value this reports is a value the rules +// must not see. +func checkSettings(rule string, kv map[string]any, warn func(string, ...any) error) (analyzer.Settings, error) { kinds, tunable := ruleSettings[rule] + kept := make(analyzer.Settings, len(kv)) for key, v := range kv { kind, known := kinds[key] if !known { if !tunable { if err := warn("rule %q has no settings, so %q is ignored", rule, key); err != nil { - return err + return nil, err } continue } if err := warn("rule %q: unknown setting %q (known: %s)", rule, key, strings.Join(sortedKeys(kinds), ", ")); err != nil { - return err + return nil, err } continue } - if err := checkSettingValue(rule, key, kind, v, warn); err != nil { - return err + bad, err := checkSettingValue(rule, key, kind, v, warn) + if err != nil { + return nil, err + } + if !bad { + kept[key] = v } } pair := pairedSettings[rule] if len(pair) == 0 { - return nil + return kept, nil } + // Judged on what survived: a pair whose other half was rejected is just as + // inert as one whose other half was never written. var have, missing []string for _, key := range pair { - if _, present := kv[key]; present { + if _, present := kept[key]; present { have = append(have, key) } else { missing = append(missing, key) } } if len(have) > 0 && len(missing) > 0 { - return warn("rule %q: setting %q has no effect without %q", + return kept, warn("rule %q: setting %q has no effect without %q", rule, strings.Join(have, ", "), strings.Join(missing, ", ")) } - return nil + return kept, nil } -func checkSettingValue(rule, key string, kind settingKind, v any, warn func(string, ...any) error) error { +// checkSettingValue reports a value the reader cannot use. bad is true when +// the value was rejected, so the caller can keep it out of the profile. +func checkSettingValue(rule, key string, kind settingKind, v any, warn func(string, ...any) error) (bad bool, err error) { // Validate through the same accessors the rules read with, so a value // accepted here can never be one the reader quietly replaces with its // default. A second copy of these parsing rules is exactly how the two @@ -406,10 +426,10 @@ func checkSettingValue(rule, key string, kind settingKind, v any, warn func(stri if !ok { // A bool, a list or a map reads back as the default, which for // n-plus-one.window means detection silently never switches on. - return warn("rule %q: setting %q: expected a duration or a number, got %v", rule, key, v) + return true, warn("rule %q: setting %q: expected a duration or a number, got %v", rule, key, v) } if kind == settingPositiveDuration && d <= 0 { - return warn("rule %q: setting %q must be greater than 0, got %v", rule, key, v) + return true, warn("rule %q: setting %q must be greater than 0, got %v", rule, key, v) } case settingInt, settingPositiveInt: n, ok := one.LookupInt(key) @@ -417,14 +437,14 @@ func checkSettingValue(rule, key string, kind settingKind, v any, warn func(stri // A quoted number is the common YAML slip. Settings.Int does not // accept a string, so it would read back as the default: for // n-plus-one.threshold that means detection never switches on. - return warn("rule %q: setting %q: expected a number, got %v (quoted numbers are strings in YAML)", + return true, warn("rule %q: setting %q: expected a number, got %v (quoted numbers are strings in YAML)", rule, key, v) } if kind == settingPositiveInt && n <= 0 { - return warn("rule %q: setting %q must be greater than 0, got %d", rule, key, n) + return true, warn("rule %q: setting %q must be greater than 0, got %d", rule, key, n) } } - return nil + return false, nil } func sortedKeys(m map[string]settingKind) []string { diff --git a/config/config_test.go b/config/config_test.go index efb9679..d74ad6c 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -242,8 +242,10 @@ func TestProfile_ValidatesSettings(t *testing.T) { }{ {"unparseable duration", map[string]map[string]any{ "slow-query": {"threshold": "200mss"}}, 1}, + // Two warnings, both true: the quoted threshold is rejected, and the + // window that survives alone no longer switches anything on. {"quoted number reads back as the default", map[string]map[string]any{ - "n-plus-one": {"threshold": "10", "window": "1m"}}, 1}, + "n-plus-one": {"threshold": "10", "window": "1m"}}, 2}, {"half a paired block is inert", map[string]map[string]any{ "n-plus-one": {"window": "1m"}}, 1}, {"quoted int on a statement rule", map[string]map[string]any{ diff --git a/config/middleware_test.go b/config/middleware_test.go index 2cef9e5..e48ec27 100644 --- a/config/middleware_test.go +++ b/config/middleware_test.go @@ -91,3 +91,46 @@ func TestOnlyDoesNotSilenceRuntimeFindings(t *testing.T) { type reporterFunc func([]analyzer.Result) func (f reporterFunc) Report(rs []analyzer.Result) { f(rs) } + +// TestRejectedSettingDoesNotReachTheReader is the half the earlier fix missed. +// Rejecting a value under `strict: true` is the easy case — the load stops. In +// lenient mode, which is the default, the load continues, so a warning is only +// half an answer: `slow-query.threshold: 0` warned and then matched every +// successful query anyway, flooding the reporter it was meant to protect. +func TestRejectedSettingDoesNotReachTheReader(t *testing.T) { + c := &Config{Rules: RulesConfig{Settings: map[string]map[string]any{ + "slow-query": {"threshold": 0}, + }}} + + p, err := c.Profile() + if err != nil { + t.Fatalf("lenient mode should not fail: %v", err) + } + if len(c.Warnings()) == 0 { + t.Error("expected a warning for the zero threshold") + } + if _, present := p.Settings["slow-query"]["threshold"]; present { + t.Errorf("the rejected value reached the profile: %v", p.Settings["slow-query"]) + } + + opts, err := c.MiddlewareOptions() + if err != nil { + t.Fatalf("MiddlewareOptions: %v", err) + } + var got []analyzer.Result + opts = append(opts, middleware.WithReporter( + reporterFunc(func(rs []analyzer.Result) { got = append(got, rs...) }))) + g := middleware.NewGuard(opts...) + + g.CheckLatency("SELECT id FROM t WHERE id = ?", time.Microsecond) + + if len(got) != 0 { + t.Errorf("a 1µs query was reported slow, so the threshold fell to 0: %+v", got) + } + + // The built-in default must be what stands in its place. + g.CheckLatency("SELECT id FROM t WHERE id = ?", 300*time.Millisecond) + if len(got) != 1 { + t.Errorf("300ms should exceed the built-in 200ms default, got %+v", got) + } +} diff --git a/explain/rule_profile_test.go b/explain/rule_profile_test.go index cdd7f12..00de18c 100644 --- a/explain/rule_profile_test.go +++ b/explain/rule_profile_test.go @@ -109,12 +109,14 @@ func TestApplyProfile_SeverityOffDisables(t *testing.T) { } } -// TestPlanSeverity_EscalationOnlyGoesUp guards against inverting the registry -// default. analyzer.Register can replace a built-in by name, so if seq-scan -// were registered above WARNING, assigning the literal outright would report -// the wide scan as *less* severe than the narrow one. -func TestPlanSeverity_EscalationOnlyGoesUp(t *testing.T) { - // Restore the built-in registration however this test exits. +// TestSeqScanSeverity_EscalationOnlyGoesUp drives walkPgPlan, the code that +// actually builds the finding. Asserting on a locally computed max would only +// exercise the builtin: it could not fail, and would keep passing if +// walkPgPlan went back to assigning SeverityWarning outright. +// +// analyzer.Register can replace a built-in by name, so a seq-scan registered +// above WARNING must not be *lowered* by the wide-scan branch. +func TestSeqScanSeverity_EscalationOnlyGoesUp(t *testing.T) { orig, ok := analyzer.RuleDefaultSeverity("seq-scan") if !ok { t.Fatal("seq-scan is not registered") @@ -122,17 +124,53 @@ func TestPlanSeverity_EscalationOnlyGoesUp(t *testing.T) { t.Cleanup(func() { analyzer.Register(analyzer.RuleSpec{Name: "seq-scan", DefaultSeverity: orig}) }) - analyzer.Register(analyzer.RuleSpec{Name: "seq-scan", DefaultSeverity: analyzer.SeverityCritical}) - base := planSeverity("seq-scan") - if base != analyzer.SeverityCritical { - t.Fatalf("planSeverity did not pick up the re-registration: %v", base) + pa := &PlanAnalyzer{} + + for _, tc := range []struct { + name string + rows int64 + }{ + {"narrow scan reports the registered severity", 10}, + {"wide scan must not drop below it", 500_000}, + } { + t.Run(tc.name, func(t *testing.T) { + var issues []analyzer.Result + pa.walkPgPlan(&pgPlanNode{NodeType: "Seq Scan", PlanRows: tc.rows}, "SELECT 1", &issues) + + var seq *analyzer.Result + for i := range issues { + if issues[i].RuleName == "seq-scan" { + seq = &issues[i] + } + } + if seq == nil { + t.Fatalf("no seq-scan finding for a Seq Scan node: %+v", issues) + } + if seq.Severity < analyzer.SeverityCritical { + t.Errorf("reported %v, below the registered CRITICAL", seq.Severity) + } + }) } +} + +// TestSeqScanSeverity_EscalatesFromTheRegisteredDefault is the other +// direction: at the built-in INFO, a wide scan must still be raised. +func TestSeqScanSeverity_EscalatesFromTheRegisteredDefault(t *testing.T) { + pa := &PlanAnalyzer{} - // The escalation branch must not lower it. - wide := max(base, analyzer.SeverityWarning) - if wide < base { - t.Errorf("a wide scan reported %v, below the registered %v", wide, base) + var narrow, wide []analyzer.Result + pa.walkPgPlan(&pgPlanNode{NodeType: "Seq Scan", PlanRows: 10}, "SELECT 1", &narrow) + pa.walkPgPlan(&pgPlanNode{NodeType: "Seq Scan", PlanRows: 500_000}, "SELECT 1", &wide) + + if len(narrow) == 0 || len(wide) == 0 { + t.Fatalf("expected a finding from each: narrow=%+v wide=%+v", narrow, wide) + } + if narrow[0].Severity != analyzer.SeverityInfo { + t.Errorf("narrow scan = %v, want the registered INFO", narrow[0].Severity) + } + if wide[0].Severity != analyzer.SeverityWarning { + t.Errorf("wide scan = %v, want WARNING", wide[0].Severity) } } diff --git a/middleware/guard.go b/middleware/guard.go index 9bbf5a8..ad67ab0 100644 --- a/middleware/guard.go +++ b/middleware/guard.go @@ -28,8 +28,12 @@ type Guard struct { } // findingPolicy is a registered rule's resolved state for a finding the -// analyzer does not evaluate itself. `enabled` folds in `disable` and `only`; -// `severity` folds in a profile override. +// analyzer does not evaluate itself. +// +// `enabled` answers `disable:` and `severity: off` only — `only:` selects the +// rules evaluated against a statement, and these are not among them (see +// analyzer.RuleEnabled). `severity` folds in a profile override. Settings +// reach these rules too, through Profile.Settings. type findingPolicy struct { enabled bool severity analyzer.Severity diff --git a/website/docs/configuration.md b/website/docs/configuration.md index 043eb0e..4f871f2 100644 --- a/website/docs/configuration.md +++ b/website/docs/configuration.md @@ -37,7 +37,12 @@ rules: disable: - orderby-without-limit - # Whitelist mode: when non-empty, ONLY these rules run (disable is ignored). + # Whitelist mode: when non-empty, a rule must be listed to run. It narrows + # the rules evaluated against a statement — the scanner and the runtime + # statement rules — and does NOT reach slow-query, n-plus-one or the EXPLAIN + # plan rules; switch one of those off by naming it in `disable` above. + # `disable` still applies to the rules listed here, so listing and disabling + # the same rule disables it. # only: # - delete-without-where # - update-without-where diff --git a/website/docs/explain.md b/website/docs/explain.md index ec6f05f..1c2775b 100644 --- a/website/docs/explain.md +++ b/website/docs/explain.md @@ -166,6 +166,44 @@ for _, issue := range res.Issues { // []analyzer.Result fmt.Println(res.RawPlan) // the plan text, for humans ``` +_Added in 0.3._ `explain.WithAnalyzer` applies a rule profile, which is how the +CLI passes your `.sqlguard.yml` through. From code you can build one without a +file: + +```go +import ( + "github.com/KARTIKrocks/sqlguard/analyzer" + "github.com/KARTIKrocks/sqlguard/explain" +) + +a := analyzer.DefaultWithProfile(analyzer.Profile{ + Disabled: map[string]bool{"high-cost": true}, + Severity: map[string]analyzer.Severity{"seq-scan": analyzer.SeverityCritical}, +}) + +pa, err := explain.New(db, "postgres", explain.WithAnalyzer(a)) +if err != nil { + return err +} +``` + +Or from a loaded config, so the same file governs the scanner, the middleware +and this: + +```go +cfg, err := config.Load(".sqlguard.yml") +if err != nil { + return err +} +a, err := cfg.Analyzer() +if err != nil { + return err +} +pa, err := explain.New(db, "postgres", explain.WithAnalyzer(a)) +``` + +Without it, every plan rule fires at its built-in severity. + `explain.Result` carries `Query`, `RawPlan` and `Issues`. Pair it with a test that runs your hottest queries through `Analyze` against a seeded database — a `seq-scan` on the orders table is cheaper to find in CI than