Harden the (*gorm.DB).Scan guard test: three evasions, an overstated doc comment, and a weak file-count floor #232

New Issue

clawbot · 2026-08-20T07:21:18+02:00

clawbot commented

2026-08-20 07:21:18 +02:00

Follow-up to #222, which added internal/gormlog/scan_guard_test.go to fail if production code calls (*gorm.DB).Scan — the one GORM path that bypasses the ParamsFilter value suppression and would leak bound values (session key, password hash) into the log.

The guard works for accidental use: a reviewer planted twelve Scan shapes and it reported nine — local variable, struct field, function return value, slice index, map index, parenthesised receiver, chain split across lines, *gorm.DB via a type alias, and a direct Raw(...).Scan(...). Merged on that basis; the items below are hardening, not a live leak. The tree today has exactly one (*gorm.DB).Scan caller, internal/database/database_test.go:91, and its SELECT 1 binds nothing.

Three evasions found:

h.QueryRow("SELECT 1").Scan(&n) where QueryRow is a repo-local method returning *gorm.DB
h.Row().Scan(&n) — same, via a method named Row
f := d.db.Scan; f(&n) — a method value. The Scan selector is never the Fun of a CallExpr, so unguardedScans never inspects it. This one is OUTSIDE the fail-closed rule rather than covered by it.

The first two work because isRowProducer matches the receiver's selector NAME only, with no type resolution — so naming a gorm-returning helper Row or QueryRow defeats it.

The doc comment overstates what the code does. scan_guard_test.go:22-25 describes isRowProducer as identifying methods that return a database/sql row handle, implying type resolution. It is a name allowlist. #222 was failed once already for shipping a false claim in a comment; the same standard applies here.

Two weaknesses in the vacuity guards:

minNonTestFiles = 40 against 60 actual files: the walk could skip all 16 files of internal/database and still pass at 44.
TestScanGuard_ReportsPlantedCalls's "seven snippets" are really two AST receiver forms among the four positives. The struct-field form its own doc comment names is untested, and the sql rows snippet would not compile (Rows() returns two values) — snippets are parsed, never type-checked.

Definition of done:

isRowProducer either resolves types (go/types) or its comment says plainly that it is a name allowlist and names the evasion
method values (f := db.Scan) are detected, or explicitly documented as out of scope with the reason
the file-count floor cannot pass while an entire package is skipped — assert on packages walked, or on an expected file set
the planted-snippet table covers each AST receiver form the guard claims to handle, and the snippets type-check

Also worth folding in, from the same review: six test-only gorm.Open sites in internal/delivery (archive_sweeper_test.go x3, engine_test.go, engine_integration_test.go, target_database_test.go) still use a bare &gorm.Config{} rather than gormlog.New. No secrets are involved — fixture data only, and all three PRODUCTION gorm.Open sites route through gormlog.New — but it leaves the unfiltered idiom in-tree to be copied, which is the same concern that failed the first round of #222. Route them through an explicit discard sink; done when no gorm.Open in the tree uses a bare &gorm.Config{}.

Follow-up to https://git.eeqj.de/sneak/webhooker/pulls/222, which added `internal/gormlog/scan_guard_test.go` to fail if production code calls `(*gorm.DB).Scan` — the one GORM path that bypasses the `ParamsFilter` value suppression and would leak bound values (session key, password hash) into the log. The guard works for accidental use: a reviewer planted twelve `Scan` shapes and it reported nine — local variable, struct field, function return value, slice index, map index, parenthesised receiver, chain split across lines, `*gorm.DB` via a type alias, and a direct `Raw(...).Scan(...)`. Merged on that basis; the items below are hardening, not a live leak. The tree today has exactly one `(*gorm.DB).Scan` caller, `internal/database/database_test.go:91`, and its `SELECT 1` binds nothing. **Three evasions found:** 1. `h.QueryRow("SELECT 1").Scan(&n)` where `QueryRow` is a repo-local method returning `*gorm.DB` 2. `h.Row().Scan(&n)` — same, via a method named `Row` 3. `f := d.db.Scan; f(&n)` — a method value. The `Scan` selector is never the `Fun` of a `CallExpr`, so `unguardedScans` never inspects it. This one is OUTSIDE the fail-closed rule rather than covered by it. The first two work because `isRowProducer` matches the receiver's selector NAME only, with no type resolution — so naming a gorm-returning helper `Row` or `QueryRow` defeats it. **The doc comment overstates what the code does.** `scan_guard_test.go:22-25` describes `isRowProducer` as identifying methods that return a `database/sql` row handle, implying type resolution. It is a name allowlist. https://git.eeqj.de/sneak/webhooker/pulls/222 was failed once already for shipping a false claim in a comment; the same standard applies here. **Two weaknesses in the vacuity guards:** - `minNonTestFiles = 40` against 60 actual files: the walk could skip all 16 files of `internal/database` and still pass at 44. - `TestScanGuard_ReportsPlantedCalls`'s "seven snippets" are really two AST receiver forms among the four positives. The struct-field form its own doc comment names is untested, and the `sql rows` snippet would not compile (`Rows()` returns two values) — snippets are parsed, never type-checked. Definition of done: - `isRowProducer` either resolves types (`go/types`) or its comment says plainly that it is a name allowlist and names the evasion - method values (`f := db.Scan`) are detected, or explicitly documented as out of scope with the reason - the file-count floor cannot pass while an entire package is skipped — assert on packages walked, or on an expected file set - the planted-snippet table covers each AST receiver form the guard claims to handle, and the snippets type-check Also worth folding in, from the same review: six test-only `gorm.Open` sites in `internal/delivery` (`archive_sweeper_test.go` x3, `engine_test.go`, `engine_integration_test.go`, `target_database_test.go`) still use a bare `&gorm.Config{}` rather than `gormlog.New`. No secrets are involved — fixture data only, and all three PRODUCTION `gorm.Open` sites route through `gormlog.New` — but it leaves the unfiltered idiom in-tree to be copied, which is the same concern that failed the first round of https://git.eeqj.de/sneak/webhooker/pulls/222. Route them through an explicit discard sink; done when no `gorm.Open` in the tree uses a bare `&gorm.Config{}`.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/webhooker#232