docs(06): add code review report
This commit is contained in:
Jakub Zych
@@ -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:<id>` 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_<garbage>` 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:<id>` else `ClientIP`) **if it runs**. It just never runs on the deny path.
|
||||
|
||||
**Fix:** Keep `inv_token` **before** throttle (so `tok:<id>` 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:<id>`.
|
||||
|
||||
## 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_
|
||||
Reference in New Issue
Block a user