docs(audit): add audit reports for pkg/testmodels and pkg/tracing

fix(json-columns): skip JSON select columns with no model scan target
Requesting a JSON sub-field (e.g. jsonvalue->'product'->>'cost') that has no matching bun scanonly field on the model made the whole read fail with "bun: ModelX does not have column Y", since bun scans SELECT results straight into the typed model struct. Add reflection.HasColumn to check whether the model can actually receive a given column (including scanonly fields, walking embedded structs), and gate the JSON select-column expression on it in ApplySelectColumns (shared by websocketspec/mqttspec) and the resolvespec/restheadspec handlers. When there's no scan target, drop just that column with a warning instead of erroring the whole request.
2026-09-29 19:42:00 +00:00 · 2026-09-29 17:15:00 +02:00 · 2026-09-28 12:30:56 +02:00
17 changed files with 10022 additions and 0 deletions
@@ -0,0 +1,646 @@
 # Audit: cross-cutting findings across `pkg/*`
 | | |
 |---|---|
 | **Scope** | all 23 packages under `pkg/` (64 065 non-test lines) |
 | **Audit date** | 2026-09-29 |
 | **Axes** | thread locking/waiting, slowness, security, panic handling & logging |
 | **Threat model** | hostile internet client; request bodies, headers, query params, schema/table/column names all attacker-controlled |
 This file records findings that are **not specific to one package** — they are
 properties of the repository or patterns repeated across many packages. The
 per-package audits reference this file rather than restating them.
 ## Findings
 | # | Severity | Axis | Finding |
 |---|---|---|---|
 | X1 | **High** | locking | `-race` is never run anywhere; no package is ever race-checked |
 | X2 | **High** | testing | `go test` runs against 2 of 23 packages; the other 21 are only compiled and vetted |
 | X3 | **High** | testing | Every integration-test step is `continue-on-error: true` — integration failures cannot fail CI |
 | X10 | **High** | security | Whole subsystems are declared, configured, documented and tested but never installed — including every protective middleware and the metrics provider |
 | X4 | **Medium** | security | `gosec` is not enabled in `.golangci.json`; no SAST runs on a package set full of dynamic SQL |
 | X5 | **Medium** | locking | Unsynchronized package-level mutable globals are the dominant concurrency pattern |
 | X6 | **Medium** | security | Insecure-by-default transport across the board: `sslmode: disable`, `WithInsecure()`, no TLS in cache configs |
 | X7 | **Medium** | panic handling | Panic handling is inconsistent and, where it exists, tends to fail open |
 | X8 | **Medium** | security | `logger.Warn`/`Error` forward every message to Sentry unscrubbed, and error strings routinely embed attacker data |
 | X9 | **Low** | testing | Test coverage is extremely uneven: 5 packages have no test file at all |
 The table is ordered by severity; the sections below are in ID order, since other
 audit files reference these findings by number.
 ---
 ### X1. High — `-race` is never run
 Verified by grep: the string `-race` does not appear in `Makefile`,
 `.github/workflows/tests.yml`, `.github/workflows/maint.yml` or
 `.github/workflows/make_tag.yml`.
 Every test invocation in the repository:
 ```makefile
 # Makefile:8
 	@go test ./pkg/resolvespec ./pkg/restheadspec -v -cover
 # Makefile:13
 	@go test -tags=integration ./pkg/resolvespec ./pkg/restheadspec -v
 # Makefile:97
 	@go test -tags=integration ./pkg/resolvespec ./pkg/restheadspec -v
 # Makefile:103
 	@go test ./pkg/resolvespec ./pkg/restheadspec -coverprofile=coverage.out
 # Makefile:110
 	@go test -tags=integration ./pkg/resolvespec ./pkg/restheadspec -coverprofile=coverage-integration.out
 ```
 ```yaml
 # .github/workflows/tests.yml — unit-tests job
      - name: Run unit tests
        run: go test ./pkg/resolvespec ./pkg/restheadspec -v -cover
 ```
 **Why this matters.** This audit found unsynchronized concurrent access to
 mutable state in **six** packages, and the Go race detector would have flagged
 every one of them on the first run:
 | Package | Racing state | Reference |
 |---|---|---|
 | `pkg/cache` | `defaultCache` read/written by concurrent request handlers | `cache.audit.md` finding 3 |
 | `pkg/config` | `*viper.Viper` has no internal lock; `configInstance` singleton | `config.audit.md` findings 1, 2 |
 | `pkg/logger` | `Logger`, `errorTracker` globals | `logger.audit.md` finding 1 |
 | `pkg/modelregistry` | `defaultRegistry` read by 6 functions without the lock | `modelregistry.audit.md` findings 2, 8 |
 | `pkg/tracing` | `tracer` global | `tracing.audit.md` finding 5 |
 | `pkg/errortracking` | `sentry.Init` mutates process globals | `errortracking.audit.md` finding 2 |
 **Failure scenario.** `pkg/config` finding 1 is the sharpest illustration. A
 concurrent `Manager.Set`/`Manager.Get` pair reaches viper's internal maps, which
 have no mutex. A concurrent map read and write in Go is not a panic — it is
 `fatal error: concurrent map read and map write`, which **`recover()` cannot
 catch**. The process dies instantly, mid-request, with no graceful shutdown and
 no error-tracker report. That is a remotely-triggerable hard crash, and it
 cannot be found by inspection at scale — it is precisely what `-race` exists to
 find. The detector has been in Go since 1.1 and costs one flag.
 **Recommendation.** Add a race job that covers everything, and keep it separate
 from the coverage run (race builds are ~2–10× slower):
 ```makefile
 test-race:
 	@go test -race -count=1 ./pkg/...
 ```
 ```yaml
  race-tests:
    name: Race Detector
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v6
      - uses: actions/setup-go@v6
        with: { go-version: "1.24" }
      - run: go test -race -count=1 ./pkg/...
 ```
 Expect it to fail on the first run — that is the point. Fix `pkg/logger`,
 `pkg/config` and `pkg/cache` first, since they are the shared dependencies. Note
 that the race detector only reports races that **actually execute**, so X1 and X2
 have to be fixed together: a race detector pointed at packages with no tests
 finds nothing.
 ---
 ### X2. High — `go test` runs against 2 of 23 packages
 Every `go test` invocation in the repository names exactly
 `./pkg/resolvespec ./pkg/restheadspec`. No invocation uses `./...` or
 `./pkg/...`.
 The test bodies that exist but are never executed by CI:
 | Package | Test files | Test lines | Run by CI? |
 |---|---|---|---|
 | `restheadspec` | 19 | 5 123 | **yes** |
 | `resolvespec` | 8 | 2 379 | **yes** |
 | `security` | 15 | 6 359 | no |
 | `common` | 10 | 3 644 | no |
 | `reflection` | 8 | 3 404 | no |
 | `websocketspec` | 6 | 3 092 | no |
 | `funcspec` | 3 | 2 416 | no |
 | `spectypes` | 7 | 2 367 | no |
 | `eventbroker` | 4 | 1 527 | no |
 | `mqttspec` | 3 | 1 408 | no |
 | `middleware` | 5 | 1 127 | no |
 | `openapi` | 2 | 1 022 | no |
 | `server` | 2 | 694 | no |
 | `dbmanager` | 2 | 659 | no |
 | `config` | 1 | 608 | no |
 | `cache` | 1 | 69 | no |
 | `errortracking` | 1 | 67 | no |
 | `metrics` | 1 | 64 | no |
 | `resolvemcp` | 1 | 34 | no |
 | `logger` | 0 | 0 | — |
 | `modelregistry` | 0 | 0 | — |
 | `testmodels` | 0 | 0 | — |
 | `tracing` | 0 | 0 | — |
 **Failure scenario.** `pkg/security` has 6 359 lines of tests — the largest test
 body in the repository — and **not one of them runs in CI**. A change that breaks
 authentication, column-level security or row-security templates merges green.
 The `maint.yml` job named "Run Vet Tests" is misleading: it runs `go mod
 download`, `go mod verify` and `go vet ./...` and contains **no `go test` step at
 all** (verified by grep). So the only signal on 21 of 23 packages is "it
 compiles and vet is happy".
 This directly explains the density of findings in this audit. The
 `pkg/modelregistry` authorization fail-open (`modelregistry.audit.md` finding 1)
 and the `pkg/cache`/`pkg/security` auth-outage-on-cache-failure
 (`cache.audit.md` finding 1) are both the kind of defect a single unit test would
 have caught, in packages that have never been tested.
 **Recommendation.** Change every invocation to `./pkg/...`:
 ```makefile
 test-unit:
 	@go test ./pkg/... -v -cover
 ```
 ```yaml
      - name: Run unit tests
        run: go test ./pkg/... -v -cover
 ```
 If some currently-unrun package fails immediately, that is a bug report, not a
 reason to keep the narrow list. Quarantine individual failing tests with
 `t.Skip` and a `TODO` referencing an issue, so the *package* stays in the set.
 ---
 ### X3. High — integration failures cannot fail CI
 `.github/workflows/tests.yml`, `integration-tests` job — every meaningful step
 carries `continue-on-error: true`:
 ```yaml
      - name: Run resolvespec integration tests
        continue-on-error: true
        env:
          TEST_DATABASE_URL: "host=localhost user=postgres password=postgres dbname=resolvespec_test port=5432 sslmode=disable"
        run: go test -tags=integration ./pkg/resolvespec -v -coverprofile=coverage-resolvespec-integration.out
      - name: Run restheadspec integration tests
        continue-on-error: true
        ...
 ```
 **Failure scenario.** The integration suites are the only tests that exercise
 real SQL generation against a real PostgreSQL — i.e. the only automated check on
 the identifier-quoting and filter-construction paths that this audit's threat
 model cares most about. Because both steps are `continue-on-error`, a SQL
 injection regression, a broken join, or a total suite failure (wrong DSN, missing
 migration) shows as a green check mark with a collapsed red step that nobody
 opens. The job has no step that fails, so the job always passes. This is
 strictly worse than not having the tests, because it creates the appearance of
 coverage.
 Note the integration DSN itself uses `sslmode=disable`, consistent with X6.
 **Recommendation.** Remove `continue-on-error` from the two `go test` steps.
 Keep it only on the coverage-report generation and artifact-upload steps, which
 genuinely should not fail a build. If the suites are currently flaky, fix or
 skip the flaky tests individually — `continue-on-error` on the whole step
 disables the signal entirely.
 ---
 ### X4. Medium — `gosec` is not enabled
 `.golangci.json` (`version: 2`) enables exactly three linters beyond the v2
 standard set:
 ```json
  "linters": {
    "enable": [
      "gocritic",
      "misspell",
      "revive"
    ],
 ```
 golangci-lint v2's standard set (`errcheck`, `govet`, `ineffassign`,
 `staticcheck`, `unused`) is on by default, so those do run. **`gosec` does not** —
 it appears in the file only inside an exclusion rule for `_test.go`:
 ```json
        {
          "linters": [
            "dupl",
            "errcheck",
            "gocritic",
            "gosec"
          ],
          "path": "_test\\.go"
        },
 ```
 Listing a linter in `exclusions.rules` does not enable it. The `lint` job in
 `.github/workflows/maint.yml:40-57` does run golangci-lint over the whole
 repository with `version: latest`, so the config is applied — it simply never
 asks for the security checks.
 **Failure scenario.** This repository builds SQL by string construction from
 attacker-controlled schema, table, column and filter names (see
 `restheadspec.audit.md` and `common.audit.md`). `gosec`'s `G201`/`G202`
 (SQL string formatting/concatenation) are exactly the rules that would flag a
 new `fmt.Sprintf` into a query, which is the single most likely way a SQL
 injection enters this codebase. Also unenabled and relevant: `G104` (unhandled
 errors — this audit found ~20 discarded errors in `pkg/cache` alone), `G304`
 (file path from variable — relevant to `PathsConfig.Join`, `config.audit.md`
 finding 14), `G402` (bad TLS settings — X6), `G404` (weak random).
 **Recommendation.** Add `gosec` to `linters.enable` and triage the initial
 findings. Expect noise on the SQL rules given the architecture; suppress
 individual verified-safe sites with `//nolint:gosec // G201: identifier is
 validated by X` comments that name the invariant, rather than disabling the rule
 globally. That converts each suppression into a reviewable claim.
 Consider also `bodyclose`, `rowserrcheck` and `sqlclosecheck` for a
 database-heavy codebase, and `contextcheck` given how many methods here accept a
 `ctx` and ignore it.
 ---
 ### X5. Medium — unsynchronized mutable package globals are the dominant pattern
 Nine of the twenty-three packages expose mutable process-wide state through
 package-level variables, and most guard it with nothing:
 | Package | Global | Guarded? |
 |---|---|---|
 | `pkg/logger` | `Logger *zap.SugaredLogger` (`logger.go:15`), `errorTracker` (`:16`) | **no** — and `Logger` is exported |
 | `pkg/cache` | `defaultCache *Cache` (`cache.go:10`) | **no** |
 | `pkg/config` | `configInstance *Manager` (`manager.go:15`) | **no** |
 | `pkg/tracing` | `tracer` (`tracing.go:19`) | **no** |
 | `pkg/modelregistry` | `defaultRegistry` | partially — `TryLock` with a retry/`time.Sleep` loop, and 6 functions read it unlocked |
 | `pkg/metrics` | `globalProvider` (`interfaces.go:50-51`) | **yes** — `globalProviderMu sync.RWMutex` |
 `pkg/metrics` is the model the others should follow:
 ```go
 // pkg/metrics/interfaces.go:50-72
 var (
 	globalProviderMu sync.RWMutex
 	globalProvider   Provider
 )
 func SetProvider(p Provider) {
 	globalProviderMu.Lock()
 	globalProvider = p
 	globalProviderMu.Unlock()
 }
 func GetProvider() Provider {
 	globalProviderMu.RLock()
 	p := globalProvider
 	globalProviderMu.RUnlock()
 	if p == nil {
 		return &NoOpProvider{}
 	}
 	return p
 }
 ```
 Note that it also returns a working `NoOpProvider` rather than `nil`, so callers
 need no nil check — the pattern `pkg/logger` and `pkg/cache` should copy.
 **Failure scenario.** Beyond the data races in X1, the shared failure mode is
 **lazy initialization on the request path**. `cache.GetDefaultCache()`
 (`cache.go:48`) and `config.GetConfigManager()` (`manager.go:18`) both
 `if x == nil { x = construct() }` with no `sync.Once`. Under concurrent first
 traffic, several instances are constructed and all but one are silently
 discarded, so writes go to an orphaned object — a cache that is permanently 100%
 miss, or two `Manager`s disagreeing about configuration. It presents as "the
 cache doesn't work" with no error anywhere.
 `pkg/logger.Logger` being **exported** and mutable is its own hazard: any
 package, or any consumer of this library, can reassign the process logger
 mid-flight while other goroutines are calling `Logger.Infow`.
 **Recommendation.** For each global: `atomic.Pointer[T]` for
 single-pointer swaps, `sync.Once` for lazy defaults, `sync.RWMutex` for
 multi-field state. Unexport `logger.Logger` behind accessors. Where a nil global
 is possible, return a no-op implementation instead of `nil`, as
 `pkg/metrics.GetProvider` does.
 ---
 ### X6. Medium — insecure transport is the default everywhere
 Every network dependency defaults to cleartext, and in two cases there is no way
 to configure otherwise:
 | Component | Default | Configurable? | Reference |
 |---|---|---|---|
 | PostgreSQL | `sslmode: disable` (`config/manager.go:242`) | yes, via config | `config.audit.md` finding 3 |
 | OTLP traces | `otlptracegrpc.WithInsecure()` hardcoded (`tracing/tracing.go:41`) | **no** — `Config` has no field for it | `tracing.audit.md` finding 1 |
 | Redis (cache) | no `TLSConfig` set | **no** — `RedisConfig` has no TLS field | `cache.audit.md` finding 15 |
 | Memcache | no TLS | **no** | `cache.audit.md` finding 15 |
 | CORS | `allowed_origins: ["*"]`, `allowed_headers: ["*"]` (`config/manager.go:214-216`) | yes | `config.audit.md` finding 3 |
 | DB user | `user: postgres` with blank password (`config/manager.go:239-240`) | yes | `config.audit.md` finding 3 |
 The `tracing.go:41` case is the most pointed, because the code knows better:
 ```go
 otlptracegrpc.WithInsecure(), // Use WithTLSCredentials in production
 ```
 The comment names the fix and the config struct provides no way to apply it.
 **Failure scenario.** The cache holds `UserContext` — identity and authorization
 data — keyed by the raw bearer token (`security/providers.go:398`). With no TLS,
 anything on the path between the service and Redis can read session contents and
 the `AUTH` password, then **write** a forged `auth:session:<token>` entry.
 `GetOrSet` returns a cache hit without consulting the database, so a forged entry
 is a complete authentication bypass. Meanwhile the trace exporter ships full
 request URLs including query strings (`tracing.audit.md` finding 2) in cleartext
 to the collector.
 **Recommendation.** Invert every default: TLS on unless explicitly disabled.
 Concretely — add `TLS`/`TLSSkipVerify`/`TLSCACertFile` to `cache.RedisConfig`
 and `tracing.Config`; change the `sslmode` default to `require`; change
 `cors.allowed_origins` to `[]` and require an explicit list; remove the default
 `postgres`/blank-password credentials so a misconfigured deployment fails to
 start rather than connecting to a local database as a superuser. Add a startup
 validation pass that logs a prominent warning for each insecure setting actually
 in effect.
 ---
 ### X7. Medium — panic handling is inconsistent, and where it exists it fails open
 Three different conventions coexist:
 1. **`logger.CatchPanic(location)`** (`logger/logger.go:184`) — recovers, logs,
   reports, and **swallows**. Both call sites are security enforcement:
   `security/provider.go:302` (`ApplyColumnSecurity`) and `:443`
   (`GetRowSecurityTemplate`). See `logger.audit.md` finding 4.
 2. **`logger.HandlePanic(method, r)`** (`logger/logger.go:197`) — converts the
   panic to an `error` the caller must handle. This is the correct shape.
 3. **Nothing at all.** `pkg/cache` has zero `recover()` calls in 1 538 lines;
   so do several other packages.
 **Failure scenario (fail-open).** `ApplyColumnSecurity` panics — a nil map, a
 bad type assertion on a rule, a reflection edge case. `CatchPanic` recovers and
 the function returns normally, so the caller believes column security was
 applied. It was not. The response contains the columns the security layer was
 supposed to strip. The panic is logged, but the request succeeds with elevated
 data exposure. A security control whose failure mode is "allow" is the wrong
 default; it must be "deny".
 **Failure scenario (panic under a lock).** `pkg/cache` holds `m.mu` across
 `m.items[key] = ...` (`provider_memory.go:111`). After `Close()` sets
 `items = nil` that assignment panics. With no recover in the package the panic
 propagates to whatever handler exists upstream; if that handler recovers, `m.mu`
 is **never unlocked** and every subsequent cache operation blocks forever. The
 process stays alive and wedged — worse than a crash, because health checks that
 do not touch the cache keep passing.
 **Recommendation.** Establish one convention and apply it:
 - **Request boundaries** (HTTP handlers, event consumers, goroutines): recover,
  log with stack, report to the error tracker, return 500 / nack. A `go`
  statement without a deferred recover is a process-kill waiting to happen —
  `security/providers.go:447` (`go a.updateSessionActivity(...)`) is one.
 - **Security enforcement**: recover, log, and **fail closed** — return an error
  that the caller must propagate as a denial. Never `CatchPanic`.
 - **Internal helpers**: do not recover. Let the boundary handle it.
 - **Anything holding a lock**: prefer `defer mu.Unlock()` (already the pattern in
  `pkg/cache`) so a panic cannot leak the lock, and keep panicking code out of
  critical sections.
 Add a `CatchPanicFailClosed(location string, err *error)` helper so the
 fail-closed variant is as easy to reach for as `CatchPanic`.
 ---
 ### X8. Medium — attacker data reaches Sentry unscrubbed
 Two facts compose badly:
 `pkg/logger/logger.go:125-140` — every `Error` (and every `Warn`, `:108-123`)
 forwards the fully-formatted message to the error tracker:
 ```go
 func Error(template string, args ...interface{}) {
 	ctx, remainingArgs := extractContext(args...)
 	message := fmt.Sprintf(template, remainingArgs...)
 	...
 	if errorTracker != nil {
 		errorTracker.CaptureMessage(ctx, message, errortracking.SeverityError, map[string]interface{}{
 			"process_id": os.Getpid(),
 		})
 	}
 }
 ```
 And `pkg/errortracking` installs **no `BeforeSend` scrubber**
 (`errortracking.audit.md` finding 1), so the message goes to Sentry verbatim.
 Meanwhile error strings across the codebase interpolate attacker-controlled
 values, sometimes secrets:
 | Site | Interpolated value |
 |---|---|
 | `cache/cache_manager.go:26`, `:40` | the full cache key — for the session cache, **the raw bearer token** |
 | `security/providers.go:391` | the raw `Authorization` header, logged at `Warn` when multiple tokens are present |
 | `config/manager.go:164` | config file paths |
 | throughout `restheadspec` | schema, table, column and filter values from the request |
 **Failure scenario.** `security/providers.go:391` is live today:
 ```go
 logger.Warn("Multiple authentication tokens provided in Authorization header (%d tokens). This is unusual and may indicate a misconfigured client. Header: %s", len(tokens), sessionToken)
 ```
 A client sends two bearer tokens. `logger.Warn` formats the full header value
 into the message and forwards it to Sentry, where a **valid session credential**
 is now stored by a third party, visible to everyone with Sentry access, retained
 per Sentry's policy, and replayable for the token's lifetime. No attacker
 sophistication is required — the trigger is a single extra header, and the
 codebase invites it by logging the header contents as the diagnostic.
 **Recommendation.**
 1. Add a `BeforeSend` hook in `pkg/errortracking` that redacts
   `Authorization`, `Cookie`, `Set-Cookie`, anything matching
   `(?i)(token|password|secret|apikey|api_key|bearer)\s*[:=]\s*\S+`, and
   long high-entropy strings. This is the one change that bounds the whole class.
 2. Never log a credential, even truncated. Change `providers.go:391` to log
   `len(tokens)` only.
 3. Replace `fmt.Errorf("key not found: %s", key)` with a sentinel
   `cache.ErrNotFound` (`cache.audit.md` finding 7).
 4. Key the session cache on `sha256(token)`, as
   `security/keystore_database.go:287` already does for API keys.
 5. Add sampling / rate limiting to the tracker fan-out
   (`logger.audit.md` finding 3) so an error storm is not also a cost and
   availability event.
 ---
 ### X9. Low — five packages have no tests at all
 `pkg/logger`, `pkg/modelregistry`, `pkg/testmodels`, `pkg/tracing` have zero
 `*_test.go` files. `pkg/resolvemcp` has 34 lines, `pkg/metrics` 64,
 `pkg/errortracking` 67, `pkg/cache` 69.
 **Failure scenario.** `pkg/modelregistry` is untested and contains this audit's
 only **Critical** authorization finding: `GetModel` returns a "registry locked"
 error under write-lock contention, which `security/hooks.go:274-294` converts
 into `return nil // model not registered, allow by default`
 (`modelregistry.audit.md` finding 1). A twenty-line test that registers a model
 from one goroutine while reading it from another would demonstrate the fail-open
 immediately. The package guards a security boundary and has never been tested.
 `pkg/logger` being untested matters for a different reason: it is imported by
 almost every other package, so a defect there (the format-string sink in `Info`
 and `Debug`, `logger.audit.md` finding 6) is repo-wide.
 **Recommendation.** Prioritize by blast radius, not by size:
 1. `pkg/modelregistry` — concurrent register/read; assert `GetModelRulesByName`
   never returns a "locked" error that a caller could read as "not registered".
 2. `pkg/logger` — nil-`Logger` fallback paths, format-string handling, and that
   `Warn`/`Error` do not forward secrets once a scrubber exists.
 3. `pkg/cache` — concurrent `GetDefaultCache`, the expired-item TOCTOU, and that
   `tagToKeys` does not grow after eviction.
 4. `pkg/tracing`, `pkg/metrics`, `pkg/errortracking` — construction and no-op
   paths; these are mostly configuration surfaces.
 Combine with X1 and X2: tests that are not run, and tests run without `-race`,
 do not close these gaps.
 ---
 ### X10. High — configured subsystems that are never installed
 Three separate subsystems are fully built — typed config, defaults, tests,
 documentation — and then never connected to anything that runs.
 **1. Every protective middleware.** `pkg/middleware` provides rate limiting, IP
 blacklisting, request-size limiting and input sanitization. Non-test callers:
 | Constructor | Non-test callers |
 |---|---|
 | `middleware.NewRateLimiter` | **0** |
 | `middleware.NewIPBlacklist` | **0** |
 | `middleware.NewRequestSizeLimiter` | **0** |
 | `middleware.DefaultSanitizer` | **0** outside the package |
 | `middleware.StrictSanitizer` | **0** |
 | `middleware.PanicRecovery` | 1 — `pkg/server/manager.go:466` |
 `pkg/server/manager.go` is the only file outside the package that imports it, and
 only for `PanicRecovery`. The config that exists to drive the rest —
 `MiddlewareConfig.RateLimitRPS`, `.RateLimitBurst`, `.MaxRequestSize`
 (`pkg/config/config.go:123-125`), defaulted at `pkg/config/manager.go:209-211` —
 has **no reader anywhere in the module**.
 **2. The metrics provider.** `metrics.SetProvider` and
 `metrics.NewPrometheusProvider` have **0 non-test callers**, so
 `metrics.GetProvider()` returns `&NoOpProvider{}`
 (`pkg/metrics/interfaces.go:63-72`) for the process lifetime. Every instrumented
 call site in the repository — 39 DB-query sites in
 `pkg/common/adapters/database`, the HTTP middleware, the event-broker counters,
 and the sole `RecordPanic` call at `pkg/middleware/panic.go:19` — writes to a
 no-op. `MetricsConfig.Enabled` and `.Provider` are likewise never read, and
 `pkg/config` has no `metrics` section at all.
 **3. The configured CORS policy.** `config.CORSConfig`
 (`pkg/config/config.go:128-134`), defaulted at `pkg/config/manager.go:214-217`,
 is never read. The policy that actually applies comes from a **different type of
 the same name**, `common.CORSConfig`, built by `common.DefaultCORSConfig()`
 (`pkg/common/cors.go:19-48`), which derives allowed origins from the configured
 server instances and the host's local IPs and ignores `cors.allowed_origins`
 entirely. It is called from ten sites across `pkg/resolvespec` and
 `pkg/restheadspec`.
 **Failure scenario.** Each of these is a silent, config-shaped lie, and they fail
 in the same way: the operator's mental model of the deployment is wrong in the
 direction of believing a control exists.
 - **Under the hostile-client threat model there is no rate limit and no
  request-body limit in the serving path.** `max_request_size: 10485760` is
  configured and unenforced, so a single unauthenticated `POST` with a
  multi-gigabyte body is read into memory and OOM-kills the process; unlimited
  request rate exhausts the 25-connection default pool
  (`pkg/config/manager.go:224`) just as cheaply. Both are one-line attacks
  against controls the configuration says are active. An operator lowering
  `rate_limit_rps` during an incident observes no change and will reasonably
  conclude the attack exceeds the limit rather than that no limit exists.
 - **There is no telemetry with which to notice any of it.** No request counts, no
  latency histograms, no `panics_total`, no DB-query metrics — the one signal
  that would show an attack in progress is wired end to end and discarded at the
  last step. This is also why the metrics cardinality defects
  (`metrics.audit.md` findings 2 and 5) are only latent: they become live the
  moment someone installs the provider that the config implies is already there.
 - **Tightening `cors.allowed_origins` does nothing.** The value is ignored, so a
  hardening change lands, reviews clean, deploys, and changes no behaviour. Two
  types named `CORSConfig` in two packages is the mechanism; nothing warns.
 The common thread is that none of this fails visibly. It compiles, the tests pass
 (`pkg/middleware` has the repo's best test ratio — 1 127 test lines to 799 code
 lines — all of it exercising code nothing calls), CI is green, and the config file
 documents features that are absent. Under X2 these packages are not even in the
 tested set, so the tests that do exist are not run.
 **Recommendation.**
 1. **Wire the middleware chain** in `pkg/server` from `MiddlewareConfig`,
   outermost first: size limiter → rate limiter → blacklist → `PanicRecovery`
   (innermost, so it sees handler panics; `trackRequestsMiddleware` at
   `manager.go:540` correctly stays outside). Fix the trusted-proxy handling
   (`middleware.audit.md` findings 2 and 3) **before** mounting the two IP-based
   layers, and do not mount the sanitizer at all until findings 5–7 there are
   resolved — as written it corrupts filter values and can synthesize a
   `javascript:` URI.
 2. **Install a metrics provider** from config, gated on `metrics.enabled`, and
   add the missing `metrics` section to `pkg/config`. Bound the label sets first
   (`metrics.audit.md` findings 2 and 5) — installing the provider as-is converts
   two latent cardinality DoS findings into live ones.
 3. **Delete the duplicate `CORSConfig`** or make `common.DefaultCORSConfig()`
   read `config.CORSConfig`. Two types with one name, one of them ignored, is a
   trap regardless of which way it is resolved.
 4. **Make the class of defect detectable.** Log at startup which middleware,
   metrics provider and CORS policy are active, so an unwired subsystem is
   visible in the first ten lines of a boot log instead of during an incident.
   A CI check that every `mapstructure` field in `pkg/config` has at least one
   reader would have caught all three of these; so would enabling `unused` in
   `.golangci.json` for exported-but-unreferenced constructors.
 ---
 ## Recommended order of work
 1. **X2 + X1** — point `go test` at `./pkg/...` and add a `-race` job. Everything
   else in this audit is easier to verify once these exist, and they will
   surface the six data races on their own.
 2. **X3** — remove `continue-on-error` from the integration `go test` steps.
 3. **X10** — mount the request-size limiter and rate limiter. Until this is
   done the service has no volumetric protection at all, and no metrics with
   which to see that. Fix `middleware.audit.md` findings 2 and 3 in the same
   change, since mounting the IP-based layers without them adds attack surface.
 4. **X8 item 1 and 2** — add the Sentry `BeforeSend` scrubber and stop logging
   the `Authorization` header. Small, self-contained, stops an active credential
   leak.
 5. **X7** — decide the panic convention; make the two `CatchPanic` sites in
   `pkg/security` fail closed, and stop returning the panic value to the client
   (`pkg/middleware/panic.go:28`).
 6. **X5** — fix the globals in `pkg/logger`, `pkg/config`, `pkg/cache`
   (the shared dependencies) first.
 7. **X6** — add TLS fields and invert the defaults.
 8. **X4** — enable `gosec` and triage.
 9. **X9** — backfill tests, in the order listed above.
 ## Per-package audits
 `cache` · `common` · `config` · `dbmanager` · `errortracking` · `eventbroker` ·
 `funcspec` · `logger` · `metrics` · `middleware` · `modelregistry` · `mqttspec` ·
 `openapi` · `reflection` · `resolvemcp` · `resolvespec` · `restheadspec` ·
 `security` · `server` · `spectypes` · `testmodels` · `tracing` · `websocketspec`
 Each is `audit/pkg/<name>.audit.md`.
@@ -0,0 +1,401 @@
 # Audit — `pkg/config`
 - **Date:** 2026-09-29
 - **Scope:** `pkg/config/{config,dbmanager,manager,paths,server}.go` (1023 LOC source, 608 LOC tests)
 - **Axes:** thread locking/waiting · slowness · security · panic handling & logging
 - **Threat model:** hostile internet client. Config itself is operator-controlled, so the security
  focus here is **insecure defaults that the internet-facing layers inherit**, plus secret handling.
 ## Summary
 Viper-backed configuration with a singleton `Manager`, a large `setDefaults` table, and per-section
 validators. Two serious issues:
 1. **`Manager` is a data race by construction.** It wraps a `*viper.Viper`, which has **no internal
   locking** (verified: no `sync.Mutex`/`RWMutex` anywhere in `viper@v1.21.0/viper.go`'s `Viper`
   struct), and exposes `Get`/`Set` as concurrently-callable methods on an unsynchronised lazy
   singleton. A concurrent `Set` + `Get` is a concurrent map write → **`fatal error`, not a
   recoverable panic**.
 2. **The default configuration is insecure on every axis that matters** — wildcard CORS,
   `sslmode=disable`, `user: postgres` with a blank password — and `Load()` silently succeeds when
   no config file is found, so a misdeployment lands on exactly those defaults with no warning.
 | # | Severity | Axis | Finding |
 |---|----------|------|---------|
 | 1 | **Critical** | Locking | `Manager.Set`/`Get` over a lock-free `*viper.Viper` → concurrent map write → process-fatal |
 | 2 | **High** | Locking | `GetConfigManager()` is an unsynchronised lazy singleton; `NewManager()` also clobbers the global as a side effect |
 | 3 | **High** | Security | Insecure defaults: `cors.allowed_origins: ["*"]`, `allowed_headers: ["*"]`, `sslmode: disable`, `user: postgres` + blank password |
 | 4 | **High** | Security | `SaveConfig` writes all secrets in plaintext at mode `0644` (viper default, never overridden) |
 | 5 | Medium | Security | `AddConfigPath(".")` is searched first — CWD config injection |
 | 6 | Medium | Observability | `Load()` swallows `ConfigFileNotFoundError` with no log at all |
 | 7 | Medium | Correctness | `PathsConfig.Set` on a nil map panics; every sibling method nil-guards |
 | 8 | Medium | Locking | `PathsConfig` is a bare `map[string]string` with a mutating `Set` — concurrent access is process-fatal |
 | 9 | Medium | Slowness | `GetIPs()` does an uncontexted `net.LookupIP` — blocks on the resolver timeout |
 | 10 | Medium | Correctness | `SetConfig` does a pointless `Unmarshal` into a discarded map whose error fails the call |
 | 11 | Low | Panic | `GetIPs()` recovers to `fmt.Println`, bypassing the logger, and returns zeroed named results |
 | 12 | Low | Security | No validation of `middleware.*` / `event_broker.worker_count` — `0` workers is accepted |
 | 13 | Low | Correctness | `ServersConfig.GetDefault()` returns a pointer to a copy of a map value |
 | 14 | Low | Security | `PathsConfig.Join` does not confine the result to the base path |
 ---
 ## Findings
 ### 1. `Manager` exposes a lock-free viper as a concurrent API (Critical, Locking)
 `manager.go:10-13`, `manager.go:133-158`
 ```go
 type Manager struct {
 	v *viper.Viper
 }
 ...
 func (m *Manager) Get(key string) interface{}    { return m.v.Get(key) }
 func (m *Manager) GetString(key string) string   { return m.v.GetString(key) }
 func (m *Manager) Set(key string, value interface{}) { m.v.Set(key, value) }
 ```
 `viper.Viper` carries its configuration in plain maps (`override`, `config`, `defaults`, `aliases`,
 …) and has **no mutex**. Verified against the module in use:
 ```
 $ grep -n 'sync\.\|Lock()' $(go env GOMODCACHE)/github.com/spf13/viper@v1.21.0/viper.go
 319:	initWG := sync.WaitGroup{}      # inside WatchConfig only
 340:		eventsWG := sync.WaitGroup{}  # inside WatchConfig only
 ```
 `Set` writes to `v.override`; `Get` reads across those maps. Because `GetConfigManager()` hands the
 *same* `*Manager` to every caller, any code path that calls `Manager.Set` at runtime while another
 goroutine reads config is a concurrent map read/write. Go's runtime detects this and issues
 `fatal error: concurrent map read and map write` — which **`recover()` cannot catch**, so none of
 the panic handlers elsewhere in the codebase will save the process.
 This is latent-but-loaded: it needs one runtime `Set` to become a crash. `SetConfig`
 (`manager.go:107-131`) performs eleven `m.v.Set` calls, so any dynamic reconfiguration triggers it.
 **Recommendation:** add a `sync.RWMutex` to `Manager` and take it in every method that touches
 `m.v` (including the `Option` functions at `manager.go:60-85`, which also mutate viper). Better:
 load once into an immutable `*Config` at startup and pass that value around, keeping `Manager`
 confined to startup.
 ### 2. Unsynchronised lazy singleton (High, Locking)
 `manager.go:15-45`
 ```go
 var configInstance *Manager
 func GetConfigManager() *Manager {
 	if configInstance == nil {
 		configInstance = NewManager()
 	}
 	return configInstance
 }
 ```
 Classic check-then-act race: two concurrent first calls both see `nil`, both build a `Manager`,
 and the two callers get *different* instances — so a `Set` through one is invisible through the
 other. The unsynchronised pointer write races with the read.
 Worse, `NewManager()` (`manager.go:27-45`) assigns `configInstance = &Manager{v: v}` at line 43 as
 a **side effect**. So a caller who deliberately builds an isolated manager silently replaces the
 global one, and `NewManagerWithOptions` (`manager.go:48-54`) publishes a half-configured manager to
 the global *before* applying its options — another goroutine can observe the instance mid-mutation.
 **Recommendation:** `sync.Once` for the singleton; remove the global assignment from `NewManager`.
 ### 3. Insecure-by-default configuration (High, Security)
 `manager.go:203-247`:
 ```go
 v.SetDefault("cors.allowed_origins", []string{"*"})
 v.SetDefault("cors.allowed_headers", []string{"*"})
 ...
 v.SetDefault("dbmanager.connections.default.user", "postgres")
 v.SetDefault("dbmanager.connections.default.password", "")
 v.SetDefault("dbmanager.connections.default.sslmode", "disable")
 ```
 Each of these is inherited by an internet-facing layer:
 - **`allowed_origins: ["*"]` + `allowed_headers: ["*"]`** — any origin may make cross-origin calls
  with arbitrary headers. Whether this is exploitable depends on whether the CORS middleware also
  sets `Access-Control-Allow-Credentials`; see `audit/pkg/middleware.audit.md` for that
  determination. Even without credentials, wildcard origin plus wildcard headers defeats any
  header-based CSRF defence and lets a malicious page read responses from a
  network-position-authenticated deployment (IP allowlisted, mTLS-terminated, VPN).
 - **`sslmode: disable`** — DB traffic unencrypted by default. Every row that crosses the wire,
  including whatever the internet-facing handlers select, is plaintext on the network.
 - **`user: postgres` with an empty password** — the default connection targets the PostgreSQL
  superuser. Combined with the identifier-handling concerns in
  `audit/pkg/common.audit.md` / `audit/pkg/restheadspec.audit.md`, running as superuser removes the
  last line of defence (least-privilege) against a query-construction bug.
 Because of finding 6, a deployment with a missing or misnamed config file runs on **all** of these
 simultaneously and reports success.
 **Recommendation:** default to `sslmode: require`, no default DB user/password (fail loudly if
 unset), and `cors.allowed_origins: []` with wildcard requiring an explicit opt-in. Add a
 `Config.Validate()` that refuses `allowed_origins: ["*"]` together with credentials.
 ### 4. `SaveConfig` writes secrets in plaintext at 0644 (High, Security)
 `manager.go:160-166`
 ```go
 func (m *Manager) SaveConfig(path string) error {
 	if err := m.v.WriteConfigAs(path); err != nil { ... }
 }
 ```
 `WriteConfigAs` serialises the **entire** merged configuration. That includes
 `dbmanager.connections.*.password`, `cache.redis.password`, `event_broker.redis.password` and
 `error_tracking.dsn` (a Sentry DSN is a credential).
 Viper writes with `v.configPermissions`, which defaults to `0o644`
 (`viper@v1.21.0/viper.go:198`). `SetConfigPermissions` is **never called anywhere in this repo**
 (verified by grep), so the file is world-readable. Any local user or any other container sharing
 the mount can read the DB superuser password.
 **Recommendation:** call `v.SetConfigPermissions(0o600)` in `NewManager`; better, strip secret keys
 before writing and document that secrets come from env/secret-manager only.
 ### 5. Current-working-directory config injection (Medium, Security)
 `manager.go:32-36`
 ```go
 v.AddConfigPath(".")
 v.AddConfigPath("./config")
 v.AddConfigPath("/etc/resolvespec")
 v.AddConfigPath("$HOME/.resolvespec")
 ```
 Viper searches these **in order** and takes the first hit, so `./config.yaml` wins over
 `/etc/resolvespec/config.yaml`. For a daemon this is backwards: the CWD is the least trustworthy of
 the four. If the process is ever started with its CWD in a shared or user-writable directory (a
 tmp dir, a bind-mounted volume, `/` in some container setups), an attacker with local write
 capability redirects the DB connection, disables TLS, or points `error_tracking.dsn` at their own
 collector — turning finding 1 of `audit/pkg/errortracking.audit.md` into a full exfiltration path.
 **Recommendation:** search `/etc/resolvespec` first, drop `"."` from the default list (keep it
 available via `WithConfigPath`), and log the resolved path at startup (`v.ConfigFileUsed()`).
 ### 6. `Load()` is silent about a missing config file (Medium, Observability)
 `manager.go:87-97`
 ```go
 if err := m.v.ReadInConfig(); err != nil {
 	if _, ok := err.(viper.ConfigFileNotFoundError); !ok {
 		return fmt.Errorf("error reading config file: %w", err)
 	}
 	// Config file not found; will rely on defaults and env vars
 }
 return nil
 ```
 The comment is the only trace. No log line, no returned indicator, no `ConfigFileUsed()` report.
 A typo in the filename, a wrong working directory, or a container that forgot to mount the
 ConfigMap is indistinguishable from a deliberate defaults-only run — and the defaults are the ones
 in finding 3.
 **Recommendation:** log at info level whether a file was used and which one; expose
 `ConfigFileUsed()` on `Manager` so startup can print it.
 ### 7. `PathsConfig.Set` panics on a nil map (Medium, Panic handling)
 `paths.go:38-40`
 ```go
 func (pc PathsConfig) Set(name, path string) {
 	pc[name] = path
 }
 ```
 `PathsConfig` is `map[string]string` (`config.go:200`). `Get`, `GetOrDefault`, `Has` and `List` all
 begin with `if pc == nil`. `Set` does not — and assignment to a nil map is
 `panic: assignment to entry in nil map`.
 `Config.Paths` is populated by `mapstructure`, which leaves the map nil when the `paths` key is
 absent from the file. `setDefaults` does register `paths.data_dir` etc. (`manager.go:249-253`), so
 the map is non-nil on the normal `GetConfig()` path — but a `Config` built in code
 (`config.Config{}`) or produced by a partial unmarshal has a nil `Paths`, and `Set` on it panics.
 Nothing in `pkg/` currently calls `Set` (verified by grep), so this is a latent API defect.
 **Recommendation:** nil-guard consistently, or change the receiver to `*PathsConfig` so `Set` can
 allocate.
 ### 8. `PathsConfig` has no synchronisation (Medium, Locking)
 Same type: a bare map with a mutating `Set` and reading `Get`/`Has`/`List`/`EnsureDir`/`AbsPath`/
 `Join`. If any consumer calls `Set` at runtime while request handlers resolve paths, that is a
 concurrent map write — again the **unrecoverable** `fatal error` class, not a panic.
 Currently unused outside the package, so severity is capped at Medium. If the intent is a runtime
 path registry, it needs a mutex and an unexported map.
 ### 9. `GetIPs()` blocks on an uncontexted DNS lookup (Medium, Slowness)
 `server.go:113-149`
 ```go
 hostname, _ = os.Hostname()
 ...
 addrs, err := net.LookupIP(hostname)
 ```
 `net.LookupIP` has no context and no timeout override — it blocks for the resolver's own timeout,
 which on a misconfigured or slow-resolver host is 5 s per attempt and up to ~15–20 s with retries
 across `/etc/resolv.conf` entries. In a container whose hostname is not in DNS (the normal case)
 this fails, but only *after* the resolver gives up.
 There is no caller in `pkg/` today, so it is not on the request path yet. It is exported and
 named like a utility, so the risk is that it lands on one.
 Secondary correctness problem in the same function: the fallback branch (`server.go:139-147`)
 appends `a.String()` for a `net.Addr` from `net.InterfaceAddrs()`, which renders as CIDR
 (`192.168.1.5/24`), into the same comma-joined string that the primary branch fills with bare IPs.
 Consumers get two formats from one field. That branch also never appends to `ipaddrlist`, so the
 third return value is empty whenever the fallback is taken.
 **Recommendation:** `net.DefaultResolver.LookupIPAddr(ctx, host)` with a short deadline; cache the
 result; normalise the fallback to bare IPs via `net.Addr.(*net.IPNet).IP`.
 ### 10. `SetConfig` does dead work that can fail the call (Medium, Correctness)
 `manager.go:107-131`
 ```go
 configMap := make(map[string]interface{})
 if err := m.v.Unmarshal(&configMap); err != nil {
 	return fmt.Errorf("failed to prepare config map: %w", err)
 }
 // configMap is never read again
 m.v.Set("servers", cfg.Servers)
 ...
 ```
 `configMap` is written and then never used. The comment says "Marshal the config to a map structure
 that viper can use", but it unmarshals *viper's current state* into a throwaway map — it has
 nothing to do with `cfg`. The only effect is that a decode error in the **existing** config makes
 `SetConfig` fail for no reason. It also does a full reflective decode of the whole config tree on
 every call.
 Note also that `SetConfig` stores Go structs into viper via `Set`, and the eleven `Set` calls are
 not atomic — a concurrent `GetConfig()` observes a torn config (new `servers`, old `cors`), on top
 of finding 1's race.
 **Recommendation:** delete the `configMap` block.
 ### 11. `GetIPs()` panic handling bypasses the logger (Low, Panic handling)
 `server.go:114-118`
 ```go
 defer func() {
 	if err := recover(); err != nil {
 		fmt.Println("Recovered in GetIPs", err)
 	}
 }()
 ```
 - Writes to stdout with `fmt.Println` rather than `logger.Error`/`logger.HandlePanic`, so the event
  never reaches the error tracker and is invisible to structured log collection.
 - No stack trace captured.
 - The function's results are named (`hostname, ipList string, ipNetList []net.IP`) but the body
  builds `iplist`/`ipaddrlist` **locals** and only assigns via the `return` statements. On a panic,
  the deferred recover swallows it and the function returns the *zero* named values — `ipNetList`
  is nil rather than the empty slice callers might expect. Silent empty success.
 `pkg/config` is otherwise the only package outside `pkg/logger` that hand-rolls a recover instead
 of using the shared helpers.
 **Recommendation:** use `defer logger.CatchPanic("GetIPs")()`, or drop the recover — there is no
 panicking operation in this function for it to catch.
 ### 12. No validation of numeric/limit settings (Low, Security)
 `ServerInstanceConfig.Validate` (`server.go:37-68`) and `ServersConfig.Validate`
 (`server.go:71-95`) are good — port range, mutually-exclusive TLS modes, cert/key pairing,
 AutoTLS domains. But nothing validates:
 - `middleware.rate_limit_rps` / `rate_limit_burst` — `0` disables rate limiting silently.
 - `middleware.max_request_size` — `0` may mean unlimited depending on the middleware; see
  `audit/pkg/middleware.audit.md`.
 - `event_broker.worker_count` (default 10) — `0` means no consumers; see
  `audit/pkg/eventbroker.audit.md` for whether that deadlocks publishers or drops events.
 - `dbmanager.max_open_conns`, retry counts/delays — negative or zero values.
 - `cors.allowed_origins: ["*"]` in combination with credentials.
 There is also no top-level `Config.Validate()` that calls the section validators, so nothing
 guarantees `ServersConfig.Validate` ever runs.
 **Recommendation:** add `func (c *Config) Validate() error` that fans out to every section, and
 call it from `GetConfig()`.
 ### 13. `GetDefault()` returns a pointer to a copy (Low, Correctness)
 `server.go:98-110`
 ```go
 instance, ok := sc.Instances[sc.DefaultServer]
 ...
 return &instance, nil
 ```
 `instance` is a copy of the map value. A caller that mutates through the returned pointer — which
 the `*ServerInstanceConfig` receiver on `ApplyGlobalDefaults` (`server.go:12`) invites — changes
 only the copy, and `sc.Instances` is unaffected. This is exactly the shape of bug where timeouts
 appear to be applied but aren't.
 **Recommendation:** make `Instances` a `map[string]*ServerInstanceConfig`, or return by value.
 ### 14. `PathsConfig.Join` does not confine to the base (Low, Security)
 `paths.go:96-104`
 ```go
 parts := append([]string{base}, elem...)
 return filepath.Join(parts...), nil
 ```
 `filepath.Join` calls `Clean`, which *resolves* `..` rather than rejecting it: `Join("data",
 "../../etc/passwd")` returns `../etc/passwd`. Any consumer that passes a request-derived segment
 gets directory traversal out of the configured base. No consumer does today, hence Low, but the
 method's name promises confinement it does not provide.
 **Recommendation:** after joining, verify `strings.HasPrefix(filepath.Clean(result), filepath.Clean(base)+string(os.PathSeparator))`, or use `os.Root`/`filepath.Localize` on the elements.
 ---
 ## What looks right
 - `ServerInstanceConfig.Validate` / `ServersConfig.Validate` (`server.go:37-95`) are thorough:
  port bounds, mutual exclusion of the three TLS modes, cert/key co-presence, AutoTLS domain
  requirement, and a key-vs-`Name` consistency check on the instances map. This is the strongest
  code in the package.
 - `ApplyGlobalDefaults` (`server.go:12-32`) uses `*time.Duration` fields so "unset" is
  distinguishable from "zero" — the right modelling choice, and it copies into a fresh local
  before taking its address rather than aliasing the loop/parameter variable.
 - `Load()` correctly distinguishes `ConfigFileNotFoundError` from real read errors instead of
  treating every failure as fatal (the *silence* is the problem, not the branch).
 - `SetEnvPrefix("RESOLVESPEC")` + `SetEnvKeyReplacer(".", "_")` + `AutomaticEnv`
  (`manager.go:38-41`) is the correct trio for env overrides, and because every key has a
  registered default, `AutomaticEnv` actually resolves nested keys — so secrets *can* be supplied
  via env instead of the file. That's the mitigation for finding 4, and it should be documented as
  the only supported way to pass secrets.
 - The defaults table is comprehensive and one place — easy to review, which is how findings 3 and
  12 were found.
 - Test coverage is reasonable for a config package (608 LOC of tests against 1023 of source),
  though it does not cover concurrency, `SaveConfig` permissions, or `PathsConfig.Set`.
 ## Suggested follow-up
 1. Lock `Manager` or make config immutable after load (findings 1, 2). Until then, treat
   `Manager.Set` as unsafe to call after startup and consider removing it from the public API.
 2. Flip the insecure defaults and add `Config.Validate()` (findings 3, 12).
 3. `SetConfigPermissions(0o600)` and secret-stripping in `SaveConfig` (finding 4).
 4. Reorder the config search path and log the resolved file (findings 5, 6).
 5. Delete the dead `Unmarshal` in `SetConfig` (finding 10).
@@ -0,0 +1,220 @@
 # Audit — `pkg/errortracking`
 - **Date:** 2026-09-29
 - **Scope:** `pkg/errortracking/{interfaces,noop,sentry,factory}.go` (260 LOC, 4 source files + 1 test file, 67 LOC)
 - **Axes:** thread locking/waiting · slowness · security · panic handling & logging
 - **Threat model:** hostile internet client; error messages and `extra` maps may contain attacker-shaped content.
 ## Summary
 Small, clean abstraction: a `Provider` interface, a no-op implementation, a Sentry implementation,
 and a config-driven factory. The concurrency story is fine — `sentry.Hub` is internally
 mutex-guarded and the provider holds no mutable state of its own. The real exposure is **what
 this package sends out of the trust boundary**: it is the egress point for every `Warn`/`Error`
 in the codebase (see `audit/pkg/logger.audit.md` findings 2 and 3) and it applies **no scrubbing
 whatsoever**.
 | # | Severity | Axis | Finding |
 |---|----------|------|---------|
 | 1 | **High** | Security | No `BeforeSend` scrubber — messages, stack traces and `extra` leave the trust boundary verbatim |
 | 2 | Medium | Security | `sentry.Init` mutates process-global state; `NewSentryProvider` can be called repeatedly and silently replaces the global client |
 | 3 | Medium | Slowness | `Flush(timeout int)` is second-granularity only; combined with `Close()` gives up to 7 s of shutdown stall |
 | 4 | Medium | Slowness | `CapturePanic` stringifies the whole stack trace into an `extra` field on every panic |
 | 5 | Low | Security | `AttachStacktrace: true` is hardcoded — source paths and function names of the deployment leak to the SaaS |
 | 6 | Low | Correctness | `CaptureError` produces an `Exception` with a nil `Stacktrace` for plain `errors.New` values |
 | 7 | Low | Correctness | Config-provided `SampleRate == 0` silently means "send everything", not "send nothing" |
 | 8 | Low | Architecture | `factory.go` imports `pkg/config`, coupling the lowest-level package to the config layer |
 ---
 ## Findings
 ### 1. No scrubbing before egress (High, Security)
 `sentry.go:29-42`
 ```go
 err := sentry.Init(sentry.ClientOptions{
 	Dsn:              config.DSN,
 	Environment:      config.Environment,
 	Release:          config.Release,
 	Debug:            config.Debug,
 	AttachStacktrace: true,
 	SampleRate:       config.SampleRate,
 	TracesSampleRate: config.TracesSampleRate,
 })
 ```
 `BeforeSend` is not set. Neither is `BeforeSendTransaction`. Nothing in `CaptureError`
 (`sentry.go:46`), `CaptureMessage` (`sentry.go:75`) or `CapturePanic` (`sentry.go:97`) inspects or
 redacts its inputs; all three copy straight into `event.Message` / `event.Exception.Value` /
 `event.Contexts["extra"]` and hand it to `hub.CaptureEvent`.
 Because `pkg/logger.Error`/`Warn` forward every formatted message here unconditionally, the set of
 things that can reach Sentry is "every error string produced anywhere in ResolveSpec". In this
 codebase that includes driver errors (which embed DSNs and sometimes credentials on connect
 failure), SQL fragments with bound values, and identifiers taken from request headers.
 Under the hostile-client threat model this is an **attacker-reachable exfiltration channel**: shape
 an input that lands in an error message, and its content is written to a third-party system
 outside the operator's control.
 **Recommendation:** set `BeforeSend` to run a redaction pass over `Message`,
 `Exception[].Value` and `Contexts` — at minimum strip `password=`, `://user:pass@`, `Bearer `,
 and anything matching the configured DSN patterns. Consider an `extra`-key allowlist rather than
 passing the caller's map through (`sentry.go:70`, `92`, `114-121`).
 ### 2. `sentry.Init` mutates process-global state (Medium, Security/Correctness)
 `sentry.go:29` calls the package-level `sentry.Init`, which installs a global client, and
 `sentry.go:40` then captures `sentry.CurrentHub()`. Consequences:
 - Calling `NewSentryProvider` twice (two `NewProviderFromConfig` calls, or a config reload)
  replaces the global client. Any previously-created `SentryProvider` keeps a `hub` pointer whose
  client has been swapped underneath it — events start going to the *new* DSN. If the two configs
  have different environments or DSNs, events are misrouted with no error.
 - Events enqueued on the old client at swap time may be dropped without flush.
 - It means this "provider" abstraction is a lie: you cannot actually have two Sentry providers
  with different configs in one process.
 **Recommendation:** build a dedicated client with `sentry.NewClient(opts)` and bind it to an
 owned `sentry.NewHub(client, scope)` rather than touching the global. That also makes `Close()`
 able to genuinely release resources.
 ### 3. Coarse, additive shutdown flush (Medium, Slowness)
 `sentry.go:125-128`
 ```go
 func (s *SentryProvider) Flush(timeout int) bool {
 	return sentry.Flush(time.Duration(timeout) * time.Second)
 }
 ```
 `timeout` is an `int` interpreted as whole seconds — the interface (`interfaces.go:30`) cannot
 express 500 ms. `Close()` (`sentry.go:131-134`) then runs a *second* `sentry.Flush(2s)`.
 `pkg/logger.CloseErrorTracking` (`logger.go:69-75`) calls `Flush(5)` then `Close()`, so a graceful
 shutdown blocks for **up to 7 seconds** in this package alone, before the HTTP drain and DB close
 budgets in `pkg/server`. If the Sentry endpoint is unreachable (the common case during an
 outage — which is when you are restarting) both flushes run to full timeout.
 Note `Flush` also flushes the *global* client, not `s.hub`'s, which is the same object today only
 because of finding 2.
 **Recommendation:** change the interface to `Flush(context.Context) bool` or
 `Flush(time.Duration) bool`; have `Close` not re-flush; and pass the server's shutdown deadline
 through instead of hardcoding 5.
 ### 4. Whole stack trace stringified into `extra` on every panic (Medium, Slowness)
 `sentry.go:117-119`
 ```go
 if stackTrace != nil {
 	extraCtx["stack_trace"] = string(stackTrace)
 }
 ```
 The caller (`pkg/logger.CatchPanicCallback`, `HandlePanic`) already produced the trace via
 `debug.Stack()`. Here it is copied again into a string and shipped as a context field. Per
 recovered panic that's two full copies of a multi-kilobyte trace plus a network event. With
 panics recovered rather than fatal on the request path, a reliably-panicking input is a cheap
 amplification primitive (see `audit/pkg/logger.audit.md` finding 5).
 Sentry also truncates large context values server-side, so much of this payload is wasted.
 **Recommendation:** put the trace in `Exception[0].Stacktrace` as structured frames (which Sentry
 groups and displays properly) rather than a blob in `extra`, and cap the byte length.
 ### 5. `AttachStacktrace: true` hardcoded (Low, Security)
 `sentry.go:35`. Not configurable. Every event carries absolute source paths, package layout and
 function names of the build. That's mostly a reconnaissance leak to whoever can read the Sentry
 project rather than to the internet attacker, but it should be an operator choice, especially for
 on-prem deployments sending to a hosted DSN.
 ### 6. Nil stack trace for plain errors (Low, Correctness)
 `sentry.go:62`
 ```go
 Stacktrace: sentry.ExtractStacktrace(err),
 ```
 `ExtractStacktrace` only finds a trace if the error implements `StackTrace()`/`Callers()`
 (`pkg/errors`-style). Nearly all errors in this codebase come from `fmt.Errorf`, so this returns
 `nil` and the Sentry event has an exception with no frames — grouping falls back to the message
 string, which (because messages embed request-specific values) fragments what should be one issue
 into thousands.
 **Recommendation:** fall back to `sentry.NewStacktrace()` when extraction yields nil, and set an
 explicit `event.Fingerprint` derived from a stable prefix rather than the full message.
 ### 7. `SampleRate == 0` means "send everything" (Low, Correctness)
 `factory.go:20-27` passes `cfg.SampleRate` through untouched, and `pkg/config/manager.go`
 registers **no default** for `error_tracking.sample_rate`. So an operator who leaves it out gets
 `0.0`, and `sentry-go@v0.46.2` `client.go:339-341` rewrites `0.0` → `1.0`.
 Verified in the module cache:
 ```go
 if options.SampleRate == 0.0 {
 	options.SampleRate = 1.0
 }
 ```
 Fail-open rather than fail-closed, which is arguably the right choice for an error tracker — but
 it means an operator who *intends* to disable sampling by setting `0` gets the opposite, silently.
 **Recommendation:** make `SampleRate` a `*float64` in the config struct, or register an explicit
 default in `setDefaults`, and validate/log the effective value at init.
 ### 8. `factory.go` imports `pkg/config` (Low, Architecture)
 `factory.go:6` — `errortracking` is imported by `pkg/logger`, which is imported by essentially
 everything. Pulling `pkg/config` (and therefore `viper`) into that dependency chain means the
 lowest-level logging path transitively depends on the configuration layer. It works today only
 because `pkg/config` imports nothing from ResolveSpec; the first time it wants to log, there is
 an import cycle.
 **Recommendation:** move `NewProviderFromConfig` into `pkg/config`-adjacent wiring code (or take
 a small local options struct instead of `config.ErrorTrackingConfig`) so `errortracking` stays a
 leaf.
 ---
 ## What looks right
 - **Concurrency is genuinely fine.** `SentryProvider` holds only an immutable `*sentry.Hub`;
  `sentry.Hub` guards its own state with a mutex, and `CaptureEvent` hands off to a background
  worker with a bounded queue, so it does not block the caller and does not need a lock here.
 - `GetHubFromContext(ctx)` with fallback to `s.hub` (`sentry.go:53-56`, `81-84`, `103-106`) is the
  correct Sentry idiom and preserves per-request scope when middleware installs a hub.
 - Nil-input guards on all three capture methods (`sentry.go:47`, `76`, `98`) — a nil error, empty
  message or nil recovered value is dropped rather than producing a junk event.
 - `event.Contexts` is safe to index: `sentry.NewEvent()` initialises the map, so
  `event.Contexts["extra"] = ...` cannot nil-panic.
 - `NoOpProvider` means a disabled tracker is always safe to call — no nil checks needed at call
  sites beyond the one in `pkg/logger`.
 - `factory.go:15-17` correctly refuses to start with `provider: sentry` and an empty DSN rather
  than silently no-oping.
 ## Panic handling
 The package neither panics nor recovers, which is correct for its role — it is the *sink* for
 panic reports, not a place that should be generating them. The nil-guards in finding "what looks
 right" cover the realistic nil-deref paths. One residual: `CapturePanic` ranges over `extra`
 (`sentry.go:115`) without a nil check, which is safe in Go (ranging a nil map yields zero
 iterations) — noted only to confirm it was checked.
 ## Suggested follow-up
 1. Add `BeforeSend` redaction (finding 1). This is the highest-value single change in the package.
 2. Stop using the global Sentry client (finding 2) — unblocks real multi-provider support and a
   meaningful `Close()`.
 3. Widen `Flush` to a duration/context (finding 3) and wire it to the server shutdown budget.
 4. Add tests for the Sentry path. The existing test file covers only `NoOpProvider`, severity
   string mapping and interface satisfaction — `SentryProvider`'s capture methods have no
   coverage at all. `sentry-go` ships a test transport that makes this straightforward.
@@ -0,0 +1,269 @@
 # Audit — `pkg/logger`
 - **Date:** 2026-09-29
 - **Scope:** `pkg/logger/logger.go` (211 LOC, 1 file, no tests)
 - **Axes:** thread locking/waiting · slowness · security · panic handling & logging
 - **Threat model:** hostile internet client; request bodies, headers, params and identifiers are attacker-controlled.
 ## Summary
 `pkg/logger` is a thin package-global wrapper over `zap.SugaredLogger` plus a fan-out to
 `pkg/errortracking`. It is the single most widely imported package in the repo, so its defects
 are systemic. Two classes of problem dominate: **unsynchronised global mutable state** (a real
 data race between logger re-initialisation and request-path logging), and **unbounded,
 unsampled, unscrubbed egress of formatted messages to a third-party error tracker** on every
 `Warn`/`Error` call — which under hostile input is both a data-leak and a cost/latency
 amplification channel.
 There are **zero tests** in this package.
 | # | Severity | Axis | Finding |
 |---|----------|------|---------|
 | 1 | **High** | Locking | Unsynchronised writes to `Logger` / `errorTracker` globals race with every log call |
 | 2 | **High** | Security | Every `Warn`/`Error` message is shipped verbatim to Sentry — no scrubbing, no allowlist |
 | 3 | **High** | Slowness | No rate limit, sampling or dedup on error-tracker fan-out; attacker-triggerable |
 | 4 | **High** | Panic | `CatchPanic` swallows panics unconditionally — and both call sites are security enforcement functions (fail-open) |
 | 5 | Medium | Slowness | `debug.Stack()` + full stack stringification on every recovered panic |
 | 6 | Medium | Security | `log.Printf(template, args...)` fallback is a format-string sink for caller-supplied text |
 | 7 | Medium | Security | No CRLF/control-char sanitisation on the stdlib fallback path → log injection |
 | 8 | Medium | Correctness | `Info`/`Debug` do not strip `context.Context` args; `Warn`/`Error` do |
 | 9 | Low | Correctness | `UpdateLogger` leaks the previous zap logger / file descriptor |
 | 10 | Low | Correctness | No `Sync()` exported → buffered log lines lost on exit |
 | 11 | Low | Slowness | `os.Getpid()` called on every log line |
 | 12 | Low | Observability | `UpdateLogger` build failure degrades silently to stdlib `log` |
 ---
 ## Findings
 ### 1. Unsynchronised global mutable state — data race (High, Locking)
 `logger.go:14-15`
 ```go
 var Logger *zap.SugaredLogger
 var errorTracker errortracking.Provider
 ```
 `Logger` is written by `Init` → `UpdateLogger` (`logger.go:51`) and by `UpdateLoggerPath`
 (`logger.go:29`). `errorTracker` is written by `InitErrorTracking` (`logger.go:57`) and read by
 `GetErrorTracker`, `CloseErrorTracking`, `Warn`, `Error`, `CatchPanicCallback`, `HandlePanic`.
 Every read site (`logger.go:100`, `108`, `123`, `139`, `156`, `199`) is unguarded. There is no
 mutex, no `atomic.Value`, no `sync.Once`.
 - **Benign case:** everything is initialised once in `main` before goroutines start. Then it's fine.
 - **Real case:** `UpdateLoggerPath` is an exported, runtime-callable API. A config reload, a
  log-rotation hook, or a test helper calling it while HTTP handlers log concurrently is an
  unsynchronised write to an interface value and a pointer, concurrent with reads. Under the Go
  memory model this is undefined behaviour; in practice a torn interface read (type word from the
  new value, data word from the old) faults.
 - `CloseErrorTracking` (`logger.go:69`) does a read-check-then-use on `errorTracker` with no
  guard, so a concurrent `InitErrorTracking(nil)` yields a nil-interface dereference inside
  `Flush`.
 **Recommendation:** store both behind `atomic.Pointer`/`atomic.Value` (or an `sync.RWMutex`),
 and gate first-time init behind `sync.Once`. Run the test suite with `-race` — see finding 12 of
 `audit/pkg/config.audit.md` for the same pattern in the config singleton.
 ### 2. Unscrubbed message egress to third-party error tracker (High, Security)
 `logger.go:110-118` and `logger.go:126-134`
 ```go
 message := fmt.Sprintf(template, remainingArgs...)
 ...
 errorTracker.CaptureMessage(ctx, message, errortracking.SeverityError, ...)
 ```
 *Every* `Warn` and `Error` call in the entire codebase has its fully-formatted message sent to
 the configured provider (Sentry, in practice). There is no allowlist, no redaction hook, and
 `pkg/errortracking/sentry.go` configures no `BeforeSend` scrubber.
 Concretely, formatted error strings across `pkg/` embed: SQL fragments and bound values, DB
 connection strings, schema/table/column identifiers, filter expressions built from request
 input, and raw request bodies in a few handlers. Under the hostile-client threat model this is
 two problems at once:
 - **Outbound data leak:** secrets that appear in wrapped driver errors (DSNs, credentials from
  `pq`/`pgx` connect failures) leave the trust boundary to a SaaS endpoint.
 - **Attacker-controlled exfil channel:** an attacker who can shape a value that ends up in an
  error message gets that value written to a third-party system — useful for exfiltrating data
  read out of the DB via an induced error.
 **Recommendation:** add a redaction step before `CaptureMessage`/`CapturePanic` (regex-strip
 DSN/`password=`/bearer-token shapes at minimum), and set Sentry's `BeforeSend` as a second
 layer. Prefer passing structured fields with an explicit allowlist over shipping the rendered
 string.
 ### 3. No rate limiting or sampling on error-tracker fan-out (High, Slowness)
 `logger.go:113`, `logger.go:129`
 An unauthenticated request that reliably produces one `Error` log (a malformed filter, an unknown
 column, a bad JSON body — all of which the spec handlers log at error level) becomes one Sentry
 event. At even modest request rates this means:
 - Sentry quota burn → a direct billing-DoS.
 - `sentry-go` enqueues onto a bounded worker queue; once saturated events are dropped, so the
  *real* errors are the ones lost.
 - `pkg/errortracking/sentry.go:34` passes `SampleRate` straight through from config, and
  `config/manager.go` sets **no default** for it. `sentry-go@v0.46.2` `client.go:339` maps
  `SampleRate == 0.0` → `1.0`, so the out-of-the-box behaviour is *send 100% of events*.
 **Recommendation:** default `error_tracking.sample_rate` to something < 1.0 for the message path,
 and put a token-bucket or a fingerprint-dedup in front of `CaptureMessage`. Keep panics at 100%.
 ### 4. `CatchPanic` swallows panics unconditionally, fail-open at both call sites (High, Panic handling)
 `logger.go:145-176`
 ```go
 func CatchPanicCallback(location string, cb func(err any), args ...interface{}) func() {
 	...
 	if err := recover(); err != nil { ... if cb != nil { cb(err) } }
 }
 ```
 The recovered value is logged and then discarded. There is no variant that logs-and-re-panics
 and no way for the caller to signal "this panic means state is corrupt, take the process down".
 This is the right default for an HTTP handler boundary. The two current call sites are **not**
 handler boundaries:
 - `pkg/security/provider.go:302` — `defer logger.CatchPanic("ApplyColumnSecurity")()`
 - `pkg/security/provider.go:443` — `defer logger.CatchPanic("GetRowSecurityTemplate")()`
 Both are *security enforcement* functions. Swallowing a panic there means the column-security
 filter or row-security template silently does not get applied, and the caller — which has no way
 to learn a panic occurred, since `CatchPanic` returns nothing and sets no error — proceeds as if
 security was applied. That is a fail-open security control; see
 `audit/pkg/security.audit.md` for the full write-up of those two sites.
 Separately: a panic while a mutex is held does not release that mutex unless an intervening
 `defer Unlock` exists, so swallowing converts a crash into a permanent deadlock at any
 lock-holding call site.
 **Recommendation:** add `CatchPanicRethrow(location string)` for internal use and reserve the
 swallowing form for the outermost request/goroutine boundary. Document which is which.
 ### 5. Full stack capture on every recovered panic (Medium, Slowness)
 `logger.go:158` and `logger.go:197`
 ```go
 callstack := debug.Stack()
 ```
 `debug.Stack()` stops the world briefly and allocates; `HandlePanic` then formats the whole trace
 into a string *and* ships it to Sentry. Because panics on the request path are recovered rather
 than fatal (finding 4), an attacker who finds one reliably-panicking input turns each request
 into a stack capture + string build + network event. That is a solid amplification factor over a
 normal request.
 **Recommendation:** cap the captured stack (`runtime.Stack` into a fixed 8–16 KiB buffer rather
 than `debug.Stack()`'s grow-until-it-fits loop), and rate-limit identical panic fingerprints.
 ### 6. Format-string sink in the stdlib fallback (Medium, Security)
 `logger.go:100`, `logger.go:142` (and `108`/`123` with `"%s"`, correctly)
 ```go
 func Info(template string, args ...interface{}) {
 	if Logger == nil {
 		log.Printf(template, args...)   // template is the caller's, args may be empty
 ```
 `Info` and `Debug` pass `template` directly to `log.Printf`. If any caller ever does
 `logger.Info(someUserString)` — the idiomatic-looking single-argument call — a `%s` or `%n` in
 that string is interpreted as a verb, producing `%!s(MISSING)` garbage and mangled logs. Note
 `Warn`/`Error` already avoid this on the fallback path by using `log.Printf("%s", message)`;
 `Info`/`Debug` do not.
 A grep of `pkg/` found **no** current single-argument call sites, so this is a latent API footgun
 rather than a live bug — but it is one that costs one line to close.
 **Recommendation:** mirror `Warn`'s shape: format first, then `log.Printf("%s", message)`.
 `govet` runs by default under golangci-lint v2's standard set, and its `printf` analyser infers
 wrappers like these — so once the fallback is fixed, call sites are checked at build time for
 free. (Note `gosec` is *not* in `.golangci.json`'s `linters.enable` list; it appears only in the
 exclusion rules. Worth enabling repo-wide.)
 ### 7. No log-injection sanitisation on the fallback path (Medium, Security)
 On the zap path, the JSON encoder escapes newlines and control characters, so injected content
 can't forge a log record. On the `Logger == nil` fallback path, `log.Printf` writes raw bytes: a
 value containing `\n2026-09-29 ... level=info authorized=true` forges a plausible second log
 line. Combined with finding 12 (silent degradation to the fallback path) this is reachable
 without the operator noticing the encoder changed.
 **Recommendation:** strip/escape `\r`, `\n` and other C0 control characters from formatted
 messages before the stdlib write.
 ### 8. `Info`/`Debug` don't strip `context.Context` arguments (Medium, Correctness)
 `extractContext` (`logger.go:79-98`) exists precisely so callers can pass a `ctx` as a trailing
 variadic arg. `Warn` (`logger.go:106`) and `Error` (`logger.go:121`) call it. `Info`
 (`logger.go:99`) and `Debug` (`logger.go:137`) **do not** — they pass every arg to `Sprintf`.
 So `logger.Info("saved %s", name, ctx)` renders as
 `saved widget%!(EXTRA *context.valueCtx=context.Background...)`, dumping the context's contents
 (which in this codebase carry auth/tenant values) into the log line. That is both noise and a
 minor disclosure.
 **Recommendation:** call `extractContext` in all four level functions for uniform behaviour.
 ### 9. `UpdateLogger` leaks the previous logger (Low)
 `logger.go:37-53` builds a new zap logger and overwrites `Logger` without calling `Sync()`/close
 on the old one. `UpdateLoggerPath` opens a new file sink each call; repeated calls leak a file
 descriptor each time and buffered lines in the old logger are lost.
 ### 10. No `Sync()` on shutdown (Low)
 Nothing in the package exposes `Logger.Sync()`, and `CloseErrorTracking` (`logger.go:69`) flushes
 only the error tracker. zap buffers writes to file sinks, so the last lines before exit — often
 the interesting ones — are dropped. Add `func Sync() error` and call it from the server's
 shutdown path alongside `CloseErrorTracking`.
 ### 11. `os.Getpid()` per log line (Low, Slowness)
 `logger.go:102`, `111`, `127`, `140`, `165`, `202`. On Linux `getpid` is cached by the runtime so
 this is cheap, but the PID cannot change for the life of the process — cache it in a package var
 and drop six calls from the hot path.
 ### 12. Silent degradation when the logger fails to build (Low, Observability)
 `logger.go:45-49`
 ```go
 logger, err := config.Build()
 if err != nil { log.Print(err); return }
 ```
 `Logger` stays `nil`, so the whole process silently falls back to unstructured stdlib logging
 (and thereby onto the format-string and log-injection paths of findings 6 and 7) with a single
 line of warning that itself goes to stderr. A bad `logger.path` in config (unwritable directory)
 triggers exactly this.
 **Recommendation:** return the error from `Init`/`UpdateLogger` and let the caller decide whether
 to fail startup.
 ---
 ## What looks right
 - `extractContext` correctly ignores second and subsequent contexts rather than fighting over them.
 - `Warn`/`Error` use `log.Printf("%s", message)` on the fallback path — the safe form.
 - `HandlePanic` returns an `error` rather than swallowing, which lets callers convert a panic into
  a normal error return. This is the better of the two panic idioms in the package.
 - The `errortracking.Provider` indirection means a nil/noop provider is always safe to call.
 ## Suggested follow-up
 1. Guard the two globals (finding 1) — prerequisite for running the suite under `-race`.
 2. Add redaction + sampling in front of the error-tracker fan-out (findings 2, 3).
 3. Split `CatchPanic` into swallow/rethrow variants and re-audit the ~60 `recover()` sites
   listed in the other package audits against the split (finding 4).
 4. Add a test file. Minimum: concurrent `UpdateLogger` + `Error` under `-race`, `Info` with a
   `%`-bearing message, and nil-provider paths.
@@ -0,0 +1,474 @@
 # Audit — `pkg/modelregistry`
 - **Date:** 2026-09-29
 - **Scope:** `pkg/modelregistry/model_registry.go` (381 LOC, 1 file, **no tests**)
 - **Axes:** thread locking/waiting · slowness · security · panic handling & logging
 - **Threat model:** hostile internet client. This package holds the `ModelRules` that
  `pkg/security/hooks.go` consults to authorise read/update/create/delete, so it is **on the
  authorisation path**.
 ## Summary
 This package is the highest-risk find in the audit. It has been deliberately reworked to "never
 hang" by replacing blocking `Lock`/`RLock` with **bounded `TryLock` retry loops that give up and
 return a wrong answer** — and because those wrong answers are consumed by
 `pkg/security/hooks.go` as authorisation decisions, the result is an **authorisation control that
 fails open under lock contention**.
 The comments in the file are explicit about the trade-off ("falls back to the last known value
 without synchronization", "the call is a no-op") — so the hazard was known at the time of writing.
 What appears not to have been traced is where those degraded results end up. They end up in
 `checkModelUpdateAllowed` / `checkModelDeleteAllowed`, which treat any error as *permit*.
 There are **no tests** in this package and no `-race` coverage of it anywhere.
 | # | Severity | Axis | Finding |
 |---|----------|------|---------|
 | 1 | **Critical** | Security + Locking | `GetModel`'s "registry locked" error is consumed by `pkg/security/hooks.go` as *allow by default* → authorisation fails open under write-lock contention |
 | 2 | **High** | Locking | `GetDefaultRegistry` documents and performs an unsynchronised read of `defaultRegistry` on lock-acquire failure — a data race by design |
 | 3 | **High** | Locking | `SetDefaultRegistry` silently no-ops after ~20 ms of contention; caller gets no error |
 | 4 | **High** | Security | `RegisterModelWithRules` is non-atomic: the model is visible with permissive `DefaultModelRules` before its real rules are applied (TOCTOU) |
 | 5 | Medium | Correctness | `GetAllModels` returns an empty map, and `GetModels` silently skips whole registries, on lock-acquire failure |
 | 6 | Medium | Locking | `IterateModels` invokes the caller's callback while holding `RLock` → guaranteed self-deadlock if the callback touches the registry |
 | 7 | Medium | Slowness | `time.Sleep(1ms)` spin loops add up to 20 ms of latency per call and defeat mutex fairness/hand-off |
 | 8 | Medium | Locking | `defaultRegistry` is read unsynchronised by six package-level functions while `SetDefaultRegistry` writes it under lock |
 | 9 | Medium | Locking | Inconsistent discipline: `SetModelRules`/`GetModelRules`/`AddRegistry`/`IterateModels` use blocking locks; the rest use try-locks |
 | 10 | Low | Slowness/Locking | Reflection (`TypeOf`, unwrap loop, `reflect.New`) runs while holding the registry **write** lock |
 | 11 | Low | Availability | Unbounded unwrap loop: a recursive pointer type (`type T *T`) spins forever holding the write lock (**verified**) |
 | 12 | Low | Panic | Package has no `recover` anywhere, and calls a caller-supplied callback under a lock (see 6) |
 | 13 | Low | Security | `DefaultModelRules()` grants `CanRead/Update/Create/Delete: true` — registration without explicit rules is fully mutable |
 ---
 ## Findings
 ### 1. Authorisation fails open under lock contention (Critical, Security + Locking)
 The mechanism spans two packages.
 **Here**, `GetModel` conflates "not found" with "could not lock" into a single `error` return
 (`model_registry.go:198-210`):
 ```go
 func (r *DefaultModelRegistry) GetModel(name string) (interface{}, error) {
 	if !r.tryRLock() {
 		return nil, fmt.Errorf("failed to get model %s: registry locked", name)
 	}
 	defer r.mutex.RUnlock()
 	model, exists := r.models[name]
 	if !exists {
 		return nil, fmt.Errorf("model %s not found", name)
 	}
 	return model, nil
 }
 ```
 `GetModelRulesByName` (`model_registry.go:364-376`) uses `GetModel` as its existence probe:
 ```go
 for _, registry := range registries {
 	if _, err := registry.GetModel(name); err == nil {
 		return registry.GetModelRules(name)
 	}
 }
 return ModelRules{}, fmt.Errorf("model %s not found in any registry", name)
 ```
 So a `tryRLock` failure makes the registry look like it does not contain the model.
 **In `pkg/security/hooks.go`**, that outcome is interpreted as *permit*
 (`pkg/security/hooks.go:274-294`, and identically at `:298-318`):
 ```go
 func checkModelUpdateAllowed(secCtx SecurityContext) error {
 	rules, ok := GetModelRulesFromContext(secCtx.GetContext())
 	if !ok {
 		schema := secCtx.GetSchema()
 		entity := secCtx.GetEntity()
 		var err error
 		if schema != "" {
 			rules, err = modelregistry.GetModelRulesByName(fmt.Sprintf("%s.%s", schema, entity))
 		}
 		if err != nil || schema == "" {
 			rules, err = modelregistry.GetModelRulesByName(entity)
 		}
 		if err != nil {
 			return nil // model not registered, allow by default
 		}
 	}
 	if !rules.CanUpdate {
 		return fmt.Errorf("update not allowed for %s", secCtx.GetEntity())
 	}
 	return nil
 }
 ```
 Note the context fast-path at `hooks.go:275`: if `NewModelAuthMiddleware` already put rules in the
 context, the registry is not consulted and this bug does not fire. The registry fallback runs
 whenever that middleware is absent or did not resolve rules — so the blast radius depends on
 deployment wiring. `audit/pkg/security.audit.md` covers whether that middleware is mandatory.
 `return nil` from `checkModelUpdateAllowed` means **the update is authorised**. Same for
 `checkModelDeleteAllowed`. `GetModelRules(name)` for the "found" path also uses a blocking
 `RLock` (`model_registry.go:253`) — so the two calls in `GetModelRulesByName` don't even use the
 same locking discipline.
 **Failure scenario.** A model `public.employees` is registered with `CanDelete: false`. A
 concurrent `RegisterModel` (or `SetModelRules`, or `RegisterModelWithRules`) holds the write lock
 for longer than `lockRetryAttempts * lockRetryDelay` = 20 ms — which is entirely achievable given
 finding 10 (reflection under the write lock) and finding 7 (each waiter sleeps in 1 ms
 increments, so N waiters serialise). During that window every `DELETE` request against
 `public.employees` has `GetModelRulesByName` return an error, `checkModelDeleteAllowed` return
 `nil`, and the delete proceeds. The model's `CanDelete: false` is not enforced.
 This is remotely triggerable if any request path can cause a model registration or a rules
 update; even without that, it is a straightforward race that will fire under load.
 **Recommendation, in order of value:**
 1. Make the security layer **fail closed**: distinguish a sentinel `ErrModelNotFound` from any
   other error, and only allow-by-default on `ErrModelNotFound`. Any other error must deny.
 2. Delete the try-lock scheme here entirely and use plain `RLock`/`Lock` (see finding 2 for why
   the scheme does not achieve its stated goal anyway).
 3. Separate the existence probe from the rules fetch so `GetModelRulesByName` takes each registry's
   lock once and returns a typed "found / not found / unavailable" result.
 ### 2. `GetDefaultRegistry` races by design (High, Locking)
 `model_registry.go:71-84`
 ```go
 // GetDefaultRegistry returns the current default registry. It uses a
 // bounded TryRLock instead of a blocking RLock so it can never hang;
 // if the lock can't be acquired in time it falls back to the last known
 // value without synchronization.
 func GetDefaultRegistry() *DefaultModelRegistry {
 	for i := 0; i < lockRetryAttempts; i++ {
 		if registriesMutex.TryRLock() {
 			defer registriesMutex.RUnlock()
 			return defaultRegistry
 		}
 		time.Sleep(lockRetryDelay)
 	}
 	return defaultRegistry
 }
 ```
 The `return defaultRegistry` on line 83 reads a pointer that `SetDefaultRegistry`
 (`model_registry.go:89-116`) writes under the write lock. The only time this path is taken is
 precisely when a writer holds or is contending for the lock — i.e. the fallback executes
 *exactly* in the window where the race is live. The trade is not "hang vs. slightly stale value";
 it is "block for 20 ms vs. data race", and a torn/`nil` pointer read here means a nil-pointer
 dereference in the caller.
 The premise is also wrong: a `sync.RWMutex.RLock` that is only ever held for a map lookup cannot
 "hang". The hang this was written to avoid must have had a different root cause — most likely
 finding 6 (self-deadlock through `IterateModels`) or a lock-ordering inversion — and the try-lock
 scheme papers over it rather than fixing it.
 **Recommendation:** revert to `RLock`/`RUnlock`. If a real hang was observed, reproduce it under
 `-race` and `GODEBUG=gctrace`/`SIGQUIT` stack dump; the fix belongs at the deadlock, not here.
 ### 3. `SetDefaultRegistry` silently no-ops (High, Locking)
 `model_registry.go:90-100`
 ```go
 acquired := false
 for i := 0; i < lockRetryAttempts; i++ {
 	if registriesMutex.TryLock() { acquired = true; break }
 	time.Sleep(lockRetryDelay)
 }
 if !acquired {
 	return
 }
 ```
 The function returns no error. A caller that swaps in a registry — plausibly one with *restrictive*
 `ModelRules* — has no way to learn the swap did not happen, and continues believing the new
 registry is in effect. Every subsequent authorisation check consults the old registry's rules.
 `GetModels` (`model_registry.go:319-329`) has the same shape and returns `nil`.
 **Recommendation:** return `error` from `SetDefaultRegistry`; or (better) use a blocking `Lock`,
 since this is a startup-time operation where blocking is correct.
 ### 4. `RegisterModelWithRules` is non-atomic (High, Security)
 `model_registry.go:270-282`
 ```go
 func (r *DefaultModelRegistry) RegisterModelWithRules(name string, model interface{}, rules ModelRules) error {
 	// First register the model
 	if err := r.RegisterModel(name, model); err != nil {
 		return err
 	}
 	// Then set the rules (we need to lock again for rules)
 	r.mutex.Lock()
 	defer r.mutex.Unlock()
 	r.rules[name] = rules
 	return nil
 }
 ```
 `RegisterModel` releases the write lock before returning, and it initialises the model's rules to
 `DefaultModelRules()` (`model_registry.go:191-194`) — which is **permissive**:
 `CanRead/CanUpdate/CanCreate/CanDelete` all `true`.
 Between the two lock acquisitions, any concurrent `GetModelRulesByName` sees the model registered
 with full read/update/create/delete permission, regardless of the restrictive `rules` the caller
 passed. The comment "we need to lock again for rules" acknowledges the re-lock without noticing
 the gap it opens.
 **Failure scenario.** `RegisterModelWithRules("public.audit_log", AuditLog{}, ModelRules{CanRead:
 true})` — intended read-only. A `DELETE /public.audit_log/...` that lands in the window is
 authorised because `rules.CanDelete` is `true` from the default.
 **Recommendation:** add an unexported `registerLocked(name, model, rules)` that writes both maps
 under one lock acquisition, and build both public constructors on it. Also change the default
 initialisation in `RegisterModel` to deny-by-default, or require rules at registration.
 ### 5. Degraded results indistinguishable from real results (Medium, Correctness)
 Three functions return a plausible-looking answer when they cannot lock:
 - `GetAllModels` (`model_registry.go:212-215`) — `return make(map[string]interface{})`, i.e. "the
  registry is empty".
 - `GetModels` (`model_registry.go:327-329`) — `return nil` on `registriesMutex` failure, and
  `model_registry.go:336-338` `continue`s past any individual registry it cannot read, returning a
  **partial** list with no indication of truncation.
 - `GetDefaultRegistry` — finding 2.
 Consumers of `GetModels`/`GetAllModels` (schema introspection, OpenAPI generation, migration
 helpers) will emit a document that is missing models, and there is no error to log. Note these two
 have no callers in `pkg/` today, which is the only reason this is Medium.
 **Recommendation:** return `(T, error)`; never manufacture an empty-but-valid result.
 ### 6. `IterateModels` calls a user callback under a read lock (Medium, Locking)
 `model_registry.go:307-314`
 ```go
 func IterateModels(fn func(name string, model interface{})) {
 	defaultRegistry.mutex.RLock()
 	defer defaultRegistry.mutex.RUnlock()
 	for name, model := range defaultRegistry.models {
 		fn(name, model)
 	}
 }
 ```
 `fn` is arbitrary caller code running with `defaultRegistry.mutex` read-held. `sync.RWMutex` is not
 reentrant, and once a writer is blocked on `Lock` it also blocks *new* readers. So:
 - `fn` calling `modelregistry.RegisterModel` / `SetModelRules` → `Lock` waits for the reader, which
  is the same goroutine. **Permanent self-deadlock.**
 - `fn` calling `GetModel` → `tryRLock` fails for 20 ms and returns "registry locked" for every
  model, which is silent nonsense rather than a deadlock (and feeds finding 1).
 - `fn` doing anything slow (I/O, a DB call) holds the registry read lock for that whole duration,
  blocking all registration and — via the blocked-writer rule — all other readers too.
 This is the most likely original cause of the "hang" the try-lock scheme was introduced to work
 around.
 **Recommendation:** snapshot under the lock, release, then iterate:
 ```go
 func IterateModels(fn func(name string, model interface{})) {
 	reg := GetDefaultRegistry()
 	snapshot := reg.GetAllModels()   // takes and releases the lock
 	for name, model := range snapshot {
 		fn(name, model)
 	}
 }
 ```
 ### 7. `time.Sleep` spin loops (Medium, Slowness)
 `tryLock` (`model_registry.go:128-136`), `tryRLock` (`:140-148`), and the inline loops in
 `GetDefaultRegistry`, `SetDefaultRegistry`, `GetModels`.
 ```go
 for i := 0; i < lockRetryAttempts; i++ {
 	if r.mutex.TryLock() { return true }
 	time.Sleep(lockRetryDelay)   // 1ms
 }
 ```
 Problems:
 - **Latency floor.** A contended call costs a multiple of 1 ms even if the lock frees after 10 µs,
  because the waiter is asleep. A blocking `Lock` would be handed the mutex in microseconds. So the
  "no-hang" scheme is *slower* in the common contended case, not faster.
 - **No fairness.** `sync.Mutex` has a starvation-avoidance mode that hands the lock to a waiter
  queued > 1 ms. `TryLock` participates in none of it, so a try-lock waiter can be starved
  indefinitely by a stream of blocking `Lock` callers (`SetModelRules`, `AddRegistry`,
  `IterateModels` all still block) — see finding 9.
 - **Timer churn.** 20 timer allocations per contended call.
 - Sleeping in a loop scales badly: 50 concurrent callers each sleep and wake 20 times, producing
  1000 needless scheduler round-trips for what a mutex does with one park/unpark.
 **Recommendation:** delete the try-lock helpers. If a bounded wait is genuinely required for an
 SLO, express it as `context`-aware acquisition (a buffered-channel semaphore with a `select` on
 `ctx.Done()`), which gives a real deadline *and* a real error — not a silent wrong answer.
 ### 8. `defaultRegistry` read without the guarding mutex (Medium, Locking)
 `SetDefaultRegistry` writes `defaultRegistry` (`model_registry.go:110`) under `registriesMutex`.
 These read it **without** taking that mutex:
 - `RegisterModel` (`model_registry.go:288`)
 - `IterateModels` (`model_registry.go:308`, `311`)
 - `SetModelRules` (`model_registry.go:354`)
 - `GetModelRules` (`model_registry.go:359`)
 - `GetDefaultRegistry`'s fallback (`model_registry.go:83`, finding 2)
 A data race on the pointer, and semantically these functions may operate on the *previous* default
 registry after a swap — so rules set through `SetModelRules` can land on a registry nobody consults
 any more.
 **Recommendation:** route every access through one accessor that takes the lock (and make
 `defaultRegistry` an `atomic.Pointer[DefaultModelRegistry]` if lock-free reads are wanted — that is
 the correct way to get the "never blocks" property finding 2 was reaching for).
 ### 9. Inconsistent locking discipline (Medium, Locking)
 Within one 381-line file:
 | Function | `registriesMutex` | `r.mutex` |
 |---|---|---|
 | `GetDefaultRegistry` | `TryRLock` + fallback | — |
 | `SetDefaultRegistry` | `TryLock`, no-op on fail | — |
 | `AddRegistry` (`:120`) | blocking `Lock` | — |
 | `GetModelByName` (`:293`) | blocking `RLock` | via `GetModel` → `TryRLock` |
 | `GetModelRulesByName` (`:365`) | blocking `RLock` | `TryRLock` then blocking `RLock` |
 | `GetModels` (`:318`) | `TryRLock`, nil on fail | `tryRLock`, skip on fail |
 | `RegisterModel` (`:150`) | — | `tryLock`, error on fail |
 | `GetModel` (`:198`) | — | `tryRLock`, error on fail |
 | `GetAllModels` (`:212`) | — | `tryRLock`, empty on fail |
 | `SetModelRules` (`:237`) | — | blocking `Lock` |
 | `GetModelRules` (`:252`) | — | blocking `RLock` |
 | `IterateModels` (`:307`) | — | blocking `RLock` |
 Four different failure behaviours for the same class of event. The mix also means the try-lock
 callers can be starved by the blocking ones (finding 7), so the functions that "can never hang" are
 the ones most likely to return garbage.
 **Recommendation:** pick one discipline — blocking locks with snapshot-and-release — and apply it
 uniformly.
 ### 10. Reflection under the write lock (Low, Slowness + Locking)
 `RegisterModel` holds `r.mutex` (write) from `model_registry.go:151` through `:195`, and inside
 that window does `reflect.TypeOf` (`:161`), the unwrap loop (`:169-171`), `reflect.New(...).Elem().Interface()`
 (`:181`), and another `reflect.TypeOf` (`:185`). None of that touches `r.models`/`r.rules` and none
 of it needs the lock.
 This directly lengthens the window that makes finding 1 exploitable. Validate first, then take the
 lock only for the two map writes.
 ### 11. Unbounded unwrap loop on a recursive pointer type (Low, Availability)
 `model_registry.go:169-171`
 ```go
 for modelType.Kind() == reflect.Pointer || modelType.Kind() == reflect.Slice || modelType.Kind() == reflect.Array {
 	modelType = modelType.Elem()
 }
 ```
 `type T *T` is legal Go, and `reflect.Type.Elem()` on it returns itself — so the loop never
 terminates. **Verified experimentally:**
 ```go
 type T *T
 var x T
 tt := reflect.TypeOf(x)            // main.T
 for tt.Kind() == reflect.Pointer { tt = tt.Elem() }   // spins on main.T forever
 // → "INFINITE LOOP CONFIRMED after 101 iterations, still main.T"
 ```
 Because the loop runs with the write lock held (finding 10), this doesn't just hang one goroutine —
 it wedges the registry permanently, at which point every try-lock caller starts returning
 "registry locked", which via finding 1 means **authorisation fails open for the rest of the process
 lifetime**.
 Requires a pathological model type, so exploitability is near zero; the fix is a one-line depth cap
 and it converts a permanent fail-open into an error return.
 **Recommendation:** bound the loop (`for depth := 0; depth < 16 && ...; depth++`) and return an
 error if the cap is hit.
 ### 12. No panic handling at all (Low, Panic handling)
 The package contains **zero** `recover()` calls and never logs — it does not import `pkg/logger`.
 For a pure data structure that is a defensible choice, with two caveats:
 - `IterateModels` runs a caller callback under a read lock (finding 6). If `fn` panics, the
  `defer RUnlock` does release the lock, so the registry is not wedged — that part is fine — but
  the panic propagates to whatever boundary handler exists, and nothing here records which model
  was being processed. A `logger`-free package can still name the model in a re-panic.
 - Every failure mode in the package is reported as a `fmt.Errorf` string with no wrapping and no
  sentinel values, so callers cannot distinguish them (finding 1). That is the panic/error-handling
  defect that actually matters here.
 **Recommendation:** define `ErrModelNotFound`, `ErrModelExists`, `ErrRegistryUnavailable` as
 sentinels and wrap them, so `errors.Is` works at the security layer.
 ### 13. Permissive default rules (Low, Security)
 `DefaultModelRules()` (`model_registry.go:24-36`) returns `CanRead`, `CanUpdate`, `CanCreate`,
 `CanDelete` all `true`. `RegisterModel` applies it to any model registered without explicit rules
 (`model_registry.go:191-194`), and `GetModelRules` falls back to it as well (`model_registry.go:266`).
 The `CanPublic*` flags default to `false` and `SecurityDisabled` to `false`, which is right. But the
 authenticated-path flags default open, so `RegisterModel(name, m)` — the form used by
 `pkg/testmodels/business.go` `RegisterTestModels` and the `modelregistry.RegisterModel` convenience wrapper —
 yields a fully mutable model. Combined with `pkg/security/hooks.go`'s allow-on-error, the system's
 default posture at every layer is permit.
 **Recommendation:** default to deny and make permissions opt-in, or at minimum log at registration
 time when a model is registered without explicit rules.
 ---
 ## What looks right
 - The struct-vs-pointer validation in `RegisterModel` (`model_registry.go:160-194`) is careful and
  well-reasoned: it rejects `nil`, unwraps pointer/slice/array to find the base type, rejects
  non-struct kinds with a message naming the original type, normalises a pointer/slice input to a
  zero struct value, and re-checks the final type. The error message even tells the caller to use
  `MyModel{}` instead of `&MyModel{}`. Good API ergonomics.
 - Duplicate registration is rejected (`model_registry.go:156-158`) rather than silently overwriting
  — important, since silent overwrite would be a rules-replacement primitive.
 - `GetAllModels` returns a **copy** of the map (`model_registry.go:218-222`) rather than the
  internal one, so callers cannot mutate registry state or race on it after the lock is dropped.
  This is the pattern the rest of the package should follow.
 - `GetModelByEntity` (`model_registry.go:225-234`) tries `schema.entity` before bare `entity`,
  which is the right precedence and matches what `pkg/security/hooks.go` does.
 - `GetModels` de-duplicates by name across registries (`model_registry.go:335-347`), so
  registry-order precedence is consistent with `GetModelByName`'s first-match rule.
 - Every `defer` for an acquired lock is correctly paired; there is no missing-`Unlock` path. The
  problems here are about *which* lock discipline was chosen, not about leaking locks.
 ## Suggested follow-up
 Ordered by risk:
 1. **Make `pkg/security/hooks.go` fail closed** (finding 1). This is the single change that
   converts a Critical authorisation bypass into a Medium availability issue. It does not require
   touching this package.
 2. **Remove the try-lock scheme** (findings 2, 3, 5, 7, 9) and fix the underlying hang by
   snapshotting in `IterateModels` (finding 6).
 3. **Make `RegisterModelWithRules` atomic** (finding 4).
 4. Route `defaultRegistry` access through a single locked accessor or `atomic.Pointer` (finding 8).
 5. Move reflection out of the write-locked region and cap the unwrap loop (findings 10, 11).
 6. **Add tests.** This package has none. Priority cases: `-race` test with concurrent
   `RegisterModel` + `GetModelRulesByName` asserting that rules are *never* observed as permissive
   for a restrictively-registered model; a test that `GetModelRulesByName` under contention does
   not return a "not found"-shaped error; `IterateModels` with a callback that calls back into the
   registry (should not deadlock); sentinel-error assertions.
@@ -0,0 +1,203 @@
 # Audit — `pkg/testmodels`
 - **Date:** 2026-09-29
 - **Scope:** `pkg/testmodels/business.go` (161 LOC, 1 file, **no tests**)
 - **Axes:** thread locking/waiting · slowness · security · panic handling & logging
 - **Threat model:** hostile internet client. These models are registered into
  `pkg/modelregistry`, which means any model here becomes a reachable entity for the spec handlers.
 ## Summary
 Six GORM struct definitions (`Department`, `Employee`, `Project`, `ProjectTask`, `Document`,
 `Comment`) used as fixtures, plus two registration helpers. No concurrency, no I/O, no panics, no
 logging — so three of the four audit axes are trivially clean.
 Two real issues: **all six registration errors are discarded**, and this fixture package ships in
 `pkg/` (not `_test.go`, not `internal/`) where a consuming application can register test tables
 into a production registry.
 | # | Severity | Axis | Finding |
 |---|----------|------|---------|
 | 1 | Medium | Correctness | `RegisterTestModels` discards all six `RegisterModel` error returns |
 | 2 | Medium | Security | Fixtures live in exported `pkg/`, registerable into a production model registry |
 | 3 | Low | Security | Models are registered via `RegisterModel`, which applies permissive `DefaultModelRules` |
 | 4 | Low | Correctness | `GetTestModels()` return order is unrelated to FK dependency order |
 | 5 | Low | Correctness | `Document.Path` is an unconstrained filesystem path exposed as a writable API field |
 ---
 ## Findings
 ### 1. All registration errors discarded (Medium, Correctness)
 `business.go:142-149`
 ```go
 func RegisterTestModels(registry *modelregistry.DefaultModelRegistry) {
 	registry.RegisterModel("departments", Department{})
 	registry.RegisterModel("employees", Employee{})
 	registry.RegisterModel("projects", Project{})
 	registry.RegisterModel("project_tasks", ProjectTask{})
 	registry.RegisterModel("documents", Document{})
 	registry.RegisterModel("comments", Comment{})
 }
 ```
 `RegisterModel` returns `error` and every return value is dropped. The function itself returns
 nothing, so a caller cannot detect failure either.
 This matters more than usual because of how `pkg/modelregistry.RegisterModel` fails. It has two
 error paths (`pkg/modelregistry/model_registry.go:151-158`):
 ```go
 if !r.tryLock() {
 	return fmt.Errorf("failed to register model %s: registry locked", name)
 }
 ...
 if _, exists := r.models[name]; exists {
 	return fmt.Errorf("model %s already registered", name)
 }
 ```
 The first is a **transient lock-contention failure** — see `audit/pkg/modelregistry.audit.md`
 finding 7, where a contended `tryLock` gives up after ~20 ms. So under concurrent registration, some
 subset of these six models silently fails to register, with no error, no log, and no panic. The
 process then runs with, say, `documents` and `comments` missing from the registry.
 That is not merely a missing-fixture annoyance. Per `audit/pkg/modelregistry.audit.md` finding 1, an
 unregistered model causes `pkg/security/hooks.go:274-294` to take the
 `return nil // model not registered, allow by default` branch — so a silently-failed registration
 turns into **authorisation fail-open** for that entity.
 Note `errcheck` is enabled (golangci-lint v2 standard set) but `.golangci.json` excludes
 `"tests?"` paths — `pkg/testmodels` does not match that pattern, so this *should* be flagged
 today. Worth checking whether the linter is actually run in CI.
 **Recommendation:** return `error`, and use `errors.Join` so a partial failure is reported in full:
 ```go
 func RegisterTestModels(registry *modelregistry.DefaultModelRegistry) error {
 	return errors.Join(
 		registry.RegisterModel("departments", Department{}),
 		registry.RegisterModel("employees", Employee{}),
 		...
 	)
 }
 ```
 ### 2. Fixtures are exported from `pkg/` (Medium, Security)
 The package path is `github.com/bitechdev/ResolveSpec/pkg/testmodels`, not a `_test.go` file and not
 under `internal/`. Consequences:
 - The six structs and both helpers are part of ResolveSpec's **public API surface**. They are
  compiled into every binary that imports anything which transitively imports this package.
 - A consuming application (or a copy-pasted quickstart) that calls
  `testmodels.RegisterTestModels(registry)` against its production registry makes
  `departments`, `employees`, `projects`, `project_tasks`, `documents` and `comments` live entities
  on the spec handlers, addressable by name. If the production database happens to have tables with
  those names — `documents` and `comments` are very common names — the handlers will happily
  read and write them under the permissive default rules of finding 3.
 - It also means any future model added here for test convenience automatically becomes reachable.
 Nothing in `pkg/` currently calls `RegisterTestModels` (only the test tree does), so this is a
 packaging hazard rather than a live exposure.
 **Recommendation:** move to `internal/testmodels` (blocks external import outright) or to a
 `testmodels_test` package / `testdata` helper. If it must stay importable for downstream tests,
 document loudly and consider a build tag.
 ### 3. Registered with permissive default rules (Low, Security)
 `RegisterTestModels` uses `RegisterModel`, not `RegisterModelWithRules`. Per
 `pkg/modelregistry/model_registry.go:191-194`, that initialises each model with
 `DefaultModelRules()`, which grants `CanRead`, `CanUpdate`, `CanCreate` and `CanDelete` — see
 `audit/pkg/modelregistry.audit.md` finding 13. `CanPublic*` are `false`, which is the saving grace.
 If finding 2 is acted on this becomes moot; if these models are intended to stay registerable, they
 should be registered read-only.
 ### 4. `GetTestModels()` order is not dependency order (Low, Correctness)
 `business.go:152-160` returns the models in declaration order:
 `Department, Employee, Project, ProjectTask, Document, Comment`.
 The FK graph is not satisfied by that order. `Employee.DepartmentID → Department.ID` happens to work,
 but `Document.OwnerID → Employee.ID` and `Document.ProjectID → Project.ID` mean `Document` must
 follow both, and `ProjectTask.AssigneeID → Employee.ID` and `ProjectTask.ProjectID → Project.ID`
 likewise. Coincidentally the declaration order does satisfy these — but nothing enforces it, and
 `Employee.ManagerID → Employee.ID` is self-referential, which several migration/auto-migrate paths
 handle only if the self-FK is deferred.
 Also the two `many2many` joins (`department_projects`, `employee_projects`, declared at
 `business.go:20`, `:45`, `:67-68`) are not in the returned list at all, so a caller using
 `GetTestModels()` to drive `AutoMigrate` gets the join tables only because GORM infers them from the
 tags — a Bun-based migration path (`pkg/common/adapters/database/bun.go`) would not.
 **Recommendation:** document that the order is migration-safe and add a comment stating the
 constraint, or return an explicitly ordered list with a test that asserts it.
 ### 5. `Document.Path` is an unconstrained path field (Low, Security)
 `business.go:107`
 ```go
 Path        string    `json:"path"`
 ```
 No validation, no length limit, no `gorm` constraint. As a plain string column it is inert — the
 risk only materialises if some handler or downstream consumer uses it to open a file, at which point
 an attacker who can `POST`/`PATCH` a `Document` controls a filesystem path (`../../etc/passwd`,
 `/proc/self/environ`). The same applies to `ContentType` (`business.go:105`) if it is ever echoed
 into a response header unvalidated, and `Size` (`business.go:106`) which is a client-settable
 `int64` that can disagree with reality.
 Nothing in `pkg/` reads these fields, so this is a note about the fixture's shape rather than a
 present vulnerability — but it is a bad example to ship, since fixtures get copied.
 **Recommendation:** if these stay, mark `Path` as server-set (a `gorm:"->"` read-only tag, or
 exclude it from the writable column set) so the fixture demonstrates the safe pattern.
 ---
 ## Axis-by-axis
 - **Thread locking / waiting:** nothing to report. The package declares no goroutines, channels,
  mutexes or atomics. Its only concurrency exposure is *through* `pkg/modelregistry`, covered in
  finding 1 and in that package's audit.
 - **Slowness:** nothing to report. `RegisterTestModels` and `GetTestModels` are O(1) with six
  elements and are startup-only. The `TableName()` methods (`business.go:23`, `:49`, `:73`, `:96`,
  `:119`, `:137`) return constants — no allocation, no reflection.
 - **Security:** findings 2, 3, 5 — all about packaging and field shape, none about code behaviour.
 - **Panic handling and logging:** the package contains no `panic`, no `recover`, and does not import
  `pkg/logger`. For plain struct definitions that is correct. The one place where logging *would*
  belong is the discarded errors of finding 1 — silently dropping six error returns is the
  panic/error-handling defect in this package, even though no panic is involved.
 ## What looks right
 - Struct tags are consistent and complete: `json` on every field, `gorm:"primaryKey"` on every ID,
  `gorm:"uniqueIndex"` on the natural keys (`Department.Code`, `Employee.Email`, `Project.Code`),
  and explicit `foreignKey`/`references` on every relation rather than relying on GORM's inference.
  That makes these fixtures genuinely useful for exercising the relation-expansion paths in
  `pkg/restheadspec` and `pkg/resolvespec`.
 - `omitempty` on every relation field prevents empty relation arrays from bloating responses — which
  matters, because these fixtures are what the handler tests measure payloads against.
 - Nullable FKs are correctly modelled as `*string` (`Employee.ManagerID` `business.go:35`,
  `Document.ProjectID` `business.go:109`) rather than empty-string sentinels.
 - The self-referential manager/reports pair (`business.go:43-44`) and the two `many2many` relations
  give reasonable coverage of the harder relation shapes — a genuinely well-chosen fixture set for
  the recursive-preload logic audited in `audit/pkg/restheadspec.audit.md`.
 - `TableName()` is defined on the value receiver for all six, so it works whether a value or a
  pointer is passed — which matters given `pkg/modelregistry.RegisterModel` normalises pointers to
  values.
 ## Suggested follow-up
 1. Return and check errors from `RegisterTestModels` (finding 1). One-line-per-call change, and it
   closes a silent path to authorisation fail-open.
 2. Decide whether this package belongs in `pkg/` at all (finding 2). `internal/testmodels` is the
   low-effort fix.
 3. Confirm `golangci-lint` runs in CI and that `errcheck` flags `business.go:143-148` — if it does
   not, the exclusion patterns in `.golangci.json` need review, since this is exactly the class of
   bug it exists to catch.
@@ -0,0 +1,265 @@
 # Audit — `pkg/tracing`
 - **Date:** 2026-09-29
 - **Scope:** `pkg/tracing/tracing.go` (146 LOC, 1 file, **no tests**)
 - **Axes:** thread locking/waiting · slowness · security · panic handling & logging
 - **Threat model:** hostile internet client. Span names and attributes here are built directly from
  request-controlled data (method, path, full URL, Host header).
 ## Summary
 A thin OpenTelemetry wrapper: `InitTracer`, an HTTP middleware, and helpers. The abstraction is
 fine; the **hardcoded choices** are the problem. Three of them are not configurable at all and each
 is wrong for a production, internet-facing deployment:
 - `otlptracegrpc.WithInsecure()` — trace export is **plaintext**, with a source comment admitting it.
 - `sdktrace.AlwaysSample()` — **100% of requests** are traced, with no sampling knob in config.
 - `semconv.HTTPURLKey.String(r.URL.String())` — the **full URL including query string** is exported.
 Combined: every request's full URL is shipped unencrypted to a collector, and an attacker sets the
 export volume. Span names are also built from raw paths, giving unbounded cardinality.
 `config.TracingConfig` (`pkg/config/config.go:86-91`) exposes only `Enabled`, `ServiceName`,
 `ServiceVersion` and `Endpoint` — there is no field for TLS or sample rate, so these cannot be fixed
 by configuration alone.
 | # | Severity | Axis | Finding |
 |---|----------|------|---------|
 | 1 | **High** | Security | `WithInsecure()` hardcoded — traces exported in plaintext, not configurable |
 | 2 | **High** | Security | Full URL **including query string** exported as a span attribute |
 | 3 | **High** | Slowness | `AlwaysSample()` hardcoded — 100% trace volume, attacker-controlled, no sampling config |
 | 4 | Medium | Slowness | Span name is `method + " " + r.URL.Path` — unbounded cardinality from raw path IDs |
 | 5 | Medium | Locking | `tracer` global written by `InitTracer`, read unsynchronised by `Middleware`/`StartSpan` |
 | 6 | Medium | Observability | `Middleware` records no HTTP status and no error status — spans never show failures |
 | 7 | Medium | Panic | `Middleware` does not recover; a downstream panic leaves the span unmarked (`Unset` status) |
 | 8 | Low | Slowness | `InitTracer` has no timeout/deadline on exporter or resource creation |
 | 9 | Low | Maintenance | `semconv/v1.4.0` (2021) — deprecated attribute names modern collectors no longer index |
 | 10 | Low | Security | `SetAttributes`/`AddEvent` pass caller data through with no size or cardinality limit |
 ---
 ## Findings
 ### 1. `WithInsecure()` hardcoded (High, Security)
 `tracing.go:38-42`
 ```go
 client := otlptracegrpc.NewClient(
 	otlptracegrpc.WithEndpoint(config.Endpoint),
 	otlptracegrpc.WithInsecure(), // Use WithTLSCredentials in production
 )
 ```
 The comment names the fix and the code does not implement it, and — critically — `Config`
 (`tracing.go:21-27`) has no field to express it:
 ```go
 type Config struct {
 	ServiceName    string
 	ServiceVersion string
 	Endpoint       string
 	Enabled        bool
 }
 ```
 So there is **no supported way** to enable TLS on trace export short of editing this file. Every
 span — carrying the full request URL per finding 2 — crosses the network in cleartext, and the
 collector endpoint is unauthenticated (no OTLP headers/bearer token option either), so anything that
 can reach it can also *inject* fabricated spans.
 **Recommendation:** add `Insecure bool`, `TLSConfig *tls.Config` and `Headers map[string]string` to
 `Config` (and the matching `tracing.*` keys to `pkg/config`), default to TLS on, and require an
 explicit opt-in for insecure. Wire `otlptracegrpc.WithTLSCredentials` / `WithHeaders`.
 ### 2. Full URL with query string exported (High, Security)
 `tracing.go:95-103`
 ```go
 ctx, span := tracer.Start(ctx, r.Method+" "+r.URL.Path,
 	trace.WithSpanKind(trace.SpanKindServer),
 	trace.WithAttributes(
 		semconv.HTTPMethodKey.String(r.Method),
 		semconv.HTTPURLKey.String(r.URL.String()),   // <- full URL, query string included
 		semconv.HTTPTargetKey.String(r.URL.Path),
 		semconv.HTTPSchemeKey.String(r.URL.Scheme),
 		semconv.NetHostNameKey.String(r.Host),
 	),
 )
 ```
 `r.URL.String()` includes `RawQuery`. For this API the query string is where the interesting data
 lives: filter expressions, column lists, and — for any client that passes credentials as a query
 parameter (`?api_key=`, `?token=`, signed-URL style parameters) — secrets. All of it lands in the
 tracing backend, and per finding 1 it gets there in plaintext.
 Note `HTTPTargetKey` is also set to `r.URL.Path`, so the *useful* part is already captured
 separately; `HTTPURLKey` adds only the sensitive part.
 Secondary: `r.Host` comes from the `Host` header, which is client-controlled and unvalidated here —
 so an attacker can pollute the `net.host.name` dimension with arbitrary values (cardinality blowup,
 and log/dashboard spoofing).
 **Recommendation:** export a redacted URL (scheme + host + path, query keys only or dropped
 entirely). OTel's own guidance is to strip or redact query parameters for exactly this reason.
 ### 3. `AlwaysSample()` hardcoded (High, Slowness)
 `tracing.go:61-65`
 ```go
 tp := sdktrace.NewTracerProvider(
 	sdktrace.WithBatcher(exporter),
 	sdktrace.WithResource(res),
 	sdktrace.WithSampler(sdktrace.AlwaysSample()),
 )
 ```
 Every request produces a recorded, exported span. There is no `SampleRate` in `Config` and no
 `tracing.sample_rate` key in `pkg/config/manager.go`'s defaults, so this is not tunable.
 Under the hostile-client threat model the request rate — and therefore the span rate, the batch
 queue pressure, the serialisation cost and the outbound bandwidth — is set by the attacker. Each
 request pays span allocation, attribute encoding (including the full URL string), and a share of
 batch export. When the batch queue fills, the SDK drops spans, so a flood also destroys the
 observability you need to see the flood.
 **Recommendation:** default to `sdktrace.ParentBased(sdktrace.TraceIDRatioBased(rate))` with a
 configurable rate (e.g. 0.01–0.1), keeping `AlwaysSample` available for development. `ParentBased`
 also means an upstream sampling decision is respected, which `AlwaysSample` currently overrides.
 ### 4. Unbounded span-name cardinality (Medium, Slowness)
 `tracing.go:95` — the span name is `r.Method + " " + r.URL.Path`.
 This API's paths embed identifiers (`/api/public/employees/7f3c…`, `/api/<schema>/<entity>/<id>`), so
 each distinct ID becomes a distinct span name. Consequences:
 - Tracing backends index on span name; unbounded distinct names is the classic cardinality-explosion
  cost bomb (and in some backends, a hard limit that starts rejecting data).
 - It violates the OTel HTTP convention, which requires a **low-cardinality route template**
  (`GET /api/{schema}/{entity}/{id}`), with the concrete value in `http.route`/attributes.
 - It is attacker-driven: requests to random paths — including 404s — each mint a new span name.
 **Recommendation:** derive the name from the matched route pattern. `pkg/server`'s router
 (chi/mux/gin, see `audit/pkg/server.audit.md`) exposes the route template after matching; use it, and
 place this middleware after the router so the pattern is available. Fall back to
 `r.Method + " " + "<unmatched>"` rather than the raw path.
 ### 5. Unsynchronised `tracer` global (Medium, Locking)
 `tracing.go:19`
 ```go
 var tracer trace.Tracer
 ```
 Written at `tracing.go:77` (`tracer = tp.Tracer(config.ServiceName)`), read at `tracing.go:86`,
 `:95` (`Middleware`) and `:116`, `:119` (`StartSpan`). No mutex, no `atomic.Value`.
 Same pattern as `pkg/logger`'s `Logger` global (see `audit/pkg/logger.audit.md` finding 1). Benign if
 `InitTracer` runs once before any request is served; a race the moment tracing is re-initialised at
 runtime. `InitTracer` is exported and callable at any time, and calling it twice also leaks the
 first `TracerProvider` (nothing shuts it down) — its batch processor goroutine and gRPC connection
 stay alive for the life of the process.
 Note the nil checks at `:86` and `:116` are the read-half of the race: a goroutine can observe a
 non-nil-but-torn interface value.
 **Recommendation:** `atomic.Pointer` or a `sync.Once`-guarded init; return an error from a second
 `InitTracer` call, or shut down the previous provider first.
 ### 6. No HTTP status or error status on spans (Medium, Observability)
 `Middleware` (`tracing.go:84-112`) never wraps `w`, so it cannot observe the status code. It sets no
 `semconv.HTTPStatusCodeKey` and never calls `span.SetStatus`. Every span therefore has status
 `Unset`, which tracing backends render as "OK".
 The practical effect: you cannot find failing requests in the traces. A 500-storm and a healthy
 period look identical in the span data, which defeats the main reason to run tracing on an
 internet-facing service.
 **Recommendation:** wrap the `ResponseWriter` to capture the status, set
 `semconv.HTTPStatusCodeKey.Int(status)`, and `span.SetStatus(codes.Error, ...)` for 5xx.
 ### 7. No panic handling in the middleware (Medium, Panic handling)
 `Middleware` has `defer span.End()` (`tracing.go:106`) but no `recover()`. If `next.ServeHTTP`
 panics:
 - The `defer span.End()` **does** run, so no span is leaked — that part is correct.
 - But the span is ended with status `Unset` and no exception event, so the panic is invisible in the
  trace. The one place a trace would be most valuable records nothing.
 - The panic propagates up to whichever handler is outermost. Whether that is
  `pkg/middleware/panic.go` depends on middleware ordering — if `tracing.Middleware` is installed
  *outside* the panic middleware, the panic escapes to `net/http`'s per-connection recovery, which
  kills the connection and logs to the default logger, bypassing `pkg/logger` and the error tracker
  entirely. See `audit/pkg/middleware.audit.md` and `audit/pkg/server.audit.md` for the actual order.
 This package does not import `pkg/logger` at all, so nothing here can be logged.
 **Recommendation:** recover, record `span.RecordError` + `span.SetStatus(codes.Error, …)`, then
 re-panic so the dedicated panic middleware still handles the response. Document the required
 middleware order.
 ### 8. No deadline on initialisation (Low, Slowness)
 `tracing.go:36` uses `ctx := context.Background()` for both `otlptrace.New` (`:44`) and
 `resource.New` (`:51`). `otlptracegrpc` does not block on connect by default, so this is unlikely to
 hang today — but `resource.New` with detectors can perform network calls (cloud metadata endpoints),
 and an unreachable metadata service is a classic multi-second startup stall. `InitTracer` should
 accept a `context.Context` from the caller so startup has a deadline.
 ### 9. `semconv/v1.4.0` (Low, Maintenance)
 `tracing.go:15` pins the 2021 semantic conventions. `http.method`, `http.url`, `http.target`,
 `http.scheme`, `net.host.name` were all renamed in v1.20+ (`http.request.method`, `url.full`,
 `url.path`, `url.scheme`, `server.address`). Current collectors, dashboards and backend
 auto-instrumentation views key off the new names, so these spans will not populate standard HTTP
 dashboards.
 ### 10. No limits on caller-supplied span data (Low, Security)
 `StartSpan`, `AddEvent`, `SetAttributes` (`tracing.go:115-145`) forward caller attributes verbatim.
 If any caller passes request-derived values (a filter expression, a row payload), span size is
 attacker-influenced. The SDK's default limits (128 attributes, 128 events) cap the count but not the
 *value* length — a 1 MB string attribute is accepted.
 **Recommendation:** set explicit `sdktrace.WithSpanLimits` including `AttributeValueLengthLimit`.
 ---
 ## What looks right
 - **Disabled path is genuinely free.** `InitTracer` with `Enabled: false` (`tracing.go:31-34`)
  returns a no-op shutdown func and never builds an exporter, so a disabled deployment pays nothing
  and cannot leak.
 - **Nil-tracer guards everywhere.** `Middleware` (`tracing.go:86-89`) passes through untouched and
  `StartSpan` (`tracing.go:116-118`) returns the incoming context plus the context's (no-op) span.
  So a partially-initialised process degrades safely rather than nil-panicking — a pattern
  `pkg/logger` gets right too.
 - **Context propagation is correct.** `Extract` from `propagation.HeaderCarrier(r.Header)`
  (`tracing.go:92`), a composite `TraceContext` + `Baggage` propagator (`tracing.go:72-75`), and
  `r = r.WithContext(ctx)` (`tracing.go:109`) before calling `next` — the span context actually
  reaches downstream handlers, which is the part most hand-rolled middlewares get wrong.
 - `SpanKindServer` is set correctly (`tracing.go:96`).
 - `WithBatcher` rather than a simple/sync span processor (`tracing.go:62`) — export does not block
  the request path.
 - `InitTracer` returns `tp.Shutdown` (`tracing.go:80`), giving the caller a real flush-on-shutdown
  hook with a caller-supplied context, which is better than the fixed-timeout pattern in
  `pkg/errortracking` (see that audit, finding 3).
 - `RecordError` nil-guards (`tracing.go:140-143`) so `RecordError(ctx, nil)` is a no-op.
 ## Suggested follow-up
 1. Extend `Config` with `Insecure`, TLS credentials, OTLP headers and `SampleRate`; add the matching
   keys to `pkg/config` (findings 1, 3). These cannot be fixed without an API change, so they should
   go together.
 2. Redact the query string from exported attributes (finding 2).
 3. Move to route-template span names, which requires positioning the middleware after routing
   (finding 4).
 4. Capture status code and panics in the middleware (findings 6, 7).
 5. Guard the `tracer` global (finding 5) and upgrade `semconv` (finding 9).
 6. Add tests: this package has none. A tracetest/in-memory exporter makes assertions on span name,
   attributes and status straightforward, and would have caught findings 2, 4 and 6.
@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"strings"
 	"github.com/bitechdev/ResolveSpec/pkg/logger"
 	"github.com/bitechdev/ResolveSpec/pkg/reflection"
 )
@@ -72,6 +73,15 @@ func ResolveJSONColumnExpr(model interface{}, tableAlias, token string) (expr st
 func ApplySelectColumns(query SelectQuery, model interface{}, tableAlias string, columns []string) SelectQuery {
 	for _, col := range columns {
 		if expr, args, alias, ok := ResolveJSONColumnExpr(model, tableAlias, col); ok {
 			if !reflection.HasColumn(model, alias) {
 				// No matching scan target on the model (e.g. no
 				// `bun:"<alias>,scanonly"` field declared for this JSON
 				// path) - bun would fail to scan the row with "does not
 				// have column X". Drop the expression rather than erroring;
 				// the rest of the requested columns still get selected.
 				logger.Warn("Skipping JSON select column %q: model has no scan target for alias %q", col, alias)
 				continue
 			}
 			query = query.ColumnExpr(expr+" AS "+QuoteIdent(alias), args...)
 			continue
 		}
@@ -2,6 +2,7 @@ package common
 import (
 	"reflect"
 	"strings"
 	"testing"
 	"github.com/bitechdev/ResolveSpec/pkg/spectypes"
@@ -162,3 +163,43 @@ func TestBuildJSONFilterCondition_QualifiedAndInjectionSafe(t *testing.T) {
 		t.Errorf("args = %#v", args)
 	}
 }
 // selectCapQuery is a minimal SelectQuery that records Column/ColumnExpr calls
 // so ApplySelectColumns' behaviour can be asserted without a real DB.
 type selectCapQuery struct {
 	SelectQuery
 	columns     []string
 	columnExprs []string
 }
 func (m *selectCapQuery) Column(cols ...string) SelectQuery {
 	m.columns = append(m.columns, cols...)
 	return m
 }
 func (m *selectCapQuery) ColumnExpr(q string, args ...interface{}) SelectQuery {
 	m.columnExprs = append(m.columnExprs, q)
 	return m
 }
 // jsonSelectModel has a real JSON column (Data) but only ONE pre-declared
 // scanonly field for a computed JSON path ("data_city"); "data_age" has no
 // matching scan target.
 type jsonSelectModel struct {
 	ID       int64              `json:"id" bun:"id,pk"`
 	Data     spectypes.SqlJSONB `json:"data" bun:"data"`
 	DataCity string             `json:"-" bun:"data_city,scanonly"`
 }
 func TestApplySelectColumns_SkipsJSONColumnWithoutScanTarget(t *testing.T) {
 	m := jsonSelectModel{}
 	q := &selectCapQuery{}
 	ApplySelectColumns(q, m, "", []string{"id", "data.city", "data.age"})
 	if !reflect.DeepEqual(q.columns, []string{"id"}) {
 		t.Errorf("columns = %#v, want [id]", q.columns)
 	}
 	if len(q.columnExprs) != 1 || !strings.Contains(q.columnExprs[0], `AS "data_city"`) {
 		t.Errorf("columnExprs = %#v, want exactly one expr aliased data_city", q.columnExprs)
 	}
 }
@@ -439,6 +439,63 @@ func GetSQLModelColumns(model any) []string {
 	return columns
 }
 // HasColumn reports whether the model has a struct field that bun/gorm would
 // scan a column named columnName into. Unlike GetSQLModelColumns, this
 // includes scanonly fields (e.g. a `bun:"jsonvalue_product_cost,scanonly"`
 // field added specifically to receive a computed/JSON-path SELECT expression)
 // since those are legitimate scan targets even though they are not writable.
 // Matching is case-insensitive against the resolved bun/gorm/json column name
 // and against the bare Go field name.
 func HasColumn(model any, columnName string) bool {
 	if columnName == "" {
 		return false
 	}
 	modelType := reflect.TypeOf(model)
 	for modelType != nil && (modelType.Kind() == reflect.Pointer || modelType.Kind() == reflect.Slice || modelType.Kind() == reflect.Array) {
 		modelType = modelType.Elem()
 	}
 	if modelType == nil || modelType.Kind() != reflect.Struct {
 		return false
 	}
 	return hasColumnInType(modelType, columnName)
 }
 func hasColumnInType(typ reflect.Type, columnName string) bool {
 	for i := 0; i < typ.NumField(); i++ {
 		field := typ.Field(i)
 		if !field.IsExported() {
 			continue
 		}
 		bunTag := field.Tag.Get("bun")
 		gormTag := field.Tag.Get("gorm")
 		if field.Anonymous {
 			fieldType := field.Type
 			if fieldType.Kind() == reflect.Pointer {
 				fieldType = fieldType.Elem()
 			}
 			if fieldType.Kind() == reflect.Struct {
 				if hasColumnInType(fieldType, columnName) {
 					return true
 				}
 				continue
 			}
 		}
 		if bunTag == "-" || gormTag == "-" {
 			continue
 		}
 		if strings.EqualFold(getColumnNameFromField(field), columnName) || strings.EqualFold(field.Name, columnName) {
 			return true
 		}
 	}
 	return false
 }
 // collectSQLColumnsFromType recursively collects SQL column names from a struct type
 // scanOnlyEmbedded indicates if we're inside a scan-only embedded struct
 func collectSQLColumnsFromType(typ reflect.Type, columns *[]string, scanOnlyEmbedded bool) {
@@ -411,6 +411,23 @@ func TestGetModelColumnsWithEmbedded(t *testing.T) {
 	}
 }
 func TestHasColumn(t *testing.T) {
 	m := ModelWithEmbedded{}
 	for _, col := range []string{"name", "description", "rid_base", "created_at", "cql1", "cql2"} {
 		if !HasColumn(m, col) {
 			t.Errorf("HasColumn(%q) = false, want true", col)
 		}
 	}
 	if HasColumn(m, "nonexistent_column") {
 		t.Error("HasColumn(nonexistent_column) = true, want false")
 	}
 	if HasColumn(m, "") {
 		t.Error("HasColumn(\"\") = true, want false")
 	}
 }
 func TestIsColumnWritableWithEmbedded(t *testing.T) {
 	tests := []struct {
 		name       string
@@ -349,6 +349,10 @@ func (h *Handler) handleRead(ctx context.Context, w common.ResponseWriter, id st
 			logger.Debug("Selecting columns: %v", options.Columns)
 			for _, col := range options.Columns {
 				if expr, jargs, alias, ok := common.ResolveJSONColumnExpr(model, "", col); ok {
 					if !reflection.HasColumn(model, alias) {
 						logger.Warn("Skipping JSON select column %q: model has no scan target for alias %q", col, alias)
 						continue
 					}
 					query = query.ColumnExpr(expr+" AS "+common.QuoteIdent(alias), jargs...)
 					continue
 				}
@@ -530,6 +530,10 @@ func (h *Handler) handleRead(ctx context.Context, w common.ResponseWriter, id st
 				// JSON sub-field selection (data->>'x', data.x, data#>>'{a,b}'):
 				// emit a parameterised expression aliased to a stable name.
 				if expr, jargs, alias, ok := common.ResolveJSONColumnExpr(model, selectAlias, col); ok {
 					if !reflection.HasColumn(model, alias) {
 						logger.Warn("Skipping JSON select column %q: model has no scan target for alias %q", col, alias)
 						continue
 					}
 					query = query.ColumnExpr(expr+" AS "+common.QuoteIdent(alias), jargs...)
 					continue
 				}
Author	SHA1	Message	Date
Hein	bc8bff7955	docs(audit): add audit reports for pkg/testmodels and pkg/tracing Tests / Unit Tests (push) Failing after 24s Details Tests / Integration Tests (push) Failing after 26s Details Build , Vet Test, and Lint / Build (push) Successful in 1m7s Details Build , Vet Test, and Lint / Run Vet Tests (1.23.x) (push) Successful in 1m31s Details Build , Vet Test, and Lint / Lint Code (push) Successful in 1m33s Details Build , Vet Test, and Lint / Run Vet Tests (1.24.x) (push) Successful in 1m34s Details	2026-09-29 17:15:00 +02:00
Hein	a74eebc7f3	fix(json-columns): skip JSON select columns with no model scan target Tests / Unit Tests (push) Failing after 28s Details Tests / Integration Tests (push) Failing after 29s Details Build , Vet Test, and Lint / Build (push) Successful in 1m12s Details Build , Vet Test, and Lint / Run Vet Tests (1.24.x) (push) Successful in 1m43s Details Build , Vet Test, and Lint / Run Vet Tests (1.23.x) (push) Successful in 1m46s Details Build , Vet Test, and Lint / Lint Code (push) Successful in 1m48s Details Requesting a JSON sub-field (e.g. jsonvalue->'product'->>'cost') that has no matching bun scanonly field on the model made the whole read fail with "bun: ModelX does not have column Y", since bun scans SELECT results straight into the typed model struct. Add reflection.HasColumn to check whether the model can actually receive a given column (including scanonly fields, walking embedded structs), and gate the JSON select-column expression on it in ApplySelectColumns (shared by websocketspec/mqttspec) and the resolvespec/restheadspec handlers. When there's no scan target, drop just that column with a warning instead of erroring the whole request.	2026-09-28 12:30:56 +02:00