diff --git a/.sqlguard.example.yml b/.sqlguard.example.yml index 556d714..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 @@ -42,6 +47,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 +68,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..21eb737 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. **`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. **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..0c105f2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,70 @@ the same version in lockstep. ## [Unreleased] +### 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 (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 + 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. +- **`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 + precedence. +- A value in `rules.settings` that will not read back as its type is now + 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. +- 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 + (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 + rule became addressable would have silenced the runtime and plan findings + as well as the static scan. + ## [0.3.0] - 2026-09-25 ### Added diff --git a/analyzer/analyzer.go b/analyzer/analyzer.go index 64356c8..81b0df3 100644 --- a/analyzer/analyzer.go +++ b/analyzer/analyzer.go @@ -34,6 +34,13 @@ type Analyzer struct { rules []boundRule parser Parser severity map[string]Severity + // 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 + // 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 @@ -103,9 +110,19 @@ 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 - for _, spec := range specs() { - if p.skip(spec.Name) { + disabled := make(map[string]bool, len(p.Disabled)) + for _, spec := range all { + if p.Disabled[spec.Name] { + disabled[spec.Name] = true + } + if p.Skip(spec.Name) { + 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 +137,57 @@ 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 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. +// +// 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] } + +// 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..a0f159d 100644 --- a/analyzer/registry.go +++ b/analyzer/registry.go @@ -2,6 +2,7 @@ package analyzer import ( "sort" + "strings" "sync" "time" ) @@ -12,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. @@ -52,24 +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: - if d, err := time.ParseDuration(v); err == nil { - return d - } + // Trimmed so this agrees with the config loader's validation; a + // value it accepts must not fall back to def here. + 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: - 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)), 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 } @@ -81,9 +106,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{} @@ -111,6 +145,48 @@ 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 +} + +// 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 { @@ -143,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/analyzer/registry_test.go b/analyzer/registry_test.go new file mode 100644 index 0000000..a76c574 --- /dev/null +++ b/analyzer/registry_test.go @@ -0,0 +1,166 @@ +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") + } +} + +// 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}}) + + 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) + } + } + + // 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)) + } +} + +func TestRuleEnabledHonoursDisable(t *testing.T) { + a := DefaultWithProfile(Profile{Disabled: map[string]bool{"seq-scan": true}}) + + if a.RuleEnabled("seq-scan") { + t.Error("seq-scan was named in disable: and should be reported off") + } + 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") + } +} + +// 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.RuleEnabled("slow-query") { + t.Error("an explicit disable should still switch slow-query off") + } + if !a.RuleEnabled("n-plus-one") { + t.Error("n-plus-one was not named and should stay on") + } +} + +// 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/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..b6d74a3 100644 --- a/cmd/sqlguard/explain.go +++ b/cmd/sqlguard/explain.go @@ -40,12 +40,24 @@ 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 } + cfg, err := resolveConfig(".") + if err != nil { + return err + } + a, err := cfg.Analyzer() + if err != nil { + return err + } + printConfigWarnings(cfg) ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() @@ -56,7 +68,7 @@ func runExplain(cmd *cobra.Command, args []string) error { } defer func() { _ = db.Close() }() - var explainOpts []explain.Option + 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 736cbb3..ddae687 100644 --- a/config/config.go +++ b/config/config.go @@ -15,6 +15,7 @@ import ( "os" "path/filepath" "regexp" + "sort" "strings" "time" @@ -29,12 +30,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 +57,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 + @@ -187,33 +181,128 @@ 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 + } + + if err := collectNames(c.Rules.Disable, p.Disabled, checkName); err != nil { + return p, err + } + 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 + } + if err := checkScanHasRules(c.Rules.Only, p, warn); err != nil { + return p, err } - for _, name := range c.Rules.Disable { - if err := checkName(name); err != nil { + for name, kv := range c.Rules.Settings { + ok, err := checkName(name) + if err != nil { return p, err } - p.Disabled[name] = true - } - for _, name := range c.Rules.Only { - if err := checkName(name); err != nil { + if !ok { + continue + } + kept, err := checkSettings(name, kv, warn) + if err != nil { return p, err } - p.Only[name] = true + if len(kept) > 0 { + p.Settings[name] = kept + } } - for name, sevStr := range c.Rules.Severity { - if err := checkName(name); err != nil { - return p, err + return p, nil +} + +// 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. +// +// All three report nothing on any codebase, which reads as a clean scan. +// +// 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(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 !p.Skip(name) { + return nil + } + } + 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)") +} + +// 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 +// `disable`. +func applySeverities( + sevs map[string]string, + p *analyzer.Profile, + checkName func(string) (bool, error), + warn func(string, ...any) error, +) error { + for name, sevStr := range sevs { + 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 p, err + return err } continue } @@ -223,13 +312,148 @@ 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 +} + +// 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 + // 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 +// 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": settingPositiveDuration}, + "n-plus-one": {"threshold": settingPositiveInt, "window": settingPositiveDuration}, + "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"}, +} + +// 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. 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 nil, err + } + continue + } + if err := warn("rule %q: unknown setting %q (known: %s)", + rule, key, strings.Join(sortedKeys(kinds), ", ")); err != nil { + return nil, err + } + continue + } + bad, err := checkSettingValue(rule, key, kind, v, warn) + if err != nil { + return nil, err + } + if !bad { + kept[key] = v } - p.Settings[name] = analyzer.Settings(kv) } - return p, nil + + pair := pairedSettings[rule] + if len(pair) == 0 { + 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 := kept[key]; present { + have = append(have, key) + } else { + missing = append(missing, key) + } + } + if len(have) > 0 && len(missing) > 0 { + return kept, warn("rule %q: setting %q has no effect without %q", + rule, strings.Join(have, ", "), strings.Join(missing, ", ")) + } + return kept, nil +} + +// 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 + // drift apart. + one := analyzer.Settings{key: v} + + switch kind { + case settingDuration, settingPositiveDuration: + 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. + return true, warn("rule %q: setting %q: expected a duration or a number, got %v", rule, key, v) + } + if kind == settingPositiveDuration && d <= 0 { + 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) + 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 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 true, warn("rule %q: setting %q must be greater than 0, got %d", rule, key, n) + } + } + return false, nil +} + +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 @@ -248,20 +472,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..d74ad6c 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -1,8 +1,10 @@ package config import ( + "fmt" "os" "path/filepath" + "strings" "testing" "time" @@ -30,8 +32,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 +57,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 +196,370 @@ 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_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}, + // 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"}}, 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{ + "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": "1s"}, + "n-plus-one": {"window": 500, "threshold": 10}, + }}} + if _, err := c.Profile(); err != nil { + t.Fatalf("valid settings rejected: %v", err) + } + }) + + // 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 "}, + }}} + p, err := c.Profile() + if err != nil { + t.Fatalf("a padded duration should be accepted: %v", err) + } + 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) + } + }) +} + +// 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) + } + // 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) + } + + 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) + } + } +} + +// 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]) + } + }) + } + + // 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 { + 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) + } + }) +} + +// 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/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/config/middleware_test.go b/config/middleware_test.go index cfe4b9f..e48ec27 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,87 @@ 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) } + +// 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/explain.go b/explain/explain.go index aa3c08a..2f571c6 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,38 @@ 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 { + return analyzer.RuleDefaultSeverityOr(name, analyzer.SeverityWarning) +} + +// 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. +// +// 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. +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. @@ -194,9 +243,13 @@ func (p *PlanAnalyzer) walkPgPlan(node *pgPlanNode, query string, issues *[]anal // Detect sequential scans if node.NodeType == "Seq Scan" { - severity := analyzer.SeverityInfo + // 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 { - severity = analyzer.SeverityWarning + severity = max(severity, analyzer.SeverityWarning) } *issues = append(*issues, analyzer.Result{ RuleName: "seq-scan", @@ -211,7 +264,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.", @@ -320,7 +373,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.", @@ -331,7 +384,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.", @@ -342,7 +395,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/explain/rule_profile_test.go b/explain/rule_profile_test.go new file mode 100644 index 0000000..00de18c --- /dev/null +++ b/explain/rule_profile_test.go @@ -0,0 +1,176 @@ +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) + } +} + +// 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{"select-star": true}, + })} + + got := p.applyProfile(planIssues()) + + 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") + } + } +} + +// 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") + } + } +} + +// 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") + } + t.Cleanup(func() { + analyzer.Register(analyzer.RuleSpec{Name: "seq-scan", DefaultSeverity: orig}) + }) + analyzer.Register(analyzer.RuleSpec{Name: "seq-scan", DefaultSeverity: analyzer.SeverityCritical}) + + 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{} + + 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 e29e92b..ad67ab0 100644 --- a/middleware/guard.go +++ b/middleware/guard.go @@ -22,6 +22,32 @@ 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` 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 +} + +// 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 := analyzer.RuleDefaultSeverityOr(name, analyzer.SeverityWarning) + return findingPolicy{ + enabled: a.RuleEnabled(name), + severity: a.RuleSeverity(name, def), + } } // NewGuard builds a Guard from the given options. @@ -33,12 +59,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") 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") + if o.enableN1 && n1.enabled { + g.tracker = NewQueryTracker(o.n1Threshold, o.n1Window, n1.severity, func(results []analyzer.Result) { o.reporter.Report(results) }) } @@ -96,12 +141,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..8263429 --- /dev/null +++ b/middleware/rule_profile_test.go @@ -0,0 +1,237 @@ +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()) + } +} + +// 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 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), WithN1Detection(2, time.Minute)) + + g.CheckLatency("SELECT 1", time.Second) + + // `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") + } + }) +} + +// 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 6c65410..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 @@ -56,15 +61,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 +87,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` | 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). | -| `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 +112,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 the N+1 and slow-query +_thresholds_. Turning either rule off goes the other way — see +[Precedence](#precedence). ## Loading it from Go @@ -124,13 +140,23 @@ 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; 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 @@ -144,8 +170,50 @@ 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]` 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:` 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 0d24ee9..1c2775b 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,25 @@ 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. + +`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 are requested as `EXPLAIN FORMAT=TRADITIONAL` — MySQL 9 defaults @@ -147,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 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/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 362f845..369d82a 100644 --- a/website/docs/rules.md +++ b/website/docs/rules.md @@ -39,6 +39,24 @@ 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` 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:` 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 +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 +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) @@ -182,8 +200,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. @@ -191,7 +212,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). @@ -199,8 +221,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) 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