From ae817ccbb9d94304b51d54fbc470dbecb64acbe7 Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Mon, 21 Sep 2026 19:49:30 +0200 Subject: [PATCH] docs(06-14): reopen and re-close phase 6 security review with T-06-28..35 --- .../06-SECURITY-REVIEW.md | 82 +++++++++++++++++-- 1 file changed, 75 insertions(+), 7 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 4871952..b780ff1 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 @@ -2,13 +2,15 @@ phase: 06 slug: http-routing-auth-groups-and-rate-limiting status: verified -threats_total: 26 -threats_closed: 26 +threats_total: 34 +threats_closed: 34 threats_open: 0 accepted_risks: 4 asvs_level: 1 created: 2026-09-19 verified: 2026-09-21 +reopened: 2026-09-21 +reverified: 2026-09-21 --- # Phase 6 — Security Review @@ -16,14 +18,18 @@ verified: 2026-09-21 > Guard registry, dual-group auth, atomic rate limiting, raw-group house-middleware refusal and transactional panic recovery, CORS/body-limit scoping, transition-aware SSRF protection, and exact personal-token denial serialization. Every reviewed ID from Plans 06-01 through 06-10 is mapped below to a named passing test or a restated accept rationale; Plan 06-11 refreshes the review only after both repositories pass their complete race and vet gates. Unmapped IDs are a review gap, not an accepted risk. **Date:** 2026-09-21 -**Scope:** Plans 06-01 through 06-10 (implementation, coverage, and corrective gap closure) plus the Plan 06-11 review refresh. +**Scope:** Plans 06-01 through 06-10 (implementation, coverage, and corrective gap closure), the Plan 06-11 review refresh (superseded, see Reopened), and gap-closure Plans 06-12 through 06-14 (T-06-28 through T-06-35). **Repos grepped:** `summercms.go` and `fonoteka.go` (excluding `.planning/` and `vendor/`). --- +## Reopened + +The 2026-09-21 verdict of 26 closed / 0 open (Plan 06-11) was contradicted by phase verification: named middleware could read past the body cap, invalid limiter definitions failed open, the SSRF classifier missed IANA special-use ranges and zoned IPv6, and several warning-class defects existed. That verdict is **superseded**; its audit-trail row is retained below. Plans 06-12 and 06-13 fixed the code and Plan 06-14 added the regression proof, so T-06-28 through T-06-35 were added, and T-06-12 is annotated below. + ## Verdict Summary -The post-gap implementation closes all five threats promoted by the Phase 6 verifier and code review. The reviewed register now contains **26 total threats: 26 closed, 0 open, with 4 unchanged accepted risks and no new accepted risk**. This verdict was published only after `go test ./... -count=1 -race -short` and `go vet ./...` passed in both `summercms.go` and `fonoteka.go`. +The register contains **34 total threats: 34 closed, 0 open, with 4 unchanged accepted risks and no new accepted risk**. This verdict follows the 2026-09-21 Plan 06-14 gate run: `go vet ./...` and `go test ./... -count=1 -race -short` passed in both `summercms.go` and `fonoteka.go`, and every test cited for T-06-28 through T-06-35 was confirmed to exist and pass. --- @@ -45,6 +51,9 @@ The post-gap implementation closes all five threats promoted by the Phase 6 veri | request body → handler | unbounded POST is a resource-exhaustion vector | `http.MaxBytesReader` | | caller-supplied URL → outbound fetch | user/third-party URL must never reach loopback, RFC1918, CGNAT, or metadata | dial-time IP, host allow-list | | IPv6 transition syntax → IPv4 SSRF policy | embedded IPv4 in NAT64 and 6to4 must receive the ordinary reserved/private classification | RFC 6052 `/96` and `/48`, RFC 3056 `2002::/16` | +| request body → named middleware | a body-consuming named middleware must be bounded by the same cap as the terminal handler | `http.MaxBytesReader`, `io.ReadAll` in middleware | +| plugin bucket definition → limiter | a plugin-supplied bucket or inline throttle must not fail open at runtime | `Bucket{Key, Max, Decay}`, `N,M` param | +| non-public IP representations → dial | zoned, special-use and mapped forms must classify as their non-public form at connect time | `netip.Addr` incl. zone, IANA special-use prefixes | | personal-token context → denial serializer | status and exact JSON bytes cross the public compatibility boundary together | `wire.WriteJSON`, raw 401/403 bytes | --- @@ -78,9 +87,17 @@ The post-gap implementation closes all five threats promoted by the Phase 6 veri | T-06-25 | Elevation of Privilege / Information Disclosure | 06-08 | mitigate | `fetchguard/ip_test.go:TestIsReservedOrPrivateIPv6Transitions`; `fetchguard/fetch_test.go:TestDialControlRejectsUnsafeIPv6Transitions`; `fetchguard.embeddedTransitionIPv4` decodes both NAT64 forms and 6to4 before the dial decision. | | T-06-26 | Information Disclosure | 06-09 | mitigate | `surf/router_test.go:TestRecoverDiscardsPartialResponse/house`; `TestRecoverDiscardsPartialResponse/raw`; `TestBufferedResponseCommitsSuccessfulOutput`; shared `bufferedResponse` commits only after a normal return. | | T-06-27 | Tampering | 06-10 | mitigate | `plugins/golem15/fonoteka/middleware/token_scope_test.go:TestInvScope/no-user-401`; `TestInvScope/missing-scope-403`; both denial branches call `wire.WriteJSON` and compare untrimmed bytes. | +| T-06-28 | Denial of Service | 06-12 | mitigate | `surf/bodylimit_test.go:TestBodyLimitBoundsBodyConsumingMiddleware`; `surf/bodylimit_test.go:TestBodyLimitBoundsNamedMiddleware`; `surf/bodylimit_test.go:TestBodyLimitInvalidParamFailsBoot`; source `surf/router.go` `wrap` | +| T-06-29 | Denial of Service | 06-12 | mitigate | `surf/limiter_test.go:TestRegisterBucketRejectsInvalid`; `surf/limiter_test.go:TestValidateThrottleRejectsOverflowAndNilStore`; `surf/limiter_test.go:TestMiddlewareFailsClosed`; source `surf/limiter.go` `RegisterBucket`/`Middleware`/`ValidateThrottle`/`resolve` | +| T-06-30 | Elevation of Privilege | 06-13 | mitigate | `fetchguard/ip_test.go:TestIsReservedOrPrivateIANABoundaries`; `fetchguard/ip_test.go:TestIsReservedOrPrivateSpecialUseSmoke`; source `fetchguard/ip.go` `privateV4`/`privateV6` | +| T-06-31 | Elevation of Privilege | 06-13 | mitigate | `fetchguard/ip_test.go:TestIsReservedOrPrivateIgnoresZone`; `fetchguard/fetch_test.go:TestDialControlRejectsZonedAndSpecialUse`; `fetchguard/fetch_test.go:TestFetchPublicOnlyMapsSpecialUseToPrivateIP`; source `fetchguard/fetch.go` `dialControl` | +| T-06-32 | Spoofing | 06-12 | mitigate | `bouncer/registry_test.go:TestRegisterRejectsTypedNilGuard`; `bouncer/registry_test.go:TestRegisterRejectsTypedNilPointerFuncMapGuards`; `bouncer/registry_test.go:TestRegisterAcceptsValidGuards`; source `bouncer/registry.go` `Register` | +| T-06-33 | Spoofing | 06-12 | mitigate | `bouncer/jwt_test.go:TestVerifyRejectsFractionalSubject`; `bouncer/jwt_test.go:TestVerifySubjectMatrix`; `bouncer/jwt_test.go:TestSubjectJSONNumber`; source `bouncer/jwt.go` `subject` | +| T-06-34 | Denial of Service | 06-12 | mitigate | `surf/bodylimit_test.go:TestCompileRouteConflictReturnsError`; `surf/router_test.go:TestFactoriesBuiltOncePerName`; source `surf/router.go` `handleRoute` | +| T-06-35 | Denial of Service | 06-12 | mitigate | `surf/router_test.go:TestBuildRouterFailsOnMissingBodyConfig`; `surf/bodylimit_test.go:TestBodyLimitMissingConfigFailsBoot`; source `surf/router.go` `requiredBytes` | | 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: 26 closed / 0 open. Dispositions are copied from the originating plans; all accept rationales remain verbatim.* +*Status: 34 closed / 0 open. Dispositions are copied from the originating plans; all accept rationales remain verbatim.* --- @@ -152,6 +169,8 @@ The post-gap implementation closes all five threats promoted by the Phase 6 veri ### T-06-12 / T-06-13 — body limits +- **Correction (06-14):** the original proof only covered the terminal handler, so a named middleware running before the handler could read an unbounded body. See T-06-28 for the corrected proof. + - **Source:** `surf/bodylimit.go`; `fonoteka.go/config/http.yaml`. - **Test evidence:** `TestBodyLimitDefaultRejectsOversizedBody` (MaxBytesReader 413 on non-raw); `TestBodyLimitRawExempt`; `TestProductionBodyLimitsOperatorConfirmed` (both keys 134217728, no INTERIM). Operator-confirmed 2026-09-19 from nginx `client_max_body_size=128M` and php.ini `post_max_size=128M` / `upload_max_filesize=128M`. - **Disposition:** closed / mitigate. @@ -218,6 +237,52 @@ The post-gap implementation closes all five threats promoted by the Phase 6 veri --- +### T-06-28 — body cap bounds body-consuming named middleware + +- **Source:** `surf/router.go` `wrap`: the `bodyLimit` wrapper is applied after all named/factory middleware, so it is outermost inside recovery. +- **Test evidence:** `TestBodyLimitBoundsBodyConsumingMiddleware` (default limit, `body.limit:N` override both raising and bounding, raw route unaffected, panic after read yields clean 500 without leaking the panic text); `TestBodyLimitBoundsNamedMiddleware`; `TestBodyLimitInvalidParamFailsBoot`. +- **Disposition:** closed / mitigate. + +### T-06-29 — invalid limiter definitions fail closed + +- **Source:** `surf/limiter.go`. +- **Test evidence:** `TestRegisterBucketRejectsInvalid` (nil Key, Max 0/-1, Decay 0/negative, nil store; errors name plugin and bucket); `TestValidateThrottleRejectsOverflowAndNilStore`; `TestMiddlewareFailsClosed` (misconfigured limiter answers 500 and never calls next). +- **Disposition:** closed / mitigate. + +### T-06-30 — IANA special-use ranges classified non-public + +- **Source:** `fetchguard/ip.go`. +- **Test evidence:** `TestIsReservedOrPrivateIANABoundaries` asserts first, last and interior address of every listed prefix are non-public, neighbours outside all ranges are public, and IPv4-mapped forms follow the IPv4 table. `isReservedOrPrivate` is at 100% statement coverage. +- **Disposition:** closed / mitigate. + +### T-06-31 — zoned IPv6 cannot evade prefix checks + +- **Source:** `fetchguard/ip.go` (zone stripped), `fetchguard/fetch.go` `dialControl` (zoned targets rejected). +- **Test evidence:** `TestIsReservedOrPrivateIgnoresZone`, `TestDialControlRejectsZonedAndSpecialUse` (`[fe80::1%eth0]`, 198.18.0.1, 192.0.0.1, 240.0.0.1 all `errPrivateIP`; public passes), `TestFetchPublicOnlyMapsSpecialUseToPrivateIP` (Fetch reason `private_ip`, no network I/O). `dialControl` is at 100% statement coverage. +- **Disposition:** closed / mitigate. + +### T-06-32 — typed-nil guards rejected at registration + +- **Test evidence:** `TestRegisterRejectsTypedNilGuard`, `TestRegisterRejectsTypedNilPointerFuncMapGuards`, `TestRegisterAcceptsValidGuards`. +- **Disposition:** closed / mitigate. + +### T-06-33 — fractional or out-of-range JWT subject rejected + +- **Test evidence:** `TestVerifyRejectsFractionalSubject`, `TestVerifySubjectMatrix` (12.5, 1e300, 2^60 float, -1, 0 rejected; 12 and "12" accepted), `TestSubjectJSONNumber`. +- **Disposition:** closed / mitigate. + +### T-06-34 — route conflicts return errors, factories built once + +- **Test evidence:** `TestCompileRouteConflictReturnsError` (no panic); `TestFactoriesBuiltOncePerName`. The latter initially failed: the 06-12 `built` cache was declared but never consulted, so factories ran once per route per pass. Fixed in Plan 06-14 commit 1d2e00c (cache keyed by `name:param`). +- **Disposition:** closed / mitigate. + +### T-06-35 — missing body config no longer becomes zero + +- **Test evidence:** `TestBuildRouterFailsOnMissingBodyConfig` (missing, zero, negative, non-numeric error; valid passes), `TestBodyLimitMissingConfigFailsBoot`. +- **Disposition:** closed / mitigate. + +--- + ## Credential / bearer logging grep `rg -n 'raw|bearer|LastUsedIP' fonoteka.go/plugins/golem15/fonoteka/classes/auth` (excluding tests): `bearerToken` helper, `LastUsedIP` column write in `UpdateColumns`, no adjacent `fmt.Print*` / `log.*` / `slog`. `summercms.go/bouncer` has no Print/log of the token. `bouncer.Credential` is read by InvScope and the named bucket key only. @@ -238,10 +303,12 @@ 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). Plans 06-06 through 06-11 add seven mitigated threats and no new accepts. 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). Plans 06-06 through 06-14 add mitigated threats only and no new accepts. Rationales are copied verbatim in the Threat Register `Proof` column for each accept row. ## Post-Gap Verification Gates +Final gates (Plan 06-14, 2026-09-21): both `go vet ./...` and `go test ./... -count=1 -race -short` passed in `summercms.go` and `fonoteka.go`. + - `summercms.go`: `go test ./... -count=1 -race -short` — pass; `go vet ./...` — pass. - `fonoteka.go`: `go test ./... -count=1 -race -short` — pass; `go vet ./...` — pass. - Source assertion: anonymous inline keys contain `inline:domainless|` and no throttle-parameter or request/forwarded-Host contribution — pass. @@ -255,4 +322,5 @@ Four accepts (06-05's "three" list omitted T-06-05, which 06-01 already accepted |------------|---------------|--------|------|--------| | 2026-09-19 | 19 | 19 | 0 | gsd-executor (06-05) | | 2026-09-20 | 21 | 21 | 0 | gsd-executor (06-06) | -| 2026-09-21 | 26 | 26 | 0 | gsd-executor (06-11 post-gap refresh) | +| 2026-09-21 | 26 | 26 | 0 | gsd-executor (06-11 post-gap refresh) -- SUPERSEDED, contradicted by verification | +| 2026-09-21 | 34 | 34 | 0 | gsd-executor (06-14 reopen and re-close; vet + race tests green in both repos) |