diff --git a/.planning/phases/06-http-routing-auth-groups-and-rate-limiting/06-REVIEW.md b/.planning/phases/06-http-routing-auth-groups-and-rate-limiting/06-REVIEW.md new file mode 100644 index 0000000..a6bf606 --- /dev/null +++ b/.planning/phases/06-http-routing-auth-groups-and-rate-limiting/06-REVIEW.md @@ -0,0 +1,263 @@ +--- +phase: 06-http-routing-auth-groups-and-rate-limiting +reviewed: 2026-09-19T19:31:09Z +depth: standard +files_reviewed: 48 +files_reviewed_list: + - 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 + - /media/nvme/dev/golem15/summercms.io/summercms/fonoteka.go/config/http.yaml + - /media/nvme/dev/golem15/summercms.io/summercms/fonoteka.go/plugins/golem15/fonoteka/classes/auth/token_guard.go + - /media/nvme/dev/golem15/summercms.io/summercms/fonoteka.go/plugins/golem15/fonoteka/classes/auth/token_guard_test.go + - /media/nvme/dev/golem15/summercms.io/summercms/fonoteka.go/plugins/golem15/fonoteka/middleware/public_share_headers.go + - /media/nvme/dev/golem15/summercms.io/summercms/fonoteka.go/plugins/golem15/fonoteka/middleware/token_scope.go + - /media/nvme/dev/golem15/summercms.io/summercms/fonoteka.go/plugins/golem15/fonoteka/plugin.go + - /media/nvme/dev/golem15/summercms.io/summercms/fonoteka.go/plugins/golem15/fonoteka/routes.go + - /media/nvme/dev/golem15/summercms.io/summercms/fonoteka.go/plugins/golem15/fonoteka/controllers/genre_controller.go + - /media/nvme/dev/golem15/summercms.io/summercms/fonoteka.go/plugins/golem15/user/plugin.go + - /media/nvme/dev/golem15/summercms.io/summercms/fonoteka.go/plugins/golem15/fonoteka/routes_isolation_test.go + - /media/nvme/dev/golem15/summercms.io/summercms/fonoteka.go/plugins/golem15/fonoteka/middleware/must_change_password.go +findings: + critical: 1 + warning: 7 + info: 4 + total: 12 +status: issues_found +--- + +# Phase 6: Code Review Report + +**Reviewed:** 2026-09-19T19:31:09Z +**Depth:** standard +**Files Reviewed:** 48 +**Status:** issues_found + +## Summary + +Phase 6's framework seams (guard registry, dial-time SSRF check, trusted-proxy ClientIP, raw-group house-middleware refusal, path-scoped CORS) are mostly sound and match the written threat model. The live personal-token genres route is not. Middleware order inverts PHP's `throttle:fonoteka-api-token` → `inv.scope` pipeline, so invalid/missing bearers never enter the 60/min bucket — a rate-limit bypass on a shipped endpoint, and the exact failure 06-RESEARCH.md Pitfall 5 told the implementer to avoid. Several other fail-open paths (nil bucket Key, `Config.Int` → 0 body cap, bare-IP trusted proxies) are latent but real. + +## Narrative Findings (AI reviewer) + +## Critical Issues + +### CR-01: Personal-token throttle runs after InvScope, so unauthenticated requests are unlimited + +**File:** `/media/nvme/dev/golem15/summercms.io/summercms/fonoteka.go/plugins/golem15/fonoteka/routes.go:14-16` +**Issue:** PHP attaches `throttle:fonoteka-api-token` on the **group** (`routes.php:450`) and `inv.scope:*` on the **route**. Laravel therefore rate-limits first; the bucket closure calls `guard('inv_token')->token()` itself and keys `tok:` or `$request->ip()` even when the later scope gate 401s/403s. + +Go lists `inv_token`, `inv.scope:read`, then `throttle:fonoteka-api-token`. `surf.Router.wrap` applies names last-to-first, so the live order is: + +1. `inv_token` (CredentialGuard; on failure it does **not** write 401 — it passes through) +2. `inv.scope:read` (401 `{"error":"Invalid token"}` and **returns**) +3. `throttle:fonoteka-api-token` — **never reached** on a missing/invalid/wrong-prefix bearer + +An attacker can spray `Authorization: Bearer inv_` at `GET /api/v1/fonoteka/genres` with no 60/min cap. Prefixed tokens still pay a SHA-256 + indexed SQL lookup per hit (`token_guard.go:43-48`). PHP would 429 after 60/min per IP. 06-RESEARCH.md Pitfall 5 named this ordering requirement explicitly. + +`fonoteka-api-token`'s Key already does the right fallback (`tok:` else `ClientIP`) **if it runs**. It just never runs on the deny path. + +**Fix:** Keep `inv_token` **before** throttle (so `tok:` is available) and `inv.scope` **after** throttle (so 401/403 still consume the budget): + +```go +r.Group("/api/v1/fonoteka", surf.Use("inv_token", "throttle:fonoteka-api-token", "inv.scope:read"), func(g pact.Router) { + g.Get("/genres", handler) +}) +``` + +Add a test that 61 unauthenticated requests to the assembled genres token route yield a 429 with `{"message":"Too Many Attempts."}`. Do **not** put throttle first: that would key every request by IP and break `tok:`. + +## Warnings + +### WR-01: TokenGuard stamps last_used_ip from RemoteAddr, ignoring trusted proxies + +**File:** `/media/nvme/dev/golem15/summercms.io/summercms/fonoteka.go/plugins/golem15/fonoteka/classes/auth/token_guard.go:79-88` +**Issue:** PHP `ApiTokenGuard` writes `'last_used_ip' => $this->request->ip()`. D-04 requires `surf.ClientIP` as the single source for limiter keys **and later logging**. `remoteIP` splits `RemoteAddr` only. Behind nginx/Nitro every token's `last_used_ip` becomes the proxy, which is both a parity miss and a useless audit column. + +**Fix:** + +```go +func (g *TokenGuard) AuthenticateCredential(r *http.Request) (*bouncer.Principal, any, error) { + // ... + ip := surf.ClientIP(r, surf.TrustedProxies(/* plugin config, same list the buckets close over */)) +``` + +Thread the trusted-prefix list into `NewTokenGuard` (or read it from the same config the plugin already loads in `Buckets()`). Do not invent a second XFF parser. + +### WR-02: FixedWindowLimiter fail-opens when Key is nil, store is nil, or resolve failed + +**File:** `surf/limiter.go:84-94` +**Issue:** `Middleware` captures `resolve` errors into the request closure, then **skips limiting**: + +```go +if err != nil || b.Key == nil || l.store == nil { + next.ServeHTTP(w, r) + return +} +``` + +`ValidateThrottle` at Assemble time covers unknown/malformed `throttle:` names, but `RegisterBucket` never requires `Key != nil` or `Max >= 1`. A plugin that registers `fonoteka-api-token` with a nil Key boots cleanly and then never rate-limits. `l == nil` is the same no-op. Fail-open is the wrong default for a security control. + +**Fix:** Reject nil Key / non-positive Max in `RegisterBucket`. In `Middleware`, if `err != nil || b.Key == nil || l.store == nil`, do not call `next` — `ValidateThrottle` should have made this unreachable; if it is reached, 500/fail closed: + +```go +if err != nil || b.Key == nil || l.store == nil { + http.Error(w, `{"error":true,"message":"Internal server error"}`, http.StatusInternalServerError) + return +} +``` + +### WR-03: Body-limit middleware never maps MaxBytesError to 413; upload_bytes is dead + +**File:** `surf/bodylimit.go:10-18`; `surf/router.go:426-440`; `surf/bodylimit_test.go:105-116` +**Issue:** `bodyLimit` only wraps `http.MaxBytesReader`. Recent Go does **not** write 413 for you; the reader returns `*http.MaxBytesError` to the handler. `TestBodyLimitDefaultRejectsOversizedBody` gets 413 only because the **test plugin** checks `errors.As(err, &maxErr)` and writes 413 itself. House handlers that `json.NewDecoder(r.Body).Decode` (or `io.ReadAll`) will 500, or worse, hang until recoverJSON. + +Separately, `r.uploadBytes` is loaded from `http.body_limits.upload_bytes` and never read. D-18 promised a larger cap for upload routes; the only override is a manual `body.limit:N` string, and no production route uses it. Today both YAML values are 128MiB so the miss is latent — the next upload route will not pick up `upload_bytes` automatically. + +**Fix:** Convert the error in the middleware (or a small inner wrapper that reads nothing, only intercepts `MaxBytesError` from `next`), and either apply `uploadBytes` to a named group/flag or delete the field until a real upload route exists: + +```go +func bodyLimit(n int64) func(http.Handler) http.Handler { + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if n > 0 && r.Body != nil { + r.Body = http.MaxBytesReader(w, r.Body, n) + } + next.ServeHTTP(w, r) + }) + } +} +``` + +is not enough. Wrap `next` with a handler that, if `next` surfaces `*http.MaxBytesError` (via a capturing ResponseWriter, or by documenting that `wire` helpers check it), writes 413 with a stable JSON body. At minimum, change the test so it does **not** convert the error — the framework must. + +### WR-04: TrustedProxies silently drops bare IPs (PHP accepts `127.0.0.1`) + +**File:** `surf/clientip.go:75-82` +**Issue:** `netip.ParsePrefix("127.0.0.1")` fails. Malformed entries `continue` with no error. PHP `TRUSTED_PROXIES=127.0.0.1` (the production `.env` value that fixed CR-01 / Nitro collapsing public-IP buckets) is therefore a silent no-op if an operator copies it into `http.trusted_proxies`. The list becomes empty → `RemoteAddr` only → every visitor behind loopback Nitro/nginx shares one limiter key. That is the exact outage the PHP comment in `config/app.php:141-153` exists to prevent. + +**Fix:** Accept a bare address as `/32` or `/128`, and fail boot on unparseable entries instead of skipping: + +```go +p, err := netip.ParsePrefix(e) +if err != nil { + if addr, aerr := netip.ParseAddr(e); aerr == nil { + p = netip.PrefixFrom(addr, addr.BitLen()) + } else { + return nil, fmt.Errorf("surf: http.trusted_proxies: invalid CIDR %q", e) + } +} +``` + +`TrustedProxies` currently returns only `[]netip.Prefix`; returning `error` from BuildRouter is the fail-boot path the rest of this phase already uses. + +### WR-05: fetchguard private table misses NAT64-encoded loopback/RFC1918/metadata + +**File:** `fetchguard/ip.go:5-45`; `fetchguard/fetch.go:145-163` +**Issue:** Dial-time `Unmap()` closes `::ffff:169.254.169.254`. It does **not** close well-known encapsulations PHP also misses, and CONTEXT said Go may be stricter, never looser: + +- `64:ff9b::7f00:1` → 127.0.0.1 (NAT64) +- `64:ff9b::a9fe:a9fe` → 169.254.169.254 (metadata via NAT64) +- `2002:7f00:1::` → 127.0.0.1 (6to4) + +`isReservedOrPrivate` only walks PHP's v4 CIDRs / v6 `::1`, `fe80::/10`, `fc00::/7`. No production caller this phase, but this is the helper Phase 12/14 will call for user-supplied cover URLs. + +**Fix:** After `Unmap()`, also reject `64:ff9b::/96` and `2002::/16`, and if the address is in those prefixes, classify the embedded v4 with the existing v4 table: + +```go +if nat64 := netip.MustParsePrefix("64:ff9b::/96"); nat64.Contains(addr) { + // extract the trailing 32 bits as IPv4 and re-run the v4 table +} +``` + +Keep the PHP CIDR table as the v4/v6 baseline; add the encapsulations as the documented Go-stricter layer. + +### WR-06: Retry-After truncates toward zero + +**File:** `surf/limiter.go:97-100` +**Issue:** `secs := int(retryAfter / time.Second)` floors. A window with 900ms left sends `Retry-After: 0`, which clients treat as "retry immediately" and stampede the same key. Laravel's `ThrottleRequests` ceils the remaining seconds. + +**Fix:** + +```go +secs := int((retryAfter + time.Second - 1) / time.Second) +if secs < 1 { + secs = 1 +} +``` + +### WR-07: Missing body-limit config fail-opens to unlimited + +**File:** `surf/router.go:438-441` +**Issue:** `app.Config.Int("http.body_limits.default_bytes")` returns **0** when the key is absent (`compass/config.go:131-136`). `routeBodyLimit` then returns 0, and `bodyLimit` is not installed. Assemble with a nil/partial config (or a forgotten `http.yaml`) accepts unbounded POST bodies. Fetch defaults explicitly error on non-positive values (D-14); body limits do not. + +**Fix:** If `default_bytes` is missing or `<= 0` at BuildRouter, fail boot (or apply the operator-confirmed 134217728 and log). Do not treat 0 as "unlimited" except on raw groups, where that exemption is already explicit. + +## Info + +### IN-01: InvScope 401/403 bodies have a trailing newline + +**File:** `/media/nvme/dev/golem15/summercms.io/summercms/fonoteka.go/plugins/golem15/fonoteka/middleware/token_scope.go:31-35` +**Issue:** `json.NewEncoder(w).Encode` appends `\n`. Tests hide it with `strings.TrimSpace`. `wire.WriteJSON` exists specifically to match PHP `json_encode` (no trailing newline). JWT `write401` has the same encoder habit (pre-Phase-6), but new token-group bodies should use `wire.WriteJSON`. + +**Fix:** `wire.WriteJSON(w, status, v)`. + +### IN-02: `bouncer.Middleware` and `jwtGuard` duplicate the same chain + +**File:** `bouncer/jwt.go:29-64` and `83-107` +**Issue:** `NewJWTGuard` copies `Middleware`'s body. Nothing in the app still calls `bouncer.Middleware(` — the user plugin derives `jwt.auth` from the registry. Two copies will drift (already almost: both are currently identical). + +**Fix:** Make `Middleware` a thin wrapper over `NewJWTGuard` + `Registry`, or delete the old func once callers are gone. + +### IN-03: CORS will emit the illegal `ACAO: *` + `Allow-Credentials: true` combo + +**File:** `surf/cors.go:105-107` and `92-94` +**Issue:** `corsAllowOrigin` returns `"*"` whenever the origin list contains `*`, then `SupportsCredentials` unconditionally adds `true`. Browsers reject that. Current `http.yaml` has `supports_credentials: false`, so production is fine; a future config change would silently ship a non-functional CORS policy. + +**Fix:** If credentials are on, reflect the request Origin (never `*`), or fail boot when both `*` and credentials are set. + +### IN-04: ListGenres OpenAPI annotation only names the JWT path + +**File:** `/media/nvme/dev/golem15/summercms.io/summercms/fonoteka.go/plugins/golem15/fonoteka/controllers/genre_controller.go:52` +**Issue:** `@Router /_fonoteka/api/v1/genres [get]` omits `GET /api/v1/fonoteka/genres`, the other live mount of the same handler. HTTP-08's generated spec is incomplete for the token surface. + +**Fix:** Add a second `@Router /api/v1/fonoteka/genres [get]` (and regenerate). + +--- + +_Reviewed: 2026-09-19T19:31:09Z_ +_Reviewer: Claude (gsd-code-reviewer)_ +_Depth: standard_