204 lines
16 KiB
Markdown
204 lines
16 KiB
Markdown
---
|
|
phase: 06-http-routing-auth-groups-and-rate-limiting
|
|
reviewed: 2026-09-21T12:27:10Z
|
|
depth: standard
|
|
files_reviewed: 37
|
|
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
|
|
findings:
|
|
critical: 4
|
|
warning: 12
|
|
info: 0
|
|
total: 16
|
|
status: 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:
|
|
|
|
```go
|
|
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_
|