golem15/summercms
3
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

docs(06): add code review report

Browse Source
This commit is contained in:
Jakub Zych
2026-09-21 14:31:47 +02:00
parent ef6d914e4c
commit c5b412c7e1
1 changed files with 101 additions and 114 deletions
Download Patch File Download Diff File Expand all files Collapse all files

215
.planning/phases/06-http-routing-auth-groups-and-rate-limiting/06-REVIEW.md
View File

@@ -1,40 +1,9 @@
---
phase: 06-http-routing-auth-groups-and-rate-limiting
reviewed: 2026-09-20T11:44:42Z
reviewed: 2026-09-21T12:27:10Z
depth: standard
files_reviewed: 68
files_reviewed: 37
files_reviewed_list:
- ../fonoteka.go/config/http.yaml
- ../fonoteka.go/docs/openapi.json
- ../fonoteka.go/parity/fixtures/routes/GET__api_v1_fonoteka_genres_personal_token.yaml
- ../fonoteka.go/parity/genre_security_test.go
- ../fonoteka.go/parity/genres_seed_test.go
- ../fonoteka.go/parity/manifest.yaml
- ../fonoteka.go/parity/parity_contract_test.go
- ../fonoteka.go/parity/parity_test.go
- ../fonoteka.go/parity/php_debug_test.go
- ../fonoteka.go/parity/php_parity.sh
- ../fonoteka.go/plugins/golem15/fonoteka/classes/auth/postgres_test.go
- ../fonoteka.go/plugins/golem15/fonoteka/classes/auth/token_guard.go
- ../fonoteka.go/plugins/golem15/fonoteka/classes/auth/token_guard_coverage_test.go
- ../fonoteka.go/plugins/golem15/fonoteka/classes/auth/token_guard_test.go
- ../fonoteka.go/plugins/golem15/fonoteka/controllers/genre_controller.go
- ../fonoteka.go/plugins/golem15/fonoteka/middleware/public_share_headers.go
- ../fonoteka.go/plugins/golem15/fonoteka/middleware/public_share_headers_test.go
- ../fonoteka.go/plugins/golem15/fonoteka/middleware/token_scope.go
- ../fonoteka.go/plugins/golem15/fonoteka/middleware/token_scope_coverage_test.go
- ../fonoteka.go/plugins/golem15/fonoteka/middleware/token_scope_test.go
- ../fonoteka.go/plugins/golem15/fonoteka/models/api_token.go
- ../fonoteka.go/plugins/golem15/fonoteka/plugin.go
- ../fonoteka.go/plugins/golem15/fonoteka/plugin_boot_test.go
- ../fonoteka.go/plugins/golem15/fonoteka/routes.go
- ../fonoteka.go/plugins/golem15/fonoteka/routes_bucket_test.go
- ../fonoteka.go/plugins/golem15/fonoteka/routes_cors_test.go
- ../fonoteka.go/plugins/golem15/fonoteka/routes_group_test.go
- ../fonoteka.go/plugins/golem15/fonoteka/routes_isolation_test.go
- ../fonoteka.go/plugins/golem15/user/plugin.go
- ../fonoteka.go/scripts/check-openapi.sh
- ../fonoteka.go/scripts/swagger2openapi.go
- bouncer/context.go
- bouncer/context_test.go
- bouncer/guard.go
@@ -74,143 +43,161 @@ files_reviewed_list:
- wire/response_test.go
findings:
critical: 4
warning: 11
warning: 12
info: 0
total: 15
total: 16
status: issues_found
---
# Phase 6: Code Review Report
**Reviewed:** 2026-09-20T11:44:42Z
**Reviewed:** 2026-09-21T12:27:10Z
**Depth:** standard
**Files Reviewed:** 68
**Files Reviewed:** 37
**Status:** issues_found
## Summary
The review found 15 defects, including four blockers. The limiter permits concurrent over-limit requests and lets anonymous callers choose a bucket through the untrusted Host header. The SSRF guard accepts transition-address encodings of private IPv4 targets, and the panic recovery can commit a successful partial response before appending the supposedly opaque 500 body. Eleven additional robustness and contract defects remain. Passing tests do not exercise these adversarial paths.
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 review:
Verification run during this review:
- `go test ./bouncer ./fetchguard ./surf ./wire -short` — pass
- `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
- `go test ./plugins/golem15/fonoteka/... -short` — pass
- `go test ./parity -short` — pass
## Narrative Findings (AI reviewer)
## Critical Issues
### CR-01: Check-then-increment is not atomic, so concurrent requests exceed the configured limit
### CR-01: Named middleware executes before the request body cap
**Classification:** BLOCKER
**File:** `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/limiter.go:95-115`; `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/limiter_store.go:73-100`
**Issue:** Each request calls `TooManyAttempts` and `Hit` as two separately locked operations. If a key is at `Max-1`, multiple requests can all observe “not limited” before any of them calls `Hit`; every one is then admitted even though the count advances beyond `Max`. With `Max: 1`, two simultaneous first requests can both pass. This is a direct bypass of the security control under normal HTTP concurrency, and all tests exercise it sequentially.
**Fix:** Make admission one atomic store operation. For example, replace the three-call protocol with `Attempt(key, max, decay) (allowed bool, attempts int, retryAfter time.Duration)` and perform expiry, threshold check, increment, and retry calculation under the same mutex/transaction. Add a barrier-based concurrent test in which many requests hit a `Max: 1` bucket and exactly one reaches the handler.
**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:
### CR-02: Inline throttles use the attacker-controlled Host header as part of the key
```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/surf/limiter.go:149-165`
**Issue:** Anonymous inline throttles key on `r.Host + "|" + ClientIP(...)`. `Host` is request input, not a registered route domain. Unless every upstream rejects or rewrites arbitrary Host values, a caller can rotate `Host` on every request and obtain a fresh bucket, bypassing `throttle:10,1` and other inline limits. The onboarding and public-invitation groups are declared with inline throttles, so this becomes live as soon as their handlers land.
**Fix:** Remove `r.Host` from the key. The inline throttle parameter already identifies the bucket, so use a stable server-side prefix such as `"inline:" + param + "|" + ClientIP(r, trusted)`. If host-scoped routes are introduced later, key from the trusted registered route metadata, never the incoming Host header. Add a test proving different Host headers from one IP share a bucket.
**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: The SSRF guard allows private IPv4 targets encoded through IPv6 transition prefixes
### CR-03: Invalid or overflowing bucket definitions silently bypass rate limiting
**Classification:** BLOCKER
**File:** `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/fetchguard/ip.go:5-45`; `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/fetchguard/fetch.go:145-163`
**Issue:** `Unmap` only handles IPv4-mapped `::ffff:x.x.x.x`. The allow decision treats all other global-looking IPv6 addresses as public. On networks supporting the corresponding transition mechanism, an attacker-controlled DNS record can therefore target loopback, RFC1918, or metadata addresses via values such as `64:ff9b::7f00:1` (NAT64 loopback), `64:ff9b::a9fe:a9fe` (NAT64 metadata), or `2002:7f00:1::` (6to4 loopback). The helper is explicitly intended as an SSRF boundary, so an environment-dependent route to private infrastructure is still a vulnerability.
**Fix:** Classify IPv6 transition prefixes before dialing. Extract and re-check embedded IPv4 for `64:ff9b::/96`, the local-use NAT64 prefix `64:ff9b:1::/48`, and 6to4 `2002::/16`; reject other transition forms that cannot be safely decoded. Add table tests for embedded loopback, RFC1918, link-local metadata, and public IPv4 values.
**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: Panic recovery can return 200 and leak a partial response instead of the opaque 500
### 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:520-541`
**Issue:** Both recovery wrappers write their 500 only after recovering. If a handler or middleware has already called `Write`/`WriteHeader`, the status and bytes are committed; the later `WriteHeader(500)` is ignored. A non-raw handler that writes sensitive partial JSON and then panics will return the earlier status (often 200) with the partial bytes followed by the opaque error JSON. The tests only panic before any write, so they do not cover this broken error boundary.
**Fix:** Buffer non-streaming house responses until the route returns successfully, then commit them. On panic, discard the buffered status/headers/body and write only the opaque 500. Raw routes need an explicit policy: either buffer them too or document that they cannot promise a clean recovery after a committed write. Add partial-write-then-panic tests for both raw and house routes.
**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: Invalid bucket definitions and missing stores silently disable rate limiting
### WR-01: Missing or malformed body-limit configuration disables the cap
**Classification:** WARNING
**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-94`
**Issue:** `RegisterBucket` accepts `Key == nil`, `Max <= 0`, and `Decay <= 0`. At request time a nil key, nil store, or resolution error simply calls the protected handler. A nil key therefore disables the control, and a non-positive decay continually expires the entry so requests remain effectively unlimited. These are boot-time configuration errors and should not produce a fail-open live route.
**Fix:** Reject nil `Key`, `Max < 1`, `Decay <= 0`, and nil stores during construction/registration. If an impossible runtime error remains, return an opaque 500 rather than calling `next`.
**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: Missing or malformed body-limit configuration turns the limit off
### WR-02: A typed-nil guard passes registration and can panic authentication
**Classification:** WARNING
**File:** `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/router.go:432-441`; `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/bodylimit.go:29-45`
**Issue:** `Config.Int` yields zero for absent and non-coercible values. `BuildRouter` copies that zero to `defaultBytes`, and `wrap` omits `MaxBytesReader` whenever the value is not positive. A missing `http.yaml`, typo, or malformed value therefore changes the advertised default from 128 MiB to unlimited without a boot error.
**Fix:** Read the raw value with presence/type checks and fail boot unless `default_bytes` is a positive integer. Keep “unlimited” available only through the explicit raw-route exemption.
**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: Trusted-proxy parsing silently drops valid operational forms and invalid entries
### WR-03: Fractional or imprecise numeric JWT subjects authenticate a different user
**Classification:** WARNING
**File:** `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/clientip.go:49-82`
**Issue:** `TrustedProxies` only accepts CIDR strings and silently skips everything else. A common deployment value such as `127.0.0.1` is discarded because it is not written as `/32`; a typo is discarded the same way. Behind a reverse proxy this collapses all users onto the proxy address and corrupts every IP-keyed limiter budget, with no startup signal.
**Fix:** Accept a bare `netip.Addr` by converting it to `/32` or `/128`, return an error for unparseable entries, and propagate that error from `BuildRouter` so the application fails boot.
**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: Token last-used auditing ignores the framework's trusted-proxy client IP
### WR-04: Credentialed CORS can emit the browser-invalid wildcard combination
**Classification:** WARNING
**File:** `/media/nvme/dev/golem15/summercms.io/summercms/fonoteka.go/plugins/golem15/fonoteka/classes/auth/token_guard.go:50-60`; `/media/nvme/dev/golem15/summercms.io/summercms/fonoteka.go/plugins/golem15/fonoteka/classes/auth/token_guard.go:79-88`
**Issue:** `last_used_ip` is derived directly from `RemoteAddr`, while rate-limit keys use `surf.ClientIP`. Behind a configured trusted proxy the same request is attributed to the real client for limiting but to the proxy for audit history, making the security audit field misleading.
**Fix:** Parse trusted proxies once during plugin boot and inject them into `TokenGuard`; use `surf.ClientIP(r, trusted)` for the audit stamp so there is one client-IP policy.
**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: Retry-After is rounded down and can tell clients to retry before the window expires
### WR-05: Inner response headers can erase `Vary: Origin`
**Classification:** WARNING
**File:** `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/limiter.go:96-103`
**Issue:** Integer duration division floors the remaining time. A request with 900 ms left receives `Retry-After: 0`; a nearly fresh one-minute window normally reports 59 instead of 60. Compliant clients can immediately retry and receive another 429.
**Fix:** Round up to whole seconds and clamp live windows to at least one second, e.g. `(retryAfter + time.Second - 1) / time.Second`.
**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: Middleware factories are invoked twice during normal Assemble
**Classification:** WARNING
**File:** `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/router.go:336-348`; `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/router.go:487-492`
**Issue:** `BuildRouter` calls `wrap` once per route as validation, and `Assemble` immediately calls `compile`, which calls `wrap` again. Each `factory.fn(param)` therefore runs twice. Plugin factories are an extension API and may allocate resources, register metrics, or bind single-use state; the first constructed chain is discarded. A factory returning nil also panics rather than producing a contextual boot error.
**Fix:** Split pure route validation from middleware construction, or compile once and retain the compiled handler. Validate factory and middleware return values and turn nil into a plugin/name-specific boot error. Add a factory call-count test around `Assemble`.
### WR-07: The generated OpenAPI document omits a live route and all authentication requirements
**Classification:** WARNING
**File:** `/media/nvme/dev/golem15/summercms.io/summercms/fonoteka.go/plugins/golem15/fonoteka/controllers/genre_controller.go:45-52`; `/media/nvme/dev/golem15/summercms.io/summercms/fonoteka.go/docs/openapi.json:40-73`
**Issue:** The same handler is live at both `/_fonoteka/api/v1/genres` and `/api/v1/fonoteka/genres`, but only the JWT path is documented. The documented operation also has no bearer security scheme or operation security requirement, so generated clients see a public endpoint and cannot discover the personal-token surface.
**Fix:** Add both route annotations, define the appropriate bearer schemes, annotate each operation's security requirement, document its 401/403/422/423 responses, and regenerate `docs/openapi.json`.
### WR-08: The Swagger-to-OpenAPI converter emits invalid OpenAPI for supported Swagger constructs
**Classification:** WARNING
**File:** `/media/nvme/dev/golem15/summercms.io/summercms/fonoteka.go/scripts/swagger2openapi.go:99-134`; `/media/nvme/dev/golem15/summercms.io/summercms/fonoteka.go/scripts/swagger2openapi.go:136-209`
**Issue:** Path-item `parameters` are copied unchanged instead of being converted; `formData` parameters are also left as OpenAPI 2 parameters rather than combined into an OpenAPI 3 `requestBody`. Top-level `produces`/`consumes` are ignored. These are standard Swagger 2 constructs, especially for the upload routes anticipated by this phase, and will either make the validation step fail or silently assign the wrong media type.
**Fix:** Carry document-level media types into path/operation conversion, run path-level parameters through the same conversion, and translate body/form-data parameters into request bodies (including multipart schemas). Add table-driven converter tests with path parameters, JSON bodies, multipart uploads, and non-JSON global media types.
### WR-09: CORS can generate the forbidden wildcard-origin plus credentials combination
**Classification:** WARNING
**File:** `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/cors.go:69-95`; `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/surf/cors.go:104-119`
**Issue:** When `allowed_origins` contains `*`, `corsAllowOrigin` always returns `*`; `SupportsCredentials` independently adds `Access-Control-Allow-Credentials: true`. Browsers reject that combination. The checked-in config currently disables credentials, but a valid-looking future config change silently produces a non-functional policy.
**Fix:** Fail boot on wildcard plus credentials, or reflect a validated request Origin and emit `Vary: Origin` when credentials are enabled.
### WR-10: An unknown Fetch policy mode fails open to PublicOnlyMode
### 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 host allow-list runs only when `policy.Mode == AllowHostsMode`; every other integer, including invalid values, behaves as `PublicOnlyMode`. A bad cast/configuration can therefore broaden a host allow-list to every public host.
**Fix:** Switch explicitly on the two defined modes and return a typed configuration error in the default case. Add an invalid-mode test that performs no network I/O.
**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-11: InvScope's wire body does not match the project's no-newline JSON convention
### WR-07: Limit arithmetic can overflow into incorrect fetch behavior
**Classification:** WARNING
**File:** `/media/nvme/dev/golem15/summercms.io/summercms/fonoteka.go/plugins/golem15/fonoteka/middleware/token_scope.go:31-35`; `/media/nvme/dev/golem15/summercms.io/summercms/fonoteka.go/plugins/golem15/fonoteka/middleware/token_scope_test.go:71-76`
**Issue:** `json.Encoder.Encode` appends a newline, while `wire.WriteJSON` exists specifically to reproduce PHP `json_encode` without one. The tests call `TrimSpace`, hiding the byte-level mismatch for both 401 and 403 responses.
**Fix:** Replace the local writer with `wire.WriteJSON` and assert exact response bytes in the tests without trimming.
**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-20T11:44:42Z_
_Reviewed: 2026-09-21T12:27:10Z_
_Reviewer: the agent (gsd-code-reviewer)_
_Depth: standard_