From ad3d6f7f84a7d1ee8114f45452044538b6431ca0 Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Sat, 19 Sep 2026 17:13:02 +0200 Subject: [PATCH] docs(06): revision pass 1 - resolve Limiter naming collision and add plugin-facing house-middleware capability --- .../06-01-PLAN.md | 4 +- .../06-02-PLAN.md | 122 +++++++++++++----- .../06-03-PLAN.md | 85 +++++++++--- .../06-05-PLAN.md | 4 +- .../06-VALIDATION.md | 4 +- 5 files changed, 163 insertions(+), 56 deletions(-) diff --git a/.planning/phases/06-http-routing-auth-groups-and-rate-limiting/06-01-PLAN.md b/.planning/phases/06-http-routing-auth-groups-and-rate-limiting/06-01-PLAN.md index cd5c4ee..3396096 100644 --- a/.planning/phases/06-http-routing-auth-groups-and-rate-limiting/06-01-PLAN.md +++ b/.planning/phases/06-http-routing-auth-groups-and-rate-limiting/06-01-PLAN.md @@ -172,7 +172,7 @@ func (r *Router) RegisterMiddlewareFactory(pluginID, name string, fn func(param Extended summercms.go/pact/capabilities.go (new capability, collected in surf.Assemble in the same loop as HasMiddleware): type HasMiddlewareFactories interface { - MiddlewareFactories() map[string]func(param string) pact.Middleware + MiddlewareFactories() map[string]func(param string) Middleware } @@ -190,7 +190,7 @@ type HasMiddlewareFactories interface { .planning/phases/06-http-routing-auth-groups-and-rate-limiting/06-RESEARCH.md (Pitfall 9: router is GET-only, exact lines to generalize; Pattern 1) - In pact/capabilities.go: add Post, Put, Patch, Delete to the Router interface with the exact same signature shape as Get (path string, handler http.HandlerFunc, middleware ...string). Add HasMiddlewareFactories interface { MiddlewareFactories() map[string]func(param string) pact.Middleware } alongside the existing HasMiddleware. + In pact/capabilities.go: add Post, Put, Patch, Delete to the Router interface with the exact same signature shape as Get (path string, handler http.HandlerFunc, middleware ...string). Add HasMiddlewareFactories interface { MiddlewareFactories() map[string]func(param string) Middleware } alongside the existing HasMiddleware (bare Middleware, not pact.Middleware -- this interface is declared inside package pact itself, where Middleware is already the unqualified local type). In surf/router.go: thread a method string parameter through add(pluginID, prefix string, groupMW []string, method, path string, handler http.HandlerFunc, extra []string); change the duplicate-detection key from the hardcoded "GET " + full to method + " " + full; change route{method: "GET", ...} to use the passed method. Add Post/Put/Patch/Delete on both *Router and *Group, each calling add with its own HTTP method string, mirroring Get's and Group.Get's existing nil-guard shape exactly. In compile(), change mux.Handle("GET "+rt.path, h) to mux.Handle(rt.method+" "+rt.path, h). diff --git a/.planning/phases/06-http-routing-auth-groups-and-rate-limiting/06-02-PLAN.md b/.planning/phases/06-http-routing-auth-groups-and-rate-limiting/06-02-PLAN.md index ee37b68..f25fdd8 100644 --- a/.planning/phases/06-http-routing-auth-groups-and-rate-limiting/06-02-PLAN.md +++ b/.planning/phases/06-http-routing-auth-groups-and-rate-limiting/06-02-PLAN.md @@ -35,11 +35,12 @@ must_haves: - "php_parity.sh exports APP_DEBUG=false so recorded error fixtures reflect the production (non-debug) body shape (user-resolved Open Question 2)" - "ROADMAP.md and REQUIREMENTS.md HTTP-04 say five named buckets, not seven (D-01 miscount correction)" - "Rate-limit counters live in-process behind a small Store interface (mutex-guarded map with expiry sweep), stdlib only -- no otter/cooler package this phase (D-03)" + - "The new concrete rate limiter is named FixedWindowLimiter, distinct from the pre-existing surf.Limiter interface seam declared in router.go; no type is redeclared (D-05, resolves the Phase 3 Limiter interface naming collision)" artifacts: - path: summercms.go/surf/limiter_store.go provides: "Store interface + MemoryStore: Hit, TooManyAttempts, AvailableIn matching Illuminate\\Cache\\RateLimiter semantics" - path: summercms.go/surf/limiter.go - provides: "Bucket type, Limiter, RegisterBucket, the throttle: middleware factory wired into surf.Router" + provides: "Bucket type, FixedWindowLimiter (the concrete rate limiter -- distinct from the pre-existing surf.Limiter interface), RegisterBucket, the throttle: middleware factory wired into surf.Router" - path: summercms.go/surf/clientip.go provides: "ClientIP(r, trusted) and TrustedProxies(cfg) -- the single source of client IP for limiter keys" - path: fonoteka.go/plugins/golem15/fonoteka/middleware/public_share_headers.go @@ -51,15 +52,15 @@ must_haves: pattern: "Buckets\\(\\) map\\[string\\]surf\\.Bucket" - from: summercms.go/surf/router.go to: summercms.go/surf/limiter.go - via: "Router registers a built-in \"throttle\" middleware factory bound to its Limiter" + via: "Router registers a built-in \"throttle\" middleware factory bound to its FixedWindowLimiter" pattern: "RegisterMiddlewareFactory\\(.*\"throttle\"" --- -Replace the Phase 3 no-op limiter seam with a real fixed-window rate limiter that ports Płytarium's rate-limit contract 1:1: the five named `fonoteka-*` buckets, inline `throttle:N,M` limits, stacked limiters on one route, and Laravel's exact wire behavior (headers on success and on 429, first-hit-wins fixed window, no token-bucket smoothing). Wire the trusted-proxy-aware client IP function that both the limiter and D-08's token surface can share, and port `PublicShareHeaders`' 429 rewrite for the anonymous public-share group. +Replace the Phase 3 no-op limiter with a real fixed-window rate limiter that ports Płytarium's rate-limit contract 1:1: the five named `fonoteka-*` buckets, inline `throttle:N,M` limits, stacked limiters on one route, and Laravel's exact wire behavior (headers on success and on 429, first-hit-wins fixed window, no token-bucket smoothing). Wire the trusted-proxy-aware client IP function that both the limiter and D-08's token surface can share, and port `PublicShareHeaders`' 429 rewrite for the anonymous public-share group. Purpose: HTTP-04 requires byte-for-byte parity with Laravel's `ThrottleRequests`/`RateLimiter`, which has documented, non-obvious semantics (fixed window, not token bucket; headers on success too; tooManyAttempts checked BEFORE hit) -- getting this wrong changes observable client-visible behavior, not just an implementation detail. -Output: `surf.Limiter`/`surf.Store`/`surf.Bucket`/`surf.ClientIP`; the `"throttle"` middleware factory; the five fonoteka buckets registered and attached to the token group; `PublicShareHeaders` ported; the remaining Phase-6-scope route groups (`jwt_locale`, `onboarding`, `public_invitation`, `public_share`/`public_wishlist`) declared as group builders per D-15. +Output: `surf.FixedWindowLimiter`/`surf.Store`/`surf.Bucket`/`surf.ClientIP`; the `"throttle"` middleware factory; the five fonoteka buckets registered and attached to the token group; `PublicShareHeaders` ported; the remaining Phase-6-scope route groups (`jwt_locale`, `onboarding`, `public_invitation`, `public_share`/`public_wishlist`) declared as group builders per D-15. @@ -76,6 +77,46 @@ Output: `surf.Limiter`/`surf.Store`/`surf.Bucket`/`surf.ClientIP`; the `"throttl +IMPORTANT -- naming collision this plan must avoid: summercms.go/surf/router.go +(as of Phase 3, unchanged by 06-01) already declares: + +package surf + +// Limiter wraps handlers. Phase 6 replaces the no-op with named buckets. +type Limiter interface { + Wrap(http.Handler) http.Handler +} + +type noopLimiter struct{} + +func (noopLimiter) Wrap(next http.Handler) http.Handler { return next } + +func noOpLimit(next http.Handler) http.Handler { + return noopLimiter{}.Wrap(next) +} + +This plan's concrete rate limiter is therefore named FixedWindowLimiter, NOT +Limiter -- declaring `type Limiter struct{...}` in the same package would be +an illegal redeclaration. Resolution (CONTEXT D-03: the real limiter lives +"behind the existing surf.Limiter seam"): +- The existing `Limiter` interface, its doc comment, and its single-argument + `Wrap(http.Handler) http.Handler` shape are left EXACTLY as they are -- + they are not deleted, not renamed, not reused as a base type. They remain + the Phase 3 seam per D-03, available to a future call site that wants a + single default limiter with no per-route bucket name. +- `FixedWindowLimiter` (below) does NOT implement `Limiter`'s `Wrap` method. + Its shape is fundamentally different (parameterized by a bucket-name-or- + inline-throttle `param string`, not a bare wrap), because D-05 requires + rate limiting to be expressed exclusively through named/parameterized + "throttle:..." middleware entries at call sites, not a blanket per-route + wrapper -- there is no meaningful "default limiter with no name" in this + design for `Wrap` to represent. +- `noopLimiter` and `noOpLimit` are REMOVED in this plan, along with their + one call site (`h = noOpLimit(h)` in `wrap()`). They existed only to + satisfy that one blanket call site; once it is removed (rate limiting is + now exclusively via named "throttle:..." entries), they have no remaining + purpose and are deleted rather than left as dead code. + New file summercms.go/surf/limiter_store.go: package surf @@ -114,15 +155,27 @@ type Bucket struct { // BucketProvider is implemented by plugins that declare named buckets (not a // pact interface: it lives in surf and is type-asserted directly in -// Assemble, since pact cannot import surf without a cycle). +// Assemble/BuildRouter, since pact cannot import surf without a cycle). type BucketProvider interface { Buckets() map[string]Bucket } -type Limiter struct{ /* unexported: store Store; buckets map[string]Bucket */ } +// FixedWindowLimiter is the concrete rate limiter (distinct from the +// pre-existing surf.Limiter interface -- see the naming-collision note +// above). It owns the Store, the named-bucket table, and the inline-throttle +// parser, and produces the "throttle" middleware factory's per-route +// pact.Middleware. +type FixedWindowLimiter struct{ /* unexported: store Store; trusted []netip.Prefix; buckets map[string]Bucket */ } -func NewLimiter(store Store) *Limiter -func (l *Limiter) RegisterBucket(pluginID, name string, b Bucket) error +// NewFixedWindowLimiter is the ONE constructor signature for this type -- +// trusted is required at construction (not a later setter) because both +// named-bucket Key closures (registered later via RegisterBucket) and the +// inline "N,M" throttle's own key resolver need the same trusted-proxy list, +// and threading it through every call site individually would risk two +// different trusted lists disagreeing within one Router. +func NewFixedWindowLimiter(store Store, trusted []netip.Prefix) *FixedWindowLimiter + +func (l *FixedWindowLimiter) RegisterBucket(pluginID, name string, b Bucket) error // Middleware builds the throttle: factory body. param is either a // registered bucket name (looked up in l.buckets) or a literal "N,M" pair @@ -130,16 +183,16 @@ func (l *Limiter) RegisterBucket(pluginID, name string, b Bucket) error // Sequence per request (Laravel ThrottleRequests::handleRequest order): // 1. resolve the Bucket (named lookup or inline N,M with an inline key // resolver: principal id if bouncer.User(ctx) is set, else -// r.Host+"|"+ClientIP) +// r.Host+"|"+ClientIP(r, l.trusted)) // 2. if store.TooManyAttempts(key, max): set Retry-After/X-RateLimit-Reset/ // X-RateLimit-Limit/X-RateLimit-Remaining=0, write the 429 body, stop // 3. else store.Hit(key, decay); set X-RateLimit-Limit/-Remaining; call next -func (l *Limiter) Middleware(param string) pact.Middleware +func (l *FixedWindowLimiter) Middleware(param string) pact.Middleware -// ValidateThrottle is called once per route at Assemble/wrap time (not per -// request) so a malformed inline "N,M" or an unregistered bucket name fails -// boot instead of the first live request. -func (l *Limiter) ValidateThrottle(param string) error +// ValidateThrottle is called once per route at Assemble/BuildRouter time +// (not per request) so a malformed inline "N,M" or an unregistered bucket +// name fails boot instead of the first live request. +func (l *FixedWindowLimiter) ValidateThrottle(param string) error New file summercms.go/surf/clientip.go: @@ -158,24 +211,31 @@ func ClientIP(r *http.Request, trusted []netip.Prefix) string // fatal (logged by the caller if desired). func TrustedProxies(cfg *compass.Config) []netip.Prefix -Router change (summercms.go/surf/router.go): Router gains a limiter *Limiter -field (nil-safe: a nil limiter behaves like today's noopLimiter). Assemble -constructs one surf.NewLimiter(surf.NewMemoryStore(2*time.Minute)) and -attaches it to the Router before the BucketProvider loop, then registers the -built-in factory: r.RegisterMiddlewareFactory("surf", "throttle", +Router change (summercms.go/surf/router.go): Router gains a limiter +*FixedWindowLimiter field (nil-safe: BuildRouter always sets it, so this is +only nil for a Router built directly via New() outside BuildRouter, e.g. in +an existing router_test.go unit test that never registers "throttle:..." -- +such a route simply never reaches the throttle factory). BuildRouter (the +06-03-introduced split of Assemble, or Assemble itself if 06-03 has not run +yet in this working tree -- Task 1's action names the exact call site either +way) constructs one surf.NewFixedWindowLimiter(surf.NewMemoryStore(2*time.Minute), +surf.TrustedProxies(app.Config)) and attaches it to the Router before the +BucketProvider loop, then registers the built-in factory: +r.RegisterMiddlewareFactory("surf", "throttle", func(param string) pact.Middleware { return r.limiter.Middleware(param) }). -The unconditional h = noOpLimit(h) line in wrap() is removed -- rate limiting -is now expressed exclusively via named "throttle:..." middleware entries at -call sites, matching D-05. +The unconditional h = noOpLimit(h) line in wrap() is removed, and +noopLimiter/noOpLimit are deleted (see the naming-collision note above) -- +rate limiting is now expressed exclusively via named "throttle:..." +middleware entries at call sites, matching D-05. - Task 1 (summercms.go): Fixed-window Store, Limiter, trusted-proxy ClientIP, and router wiring + Task 1 (summercms.go): Fixed-window Store, FixedWindowLimiter, trusted-proxy ClientIP, and router wiring summercms.go/surf/limiter.go, summercms.go/surf/limiter_store.go, summercms.go/surf/clientip.go, summercms.go/surf/router.go, summercms.go/surf/limiter_test.go, summercms.go/surf/clientip_test.go, summercms.go/pact/capabilities.go - summercms.go/surf/router.go (full, post-06-01 -- wrap()'s noOpLimit call site, RegisterMiddlewareFactory, Assemble) + summercms.go/surf/router.go (full, post-06-01 -- wrap()'s noOpLimit call site, the existing Limiter interface/noopLimiter/noOpLimit at the bottom of the file, RegisterMiddlewareFactory, Assemble) summercms.go/compass/config.go (full -- Lookup/String/Int/Bool/LoadSection for reading http.trusted_proxies) .planning/phases/06-http-routing-auth-groups-and-rate-limiting/06-RESEARCH.md (Pitfalls 1, 3, 4, 6 and the "Fixed-window limiter Store" code example, lines 399-414) /media/nvme/dev/golem15/fonoteka/vendor/laravel/framework/src/Illuminate/Cache/RateLimiter.php (hit/tooManyAttempts/availableIn -- read directly, do not paraphrase from memory) @@ -184,24 +244,26 @@ call sites, matching D-05. Create limiter_store.go: counterEntry{count int; resetAt time.Time}; MemoryStore{mu sync.Mutex; entries map[string]*counterEntry; sweep time.Duration}; NewMemoryStore(sweep time.Duration) *MemoryStore starting a background goroutine (time.Ticker on sweep, or time.AfterFunc re-armed each tick) that deletes entries where time.Now().After(resetAt). Hit(key, decay): lock; if entry absent or now is after entry.resetAt, replace with a fresh {count:0, resetAt: now.Add(decay)} (first-hit-wins: an existing unexpired window is never extended); increment count; return count. TooManyAttempts(key, max): lock; if entry absent, return false; if now is after entry.resetAt, delete the entry (mirrors PHP's resetAttempts() side effect) and return false; return entry.count >= max. AvailableIn(key): lock; if entry absent, return 0; return max(0, entry.resetAt.Sub(now)). - Create limiter.go per the exact contract in the interfaces block: Bucket, BucketProvider, Limiter, NewLimiter, RegisterBucket (dup-fail shape identical to RegisterMiddleware's, error "surf: bucket %q already registered by %s"), Middleware(param), ValidateThrottle(param). Resolve param in this order: (1) if param matches a registered bucket name, use it; (2) else parse param as "N,M" via strings.Cut(param, ",") + strconv.Atoi on both parts (N=max attempts, M=decay in minutes, matching Laravel's throttle:N,M semantics where M is minutes) and build an ad-hoc Bucket with Max=N, Decay=time.Duration(M)*time.Minute, and Key resolving per Pitfall 4: if bouncer.User(r.Context()) is set, key is "u:"+principal ID; else key is r.Host+"|"+ClientIP(r, trusted) (the inline-throttle Limiter needs a trusted-proxy list too -- thread it through NewLimiter's constructor or a setter, e.g. NewLimiter(store Store, trusted []netip.Prefix)); (3) else ValidateThrottle/Middleware returns an error (surfaced at Assemble time via the factory's first invocation during Router.wrap()'s per-route validation loop, not deferred to the first live request -- Assemble already calls r.wrap(rt) once per route after Routes() collection specifically to catch unknown-middleware errors at boot; the same loop now also catches unknown bucket/malformed inline-throttle errors). Request-time sequence inside Middleware's returned handler: call store.TooManyAttempts(key, max) FIRST; if true, compute retryAfter := store.AvailableIn(key), set headers Retry-After (seconds, integer), X-RateLimit-Reset (unix timestamp of now+retryAfter), X-RateLimit-Limit (max), X-RateLimit-Remaining ("0"), write status 429 with body {"message":"Too Many Attempts."} (the house-default shape per Pitfall 2 -- PublicShareHeaders rewrites this for its own group in Task 2), and return without calling next; else call attempts := store.Hit(key, decay), set X-RateLimit-Limit and X-RateLimit-Remaining (max(0, max-attempts)) on the response, then call next.ServeHTTP. + Create limiter.go per the exact contract in the interfaces block above -- read the naming-collision note first: Bucket, BucketProvider, FixedWindowLimiter (NOT Limiter -- that name is already the pre-existing interface in router.go), NewFixedWindowLimiter(store Store, trusted []netip.Prefix) *FixedWindowLimiter (the ONE constructor signature -- trusted is a required constructor argument, never a setter, never a second optional overload), RegisterBucket (dup-fail shape identical to RegisterMiddleware's, error "surf: bucket %q already registered by %s"), Middleware(param), ValidateThrottle(param). Resolve param in this order: (1) if param matches a registered bucket name, use it; (2) else parse param as "N,M" via strings.Cut(param, ",") + strconv.Atoi on both parts (N=max attempts, M=decay in minutes, matching Laravel's throttle:N,M semantics where M is minutes) and build an ad-hoc Bucket with Max=N, Decay=time.Duration(M)*time.Minute, and Key resolving per Pitfall 4: if bouncer.User(r.Context()) is set, key is "u:"+principal ID; else key is r.Host+"|"+ClientIP(r, l.trusted) (l.trusted is the slice captured at NewFixedWindowLimiter construction time -- do not re-read config per request); (3) else ValidateThrottle/Middleware returns an error (surfaced at Assemble/BuildRouter time via the factory's first invocation during Router.wrap()'s per-route validation loop, not deferred to the first live request -- Assemble already calls r.wrap(rt) once per route after Routes() collection specifically to catch unknown-middleware errors at boot; the same loop now also catches unknown bucket/malformed inline-throttle errors). Request-time sequence inside Middleware's returned handler: call store.TooManyAttempts(key, max) FIRST; if true, compute retryAfter := store.AvailableIn(key), set headers Retry-After (seconds, integer), X-RateLimit-Reset (unix timestamp of now+retryAfter), X-RateLimit-Limit (max), X-RateLimit-Remaining ("0"), write status 429 with body {"message":"Too Many Attempts."} (the house-default shape per Pitfall 2 -- PublicShareHeaders rewrites this for its own group in Task 2), and return without calling next; else call attempts := store.Hit(key, decay), set X-RateLimit-Limit and X-RateLimit-Remaining (max(0, max-attempts)) on the response, then call next.ServeHTTP. Create clientip.go: ClientIP(r, trusted) -- parse r.RemoteAddr via net.SplitHostPort to get the bare IP; if trusted is empty or the parsed IP does not match any prefix in trusted (netip.Prefix.Contains), return that IP; otherwise read X-Forwarded-For, split on comma, trim each hop, walk the list RIGHT TO LEFT, return the first hop whose parsed address is NOT contained in any trusted prefix; if every hop is trusted (or the header is empty/absent), fall back to the original RemoteAddr IP. TrustedProxies(cfg *compass.Config): read http.trusted_proxies as a []string (mirror corsOrigins's []string/[]any type-switch pattern already in router.go), netip.ParsePrefix each entry, skip invalid entries, return the slice (nil/empty when the key is absent). - In router.go: add a limiter *Limiter field to Router; in Assemble(), after constructing r := New(corsOrigins(app)), build trusted := TrustedProxies(app.Config), lim := NewLimiter(NewMemoryStore(2*time.Minute), trusted) (document the 2-minute sweep default inline as "longest bucket decay is 1 minute; sweep at 2x"), set r.limiter = lim, and call r.RegisterMiddlewareFactory("surf", "throttle", func(param string) pact.Middleware { return lim.Middleware(param) }) before the existing HasMiddleware/HasMiddlewareFactories/BucketProvider loops (BucketProvider loop is new: for each plugin implementing surf.BucketProvider, call lim.RegisterBucket(p.ID(), name, b) for every entry). Remove the unconditional h = noOpLimit(h) line from wrap() (rate limiting is now exclusively expressed via "throttle:..." middleware entries); delete noOpLimit/noopLimiter only if nothing else references them, otherwise leave them unused-but-harmless is NOT acceptable in Go (unused private funcs are fine, but confirm via go vet/staticcheck that removing the call site doesn't orphan an exported symbol other packages depend on -- grep first). + In router.go: add a limiter *FixedWindowLimiter field to Router. In Assemble() (before 06-03 introduces the BuildRouter split -- if 06-03 has already executed in this working tree when this task runs, make the equivalent change in BuildRouter instead, since Assemble will then just call BuildRouter+compile), after constructing r := New(corsOrigins(app)), build trusted := TrustedProxies(app.Config), lim := NewFixedWindowLimiter(NewMemoryStore(2*time.Minute), trusted) (document the 2-minute sweep default inline as "longest bucket decay is 1 minute; sweep at 2x"), set r.limiter = lim, and call r.RegisterMiddlewareFactory("surf", "throttle", func(param string) pact.Middleware { return lim.Middleware(param) }) before the existing HasMiddleware/HasMiddlewareFactories/BucketProvider loops (BucketProvider loop is new: for each plugin implementing surf.BucketProvider, call lim.RegisterBucket(p.ID(), name, b) for every entry). Remove the unconditional h = noOpLimit(h) line from wrap(). Delete the now-dead noopLimiter type and noOpLimit function entirely, and delete nothing else from the existing Limiter interface declaration or its doc comment -- it remains, untouched, as the Phase 3 seam (per the interfaces block's naming-collision note; confirm via grep that noOpLimit/noopLimiter have no other call sites in the repo before deleting -- there are none as of Phase 3/06-01). - cd /media/nvme/dev/golem15/summercms.io/summercms/summercms.go && go vet ./... && go test ./surf/... -run TestLimiter -short && go test ./surf/... -run TestClientIP -short + cd /media/nvme/dev/golem15/summercms.io/summercms/summercms.go && go vet ./... && go test ./surf/... -run TestFixedWindowLimiter -short && go test ./surf/... -run TestClientIP -short + - go build ./... succeeds with both the pre-existing surf.Limiter interface (unchanged, still declared) and the new surf.FixedWindowLimiter type coexisting in package surf -- no redeclaration error. - A fuzz-free unit test drives MemoryStore directly and asserts: three Hit calls within one decay window return counts 1,2,3; TooManyAttempts is false at count==max-1 and true at count==max; after the window elapses, TooManyAttempts resets to false and a subsequent Hit reopens a fresh window (first-hit-wins, not extended). - A test asserts X-RateLimit-Limit/X-RateLimit-Remaining are present on a SUCCESSFUL (200) response that passed through a throttle: middleware, not just on the 429. - A test asserts the 429 response carries Retry-After, X-RateLimit-Reset, X-RateLimit-Limit, and X-RateLimit-Remaining=0, with body {"message":"Too Many Attempts."}. - A test registers two buckets and stacks "throttle:bucket-a","throttle:bucket-b" on one route; exhausting bucket-a's budget alone (while bucket-b still has room) returns 429; a separate test exhausting only bucket-b (bucket-a fresh) also returns 429 -- both budgets are enforced independently. - ClientIP tests: RemoteAddr used when trusted is empty; RemoteAddr used when RemoteAddr is untrusted even if X-Forwarded-For is present (spoofing test); rightmost untrusted hop used when RemoteAddr is trusted and X-Forwarded-For has a mixed trusted/untrusted chain. - An inline "throttle:10,1" test asserts the key differs for two different bouncer.User(ctx) principals sharing one IP, but is IDENTICAL for two anonymous requests from the same IP+Host (Pitfall 4). + - grep -rn "noopLimiter\|noOpLimit" summercms.go/surf/ returns zero matches after this task (confirms full removal, not a half-deleted call site). - surf.Limiter reproduces Laravel's fixed-window ThrottleRequests wire contract exactly, including header placement and stacking; ClientIP is the single trusted-proxy-aware resolver. + surf.FixedWindowLimiter reproduces Laravel's fixed-window ThrottleRequests wire contract exactly, including header placement and stacking, sitting alongside the untouched pre-existing surf.Limiter interface seam; ClientIP is the single trusted-proxy-aware resolver; noopLimiter/noOpLimit are fully removed. @@ -216,7 +278,7 @@ call sites, matching D-05. fonoteka.go/parity/manifest.yaml lines 1-9 (the seven auth_groups: jwt_locale, jwt, onboarding, public_invitation, public_share, personal_token, oauth) - In plugin.go, add Buckets() map[string]surf.Bucket implementing surf.BucketProvider, building each Key closure with a trusted := surf.TrustedProxies(p.app.Config) captured once: "fonoteka-api-token" {Max:60, Decay:time.Minute, Key: func(r){ if tok, ok := bouncer.Credential(r.Context()).(*models.ApiToken); ok { return "tok:"+strconv.FormatUint(uint64(tok.ID),10) }; return surf.ClientIP(r, trusted) }} (note: this bucket sits at GROUP level in PHP, meaning the inv_token guard middleware must run BEFORE this bucket's Key resolver sees a credential -- confirm the group's middleware order in routes.go is surf.Use("inv_token", "inv.scope:", "throttle:fonoteka-api-token") so bouncer.Credential is already populated); "fonoteka-oauth-token" {Max:30, Decay:time.Minute, Key: func(r){ return "oauthtok:"+surf.ClientIP(r, trusted) }}; "fonoteka-oauth-register" {Max:30, Decay:time.Minute, Key: func(r){ return "oauthreg:"+surf.ClientIP(r, trusted) }}; "fonoteka-public-token" {Max:60, Decay:time.Minute, Key: func(r){ return "pubtok:"+r.PathValue("token") }}; "fonoteka-public-ip" {Max:120, Decay:time.Minute, Key: func(r){ return surf.ClientIP(r, trusted) }}. Register these by returning them from Buckets() (Assemble's new BucketProvider loop calls lim.RegisterBucket(p.ID(), name, b) for each). + In plugin.go, add Buckets() map[string]surf.Bucket implementing surf.BucketProvider, building each Key closure with a trusted := surf.TrustedProxies(p.app.Config) captured once: "fonoteka-api-token" {Max:60, Decay:time.Minute, Key: func(r){ if tok, ok := bouncer.Credential(r.Context()).(*models.ApiToken); ok { return "tok:"+strconv.FormatUint(uint64(tok.ID),10) }; return surf.ClientIP(r, trusted) }} (note: this bucket sits at GROUP level in PHP, meaning the inv_token guard middleware must run BEFORE this bucket's Key resolver sees a credential -- confirm the group's middleware order in routes.go is surf.Use("inv_token", "inv.scope:", "throttle:fonoteka-api-token") so bouncer.Credential is already populated); "fonoteka-oauth-token" {Max:30, Decay:time.Minute, Key: func(r){ return "oauthtok:"+surf.ClientIP(r, trusted) }}; "fonoteka-oauth-register" {Max:30, Decay:time.Minute, Key: func(r){ return "oauthreg:"+surf.ClientIP(r, trusted) }}; "fonoteka-public-token" {Max:60, Decay:time.Minute, Key: func(r){ return "pubtok:"+r.PathValue("token") }}; "fonoteka-public-ip" {Max:120, Decay:time.Minute, Key: func(r){ return surf.ClientIP(r, trusted) }}. Register these by returning them from Buckets() (Assemble's/BuildRouter's new BucketProvider loop calls lim.RegisterBucket(p.ID(), name, b) for each). In routes.go: on the existing personal-token r.Group("/api/v1/fonoteka", ...) call from 06-01, change surf.Use("inv_token", "inv.scope:read") to surf.Use("inv_token", "inv.scope:read", "throttle:fonoteka-api-token") and remove the "// TODO(06-02)" comment left by 06-01 -- this is the landing spot it named. Add four more r.Group calls, each with ZERO g.Get/g.Post calls inside (empty body -- no ported handler exists for any route in these groups yet; the group declaration itself is what D-15 requires, and its middleware STRING LIST is what a boot-time smoke test (Task 3) proves resolves without error): - r.Group("/_fonoteka/api/v1", surf.Use("jwt.auth"), func(g pact.Router) {}) for jwt_locale (PHP's me/locale group carries jwt.auth+bindings only, no inv.must-change-password -- do not add inv.must-change-password here even though the main jwt group has it). @@ -296,7 +358,7 @@ cd ../fonoteka.go && go vet ./... && go test ./plugins/golem15/fonoteka/... -sho -- surf.Limiter reproduces Laravel's fixed-window ThrottleRequests contract, including success-response headers and stacking. +- surf.FixedWindowLimiter reproduces Laravel's fixed-window ThrottleRequests contract, including success-response headers and stacking, alongside the untouched pre-existing surf.Limiter interface. - The five fonoteka buckets are registered with exact names/limits/keys; the token group is throttled by fonoteka-api-token. - PublicShareHeaders ports its exact 429 rewrite and unconditional headers. - All six Phase-6-scope route groups (everything but oauth) boot cleanly with their real middleware stacks. diff --git a/.planning/phases/06-http-routing-auth-groups-and-rate-limiting/06-03-PLAN.md b/.planning/phases/06-http-routing-auth-groups-and-rate-limiting/06-03-PLAN.md index c3ea3b0..fdef0c0 100644 --- a/.planning/phases/06-http-routing-auth-groups-and-rate-limiting/06-03-PLAN.md +++ b/.planning/phases/06-http-routing-auth-groups-and-rate-limiting/06-03-PLAN.md @@ -38,6 +38,7 @@ must_haves: truths: - "A group flagged Raw refuses any middleware tagged house-envelope/error at registration time, failing boot rather than silently mounting it (D-16)" - "A panic inside a raw-group route returns a bare 500 with no JSON body, while a panic in any other route still returns the house {\"error\":true,\"message\":\"Internal server error\"} body (D-16)" + - "Plugins declare house-tagged middleware through a capability interface (pact.HasHouseMiddleware), never by calling a Register* function directly -- Assemble/BuildRouter's plugin loop is the only caller of RegisterHouseMiddleware, mirroring how HasMiddleware/Middlewares() already works (D-16)" - "The .well-known/oauth-authorization-server and oauth/mcp/* group is declared raw with fonoteka-oauth-token and fonoteka-oauth-register attached to its two future routes, and carries no handlers yet (D-16)" - "A route table (method, pattern, plugin, middleware chain, raw flag) is exposed and used both by a test and by a route:list CLI command (D-16)" - "No route under /api/v1/fonoteka carries jwt.auth in its middleware chain, and no route under /_fonoteka/api/v1 carries inv_token or an inv.scope: entry, verified by inspecting the route table (D-16, completes T-06-02 from 06-01)" @@ -48,6 +49,8 @@ must_haves: artifacts: - path: summercms.go/surf/routetable.go provides: "RouteInfo{Method,Pattern,PluginID,Middleware,Raw} and Router.Routes()" + - path: summercms.go/pact/capabilities.go + provides: "GroupRaw on the Router interface; HasHouseMiddleware{HouseMiddlewares() map[string]Middleware} plugin capability" - path: summercms.go/wire/response.go provides: "WriteJSON, WriteOpaque500, Time, TriBool, Slice[T]" - path: summercms.go/surf/cors.go @@ -59,6 +62,10 @@ must_haves: to: summercms.go/surf/router.go via: "the oauth group is declared via GroupRaw, not Group" pattern: "GroupRaw\\(" + - from: fonoteka.go/plugins/golem15/fonoteka/plugin.go + to: summercms.go/pact/capabilities.go + via: "Plugin implements HasHouseMiddleware, moving inv.must-change-password out of Middlewares()" + pattern: "HouseMiddlewares\\(\\)" - from: summercms.go/surf/routelist_command.go to: summercms.go/surf/routetable.go via: "the route:list command calls Router.Routes() and renders it as a table" @@ -70,10 +77,10 @@ must_haves: --- -Ship the structural guarantees HTTP-06/HTTP-08/HTTP-09 require and that no later phase can retrofit without a breaking change: a `Raw` group flag enforced at registration (not by convention), a route table + `route:list` CLI command that makes group mutual-exclusivity and the OAuth exemption independently verifiable, promoted response-convention helpers (`wire` package), path-scoped config-driven CORS matching `config/cors.php` exactly, per-group JSON body limits, and the swag/`openapi-typescript` pipeline on the one real handler this phase has (`ListGenres`). +Ship the structural guarantees HTTP-06/HTTP-08/HTTP-09 require and that no later phase can retrofit without a breaking change: a `Raw` group flag enforced at registration (not by convention) with a plugin-facing capability for declaring which middleware is refused there, a route table + `route:list` CLI command that makes group mutual-exclusivity and the OAuth exemption independently verifiable, promoted response-convention helpers (`wire` package), path-scoped config-driven CORS matching `config/cors.php` exactly, per-group JSON body limits, and the swag/`openapi-typescript` pipeline on the one real handler this phase has (`ListGenres`). Purpose: this plan is the "contract surface" of the phase -- everything a future handler or a future auth guard (OAuth, Phase 8) must be structurally prevented from getting wrong, proven by inspection rather than trusted by convention. -Output: `Router.GroupRaw`, `Router.Routes()`, `surf.RouteListCommand`, `wire.WriteJSON`/`Time`/`TriBool`/`Slice`, path-scoped `surf.cors`, per-group body limits, the oauth group declared raw, a committed OpenAPI document, and a resolved (not guessed) production body-size limit. +Output: `Router.GroupRaw`, `pact.HasHouseMiddleware`, `Router.Routes()`, `surf.RouteListCommand`, `wire.WriteJSON`/`Time`/`TriBool`/`Slice`, path-scoped `surf.cors`, per-group body limits, the oauth group declared raw, a committed OpenAPI document, and a resolved (not guessed) production body-size limit. @@ -105,6 +112,25 @@ type Router interface { WhereIn(param string, values ...string) } +New capability in summercms.go/pact/capabilities.go, alongside the existing +HasMiddleware (bare Middleware, not pact.Middleware -- this is declared +inside package pact itself, same reasoning as 06-01's HasMiddlewareFactories): + +// HasHouseMiddleware is implemented by plugins that register middleware +// tagged as house-envelope/error handling -- refused inside a raw group +// (D-16). This is the ONLY way a plugin declares a house-tagged name: +// plugins never call a Router.Register* method directly (there is no such +// call site anywhere in this codebase -- RegisterMiddleware/ +// RegisterMiddlewareFactory/RegisterHouseMiddleware are all called +// exclusively from surf.Assemble/BuildRouter's plugin loop, the same way +// HasMiddleware's Middlewares() map is today). A name present in both +// Middlewares() and HouseMiddlewares() (from the same or a different +// plugin) fails boot with the existing duplicate-name error, since both are +// registered into the same underlying name table. +type HasHouseMiddleware interface { + HouseMiddlewares() map[string]Middleware +} + New file summercms.go/surf/routetable.go: package surf @@ -137,9 +163,23 @@ Router internals (summercms.go/surf/router.go) needed to support the above: nested Group() calls -- a raw group cannot spawn a non-raw child). - RegisterMiddleware and RegisterMiddlewareFactory both gain a variant that tags an entry as house-envelope/error (see Task 1 action for the exact - shape); wrap() refuses a raw route referencing a house-tagged name at - Assemble time (registration time), returning a "surf: raw group cannot use - house-envelope middleware %q (plugin %q)" error -- not a runtime check. + shape): RegisterHouseMiddleware(pluginID, name string, fn pact.Middleware) + error and RegisterHouseMiddlewareFactory(pluginID, name string, fn + func(string) pact.Middleware) error. These are Router methods, called + ONLY from Assemble/BuildRouter's plugin loop (never directly by a plugin + -- see the pact.HasHouseMiddleware capability above); wrap() refuses a raw + route referencing a house-tagged name at Assemble time (registration + time), returning a "surf: raw group cannot use house-envelope middleware + %q (plugin %q)" error -- not a runtime check. +- Assemble's/BuildRouter's plugin loop gains a third pass (after the + existing HasMiddleware and HasMiddlewareFactories passes): for each + plugin implementing pact.HasHouseMiddleware, call + r.RegisterHouseMiddleware(p.ID(), name, fn) for every entry in + HouseMiddlewares(). No HasHouseMiddlewareFactories capability is added + this phase -- there is no house-tagged parameterized-middleware consumer + yet (the one real proof case, "inv.must-change-password", is a fixed + middleware); RegisterHouseMiddlewareFactory exists on Router as available + infrastructure but has no plugin-facing capability wired to it this phase. - compile()/wrap() recovery moves from one blanket recoverJSON(cors(...)) wrap in compile() to a per-route choice inside wrap(): raw routes get a bare-500 recoverBare; non-raw routes keep recoverJSON's existing @@ -177,10 +217,11 @@ func Slice[T any](s []T) []T - Task 1 (summercms.go): Raw group enforcement, route table, and the route:list command + Task 1 (summercms.go): Raw group enforcement, plugin-facing house-middleware capability, route table, and the route:list command summercms.go/pact/capabilities.go, summercms.go/surf/router.go, summercms.go/surf/router_test.go, summercms.go/surf/routetable.go, summercms.go/surf/routetable_test.go, summercms.go/surf/routelist_command.go, summercms.go/internal/build/build.go, summercms.go/internal/build/build_test.go - summercms.go/surf/router.go (full, post-06-02 -- route/Group structs, add/compile/wrap, RegisterMiddleware, Assemble, recoverJSON, cors) + summercms.go/surf/router.go (full, post-06-02 -- route/Group structs, add/compile/wrap, RegisterMiddleware, RegisterMiddlewareFactory, Assemble, recoverJSON, cors) + summercms.go/pact/capabilities.go (full, post-06-01 -- Router interface, HasMiddleware, HasMiddlewareFactories) summercms.go/surf/serve.go (full -- ServeCommand's bonfire.Command shape to mirror for RouteListCommand) summercms.go/bonfire/command.go (Command/Output shapes; bonfire/widgets.go's Table(headers []string, rows [][]string) signature) summercms.go/lagoon/commands.go (RuntimeCommands -- an existing bonfire.Command-returning function to mirror for style, not for content) @@ -189,11 +230,11 @@ func Slice[T any](s []T) []T .planning/phases/06-http-routing-auth-groups-and-rate-limiting/06-RESEARCH.md Pattern 3 (raw group structural exemption, lines 303-321) - In pact/capabilities.go, add GroupRaw(prefix string, middleware []string, fn func(Router)) to the Router interface, same signature shape as Group. + In pact/capabilities.go, add GroupRaw(prefix string, middleware []string, fn func(Router)) to the Router interface, same signature shape as Group. Add the HasHouseMiddleware interface exactly as specified in the interfaces block above (bare `Middleware`, not `pact.Middleware`, matching 06-01's HasMiddlewareFactories convention for code declared inside package pact itself). In surf/router.go: add raw bool to both Group and route. Add GroupRaw on *Router and *Group mirroring Group's existing body exactly, except the constructed child Group has raw: true; when Group() is called on an ALREADY-raw Group (g.raw == true), the child it constructs must also be raw:true (sticky inheritance -- a raw group cannot spawn a non-raw child via a later Group() call). Thread raw through add()'s signature (alongside pluginID/prefix/groupMW/method/path/handler/extra) so route.raw is set from whichever Group/GroupRaw declared it. - Add a houseTagged bool field to namedMiddleware and namedMiddlewareFactory. Add RegisterHouseMiddleware(pluginID, name string, fn pact.Middleware) error and RegisterHouseMiddlewareFactory(pluginID, name string, fn func(string) pact.Middleware) error, each calling the existing Register*/RegisterMiddlewareFactory internals but setting houseTagged: true on the stored entry (do not duplicate the dup-check logic -- factor a shared unexported registerNamed(pluginID, name string, tagged bool, ...) if that keeps both call sites DRY, or two thin wrappers if simpler; either is acceptable as long as the dup/empty-name checks are not duplicated verbatim). In fonoteka.go's plugin.go (see Task 3 of this plan for the actual call-site change), "inv.must-change-password" switches from RegisterMiddleware to RegisterHouseMiddleware. + Add a houseTagged bool field to namedMiddleware and namedMiddlewareFactory. Add RegisterHouseMiddleware(pluginID, name string, fn pact.Middleware) error and RegisterHouseMiddlewareFactory(pluginID, name string, fn func(string) pact.Middleware) error on *Router, each calling the existing RegisterMiddleware/RegisterMiddlewareFactory internals but setting houseTagged: true on the stored entry (do not duplicate the dup-check logic -- factor a shared unexported registerNamed(pluginID, name string, tagged bool, ...) if that keeps both call sites DRY, or two thin wrappers if simpler; either is acceptable as long as the dup/empty-name checks are not duplicated verbatim). These two methods are called EXCLUSIVELY from Assemble's/BuildRouter's plugin loop -- add a third pass there (after the existing HasMiddleware and HasMiddlewareFactories passes) that type-asserts each plugin against pact.HasHouseMiddleware and calls r.RegisterHouseMiddleware(p.ID(), name, fn) for every entry in HouseMiddlewares(); no plugin file calls RegisterHouseMiddleware directly (Task 3 of this plan implements the capability on fonoteka's Plugin, not a direct Router method call). In wrap()'s middleware-resolution loop: after resolving a name to either a namedMiddleware or a factory-built middleware, check `if rt.raw && resolvedHouseTagged { return nil, fmt.Errorf("surf: raw group cannot use house-envelope middleware %q (plugin %q)", name, rt.pluginID) }` -- this makes the refusal a registration-time (Assemble-time) failure, since Assemble already calls r.wrap(rt) once per route specifically to surface these errors before compile(). @@ -208,15 +249,16 @@ func Slice[T any](s []T) []T In internal/build/build.go's generateMain, add a line `b.WriteString("\tcommands = append(commands, surf.RouteListCommand(app, plugins))\n")` directly after the existing `surf.ServeCommand` append line. Update build_test.go's existing content-assertion test to also assert the generated main.go contains "surf.RouteListCommand". - cd /media/nvme/dev/golem15/summercms.io/summercms/summercms.go && go vet ./... && go test ./surf/... -run TestRawGroup -short && go test ./surf/... -run TestRouteTable -short && go test ./internal/build/... -short + cd /media/nvme/dev/golem15/summercms.io/summercms/summercms.go && go vet ./... && go test ./surf/... -run TestRawGroup -short && go test ./surf/... -run TestRouteTable -short && go test ./surf/... -run TestHouseMiddleware -short && go test ./internal/build/... -short - - A test registers a house-tagged middleware and a raw group referencing it by name; asserts Assemble/BuildRouter returns an error naming the middleware and the plugin. + - A test registers a fixture plugin implementing pact.HasHouseMiddleware (not a direct RegisterHouseMiddleware call) and a raw group referencing that name; asserts BuildRouter returns an error naming the middleware and the plugin -- proving the capability-to-registration wiring, not just the Router method in isolation. + - A test asserts a name registered via both Middlewares() and HouseMiddlewares() (two different plugins, or a contrived single plugin implementing both) fails boot with the existing duplicate-name error. - A test registers a raw group with a panicking handler; asserts the response is status 500 with an EMPTY body and no Content-Type header; a parallel non-raw panicking route still returns the existing {"error":true,"message":"Internal server error"} JSON body. - A test asserts Router.Routes() reflects Raw:true only for routes declared inside GroupRaw, and that a route declared via a plain Group() nested inside a GroupRaw() is also Raw:true (sticky inheritance). - go test ./internal/build/... confirms the generated main.go template contains both "surf.ServeCommand" and "surf.RouteListCommand". - Raw-group refusal is a registration-time failure, not a runtime check; the route table and route:list command exist and are independently testable. + Raw-group refusal is a registration-time failure reachable only through the plugin-facing pact.HasHouseMiddleware capability (no plugin calls Register* directly); the route table and route:list command exist and are independently testable. @@ -249,11 +291,13 @@ func Slice[T any](s []T) []T - Task 3 (fonoteka.go): Path-scoped CORS, per-group body limits, the raw OAuth group, and the production body-limit checkpoint + Task 3 (fonoteka.go): Path-scoped CORS, per-group body limits, the raw OAuth group, house-tagged plugin middleware, and the production body-limit checkpoint summercms.go/surf/cors.go, summercms.go/surf/cors_test.go, summercms.go/surf/bodylimit.go, summercms.go/surf/bodylimit_test.go, fonoteka.go/plugins/golem15/fonoteka/routes.go, fonoteka.go/plugins/golem15/fonoteka/plugin.go, fonoteka.go/config/http.yaml - summercms.go/surf/router.go (post-Task-1 of this plan -- compile()'s current blanket cors(r.origins, mux) call to replace) + summercms.go/surf/router.go (post-Task-1 of this plan -- compile()'s current blanket cors(r.origins, mux) call to replace, and the new pact.HasHouseMiddleware collection loop) + summercms.go/pact/capabilities.go (post-Task-1 -- HasHouseMiddleware's exact signature) summercms.go/compass/config.go (LoadSection with koanf tags -- the mechanism for CORSConfig) + fonoteka.go/plugins/golem15/fonoteka/plugin.go (current Middlewares() -- "inv.must-change-password" entry to move) /media/nvme/dev/golem15/fonoteka/config/cors.php (full, already read -- exact paths/methods/origins/headers/max_age/credentials values) /media/nvme/dev/golem15/fonoteka/plugins/golem15/fonoteka/routes.php lines 521-567 (the oauth group: bindings only, two POST routes each with one throttle, no jwt.auth/inv.scope, handlers arrive Phase 8) fonoteka.go/parity/fixtures/routes/GET__api_v1_fonoteka_genres_personal_token.yaml (confirms wildcard CORS on the personal-token group) @@ -266,18 +310,19 @@ func Slice[T any](s []T) []T In fonoteka.go/config/http.yaml, add: cors: paths: ["api/*", "_user/api/*", "_journal/api/*", "_feedback/api/*", "oauth/mcp/*"], allowed_methods: ["*"], allowed_origins: ["*"], allowed_origins_patterns: [], allowed_headers: ["*"], exposed_headers: [], max_age: 0, supports_credentials: false (byte-for-byte from config/cors.php); body_limits: default_bytes: 8388608 (8 MiB, PHP's stock post_max_size, marked with a comment "INTERIM default per 06-RESEARCH.md Assumption A2 -- replace with the real production client_max_body_size/post_max_size once read off the host"), upload_bytes: 2097152 (2 MiB, PHP's stock upload_max_filesize, same INTERIM comment). - In fonoteka.go/plugins/golem15/fonoteka/plugin.go, switch the "inv.must-change-password" registration from RegisterMiddleware to RegisterHouseMiddleware (Task 1's new capability) -- this is the concrete proof case for D-16's registration-time refusal (a raw group referencing "inv.must-change-password" must now fail boot). + In fonoteka.go/plugins/golem15/fonoteka/plugin.go: remove the "inv.must-change-password": middleware.MustChangePassword entry from Middlewares()'s returned map, and add a new method HouseMiddlewares() map[string]pact.Middleware { return map[string]pact.Middleware{"inv.must-change-password": middleware.MustChangePassword} } implementing pact.HasHouseMiddleware (Task 1's new capability), plus `_ pact.HasHouseMiddleware = (*Plugin)(nil)` alongside the existing interface assertions. This is the concrete, production-path proof case for D-16's registration-time refusal: the plugin never calls RegisterHouseMiddleware itself -- Assemble's/BuildRouter's new third plugin-loop pass (Task 1) does, from this capability method, exactly the way Middlewares() already flows through the existing HasMiddleware pass. In fonoteka.go/plugins/golem15/fonoteka/routes.go, add the oauth group via r.GroupRaw("/", surf.Use(), func(g pact.Router) {}) -- bindings has no Go equivalent (no route-model binding concept), so the middleware list is empty; the group exists purely to prove GroupRaw's structural behavior at this prefix and to be the landing spot for Phase 8's two real routes, each of which will individually add "throttle:fonoteka-oauth-token"/"throttle:fonoteka-oauth-register" per-route (not group-level, matching routes.php's actual per-route ->middleware() calls at lines 561/565) -- do not attach the throttle names to the empty GroupRaw call itself, since PHP attaches them per-route, not per-group, and there is no route to attach them to yet. - cd /media/nvme/dev/golem15/summercms.io/summercms/summercms.go && go vet ./... && go test ./surf/... -run TestCORS -short && go test ./surf/... -run TestBodyLimit -short + cd /media/nvme/dev/golem15/summercms.io/summercms/summercms.go && go vet ./... && go test ./surf/... -run TestCORS -short && go test ./surf/... -run TestBodyLimit -short && cd ../fonoteka.go && go vet ./... && go test ./plugins/golem15/fonoteka/... -short - A test against the assembled fonoteka router asserts a request to /_fonoteka/api/v1/genres carries NO Access-Control-Allow-Origin header, and a request to /api/v1/fonoteka/genres carries Access-Control-Allow-Origin: * (Pitfall 10, matching the recorded parity fixture). - A test asserts a request body larger than http.body_limits.default_bytes on a non-raw route fails with the standard http.MaxBytesReader error surfaced as a 4xx/error response (exact status code documented in the test, matching whatever the handler's own body-read error path already returns via the existing recoverJSON/error handling -- if no handler currently reads a body, test MaxBytesReader directly against a framework fixture route). - A test asserts the oauth GroupRaw declaration has Raw:true in the route table (even with zero routes, assert via boot-time introspection of the Group's raw flag if Routes() has nothing to show for an empty group -- prefer adding one framework-fixture raw route inside a TEST-only GroupRaw call, not inside production routes.go, to get a concrete RouteInfo{Raw:true} entry to assert against). - - A test asserts registering "inv.must-change-password" (now house-tagged) inside a raw group fails Assemble/BuildRouter with an error naming the middleware. + - A test activates the REAL golem15.user and golem15.fonoteka plugins through party.Activate and surf.BuildRouter (the production Boot/Assemble path, not a synthetic fixture plugin) and asserts boot succeeds normally now that "inv.must-change-password" is house-tagged via HouseMiddlewares() -- this proves the capability wiring end to end on real plugin code, not just the isolated Task-1 mechanism test. + - A second test, using the same real-plugin activation, constructs a router where a raw group additionally references "inv.must-change-password" (a temporary test-only route addition, not committed to production routes.go) and asserts BuildRouter fails with an error naming "inv.must-change-password" and "golem15.fonoteka" -- the D-16 acceptance case reachable through the production Boot/Assemble path, not just Task 1's synthetic-plugin unit test. This task is a blocking checkpoint per the user-resolved Open Question 1 (body-size limits). Before this plan is considered complete: @@ -287,7 +332,7 @@ func Slice[T any](s []T) []T 4. Re-run the body-limit tests to confirm they still pass against the new numbers (they assert against config-loaded values, not hardcoded ones, so no test code changes should be needed -- only the config numbers and their comments). Type "approved" once the real production body-size numbers are recorded in fonoteka.go/config/http.yaml, or describe what changed if the numbers differ from the INTERIM defaults. - CORS is path-scoped and matches config/cors.php exactly; body limits are enforced per group with the real (operator-confirmed) production numbers, not a guess; the oauth group is structurally raw with zero handlers. + CORS is path-scoped and matches config/cors.php exactly; body limits are enforced per group with the real (operator-confirmed) production numbers, not a guess; the oauth group is structurally raw with zero handlers; "inv.must-change-password" is declared house-tagged through the plugin-facing capability and proven end to end via the real plugin's Boot/Assemble path. @@ -306,7 +351,7 @@ func Slice[T any](s []T) []T | Threat ID | Category | Component | Disposition | Mitigation Plan | |-----------|----------|-----------|-------------|-----------------| | T-06-10 | Elevation of Privilege | Route table mutual-exclusivity | mitigate | routetable_test.go asserts, over the FULL assembled route table, that no /api/v1/fonoteka route carries jwt.auth and no /_fonoteka/api/v1 route carries inv_token/inv.scope: -- this completes the partial coverage 06-01 accepted (T-06-02) | -| T-06-11 | Tampering | Raw-group house-middleware refusal | mitigate | Registration-time (not request-time) failure; a future PR cannot silently attach an envelope middleware to the OAuth group and have it ship | +| T-06-11 | Tampering | Raw-group house-middleware refusal | mitigate | Registration-time (not request-time) failure, reachable only through the pact.HasHouseMiddleware capability (no plugin calls Register* directly); a future PR cannot silently attach an envelope middleware to the OAuth group and have it ship, and cannot bypass the capability by calling a Router method a plugin package cannot even reach | | T-06-12 | Denial of Service | Unbounded request body on a non-raw route | mitigate | http.MaxBytesReader applied unconditionally per non-raw route via the default body limit; raw (RFC) routes rely on their own future handler-level limits, matching PHP's own scoping | | T-06-13 | Information Disclosure | Body-size limits shipped as a guess | mitigate | INTERIM defaults are clearly commented as unverified; the plan cannot close without the blocking checkpoint recording operator-confirmed production numbers -- never presented as verified when it is not | | T-06-SC | Tampering | npm/go install in scripts/check-openapi.sh (swag, openapi-typescript) | 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 | @@ -318,7 +363,7 @@ cd ../fonoteka.go && go vet ./... && go test ./plugins/golem15/fonoteka/... -sho -- Raw groups refuse house-tagged middleware at registration time and recover with a bare 500. +- Raw groups refuse house-tagged middleware at registration time and recover with a bare 500; the refusal is reachable only through the plugin-facing pact.HasHouseMiddleware capability. - The route table and route:list command exist and prove group mutual-exclusivity by inspection. - wire package ships the promoted response-convention helpers, unit-tested independently. - CORS is path-scoped and matches config/cors.php exactly (JWT group: no headers; token group: wildcard). diff --git a/.planning/phases/06-http-routing-auth-groups-and-rate-limiting/06-05-PLAN.md b/.planning/phases/06-http-routing-auth-groups-and-rate-limiting/06-05-PLAN.md index 89a319e..247c945 100644 --- a/.planning/phases/06-http-routing-auth-groups-and-rate-limiting/06-05-PLAN.md +++ b/.planning/phases/06-http-routing-auth-groups-and-rate-limiting/06-05-PLAN.md @@ -117,7 +117,7 @@ Output: coverage-gap tests across both repos; a full route-table isolation test; All coverage test files created in Task 1 and Task 2 of this plan - Write 06-SECURITY-REVIEW.md with one table row per threat ID from T-06-01 through T-06-18 plus T-06-SC (19 entries total across Plans 06-01-06-04's threat_model blocks): Threat ID, Category, Plan of origin, Disposition (copy verbatim from the originating plan), and either the exact test function name (file:TestName) that proves the mitigation holds, or a restated accept/transfer rationale copied verbatim from the originating plan's threat_model table (for the three `accept` dispositions: T-06-03, T-06-08, T-06-SC). Explicitly grep both repos for any place `bouncer.Credential` or `TokenGuard`'s resolved token is logged, and confirm no call site logs the raw bearer token or the token hash outside the guard's own DB update (grep -rn "raw\|bearer\|LastUsedIP" across the auth package and confirm no fmt.Print*/log.* call is adjacent). Explicitly grep for any second `RegisterMiddleware`(non-house) call registering `"inv.must-change-password"` to confirm Plan 06-03's switch to `RegisterHouseMiddleware` is the only registration site (no stale duplicate). + Write 06-SECURITY-REVIEW.md with one table row per threat ID from T-06-01 through T-06-18 plus T-06-SC (19 entries total across Plans 06-01-06-04's threat_model blocks): Threat ID, Category, Plan of origin, Disposition (copy verbatim from the originating plan), and either the exact test function name (file:TestName) that proves the mitigation holds, or a restated accept/transfer rationale copied verbatim from the originating plan's threat_model table (for the three `accept` dispositions: T-06-03, T-06-08, T-06-SC). Explicitly grep both repos for any place `bouncer.Credential` or `TokenGuard`'s resolved token is logged, and confirm no call site logs the raw bearer token or the token hash outside the guard's own DB update (grep -rn "raw\|bearer\|LastUsedIP" across the auth package and confirm no fmt.Print*/log.* call is adjacent). Explicitly grep fonoteka.go/plugins/golem15/fonoteka/plugin.go to confirm "inv.must-change-password" appears only inside HouseMiddlewares() (the pact.HasHouseMiddleware capability Plan 06-03 added) and nowhere inside Middlewares() -- confirming Plan 06-03's move off the house-envelope-tagged name is the only registration path (no stale duplicate left behind in the fixed-middleware map). cd /media/nvme/dev/golem15/summercms.io/summercms/summercms.go && grep -c "^| T-06-" .planning/phases/06-http-routing-auth-groups-and-rate-limiting/06-SECURITY-REVIEW.md @@ -125,7 +125,7 @@ Output: coverage-gap tests across both repos; a full route-table isolation test; - The security review table contains exactly 19 T-06-xx rows (T-06-01 through T-06-18 plus T-06-SC), none missing. - Every `mitigate` disposition names a real, currently-passing test function (verified by cross-referencing the test file); every `accept` disposition restates the originating plan's rationale rather than inventing a new one. - - `grep -rn "RegisterMiddleware(p.ID(), \"inv.must-change-password\"" fonoteka.go/plugins/golem15/fonoteka/plugin.go` returns zero matches (confirms the house-tagged variant is the only registration). + - `grep -n "inv.must-change-password" fonoteka.go/plugins/golem15/fonoteka/plugin.go` shows the name only inside `HouseMiddlewares()`, never inside `Middlewares()` (confirms Plan 06-03's move to the pact.HasHouseMiddleware capability is complete, with no stale duplicate registration). Every threat identified across Phase 6's four implementation plans is mapped to a passing test or a deliberate, restated risk acceptance -- no threat ID is silently dropped between planning and phase close. diff --git a/.planning/phases/06-http-routing-auth-groups-and-rate-limiting/06-VALIDATION.md b/.planning/phases/06-http-routing-auth-groups-and-rate-limiting/06-VALIDATION.md index 109f0da..6eca04c 100644 --- a/.planning/phases/06-http-routing-auth-groups-and-rate-limiting/06-VALIDATION.md +++ b/.planning/phases/06-http-routing-auth-groups-and-rate-limiting/06-VALIDATION.md @@ -41,10 +41,10 @@ created: 2026-09-19 | 06-01-T1 | 06-01 | 1 | HTTP-05 | T-06-01 | Router verbs + parameterized middleware factories; guard registry primitives unit-tested in isolation | unit | `go test ./surf/... ./bouncer/... ./pact/... -short` (summercms.go) | ✅ created by plan | ⬜ pending | | 06-01-T2 | 06-01 | 1 | HTTP-05 | T-06-04, T-06-05 | Real inv_token guard + inv.scope exact 401/403 bodies; jwt guard behaviorally unchanged | unit + testcontainers | `go test ./plugins/golem15/fonoteka/... ./plugins/golem15/user/... -short` (fonoteka.go) | ✅ created by plan | ⬜ pending | | 06-01-T3 | 06-01 | 1 | HTTP-03 | T-06-02 | Same handler serves JWT and personal-token genres; parity-green on both | integration + parity | `go test ./plugins/golem15/fonoteka/... -run TestGenresSharedHandler -short && go test ./parity/... -run TestParityCorpus` (fonoteka.go) | ✅ created by plan | ⬜ pending | -| 06-02-T1 | 06-02 | 2 | HTTP-04 | T-06-06 | Fixed-window Store/Limiter matching Laravel wire contract; trusted-proxy ClientIP | unit | `go test ./surf/... -run 'TestLimiter\|TestClientIP' -short` (summercms.go) | ✅ created by plan | ⬜ pending | +| 06-02-T1 | 06-02 | 2 | HTTP-04 | T-06-06 | Fixed-window Store/FixedWindowLimiter (distinct from the pre-existing surf.Limiter interface seam) matching Laravel wire contract; trusted-proxy ClientIP | unit | `go test ./surf/... -run 'TestFixedWindowLimiter\|TestClientIP' -short` (summercms.go) | ✅ created by plan | ⬜ pending | | 06-02-T2 | 06-02 | 2 | HTTP-04 | T-06-07 | Five named buckets registered; token group throttled; PublicShareHeaders 429 rewrite | unit + integration | `go test ./plugins/golem15/fonoteka/... -run TestPublicShareHeaders -short` (fonoteka.go) | ✅ created by plan | ⬜ pending | | 06-02-T3 | 06-02 | 2 | HTTP-04 | T-06-09 | All six declared route groups boot cleanly; APP_DEBUG=false pinned for future fixture recordings | integration | `go test ./plugins/golem15/fonoteka/... -run TestAllRouteGroupsBoot -short` (fonoteka.go) | ✅ created by plan | ⬜ pending | -| 06-03-T1 | 06-03 | 3 | HTTP-06 | T-06-10, T-06-11 | Raw group refuses house-envelope middleware at registration; bare 500 on raw panic; route table + route:list | unit | `go test ./surf/... -run 'TestRawGroup\|TestRouteTable' -short` (summercms.go) | ✅ created by plan | ⬜ pending | +| 06-03-T1 | 06-03 | 3 | HTTP-06 | T-06-10, T-06-11 | Raw group refuses house-envelope middleware (declared only via the pact.HasHouseMiddleware plugin capability) at registration; bare 500 on raw panic; route table + route:list | unit | `go test ./surf/... -run 'TestRawGroup\|TestRouteTable\|TestHouseMiddleware' -short` (summercms.go) | ✅ created by plan | ⬜ pending | | 06-03-T2 | 06-03 | 3 | HTTP-06, HTTP-08 | — | wire response helpers (JSON writer, +00:00 time, tri-state bool, never-nil slice); swag→openapi-typescript pipeline | unit + build-gate | `go test ./wire/... -short && bash scripts/check-openapi.sh` (fonoteka.go) | ✅ created by plan | ⬜ pending | | 06-03-T3 | 06-03 | 3 | HTTP-09 | T-06-12, T-06-13 | Path-scoped CORS matches config/cors.php; body limits enforced with operator-confirmed production numbers; oauth group raw | integration + human-verify | `go test ./surf/... -run 'TestCORS\|TestBodyLimit' -short` (summercms.go); blocking checkpoint for production body-size numbers | ✅ created by plan | ⬜ pending | | 06-04-T1 | 06-04 | 1 | HTTP-07 | T-06-14 | Private/reserved/CGNAT/metadata IP table matches ManualCoverUrlFetcher.php exactly | unit | `go test ./fetchguard/... -run TestIsReservedOrPrivate -v` (summercms.go) | ✅ created by plan | ⬜ pending |