17 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-20T11:44:42Z | standard | 68 |
|
|
issues_found |
Phase 6: Code Review Report
Reviewed: 2026-09-20T11:44:42Z Depth: standard Files Reviewed: 68 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.
Verification run during review:
go test ./bouncer ./fetchguard ./surf ./wire -short— passgo vet ./bouncer ./fetchguard ./surf ./wire ./internal/build— passgo test ./plugins/golem15/fonoteka/... -short— passgo 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
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.
CR-02: Inline throttles use the attacker-controlled Host header as part of the key
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.
CR-03: The SSRF guard allows private IPv4 targets encoded through IPv6 transition prefixes
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.
CR-04: Panic recovery can return 200 and leak a partial response instead of the opaque 500
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.
Warnings
WR-01: Invalid bucket definitions and missing stores silently disable rate limiting
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.
WR-02: Missing or malformed body-limit configuration turns the limit off
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.
WR-03: Trusted-proxy parsing silently drops valid operational forms and invalid entries
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.
WR-04: Token last-used auditing ignores the framework's trusted-proxy client IP
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.
WR-05: Retry-After is rounded down and can tell clients to retry before the window expires
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.
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
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.
WR-11: InvScope's wire body does not match the project's no-newline JSON convention
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.
Reviewed: 2026-09-20T11:44:42Z Reviewer: the agent (gsd-code-reviewer) Depth: standard