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

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-21T12:27:10Z	standard	37	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	critical warning info total 4 12 0 16	issues_found

Phase 6: Code Review Report

Reviewed: 2026-09-21T12:27:10Z Depth: standard Files Reviewed: 37 Status: issues_found

Summary

The current post-gap implementation still has four blockers and twelve warnings. The most serious problems are that named middleware can consume an unlimited request body before the body cap is installed, the SSRF classifier still treats multiple non-public address ranges as public, malformed bucket definitions can silently bypass throttling, and route conflicts can panic application assembly instead of returning an error. The passing tests do not cover these paths.

Verification run during this review:

• go test ./bouncer ./fetchguard ./surf ./wire ./internal/build -count=1 -short — pass
• go test -race ./bouncer ./fetchguard ./surf ./wire -count=1 -short — pass
• go vet ./bouncer ./fetchguard ./surf ./wire ./internal/build — pass

Narrative Findings (AI reviewer)

Critical Issues

CR-01: Named middleware executes before the request body cap

Classification: BLOCKER File: /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/router.go:353-399; /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/bodylimit.go:10-17 Issue: wrap applies bodyLimit to the handler first, then wraps that result in every named middleware. Requests therefore flow through authentication, scope, plugin, and other named middleware before r.Body is replaced by http.MaxBytesReader. Any such middleware that decodes or buffers the body can read an arbitrarily large payload, bypassing the phase's request-size denial-of-service boundary. The body-limit tests read only in the terminal handler, so they cannot detect the bypass. Fix: Apply the selected body limit after the named-middleware loop, so it is outside all middleware that could consume the request body while remaining inside recovery:

h := constrain(rt.handler, rt.constraints)
h = orgSlot(h)
// resolve and wrap named middleware here
if limit > 0 {
	h = bodyLimit(limit)(h)
}
h = locale(h)

Add a regression middleware that reads r.Body and prove an oversized request fails before that middleware can consume beyond the cap.

CR-02: The SSRF boundary allows non-public special-use destinations

Classification: BLOCKER File: /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/fetchguard/ip.go:5-54; /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/fetchguard/fetch.go:145-163 Issue: The classifier blocks a small hand-written set, multicast, and unspecified addresses, but treats every other address as public. IPv4 ranges such as 198.18.0.0/15 (benchmark networks commonly routed internally), 192.0.0.0/24, documentation ranges, and 240.0.0.0/4 are accepted. Native IPv6 special-use ranges beyond loopback/link-local/ULA are also accepted. This contradicts PublicOnlyMode: an attacker-controlled DNS record can resolve to a non-public range that is reachable inside a deployment and the dial hook will allow it. Fix: Base the decision on a maintained IANA special-purpose deny table (including IPv4 and IPv6 exceptions whose global flag is true), then apply the existing transition-address decoding before the final decision. Add boundary tests for every denied prefix, especially 198.18.0.1, 192.0.0.1, 240.0.0.1, and representative native IPv6 special-use addresses.

CR-03: Invalid or overflowing bucket definitions silently bypass rate limiting

Classification: BLOCKER 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-96; /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/limiter.go:139-153; /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/limiter_store.go:71-92 Issue: RegisterBucket accepts Key == nil, Max <= 0, and Decay <= 0. A nil key makes middleware call the protected handler directly; a non-positive decay causes each request to observe an already-expired window and be admitted. Inline throttles also multiply an unbounded integer by time.Minute; overflow can produce a non-positive decay with the same bypass. A nil store is likewise treated as permission to continue. These are security-control configuration errors but the implementation fails open. Fix: Reject a nil store at construction and reject buckets unless Key != nil, Max >= 1, and Decay > 0. Before converting inline minutes, enforce m <= int(math.MaxInt64/int64(time.Minute)). Remove the runtime next fail-open path for impossible invalid state and add boot-failure tests for every invalid definition and an overflowing inline duration.

CR-04: Conflicting or invalid route patterns panic application assembly

Classification: BLOCKER File: /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/router.go:318-349 Issue: The seen map detects only byte-identical method/path strings. http.ServeMux.Handle panics for malformed patterns and semantic conflicts such as GET /items/{id} versus GET /items/{slug}. Because compile calls Handle without validation or recovery, plugin route input can crash boot instead of honoring the framework's fail-boot-with-error contract. Fix: Register each pattern through a helper that recovers ServeMux's pattern panic and converts it to an error containing the method, path, and plugin owner. Also reject nil handlers before registration. Add tests for malformed wildcard syntax and two differently named but equivalent wildcard routes.

Warnings

WR-01: Missing or malformed body-limit configuration disables the cap

Classification: WARNING File: /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/router.go:434-443; /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/bodylimit.go:29-45 Issue: BuildRouter reads both limits through Config.Int, whose absent-value result is zero. wrap applies no reader when the effective value is non-positive. A missing key, typo, wrong type, nil config, or negative value therefore silently changes every non-raw route from capped to unlimited. Fix: Parse both keys with presence and type checks, require positive int64 values, and return a contextual boot error. If a framework fallback is desired, make it an explicit positive constant rather than using zero as both “missing” and “disabled.”

WR-02: A typed-nil guard passes registration and can panic authentication

Classification: WARNING File: /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/bouncer/registry.go:28-47; /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/bouncer/registry.go:87-95 Issue: g == nil does not detect an interface holding a nil guard pointer. Such a value satisfies Guard/CredentialGuard, is registered, and is later invoked. Pointer-receiver implementations such as jwtGuard then dereference a nil receiver and panic. This violates Register's documented nil rejection. Fix: Reject nil-capable dynamic values with a small reflection helper (reflect.ValueOf(g).Kind() plus IsNil) before interface assertions, and add typed-nil tests for both guard interfaces.

WR-03: Fractional or imprecise numeric JWT subjects authenticate a different user

Classification: WARNING File: /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/bouncer/jwt.go:146-159 Issue: A float64 subject is truncated with int64(v) without requiring an integer or checking its safe range. A valid signed token with "sub": 1.9 authenticates user 1; large numeric subjects can also be rounded before conversion. The sub claim is an identity boundary and must not be lossy. Fix: Prefer accepting only non-empty string subjects. If numeric compatibility is required, require math.Trunc(v) == v, reject values outside the exact integer/supported user-ID range, and format only after those checks. Add fractional and greater-than-2^53 regression cases.

WR-04: Credentialed CORS can emit the browser-invalid wildcard combination

Classification: WARNING File: /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/cors.go:52-95; /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/cors.go:104-119 Issue: allowed_origins: ["*"] always emits Access-Control-Allow-Origin: *, while supports_credentials: true independently emits Access-Control-Allow-Credentials: true. Browsers reject that combination. Wildcard methods and headers also do not have wildcard semantics for credentialed requests, so a plausible configuration silently produces non-functional CORS. Fix: Fail boot on incompatible wildcard/credentials configuration, or validate and reflect the request origin/method/headers with Vary when credentials are enabled. Add a credentialed wildcard configuration test.

WR-05: Inner response headers can erase Vary: Origin

Classification: WARNING File: /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/cors.go:69-75; /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/router.go:579-588 Issue: CORS sets Vary: Origin on the destination writer before calling the buffered route. On success, bufferedResponse.commit replaces destination values for every header set by the handler. If a handler or middleware sets Vary: Accept-Encoding (or any other value), commit overwrites Origin, allowing shared caches to reuse the wrong CORS variant. Fix: Merge Vary tokens case-insensitively during commit, or apply CORS headers through a response wrapper at final WriteHeader time. Add a test where the route sets its own Vary value and assert both tokens survive.

WR-06: Unknown fetch policy modes broaden to public-host access

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 allow-list is checked only for exact AllowHostsMode; every other integer, including an invalid enum value, behaves as PublicOnlyMode. A bad cast or configuration therefore broadens a restricted policy to every accepted public address. Fix: Switch explicitly over AllowHostsMode and PublicOnlyMode, returning a typed error in the default branch before any network operation. Add an invalid-mode/no-dial test.

WR-07: Limit arithmetic can overflow into incorrect fetch behavior

Classification: WARNING File: /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/fetchguard/fetch.go:87-92; /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/fetchguard/policy.go:96-105 Issue: maxBytes+1 overflows when the caller/config supplies math.MaxInt64; io.LimitReader then receives a negative limit and the fetch returns a successful empty body instead of enforcing the requested cap. time.Duration(n) * time.Second can also overflow for a positive config integer and yield an unrelated positive duration that passes the later > 0 check. Fix: Reject maxBytes == math.MaxInt64 (or use an overflow-safe bounded-read helper) and require n <= int64(math.MaxInt64/time.Second) before converting timeout seconds. Add maximum-boundary tests.

WR-08: Invalid trusted-proxy entries are silently discarded

Classification: WARNING File: /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/clientip.go:49-82 Issue: Malformed entries and common bare-IP forms are skipped without an error. A typo can collapse all users behind a proxy onto the proxy address, corrupting every IP-keyed rate-limit budget while startup still reports success. Fix: Return ([]netip.Prefix, error), accept bare IPs as /32 or /128 if desired, and propagate invalid entries as a boot error from BuildRouter. Include the bad value and config path in the error.

WR-09: Retry-After is rounded down

Classification: WARNING File: /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/limiter.go:97-103 Issue: Integer duration division floors a live window. A fresh one-minute window usually reports 59, and a sub-second remainder reports 0, telling compliant clients to retry before the limiter will admit them. Fix: Round up and clamp live windows to at least one second, for example secs := max(1, int((retryAfter+time.Second-1)/time.Second)), and test both minute and sub-second buckets.

WR-10: Middleware factories are constructed twice and nil results panic

Classification: WARNING File: /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/router.go:353-393; /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/router.go:489-493 Issue: BuildRouter invokes wrap once for validation, and Assemble immediately invokes it again through compile. Every factory therefore runs twice and the first middleware chain is discarded. Factories may allocate resources or bind single-use state. A factory returning a nil middleware/handler also causes an uncontextualized panic instead of a plugin-specific boot error. Fix: Split pure validation from construction or compile each route once and retain the result. Validate both the returned middleware function and handler for nil, returning an error that names the plugin and middleware reference. Add factory call-count and nil-return tests around Assemble.

WR-11: The configured upload limit is loaded but never enforced

Classification: WARNING File: /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/router.go:55-59; /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/router.go:434-443; /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/bodylimit.go:29-45 Issue: http.body_limits.upload_bytes is copied into Router.uploadBytes, but no code reads the field. Tests assert only that it was loaded. Operators can change the advertised upload limit without affecting any request, and future upload routes will continue using the default unless they duplicate a literal byte count. Fix: Either remove the unused setting/field or define and enforce a documented upload override (for example body.limit:upload resolved from uploadBytes) and test a real route above the default but below the upload cap.

WR-12: Invalid CORS origin regexes silently disappear

Classification: WARNING File: /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/cors.go:37-51 Issue: A malformed allowed_origins_patterns entry is skipped. The application boots with a policy different from its configuration, leaving intended frontends unable to use the API and providing no diagnostic. Fix: Compile and validate all patterns while loading/building the router and return a config-path-specific boot error on the first invalid expression. Add a malformed-pattern test that expects BuildRouter to fail.

---

Reviewed: 2026-09-21T12:27:10Z Reviewer: the agent (gsd-code-reviewer) Depth: standard