14 KiB
phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status
| phase | reviewed | depth | files_reviewed | files_reviewed_list | findings | status | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 06-http-routing-auth-groups-and-rate-limiting | 2026-09-19T19:31:09Z | standard | 48 |
|
|
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:
inv_token(CredentialGuard; on failure it does not write 401 — it passes through)inv.scope:read(401{"error":"Invalid token"}and returns)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):
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:
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:
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:
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:
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:
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:
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:
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