summercms/.planning/phases/06-http-routing-auth-groups-and-rate-limiting/06-REVIEW.md

---
phase: 06-http-routing-auth-groups-and-rate-limiting
reviewed: 2026-09-20T11:44:42Z
depth: standard
files_reviewed: 68
files_reviewed_list:
  - ../fonoteka.go/config/http.yaml
  - ../fonoteka.go/docs/openapi.json
  - ../fonoteka.go/parity/fixtures/routes/GET__api_v1_fonoteka_genres_personal_token.yaml
  - ../fonoteka.go/parity/genre_security_test.go
  - ../fonoteka.go/parity/genres_seed_test.go
  - ../fonoteka.go/parity/manifest.yaml
  - ../fonoteka.go/parity/parity_contract_test.go
  - ../fonoteka.go/parity/parity_test.go
  - ../fonoteka.go/parity/php_debug_test.go
  - ../fonoteka.go/parity/php_parity.sh
  - ../fonoteka.go/plugins/golem15/fonoteka/classes/auth/postgres_test.go
  - ../fonoteka.go/plugins/golem15/fonoteka/classes/auth/token_guard.go
  - ../fonoteka.go/plugins/golem15/fonoteka/classes/auth/token_guard_coverage_test.go
  - ../fonoteka.go/plugins/golem15/fonoteka/classes/auth/token_guard_test.go
  - ../fonoteka.go/plugins/golem15/fonoteka/controllers/genre_controller.go
  - ../fonoteka.go/plugins/golem15/fonoteka/middleware/public_share_headers.go
  - ../fonoteka.go/plugins/golem15/fonoteka/middleware/public_share_headers_test.go
  - ../fonoteka.go/plugins/golem15/fonoteka/middleware/token_scope.go
  - ../fonoteka.go/plugins/golem15/fonoteka/middleware/token_scope_coverage_test.go
  - ../fonoteka.go/plugins/golem15/fonoteka/middleware/token_scope_test.go
  - ../fonoteka.go/plugins/golem15/fonoteka/models/api_token.go
  - ../fonoteka.go/plugins/golem15/fonoteka/plugin.go
  - ../fonoteka.go/plugins/golem15/fonoteka/plugin_boot_test.go
  - ../fonoteka.go/plugins/golem15/fonoteka/routes.go
  - ../fonoteka.go/plugins/golem15/fonoteka/routes_bucket_test.go
  - ../fonoteka.go/plugins/golem15/fonoteka/routes_cors_test.go
  - ../fonoteka.go/plugins/golem15/fonoteka/routes_group_test.go
  - ../fonoteka.go/plugins/golem15/fonoteka/routes_isolation_test.go
  - ../fonoteka.go/plugins/golem15/user/plugin.go
  - ../fonoteka.go/scripts/check-openapi.sh
  - ../fonoteka.go/scripts/swagger2openapi.go
  - bouncer/context.go
  - bouncer/context_test.go
  - bouncer/guard.go
  - bouncer/jwt.go
  - bouncer/registry.go
  - bouncer/registry_coverage_test.go
  - bouncer/registry_test.go
  - fetchguard/fetch.go
  - fetchguard/fetch_coverage_test.go
  - fetchguard/fetch_test.go
  - fetchguard/ip.go
  - fetchguard/ip_test.go
  - fetchguard/policy.go
  - internal/build/build.go
  - internal/build/build_test.go
  - pact/capabilities.go
  - surf/bodylimit.go
  - surf/bodylimit_test.go
  - surf/clientip.go
  - surf/clientip_test.go
  - surf/cors.go
  - surf/cors_coverage_test.go
  - surf/cors_test.go
  - surf/limiter.go
  - surf/limiter_coverage_test.go
  - surf/limiter_store.go
  - surf/limiter_test.go
  - surf/middleware_test.go
  - surf/routelist_command.go
  - surf/router.go
  - surf/router_test.go
  - surf/routetable.go
  - surf/routetable_coverage_test.go
  - surf/routetable_test.go
  - wire/response.go
  - wire/response_coverage_test.go
  - wire/response_test.go
findings:
  critical: 4
  warning: 11
  info: 0
  total: 15
status: issues_found
---

# Phase 6: Code Review Report

**Reviewed:** 2026-09-20T11:44:42Z
**Depth:** standard
**Files Reviewed:** 68
**Status:** issues_found

## Summary

The review found 15 defects, including four blockers. The limiter permits concurrent over-limit requests and lets anonymous callers choose a bucket through the untrusted Host header. The SSRF guard accepts transition-address encodings of private IPv4 targets, and the panic recovery can commit a successful partial response before appending the supposedly opaque 500 body. Eleven additional robustness and contract defects remain. Passing tests do not exercise these adversarial paths.

Verification run during review:

- `go test ./bouncer ./fetchguard ./surf ./wire -short` — pass
- `go vet ./bouncer ./fetchguard ./surf ./wire ./internal/build` — pass
- `go test ./plugins/golem15/fonoteka/... -short` — pass
- `go test ./parity -short` — pass

## Narrative Findings (AI reviewer)

## Critical Issues

### CR-01: Check-then-increment is not atomic, so concurrent requests exceed the configured limit

**Classification:** BLOCKER
**File:** `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/limiter.go:95-115`; `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/limiter_store.go:73-100`
**Issue:** Each request calls `TooManyAttempts` and `Hit` as two separately locked operations. If a key is at `Max-1`, multiple requests can all observe “not limited” before any of them calls `Hit`; every one is then admitted even though the count advances beyond `Max`. With `Max: 1`, two simultaneous first requests can both pass. This is a direct bypass of the security control under normal HTTP concurrency, and all tests exercise it sequentially.
**Fix:** Make admission one atomic store operation. For example, replace the three-call protocol with `Attempt(key, max, decay) (allowed bool, attempts int, retryAfter time.Duration)` and perform expiry, threshold check, increment, and retry calculation under the same mutex/transaction. Add a barrier-based concurrent test in which many requests hit a `Max: 1` bucket and exactly one reaches the handler.

### CR-02: Inline throttles use the attacker-controlled Host header as part of the key

**Classification:** BLOCKER
**File:** `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/limiter.go:149-165`
**Issue:** Anonymous inline throttles key on `r.Host + "|" + ClientIP(...)`. `Host` is request input, not a registered route domain. Unless every upstream rejects or rewrites arbitrary Host values, a caller can rotate `Host` on every request and obtain a fresh bucket, bypassing `throttle:10,1` and other inline limits. The onboarding and public-invitation groups are declared with inline throttles, so this becomes live as soon as their handlers land.
**Fix:** Remove `r.Host` from the key. The inline throttle parameter already identifies the bucket, so use a stable server-side prefix such as `"inline:" + param + "|" + ClientIP(r, trusted)`. If host-scoped routes are introduced later, key from the trusted registered route metadata, never the incoming Host header. Add a test proving different Host headers from one IP share a bucket.

### CR-03: The SSRF guard allows private IPv4 targets encoded through IPv6 transition prefixes

**Classification:** BLOCKER
**File:** `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/fetchguard/ip.go:5-45`; `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/fetchguard/fetch.go:145-163`
**Issue:** `Unmap` only handles IPv4-mapped `::ffff:x.x.x.x`. The allow decision treats all other global-looking IPv6 addresses as public. On networks supporting the corresponding transition mechanism, an attacker-controlled DNS record can therefore target loopback, RFC1918, or metadata addresses via values such as `64:ff9b::7f00:1` (NAT64 loopback), `64:ff9b::a9fe:a9fe` (NAT64 metadata), or `2002:7f00:1::` (6to4 loopback). The helper is explicitly intended as an SSRF boundary, so an environment-dependent route to private infrastructure is still a vulnerability.
**Fix:** Classify IPv6 transition prefixes before dialing. Extract and re-check embedded IPv4 for `64:ff9b::/96`, the local-use NAT64 prefix `64:ff9b:1::/48`, and 6to4 `2002::/16`; reject other transition forms that cannot be safely decoded. Add table tests for embedded loopback, RFC1918, link-local metadata, and public IPv4 values.

### CR-04: Panic recovery can return 200 and leak a partial response instead of the opaque 500

**Classification:** BLOCKER
**File:** `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/router.go:520-541`
**Issue:** Both recovery wrappers write their 500 only after recovering. If a handler or middleware has already called `Write`/`WriteHeader`, the status and bytes are committed; the later `WriteHeader(500)` is ignored. A non-raw handler that writes sensitive partial JSON and then panics will return the earlier status (often 200) with the partial bytes followed by the opaque error JSON. The tests only panic before any write, so they do not cover this broken error boundary.
**Fix:** Buffer non-streaming house responses until the route returns successfully, then commit them. On panic, discard the buffered status/headers/body and write only the opaque 500. Raw routes need an explicit policy: either buffer them too or document that they cannot promise a clean recovery after a committed write. Add partial-write-then-panic tests for both raw and house routes.

## Warnings

### WR-01: Invalid bucket definitions and missing stores silently disable rate limiting

**Classification:** WARNING
**File:** `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/limiter.go:63-79`; `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/limiter.go:84-94`
**Issue:** `RegisterBucket` accepts `Key == nil`, `Max <= 0`, and `Decay <= 0`. At request time a nil key, nil store, or resolution error simply calls the protected handler. A nil key therefore disables the control, and a non-positive decay continually expires the entry so requests remain effectively unlimited. These are boot-time configuration errors and should not produce a fail-open live route.
**Fix:** Reject nil `Key`, `Max < 1`, `Decay <= 0`, and nil stores during construction/registration. If an impossible runtime error remains, return an opaque 500 rather than calling `next`.

### WR-02: Missing or malformed body-limit configuration turns the limit off

**Classification:** WARNING
**File:** `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/router.go:432-441`; `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/bodylimit.go:29-45`
**Issue:** `Config.Int` yields zero for absent and non-coercible values. `BuildRouter` copies that zero to `defaultBytes`, and `wrap` omits `MaxBytesReader` whenever the value is not positive. A missing `http.yaml`, typo, or malformed value therefore changes the advertised default from 128 MiB to unlimited without a boot error.
**Fix:** Read the raw value with presence/type checks and fail boot unless `default_bytes` is a positive integer. Keep “unlimited” available only through the explicit raw-route exemption.

### WR-03: Trusted-proxy parsing silently drops valid operational forms and invalid entries

**Classification:** WARNING
**File:** `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/clientip.go:49-82`
**Issue:** `TrustedProxies` only accepts CIDR strings and silently skips everything else. A common deployment value such as `127.0.0.1` is discarded because it is not written as `/32`; a typo is discarded the same way. Behind a reverse proxy this collapses all users onto the proxy address and corrupts every IP-keyed limiter budget, with no startup signal.
**Fix:** Accept a bare `netip.Addr` by converting it to `/32` or `/128`, return an error for unparseable entries, and propagate that error from `BuildRouter` so the application fails boot.

### WR-04: Token last-used auditing ignores the framework's trusted-proxy client IP

**Classification:** WARNING
**File:** `/media/nvme/dev/golem15/summercms.io/summercms/fonoteka.go/plugins/golem15/fonoteka/classes/auth/token_guard.go:50-60`; `/media/nvme/dev/golem15/summercms.io/summercms/fonoteka.go/plugins/golem15/fonoteka/classes/auth/token_guard.go:79-88`
**Issue:** `last_used_ip` is derived directly from `RemoteAddr`, while rate-limit keys use `surf.ClientIP`. Behind a configured trusted proxy the same request is attributed to the real client for limiting but to the proxy for audit history, making the security audit field misleading.
**Fix:** Parse trusted proxies once during plugin boot and inject them into `TokenGuard`; use `surf.ClientIP(r, trusted)` for the audit stamp so there is one client-IP policy.

### WR-05: Retry-After is rounded down and can tell clients to retry before the window expires

**Classification:** WARNING
**File:** `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/limiter.go:96-103`
**Issue:** Integer duration division floors the remaining time. A request with 900 ms left receives `Retry-After: 0`; a nearly fresh one-minute window normally reports 59 instead of 60. Compliant clients can immediately retry and receive another 429.
**Fix:** Round up to whole seconds and clamp live windows to at least one second, e.g. `(retryAfter + time.Second - 1) / time.Second`.

### WR-06: Middleware factories are invoked twice during normal Assemble

**Classification:** WARNING
**File:** `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/router.go:336-348`; `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/router.go:487-492`
**Issue:** `BuildRouter` calls `wrap` once per route as validation, and `Assemble` immediately calls `compile`, which calls `wrap` again. Each `factory.fn(param)` therefore runs twice. Plugin factories are an extension API and may allocate resources, register metrics, or bind single-use state; the first constructed chain is discarded. A factory returning nil also panics rather than producing a contextual boot error.
**Fix:** Split pure route validation from middleware construction, or compile once and retain the compiled handler. Validate factory and middleware return values and turn nil into a plugin/name-specific boot error. Add a factory call-count test around `Assemble`.

### WR-07: The generated OpenAPI document omits a live route and all authentication requirements

**Classification:** WARNING
**File:** `/media/nvme/dev/golem15/summercms.io/summercms/fonoteka.go/plugins/golem15/fonoteka/controllers/genre_controller.go:45-52`; `/media/nvme/dev/golem15/summercms.io/summercms/fonoteka.go/docs/openapi.json:40-73`
**Issue:** The same handler is live at both `/_fonoteka/api/v1/genres` and `/api/v1/fonoteka/genres`, but only the JWT path is documented. The documented operation also has no bearer security scheme or operation security requirement, so generated clients see a public endpoint and cannot discover the personal-token surface.
**Fix:** Add both route annotations, define the appropriate bearer schemes, annotate each operation's security requirement, document its 401/403/422/423 responses, and regenerate `docs/openapi.json`.

### WR-08: The Swagger-to-OpenAPI converter emits invalid OpenAPI for supported Swagger constructs

**Classification:** WARNING
**File:** `/media/nvme/dev/golem15/summercms.io/summercms/fonoteka.go/scripts/swagger2openapi.go:99-134`; `/media/nvme/dev/golem15/summercms.io/summercms/fonoteka.go/scripts/swagger2openapi.go:136-209`
**Issue:** Path-item `parameters` are copied unchanged instead of being converted; `formData` parameters are also left as OpenAPI 2 parameters rather than combined into an OpenAPI 3 `requestBody`. Top-level `produces`/`consumes` are ignored. These are standard Swagger 2 constructs, especially for the upload routes anticipated by this phase, and will either make the validation step fail or silently assign the wrong media type.
**Fix:** Carry document-level media types into path/operation conversion, run path-level parameters through the same conversion, and translate body/form-data parameters into request bodies (including multipart schemas). Add table-driven converter tests with path parameters, JSON bodies, multipart uploads, and non-JSON global media types.

### WR-09: CORS can generate the forbidden wildcard-origin plus credentials combination

**Classification:** WARNING
**File:** `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/cors.go:69-95`; `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/cors.go:104-119`
**Issue:** When `allowed_origins` contains `*`, `corsAllowOrigin` always returns `*`; `SupportsCredentials` independently adds `Access-Control-Allow-Credentials: true`. Browsers reject that combination. The checked-in config currently disables credentials, but a valid-looking future config change silently produces a non-functional policy.
**Fix:** Fail boot on wildcard plus credentials, or reflect a validated request Origin and emit `Vary: Origin` when credentials are enabled.

### WR-10: An unknown Fetch policy mode fails open to PublicOnlyMode

**Classification:** WARNING
**File:** `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/fetchguard/fetch.go:41-51`; `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/fetchguard/policy.go:14-19`
**Issue:** The host allow-list runs only when `policy.Mode == AllowHostsMode`; every other integer, including invalid values, behaves as `PublicOnlyMode`. A bad cast/configuration can therefore broaden a host allow-list to every public host.
**Fix:** Switch explicitly on the two defined modes and return a typed configuration error in the default case. Add an invalid-mode test that performs no network I/O.

### WR-11: InvScope's wire body does not match the project's no-newline JSON convention

**Classification:** WARNING
**File:** `/media/nvme/dev/golem15/summercms.io/summercms/fonoteka.go/plugins/golem15/fonoteka/middleware/token_scope.go:31-35`; `/media/nvme/dev/golem15/summercms.io/summercms/fonoteka.go/plugins/golem15/fonoteka/middleware/token_scope_test.go:71-76`
**Issue:** `json.Encoder.Encode` appends a newline, while `wire.WriteJSON` exists specifically to reproduce PHP `json_encode` without one. The tests call `TrimSpace`, hiding the byte-level mismatch for both 401 and 403 responses.
**Fix:** Replace the local writer with `wire.WriteJSON` and assert exact response bytes in the tests without trimming.

---

_Reviewed: 2026-09-20T11:44:42Z_
_Reviewer: the agent (gsd-code-reviewer)_
_Depth: standard_