175 lines
11 KiB
Markdown
175 lines
11 KiB
Markdown
---
|
|
phase: 06-http-routing-auth-groups-and-rate-limiting
|
|
plan: 12
|
|
type: execute
|
|
wave: 1
|
|
depends_on: []
|
|
files_modified:
|
|
- surf/router.go
|
|
- surf/bodylimit.go
|
|
- surf/limiter.go
|
|
- bouncer/registry.go
|
|
- bouncer/jwt.go
|
|
autonomous: true
|
|
gap_closure: true
|
|
requirements: [HTTP-04, HTTP-09, HTTP-05, HTTP-06]
|
|
|
|
must_haves:
|
|
truths:
|
|
- "A named middleware that reads r.Body on a non-raw route cannot read more than the selected body limit (default or body.limit:N override); raw routes remain unlimited"
|
|
- "The body limit sits inside recoverJSON/recoverBare and outside every named/factory middleware"
|
|
- "RegisterBucket fails boot on nil Key, Max < 1, Decay <= 0, or a limiter with a nil store"
|
|
- "Inline throttle minutes that overflow time.Duration fail boot; Middleware no longer passes traffic through when Key/store/resolution is missing"
|
|
- "Missing or non-positive http.body_limits.default_bytes / upload_bytes config fails BuildRouter instead of becoming 0"
|
|
- "A ServeMux semantic route conflict is returned as an error from compile/Assemble, never a panic"
|
|
- "Middleware factories are constructed once per distinct name:param during BuildRouter+compile"
|
|
- "Typed-nil guards are rejected by bouncer Registry.Register; a fractional or non-finite JWT numeric subject is rejected"
|
|
artifacts:
|
|
- path: surf/router.go
|
|
provides: "Corrected wrap ordering, factory cache, body-config validation, panic-to-error mux registration"
|
|
- path: surf/limiter.go
|
|
provides: "Fail-closed limiter definition validation"
|
|
- path: bouncer/registry.go
|
|
provides: "Typed-nil guard rejection"
|
|
- path: bouncer/jwt.go
|
|
provides: "Integer-only numeric subject"
|
|
key_links:
|
|
- from: surf/router.go
|
|
to: surf/bodylimit.go
|
|
via: "bodyLimit applied after the named-middleware loop, before locale/recover"
|
|
pattern: "bodyLimit\\("
|
|
- from: surf/router.go
|
|
to: surf/limiter.go
|
|
via: "RegisterBucket errors propagate from BuildRouter"
|
|
pattern: "RegisterBucket"
|
|
---
|
|
|
|
<objective>
|
|
Close the surf/bouncer verification gaps: body cap must bound body-consuming middleware (HTTP-09), invalid limiter definitions must fail boot rather than fail open (HTTP-04), plus the five verification warnings.
|
|
|
|
Purpose: shared infrastructure must fail closed. Output: edits to surf/router.go, surf/limiter.go, bouncer/registry.go, bouncer/jwt.go. Comprehensive tests are Plan 06-14; this plan only fixes existing tests that the stricter validation legitimately breaks and may add one smoke test per fix.
|
|
</objective>
|
|
|
|
<execution_context>
|
|
@$HOME/.claude/get-shit-done/workflows/execute-plan.md
|
|
@$HOME/.claude/get-shit-done/templates/summary.md
|
|
</execution_context>
|
|
|
|
<context>
|
|
@CLAUDE.md
|
|
@.planning/STATE.md
|
|
@.planning/phases/06-http-routing-auth-groups-and-rate-limiting/06-VERIFICATION.md
|
|
@.planning/phases/06-http-routing-auth-groups-and-rate-limiting/06-CONTEXT.md
|
|
@surf/router.go
|
|
@surf/bodylimit.go
|
|
@surf/limiter.go
|
|
@bouncer/registry.go
|
|
@bouncer/jwt.go
|
|
</context>
|
|
|
|
<tasks>
|
|
|
|
<task type="auto">
|
|
<name>Task 1: Router ordering, factory cache, body-config validation, mux conflict error (surf/router.go)</name>
|
|
<files>surf/router.go, surf/bodylimit.go</files>
|
|
<read_first>surf/router.go (wrap lines 353-401, compile 338-351, BuildRouter 414-495), surf/bodylimit.go, compass/config.go (Lookup, Int at ~113-140), 06-VERIFICATION.md gaps 1 and warnings</read_first>
|
|
<action>
|
|
In Router.wrap, move the bodyLimit application: remove it from directly around the terminal handler and apply it after the named-middleware loop finishes (so it is the outermost wrapper of all named/factory middleware) but before locale(h) and the recoverJSON/recoverBare wrapping (per D-verification gap 1, so panics inside body-consuming middleware are still recovered and raw routes still get limit 0 from routeBodyLimit). Keep orgSlot and constrain innermost. Update the comment in the body.limit factory registration in BuildRouter to say the limit is applied outermost-inside-recovery in wrap().
|
|
|
|
Build middleware factories once: add a per-Router cache map keyed by the full "base:param" name holding the constructed pact.Middleware (initialize in New). In wrap, look up the cache before calling factory.fn(param); store after first construction. BuildRouter's validation pass and compile's second wrap then reuse the same instances. Validation (ValidateThrottle, parseBodyLimit) still runs for every route.
|
|
|
|
Body config fail-boot: in BuildRouter when app != nil and app.Config != nil, read http.body_limits.default_bytes and http.body_limits.upload_bytes with Config.Lookup; if either is absent, not numeric, or converts to a value < 1, return an error naming the key (do not fall through to zero). Convert via a small helper that accepts int, int64, uint64 and float64 whole values. When app or app.Config is nil keep the existing zero (no config source at all) behaviour used by unit tests. If existing tests build a Config without these keys, add the two keys to those test configs rather than weakening the check.
|
|
|
|
Route conflict: in compile, register each route through a helper that defers recover() around mux.Handle and returns fmt.Errorf("surf: route conflict for %s (plugin %q): %v", ...). compile then returns that error instead of panicking.
|
|
</action>
|
|
<verify>
|
|
<automated>go vet ./surf/... && go test ./surf/... -count=1 -short</automated>
|
|
</verify>
|
|
<acceptance_criteria>
|
|
- grep of surf/router.go shows `bodyLimit(` is called after the `for i := len(rt.middleware) - 1` loop and before `h = locale(h)`
|
|
- A quick smoke test (added to surf/bodylimit_test.go) with limit 4, named middleware doing io.ReadAll(r.Body) and a 10-byte body observes a read error (http.MaxBytesError) inside the middleware
|
|
- compile of two semantically conflicting patterns returns a non-nil error and does not panic
|
|
- BuildRouter with a Config lacking http.body_limits.default_bytes returns an error containing "default_bytes"
|
|
- `go vet ./surf/...` and `go test ./surf/... -short` exit 0
|
|
</acceptance_criteria>
|
|
<done>Cap bounds all named middleware; factories built once; missing body config and mux conflicts are boot errors.</done>
|
|
</task>
|
|
|
|
<task type="auto">
|
|
<name>Task 2: Fail-closed limiter definitions (surf/limiter.go)</name>
|
|
<files>surf/limiter.go</files>
|
|
<read_first>surf/limiter.go, surf/limiter_store.go, surf/limiter_test.go, surf/limiter_coverage_test.go (find tests that register nil-Key or zero-decay buckets or pass a nil store)</read_first>
|
|
<action>
|
|
RegisterBucket: after existing name check, return errors naming plugin and bucket when l.store == nil, b.Key == nil, b.Max < 1, or b.Decay <= 0. ValidateThrottle: also error when l.store == nil. In resolve, before multiplying the inline minutes, reject m greater than math.MaxInt64 / int64(time.Minute) (import math) with a "malformed throttle" style error, so the Duration conversion cannot overflow; also keep n<1 / m<1 rejection.
|
|
|
|
Middleware: remove every pass-through. When l is nil, resolve returns an error, or Key/store is nil at construction, return a middleware whose handler writes a 500 (http.StatusInternalServerError, empty body via WriteHeader only) and never calls next. This is unreachable at boot because ValidateThrottle/RegisterBucket reject the same states, but must be fail-closed if reached. Do not change the 429 wire format or headers.
|
|
|
|
Fix existing tests only where they registered now-invalid buckets or relied on pass-through, by giving them a real Key/Max/Decay and a real store (do not weaken validation). Add one smoke test asserting RegisterBucket rejects a zero-decay bucket.
|
|
</action>
|
|
<verify>
|
|
<automated>go vet ./surf/... && go test ./surf/... -count=1 -race -short</automated>
|
|
</verify>
|
|
<acceptance_criteria>
|
|
- `RegisterBucket("p","n",Bucket{Max:1,Decay:0,Key:k})` returns non-nil error (smoke test)
|
|
- `grep -n "next.ServeHTTP" surf/limiter.go` shows only the post-Attempt admitted call
|
|
- Inline throttle "1,9223372036854775807" fails ValidateThrottle
|
|
- The 429 exact body/header tests already in the suite still pass
|
|
</acceptance_criteria>
|
|
<done>No limiter state can silently disable enforcement.</done>
|
|
</task>
|
|
|
|
<task type="auto">
|
|
<name>Task 3: Typed-nil guard rejection and integer-only JWT subject (bouncer)</name>
|
|
<files>bouncer/registry.go, bouncer/jwt.go</files>
|
|
<read_first>bouncer/registry.go, bouncer/jwt.go lines 120-186, bouncer/registry_test.go, bouncer/jwt_test.go</read_first>
|
|
<action>
|
|
registry.go Register: after the g == nil check, use reflect (stdlib) to reject any g whose reflect.ValueOf kind is Pointer, Map, Slice, Func, Chan or Interface and IsNil() is true, returning the existing "registered empty guard" style error naming plugin and guard.
|
|
|
|
jwt.go subject: for float64 reject NaN, Inf, v <= 0, v != math.Trunc(v), or v > 9007199254740992 (2^53) by returning "" ; otherwise format as int64. For json.Number reject values that do not parse with strconv.ParseInt as a positive integer (return ""); string branch unchanged. Existing legacy-equivalence tests for whole-number float subjects must keep passing.
|
|
</action>
|
|
<verify>
|
|
<automated>go vet ./bouncer/... && go test ./bouncer/... -count=1 -race -short</automated>
|
|
</verify>
|
|
<acceptance_criteria>
|
|
- Registering a typed-nil pointer implementing Guard returns an error (smoke test)
|
|
- A token with sub 12.5 fails authentication; sub 12 (float64) still resolves user 12
|
|
- `go test ./bouncer/... -short` exits 0
|
|
</acceptance_criteria>
|
|
<done>Typed-nil guards and fractional subjects are rejected.</done>
|
|
</task>
|
|
|
|
</tasks>
|
|
|
|
<threat_model>
|
|
## Trust Boundaries
|
|
|
|
| Boundary | Description |
|
|
|----------|-------------|
|
|
| client body -> named middleware | untrusted body stream read by auth/plugin middleware before any handler |
|
|
| plugin bucket definitions -> limiter | plugin-declared Max/Decay/Key define whether a security control enforces |
|
|
| signed JWT claim -> user id | numeric subject converted to a lookup key |
|
|
|
|
## STRIDE Threat Register
|
|
|
|
| Threat ID | Category | Component | Disposition | Mitigation Plan |
|
|
|-----------|----------|-----------|-------------|-----------------|
|
|
| T-06-28 | Denial of Service | surf/router.go wrap | mitigate | bodyLimit outermost of named middleware, inside recovery; regression in 06-14 |
|
|
| T-06-29 | Denial of Service / Elevation | surf/limiter.go | mitigate | RegisterBucket/ValidateThrottle reject nil store/Key, Max<1, Decay<=0, overflow; Middleware fails closed |
|
|
| T-06-32 | Denial of Service | bouncer/registry.go | mitigate | reflect-based typed-nil rejection at Register |
|
|
| T-06-33 | Spoofing | bouncer/jwt.go subject | mitigate | reject fractional/non-finite/oversized numeric sub |
|
|
| T-06-34 | Denial of Service | surf/router.go compile | mitigate | recover ServeMux conflict panic into returned error |
|
|
| T-06-35 | Denial of Service | surf/router.go BuildRouter | mitigate | missing/malformed body_limits config fails boot |
|
|
</threat_model>
|
|
|
|
<verification>
|
|
`go vet ./... && go test ./... -count=1 -race -short` green in summercms.go; the sibling ../fonoteka.go `go test ./... -short` still green (its buckets in plugin.go must satisfy the new validation).
|
|
</verification>
|
|
|
|
<success_criteria>
|
|
All must_haves truths hold; both repositories build and test green; commit is code only (no planning docs), no co-author tags.
|
|
</success_criteria>
|
|
|
|
<output>
|
|
Create `.planning/phases/06-http-routing-auth-groups-and-rate-limiting/06-12-SUMMARY.md` when done
|
|
</output>
|