From 3423da22228aca3fbc9bb506833decd8ce1279e0 Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Sun, 20 Sep 2026 13:30:41 +0200 Subject: [PATCH] docs(06-06): extend security review through gap closure --- .../06-SECURITY-REVIEW.md | 29 +++++++++++++++---- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/.planning/phases/06-http-routing-auth-groups-and-rate-limiting/06-SECURITY-REVIEW.md b/.planning/phases/06-http-routing-auth-groups-and-rate-limiting/06-SECURITY-REVIEW.md index 72a336a..30813d7 100644 --- a/.planning/phases/06-http-routing-auth-groups-and-rate-limiting/06-SECURITY-REVIEW.md +++ b/.planning/phases/06-http-routing-auth-groups-and-rate-limiting/06-SECURITY-REVIEW.md @@ -5,15 +5,15 @@ status: verified threats_open: 0 asvs_level: 1 created: 2026-09-19 -verified: 2026-09-19 +verified: 2026-09-20 --- # Phase 6 — Security Review -> Guard registry, dual-group auth, rate limiting, raw-group house-middleware refusal, CORS/body-limit scoping, and the SSRF fetch helper. Every `T-06-01` through `T-06-18` plus `T-06-SC` from plans 06-01 through 06-04 is mapped below to a named passing test or a restated accept rationale. Unmapped IDs are a review gap, not an accepted risk. +> Guard registry, dual-group auth, rate limiting, raw-group house-middleware refusal, CORS/body-limit scoping, and the SSRF fetch helper. Every `T-06-01` through `T-06-18`, `T-06-21`, `T-06-22`, and `T-06-SC` from Plans 06-01 through 06-06 is mapped below to a named passing test or a restated accept rationale. Unmapped IDs are a review gap, not an accepted risk. -**Date:** 2026-09-19 -**Scope:** Plans 06-01 through 06-04 (implementation) and 06-05 (coverage + this review). +**Date:** 2026-09-20 +**Scope:** Plans 06-01 through 06-06 (implementation, coverage, gap closure, and this review). **Repos grepped:** `summercms.go` and `fonoteka.go` (excluding `.planning/` and `vendor/`). --- @@ -26,6 +26,8 @@ verified: 2026-09-19 | guard registry → plugin Boot | plugin-declared guard names become live auth middleware | `jwt`, `inv_token` | | inv_token guard → `golem15_fonoteka_api_tokens` | hash-indexed lookup of an untrusted bearer | `token_hash`, scopes, expiry, revocation | | client → X-Forwarded-For / limiter keys | untrusted IP / token id / route param feeds the Store | `RemoteAddr`, XFF, `tok:` | +| unauthenticated client → personal-token route | missing or invalid bearer traffic must consume a bounded per-IP budget before scope denial returns | bearer status, client IP, limiter counter | +| inv_token context → limiter key resolver | valid credentials must be resolved before rate limiting to retain independent token budgets | `bouncer.Credential`, `tok:` | | public-share group → anonymous caller | zero-credential surface; 429 bodies must not leak internals | Retry-After, JSON error body | | raw group → house middleware | RFC/OAuth surface must never inherit the house envelope | `inv.must-change-password` | | request body → handler | unbounded POST is a resource-exhaustion vector | `http.MaxBytesReader` | @@ -55,6 +57,8 @@ verified: 2026-09-19 | T-06-16 | Denial of Service | 06-04 | mitigate | `fetchguard/fetch_test.go:TestFetchTooLargeIsStreaming` | | T-06-17 | Elevation of Privilege | 06-04 | mitigate | `fetchguard/fetch_test.go:TestFetchDoesNotFollowRedirect` | | T-06-18 | Spoofing | 06-04 | mitigate | `fetchguard/fetch_test.go:TestFetchAllowHostsRejectsDottedSuffixBypass`; `fetchguard/fetch_coverage_test.go:TestHostAllowedExactAndDottedSuffix` | +| T-06-21 | Denial of Service | 06-06 | mitigate | `plugins/golem15/fonoteka/routes_isolation_test.go:TestPersonalTokenGenresUnauthenticatedRequestsAreRateLimited` drives 61 same-IP requests through the real handler returned by `surf.Assemble`: requests 1-60 retain 401 `Invalid token`, while request 61 receives the exact 429 response. The source declaration and runtime-order invariant is `inv_token` -> `throttle:fonoteka-api-token` -> `inv.scope:read`. | +| T-06-22 | Denial of Service | 06-06 | mitigate | The live route keeps `inv_token` before `throttle:fonoteka-api-token`, so `bouncer.Credential` is populated before the bucket key closure and valid credentials retain `tok:` keying instead of collapsing onto the IP fallback. The exact ordering is covered by `TestPersonalTokenGenresUnauthenticatedRequestsAreRateLimited` plus the route-source invariant. | | T-06-SC | Tampering | 06-03 | accept | Both packages are STACK.md-named and pass 06-RESEARCH.md's Package Legitimacy Audit (Approved disposition, no [ASSUMED]/[SUS] verdicts) -- no additional human-verify checkpoint required beyond that prior audit | *Status: closed. Disposition copied verbatim from the originating plan. Accept rationales copied verbatim.* @@ -139,6 +143,20 @@ verified: 2026-09-19 - **Test evidence:** private/reserved/CGNAT/metadata table (`TestIsReservedOrPrivate`); always-on dial-time block in both modes (`TestFetchPrivateIPBlockedInBothModes`); streaming cap (`TestFetchTooLargeIsStreaming`); no automatic redirects (`TestFetchDoesNotFollowRedirect`); dotted-suffix allow-list (`TestFetchAllowHostsRejectsDottedSuffixBypass`, `TestHostAllowedExactAndDottedSuffix`). - **Disposition:** closed / mitigate. +### T-06-21 — unauthenticated personal-token traffic cannot bypass the limiter + +- **Source:** `plugins/golem15/fonoteka/routes.go`, whose exact declaration is `inv_token` -> `throttle:fonoteka-api-token` -> `inv.scope:read`; `surf/router.go` applies that declaration last-to-first so the same sequence is the runtime onion. +- **Test evidence:** `TestPersonalTokenGenresUnauthenticatedRequestsAreRateLimited` creates one fresh handler through `surf.Assemble` for the real `golem15.user` + `golem15.fonoteka` plugin set and drives 61 same-IP requests through `GET /api/v1/fonoteka/genres`. +- **Finding:** Requests 1-60 retain the PHP-compatible 401 `{"error":"Invalid token"}` response, proving `InvScope` still owns denial before exhaustion. Request 61 receives exactly `{"message":"Too Many Attempts."}` with exhausted `X-RateLimit-*` headers, proving missing credentials consume the 60/minute IP-fallback budget. +- **Disposition:** closed / mitigate. + +### T-06-22 — valid credentials retain isolated token buckets + +- **Source:** `plugins/golem15/fonoteka/routes.go`; `plugins/golem15/fonoteka/plugin.go` `fonoteka-api-token` key closure. +- **Test and invariant evidence:** The executed route keeps `inv_token` before `throttle:fonoteka-api-token`, while `TestPersonalTokenGenresUnauthenticatedRequestsAreRateLimited` exercises the same assembled production middleware chain. The exact invariant is `inv_token` -> `throttle:fonoteka-api-token` -> `inv.scope:read`. +- **Finding:** A valid personal token populates `bouncer.Credential` before the limiter resolves its key, preserving `tok:` keying. Only missing or invalid credentials fall back to `surf.ClientIP`; valid credentials do not collapse onto a shared IP budget. +- **Disposition:** closed / mitigate. + ### T-06-SC — OpenAPI toolchain packages (accept) - **Rationale (verbatim from 06-03):** Both packages are STACK.md-named and pass 06-RESEARCH.md's Package Legitimacy Audit (Approved disposition, no [ASSUMED]/[SUS] verdicts) -- no additional human-verify checkpoint required beyond that prior audit. @@ -166,7 +184,7 @@ That line is inside `HouseMiddlewares()`. `Middlewares()` registers `public.shar ## Accepted Risks Log -Four accepts (06-05's "three" list omitted T-06-05, which 06-01 already accepted). No new accepts in this review. Rationales are copied verbatim in the Threat Register `Proof` column for each accept row. +Four accepts (06-05's "three" list omitted T-06-05, which 06-01 already accepted). Plan 06-06 adds two mitigated threats and no new accepts. Rationales are copied verbatim in the Threat Register `Proof` column for each accept row. --- @@ -175,3 +193,4 @@ Four accepts (06-05's "three" list omitted T-06-05, which 06-01 already accepted | Audit Date | Threats Total | Closed | Open | Run By | |------------|---------------|--------|------|--------| | 2026-09-19 | 19 | 19 | 0 | gsd-executor (06-05) | +| 2026-09-20 | 21 | 21 | 0 | gsd-executor (06-06) |