# Audit — `pkg/modelregistry` - **Date:** 2026-09-29 - **Scope:** `pkg/modelregistry/model_registry.go` (381 LOC, 1 file, **no tests**) - **Axes:** thread locking/waiting · slowness · security · panic handling & logging - **Threat model:** hostile internet client. This package holds the `ModelRules` that `pkg/security/hooks.go` consults to authorise read/update/create/delete, so it is **on the authorisation path**. ## Summary This package is the highest-risk find in the audit. It has been deliberately reworked to "never hang" by replacing blocking `Lock`/`RLock` with **bounded `TryLock` retry loops that give up and return a wrong answer** — and because those wrong answers are consumed by `pkg/security/hooks.go` as authorisation decisions, the result is an **authorisation control that fails open under lock contention**. The comments in the file are explicit about the trade-off ("falls back to the last known value without synchronization", "the call is a no-op") — so the hazard was known at the time of writing. What appears not to have been traced is where those degraded results end up. They end up in `checkModelUpdateAllowed` / `checkModelDeleteAllowed`, which treat any error as *permit*. There are **no tests** in this package and no `-race` coverage of it anywhere. | # | Severity | Axis | Finding | |---|----------|------|---------| | 1 | **Critical** | Security + Locking | `GetModel`'s "registry locked" error is consumed by `pkg/security/hooks.go` as *allow by default* → authorisation fails open under write-lock contention | | 2 | **High** | Locking | `GetDefaultRegistry` documents and performs an unsynchronised read of `defaultRegistry` on lock-acquire failure — a data race by design | | 3 | **High** | Locking | `SetDefaultRegistry` silently no-ops after ~20 ms of contention; caller gets no error | | 4 | **High** | Security | `RegisterModelWithRules` is non-atomic: the model is visible with permissive `DefaultModelRules` before its real rules are applied (TOCTOU) | | 5 | Medium | Correctness | `GetAllModels` returns an empty map, and `GetModels` silently skips whole registries, on lock-acquire failure | | 6 | Medium | Locking | `IterateModels` invokes the caller's callback while holding `RLock` → guaranteed self-deadlock if the callback touches the registry | | 7 | Medium | Slowness | `time.Sleep(1ms)` spin loops add up to 20 ms of latency per call and defeat mutex fairness/hand-off | | 8 | Medium | Locking | `defaultRegistry` is read unsynchronised by six package-level functions while `SetDefaultRegistry` writes it under lock | | 9 | Medium | Locking | Inconsistent discipline: `SetModelRules`/`GetModelRules`/`AddRegistry`/`IterateModels` use blocking locks; the rest use try-locks | | 10 | Low | Slowness/Locking | Reflection (`TypeOf`, unwrap loop, `reflect.New`) runs while holding the registry **write** lock | | 11 | Low | Availability | Unbounded unwrap loop: a recursive pointer type (`type T *T`) spins forever holding the write lock (**verified**) | | 12 | Low | Panic | Package has no `recover` anywhere, and calls a caller-supplied callback under a lock (see 6) | | 13 | Low | Security | `DefaultModelRules()` grants `CanRead/Update/Create/Delete: true` — registration without explicit rules is fully mutable | ## Resolution (2026-09-30) Fixed in `pkg/modelregistry/model_registry.go`, `pkg/security/hooks.go`, and new `pkg/modelregistry/model_registry_test.go` (passes under `-race`). | # | Status | What changed | |---|--------|--------------| | 1 | **Fixed** | Added sentinels `ErrModelNotFound`, `ErrModelExists`, `ErrInvalidModel` (wrapped, `errors.Is`-friendly). `checkModelUpdateAllowed`/`checkModelDeleteAllowed` now allow-by-default **only** on `ErrModelNotFound`; any other error denies. Lookups can no longer return a "locked" error at all. | | 2 | **Fixed** | `GetDefaultRegistry` uses a plain `RLock`; no unsynchronised fallback. | | 3 | **Fixed** | `SetDefaultRegistry` uses a blocking `Lock` (cannot silently no-op); a nil registry is ignored. | | 4 | **Fixed** | `RegisterModelWithRules` and `RegisterModel` share `registerLocked`, which writes model + rules under one lock acquisition. | | 5 | **Fixed** | `GetAllModels`/`GetModels` use blocking locks and can no longer return empty/partial results due to contention. Signatures unchanged (`GetAllModels` is used through interfaces by resolvespec/restheadspec/openapi). | | 6 | **Fixed** | `IterateModels` iterates a snapshot; the callback runs with no lock held (regression test re-enters the registry). | | 7 | **Fixed** | Try-lock/sleep helpers and `lockRetry*` constants removed. | | 8 | **Fixed** | All package-level functions go through `GetDefaultRegistry()` / `registriesSnapshot()`; `defaultRegistry` is only touched under `registriesMutex`. | | 9 | **Fixed** | One discipline: blocking locks, snapshot-and-release, documented lock order (`registriesMutex` before a registry's mutex). | | 10 | **Fixed** | Reflection/validation (`validateModel`) runs before the write lock is taken. | | 11 | **Fixed** | Unwrap loop capped at 16 levels; `type T *T` now returns `ErrInvalidModel` (tested). | | 12 | **Fixed** | Sentinel errors added. `IterateModels` recovers a callback panic per model, logs it via `logger.HandlePanic` with the model name, and continues; `validateModel` recovers reflection panics and returns `ErrInvalidModel` so registration fails closed. No lock is held during either, so the registry cannot be wedged. | | 13 | **Accepted (decision)** | Allow-by-default retained deliberately: `DefaultModelRules()` still grants read/update/create/delete. Callers wanting restrictions must use `RegisterModelWithRules`/`SetModelRules`. | Tests added: sentinel errors, recursive pointer type, pointer normalisation, atomic `RegisterModelWithRules` (concurrent reader never sees permissive rules), re-entrant `IterateModels`, cross-registry `GetModelRulesByName`, and a concurrent `-race` stress test. Not changed: the `pkg/security` middleware-wiring question (context fast-path) remains tracked in `audit/pkg/security.audit.md`. --- ## Findings ### 1. Authorisation fails open under lock contention (Critical, Security + Locking) The mechanism spans two packages. **Here**, `GetModel` conflates "not found" with "could not lock" into a single `error` return (`model_registry.go:198-210`): ```go func (r *DefaultModelRegistry) GetModel(name string) (interface{}, error) { if !r.tryRLock() { return nil, fmt.Errorf("failed to get model %s: registry locked", name) } defer r.mutex.RUnlock() model, exists := r.models[name] if !exists { return nil, fmt.Errorf("model %s not found", name) } return model, nil } ``` `GetModelRulesByName` (`model_registry.go:364-376`) uses `GetModel` as its existence probe: ```go for _, registry := range registries { if _, err := registry.GetModel(name); err == nil { return registry.GetModelRules(name) } } return ModelRules{}, fmt.Errorf("model %s not found in any registry", name) ``` So a `tryRLock` failure makes the registry look like it does not contain the model. **In `pkg/security/hooks.go`**, that outcome is interpreted as *permit* (`pkg/security/hooks.go:274-294`, and identically at `:298-318`): ```go func checkModelUpdateAllowed(secCtx SecurityContext) error { rules, ok := GetModelRulesFromContext(secCtx.GetContext()) if !ok { schema := secCtx.GetSchema() entity := secCtx.GetEntity() var err error if schema != "" { rules, err = modelregistry.GetModelRulesByName(fmt.Sprintf("%s.%s", schema, entity)) } if err != nil || schema == "" { rules, err = modelregistry.GetModelRulesByName(entity) } if err != nil { return nil // model not registered, allow by default } } if !rules.CanUpdate { return fmt.Errorf("update not allowed for %s", secCtx.GetEntity()) } return nil } ``` Note the context fast-path at `hooks.go:275`: if `NewModelAuthMiddleware` already put rules in the context, the registry is not consulted and this bug does not fire. The registry fallback runs whenever that middleware is absent or did not resolve rules — so the blast radius depends on deployment wiring. `audit/pkg/security.audit.md` covers whether that middleware is mandatory. `return nil` from `checkModelUpdateAllowed` means **the update is authorised**. Same for `checkModelDeleteAllowed`. `GetModelRules(name)` for the "found" path also uses a blocking `RLock` (`model_registry.go:253`) — so the two calls in `GetModelRulesByName` don't even use the same locking discipline. **Failure scenario.** A model `public.employees` is registered with `CanDelete: false`. A concurrent `RegisterModel` (or `SetModelRules`, or `RegisterModelWithRules`) holds the write lock for longer than `lockRetryAttempts * lockRetryDelay` = 20 ms — which is entirely achievable given finding 10 (reflection under the write lock) and finding 7 (each waiter sleeps in 1 ms increments, so N waiters serialise). During that window every `DELETE` request against `public.employees` has `GetModelRulesByName` return an error, `checkModelDeleteAllowed` return `nil`, and the delete proceeds. The model's `CanDelete: false` is not enforced. This is remotely triggerable if any request path can cause a model registration or a rules update; even without that, it is a straightforward race that will fire under load. **Recommendation, in order of value:** 1. Make the security layer **fail closed**: distinguish a sentinel `ErrModelNotFound` from any other error, and only allow-by-default on `ErrModelNotFound`. Any other error must deny. 2. Delete the try-lock scheme here entirely and use plain `RLock`/`Lock` (see finding 2 for why the scheme does not achieve its stated goal anyway). 3. Separate the existence probe from the rules fetch so `GetModelRulesByName` takes each registry's lock once and returns a typed "found / not found / unavailable" result. ### 2. `GetDefaultRegistry` races by design (High, Locking) `model_registry.go:71-84` ```go // GetDefaultRegistry returns the current default registry. It uses a // bounded TryRLock instead of a blocking RLock so it can never hang; // if the lock can't be acquired in time it falls back to the last known // value without synchronization. func GetDefaultRegistry() *DefaultModelRegistry { for i := 0; i < lockRetryAttempts; i++ { if registriesMutex.TryRLock() { defer registriesMutex.RUnlock() return defaultRegistry } time.Sleep(lockRetryDelay) } return defaultRegistry } ``` The `return defaultRegistry` on line 83 reads a pointer that `SetDefaultRegistry` (`model_registry.go:89-116`) writes under the write lock. The only time this path is taken is precisely when a writer holds or is contending for the lock — i.e. the fallback executes *exactly* in the window where the race is live. The trade is not "hang vs. slightly stale value"; it is "block for 20 ms vs. data race", and a torn/`nil` pointer read here means a nil-pointer dereference in the caller. The premise is also wrong: a `sync.RWMutex.RLock` that is only ever held for a map lookup cannot "hang". The hang this was written to avoid must have had a different root cause — most likely finding 6 (self-deadlock through `IterateModels`) or a lock-ordering inversion — and the try-lock scheme papers over it rather than fixing it. **Recommendation:** revert to `RLock`/`RUnlock`. If a real hang was observed, reproduce it under `-race` and `GODEBUG=gctrace`/`SIGQUIT` stack dump; the fix belongs at the deadlock, not here. ### 3. `SetDefaultRegistry` silently no-ops (High, Locking) `model_registry.go:90-100` ```go acquired := false for i := 0; i < lockRetryAttempts; i++ { if registriesMutex.TryLock() { acquired = true; break } time.Sleep(lockRetryDelay) } if !acquired { return } ``` The function returns no error. A caller that swaps in a registry — plausibly one with *restrictive* `ModelRules* — has no way to learn the swap did not happen, and continues believing the new registry is in effect. Every subsequent authorisation check consults the old registry's rules. `GetModels` (`model_registry.go:319-329`) has the same shape and returns `nil`. **Recommendation:** return `error` from `SetDefaultRegistry`; or (better) use a blocking `Lock`, since this is a startup-time operation where blocking is correct. ### 4. `RegisterModelWithRules` is non-atomic (High, Security) `model_registry.go:270-282` ```go func (r *DefaultModelRegistry) RegisterModelWithRules(name string, model interface{}, rules ModelRules) error { // First register the model if err := r.RegisterModel(name, model); err != nil { return err } // Then set the rules (we need to lock again for rules) r.mutex.Lock() defer r.mutex.Unlock() r.rules[name] = rules return nil } ``` `RegisterModel` releases the write lock before returning, and it initialises the model's rules to `DefaultModelRules()` (`model_registry.go:191-194`) — which is **permissive**: `CanRead/CanUpdate/CanCreate/CanDelete` all `true`. Between the two lock acquisitions, any concurrent `GetModelRulesByName` sees the model registered with full read/update/create/delete permission, regardless of the restrictive `rules` the caller passed. The comment "we need to lock again for rules" acknowledges the re-lock without noticing the gap it opens. **Failure scenario.** `RegisterModelWithRules("public.audit_log", AuditLog{}, ModelRules{CanRead: true})` — intended read-only. A `DELETE /public.audit_log/...` that lands in the window is authorised because `rules.CanDelete` is `true` from the default. **Recommendation:** add an unexported `registerLocked(name, model, rules)` that writes both maps under one lock acquisition, and build both public constructors on it. Also change the default initialisation in `RegisterModel` to deny-by-default, or require rules at registration. ### 5. Degraded results indistinguishable from real results (Medium, Correctness) Three functions return a plausible-looking answer when they cannot lock: - `GetAllModels` (`model_registry.go:212-215`) — `return make(map[string]interface{})`, i.e. "the registry is empty". - `GetModels` (`model_registry.go:327-329`) — `return nil` on `registriesMutex` failure, and `model_registry.go:336-338` `continue`s past any individual registry it cannot read, returning a **partial** list with no indication of truncation. - `GetDefaultRegistry` — finding 2. Consumers of `GetModels`/`GetAllModels` (schema introspection, OpenAPI generation, migration helpers) will emit a document that is missing models, and there is no error to log. Note these two have no callers in `pkg/` today, which is the only reason this is Medium. **Recommendation:** return `(T, error)`; never manufacture an empty-but-valid result. ### 6. `IterateModels` calls a user callback under a read lock (Medium, Locking) `model_registry.go:307-314` ```go func IterateModels(fn func(name string, model interface{})) { defaultRegistry.mutex.RLock() defer defaultRegistry.mutex.RUnlock() for name, model := range defaultRegistry.models { fn(name, model) } } ``` `fn` is arbitrary caller code running with `defaultRegistry.mutex` read-held. `sync.RWMutex` is not reentrant, and once a writer is blocked on `Lock` it also blocks *new* readers. So: - `fn` calling `modelregistry.RegisterModel` / `SetModelRules` → `Lock` waits for the reader, which is the same goroutine. **Permanent self-deadlock.** - `fn` calling `GetModel` → `tryRLock` fails for 20 ms and returns "registry locked" for every model, which is silent nonsense rather than a deadlock (and feeds finding 1). - `fn` doing anything slow (I/O, a DB call) holds the registry read lock for that whole duration, blocking all registration and — via the blocked-writer rule — all other readers too. This is the most likely original cause of the "hang" the try-lock scheme was introduced to work around. **Recommendation:** snapshot under the lock, release, then iterate: ```go func IterateModels(fn func(name string, model interface{})) { reg := GetDefaultRegistry() snapshot := reg.GetAllModels() // takes and releases the lock for name, model := range snapshot { fn(name, model) } } ``` ### 7. `time.Sleep` spin loops (Medium, Slowness) `tryLock` (`model_registry.go:128-136`), `tryRLock` (`:140-148`), and the inline loops in `GetDefaultRegistry`, `SetDefaultRegistry`, `GetModels`. ```go for i := 0; i < lockRetryAttempts; i++ { if r.mutex.TryLock() { return true } time.Sleep(lockRetryDelay) // 1ms } ``` Problems: - **Latency floor.** A contended call costs a multiple of 1 ms even if the lock frees after 10 µs, because the waiter is asleep. A blocking `Lock` would be handed the mutex in microseconds. So the "no-hang" scheme is *slower* in the common contended case, not faster. - **No fairness.** `sync.Mutex` has a starvation-avoidance mode that hands the lock to a waiter queued > 1 ms. `TryLock` participates in none of it, so a try-lock waiter can be starved indefinitely by a stream of blocking `Lock` callers (`SetModelRules`, `AddRegistry`, `IterateModels` all still block) — see finding 9. - **Timer churn.** 20 timer allocations per contended call. - Sleeping in a loop scales badly: 50 concurrent callers each sleep and wake 20 times, producing 1000 needless scheduler round-trips for what a mutex does with one park/unpark. **Recommendation:** delete the try-lock helpers. If a bounded wait is genuinely required for an SLO, express it as `context`-aware acquisition (a buffered-channel semaphore with a `select` on `ctx.Done()`), which gives a real deadline *and* a real error — not a silent wrong answer. ### 8. `defaultRegistry` read without the guarding mutex (Medium, Locking) `SetDefaultRegistry` writes `defaultRegistry` (`model_registry.go:110`) under `registriesMutex`. These read it **without** taking that mutex: - `RegisterModel` (`model_registry.go:288`) - `IterateModels` (`model_registry.go:308`, `311`) - `SetModelRules` (`model_registry.go:354`) - `GetModelRules` (`model_registry.go:359`) - `GetDefaultRegistry`'s fallback (`model_registry.go:83`, finding 2) A data race on the pointer, and semantically these functions may operate on the *previous* default registry after a swap — so rules set through `SetModelRules` can land on a registry nobody consults any more. **Recommendation:** route every access through one accessor that takes the lock (and make `defaultRegistry` an `atomic.Pointer[DefaultModelRegistry]` if lock-free reads are wanted — that is the correct way to get the "never blocks" property finding 2 was reaching for). ### 9. Inconsistent locking discipline (Medium, Locking) Within one 381-line file: | Function | `registriesMutex` | `r.mutex` | |---|---|---| | `GetDefaultRegistry` | `TryRLock` + fallback | — | | `SetDefaultRegistry` | `TryLock`, no-op on fail | — | | `AddRegistry` (`:120`) | blocking `Lock` | — | | `GetModelByName` (`:293`) | blocking `RLock` | via `GetModel` → `TryRLock` | | `GetModelRulesByName` (`:365`) | blocking `RLock` | `TryRLock` then blocking `RLock` | | `GetModels` (`:318`) | `TryRLock`, nil on fail | `tryRLock`, skip on fail | | `RegisterModel` (`:150`) | — | `tryLock`, error on fail | | `GetModel` (`:198`) | — | `tryRLock`, error on fail | | `GetAllModels` (`:212`) | — | `tryRLock`, empty on fail | | `SetModelRules` (`:237`) | — | blocking `Lock` | | `GetModelRules` (`:252`) | — | blocking `RLock` | | `IterateModels` (`:307`) | — | blocking `RLock` | Four different failure behaviours for the same class of event. The mix also means the try-lock callers can be starved by the blocking ones (finding 7), so the functions that "can never hang" are the ones most likely to return garbage. **Recommendation:** pick one discipline — blocking locks with snapshot-and-release — and apply it uniformly. ### 10. Reflection under the write lock (Low, Slowness + Locking) `RegisterModel` holds `r.mutex` (write) from `model_registry.go:151` through `:195`, and inside that window does `reflect.TypeOf` (`:161`), the unwrap loop (`:169-171`), `reflect.New(...).Elem().Interface()` (`:181`), and another `reflect.TypeOf` (`:185`). None of that touches `r.models`/`r.rules` and none of it needs the lock. This directly lengthens the window that makes finding 1 exploitable. Validate first, then take the lock only for the two map writes. ### 11. Unbounded unwrap loop on a recursive pointer type (Low, Availability) `model_registry.go:169-171` ```go for modelType.Kind() == reflect.Pointer || modelType.Kind() == reflect.Slice || modelType.Kind() == reflect.Array { modelType = modelType.Elem() } ``` `type T *T` is legal Go, and `reflect.Type.Elem()` on it returns itself — so the loop never terminates. **Verified experimentally:** ```go type T *T var x T tt := reflect.TypeOf(x) // main.T for tt.Kind() == reflect.Pointer { tt = tt.Elem() } // spins on main.T forever // → "INFINITE LOOP CONFIRMED after 101 iterations, still main.T" ``` Because the loop runs with the write lock held (finding 10), this doesn't just hang one goroutine — it wedges the registry permanently, at which point every try-lock caller starts returning "registry locked", which via finding 1 means **authorisation fails open for the rest of the process lifetime**. Requires a pathological model type, so exploitability is near zero; the fix is a one-line depth cap and it converts a permanent fail-open into an error return. **Recommendation:** bound the loop (`for depth := 0; depth < 16 && ...; depth++`) and return an error if the cap is hit. ### 12. No panic handling at all (Low, Panic handling) The package contains **zero** `recover()` calls and never logs — it does not import `pkg/logger`. For a pure data structure that is a defensible choice, with two caveats: - `IterateModels` runs a caller callback under a read lock (finding 6). If `fn` panics, the `defer RUnlock` does release the lock, so the registry is not wedged — that part is fine — but the panic propagates to whatever boundary handler exists, and nothing here records which model was being processed. A `logger`-free package can still name the model in a re-panic. - Every failure mode in the package is reported as a `fmt.Errorf` string with no wrapping and no sentinel values, so callers cannot distinguish them (finding 1). That is the panic/error-handling defect that actually matters here. **Recommendation:** define `ErrModelNotFound`, `ErrModelExists`, `ErrRegistryUnavailable` as sentinels and wrap them, so `errors.Is` works at the security layer. ### 13. Permissive default rules (Low, Security) `DefaultModelRules()` (`model_registry.go:24-36`) returns `CanRead`, `CanUpdate`, `CanCreate`, `CanDelete` all `true`. `RegisterModel` applies it to any model registered without explicit rules (`model_registry.go:191-194`), and `GetModelRules` falls back to it as well (`model_registry.go:266`). The `CanPublic*` flags default to `false` and `SecurityDisabled` to `false`, which is right. But the authenticated-path flags default open, so `RegisterModel(name, m)` — the form used by `pkg/testmodels/business.go` `RegisterTestModels` and the `modelregistry.RegisterModel` convenience wrapper — yields a fully mutable model. Combined with `pkg/security/hooks.go`'s allow-on-error, the system's default posture at every layer is permit. **Recommendation:** default to deny and make permissions opt-in, or at minimum log at registration time when a model is registered without explicit rules. --- ## What looks right - The struct-vs-pointer validation in `RegisterModel` (`model_registry.go:160-194`) is careful and well-reasoned: it rejects `nil`, unwraps pointer/slice/array to find the base type, rejects non-struct kinds with a message naming the original type, normalises a pointer/slice input to a zero struct value, and re-checks the final type. The error message even tells the caller to use `MyModel{}` instead of `&MyModel{}`. Good API ergonomics. - Duplicate registration is rejected (`model_registry.go:156-158`) rather than silently overwriting — important, since silent overwrite would be a rules-replacement primitive. - `GetAllModels` returns a **copy** of the map (`model_registry.go:218-222`) rather than the internal one, so callers cannot mutate registry state or race on it after the lock is dropped. This is the pattern the rest of the package should follow. - `GetModelByEntity` (`model_registry.go:225-234`) tries `schema.entity` before bare `entity`, which is the right precedence and matches what `pkg/security/hooks.go` does. - `GetModels` de-duplicates by name across registries (`model_registry.go:335-347`), so registry-order precedence is consistent with `GetModelByName`'s first-match rule. - Every `defer` for an acquired lock is correctly paired; there is no missing-`Unlock` path. The problems here are about *which* lock discipline was chosen, not about leaking locks. ## Suggested follow-up Ordered by risk: 1. **Make `pkg/security/hooks.go` fail closed** (finding 1). This is the single change that converts a Critical authorisation bypass into a Medium availability issue. It does not require touching this package. 2. **Remove the try-lock scheme** (findings 2, 3, 5, 7, 9) and fix the underlying hang by snapshotting in `IterateModels` (finding 6). 3. **Make `RegisterModelWithRules` atomic** (finding 4). 4. Route `defaultRegistry` access through a single locked accessor or `atomic.Pointer` (finding 8). 5. Move reflection out of the write-locked region and cap the unwrap loop (findings 10, 11). 6. **Add tests.** This package has none. Priority cases: `-race` test with concurrent `RegisterModel` + `GetModelRulesByName` asserting that rules are *never* observed as permissive for a restrictively-registered model; a test that `GetModelRulesByName` under contention does not return a "not found"-shaped error; `IterateModels` with a callback that calls back into the registry (should not deadlock); sentinel-error assertions.