From bc8bff7955e60b4e234deab23b65ca39f9ade66c Mon Sep 17 00:00:00 2001 From: Hein Date: Tue, 29 Sep 2026 17:15:00 +0200 Subject: [PATCH] docs(audit): add audit reports for pkg/testmodels and pkg/tracing --- audit/pkg/_CROSS-CUTTING.audit.md | 646 ++++++ audit/pkg/cache.audit.md | 1359 +++++++++++ audit/pkg/config.audit.md | 401 ++++ audit/pkg/errortracking.audit.md | 220 ++ audit/pkg/logger.audit.md | 269 +++ audit/pkg/metrics.audit.md | 1115 +++++++++ audit/pkg/middleware.audit.md | 1423 ++++++++++++ audit/pkg/modelregistry.audit.md | 474 ++++ audit/pkg/security.audit.md | 3514 +++++++++++++++++++++++++++++ audit/pkg/testmodels.audit.md | 203 ++ audit/pkg/tracing.audit.md | 265 +++ 11 files changed, 9889 insertions(+) create mode 100644 audit/pkg/_CROSS-CUTTING.audit.md create mode 100644 audit/pkg/cache.audit.md create mode 100644 audit/pkg/config.audit.md create mode 100644 audit/pkg/errortracking.audit.md create mode 100644 audit/pkg/logger.audit.md create mode 100644 audit/pkg/metrics.audit.md create mode 100644 audit/pkg/middleware.audit.md create mode 100644 audit/pkg/modelregistry.audit.md create mode 100644 audit/pkg/security.audit.md create mode 100644 audit/pkg/testmodels.audit.md create mode 100644 audit/pkg/tracing.audit.md diff --git a/audit/pkg/_CROSS-CUTTING.audit.md b/audit/pkg/_CROSS-CUTTING.audit.md new file mode 100644 index 0000000..b63b491 --- /dev/null +++ b/audit/pkg/_CROSS-CUTTING.audit.md @@ -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:` 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/.audit.md`. diff --git a/audit/pkg/cache.audit.md b/audit/pkg/cache.audit.md new file mode 100644 index 0000000..a6f984c --- /dev/null +++ b/audit/pkg/cache.audit.md @@ -0,0 +1,1359 @@ +# Audit: `pkg/cache` + +| | | +|---|---| +| **Package** | `github.com/bitechdev/ResolveSpec/pkg/cache` | +| **Files** | `cache.go` (76), `cache_manager.go` (167), `provider.go` (65), `provider_memory.go` (342), `provider_memcache.go` (284), `provider_redis.go` (269), `example_usage.go` (266), `cache_test.go` (69) | +| **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 | +| **Depth** | deep (hot package) | + +## Summary + +`pkg/cache` is a thin `Provider` abstraction over three backends. Two things +make it much more security-relevant than a cache normally is: + +1. **It stores authentication state.** `pkg/security/providers.go:398-402` caches + `UserContext` under the key `fmt.Sprintf("auth:session:%s", token)` — the raw + bearer token from the `Authorization` header. So **cache keys are directly + attacker-controlled**, the cached value is an authorization decision, and + `DeleteByPattern` is the only session-revocation mechanism. +2. **It is never explicitly initialized.** Nothing in `pkg/` calls + `Initialize`/`UseMemory`/`UseRedis`/`UseMemcache`; every consumer reaches the + cache through `GetDefaultCache()` (`cache.go:48`), which lazily constructs a + `MemoryProvider` **on the request path** without synchronization. + +The most serious findings are: a cache-write failure being converted into an +authentication failure (`GetOrSet`, `cache_manager.go:126`), session revocation +that silently cannot work on memcache, an unsynchronized lazy singleton read by +concurrent HTTP handlers, an unbounded `tagToKeys` index that `MaxSize` does not +cover, and `MemoryProvider.Get` taking the **write** lock on every cache hit. + +There is **no `recover()` anywhere in this package** and no logging at all — +grep for `recover()` and `logger.` across all 1 538 lines returns zero hits. +Every error is either returned to the caller or discarded with `_ =`. + +## Findings + +| # | Severity | Axis | Finding | +|---|---|---|---| +| 1 | **Critical** | security | Cache-write failure is returned as an error from `GetOrSet`, so a cache outage becomes a total authentication outage | +| 2 | **Critical** | security | Session revocation (`DeleteByPattern`) is unimplemented on memcache and returns an error — revoked sessions keep authenticating | +| 3 | **High** | locking | `defaultCache` is an unsynchronized lazy singleton read/written from concurrent request handlers | +| 4 | **High** | security / slowness | `tagToKeys` is unbounded and leaked by six paths; `MaxSize` bounds only `items` | +| 5 | **High** | locking / slowness | `MemoryProvider.Get` acquires the **write** lock on every hit | +| 6 | **High** | security | `DeleteByPattern` pattern syntax differs per provider (Go regexp vs Redis glob vs error); memory matches unanchored | +| 7 | **High** | security | Raw session token embedded in the `"key not found: %s"` error string | +| 8 | **High** | security | No single-flight: concurrent misses on one key run N loaders (auth stampede against the session stored procedure) | +| 9 | **Medium** | security | Attacker-controlled keys break memcache's 250-byte/no-whitespace key rule; error is swallowed as a miss, then fails the write | +| 10 | **Medium** | correctness | TOCTOU in `MemoryProvider.Get`: expired-item path deletes unconditionally after dropping the read lock | +| 11 | **Medium** | correctness | `MemoryProvider.Close()` sets `items = nil`; a subsequent `Set` panics and nothing recovers | +| 12 | **Medium** | security | Memcache tag index is non-atomic read-modify-write and shares the key namespace; lost updates silently drop keys from invalidation | +| 13 | **Medium** | security | `Clear()` maps to `FlushAll()` / `FlushDB()` — wipes the whole shared server/DB | +| 14 | **Medium** | correctness | `MemcacheProvider.Close()` is a no-op justified by a false comment; gomemcache **does** have `Close()` | +| 15 | **Medium** | security | Redis and memcache have no TLS option at all; `AUTH` password and cached `UserContext` cross the wire in cleartext | +| 16 | **Medium** | correctness | `Cache.Remember` returns a different Go type on hit vs miss | +| 17 | **Medium** | slowness | `Get`/`Exists` swallow **all** backend errors as a cache miss — a degraded backend is invisible and stampedes the DB | +| 18 | **Low** | slowness | `evictOne` is an O(n) scan under the write lock, run per insertion at capacity | +| 19 | **Low** | correctness | `CleanExpired` is dead code; there is no janitor, so expired items are only reclaimed on access | +| 20 | **Low** | security | `RedisProvider.Stats` returns the raw `INFO` output in `ProviderStats["info"]` | +| 21 | **Low** | correctness | `ctx` is accepted and completely ignored by the memcache provider | +| 22 | **Low** | correctness | `MaxSize <= 0` disables eviction entirely — an unbounded in-memory cache | +| 23 | **Low** | correctness | Provider constructors mutate the caller's config struct | +| 24 | **Low** | correctness | No defensive copy of `[]byte` on `Set`/`Get` in the memory provider | +| 25 | **Low** | hygiene | `example_usage.go` ships `log.Fatal` calls in a library package | + +--- + +### 1. Critical — a cache-write failure is an authentication failure + +`pkg/cache/cache_manager.go:112-141`: + +```go +func (c *Cache) GetOrSet(ctx context.Context, key string, dest interface{}, ttl time.Duration, loader func() (interface{}, error)) error { + err := c.Get(ctx, key, dest) + if err == nil { + return nil + } + + value, err := loader() + if err != nil { + return fmt.Errorf("loader failed: %w", err) + } + + // Store in cache + if err := c.Set(ctx, key, value, ttl); err != nil { + return fmt.Errorf("failed to cache value: %w", err) // <-- line 127 + } + ... +} +``` + +The loader has already succeeded — the authoritative value is in hand — but a +failure to *cache* it aborts the whole call. The only consumer of `GetOrSet` is +authentication, `pkg/security/providers.go:398-444`: + +```go +cacheKey := fmt.Sprintf("auth:session:%s", token) + +var userCtx UserContext +err := a.cache.GetOrSet(r.Context(), cacheKey, &userCtx, a.cacheTTL, func() (any, error) { + // ... queries the session stored procedure, returns &user on success +}) + +if err != nil { + lastErr = err + continue // Try next token +} +``` + +Any error — including "failed to cache value" — is treated as *this token is not +valid*, and after the token loop the request is rejected. + +**Failure scenario.** Redis is configured and becomes unreachable (restart, +failover, network partition, `maxmemory` reached with `noeviction`). +`RedisProvider.Get` swallows the error and reports a miss +(`provider_redis.go:91-93`), the loader runs and the database confirms the +session is valid, then `RedisProvider.Set` returns the connection error and +`GetOrSet` returns it. **Every request from every user is now rejected with an +authentication error**, even though both the database and the sessions are +healthy. A cache is supposed to be a latency optimization; here it is a hard +dependency of the auth path, and its failure mode is total outage. The same +applies to `maxmemory` pressure, which an attacker can induce (see finding 4). + +**Recommendation.** A cache-write failure must be non-fatal. Log it and return +the loaded value: + +```go +if err := c.Set(ctx, key, value, ttl); err != nil { + logger.Warn("cache: failed to store key (continuing uncached): %v", ctx, err) +} +``` + +Separately, `pkg/security` should not conflate "cache layer failed" with +"credential rejected"; the loader's own error is the only one that should fail +authentication. Consider having `GetOrSet` return the loaded value plus a +non-fatal cache error, or wrap cache errors in a sentinel the caller can test +with `errors.Is`. + +--- + +### 2. Critical — session revocation silently cannot work on memcache + +`pkg/security/providers.go:463-479` is the only session-revocation path: + +```go +func (a *DatabaseAuthenticator) ClearCache(token string) error { + ctx := context.Background() + if token != "" { + cacheKey := fmt.Sprintf("auth:session:%s", token) + return a.cache.Delete(ctx, cacheKey) + } + // Clear all auth cache entries + return a.cache.DeleteByPattern(ctx, "auth:session:*") +} + +func (a *DatabaseAuthenticator) ClearUserCache(userID int) error { + ctx := context.Background() + pattern := "auth:session:*" + return a.cache.DeleteByPattern(ctx, pattern) +} +``` + +`pkg/cache/provider_memcache.go:249-254`: + +```go +// DeleteByPattern removes all keys matching the pattern. +// Note: Memcache does not support pattern-based deletion natively. +// This is a no-op for memcache and returns an error. +func (m *MemcacheProvider) DeleteByPattern(ctx context.Context, pattern string) error { + return fmt.Errorf("pattern-based deletion is not supported by Memcache") +} +``` + +**Failure scenario.** A deployment uses memcache (`cache.provider: memcache`). +An account is compromised; an operator disables the user or the sessions are +revoked in the database, and the application calls `ClearUserCache(userID)`. +That returns an error and **removes nothing**. The attacker's cached +`UserContext` continues to authenticate every request for the full +`a.cacheTTL` — the database is never consulted again during that window +(`GetOrSet` short-circuits on a cache hit). Whether the operator even learns +this failed depends entirely on whether the caller checks the returned error; +`ClearUserCache` is also broken for a second reason — it ignores `userID` and +would have revoked every session in the process. + +Note also that `ClearUserCache`'s pattern is *not* user-scoped, so even on Redis +and memory it is a global logout, not a per-user one. That direction is at least +fail-safe. + +**Recommendation.** Revocation must not depend on a capability the provider may +not have. Options, in order of preference: + +- Tag every session entry (`SetWithTags` with tags `auth:session`, + `auth:user:`) and revoke with `DeleteByTag`, which all three providers + implement. This also makes `ClearUserCache` actually per-user. +- Keep a short `cacheTTL` (seconds, not minutes) so the revocation window is + bounded regardless. +- Make `DeleteByPattern`'s unsupported case loud: have `pkg/security` refuse to + start, or fall back to `Clear`, when the configured provider cannot revoke. +- At minimum, log at error level when a revocation call fails. + +--- + +### 3. High — `defaultCache` is an unsynchronized lazy singleton on the request path + +`pkg/cache/cache.go:9-62`: + +```go +var ( + defaultCache *Cache +) + +func Initialize(provider Provider) { + defaultCache = NewCache(provider) +} + +func UseMemory(opts *Options) error { + provider := NewMemoryProvider(opts) + defaultCache = NewCache(provider) + return nil +} +// ... UseRedis (:32), UseMemcache (:42) likewise + +func GetDefaultCache() *Cache { + if defaultCache == nil { + _ = UseMemory(&Options{ + DefaultTTL: 5 * time.Minute, + MaxSize: 10000, + }) + } + return defaultCache +} + +func SetDefaultCache(cache *Cache) { + defaultCache = cache +} +``` + +Six functions write `defaultCache` and `GetDefaultCache` both reads and writes +it, with no mutex, no `sync.Once` and no `atomic.Pointer`. `GetDefaultCache` is +called **from HTTP request handlers**: `pkg/restheadspec/handler.go:833` +(`cache.GetDefaultCache().Get(ctx, cacheKey, cachedTotalData)`), +`pkg/restheadspec/cache_helpers.go:109` and `:118`, and the equivalent +`pkg/resolvespec` paths. + +Nothing in `pkg/` ever calls `Initialize` or `Use*`, so in a default deployment +**the first traffic to arrive is what initializes the cache**, concurrently. + +**Failure scenario.** Two requests arrive simultaneously on a cold process. +Both observe `defaultCache == nil`, both run `UseMemory`, each constructing its +own `MemoryProvider`. One assignment wins. Request A writes its query total into +the provider that loses and is immediately garbage — so the entry is +unreachable, the cache reports a permanent miss for it, and the count is +recomputed from the database on every subsequent request. Worse, if this races +with an application's explicit `UseRedis` during startup, the Redis provider can +be clobbered by the lazy memory provider (or vice versa) and **the process +silently runs on the wrong backend**, which for `pkg/security` means session +cache entries that no other process shares and that `ClearCache` on another +instance can never reach. + +This is also a genuine data race on the pointer word: unsynchronized +read/write of `defaultCache`, which `go test -race` would report immediately. +See `_CROSS-CUTTING.audit.md` — `-race` is never run in this repo, and +`pkg/cache` is not in the tested package set. + +Note the same pattern exists in `pkg/security/providers.go:145` +(`cacheInstance = cache.GetDefaultCache()`), which at least happens at +construction time. + +**Recommendation.** Guard the global with `sync.RWMutex` or store it in an +`atomic.Pointer[Cache]`, and make the lazy default a `sync.Once`: + +```go +var ( + defaultCache atomic.Pointer[Cache] + defaultOnce sync.Once +) + +func GetDefaultCache() *Cache { + if c := defaultCache.Load(); c != nil { + return c + } + defaultOnce.Do(func() { + defaultCache.CompareAndSwap(nil, NewCache(NewMemoryProvider(&Options{ + DefaultTTL: 5 * time.Minute, MaxSize: 10000, + }))) + }) + return defaultCache.Load() +} +``` + +Also: every replacement path drops the previous provider **without closing it**, +so `UseRedis` after a lazy `UseMemory` (or two `UseRedis` calls) leaks the old +provider's connection pool and, for Redis, its background goroutines. Close the +old provider on swap. + +--- + +### 4. High — `tagToKeys` is unbounded; `MaxSize` bounds only `items` + +`MemoryProvider` holds two maps (`provider_memory.go:30-37`): + +```go +type MemoryProvider struct { + mu sync.RWMutex + items map[string]*memoryItem + tagToKeys map[string]map[string]struct{} // tag -> set of keys + options *Options + ... +} +``` + +`MaxSize` is checked only against `len(m.items)` (`:105`, `:135`). Six paths +remove entries from `items` **without** removing them from `tagToKeys`: + +| Path | Line | Cleans `tagToKeys`? | +|---|---|---| +| `Get` — expired-item delete | `:70` | no | +| `Set` — overwrites a tagged key | `:111` | no (and drops `Tags`, so the entry becomes unreachable for cleanup) | +| `evictOne` — expired scan | `:313` | no | +| `evictOne` — LRU victim | `:324` | no | +| `DeleteByPattern` | `:242` | no | +| `Clear` | `:254` | no — `m.tagToKeys` is never reset | +| `CleanExpired` | `:336` | no | + +Only `Delete` (`:178-187`) and `SetWithTags` (`:141-151`) maintain it, and +`DeleteByTag` (`:226`) drops one whole tag. + +Tags come from `pkg/restheadspec/cache_helpers.go:99-105`: + +```go +func buildCacheTags(schema, tableName string) []string { + return []string{ + fmt.Sprintf("schema:%s", strings.ToLower(schema)), + fmt.Sprintf("table:%s", strings.ToLower(tableName)), + } +} +``` + +and keys from `buildExtendedQueryCacheKey` (`:43-85`) — a SHA-256 of the full +query shape, including filters, sort, `customWhere`, `customOr`, `customJoin`, +expand and cursors. + +**Failure scenario.** An attacker issues `GET /api/public/orders?...` in a loop, +varying one filter value each time. Every request produces a distinct SHA-256 +key, `setQueryTotalCache` stores it under the tags `schema:public` and +`table:orders`, and `tagToKeys["table:orders"][key]` gains a member. Once +`items` reaches `MaxSize` (10 000 by default), `evictOne` starts discarding +items — but **never** the corresponding `tagToKeys` members. `items` stays +capped at 10 000; `tagToKeys["table:orders"]` grows by one 64-character key per +request, forever. At roughly 100 bytes per map entry, a few million requests — +easily reachable at modest rate — costs hundreds of megabytes of heap that +nothing will ever reclaim, because `Clear()` does not reset the map and +`CleanExpired` does not touch it. This is a **memory-exhaustion DoS driven +purely by query-string variation**, and it is cheap for the attacker: the +expensive part (the actual count query) can be avoided by hitting a table whose +count is trivial. + +The leak also breaks invalidation correctness: `DeleteByTag` iterates a key set +full of keys that no longer exist, and a plain `Set` over a previously tagged +key leaves that key in the tag index while clearing its `Tags` — so the item can +be deleted by a tag it no longer claims. + +**Recommendation.** Factor tag maintenance into a single private helper and call +it from every removal path: + +```go +// caller must hold m.mu for writing +func (m *MemoryProvider) removeLocked(key string) { + if item, ok := m.items[key]; ok { + for _, tag := range item.Tags { + if ks := m.tagToKeys[tag]; ks != nil { + delete(ks, key) + if len(ks) == 0 { + delete(m.tagToKeys, tag) + } + } + } + } + delete(m.items, key) +} +``` + +Use it in `Get`'s expired path, `Set` (before overwrite), `evictOne`, +`DeleteByPattern` and `CleanExpired`; reset `m.tagToKeys` in `Clear`; and +account `len(m.tagToKeys)` (or total members) against an explicit bound. +Independently, cap the number of distinct tags and the members per tag. + +--- + +### 5. High — `MemoryProvider.Get` takes the write lock on every hit + +`provider_memory.go:56-88`: + +```go +func (m *MemoryProvider) Get(ctx context.Context, key string) ([]byte, bool) { + // First try with read lock for fast path + m.mu.RLock() + item, exists := m.items[key] + ... + value := item.Value + m.mu.RUnlock() + + // Update access tracking with write lock + m.mu.Lock() + item.LastAccess = time.Now() + item.HitCount++ + m.mu.Unlock() + + m.hits.Add(1) + return value, true +} +``` + +The comment promises a read-lock fast path, but every **successful** lookup ends +in an exclusive lock to bump two bookkeeping fields. The `RWMutex` therefore +provides no read concurrency at all on the hot path, and Go's `RWMutex` blocks +*new* readers once a writer is waiting — so a burst of concurrent hits +degenerates into a fully serialized queue with two lock handoffs per operation. + +**Failure scenario.** The session cache is the default `MemoryProvider`. Under +concurrent load, every authenticated request performs +`GetOrSet` → `Get` → cache hit → exclusive lock. With hundreds of in-flight +requests the mutex becomes the throughput ceiling for the entire API, and +because the write lock is taken *after* the read lock is released, each hit pays +two full lock acquisitions. Adding `HitCount` to a per-item `atomic.Int64` +would make this free; as written, the cache that exists to reduce latency is the +serialization point. + +A secondary defect: between `RUnlock` at `:78` and `Lock` at `:81` the item may +have been deleted or replaced, so the code can mutate an orphaned struct. Harmless +but confirms the bookkeeping does not need the lock. + +**Recommendation.** Make the counters lock-free and drop the write lock: + +```go +type memoryItem struct { + Value []byte + Expiration time.Time + lastAccess atomic.Int64 // unix nanos + hitCount atomic.Int64 + Tags []string +} +``` + +Then the whole `Get` runs under `RLock`. If exact LRU ordering matters, consider +an approximate clock (update `lastAccess` only if it is more than a second +stale) or a sharded map to cut contention. + +--- + +### 6. High — `DeleteByPattern` has three incompatible pattern languages + +The interface (`provider.go:29-31`) says only "Pattern syntax depends on the +provider implementation", and the three implementations diverge completely: + +| Provider | Line | Semantics | +|---|---|---| +| memory | `provider_memory.go:235-244` | `regexp.Compile` + **unanchored** `MatchString` | +| redis | `provider_redis.go:197` | `SCAN MATCH` — Redis glob | +| memcache | `provider_memcache.go:252-254` | always an error | + +```go +// memory +re, err := regexp.Compile(pattern) +if err != nil { + return fmt.Errorf("invalid pattern: %w", err) +} +for key := range m.items { + if re.MatchString(key) { + delete(m.items, key) + } +} +``` + +The one caller, `pkg/security/providers.go:470`, passes `"auth:session:*"` — +a Redis glob. Interpreted as a Go regexp that is `auth:session` followed by zero +or more `:`, matched unanchored, so it happens to match the intended keys (and +any key merely *containing* `auth:session`). It works by coincidence, not design. + +**Failure scenario.** Two ways this bites: + +- **Over-deletion.** Because matching is unanchored, a glob like `user:*` becomes + the regexp `user:*` = `user` + zero-or-more colons, which matches *any* key + containing `user` — including `auth:session:` if a token happens to + contain that substring. Conversely a caller who writes a glob such as + `*` gets `regexp.Compile("*")` → `error parsing regexp: missing argument to + repetition operator`, i.e. a silent no-op invalidation where the author + expected a full flush. Stale authorization data continues to be served. +- **Attacker-influenced regexp.** `regexp.Compile` runs on the caller's string + **while holding the write lock** (`:232-238`), so if any future caller derives + a pattern from request input, an attacker both controls the compiled program + and blocks every other cache operation for its duration. Go's RE2 has no + catastrophic backtracking, but compilation of a large pattern is not free and + the lock is held across it. + +Redis's side has its own cost: `r.client.Scan(ctx, 0, pattern, 0)` with +`count = 0` leaves the server at its default `COUNT 10`, so revoking sessions +walks the entire keyspace in ~10-key increments — thousands of round trips on a +large DB, executed synchronously inside `ClearCache`. + +**Recommendation.** Define one pattern language in the interface — a glob is the +right choice, since it is the one Redis supports natively — and implement it for +memory with `path.Match` (or an explicit anchored translation to regexp), +compiled **before** taking the lock. Reject patterns the provider cannot honour +with a typed `ErrUnsupported` so callers can branch. Pass a sensible `COUNT` +(e.g. 500) to `SCAN`. Better still, replace pattern deletion with tag deletion +at the one call site (see finding 2). + +--- + +### 7. High — the session token is embedded in an error string + +`cache_manager.go:22-43`: + +```go +func (c *Cache) Get(ctx context.Context, key string, dest interface{}) error { + data, exists := c.provider.Get(ctx, key) + if !exists { + return fmt.Errorf("key not found: %s", key) + } + ... +} + +func (c *Cache) GetBytes(ctx context.Context, key string) ([]byte, error) { + data, exists := c.provider.Get(ctx, key) + if !exists { + return nil, fmt.Errorf("key not found: %s", key) + } + return data, nil +} +``` + +The key for the session cache is `"auth:session:" + token` — the raw bearer +credential. Every cache miss therefore allocates an error whose text contains a +live secret. `GetOrSet` discards it, but this is a public API on a `*Cache` that +`pkg/security` holds directly, and `pkg/security/keystore_database.go:232` calls +`ks.cache.Get` on an API-key cache too. + +**Failure scenario.** Any caller that does `logger.Error("cache lookup failed: %v", err)` +publishes the bearer token to the application log **and**, per +`audit/pkg/logger.audit.md` finding 2, forwards it verbatim to Sentry, where it +is retained by a third party with no scrubbing (`pkg/errortracking` has no +`BeforeSend` hook). A token in a log aggregator is a replayable credential for +the whole `cacheTTL` — longer, if the log outlives the session. Note that this +requires only one careless `%v` at a call site; the package is handing out the +loaded weapon. + +**Recommendation.** Never interpolate cache keys into errors. Use a package +sentinel and let the caller decide what is safe to log: + +```go +var ErrNotFound = errors.New("cache: key not found") +... +if !exists { + return ErrNotFound +} +``` + +Callers then use `errors.Is(err, cache.ErrNotFound)` instead of matching on +strings, which also fixes the miss/error conflation noted in finding 17. If a +key must appear in diagnostics, log a truncated hash of it. Separately, +`pkg/security` should key the cache on a SHA-256 of the token rather than the +token itself, exactly as `keystore_database.go` already does for API keys +(`keystoreCacheKey(hash)`, `:287`). + +--- + +### 8. High — no single-flight: concurrent misses run N loaders + +`GetOrSet` (`cache_manager.go:112`) and `Remember` (`:145`) both do +check → load → store with nothing serializing concurrent callers on the same +key. + +**Failure scenario (auth).** A client opens 200 connections with the same fresh +session token. All 200 miss the cache, all 200 enter the loader, and all 200 +execute the session stored procedure +(`SELECT p_success, p_error, p_user::text FROM ($1, $2)`, +`pkg/security/providers.go:413-415`) concurrently. Each success then also spawns +`go a.updateSessionActivity(...)` (`:447`), i.e. 200 more goroutines each issuing +a database write. The cache provides no protection at all for the first +round-trip, and under sustained concurrency where request arrival outpaces query +latency it never catches up: throughput is bounded by the database, not the +cache. A single valid credential is enough to drive this — no privilege needed. + +**Failure scenario (query totals).** The same shape applies to +`restheadspec/handler.go:833`: a burst of identical expensive `COUNT(*)` queries +all miss together and all hit the database. + +This is the classic cache stampede / thundering herd, and it is the reason +single-flight exists. + +**Recommendation.** Wrap the loader in `golang.org/x/sync/singleflight`, which is +already an indirect dependency of most Go service stacks: + +```go +type Cache struct { + provider Provider + sf singleflight.Group +} + +func (c *Cache) GetOrSet(ctx context.Context, key string, dest any, ttl time.Duration, loader func() (any, error)) error { + if err := c.Get(ctx, key, dest); err == nil { + return nil + } + v, err, _ := c.sf.Do(key, func() (any, error) { + // re-check under the flight, then load and store + ... + }) + ... +} +``` + +Note the group must be keyed per-`Cache`, and `Forget` should be called on +loader error so a failure is not shared beyond the in-flight set. For the auth +path specifically, also bound `updateSessionActivity` — an unbounded `go` per +request is its own DoS vector (raised again in the `pkg/security` audit). + +--- + +### 9. Medium — attacker-controlled keys violate memcache's key rules + +gomemcache enforces the protocol limits (verified in +`gomemcache@v0.0.0-20260422231931-4d751bb6e37c/memcache.go:58-91`): + +```go +// ErrMalformedKey is returned when an invalid key is used. +// Keys must be at maximum 250 bytes long and not +// contain whitespace or control characters. +ErrMalformedKey = errors.New("malformed: key is too long or contains invalid characters") + +func legalKey(key string) bool { + if len(key) > 250 { + return false + } + ... +} +``` + +The session cache key is `"auth:session:" + token` with `token` taken from the +`Authorization` header, so its length and byte content are chosen by the client. + +**Failure scenario.** A deployment uses memcache and issues JWT session tokens, +which routinely exceed 238 bytes. `MemcacheProvider.Get` receives +`ErrMalformedKey` and — per finding 17 — reports it as a plain cache miss +(`provider_memcache.go:80-82`). The loader runs, the database validates the +session, and then `MemcacheProvider.Set` returns `ErrMalformedKey`, which +finding 1 converts into an authentication failure. **Every user with a long +token is permanently unable to authenticate**, and the logs show nothing but a +generic "failed to cache value". A client can also trigger this deliberately +with a token containing a space to probe the backend. + +**Recommendation.** Hash keys inside the provider so key length and charset are +bounded regardless of caller input — e.g. `sha256` hex of the key when it +exceeds 200 bytes or contains illegal bytes, with a fixed prefix. And, as in +finding 7, `pkg/security` should hash the token before it ever becomes a key. + +--- + +### 10. Medium — TOCTOU when deleting an expired item + +`provider_memory.go:66-74`: + +```go +if item.isExpired() { + m.mu.RUnlock() + // Upgrade to write lock to delete expired item + m.mu.Lock() + delete(m.items, key) + m.mu.Unlock() + m.misses.Add(1) + return nil, false +} +``` + +Go's `RWMutex` has no lock upgrade, so the read lock is genuinely released and +the write lock separately acquired. The `delete` is then **unconditional** — it +does not re-check that the entry still exists or is still the expired one. + +**Failure scenario.** Goroutine A reads an expired entry for key `K` and drops +the read lock. Goroutine B takes the write lock and `Set`s a fresh value for +`K`. Goroutine A now takes the write lock and deletes B's fresh entry. B has no +idea; the value it believes it cached is gone, and the next reader recomputes +it. In the auth path this means a just-validated session is dropped immediately, +forcing another stored-procedure call — and under load this can repeat, since the +interleaving recurs whenever expiry and refresh coincide, which is exactly when +traffic for that key is highest. + +**Recommendation.** Re-check under the write lock and delete only the same +entry: + +```go +m.mu.Lock() +if cur, ok := m.items[key]; ok && cur == item { + m.removeLocked(key) // see finding 4 +} +m.mu.Unlock() +``` + +--- + +### 11. Medium — `Close()` makes the provider panic on next use + +`provider_memory.go:273-280`: + +```go +func (m *MemoryProvider) Close() error { + m.mu.Lock() + defer m.mu.Unlock() + + m.items = nil + return nil +} +``` + +Reads of a nil map are fine, but `Set`/`SetWithTags` do +`m.items[key] = &memoryItem{...}` (`:111`, `:154`), which on a nil map panics +with `assignment to entry in nil map`. + +**Failure scenario.** A graceful-shutdown handler calls `cache.Close()` while +requests are still draining — the normal ordering, since `pkg/config` defaults +`servers.drain_timeout` to 25s. The next in-flight request reaches +`setQueryTotalCache` and the process panics **while holding `m.mu`**. There is +no `recover()` anywhere in `pkg/cache`, so this unwinds into whatever the caller +has; if the HTTP server's panic handler recovers it, the mutex is never +unlocked and **every subsequent cache operation blocks forever** — the process +is alive, serving, and permanently wedged on the first cache access. That is a +worse outcome than crashing. + +**Recommendation.** Mark the provider closed instead of destroying the map, and +return an error from every method afterwards: + +```go +type MemoryProvider struct { + ... + closed bool +} + +func (m *MemoryProvider) Close() error { + m.mu.Lock() + defer m.mu.Unlock() + m.closed = true + m.items = make(map[string]*memoryItem) + m.tagToKeys = make(map[string]map[string]struct{}) + return nil +} +``` + +with `if m.closed { return ErrClosed }` at the top of each mutating method. +More generally, the package should use `defer logger.CatchPanicCallback(...)` at +its exported boundaries so a panic inside a lock is reported rather than +silently converted into a deadlock — but note `audit/pkg/logger.audit.md` +finding 4 on `CatchPanic` swallowing unconditionally; here the panic should be +logged and **re-raised** or converted to an error, not absorbed. + +--- + +### 12. Medium — the memcache tag index is a lost-update machine + +`provider_memcache.go:136-170` maintains, for each tag, a JSON array of keys: + +```go +for _, tag := range tags { + tagKey := fmt.Sprintf("cache:tag:%s", tag) + + // Get existing keys for this tag + var keys []string + if item, err := m.client.Get(tagKey); err == nil { + _ = json.Unmarshal(item.Value, &keys) + } + + // Add current key if not already present + found := false + for _, k := range keys { ... } + if !found { + keys = append(keys, key) + } + + keysData, err := json.Marshal(keys) + if err != nil { + continue + } + + tagItem := &memcache.Item{ + Key: tagKey, + Value: keysData, + Expiration: expiration + 3600, // Give tag lists longer TTL + } + _ = m.client.Set(tagItem) +} +``` + +Four distinct defects in fifteen lines: + +1. **Non-atomic read-modify-write.** `Get` then `Set` with no CAS, across a + network, for a value that every concurrent writer of the same tag touches. +2. **Unbounded growth.** The list only ever grows within its TTL, and every + writer re-reads and re-writes the whole array. `Delete` (`:190-200`) rewrites + it too. +3. **1 MB item limit.** Once the array exceeds memcached's default item size the + `Set` fails — and the error is discarded by `_ =`. +4. **`expiration + 3600` can cross memcached's 30-day boundary.** The protocol + treats an expiry value above 2 592 000 as an **absolute Unix timestamp**. A + caller passing a 30-day TTL yields `2592000 + 3600 = 2595600`, which + memcached reads as 1970-01-31 — already past, so the tag list is dead on + arrival. `int32(ttl.Seconds())` at `:108` also truncates for very large TTLs + and, for a negative TTL, expires the item immediately. + +**Failure scenario.** Two requests cache different query totals for the same +table concurrently. Both read `cache:tag:table:orders` and see `[k1]`; one +writes `[k1,k2]`, the other writes `[k1,k3]`. The second write wins and `k2` is +**no longer in the tag index**. A later `POST` to that table calls +`invalidateCacheForTags` → `DeleteByTag("table:orders")`, which deletes `k1` and +`k3` but not `k2`. `k2` continues to serve a **stale row count from before the +write** for its full TTL. Under concurrency this is not an edge case — it is the +normal outcome, and lost updates accumulate. When the list crosses 1 MB the +failure becomes total: no further keys are indexed and nothing is logged. + +Additionally, the prefixes `cache:tag:` and `cache:tags:` (`:128`, `:138`, +`:179`, `:185`, `:219`, `:239`) share the key namespace with ordinary cache +keys. No consumer currently writes keys under that prefix — the restheadspec +keys are `query_total:` and the security keys `auth:session:*` — but +nothing enforces it, and a consumer that ever allows an attacker-derived key +beginning `cache:tag:` could forge or destroy the invalidation index (mass +invalidation → database load, or suppressed invalidation → stale authorization +data served). + +**Recommendation.** Use `CompareAndSwap` (gomemcache exposes it) in a bounded +retry loop, cap the list length, honour the 30-day rule, and stop discarding +errors: + +```go +func memcacheExpiry(ttl time.Duration) int32 { + secs := int64(ttl.Seconds()) + if secs < 0 { secs = 0 } + if secs > 2592000 { // >30d must be an absolute timestamp + return int32(time.Now().Add(ttl).Unix()) + } + return int32(secs) +} +``` + +Given how weak tag support is on memcache, the honest alternative is to return +`ErrUnsupported` from `SetWithTags`/`DeleteByTag` and force callers to pick a +provider that can do it, rather than offering invalidation that silently misses +keys. Namespace the index keys under a prefix that ordinary keys cannot reach +(e.g. by prefixing all user keys with `k:`). + +--- + +### 13. Medium — `Clear()` flushes the entire shared server + +```go +// provider_memcache.go:257-259 +func (m *MemcacheProvider) Clear(ctx context.Context) error { + return m.client.FlushAll() +} + +// provider_redis.go:228-230 +func (r *RedisProvider) Clear(ctx context.Context) error { + return r.client.FlushDB(ctx).Err() +} +``` + +Neither is scoped to this application's keys. `FlushAll` wipes every key on +every configured memcached server; `FlushDB` wipes the whole logical Redis DB — +and `RedisConfig.DB` defaults to `0`, which is also where `pkg/config` puts the +event broker (`event_broker.redis.db: 0`, `pkg/config/manager.go:265`) and its +`resolvespec:events` stream. + +**Failure scenario.** An operator or an admin endpoint calls `cache.Clear()` to +drop stale entries. On Redis with the default config this also deletes the event +broker's stream and consumer-group state, so queued events are lost and +consumers fail; on memcached it evicts every other tenant sharing that server. +There is no confirmation, no scoping, and the method is one call away from any +consumer holding the `*Cache`. + +**Recommendation.** Implement `Clear` as a scoped delete over the application's +own key prefix (`SCAN`+`DEL` for Redis), require an explicitly opted-in +"destructive flush" flag to use `FlushDB`/`FlushAll`, and document that the +cache must not share a Redis DB with the event broker. Consider adding a +mandatory `KeyPrefix` to `Options` so scoping is always possible. + +--- + +### 14. Medium — `MemcacheProvider.Close()` is a no-op based on a false comment + +`provider_memcache.go:267-271`: + +```go +func (m *MemcacheProvider) Close() error { + // Memcache client doesn't have a close method + return nil +} +``` + +This is factually wrong for the pinned dependency: gomemcache +`v0.0.0-20260422231931-4d751bb6e37c` exposes `func (c *Client) Close() error` at +`memcache.go:836`. Its documented behaviour is to close all currently-open idle +connections (the client stays usable afterwards), which is exactly what a +provider `Close()` should be releasing. + +**Failure scenario.** Every provider replacement (finding 3) and every +`cache.Close()` leaves the memcache client's idle connections — up to +`MaxIdleConns` per server — established. In a process that reconfigures the +cache, or in tests that construct providers repeatedly, file descriptors +accumulate until `accept`/`dial` starts failing with `too many open files`, +which manifests as unrelated failures elsewhere in the process. + +**Recommendation.** `return m.client.Close()`. Also add `Close` handling to the +swap paths in `cache.go` per finding 3. + +--- + +### 15. Medium — no TLS option for Redis or memcache + +`RedisConfig` (`provider_redis.go:17-36`) has `Host`, `Port`, `Password`, `DB`, +`PoolSize`, `Options` — and nothing else. `redis.Options` supports `TLSConfig`, +but the constructor never sets it: + +```go +client := redis.NewClient(&redis.Options{ + Addr: fmt.Sprintf("%s:%d", config.Host, config.Port), + Password: config.Password, + DB: config.DB, + PoolSize: config.PoolSize, +}) +``` + +`MemcacheConfig` (`:18-31`) likewise has no transport security, and the +memcached protocol has no in-band auth here at all. + +**Failure scenario.** The cached values are `UserContext` objects — identity and +authorization data — and the `AUTH` password is sent in cleartext on the first +command of every new connection. Anything able to observe the path between the +service and Redis (a shared VPC, a misconfigured security group, a compromised +sidecar, a managed Redis reached over the public internet) can read session +contents and steal the Redis password, then write forged `auth:session:*` entries +directly. Writing a crafted `UserContext` into the cache is a **complete +authentication bypass**: `GetOrSet` returns it on a hit and never consults the +database. + +This is the same theme as `pkg/config`'s `sslmode: disable` default +(`pkg/config/manager.go:242`) — see `audit/pkg/config.audit.md` finding 3. + +**Recommendation.** Add TLS configuration to both configs and plumb it through: + +```go +type RedisConfig struct { + ... + TLS bool + TLSSkipVerify bool // must default false + TLSCACertFile string +} +``` + +and set `redis.Options.TLSConfig` accordingly. Since the cache holds +authentication material, treat encrypted transport as the default and require an +explicit opt-out. For memcache, prefer Redis for this workload or terminate TLS +with a local proxy (stunnel/envoy) and document it. + +--- + +### 16. Medium — `Remember` returns a different type on hit vs miss + +`cache_manager.go:145-167`: + +```go +func (c *Cache) Remember(ctx context.Context, key string, ttl time.Duration, loader func() (interface{}, error)) (interface{}, error) { + data, err := c.GetBytes(ctx, key) + if err == nil { + var result interface{} + if err := json.Unmarshal(data, &result); err == nil { + return result, nil // <-- map[string]interface{} / float64 / ... + } + } + + value, err := loader() + ... + return value, nil // <-- whatever the loader returned +} +``` + +On a hit the value comes back as generic JSON (`map[string]interface{}`, +`[]interface{}`, `float64`, `string`); on a miss it is the loader's concrete Go +type. + +**Failure scenario.** A caller writes the natural thing: + +```go +v, err := c.Remember(ctx, key, ttl, func() (any, error) { return loadUser(id) }) +u := v.(*User) // panics on every cache hit +``` + +This passes every test run against a cold cache and panics in production as soon +as the cache warms — the worst possible failure timing. `Remember` has no +callers in `pkg/` today, so this is latent, but it is a trap laid for the next +consumer and there is no `recover()` in the package to contain it. + +**Recommendation.** Either delete `Remember` in favour of `GetOrSet` (which +takes a typed `dest`), or give it the same contract: + +```go +func (c *Cache) Remember(ctx context.Context, key string, dest any, ttl time.Duration, loader func() (any, error)) error +``` + +If the generic-return shape must stay, document it loudly and name it +`RememberAny`. + +--- + +### 17. Medium — `Get`/`Exists` swallow every backend error as a cache miss + +```go +// provider_redis.go:86-95 +val, err := r.client.Get(ctx, key).Bytes() +if err == redis.Nil { + return nil, false +} +if err != nil { + return nil, false // network error, WRONGTYPE, auth failure, timeout... +} + +// provider_memcache.go:75-84 — same shape +// provider_memcache.go:262-265 +func (m *MemcacheProvider) Exists(ctx context.Context, key string) bool { + _, err := m.client.Get(key) + return err == nil +} +``` + +The `Provider.Get` signature `([]byte, bool)` cannot express "I don't know", so +every failure is indistinguishable from an absent key. Nothing is logged — +`pkg/cache` imports no logger. + +**Failure scenario.** Redis develops packet loss, or memcached is restarted, or +`maxmemory` is hit. Every lookup reports a miss, so every request falls through +to the database: the count query at `restheadspec/handler.go:857` and the session +stored procedure at `providers.go:413`. Load multiplies by the cache hit ratio +in an instant — typically 10–100× — and the database becomes the next thing to +fall over. Meanwhile the metrics say "cache miss", the logs say nothing, and the +`Stats` endpoint reports a plausible-looking miss count, so the actual cause is +invisible during the incident. Combined with finding 1, the subsequent `Set` +failure also rejects every request, so the symptom presented to operators is +"authentication is broken" with no mention of Redis. + +**Recommendation.** Widen the interface to `Get(ctx, key) ([]byte, bool, error)` +(or keep the two-value form and add `GetE`), and at minimum log at warn level +with a rate limit inside each provider: + +```go +if err != nil && err != redis.Nil { + logger.Warn("cache: redis GET failed for prefix %s: %v", ctx, keyPrefix(key), err) + return nil, false +} +``` + +Note `keyPrefix` rather than `key`, per finding 7. Feed a +`cache_errors_total{provider}` counter into `pkg/metrics` so a degraded backend +is alertable — the `Provider` interface there already has `RecordCacheHit`/ +`RecordCacheMiss` but no error counter. + +--- + +### 18. Low — `evictOne` is an O(n) scan under the write lock + +`provider_memory.go:306-326`: + +```go +func (m *MemoryProvider) evictOne() { + var oldestKey string + var oldestTime time.Time + + for key, item := range m.items { + if item.isExpired() { + delete(m.items, key) + return + } + if oldestKey == "" || item.LastAccess.Before(oldestTime) { + oldestKey = key + oldestTime = item.LastAccess + } + } + if oldestKey != "" { + delete(m.items, oldestKey) + } +} +``` + +Called from `Set` (`:107`) and `SetWithTags` (`:137`) whenever the cache is at +capacity — i.e. on **every insertion** in steady state — while the write lock is +held. With the default `MaxSize: 10000` that is a 10 000-entry map walk plus +10 000 `time.Time` comparisons per write, blocking all readers (which, per +finding 5, includes every cache hit). + +`Stats` (`:283-304`) has the same O(n) shape under `RLock`, and its comment +"Clean expired items first" is wrong — it holds a **read** lock and only counts. + +**Recommendation.** Use a proper LRU (an intrusive doubly-linked list beside the +map, as `hashicorp/golang-lru` or `container/list` gives you) for O(1) eviction, +or sample k random entries and evict the oldest of those (Redis's approach) if +approximate LRU is acceptable. Maintain a counter for `Stats` instead of +walking. Fix the misleading comment. + +--- + +### 19. Low — `CleanExpired` is dead code; there is no janitor + +`provider_memory.go:329` `CleanExpired` has no callers anywhere in the +repository (verified by grep — the only hits are its own declaration and +comment). No goroutine sweeps expirations. + +**Failure scenario.** Expired entries are reclaimed only when someone looks them +up (`Get`, `:66`) or when `evictOne` happens to walk past one. A workload whose +keys are written once and never re-read — which is exactly the +`query_total:` pattern, since a distinct filter combination is usually +requested once — retains every expired entry until `MaxSize` forces eviction. +The cache therefore sits permanently at its maximum footprint holding mostly +dead data, and the LRU scan in finding 18 walks those dead entries on every +insertion. With `MaxSize <= 0` (finding 22) nothing reclaims them at all. + +**Recommendation.** Start a janitor goroutine from `NewMemoryProvider` with a +configurable interval, and stop it in `Close()`: + +```go +func NewMemoryProvider(opts *Options) *MemoryProvider { + m := &MemoryProvider{...; done: make(chan struct{})} + go m.janitor(opts.CleanupInterval) // default e.g. 1 minute + return m +} +``` + +Guard the goroutine with `defer logger.CatchPanicCallback("cache.janitor", ...)` +so a panic in the sweep is reported rather than killing the process silently. + +--- + +### 20. Low — `RedisProvider.Stats` returns the raw `INFO` output + +`provider_redis.go:247-268`: + +```go +info, err := r.client.Info(ctx, "stats", "keyspace").Result() +... +stats := &CacheStats{ + Keys: dbSize, + ProviderType: "redis", + ProviderStats: map[string]any{ + "info": info, + }, +} +``` + +`CacheStats.ProviderStats` is `json:"provider_stats,omitempty"` — i.e. designed +to be serialized. The `stats` and `keyspace` sections include the Redis version, +uptime, connected-client counts, keyspace hit/miss totals, eviction and +expiration counters, and per-database key counts. + +**Failure scenario.** `cache.GetStats` (`cache.go:65`) has no callers today, but +it is an obvious thing to wire to a `/health` or `/admin/stats` endpoint. Doing +so exposes infrastructure fingerprinting to any client that reaches it, and the +keyspace counters leak activity volume. It is an information-disclosure +primitive waiting for a route. + +**Recommendation.** Parse `INFO` into a small allowlisted set of numeric fields +(`keyspace_hits`, `keyspace_misses`, `evicted_keys`, `expired_keys`) and +populate `CacheStats.Hits`/`Misses` from them rather than passing the blob +through. If the raw text is wanted for debugging, gate it behind an explicit +debug flag and never include it in a response body. + +--- + +### 21. Low — the memcache provider ignores `ctx` entirely + +Every method on `MemcacheProvider` accepts `ctx context.Context` and none uses +it (`:75`, `:87`, `:103`, `:177`, `:218`, `:252`, `:257`, `:262`, `:275`). The +only timeout is the client-wide `config.Timeout` (default 1s, `:49-51`). + +**Failure scenario.** A client disconnects or the request deadline expires; the +handler's context is cancelled, but the cache call continues to completion. In +`SetWithTags` that is one `Set` plus, per tag, a `Get` and a `Set` — so a +two-tag write is five sequential round trips, each able to consume the full 1s +client timeout, all after the caller has given up. Under load-shedding +conditions the server keeps doing work for requests nobody is waiting for, which +is precisely when it can least afford to. + +**Recommendation.** gomemcache's API is context-free, so either wrap each call +with a `select` on `ctx.Done()` and a goroutine, or switch to a +context-aware client. At minimum, check `ctx.Err()` at the top of each method +and return early, and document that `config.Timeout` is the real bound. + +--- + +### 22. Low — `MaxSize <= 0` silently disables eviction + +`provider_memory.go:105` and `:135` both guard with `if m.options.MaxSize > 0 && ...`. +A caller who constructs `&Options{DefaultTTL: time.Minute}` — as +`NewRedisProvider` (`:59`) and `NewMemcacheProvider` (`:54`) do for their own +defaults, and as any hand-written config easily does — gets an **unbounded** +in-memory cache. Combined with findings 4 and 19, memory then grows with +attacker-controlled query variation until the process is OOM-killed. + +**Recommendation.** Treat `MaxSize <= 0` as "use the default" (10 000) rather +than "unlimited", and require an explicit sentinel such as `MaxSize: -1` to +opt into unbounded. Validate `Options` in `NewMemoryProvider` and log the +effective values once at startup. + +--- + +### 23. Low — constructors mutate the caller's config struct + +`NewMemcacheProvider` writes `config.Servers`, `config.MaxIdleConns`, +`config.Timeout` and `config.Options` (`:41-57`); `NewRedisProvider` writes +`config.Host`, `config.Port`, `config.PoolSize`, `config.Options` (`:48-62`). +Both also assign into the `config == nil` replacement, which is at least local. + +**Failure scenario.** A caller holds one config struct and constructs two +providers from it (a common test pattern, or a primary/replica setup). The second +construction sees the first one's defaults already applied, so "zero means +default" no longer holds and an intentional later change is silently ignored. +Callers reasonably assume a constructor does not write to their arguments. + +**Recommendation.** Copy first: `cfg := *config` (plus a copy of `Options` if it +is non-nil, since it is a pointer), then apply defaults to the local copy. + +--- + +### 24. Low — no defensive copy of `[]byte` in the memory provider + +`Set` stores the caller's slice directly (`provider_memory.go:112`) and `Get` +returns the stored slice directly (`:77`, `:87`). Both share backing memory with +the caller. + +**Failure scenario.** Two requests `GetBytes` the same key and receive the same +underlying array. If either mutates it in place — or if a caller reuses a buffer +it passed to `SetBytes` — the cached value changes for everyone, with no lock +held and no copy. For the session cache that means one request's scratch buffer +can rewrite another user's cached `UserContext`. Today every consumer goes +through `json.Marshal`/`json.Unmarshal` in `cache_manager.go`, which allocates +fresh slices, so this is latent rather than live; the `SetBytes`/`GetBytes` +API (`:56`, `:37`) exposes it directly to any future caller. + +**Recommendation.** Copy on both sides in `MemoryProvider` — the Redis and +memcache providers get copies for free because the data crosses a socket, so +this also removes a behavioural difference between providers: + +```go +buf := make([]byte, len(value)) +copy(buf, value) +``` + +Document the ownership rule on the `Provider` interface either way. + +--- + +### 25. Low — `example_usage.go` is a library file full of `log.Fatal` + +`pkg/cache/example_usage.go` (266 lines) is compiled into the package and calls +`log.Fatal` at seventeen sites (`:18`, `:36`, `:44`, `:57`, `:64`, `:83`, `:96`, +`:103`, `:112`, `:127`, `:146`, `:154`, `:168`, `:187`, `:199`, `:224`, `:245`). +`log.Fatal` calls `os.Exit(1)`. + +**Failure scenario.** These are exported functions (`ExampleInMemoryCache`, +`ExampleRedisCache`, `ExampleMemcacheCache`, …) with no `_test.go` suffix, so +they are part of the package's public API. Anything that calls one — a +misremembered name, a code-completion accident, a copied snippet — can terminate +the host process on a cache error, bypassing every panic handler and graceful +shutdown path. They also drag `log` into the package's dependency set while the +package deliberately imports no logger. + +**Recommendation.** Move the file to `example_usage_test.go` (Go's testable-example +convention, which also makes them compile-checked and runnable), or to a +`_examples/` directory outside the package. Replace `log.Fatal` with returned +errors. + +--- + +## What looks right + +- `Provider` (`provider.go:9-44`) is a clean, minimal interface; the three + backends are genuinely swappable and the package has no import cycle problems + (it imports nothing from `ResolveSpec` at all). +- `hits`/`misses` use `atomic.Int64` (`provider_memory.go:35-36`) and are read + with `Load()` in `Stats`, so the counters themselves are race-free. +- `MemoryProvider.Delete` (`:173-191`) and `SetWithTags` (`:141-151`) do maintain + `tagToKeys` correctly, including deleting the tag entry when its key set + empties — the bug in finding 4 is the *other* paths not doing the same. +- `DeleteByTag` (`:194-228`) correctly handles multi-tag items: it strips only + the invalidated tag and keeps the item alive if other tags remain. +- `RedisProvider.DeleteByPattern` (`:196-225`) batches `DEL` in groups of 100 + rather than buffering an unbounded pipeline, and checks `iter.Err()`. +- `RedisProvider.Close` (`:242-244`) correctly delegates to `client.Close()`. +- Both network providers verify connectivity at construction + (`provider_redis.go:75`, `provider_memcache.go:64`) and return an error rather + than deferring the failure to the first request — though the Redis `Ping` uses + a 5s blocking timeout on a `context.Background()`, which delays startup. +- Cache keys for query totals are SHA-256 hashes of the query shape + (`restheadspec/cache_helpers.go:88-92`), not raw user input — which is why the + key-injection risk in findings 9 and 12 is confined to the `pkg/security` + session path. +- `pkg/security/keystore_database.go:287` already keys on a hash + (`keystoreCacheKey(hash)`), which is the pattern `providers.go:398` should + follow. + +## Suggested follow-up + +Ordered by value: + +1. **Make cache failures non-fatal in the auth path** (findings 1, 2, 9). This + is the single highest-value change: a cache problem should never be able to + reject a valid credential, and a revocation that cannot be honoured must be + loud. +2. **Key the session cache on `sha256(token)`** (findings 7, 9, 12) in + `pkg/security/providers.go:318`, `:398`, `:466`. Removes attacker control of + cache keys, bounds their length, and keeps the credential out of error + strings. +3. **Replace `DeleteByPattern`-based revocation with `DeleteByTag`** + (findings 2, 6), and make `ClearUserCache` actually scoped to the user. +4. **Fix the `defaultCache` race** (finding 3) with `atomic.Pointer` + + `sync.Once`, and close the displaced provider on swap (finding 14). +5. **Add `go test -race ./pkg/cache/...` to CI.** Findings 3 and 10 are + detectable in minutes with a small concurrent test; see + `_CROSS-CUTTING.audit.md`. `pkg/cache` currently has 69 lines of tests — two + functions, `TestSetDefaultCache` (`cache_test.go:9`) and + `TestGetDefaultCacheInitialization` (`:50`) — and is not in the package list + that `Makefile`/`.github/workflows/tests.yml` run. +6. **Centralize tag-index maintenance in `MemoryProvider`** (finding 4) and reset + it in `Clear`; add an eviction path that cannot leak. +7. **Make `MemoryProvider.Get` read-only** (finding 5) by moving `LastAccess` + and `HitCount` to atomics. +8. **Add single-flight to `GetOrSet`** (finding 8). +9. **Add TLS to `RedisConfig`/`MemcacheConfig`** (finding 15) and default it on. +10. **Give the package a logger and error metrics** (finding 17). Right now + `pkg/cache` cannot tell anyone that anything went wrong: 1 538 lines, zero + log statements, zero `recover()`, and eight `_ =` error discards outside the + examples file — seven of them in `provider_memcache.go`, precisely where the + tag index breaks (finding 12). +11. **Scope `Clear`** (finding 13) so it cannot flush the event broker's Redis DB. + +## Cross-references + +- `audit/pkg/logger.audit.md` — finding 2 (messages forwarded to Sentry + unscrubbed) is what makes finding 7 here dangerous; finding 4 (`CatchPanic` + swallows) is why findings 11/16/19 should not simply add `CatchPanic`. +- `audit/pkg/config.audit.md` — finding 3 (insecure transport defaults) is the + same theme as finding 15; `cache.redis.*` defaults live at + `pkg/config/manager.go:195-202` and expose no TLS field. +- `audit/pkg/security.audit.md` — findings 1, 2, 7, 8, 9 all land in + `pkg/security/providers.go`; the unbounded `go a.updateSessionActivity(...)` + per request (`:447`) is raised there. +- `audit/pkg/restheadspec.audit.md` — the query-total cache path + (`handler.go:829-860`) and tag-based invalidation (`:1477`, `:1711`, `:1785`, + `:1859`, `:1919`, `:2024`) are the other consumer; error returns from + `invalidateCacheForTags` are the invalidation-failure signal. +- `audit/pkg/metrics.audit.md` — `Provider` has `RecordCacheHit`/ + `RecordCacheMiss`/`UpdateCacheSize` but no cache-error counter, and + `pkg/cache` calls none of them. +- `audit/pkg/_CROSS-CUTTING.audit.md` — no `-race` in CI; only + `pkg/resolvespec` and `pkg/restheadspec` are tested. diff --git a/audit/pkg/config.audit.md b/audit/pkg/config.audit.md new file mode 100644 index 0000000..fc0c4dd --- /dev/null +++ b/audit/pkg/config.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). diff --git a/audit/pkg/errortracking.audit.md b/audit/pkg/errortracking.audit.md new file mode 100644 index 0000000..11d3f46 --- /dev/null +++ b/audit/pkg/errortracking.audit.md @@ -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. diff --git a/audit/pkg/logger.audit.md b/audit/pkg/logger.audit.md new file mode 100644 index 0000000..f6cf763 --- /dev/null +++ b/audit/pkg/logger.audit.md @@ -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. diff --git a/audit/pkg/metrics.audit.md b/audit/pkg/metrics.audit.md new file mode 100644 index 0000000..8bbe038 --- /dev/null +++ b/audit/pkg/metrics.audit.md @@ -0,0 +1,1115 @@ +# Audit: `pkg/metrics` + +| | | +|---|---| +| **Package** | `github.com/bitechdev/ResolveSpec/pkg/metrics` | +| **Files** | `interfaces.go` (98), `config.go` (64), `prometheus.go` (312), `example_test.go` (64) | +| **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 | +| **Depth** | lighter (small, mostly-declarative package) — but reachability is traced into every consumer | + +## Summary + +`pkg/metrics` is the best-behaved global-state package in this repository: the +provider singleton is properly mutex-guarded and returns a working +`NoOpProvider` instead of `nil` (`interfaces.go:50-72`), which is the pattern +`pkg/logger` and `pkg/cache` should copy (see `_CROSS-CUTTING.audit.md` +finding X5). Prometheus client types are goroutine-safe, so there are **no +locking or data-race defects in this package**. + +The dominant finding is not a defect in any one function but the state of the +subsystem as a whole: **no provider is ever installed.** `metrics.SetProvider` is +called only from `_test.go` files, and `NewPrometheusProvider` has no non-test +caller anywhere in the repository. Every `GetProvider()` therefore returns +`&NoOpProvider{}`, so all 39 database instrumentation points, the panic counter, +and the event-broker metrics discard their observations, and `/metrics` — if it +were routed — answers `404 Metrics provider not configured`. The instrumentation +is written, tested and wired, and then goes nowhere. + +That fact reclassifies most of the rest. Two genuine label-cardinality hazards +exist (findings 2 and 5), but neither is live today: the HTTP middleware that +would create them has no callers, and the database labels are in practice bounded +by the model registry, because `pkg/restheadspec` returns early for an +unregistered `schema.entity` before any query is built (`handler.go:130-141`). +They are recorded here as **latent** — the traps that spring on whoever first +installs a real provider, which is exactly what fixing finding 1 requires. Fix +finding 1 without findings 2 and 5 and the result is a remotely triggerable +memory leak. + +Secondary themes: a duplicate-registration panic at construction, an +unauthenticated handler that exposes the default registry, a config struct whose +`Enabled` and `Provider` fields are never read and have no section in +`pkg/config` to bind to, and a Pushgateway goroutine that discards its errors +under a comment admitting it should log. + +## Findings + +| # | Severity | Axis | Finding | +|---|---|---|---| +| 1 | **High** | correctness / logging | No provider is ever installed: all instrumentation is silently a no-op and `/metrics` 404s | +| 2 | **Medium** (latent) | security / slowness | `path` label is `r.URL.Path` — unbounded cardinality from any client, once `Middleware` is mounted | +| 3 | **Medium** | panic handling | `promauto` panics on duplicate registration; `NewPrometheusProvider` cannot be called twice and returns no error | +| 4 | **Medium** (latent) | security | `Handler()` returns `promhttp.Handler()` — no auth, and exposes the default registry including Go/process collectors | +| 5 | **Medium** (latent) | security / slowness | DB label values have no length or charset bound, and the fallback puts a 120-char SQL fragment in the `entity` label | +| 6 | **Medium** | correctness | `Config.Enabled` and `Config.Provider` are never read; there is no provider factory | +| 7 | **Medium** | logging | `startAutoPush` discards every push error with `_ = err`; the comment says a logger is needed; no `recover()` | +| 8 | **Medium** | panic handling | `StopAutoPush` panics on a second call and does not wait for the goroutine | +| 9 | **Medium** | correctness | `tableFromRawQuery` labels a subquery-in-`FROM` with the literal table name `SELECT` | +| 10 | **Low** | correctness | `ResponseWriter` drops `http.Flusher`/`http.Hijacker`/`io.ReaderFrom`, breaking SSE and WebSocket upgrade | +| 11 | **Low** | correctness | `ResponseWriter.statusCode` stays 200 when the handler never calls `WriteHeader` | +| 12 | **Low** | correctness | `eventDuration` reuses `DBQueryBuckets`, so a `db_query_buckets` change silently reshapes event histograms | +| 13 | **Low** | correctness | `PushgatewayInterval` is an untyped `int` of seconds, inconsistent with the duration strings used elsewhere | +| 14 | **Low** | correctness | `Push()` returns `nil` when Pushgateway is unconfigured, so a caller cannot tell "pushed" from "no-op" | +| 15 | **Low** | correctness | `Pusher` uses `prometheus.DefaultGatherer`, ignoring which registry the metrics went to | +| 16 | **Low** | security | `NoOpProvider.Handler()` returns a 404 body that fingerprints the metrics subsystem | +| 17 | **Low** | correctness | `metrics.Config` has no section in `pkg/config`, so its `mapstructure` tags bind to nothing | +| 18 | **Low** | slowness | `WithLabelValues` resolves a child by string on every observation; no pre-curried handles | + +--- + +### 1. High — the metrics subsystem is never activated + +`GetProvider()` falls back to a no-op when no provider has been set +(`interfaces.go:63-72`): + +```go +func GetProvider() Provider { + globalProviderMu.RLock() + p := globalProvider + globalProviderMu.RUnlock() + if p == nil { + return &NoOpProvider{} + } + return p +} +``` + +That fallback is sound design. The problem is that nothing ever replaces it. +Searching the whole module for the two functions that would install a provider: + +| Symbol | Non-test callers | Test-only callers | +|---|---|---| +| `metrics.SetProvider` | **0** | `pkg/middleware/panic_test.go:32-33`, `pkg/metrics/example_test.go:13`, `:32`, `pkg/common/adapters/database/query_metrics_test.go` (9 pairs, `:106` … `:347`) | +| `metrics.NewPrometheusProvider` | **0** | `pkg/metrics/example_test.go:12`, `:31` | + +Only three files in the repository import `pkg/metrics` outside of tests — +`pkg/eventbroker/metrics.go`, `pkg/middleware/panic.go` and +`pkg/common/adapters/database/query_metrics.go` — and all three are *producers* +that call `GetProvider()`. No `cmd/`, no `pkg/server`, and no example installs a +provider. + +**Failure scenario.** Everything below is instrumented and none of it records +anything: + +- **39 database call sites** (`bun.go` 12, `pgsql.go` 17, `gorm.go` 10) route + through `recordQueryMetrics` (`query_metrics.go:15`), which calls + `metrics.GetProvider().RecordDBQuery(...)` (`:20-27`) — into the no-op. Note + these are additionally gated on a per-connection `metricsEnabled` flag + (`:16-18`) whose config default is `false` + (`pkg/config/manager.go:246`), so there are **two** independent switches, both + off. +- **Panic counting.** `pkg/middleware/panic.go:19` calls + `metrics.GetProvider().RecordPanic(...)`, so `panics_total` never increments. + A service can panic on every request and the metric an operator would alert on + stays at zero — the failure is invisible in precisely the way metrics exist to + prevent. +- **Event broker.** `pkg/eventbroker/metrics.go:11`, `:18`, `:25` likewise. +- **`/metrics`.** `NoOpProvider.Handler()` (`interfaces.go:90-98`) returns + `404 Metrics provider not configured`. A Prometheus scrape job configured + against this service fails every scrape, and `up == 0` is the only signal. + +The severity is in the silence. Nothing logs "metrics disabled", no startup +warning fires, and `NoOpProvider` satisfies the interface perfectly — so the +code reads as fully instrumented at every call site. An operator who enables +`enable_metrics: true` on a connection, expecting query metrics, gets nothing and +has no diagnostic to explain why. The cost already paid — ~39 instrumented call +sites, 350+ lines of `query_metrics.go`, a tested `PrometheusProvider` — delivers +zero observability. + +**Recommendation.** Install a provider during startup, from config, and say so: + +```go +// in server bootstrap, after config load +provider, err := metrics.NewProvider(&cfg.Metrics) // see finding 6 +if err != nil { + return fmt.Errorf("metrics: %w", err) +} +metrics.SetProvider(provider) +logger.Info("Metrics initialized: provider=%s namespace=%q", cfg.Metrics.Provider, cfg.Metrics.Namespace) +``` + +and route the handler on an internal listener (finding 4). Order matters: +`SetProvider` must run before the first request is served, since the DB adapters +capture nothing before it. **Fix findings 2, 4 and 5 in the same change** — they +are dormant only because this one is unfixed. Add a startup `logger.Warn` when +`enable_metrics` is true on any connection while the global provider is still a +no-op; that single line turns this class of silent misconfiguration into a log +message. + +--- + +### 2. Medium (latent) — `path` label is the raw request path + +`prometheus.go:258-278`: + +```go +func (p *PrometheusProvider) Middleware(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + start := time.Now() + + // Increment in-flight requests + p.IncRequestsInFlight() + defer p.DecRequestsInFlight() + + // Wrap response writer to capture status code + rw := NewResponseWriter(w) + + // Call next handler + next.ServeHTTP(rw, r) + + // Record metrics + duration := time.Since(start) + status := strconv.Itoa(rw.statusCode) + + p.RecordHTTPRequest(r.Method, r.URL.Path, status, duration) + }) +} +``` + +feeding metrics declared with `[]string{"method", "path", "status"}` +(`prometheus.go:64`, `:71`). `RecordHTTPRequest` calls +`WithLabelValues(method, path, status)` (`:192-193`), which **creates and +permanently retains** a child series per unseen tuple. `prometheus.HistogramVec` +has no eviction and no cardinality cap. + +**Why this is latent, not live.** `Middleware` has **no callers** in the +repository, and `RecordHTTPRequest`/`IncRequestsInFlight` have none either — so +the HTTP metrics are dead code today, on top of finding 1. This is a trap, not a +live vulnerability. + +**Failure scenario, once mounted.** An attacker requests `GET /api/public/orders/1`, +`/2`, `/3`, … or simply `GET /` in a loop. Every distinct path creates +one `http_request_duration_seconds` child — with the default 11 buckets +(`config.go:43`) that is 11 counters plus `_sum`, `_count` and the retained label +strings, on the order of 400–600 bytes of live heap — plus one +`http_requests_total` child. A million distinct paths, reachable in minutes at a +few thousand requests per second and requiring **no valid credential** (the label +is recorded whatever the status), is several hundred megabytes of permanently +retained heap. The process is OOM-killed, and because the series live in the +default registry they cannot be dropped without a restart. The same requests +inflate every scrape into a multi-megabyte response, so the monitoring system +amplifies the attack and Prometheus's own ingestion suffers. + +No hostile client is even needed: a REST API with ID-bearing paths — which this +project is — generates unbounded cardinality from ordinary traffic. `method` is a +second attacker-controlled axis, since an arbitrary request method string becomes +a label value unchecked. + +**Recommendation.** Label with the matched route pattern, never the URL. Because +the pattern is only known after `ServeHTTP`, resolve it there: + +```go +next.ServeHTTP(rw, r) + +route := "unmatched" +if cr := mux.CurrentRoute(r); cr != nil { + if tpl, err := cr.GetPathTemplate(); err == nil { + route = tpl + } +} +p.RecordHTTPRequest(normalizeMethod(r.Method), route, status, duration) +``` + +(`chi.RouteContext(r.Context()).RoutePattern()` for chi.) Normalize `method` +against the known verbs, mapping anything else to `"OTHER"`. Add a hard backstop +independent of the router: track distinct label tuples per metric and collapse +anything past a few hundred to `"other"`, so no future consumer can reintroduce +this. + +--- + +### 3. Medium — `promauto` panics on duplicate registration + +Every metric is built with `promauto.New*` (`prometheus.go:58`–`:150`), which +registers into `prometheus.DefaultRegisterer` and **panics** on an +`AlreadyRegisteredError`. The constructor has no `recover()` and returns no +error: + +```go +func NewPrometheusProvider(cfg *Config) *PrometheusProvider { +``` + +**Failure scenario.** Calling `NewPrometheusProvider` twice in one process — +two config sections, a reload path, a test constructing a provider per case, or +an embedding application that also configures metrics — panics with +`duplicate metrics collector registration attempted`. From `init` or `TestMain` +that is a hard abort; from a config-reload handler it takes down a running +server. Since the signature has no `error`, a caller cannot defend against it +without wrapping the call in its own `recover()`. + +This is reachable **today in tests**: `example_test.go:12` and `:31` both call +`NewPrometheusProvider` in the same package. Those pass only because +`ExampleNewPrometheusProvider_custom` sets `Namespace` — `metricName` +(`prometheus.go:50-55`) prefixes names only when `cfg.Namespace != ""`, so the +two providers register under different names. Drop that namespace and the test +binary panics. The failure depends on configuration, not on code, which makes it +easy to ship. + +**Recommendation.** Return an error and register against an injectable registry: + +```go +func NewPrometheusProvider(cfg *Config) (*PrometheusProvider, error) { + ... + reg := cfg.Registry + if reg == nil { + reg = prometheus.DefaultRegisterer + } + p := &PrometheusProvider{...} // prometheus.NewHistogramVec, not promauto + for _, c := range p.collectors() { + if err := reg.Register(c); err != nil { + var are prometheus.AlreadyRegisteredError + if errors.As(err, &are) { + continue // or adopt are.ExistingCollector + } + return nil, fmt.Errorf("metrics: register %T: %w", c, err) + } + } + return p, nil +} +``` + +An owned `*prometheus.Registry` also fixes findings 4 and 15. + +--- + +### 4. Medium (latent) — the metrics handler has no access control + +`prometheus.go:252-255`: + +```go +func (p *PrometheusProvider) Handler() http.Handler { + return promhttp.Handler() +} +``` + +`promhttp.Handler()` serves `prometheus.DefaultGatherer` — which, because the +default registry is used, includes the Go collector (`go_*`: goroutine count, +heap sizes, GC pauses, **Go version**) and the process collector (`process_*`: +resident memory, open file descriptors, start time, CPU seconds) alongside the +application metrics. There is no authentication, no IP restriction, and no +timeout or in-flight limit. + +Latent for two reasons: no provider is installed (finding 1), and nothing routes +`Handler()` today. It becomes live the moment finding 1 is fixed in the obvious +way. + +**Failure scenario.** Mounted on the public listener — the natural thing to do, +since `Handler()` is the package's only HTTP surface and the interface comment +says "e.g., /metrics endpoint" (`interfaces.go:46`) — any client learns the Go +version and build (fingerprinting against known CVEs), in-flight request counts +and traffic volume, cache hit/miss ratios, per-table database query latencies, +panic counts, and — via findings 2 and 5 — the set of paths and SQL shapes the +application uses. `process_open_fds` and `go_goroutines` also make the +effectiveness of a resource-exhaustion attack directly observable, letting an +attacker tune it in real time. + +**Recommendation.** Serve metrics on a separate listener bound to a private +interface. `pkg/config` already supports multiple server instances +(`servers.instances.*`, `pkg/config/manager.go:182-186`), so this is +configuration plus a documented pattern. If it must share the public listener, +require authentication and bound the handler: + +```go +func (p *PrometheusProvider) Handler() http.Handler { + return promhttp.HandlerFor(p.registry, promhttp.HandlerOpts{ + ErrorHandling: promhttp.HTTPErrorOnError, + Timeout: 10 * time.Second, + MaxRequestsInFlight: 3, + }) +} +``` + +Using an owned registry (finding 3) also stops `go_*`/`process_*` from leaking; +register `collectors.NewGoCollector()` deliberately if those are wanted. + +--- + +### 5. Medium (latent) — DB label values are unbounded by design, and the fallback puts SQL text in a label + +`db_query_duration_seconds` uses `[]string{"operation", "schema", "entity", "table"}` +and `db_queries_total` adds `"status"` (`prometheus.go:86`, `:93`) — a four- and +five-dimensional label space. The values are produced by +`pkg/common/adapters/database/query_metrics.go` and normalized by four helpers +(`:30-66`) whose only sanitizer is `cleanMetricIdentifier` (`:330-335`): + +```go +func cleanMetricIdentifier(value string) string { + value = strings.TrimSpace(value) + value = strings.Trim(value, "\"'`[]") + value = strings.TrimRight(value, ";") + return value +} +``` + +That trims quotes and a trailing semicolon. It does **not** bound length, does +not restrict the character set, and does not check against any allowlist. + +**What bounds this today.** Ten of the 39 `recordQueryMetrics` call sites parse +their labels out of raw SQL (`bun.go:195`, `:215`, `:1731`, `:1739`; +`pgsql.go:128`, `:154`, `:1092`, `:1106`; `gorm.go:144`, `:165`). The rest take +them from the model-derived helpers `schemaAndTableFromModel` / +`entityNameFromModel` — 27 call sites, nine in each of `bun.go`, `pgsql.go` and +`gorm.go` — and `schemaAndTableFromModel` (`:89-96`) returns +`parseTableName(provider.TableName(), driverName)`, the model's own declared +name rather than request input. Critically, `pkg/restheadspec` resolves the +model **before** building any query and returns early when it does not exist +(`handler.go:130-141`): + +```go +model, err := h.registry.GetModelByEntity(schema, entity) +if err != nil { + // Model not found - call fallback handler if set, otherwise pass through + logger.Debug("Model not found for %s.%s", schema, entity) + ... + return +} +``` + +So `GET /api//` never reaches the database, and the label space +is in practice bounded by the number of registered models. **The cardinality +risk is latent, resting on a guarantee enforced elsewhere and nowhere documented +here.** + +**Failure scenario.** Two things remain wrong regardless: + +- **SQL text as a label value.** When no table can be parsed, + `fallbackMetricEntityFromQuery` (`:149-160`) puts up to 120 characters of the + statement (`maxMetricFallbackEntityLength`, `:13`) into `entity`. The fallback + triggers when `tableFromRawQuery` returns `""` (`:290-308`), i.e. whenever the + first keyword is not `SELECT`/`INSERT`/`UPDATE`/`DELETE` — a `WITH` CTE, + `EXPLAIN`, `CALL`, or a leading comment. Since `pkg/restheadspec` builds raw + SQL from request-supplied filters, sort and `custom_sql_where`/`custom_sql_join` + fragments (see `FetchRowNumber`, `handler.go:3198`), the label then varies with + **placeholder arity**: `IN (?)`, `IN (?,?)`, `IN (?,?,?)` are three distinct + shapes, so one client varying a filter list length mints a new permanent series + per length. On an unauthenticated `/metrics` (finding 4), that same label + discloses table names, join structure, and the presence of soft-delete and + tenancy predicates. `sanitizeMetricQueryShape` (`:162-234`) is careful and does + strip quoted literals and `?`/`$n` placeholders, so **parameter values do not + leak** — the structure does. +- **No defensive bound.** `cleanMetricIdentifier` is the single choke point for + every identifier label, and it enforces nothing. The safety property lives in a + different package, so any new caller — a migration tool, an admin endpoint, a + fallback handler that does reach the DB — silently reintroduces unbounded + cardinality with no review signal. + +**Recommendation.** + +1. **Bound every label value at the choke point**, so the guarantee is local: + +```go +var metricIdentRe = regexp.MustCompile(`^[A-Za-z0-9_]{1,32}$`) + +func cleanMetricIdentifier(value string) string { + value = strings.Trim(strings.TrimSpace(value), "\"'`[]") + value = strings.TrimRight(value, ";") + if !metricIdentRe.MatchString(value) { + return "" // callers already map "" to "default"/"unknown" + } + return value +} +``` + +2. **Delete the query-shape fallback** — return `"unknown"`. A truncated SQL + statement is not a label value; if the shape is wanted for debugging, log it + at debug level or attach it to a trace span, which has no cardinality + contract. +3. **Resolve identifiers against the model registry** in `query_metrics.go` + rather than trusting SQL parsing, so the bound is explicit rather than + inherited. +4. **Collapse `entity` and `table`.** They are identical whenever a table is + parsed (`:145` sets `entity = cleanMetricIdentifier(table)`), so the fourth + dimension buys nothing and widens every tuple. +5. Add the cardinality backstop from finding 2 in `pkg/metrics` itself. + +--- + +### 6. Medium — `Config.Enabled` and `Config.Provider` are never read + +`config.go:4-6` declares the flag: + +```go +type Config struct { + // Enabled determines whether metrics collection is enabled + Enabled bool `mapstructure:"enabled"` +``` + +`DefaultConfig()` sets it to `true` (`:40`); `ApplyDefaults()` (`:50-64`) never +touches it; `NewPrometheusProvider` never reads it. The same holds for +`Config.Provider` (`:9`), defaulted in two places (`:41`, `:52`) and never read — +nothing in the package dispatches on it to choose between `prometheus` and +`noop`, because no factory exists. + +**Failure scenario.** An operator sets `enabled: false` to stop collection — +perhaps because finding 2 or 5 is consuming memory. Nothing happens. +`NewPrometheusProvider` still registers every collector and records every +observation; the only thing that decides is which constructor the caller +hard-coded. Setting `provider: noop` fails identically. The configuration lies, +and silently: no warning, no log line. Combined with finding 17 (no `metrics` +section exists to set these in) the config surface is entirely decorative. + +**Recommendation.** Add the factory the config implies, and make it the only +supported entry point: + +```go +func NewProvider(cfg *Config) (Provider, error) { + if cfg == nil { + cfg = DefaultConfig() + } + cfg.ApplyDefaults() + if !cfg.Enabled { + return &NoOpProvider{}, nil + } + switch cfg.Provider { + case "prometheus": + return NewPrometheusProvider(cfg) // per finding 3 + case "noop": + return &NoOpProvider{}, nil + default: + return nil, fmt.Errorf("metrics: unknown provider %q", cfg.Provider) + } +} +``` + +Erroring on an unknown name matters — today a typo would fall through to +whatever the caller chose. + +--- + +### 7. Medium — the Pushgateway goroutine discards every error + +`prometheus.go:290-304`: + +```go +func (p *PrometheusProvider) startAutoPush() { + for { + select { + case <-p.pushTicker.C: + if err := p.Push(); err != nil { + // Log error but continue pushing + // Note: In production, you might want to use a proper logger + _ = err + } + case <-p.pushStop: + p.pushTicker.Stop() + return + } + } +} +``` + +The comment states the requirement and the code does the opposite. There is also +no `recover()` in this goroutine. + +**Failure scenario.** The Pushgateway URL is wrong, DNS fails, or the gateway +rejects the payload. Every tick fails, forever, in complete silence — no log +line, no counter, nothing. Pushgateway is chosen for short-lived or non-scrapable +workloads, so those are exactly the deployments where **there is no other path +for metrics to arrive**: the observability system is dark and nothing says so. +Discovery happens when someone notices a dashboard has been empty for a week. + +The missing `recover()` compounds it: `push.Pusher.Push` gathers from +`DefaultGatherer`, so a panicking custom collector would crash the whole process +from a background goroutine with no handler in the stack — the pattern flagged in +`_CROSS-CUTTING.audit.md` finding X7. + +**Recommendation.** `pkg/metrics` already imports `pkg/logger` +(`interfaces.go:8`), so this costs nothing: + +```go +func (p *PrometheusProvider) startAutoPush() { + defer logger.CatchPanicCallback("metrics.startAutoPush", nil)() + failures := 0 + for { + select { + case <-p.pushTicker.C: + if err := p.Push(); err != nil { + failures++ + if failures == 1 || failures%60 == 0 { + logger.Warn("metrics: pushgateway push failed (%d consecutive): %v", failures, err) + } + continue + } + failures = 0 + case <-p.pushStop: + p.pushTicker.Stop() + return + } + } +} +``` + +The backoff matters: an unconditional `Warn` per tick forwards a message to +Sentry per tick (`_CROSS-CUTTING.audit.md` finding X8), turning a broken gateway +into a second incident. Add a `metrics_push_failures_total` counter so the +failure is visible in whatever monitoring *is* working. + +--- + +### 8. Medium — `StopAutoPush` panics on a second call and does not wait + +`prometheus.go:308-312`: + +```go +func (p *PrometheusProvider) StopAutoPush() { + if p.pushStop != nil { + close(p.pushStop) + } +} +``` + +`close` of an already-closed channel panics with `close of closed channel`. The +check guards only `nil`, not "already closed", and the method is documented as a +shutdown hook — the code most likely to run twice. (`pushStop` is declared +`chan bool` at `prometheus.go:35` and created at `:163`; the value sent is never +read, so `chan struct{}` is the more honest type.) + +**Failure scenario.** A shutdown path calls `StopAutoPush` and a deferred cleanup +or signal handler calls it again — or two signals arrive. The second `close` +panics **during shutdown**, where `pkg/middleware/panic.go` is not in the stack +because this is not a request. The process aborts mid-drain, losing the in-flight +requests that the 25-second `servers.drain_timeout` +(`pkg/config/manager.go:176`) exists to protect. + +Separately, the method returns without waiting for `startAutoPush` to observe the +close. A push in progress is abandoned and the process's final interval of +metrics is lost — precisely the interval a Pushgateway deployment most wants. + +**Recommendation.** `sync.Once` plus a done channel, making shutdown idempotent +and synchronous: + +```go +type PrometheusProvider struct { + ... + pushStop chan struct{} + pushStopOnce sync.Once + pushDone chan struct{} +} + +func (p *PrometheusProvider) StopAutoPush() { + if p.pushStop == nil { + return + } + p.pushStopOnce.Do(func() { close(p.pushStop) }) + <-p.pushDone +} +``` + +with `defer close(p.pushDone)` in `startAutoPush` and a final `p.Push()` before +it returns. Consider `StopAutoPush(ctx)` so the wait is bounded. + +--- + +### 9. Medium — a subquery in `FROM` is labelled as a table named `SELECT` + +`tableFromRawQuery` (`query_metrics.go:290-308`) takes the first token after the +first `FROM`, and `tokenizeQuery` (`:319-328`) replaces `(` and `)` with spaces: + +```go +func tokenizeQuery(query string) []string { + replacer := strings.NewReplacer( + "\n", " ", "\t", " ", "(", " ", ")", " ", ",", " ", + ) + return strings.Fields(replacer.Replace(query)) +} +``` + +so for `SELECT ... FROM ( SELECT ... )` the token after `FROM` is the inner +`SELECT`. `parseTableName("SELECT", driver)` then yields `table = "SELECT"`, and +`metricTargetFromRawQuery` sets `entity = cleanMetricIdentifier(table)` (`:145`), +so both identifier labels become the keyword. + +**Failure scenario.** `FetchRowNumber` (`pkg/restheadspec/handler.go:3114`) +builds exactly that shape — an outer select over a `ROW_NUMBER() OVER (...)` +subquery, assembled with `fmt.Sprintf` at `:3173-3189` — and executes it through +the raw path (`db.Query(ctx, &result, queryStr, pkValue)`, `:3198`). Its metrics are recorded with +`entity = "SELECT"` and `table = "SELECT"`. Every such query across every table +collapses into one series named after a keyword. An operator investigating slow +cursor pagination sees a `db_query_duration_seconds{table="SELECT"}` bucket with +no indication of which table is slow, while the real table's own series omits +this traffic entirely — the metric is not merely missing but **actively +misleading**, and it silently aggregates unrelated tables into one latency +distribution. + +The same tokenizer means `tokenAfter` matches a `FROM` appearing anywhere, +including inside a subquery or a `CASE` expression, so the first `FROM` is not +reliably the outer one. + +**Recommendation.** Treat an unparseable target as unknown rather than guessing. +Reject any candidate that is a SQL keyword before accepting it: + +```go +var sqlKeywords = map[string]struct{}{ + "select": {}, "insert": {}, "update": {}, "delete": {}, "with": {}, + "values": {}, "set": {}, "where": {}, "from": {}, "join": {}, "as": {}, +} + +func tokenAfter(tokens []string, keyword string) string { + for idx, token := range tokens { + if strings.EqualFold(token, keyword) && idx+1 < len(tokens) { + cand := cleanMetricIdentifier(tokens[idx+1]) + if _, isKw := sqlKeywords[strings.ToLower(cand)]; isKw { + return "" + } + return cand + } + } + return "" +} +``` + +Better: have the callers that already know the target pass it explicitly. +`FetchRowNumber` knows `tableName` — threading it into the metric call is more +accurate than any amount of SQL parsing, and removes the guesswork for the +highest-traffic raw query in the codebase. Note that with finding 5's +recommendation applied, `""` here means `entity`/`table` become `"unknown"` +rather than falling back to a SQL fragment. + +--- + +### 10. Low — `ResponseWriter` drops optional interfaces + +`prometheus.go:172-176`: + +```go +type ResponseWriter struct { + http.ResponseWriter + statusCode int +} +``` + +Embedding promotes only `Header`, `Write` and `WriteHeader` (the latter +overridden at `:185-188`). The wrapper does not implement `http.Flusher`, +`http.Hijacker`, `http.Pusher` or `io.ReaderFrom`, so a handler's type assertion +for any of them fails once this middleware is in the chain. + +**Failure scenario.** This repository has `pkg/websocketspec`. A WebSocket +upgrade requires `w.(http.Hijacker)`; with this middleware in front, the +assertion fails and `gorilla/websocket` returns +`websocket: response does not implement http.Hijacker`. **Every WebSocket +connection fails**, and the cause is a metrics wrapper several layers up — a hard +bug to locate, since removing the middleware "fixes" it and the code looks +unrelated. The same applies to SSE, chunked streaming and long-polling: without +`Flusher`, output buffers until the handler returns. Losing `io.ReaderFrom` +silently disables `sendfile` for large bodies. + +**Recommendation.** Implement the optional interfaces with pass-through and a +capability check, and add `Unwrap`: + +```go +func (rw *ResponseWriter) Flush() { + if f, ok := rw.ResponseWriter.(http.Flusher); ok { + f.Flush() + } +} + +func (rw *ResponseWriter) Hijack() (net.Conn, *bufio.ReadWriter, error) { + if h, ok := rw.ResponseWriter.(http.Hijacker); ok { + return h.Hijack() + } + return nil, nil, fmt.Errorf("metrics: ResponseWriter does not support Hijack") +} + +func (rw *ResponseWriter) Unwrap() http.ResponseWriter { return rw.ResponseWriter } +``` + +`Unwrap` is what Go 1.20+'s `http.ResponseController` uses, making the wrapper +transparent to any handler written against that API. Better still, use +`httpsnoop` or `ResponseController` and delete the wrapper. Audit the other +response wrappers in `pkg/middleware` and `pkg/server` for the same defect. + +--- + +### 11. Low — `statusCode` stays 200 when nothing was written + +`NewResponseWriter` seeds `statusCode: http.StatusOK` (`prometheus.go:178-183`), +correct for Go's implicit-200 behaviour, and the middleware records it +unconditionally after `ServeHTTP` returns (`:273-276`). + +**Failure scenario.** A handler panics. If `PanicRecovery` is *inside* the +metrics middleware it writes a 500 and the label is right; if it is *outside* — +or if the panic escapes to `net/http`'s own recovery, which writes nothing — +`statusCode` is still 200 and a failed request is counted as a success. +Identically, a client that disconnects mid-response, or a handler that returns +without writing, is recorded as `status="200"`. Error-rate alerts built on +`http_requests_total{status=~"5.."}` under-report exactly the failures they exist +to catch. + +**Recommendation.** Track whether the header was written and label accordingly: + +```go +type ResponseWriter struct { + http.ResponseWriter + statusCode int + wroteHeader bool +} + +func (rw *ResponseWriter) WriteHeader(code int) { + if rw.wroteHeader { + return // also suppresses the "superfluous WriteHeader" warning + } + rw.statusCode, rw.wroteHeader = code, true + rw.ResponseWriter.WriteHeader(code) +} + +func (rw *ResponseWriter) Write(b []byte) (int, error) { + if !rw.wroteHeader { + rw.wroteHeader = true // implicit 200 + } + return rw.ResponseWriter.Write(b) +} +``` + +Use a distinct `"none"` status when `!wroteHeader`, and document that +`PanicRecovery` must be mounted **inside** the metrics middleware so panics are +observed as 500s. Ordering is covered in `middleware.audit.md` and +`server.audit.md`. + +--- + +### 12. Low — event histograms reuse the DB query buckets + +`prometheus.go:130-137`: + +```go +eventDuration: promauto.NewHistogramVec( + prometheus.HistogramOpts{ + Name: metricName("event_processing_duration_seconds"), + Help: "Event processing duration in seconds", + Buckets: cfg.DBQueryBuckets, // Events are typically fast like DB queries + }, + []string{"source", "event_type"}, +), +``` + +There is no `EventBuckets` field in `Config`. + +**Failure scenario.** An operator tunes `db_query_buckets` for their database — +say narrowing the top bucket to 1s because queries are fast — and silently +changes the resolution of event-processing histograms too. Event handlers do +arbitrary work, including outbound HTTP, so their latency distribution has no +reason to match a query's. Observations above the last bucket land only in +`+Inf`, so `histogram_quantile` returns the last boundary and a slow consumer is +hidden. The coupling is invisible from the config file. + +**Recommendation.** Add `EventBuckets []float64` to `Config` with its own default +in `DefaultConfig`/`ApplyDefaults`, defaulting to the DB buckets for +compatibility. Consider a native histogram (`NativeHistogramBucketFactor`), which +removes bucket tuning entirely on a Prometheus that supports it. + +--- + +### 13. Low — `PushgatewayInterval` is an untyped seconds `int` + +`config.go:34` declares `PushgatewayInterval int` and `prometheus.go:164` reads +it as seconds: + +```go +p.pushTicker = time.NewTicker(time.Duration(cfg.PushgatewayInterval) * time.Second) +``` + +Everywhere else in this project, intervals are duration strings parsed by viper — +`servers.shutdown_timeout: "30s"`, `dbmanager.health_check_interval: "30s"`, +`cache.memcache.timeout: "100ms"` (`pkg/config/manager.go:175`, `:231`, `:202`). + +**Failure scenario.** An operator follows the surrounding convention and writes +`pushgateway_interval: "30s"`. viper's `mapstructure` decode into an `int` fails +or yields `0`, and `0` is the documented "automatic pushing is disabled" value +(`config.go:32-33`) — so **pushing is silently off** and, as in finding 7, +nothing logs it. A value of `1` instead pushes every second, hammering the +gateway. + +**Recommendation.** Change the field to `time.Duration`; viper decodes duration +strings natively via `StringToTimeDurationHookFunc`. Validate in `ApplyDefaults` +that a configured interval is at least a second, and log the effective value once +at startup. + +--- + +### 14. Low — `Push()` cannot report "no-op" + +`prometheus.go:282-287`: + +```go +func (p *PrometheusProvider) Push() error { + if p.pusher == nil { + return nil // Pushgateway not configured, silently skip + } + return p.pusher.Push() +} +``` + +**Failure scenario.** A short-lived job — a migration, a cron task — calls +`Push()` before exiting, checks the error, sees `nil`, and exits believing its +metrics were delivered. `PushgatewayURL` was empty (a typo, an unset environment +variable), so nothing was sent and nothing will ever scrape a process that has +already exited. The metrics are lost with an explicit success report, which is +worse than an error. + +**Recommendation.** Return a sentinel the caller can distinguish: + +```go +var ErrPushgatewayNotConfigured = errors.New("metrics: pushgateway not configured") + +func (p *PrometheusProvider) Push() error { + if p.pusher == nil { + return ErrPushgatewayNotConfigured + } + return p.pusher.Push() +} +``` + +`startAutoPush` never reaches `Push` with a nil pusher — the goroutine starts only +inside the `cfg.PushgatewayURL != ""` branch (`:157-167`) — so the sentinel needs +no special handling there, but treat it as non-failing if that changes. + +--- + +### 15. Low — the pusher gathers from the default gatherer + +`prometheus.go:158-159`: + +```go +p.pusher = push.New(cfg.PushgatewayURL, cfg.PushgatewayJobName). + Gatherer(prometheus.DefaultGatherer) +``` + +**Failure scenario.** Consistent with `promauto` registering into the default +registerer, so it works today — but it silently couples the pusher to global +state. Once findings 3 and 4 move the provider to an owned registry, the pusher +keeps gathering from the default one and pushes **the wrong set of metrics** (Go +and process collectors, plus whatever any other library registered) while +omitting the application metrics it was configured for. Nothing errors; the +gateway receives a plausible payload. + +It also means the pushed payload currently includes `go_*` and `process_*` from +every instance, the Pushgateway anti-pattern — those are per-process and the +gateway has no instance label to separate them, so instances overwrite each +other. + +**Recommendation.** Gather from the provider's own registry and add an instance +grouping: + +```go +p.pusher = push.New(cfg.PushgatewayURL, cfg.PushgatewayJobName). + Gatherer(p.registry). + Grouping("instance", instanceID) +``` + +`pkg/config` already has `event_broker.instance_id` +(`pkg/config/manager.go:256`) that could supply the value. + +--- + +### 16. Low — the no-op handler fingerprints the subsystem + +`interfaces.go:90-98`: + +```go +func (n *NoOpProvider) Handler() http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + _, err := w.Write([]byte("Metrics provider not configured")) + if err != nil { + logger.Warn("Failed to write. %v", err) + } + }) +} +``` + +**Failure scenario.** Minor, but distinguishable: a client probing `/metrics` +gets `404 Metrics provider not configured` rather than the router's generic 404, +confirming the route is registered and metrics merely disabled. That tells an +attacker a configuration change or restart may expose the data in finding 4, and +distinguishes this application from others behind the same proxy. Given finding +1, this is also the response a real Prometheus scrape gets today. + +The `logger.Warn` on a failed write is the wrong level: a client disconnecting +mid-response is routine, and per `_CROSS-CUTTING.audit.md` finding X8 every +`Warn` is forwarded to Sentry — so a closed connection generates a third-party +error event. + +**Recommendation.** Return a bare `http.NotFound(w, r)` so the response is +indistinguishable from any unregistered path, and drop the log line or make it +`Debug`. + +--- + +### 17. Low — `metrics.Config` is not wired into `pkg/config` + +`pkg/config/config.go` has sections for tracing, cache, logger, error tracking, +middleware, CORS, event broker, dbmanager, paths and extensions, and +`setDefaults` (`pkg/config/manager.go:170-293`) sets defaults for all of them. +There is **no `metrics` section and no `metrics.*` default**, and +`Manager.SetConfig` (`:112-134`) likewise sets every section but metrics. +`metrics.Config`'s `mapstructure` tags (`config.go:6`, `:9`, `:12`, …) therefore +have nothing to bind to. + +**Failure scenario.** An operator cannot configure metrics through the normal +mechanism — `RESOLVESPEC_METRICS_ENABLED` does nothing, and neither does a +`metrics:` block in `config.yaml`. The struct's tags imply otherwise, so the +natural conclusion is that configuration is broken rather than absent, and the +only way to learn the truth is to read `setDefaults`. Together with findings 1 +and 6, metrics are unconfigurable and uninstallable without writing Go. + +**Recommendation.** Add `Metrics metrics.Config \`mapstructure:"metrics"\`` to +`config.Config` and defaults to `setDefaults`: + +```go +v.SetDefault("metrics.enabled", false) // see findings 2 and 5 before defaulting to true +v.SetDefault("metrics.provider", "prometheus") +v.SetDefault("metrics.namespace", "resolvespec") +``` + +Check the import direction first: `pkg/metrics` imports `pkg/logger` +(`interfaces.go:8`), and `pkg/logger` imports `pkg/errortracking` +(`logger.go:12`), so `pkg/config` → `pkg/metrics` → `pkg/logger` → +`pkg/errortracking` must not close back on `pkg/config` +(`errortracking.audit.md` finding 8 flags this coupling). If it does, duplicate +the fields in `pkg/config` rather than importing. + +--- + +### 18. Low — `WithLabelValues` on every observation + +Each record method resolves its child from strings on every call, e.g. +`prometheus.go:192-193`: + +```go +p.requestDuration.WithLabelValues(method, path, status).Observe(duration.Seconds()) +p.requestTotal.WithLabelValues(method, path, status).Inc() +``` + +`WithLabelValues` hashes the label values and takes the `MetricVec`'s internal +`RWMutex` to look up or create the child — twice here, twice in `RecordDBQuery` +(`:212-213`) and twice in `RecordEventProcessed` (`:238-239`). + +**Failure scenario.** A small constant cost, not a defect, and the normal way to +use the API. It becomes measurable only when the label space is large — which +findings 2 and 5 would guarantee. A map with millions of entries has poor cache +locality, so the two lookups per request grow from tens of nanoseconds to +microseconds, and the `MetricVec` mutex becomes a contention point across all +request-serving goroutines. + +**Recommendation.** Fix findings 2 and 5 first; the cardinality bound is the real +remedy. If the DB path proves hot afterwards, pre-curry per-table children at +model-registration time and cache the `prometheus.Observer`/`Counter` handles +rather than resolving by string per query — but only with a bounded, known label +set. + +--- + +## What looks right + +- **The global provider is correctly synchronized** (`interfaces.go:50-72`) — + `sync.RWMutex`, read under `RLock`, written under `Lock`, with the pointer + copied to a local before the lock is released. This is the only global in + `pkg/` that gets this right, and `GetProvider` returning `&NoOpProvider{}` + instead of `nil` means no caller needs a nil check. `pkg/logger`, `pkg/cache`, + `pkg/config` and `pkg/tracing` should adopt this shape + (`_CROSS-CUTTING.audit.md` finding X5). +- `NoOpProvider` implements the full interface (`interfaces.go:74-98`) with + genuine no-ops, so disabling metrics costs one interface dispatch. +- Prometheus client types are goroutine-safe by contract, so the record methods + need no locking of their own — and correctly have none. +- `IncRequestsInFlight`/`DecRequestsInFlight` are paired with `defer` in the + middleware (`prometheus.go:263-264`), so the gauge cannot leak on a panic. +- Bucket defaults are sensible and distinct for HTTP versus DB + (`config.go:43-45`), with the reasoning in comments. +- `ApplyDefaults` (`config.go:50-64`) is idempotent and only fills empty values. +- `RecordDBQuery` derives `status` from `err != nil` (`prometheus.go:208-211`) + rather than taking a caller-supplied string, so that label is bounded to two + values by construction — the right instinct, applied to one label out of five. +- In the consumers: `sanitizeMetricQueryShape` (`query_metrics.go:162-234`) + correctly strips single-quoted literals (including `''` escapes) and `?`/`$n` + placeholders before query text is used as a label, so **parameter values do not + reach `/metrics`**. `tableNameProviderType` is cached at package level (`:99`) + and `tableNameProviderFromModel` checks `Implements` before `reflect.New` + (`:121-125`) to avoid an allocation — careful, deliberate work. +- The adapter raw-query methods wrap themselves in + `recover()` + `logger.HandlePanic` (e.g. `bun.go:189-193`, `:209-213`) and + still call `recordQueryMetrics` on the error path (`:204`, `:222`), so a failed + query is counted rather than dropped. +- `pkg/common/adapters/database/query_metrics_test.go` has 11 test functions, + nine of which install a recording provider via `SetProvider` and restore the + previous one with `defer` — the correct pattern for a global, and the + best-tested metrics code in the repo. +- `pkg/eventbroker/metrics.go:11`, `:18`, `:25` nil-check `GetProvider()` + defensively even though it cannot return nil. Harmless belt-and-braces. +- `example_test.go` is correctly a `_test.go` file in package `metrics_test` with + `// Output:` assertions — unlike `pkg/cache/example_usage.go`, which ships + `log.Fatal` in the library (`cache.audit.md` finding 25). + +## Suggested follow-up + +1. **Decide whether metrics are a supported feature.** If yes, install a provider + at startup from config (finding 1) and add the factory and config section + (findings 6, 17). If no, delete `PrometheusProvider` and the 39 instrumented + call sites rather than carrying dead weight that reads as working + instrumentation. The current middle state is the worst of both. +2. **Fix findings 2, 4 and 5 in the same change as finding 1**, not after. Route + templates plus a cardinality backstop, an internal-only listener, and a bounded + `cleanMetricIdentifier`. Activating metrics without these converts three latent + defects into a remotely triggerable memory leak and an unauthenticated + information leak on the same day. +3. **Make `NewPrometheusProvider` return an error against an owned registry** + (finding 3), which also fixes findings 4 and 15 and makes the package testable. +4. **Fix the `SELECT`-as-table-name mislabelling** (finding 9) — have + `FetchRowNumber` and the other raw-query callers pass the target they already + know. +5. **Fix the `ResponseWriter` wrapper** (findings 10, 11) before anything depends + on streaming or a WebSocket upgrade through this middleware. Add `Unwrap()`. +6. **Log Pushgateway failures with backoff and make `StopAutoPush` idempotent** + (findings 7, 8). +7. **Add `go test -race ./pkg/metrics/...` to CI.** The package's 64 test lines + are four `Example*` functions with no assertions beyond `// Output:`, and it is + not in the tested package set (`_CROSS-CUTTING.audit.md` findings X1, X2, X9). + A test that constructs two providers with the same namespace would catch + finding 3 immediately; one asserting `GetProvider()` is not a `*NoOpProvider` + after bootstrap would catch finding 1. + +## Cross-references + +- `audit/pkg/_CROSS-CUTTING.audit.md` — **X10 (declared-but-never-installed + subsystems) generalizes finding 1 of this audit**: the same defect shape appears + in `pkg/middleware` and in the ignored CORS config, and X10 carries the combined + remediation order. Finding X5 cites this package's + `globalProviderMu` as the pattern to copy; X1/X2/X9 cover the absent tests; X8 + is why finding 7's log line needs rate limiting and why finding 16's `Warn` is + the wrong level. +- `audit/pkg/cache.audit.md` — the `Provider` interface here has + `RecordCacheHit`/`RecordCacheMiss`/`UpdateCacheSize` but **no cache-error + counter**, and `pkg/cache` calls none of the three (finding 17 there). Those + three methods have zero callers repo-wide, so cache observability is not merely + a no-op but entirely unwritten. +- `audit/pkg/middleware.audit.md` — `PanicRecovery` + (`pkg/middleware/panic.go:14-33`) is the only caller of `RecordPanic`, and its + ordering relative to this package's `Middleware` decides whether finding 11 + misreports panics as 200s. `panic.go:28` also returns the panic value to the + client, covered there. +- `audit/pkg/common.audit.md` — `query_metrics.go` produces findings 5 and 9's + label values; `cleanMetricIdentifier` (`:330`) is the choke point, and + `tokenizeQuery` (`:319`) the parser at fault. +- `audit/pkg/restheadspec.audit.md` — `handler.go:130-141` is the early return + that currently bounds finding 5; `FetchRowNumber` (`:3175-3198`) is the + raw-query path behind findings 5 and 9 (`FetchRowNumber` begins at `:3114`). +- `audit/pkg/config.audit.md` — finding 17 (no `metrics` section); + `dbmanager.connections.*.enable_metrics` defaults to `false` + (`manager.go:246`), the second of the two switches keeping finding 5 dormant. +- `audit/pkg/eventbroker.audit.md` — `UpdateEventQueueSize` is an unlabelled + gauge, so it is cardinality-safe; `source` and `event_type` + (`prometheus.go:121`, `:128`, `:136`) are bounded only if event types are. +- `audit/pkg/server.audit.md` — where a separate metrics listener would be + configured, where `SetProvider` should be called, and where middleware order is + decided. diff --git a/audit/pkg/middleware.audit.md b/audit/pkg/middleware.audit.md new file mode 100644 index 0000000..1052da0 --- /dev/null +++ b/audit/pkg/middleware.audit.md @@ -0,0 +1,1423 @@ +# Audit: `pkg/middleware` + +| | | +|---|---| +| **Package** | `github.com/bitechdev/ResolveSpec/pkg/middleware` | +| **Files** | `panic.go` (33), `ratelimit.go` (233), `blacklist.go` (212), `sanitize.go` (251), `sizelimit.go` (70), `README.md` (18 KB) | +| **Tests** | `panic_test.go` (86), `ratelimit_test.go` (388), `blacklist_test.go` (254), `sanitize_test.go` (273), `sizelimit_test.go` (126) — 1 127 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 | +| **Depth** | deep | + +## Summary + +This package is the project's perimeter: rate limiting, IP blacklisting, request +size limiting, input sanitization and panic recovery. It is also the package where +the threat model bites hardest, and it has two structural problems. + +**First, almost none of it is mounted.** `NewRateLimiter`, `NewIPBlacklist`, +`NewRequestSizeLimiter`, `DefaultSanitizer` and `StrictSanitizer` have **zero +non-test callers** anywhere in the repository. Only `pkg/server/manager.go` +imports the package, and only to apply `PanicRecovery` (`manager.go:466`). The +`MiddlewareConfig` fields that would configure the rest +(`pkg/config/config.go:123-125`) are declared, defaulted +(`pkg/config/manager.go:209-211`) and **read by nothing**. So against the stated +threat model there is currently **no rate limiting, no request-size limit, no IP +blocking and no input sanitization in the serving path** — finding 1. + +**Second, the protections themselves would not hold if mounted.** Both +IP-based controls derive the client address from `getClientIP` +(`ratelimit.go:210-233`), which trusts `X-Forwarded-For` unconditionally and with +no trusted-proxy configuration. One attacker-chosen header therefore bypasses the +rate limiter entirely *and* makes its limiter map grow without bound (finding 2), +and the same header defeats the IP blacklist outright when `UseProxy` is set +(finding 3). The sanitizer is a denylist that I verified is bypassable four +different ways — and in one case **manufactures a `javascript:` URI from input +that contained none** — while simultaneously corrupting ordinary filter values +like `price>100` and any JSON parameter (findings 6 and 7). + +The one live middleware, `PanicRecovery`, writes the panic value into the HTTP 500 +body (`panic.go:28`), and `panic_test.go:57` asserts that it does — the leak is +test-locked as the intended contract (finding 4). + +On the positive side the locking is genuinely careful: `getLimiter` +(`ratelimit.go:42-62`) implements correct double-checked locking, the blacklist +guards every field with an `RWMutex`, and the package carries 1 127 lines of +tests — more than the 799 lines of code. There are **no data races** in the +package. The defects are in trust boundaries and in what is wired up, not in +concurrency. + +## Findings + +| # | Severity | Axis | Finding | +|---|---|---|---| +| 1 | **High** | security | No protective middleware is mounted: rate limit, size limit, blacklist and sanitizer all have zero non-test callers, and `MiddlewareConfig` is read by nothing | +| 2 | **High** | security / slowness | `getClientIP` trusts `X-Forwarded-For` unconditionally — the rate limiter is bypassable per-request and its limiter map grows without bound | +| 3 | **High** | security | With `UseProxy: true` the IP blacklist is defeated by one client-supplied header; with it false, behind a proxy, it blocks everyone or no one | +| 4 | **High** | security / panic handling | `PanicRecovery` writes the panic value into the 500 body, and a test asserts it | +| 5 | **High** | security | `Sanitize` pattern-stripping **creates** a `javascript:` URI from input that had none (verified) | +| 6 | **High** | correctness | `EscapeHTML` on query params corrupts every value containing `< > & " '` — `price>100`, `A&B Corp` and all JSON params (verified) | +| 7 | **Medium** | security | Three further verified denylist bypasses: newline in `` | +| 8 | **Medium** | correctness | `cleanupRoutine` flushes every limiter every 5 minutes, handing each client a fresh full burst | +| 9 | **Medium** | security | `MiddlewareWithKeyFunc` falls back to `r.RemoteAddr` **with port**, giving each TCP connection its own bucket | +| 10 | **Medium** | security | Both `StatsHandler`s expose client IPs, remaining budgets and the whole blacklist with no authentication | +| 11 | **Medium** | logging | No log line or metric is emitted when a request is rate-limited, blocked or sanitized — the perimeter has no audit trail | +| 12 | **Medium** | correctness | `UnblockCIDR` silently fails to unblock a non-canonical CIDR while deleting its reason | +| 13 | **Medium** | correctness | `IsBlocked` fails open on an unparseable IP, and stores/compares IPs as raw strings, so IPv6 forms evade blocking | +| 14 | **Medium** | correctness | Byte-slicing `MaxStringLength` and `SanitizeFilename` produces invalid UTF-8 (verified) | +| 15 | **Medium** | correctness | Sanitizing one query param rewrites `RawQuery` via `q.Encode()`, dropping malformed params and reordering the rest | +| 16 | **Medium** | slowness / correctness | `cleanupRoutine` goroutine has no stop, no `recover()`, and leaks per `RateLimiter` | +| 17 | **Low** | correctness | `removeControlCharacters` keeps DEL and the C1 range despite its contract (verified) | +| 18 | **Low** | correctness | 429 responses send JSON with `Content-Type: text/plain` and no `Retry-After` | +| 19 | **Low** | slowness | `IsBlocked` is O(n) in CIDRs per request, with a redundant O(m) reason scan nested inside | +| 20 | **Low** | correctness | `GetAllRateLimitInfo` takes n+1 lock acquisitions | +| 21 | **Low** | security | `SanitizeURL` blocks only two schemes by prefix; `SanitizeFilename` misses encoded traversal and Windows drive prefixes | +| 22 | **Low** | correctness | `getClientIP` mishandles a port-less IPv6 `RemoteAddr` and returns bracketed forms inconsistently | +| 23 | **Low** | maintainability | `//nolint:all` at `ratelimit.go:4` blanket-suppresses linting | + +--- + +### 1. High — the perimeter is not connected + +Searching the module for every constructor in this package: + +| Constructor | Non-test callers | +|---|---| +| `NewRateLimiter` | **0** | +| `NewIPBlacklist` | **0** | +| `NewRequestSizeLimiter` | **0** | +| `DefaultSanitizer` | **0** outside the package (one internal, `sanitize.go:57`) | +| `StrictSanitizer` | **0** | +| `PanicRecovery` | 1 (`pkg/server/manager.go:466`) | + +Only one file outside the package imports it at all — `pkg/server/manager.go` — +and only for panic recovery (`:466`). The configuration that exists to drive the +rest is inert: + +```go +// pkg/config/config.go:121-126 +// MiddlewareConfig holds middleware configuration +type MiddlewareConfig struct { + RateLimitRPS float64 `mapstructure:"rate_limit_rps"` + RateLimitBurst int `mapstructure:"rate_limit_burst"` + MaxRequestSize int64 `mapstructure:"max_request_size"` +} +``` + +All three have defaults (`pkg/config/manager.go:209-211`) and **no reader +anywhere** — grep for `RateLimitRPS`, `RateLimitBurst` and `MaxRequestSize` +outside tests returns only these declarations and `pkg/middleware`'s own +unrelated `DefaultMaxRequestSize` constant. + +**Failure scenario.** Under the stated threat model — a hostile internet client — +the service as assembled has: + +- **No rate limit.** A single client can issue unlimited requests. Every one is a + database round trip through `pkg/restheadspec`, so a trivial loop exhausts the + connection pool (`dbmanager.max_open_conns` defaults to 25, + `pkg/config/manager.go:224`) and the service stops answering for everyone. + There is no other limiter in the stack. +- **No request-size limit.** `max_request_size: 10485760` is configured and + unenforced, so `r.Body` is unbounded. A single `POST` with a multi-gigabyte body + is read into memory by the JSON decoder and OOM-kills the process. This is the + cheapest possible denial of service and the configuration says it is prevented. +- **No IP blacklist**, so an operator has no way to shed a known-bad source. +- **No input sanitization** — less serious, given findings 5–7 argue the + sanitizer should not be mounted in its current form. + +The gap is invisible from the config file, which advertises all three knobs, and +invisible from `pkg/middleware/README.md` (18 KB of documentation for middleware +that is never installed). An operator tuning `rate_limit_rps` downward during an +incident would see no change and reasonably conclude the attack was overwhelming +the limit rather than that no limit exists. + +**Recommendation.** Wire the chain in `pkg/server` from `MiddlewareConfig`, in an +order that matters (outermost first): + +```go +// outermost → innermost +handler = sizeLimiter.Middleware(handler) // cheapest rejection first +handler = rateLimiter.Middleware(handler) +handler = blacklist.Middleware(handler) +handler = middleware.PanicRecovery(handler) // innermost: must see handler panics +``` + +Rationale for the order: the size limit and blacklist are O(1)-ish and should +reject before any expensive work; `PanicRecovery` must be **innermost** of these +so that a panic in the handler is converted to a 500 before the outer layers +observe the response — and note `trackRequestsMiddleware` is applied later still +(`manager.go:540`), so it correctly ends up outside `PanicRecovery`. Fix findings +2 and 3 *before* mounting the IP-based layers, and findings 5–7 before mounting +the sanitizer; mounting them as they stand adds attack surface rather than +removing it. Add a startup log line naming which middleware is active, so the +inert state cannot recur silently. + +--- + +### 2. High — `getClientIP` trusts `X-Forwarded-For` unconditionally + +`ratelimit.go:210-233`: + +```go +func getClientIP(r *http.Request) string { + // Check X-Forwarded-For header (most common in production) + // Format: X-Forwarded-For: client, proxy1, proxy2 + if xff := r.Header.Get("X-Forwarded-For"); xff != "" { + // Take the first IP (the original client) + if idx := strings.Index(xff, ","); idx != -1 { + return strings.TrimSpace(xff[:idx]) + } + return strings.TrimSpace(xff) + } + + // Check X-Real-IP header (used by some proxies like nginx) + if xri := r.Header.Get("X-Real-IP"); xri != "" { + return strings.TrimSpace(xri) + } + ... +} +``` + +There is no trusted-proxy list, no hop counting, and no validation that the +returned string is even an IP address. The value is whatever the client sent, and +it becomes the rate-limiter key at `ratelimit.go:83`. + +**Failure scenario — bypass.** The attacker sends a distinct +`X-Forwarded-For` on every request: + +``` +GET /api/public/orders HTTP/1.1 +X-Forwarded-For: 1.2.3.4 +... +X-Forwarded-For: 1.2.3.5 +``` + +Each value is a new key, so `getLimiter` mints a **fresh** `rate.Limiter` with a +full burst allowance for every request. `limiter.Allow()` on a brand-new limiter +always succeeds, so **the rate limiter never denies anything**. Its entire purpose +is defeated by a header an attacker types once. Note the first-value choice makes +this worse than the usual mistake: when a real proxy *is* in front, the +left-most XFF entry is precisely the one field the client controls end to end, so +the header cannot be trusted even in the deployment it was written for. + +**Failure scenario — amplification.** The same requests grow +`rl.limiters` without bound (`ratelimit.go:59-60`): + +```go +limiter = rate.NewLimiter(rl.rate, rl.burst) +rl.limiters[key] = limiter +``` + +Each entry is a `*rate.Limiter` (~64 bytes) plus the attacker-supplied key string +plus map overhead — call it 150–200 bytes, and the key length is attacker-chosen +up to the header size limit, so it can be far larger. There is no cap on entries +and no per-key validation. A few million requests — minutes of traffic — is +hundreds of megabytes of live heap, and the key is unvalidated so it need not +resemble an IP at all. **The component intended to prevent resource exhaustion +becomes the most efficient way to cause it**, because a single cheap request with +no authentication allocates permanent server memory. Finding 8's five-minute flush +bounds the growth to one interval's worth, which is the only thing standing +between this and a certain OOM. + +**Recommendation.** Only consult proxy headers when the immediate peer is a +trusted proxy, and validate the result: + +```go +type ClientIPConfig struct { + TrustedProxies []*net.IPNet // empty ⇒ never trust XFF/X-Real-IP +} + +func (c *ClientIPConfig) ClientIP(r *http.Request) string { + host, _, err := net.SplitHostPort(r.RemoteAddr) + if err != nil { + host = r.RemoteAddr + } + peer := net.ParseIP(host) + if peer == nil { + return host + } + if !c.isTrusted(peer) { + return peer.String() // normalized; ignore all proxy headers + } + // Walk XFF right-to-left, returning the first address that is NOT a + // trusted proxy — that is the furthest hop we can actually vouch for. + parts := strings.Split(r.Header.Get("X-Forwarded-For"), ",") + for i := len(parts) - 1; i >= 0; i-- { + ip := net.ParseIP(strings.TrimSpace(parts[i])) + if ip != nil && !c.isTrusted(ip) { + return ip.String() + } + } + return peer.String() +} +``` + +Three properties matter and all three are missing today: **trust is opt-in** +(default deny), the scan is **right-to-left** so the client cannot inject hops, +and the result is **normalized through `net.ParseIP`** so it is a canonical +address and nothing else. Independently, cap `rl.limiters` (evict LRU past N) so +a key-space bug can never again be a memory leak, and add the same +trusted-proxy plumbing to `blacklist.go` (finding 3). Surface `TrustedProxies` in +`MiddlewareConfig`. + +--- + +### 3. High — the IP blacklist is defeated by a client header + +`blacklist.go:154-169`: + +```go +func (bl *IPBlacklist) Middleware(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var clientIP string + if bl.useProxy { + clientIP = getClientIP(r) + // Clean up IPv6 brackets if present + clientIP = strings.Trim(clientIP, "[]") + } else { + // Extract IP from RemoteAddr + if idx := strings.LastIndex(r.RemoteAddr, ":"); idx != -1 { + clientIP = r.RemoteAddr[:idx] + } else { + clientIP = r.RemoteAddr + } + clientIP = strings.Trim(clientIP, "[]") + } + ... +``` + +**Failure scenario.** `UseProxy` is a single boolean +(`blacklist.go:23-26`) and both settings are wrong without a trusted-proxy list: + +- **`UseProxy: true`** — the address comes from `getClientIP`, so a blocked + attacker sends `X-Forwarded-For: 203.0.113.9` and is no longer blocked. The + blacklist is a **security control that any client can switch off by naming a + header**, and because `IsBlocked` also fails open on an unparseable address + (finding 13), even `X-Forwarded-For: x` suffices. Nothing is logged when a block + is evaded, or at all (finding 11), so an operator watching the blacklist "work" + sees blocked counts of zero and no indication why. +- **`UseProxy: false` behind a proxy** — every request carries the proxy's + address, so the blacklist either blocks nothing (the proxy is not listed) or + blocks **all traffic at once** the moment the proxy's IP is added. Blocking one + abusive client is impossible. + +Since a deployment behind a load balancer or CDN is the normal case for an +internet-facing API, the correct configuration does not exist. + +**Recommendation.** Replace `UseProxy bool` with the `TrustedProxies` plumbing +from finding 2 and share one `ClientIP` implementation between both middlewares — +the two files currently disagree about bracket handling and port stripping, which +is itself a source of mismatch between "the IP we rate-limit" and "the IP we +block". Normalize through `net.ParseIP(...).String()` before both storing and +comparing (finding 13), and log every block at `Info` with the matched rule +(finding 11). + +--- + +### 4. High — the panic value is returned to the client, and a test locks it in + +`panic.go:14-33` is the only middleware actually mounted: + +```go +func PanicRecovery(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + defer func() { + if rcv := recover(); rcv != nil { + // Record the panic metric + metrics.GetProvider().RecordPanic(panicMiddlewareMethodName) + ... + ctx := r.Context() + err := logger.HandlePanic(panicMiddlewareMethodName, rcv, ctx) + + // Respond with a 500 error + http.Error(w, err.Error(), http.StatusInternalServerError) + } + }() + next.ServeHTTP(w, r) + }) +} +``` + +`logger.HandlePanic` returns `fmt.Errorf("panic in %s: %v", methodName, r)` +(`pkg/logger/logger.go:210`), so the **raw panic value is written into the +response body**. + +**Failure scenario.** An attacker probes endpoints until something panics, then +reads the internals out of the 500 body: + +- `panic in PanicMiddleware: runtime error: index out of range [5] with length 3` + — confirms an exploitable parsing path and gives exact bounds. +- A panicking database driver leaks the failing SQL, and `pq`/`pgx` errors can + carry the connection string — including credentials, since + `dbmanager.connections.default.password` is part of the DSN. +- `runtime error: invalid memory address or nil pointer dereference` maps + attacker input to specific unguarded code paths, turning blind probing into a + guided search. + +Because panics are reachable from request parsing, this is a repeatable oracle +rather than a one-off. `pkg/restheadspec` and the adapters recover in several +places, so the panics that reach here are the unanticipated ones — exactly those +whose messages are most revealing. + +The leak is **deliberate and test-locked** — `panic_test.go:57`: + +```go +assert.Contains(t, rr.Body.String(), "panic in PanicMiddleware: something went terribly wrong", "expected error message in response body") +``` + +so any fix must change this assertion. That matters: a reviewer changing +`panic.go` would see a test failure and assume they had broken something. + +**Mitigation that already exists.** `pkg/server` lets a caller supply +`PanicHandler` (`pkg/server/interfaces.go:47`), used in preference to this +middleware (`manager.go:456-466`). An embedder can therefore avoid the leak — but +it is opt-out, not opt-in, so the insecure path is the default. + +**Recommendation.** Log in full, respond with nothing: + +```go +if rcv := recover(); rcv != nil { + metrics.GetProvider().RecordPanic(panicMiddlewareMethodName) + _ = logger.HandlePanic(panicMiddlewareMethodName, rcv, r.Context()) + http.Error(w, "Internal Server Error", http.StatusInternalServerError) +} +``` + +Update `panic_test.go:57` to assert the body does **not** contain the panic text, +which converts the test from locking in the bug to guarding the fix. Consider +emitting a correlation ID in both the log line and the response body so support +can still tie a report to a stack trace without disclosing it. + +Two further defects in the same nine lines: + +- **`http.Error` after a partial response.** If the handler wrote a 200 and + streamed bytes before panicking, `WriteHeader(500)` is ignored ("superfluous + WriteHeader" is logged by `net/http`) and the error text is appended **into the + middle of the response body**. The client receives a 200 with corrupt trailing + content — worse than a clean failure, because caches and clients treat it as + valid. Track whether anything was written (see `metrics.audit.md` finding 11) + and, when it was, abandon the connection with `panic(http.ErrAbortHandler)` + instead. +- **`RecordPanic` goes nowhere.** `metrics.GetProvider()` returns a + `NoOpProvider` because nothing ever installs a provider + (`metrics.audit.md` finding 1), so `panics_total` never increments. This is the + *only* caller of `RecordPanic` in the repository, so panic counting is entirely + non-functional. + +--- + +### 5. High — the sanitizer manufactures a `javascript:` URI + +`sanitize.go:80-85` applies each block pattern exactly once: + +```go + // Check block patterns + for _, pattern := range s.BlockPatterns { + if pattern.MatchString(value) { + // Replace matched pattern with empty string + value = pattern.ReplaceAllString(value, "") + } + } +``` + +Removing a substring can **join its neighbours into a new match**, and because +each pattern runs once there is no second pass to catch the result. I verified +this against a faithful reimplementation of `DefaultSanitizer` +(`sanitize.go:35-53`): + +| Input | Output | +|---|---| +| `javajavascript:script:alert(1)` | **`javascript:alert(1)`** | + +Removing the inner `javascript:` leaves `java` + `script:alert(1)` — a working +`javascript:` URI. **The sanitizer produced the exact payload it exists to +block, from input that did not contain it.** + +**Failure scenario.** An attacker submits +`javajavascript:script:alert(1)` as a profile URL, comment or any stored string. +Every layer that inspects the input sees a harmless string with no `javascript:` +substring — a WAF, a manual review, a downstream denylist. The sanitizer then +converts it into a live XSS payload, which is stored and later rendered into an +`href`. The control inverts: input that would have been rejected as suspicious is +laundered into something dangerous, and the transformation happens inside the +component whose job is to prevent it. Note the HTML-escaping step +(`sanitize.go:93-94`) does not help here, because the resulting string contains no +HTML metacharacters to escape. + +**Recommendation.** Do not sanitize by deletion. Two changes, in order of +importance: + +1. **Reject rather than repair.** If a pattern matches, fail the request with + 400 — never edit the value and continue. Editing guarantees this class of bug + and silently changes user data (finding 11 means nobody finds out). +2. **Stop relying on a denylist for XSS.** Escape at the point of *output*, + per context (HTML body, attribute, JS, URL), which is the only place the + correct encoding is known. For rich text use a real allowlist sanitizer with a + parser — `bluemonday` — instead of regexes over a string. Regex-based HTML + filtering cannot be made correct; findings 7's bypasses are symptoms of that, + not fixable individually. + +If deletion must be kept as a stopgap, loop each pattern to a fixed point +(`for pattern.MatchString(v) { v = pattern.ReplaceAllString(v, "") }`) with an +iteration cap — but note this makes the function O(n·k) on attacker-controlled +input and still does not make the denylist complete. + +--- + +### 6. High — HTML-escaping query parameters corrupts ordinary data + +`DefaultSanitizer` sets `EscapeHTML: true` (`sanitize.go:38`), applied at +`sanitize.go:93-94`: + +```go + // Escape HTML entities + if s.EscapeHTML && !s.StripHTML { + value = html.EscapeString(value) + } +``` + +and `Middleware` runs `Sanitize` over **every query parameter value** +(`sanitize.go:145-159`) and four headers (`:163-177`), writing the results back +into the request. Verified outputs: + +| Input | Output | +|---|---| +| `price>100` | `price>100` | +| `A&B Corp` | `A&B Corp` | +| `{"filter":"name = 'O'Brien'"}` | `{"filter":"name = 'O'Brien'"}` | + +**Failure scenario.** This project's API is query- and header-driven: filters, +sort specifications and expand options arrive as parameters, and +`pkg/restheadspec` carries structured options in headers. With this middleware +mounted: + +- A filter `price>100` reaches the handler as `price>100` and either fails to + parse or is treated as a literal — the query silently returns the wrong rows. +- A customer named `A&B Corp` is **written to the database** as `A&B Corp`. + The corruption is persistent, silent, and compounds on each edit + (`&amp;`…). There is no log line (finding 11), so the first report is a user + asking why their company name looks wrong. +- Any JSON-valued parameter has its quotes turned into `"` and becomes + **unparseable**, so requests fail with a decoding error that names the JSON, not + the middleware. + +This is output encoding applied at input time — the canonical mistake. It does +not prevent XSS (the escape is applied again, or undone, at render time, +producing double-escaping or none) and it destroys data integrity for every +non-HTML consumer, which here is all of them. + +**Recommendation.** Set `EscapeHTML: false` in `DefaultSanitizer` and escape in +the template or serializer that renders the value, where the output context is +known. If a stored-XSS defense is wanted at ingress, validate and reject +(finding 5) rather than transform. Note `StrictSanitizer` (`sanitize.go:56-61`) +is worse still: it sets `StripHTML: true`, so `<` in a legitimate value silently +deletes everything up to the next `>`. + +--- + +### 7. Medium — three further verified denylist bypasses + +Also verified against `DefaultSanitizer`'s patterns +(`sanitize.go:44-51`) — all three reach the handler with the script intact +(HTML-escaped only, which finding 6 shows is not a defense and is undone +elsewhere): + +| Input | Result | Cause | +|---|---|---| +| `` | **not matched** | `.` does not match `\n` without the `(?s)` flag, so `]*>.*?` cannot span lines | +| ``; an external-source tag has none | +| `` | **not matched** | `` is matched literally, so a space before `>` evades it | + +The newline case is the most serious: a multi-line `` +produces `<scr`, which is safe here but demonstrates the same +removal-creates-new-text mechanism as finding 5. + +**Failure scenario.** Any of the three is stored and rendered, giving stored XSS +in an application whose operators believe input is sanitized — the false +assurance is the harm, because it displaces real output encoding. + +**Recommendation.** As finding 5: reject instead of strip, and replace the regex +set with an allowlist parser. If the patterns are kept for defense in depth, add +`(?s)` and make the closing-tag matching tolerant (``), while +treating the list as best-effort and never as the control. + +--- + +### 8. Medium — the cleanup routine hands every client a fresh burst + +`ratelimit.go:65-76`: + +```go +func (rl *RateLimiter) cleanupRoutine() { + ticker := time.NewTicker(rl.cleanup) + defer ticker.Stop() + + for range ticker.C { + rl.mu.Lock() + // Simple cleanup: remove all limiters + // In production, you might want to track last access time + rl.limiters = make(map[string]*rate.Limiter) + rl.mu.Unlock() + } +} +``` + +The interval is hard-coded to 5 minutes (`:32`) with no way to configure it. + +**Failure scenario.** This does not evict *stale* limiters — it discards **all** +of them, including those belonging to clients currently being throttled. A client +that has exhausted its bucket gets a brand-new limiter with a full `burst` +allowance (default 200, `pkg/config/manager.go:210`) at the next tick. An attacker +who simply waits out the interval receives 200 free requests every 5 minutes on +top of the sustained rate, indefinitely, and the penalty for abuse resets on a +predictable schedule. Because the flush is unconditional, **the harder a client is +being throttled, the more it gains** from each tick. + +The comment acknowledges the design is provisional; the consequence is that the +limiter's long-run behaviour is not the configured rate. + +**Recommendation.** Evict by last use, not wholesale: + +```go +type entry struct { + limiter *rate.Limiter + lastSeen atomic.Int64 // unix nanos, updated in getLimiter +} + +for range ticker.C { + cutoff := time.Now().Add(-idleTTL).UnixNano() + rl.mu.Lock() + for k, e := range rl.limiters { + if e.lastSeen.Load() < cutoff { + delete(rl.limiters, k) + } + } + rl.mu.Unlock() +} +``` + +An idle limiter at full tokens is indistinguishable from no limiter, so evicting +only idle entries is both safe and sufficient. Deleting in place also avoids +handing the whole old map to the GC at once. Make the interval and TTL +configurable, and pair this with the hard entry cap from finding 2 — eviction by +age alone does not bound a burst of new keys within one interval. + +--- + +### 9. Medium — `MiddlewareWithKeyFunc` falls back to a per-connection key + +`ratelimit.go:97-115`: + +```go + key := keyFunc(r) + if key == "" { + key = r.RemoteAddr + } +``` + +`r.RemoteAddr` is `"ip:port"` — **including the ephemeral source port** — unlike +`getClientIP`, which strips it (`ratelimit.go:228-230`). + +**Failure scenario.** Whenever the supplied `keyFunc` returns `""` — for an +unauthenticated request if it keys on user ID, for a request missing the chosen +header, or for every request if the function has a bug — the key becomes unique +**per TCP connection**. A client that opens a new connection per request (trivial: +`Connection: close`, or any non-pooling HTTP client) therefore gets a fresh +limiter every time and is never limited. The failure is silent and +input-dependent: the limiter appears to work for authenticated traffic and +disappears for exactly the anonymous traffic that most needs limiting. + +**Recommendation.** Use the same normalized client IP as the default path, and +make the fallback explicit: + +```go +key := keyFunc(r) +if key == "" { + key = "ip:" + clientIP(r) // shared, port-stripped, trusted-proxy aware +} +``` + +Prefixing by key type (`ip:`, `user:`) also prevents a user ID from colliding with +an IP string in the shared map — worth doing regardless. + +--- + +### 10. Medium — both stats handlers are unauthenticated + +`ratelimit.go:175-206` and `blacklist.go:195-212` return JSON with no +authentication, no authorization and no way to require any: + +```go +// ratelimit.go:191-198 + stats := map[string]interface{}{ + "total_tracked_ips": len(allInfo), + "rate_limit_config": map[string]interface{}{ + "requests_per_second": float64(rl.rate), + "burst": rl.burst, + }, + "tracked_ips": allInfo, + } +``` + +**Failure scenario.** If either is routed — and the doc comment invites it, +"Example: GET /rate-limit-stats" (`ratelimit.go:174`) — any client learns: + +- **Every client IP currently using the service**, with remaining token counts + (`RateLimitInfo`, `:118-123`). That is personal data about other users + disclosed to an anonymous third party, and in most jurisdictions an IP address + tied to activity is regulated. +- **The exact limiter configuration** — `requests_per_second` and `burst` — plus, + via `?ip=` (`:178`), the attacker's **own remaining budget in real time**. That + converts rate-limit evasion from guesswork into a control loop: poll the + endpoint, stay one token below the threshold, never get a 429. +- From the blacklist handler, **the complete set of blocked IPs and CIDRs** with + their `reason` strings (`blacklist.go:199-204`). An attacker learns which + ranges to avoid and which proxies remain usable, and the reasons are free-text + operator notes that may name incidents, customers or internal tooling. + +**Recommendation.** Do not ship unauthenticated introspection of a security +control. Require authentication and authorization at the route, bind these to an +internal-only listener (`servers.instances.*`, `pkg/config/manager.go:182-186`) as +recommended for `/metrics` in `metrics.audit.md` finding 4, and drop `tracked_ips` +from the default payload — the aggregate count is enough for a dashboard. If +per-IP detail is needed, gate it behind an explicit admin scope and log each +access. + +--- + +### 11. Medium — the perimeter emits no audit trail + +Neither rate limiting nor blacklisting nor sanitization logs anything on the path +that matters. `ratelimit.go:87-90`: + +```go + if !limiter.Allow() { + http.Error(w, `{"error":"rate_limit_exceeded","message":"Too many requests"}`, http.StatusTooManyRequests) + return + } +``` + +`blacklist.go:171-188` likewise writes a 403 with no log line — the only +`logger` call in either file is a `Debug` on a **JSON encoding failure** +(`blacklist.go:185`, `ratelimit.go:183`, `:203`). `sanitize.go` imports no logger +at all and modifies request data silently. No middleware in the package records a +metric; `panic.go:19` is the package's only `metrics` call, and it reaches a no-op +(finding 4). + +**Failure scenario.** During an attack an operator cannot answer the first +questions asked: is the rate limiter firing, for which clients, and at what rate? +Nothing distinguishes "no attack" from "limiter bypassed via finding 2" from +"limiter not mounted at all" (finding 1) — all three produce identical silence. +There is no signal to alert on, no data to tune `rate_limit_rps` with, and no +forensic record of which addresses were blocked or why. For the blacklist, an +operator adding an entry gets no confirmation that it ever matched. And because +the sanitizer edits values without logging, the data corruption in finding 6 is +undiagnosable from the server side — the request that arrived and the value that +was stored differ, with nothing recording the difference. + +**Recommendation.** Log every enforcement action at `Info` with the client IP, the +matched rule and the request path, and add counters: + +```go +if !limiter.Allow() { + logger.Info("rate limit exceeded: client=%s path=%s rps=%v burst=%d", key, r.URL.Path, rl.rate, rl.burst) + metrics.GetProvider().RecordRateLimited(key) // new interface method + ... +} +``` + +Log at `Info`, not `Warn`: per `_CROSS-CUTTING.audit.md` finding X8 every `Warn` +is forwarded to the error tracker, so logging a rate-limit event at `Warn` would +turn a volumetric attack into an equal-volume flood of Sentry events — a second +outage caused by the instrumentation. The `metrics.Provider` interface has no +method for this today (`pkg/metrics/interfaces.go:12-48`); adding +`RecordRateLimited`/`RecordIPBlocked` is the right place, and note the label must +not be the raw client IP unless bounded — see `metrics.audit.md` findings 2 and 5. + +--- + +### 12. Medium — `UnblockCIDR` cannot unblock a non-canonical range + +`blacklist.go:82-94`: + +```go +func (bl *IPBlacklist) UnblockCIDR(cidr string) { + bl.mu.Lock() + defer bl.mu.Unlock() + + // Find and remove the CIDR + for i, ipNet := range bl.cidrs { + if ipNet.String() == cidr { + bl.cidrs = append(bl.cidrs[:i], bl.cidrs[i+1:]...) + break + } + } + delete(bl.reason, cidr) +} +``` + +`BlockCIDR` stores the parsed network in `bl.cidrs` but files the reason under the +**caller's original string** (`:65-68`), while `UnblockCIDR` compares against +`ipNet.String()` — the *canonical* form. + +**Failure scenario.** An operator blocks `BlockCIDR("10.0.0.1/8", "abuse")`. +`net.ParseCIDR` normalizes the network to `10.0.0.0/8`, so `bl.cidrs` holds +`10.0.0.0/8` while `bl.reason` holds the key `10.0.0.1/8`. Calling +`UnblockCIDR("10.0.0.1/8")` then: + +- fails the comparison (`"10.0.0.0/8" != "10.0.0.1/8"`), so **the range stays + blocked**, and +- succeeds at `delete(bl.reason, cidr)`, so the reason is erased. + +The operator's own input, echoed back verbatim, does not undo their own action; +the range remains blocked with no recorded reason, and nothing is logged or +returned — `UnblockCIDR` has no error return. Restoring service to a wrongly +blocked customer requires knowing to pass the canonical form, which is never +displayed. `GetBlacklist` reports `ipNet.String()` (`:147`), so the value shown to +the operator *is* the one that works — but the value they typed is not, and no +message connects the two. + +The same mismatch makes the reason unreachable in `IsBlocked`, which looks it up +by `ipNet.String()` (`:114-118`): a non-canonically-blocked range is reported with +an empty reason, which is what the dead fallback loop at `:120-124` was evidently +meant to fix. That loop cannot work — it tests `key == cidr` after the map lookup +for exactly that key already failed — and `if i < len(bl.cidrs)` at `:126` is +always true inside `range bl.cidrs`, so the whole block reduces to +`return true, ""`. + +**Recommendation.** Canonicalize on the way in and key everything consistently: + +```go +func (bl *IPBlacklist) BlockCIDR(cidr, reason string) error { + _, ipNet, err := net.ParseCIDR(cidr) + if err != nil { + return err + } + key := ipNet.String() // canonical, for both maps + bl.mu.Lock() + defer bl.mu.Unlock() + bl.cidrs = append(bl.cidrs, ipNet) + if reason != "" { + bl.reason[key] = reason + } + return nil +} +``` + +and have `UnblockCIDR` parse its argument the same way, returning an error when +the CIDR is invalid or not present, so a failed unblock is visible. Delete the +dead loop at `:120-128`. Storing `cidrs` as a map keyed by the canonical string +would remove the O(n) removal and the possibility of duplicate entries at the same +time. + +--- + +### 13. Medium — `IsBlocked` fails open, and IPs are compared as raw strings + +`blacklist.go:97-133`: + +```go + // Check individual IPs + if bl.ips[ip] { + return true, bl.reason[ip] + } + + // Check CIDR ranges + parsedIP := net.ParseIP(ip) + if parsedIP == nil { + return false, "" + } +``` + +Two problems. **Fail-open on unparseable input**: when `net.ParseIP` fails the +function returns `false` — allow. **String-keyed exact matching**: `BlockIP` +stores the caller's string verbatim (`:48`) and the lookup at `:102` is a raw map +hit, so the textual form must match exactly. + +**Failure scenario — fail-open.** With `UseProxy: true` (finding 3), +`getClientIP` returns arbitrary attacker text. `X-Forwarded-For: blocked` is not +in `bl.ips`, does not parse as an IP, and therefore **returns allow** — the CIDR +checks are skipped entirely. Any garbage value bypasses every range rule. A +security control should fail closed on input it cannot interpret, or at minimum +log and fall back to `RemoteAddr`; this does neither, silently. + +**Failure scenario — IPv6 evasion.** `BlockIP("2001:db8::1", …)` stores that +exact string. The same host presenting `2001:0db8:0:0:0:0:0:1` or +`2001:DB8::1` — all valid textual forms of one address — produces a different map +key, misses, and (being parseable) is only caught if a CIDR happens to cover it. +IPv4-mapped forms (`::ffff:192.0.2.1` vs `192.0.2.1`) diverge the same way. +Blocking a single IPv6 address is therefore unreliable in a way that is invisible +in testing, because tests naturally reuse the same string on both sides. + +**Recommendation.** Normalize once, at both ends, and fail closed: + +```go +func normalizeIP(s string) (string, bool) { + ip := net.ParseIP(strings.Trim(strings.TrimSpace(s), "[]")) + if ip == nil { + return "", false + } + return ip.String(), true // canonical form for map keys +} +``` + +Use it in `BlockIP`, `UnblockIP` and `IsBlocked`. When the address cannot be +parsed, log it and decide deliberately — for a blacklist, treating an +uninterpretable client address as blocked is the defensible default, and is only +safe to do once finding 2's trusted-proxy handling guarantees the value comes +from the connection rather than a header. + +--- + +### 14. Medium — byte-slicing truncation produces invalid UTF-8 + +`sanitize.go:97-100`: + +```go + // Apply max length + if s.MaxStringLength > 0 && len(value) > s.MaxStringLength { + value = value[:s.MaxStringLength] + } +``` + +and `SanitizeFilename` (`:217-219`) does the same at 255. `len()` counts **bytes** +and the slice cuts at a byte offset, so a multi-byte rune can be split. Verified: +truncating `"héllo wörld"` at 2 yields `"h\xc3"` — not valid UTF-8. + +**Failure scenario.** `StrictSanitizer` sets `MaxStringLength: 10000` +(`sanitize.go:59`), so any string field at the limit whose 10 000th byte falls +inside a multi-byte character is corrupted. Consequences downstream: + +- `encoding/json` replaces the invalid byte with U+FFFD when marshalling, so the + stored value silently gains a replacement character. +- PostgreSQL **rejects** invalid UTF-8 outright (`invalid byte sequence for + encoding "UTF8"`), so the insert fails with a 500 that names an encoding + problem, pointing an engineer at the database rather than at this middleware. + +Either way the trigger is a specific input length combined with non-ASCII text, so +it reproduces for some users and never for others. Any non-Latin script hits the +limit sooner and more often, so the bug lands hardest on non-English data. + +**Recommendation.** Truncate on rune boundaries, and count what the limit is meant +to mean: + +```go +if s.MaxStringLength > 0 && utf8.RuneCountInString(value) > s.MaxStringLength { + runes := []rune(value) + value = string(runes[:s.MaxStringLength]) +} +``` + +If the limit is genuinely a byte budget (a database column width), keep `len` but +back off to the last valid boundary, e.g. with +`for !utf8.ValidString(value) { value = value[:len(value)-1] }` or +`utf8.DecodeLastRuneInString`. Apply the same fix at `sanitize.go:218`. + +--- + +### 15. Medium — sanitizing one parameter rewrites the whole query string + +`sanitize.go:142-160`: + +```go + if r.URL.RawQuery != "" { + q := r.URL.Query() + sanitized := false + for key, values := range q { + for i, value := range values { + sanitizedValue := s.Sanitize(value) + if sanitizedValue != value { + values[i] = sanitizedValue + sanitized = true + } + } + if sanitized { + q[key] = values + } + } + if sanitized { + r.URL.RawQuery = q.Encode() + } + } +``` + +**Failure scenario.** A single modified value triggers +`r.URL.RawQuery = q.Encode()`, which re-serializes the query from the parsed map. +That loses information `url.Values` cannot represent: + +- **Malformed pairs are discarded.** `r.URL.Query()` drops any pair with invalid + percent-encoding (and returns an error the code never checks). Those parameters + vanish from the rewritten query, so a request that would have failed validation + visibly instead proceeds with fields **missing**. +- **Bare keys gain `=`.** `?flag` re-encodes as `flag=`, changing presence + semantics for any handler distinguishing the two. +- **Order is destroyed.** `Encode` sorts keys alphabetically, so any handler + reading repeated parameters positionally sees a different request. + +Given finding 6 — where `EscapeHTML` alters almost any value containing `&`, `<`, +`>` or a quote — the `sanitized` flag is set on most real requests, so this +rewrite is the common path rather than the exception. The `sanitized` flag is also +never reset between keys (it is declared outside the `for key` loop at `:144`), so +once any value changes, `q[key] = values` executes for every later key too; that +assignment is harmless — `values` already aliases the map's slice — but it shows +the flag is not doing what it appears to. + +**Recommendation.** Do not rewrite `RawQuery`. Under finding 5's recommendation +the middleware rejects rather than edits, which removes this code path entirely. +If values must be modified, attach the sanitized `url.Values` to the request +context and have handlers read from there, leaving `r.URL` untouched: + +```go +ctx := context.WithValue(r.Context(), sanitizedQueryKey{}, q) +next.ServeHTTP(w, r.WithContext(ctx)) +``` + +Also check the error from `url.ParseQuery` and reject a malformed query string +outright instead of silently discarding parameters. + +--- + +### 16. Medium — the cleanup goroutine cannot be stopped and has no recover + +`NewRateLimiter` starts a goroutine (`ratelimit.go:36`) with no corresponding +`Stop`/`Close`: + +```go + // Start cleanup goroutine + go rl.cleanupRoutine() +``` + +`cleanupRoutine` loops `for range ticker.C` forever (`:69`); the +`defer ticker.Stop()` at `:67` is unreachable because the loop has no exit. There +is no `recover()` in the goroutine. + +**Failure scenario.** Every `RateLimiter` ever constructed leaks one goroutine and +one live `time.Ticker` for the process lifetime. With a single limiter created at +startup that is negligible — but the type is constructed per configuration, and +any test suite, config reload, or per-tenant limiter accumulates them. The +goroutine also retains the whole `RateLimiter`, so every limiter map it ever held +stays reachable and unreclaimable. + +The missing `recover()` is the more serious half: a panic anywhere in this +goroutine — today only a map operation, but any future eviction logic — crashes +the **entire process**, because a panic in a goroutine cannot be recovered by +`PanicRecovery` or by any handler in the stack. This is the pattern flagged in +`_CROSS-CUTTING.audit.md` finding X7. + +**Recommendation.** Give it a lifecycle and a guard: + +```go +func (rl *RateLimiter) cleanupRoutine() { + defer logger.CatchPanicCallback("middleware.cleanupRoutine", nil)() + ticker := time.NewTicker(rl.cleanup) + defer ticker.Stop() + for { + select { + case <-ticker.C: + rl.evictIdle() // finding 8 + case <-rl.stop: + return + } + } +} + +// Close stops the cleanup goroutine. Safe to call more than once. +func (rl *RateLimiter) Close() { + rl.stopOnce.Do(func() { close(rl.stop) }) +} +``` + +Use `sync.Once` so a double `Close` cannot panic — the defect +`metrics.audit.md` finding 8 records for `StopAutoPush`. Have `pkg/server` call +`Close` during shutdown once the limiter is wired (finding 1). + +--- + +### 17. Low — `removeControlCharacters` keeps DEL and the C1 range + +`sanitize.go:186-195`: + +```go +// removeControlCharacters removes control characters except \n, \r, \t +func removeControlCharacters(s string) string { + var result strings.Builder + for _, r := range s { + // Keep newline, carriage return, tab, and non-control characters + if r == '\n' || r == '\r' || r == '\t' || r >= 32 { + result.WriteRune(r) + } + } + return result.String() +} +``` + +`r >= 32` admits everything above the C0 block. Verified: `\x7f` (DEL) and +`U+0085` (NEL, a C1 control) both survive, contradicting the doc comment. + +**Failure scenario.** Minor in isolation. DEL and C1 controls reaching logs can +corrupt or forge log lines — `U+0085` is a line terminator to some log processors, +enabling log injection by an attacker who controls a sanitized field. The function +also passes Unicode characters that matter more than C1 controls: U+202E +(right-to-left override) for filename and display spoofing, and zero-width +characters (U+200B, U+FEFF) for filter evasion and homograph tricks. The contract +says control characters are removed, so callers reasonably trust it. + +**Recommendation.** Use the standard predicate and extend to the formatting +category: + +```go +for _, r := range s { + switch { + case r == '\n' || r == '\t' || r == '\r': + result.WriteRune(r) + case unicode.IsControl(r), unicode.Is(unicode.Cf, r): // Cf: bidi + zero-width + // drop + default: + result.WriteRune(r) + } +} +``` + +`unicode.IsControl` covers both C0 and C1; `unicode.Cf` covers the bidi overrides +and zero-width characters. Also consider normalizing to NFC. + +--- + +### 18. Low — 429 responses are mislabelled and omit `Retry-After` + +`ratelimit.go:88` and `:108`: + +```go + http.Error(w, `{"error":"rate_limit_exceeded","message":"Too many requests"}`, http.StatusTooManyRequests) +``` + +`http.Error` sets `Content-Type: text/plain; charset=utf-8` and appends a newline, +so a JSON body is served declared as plain text. There is no `Retry-After`. + +**Failure scenario.** A client that dispatches on `Content-Type` — the correct +behaviour — treats the body as text and cannot read the `error` code, so it +surfaces a generic failure instead of "rate limited". Without `Retry-After`, a +well-behaved client has no idea how long to wait and will typically retry +immediately, so the limiter produces **more** load from compliant clients than it +would with the header. `rate.Limiter` can supply the delay directly, and the +blacklist path does this correctly (`blacklist.go:181-183` sets the header and +uses `json.NewEncoder`), so the package is internally inconsistent. + +**Recommendation.** Mirror the blacklist's approach and include the delay: + +```go +res := limiter.Reserve() +if !res.OK() || res.Delay() > 0 { + if d := res.Delay(); d > 0 { + res.Cancel() + w.Header().Set("Retry-After", strconv.Itoa(int(math.Ceil(d.Seconds())))) + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusTooManyRequests) + _ = json.NewEncoder(w).Encode(map[string]string{ + "error": "rate_limit_exceeded", "message": "Too many requests", + }) + return +} +``` + +Note `Reserve` must be `Cancel`ed when rejecting, or the token is consumed twice. +Adding `X-RateLimit-Limit`/`-Remaining` is conventional, but see finding 10 — they +disclose budget state, so expose them only to authenticated clients. + +--- + +### 19. Low — `IsBlocked` is O(n) per request with a redundant nested scan + +`blacklist.go:112-130` walks every CIDR on every request under `RLock`, and for +each *match* additionally iterates the whole `reason` map (`:120-124`) in a loop +that cannot succeed (finding 12). + +**Failure scenario.** A sizeable blacklist — a few thousand ranges from a threat +feed is ordinary — means a few thousand `ipNet.Contains` calls per request, each +allocating nothing but branching over 4 or 16 bytes. Measurable but not fatal; +the concern is that the work happens while holding `RLock`, so it contends with +`BlockIP`/`BlockCIDR` writers, and grows linearly with a list operators are +encouraged to extend. Coupled with finding 8's write-lock flush in the rate +limiter, a large deployment sees both maps serialized on the request path. + +**Recommendation.** Use a prefix-trie lookup (`cidranger`, or +`netipx.IPSet` with `net/netip`) for O(prefix-length) matching instead of O(n), +and drop the nested reason scan. Migrating to `net/netip` also removes the +per-request `net.IP` allocation that `net.ParseIP` makes today. + +--- + +### 20. Low — `GetAllRateLimitInfo` takes n+1 lock acquisitions + +`ratelimit.go:162-171`: + +```go +func (rl *RateLimiter) GetAllRateLimitInfo() []*RateLimitInfo { + ips := rl.GetTrackedIPs() + info := make([]*RateLimitInfo, 0, len(ips)) + + for _, ip := range ips { + info = append(info, rl.GetRateLimitInfo(ip)) + } + + return info +} +``` + +`GetTrackedIPs` takes `RLock` once (`:127`), then `GetRateLimitInfo` takes it +again per IP (`:139`). + +**Failure scenario.** Serving the stats endpoint acquires and releases the read +lock once per tracked IP. Reads do not block each other, but each acquisition +contends with the write lock, and under finding 2 the map can hold millions of +entries — so one stats request performs millions of lock operations while +`getLimiter` writers and finding 8's flush are trying to acquire the write lock. +An unauthenticated endpoint (finding 10) that scales its own cost with +attacker-controlled map size is a small amplification primitive. Entries added +between the snapshot and the per-IP read are also reported with default values, so +the output is mildly inconsistent. + +**Recommendation.** Collect everything under one `RLock`: + +```go +func (rl *RateLimiter) GetAllRateLimitInfo() []*RateLimitInfo { + rl.mu.RLock() + defer rl.mu.RUnlock() + info := make([]*RateLimitInfo, 0, len(rl.limiters)) + for ip, l := range rl.limiters { + info = append(info, &RateLimitInfo{ + IP: ip, TokensRemaining: l.Tokens(), + Limit: float64(rl.rate), Burst: rl.burst, + }) + } + return info +} +``` + +and bound or paginate the result. + +--- + +### 21. Low — `SanitizeURL` and `SanitizeFilename` are easily evaded + +`sanitize.go:236-251`: + +```go + // Block javascript: and data: protocols + if strings.HasPrefix(strings.ToLower(url), "javascript:") { + return "" + } + if strings.HasPrefix(strings.ToLower(url), "data:") { + return "" + } +``` + +Only `\x00` is stripped beforehand (`:240`), so `"\x01javascript:alert(1)"` fails +both prefix tests and is returned unchanged — browsers ignore the leading control +character and execute it. `vbscript:`, `file:` and `blob:` are not covered, and a +scheme-relative `//evil.com` passes. `SanitizeFilename` (`:207-222`) removes `..`, +`/` and `\` textually, so it misses percent-encoded traversal (`%2e%2e%2f`, since +nothing decodes) and Windows drive prefixes (`C:`), while mangling legitimate +names containing `..`. + +**Failure scenario.** A stored URL passes `SanitizeURL` and is rendered into an +`href`, giving XSS on click. An uploaded filename passes `SanitizeFilename` in +encoded form and escapes the intended directory once some later layer decodes it. +Both functions are exported helpers, so callers reasonably treat them as +sufficient — that assumption is the risk, more than the specific gaps. + +**Recommendation.** Parse rather than pattern-match. For URLs, use +`net/url.Parse` and allowlist the scheme: + +```go +u, err := url.Parse(strings.TrimSpace(raw)) +if err != nil || (u.Scheme != "http" && u.Scheme != "https") { + return "" +} +``` + +For filenames, take `filepath.Base` of the **decoded** value and validate the +result against `^[A-Za-z0-9._-]{1,255}$`, rejecting `.`/`..` explicitly, rather +than deleting substrings. Never construct a path by removing characters. + +--- + +### 22. Low — `getClientIP` mishandles port-less and bracketed addresses + +`ratelimit.go:228-232`: + +```go + if idx := strings.LastIndex(r.RemoteAddr, ":"); idx != -1 { + return r.RemoteAddr[:idx] + } + + return r.RemoteAddr +``` + +**Failure scenario.** For the normal `"[::1]:54321"` this happens to work, giving +`"[::1]"` — but the brackets are kept, while a client-supplied `X-Forwarded-For` +would give the same address as `"::1"`. The two forms are different map keys, so +one client can occupy two rate-limit buckets (and evade one blacklist entry). +`blacklist.go:160` and `:168` paper over this with `strings.Trim(clientIP, "[]")` +while `ratelimit.go` does not — so the two middlewares key on different strings +for the same client. If `RemoteAddr` ever lacks a port (a synthetic request, a +test, a non-TCP listener), `LastIndex` finds a colon **inside** the IPv6 address +and truncates it — `"::1"` becomes `"::"` — silently merging unrelated clients +into one bucket. + +**Recommendation.** Use the standard parser and normalize, once, in the shared +helper from finding 2: + +```go +host, _, err := net.SplitHostPort(r.RemoteAddr) +if err != nil { + host = r.RemoteAddr +} +if ip := net.ParseIP(strings.Trim(host, "[]")); ip != nil { + return ip.String() +} +return host +``` + +`net.ParseIP(...).String()` yields one canonical form, which fixes the +rate-limit/blacklist key mismatch and finding 13's IPv6 evasion together. + +--- + +### 23. Low — `//nolint:all` suppresses linting on the import block + +`ratelimit.go:4`: + +```go +// Package middleware provides HTTP middleware functionalities such as rate limiting and IP blacklisting. +package middleware + +//nolint:all +import ( +``` + +**Failure scenario.** A blanket `//nolint:all` with no rule named and no +justification. Attached to the import declaration its effect is narrow, but it +suppresses *every* linter there — including `depguard` and `gosec` import rules, +which is exactly where a prohibited or vulnerable dependency would be flagged. +More practically it sets a precedent: the directive gives no reason, so nobody can +tell whether it is still needed, and it survives indefinitely. Given `gosec` is +not enabled at all (`_CROSS-CUTTING.audit.md` finding X4), suppressions that hide +security linting deserve removal on principle. + +**Recommendation.** Delete it and fix whatever it was hiding; if a suppression is +genuinely required, name the rule and give a reason — +`//nolint:depguard // x/time/rate is an approved dependency`. `golangci-lint`'s +`nolintlint` setting enforces exactly this and is worth enabling repo-wide. + +--- + +## What looks right + +- **`getLimiter` implements correct double-checked locking** + (`ratelimit.go:42-62`): read under `RLock`, release, take `Lock`, **re-check** + before creating. The re-check at `:55` is the step usually omitted, and getting + it right means two goroutines racing on a new key cannot produce two limiters — + which would silently double a client's allowance. This is the pattern + `pkg/cache`'s `defaultCache` singleton should adopt (`cache.audit.md` + finding 3). +- **Every shared field is consistently guarded.** `IPBlacklist` takes `Lock` for + all four mutators (`blacklist.go:45`, `:62`, `:74`, `:83`) and `RLock` for both + readers (`:98`, `:137`); `RateLimiter` does the same. There are **no + unsynchronized globals in this package**, unlike most others in the repo + (`_CROSS-CUTTING.audit.md` finding X5), and I found no data race on any path. +- Locks are held for short, bounded critical sections with no I/O, no callbacks + and no lock nesting inside them, so deadlock is structurally impossible here. +- `blacklist.go` returns its 403 correctly: sets `Content-Type` **before** + `WriteHeader`, then encodes (`:181-186`) — the ordering `ratelimit.go:88` gets + wrong (finding 18). +- `BlockIP` validates with `net.ParseIP` before storing (`blacklist.go:41-43`) and + `BlockCIDR` propagates `net.ParseCIDR`'s error (`:57-60`), so invalid rules are + rejected at the point of entry rather than silently ignored. +- `RequestSizeLimiter` uses `http.MaxBytesReader` (`sizelimit.go:36`) rather than + trusting `Content-Length` — the correct choice, since `Content-Length` is + attacker-supplied and absent on chunked requests. `NewRequestSizeLimiter` + defaults a non-positive `maxSize` to 10 MB (`:24-26`) rather than to unlimited, + which is the safe direction. (It is still never mounted — finding 1.) +- `PanicRecovery` passes the request context into `logger.HandlePanic` + (`panic.go:24-25`) so the error tracker can correlate the panic with the request + trace — a deliberate touch, and the comment explains why. +- `pkg/server` applies `PanicRecovery` **inside** `trackRequestsMiddleware` + (`manager.go:466` vs `:540`), so in-flight accounting survives a panicking + handler, and offers `PanicHandler` (`interfaces.go:47`) as a documented override + for embedders who need different behaviour. +- `sanitizeValue` recurses correctly through nested maps and slices + (`sanitize.go:120-135`) with a `default` branch that passes non-strings through + untouched, so numbers and booleans are not stringified. +- `Sanitizer`'s fields are read-only after construction and `regexp.Regexp` is + safe for concurrent matching, so a shared `*Sanitizer` across handlers is + race-free — provided callers do not mutate the exported fields at runtime, which + nothing guards against but nothing does. +- **1 127 lines of tests against 799 lines of code**, the best ratio in the + repository, including concurrency tests. The tests are why findings 12, 13 and + 17 are the only correctness bugs of their kind left; they are also why finding 4 + is *locked in* rather than merely present. + +## Suggested follow-up + +1. **Decide the perimeter story (finding 1).** Either wire rate limiting, size + limiting and blacklisting into `pkg/server` from `MiddlewareConfig`, or delete + them and the 18 KB README that documents them as available. The present state — + configurable, documented, tested, unmounted — is the one that misleads + operators into believing they are protected. +2. **Fix the trusted-proxy model before mounting anything IP-based** + (findings 2, 3, 22). One shared, normalized, default-deny `ClientIP` helper for + both middlewares, with `TrustedProxies` in config. Mounting the current + `getClientIP` gives an attacker an unauthenticated memory-growth primitive and + a header that switches the blacklist off. +3. **Stop returning the panic value** (finding 4) and update + `panic_test.go:57` to assert the opposite. Smallest diff, largest security + win, and it is the only middleware currently in the request path. +4. **Do not mount the sanitizer as it stands** (findings 5, 6, 7, 14, 15). + Convert it from strip-and-continue to validate-and-reject, set + `EscapeHTML: false`, and move XSS defense to context-aware output encoding. + Mounting it today would corrupt filter values and JSON parameters on the first + request while leaving multi-line `