amadan
idear: key the limiter by network, pin the grant matrix to the spec (Task 5, fix round 1)
The rate-limit key was the defect. On IPv6 every host holds a /64, so
keying on the bare address meant one machine could rotate source
addresses for free — no per-client limit at all, and 4096 rotations
filled the bounded table into its fail-closed arm, refusing every
UNSEEN client: invitees, and orphans, whose only healing path is
POST /invitations/{token}. clientIP now folds v6 to its /64 and leaves
v4 whole, unmapping IPv4-mapped forms and stripping the zone so one
client cannot hold two budgets. The claim that idear never reads a
forwarding header is now a test rather than a comment.
Grantable had a hole the derived tests could not see: the test asks
Grantable and Grantable asks checkInviteRole, so relaxing
checkInviteRole moved the expectation with it — an Admin minting a
peer Admin left handlers_test.go entirely green.
TestGrantableMatchesTheDesign writes the matrix out from spec §5 and
compares both ways, the way TestRoutesMatchTheDesign already pins the
routes. Writing it found a real defect: Grantable checked the rank rule
but not the authority floor, so a deactivated admin's page rendered a
role selector; it now asks mayManage too.
standing() could be gutted with the suite green, which would leave the
orphan with no accept control while the POST still worked; the
invitation page's SignedIn/Reconcile are now asserted for all three
standings. Also: a member-id miss is proved to render the app's own 404
bytes, multipart bodies are parsed (an app whose form grows a file
input was silently 400ing), Routes() is proved complete by reflection
so an unmounted handler cannot hide from every derived test, Site is
documented as coming from an untrusted header, and the Deliver-nil
fallback's cookie is documented as un-Secure and warned about at boot.
Eight mutations run, each caught by the test it was aimed at; the
fourteen from round 1 behave unchanged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
5 files changed,
+435
−40
handlers.go+62 −12handlers_test.go+186 −6internal/ideartest/app.go+36 −0ratelimit.go+51 −12ratelimit_internal_test.go+100 −10
diff --git a/handlers.go b/handlers.go| index b79e3d4..994f436 100644 |
| --- a/handlers.go |
| +++ b/handlers.go |
| @@ -105,8 +105,18 @@ type HandlerConfig struct { |
| RenderInvitation func(w http.ResponseWriter, r *http.Request, d InvitationPage) |
| // Site names this instance on the invitation page — "Acme's |
| - // board". Default: the request's Host, which is honest but |
| - // unlovely. |
| + // board". |
| + // |
| + // SET IT IN PRODUCTION. The default is the request's Host, and |
| + // Host is a CLIENT-SUPPLIED HEADER: on the public, unauthenticated |
| + // invitation page, an attacker who mails somebody a link and can |
| + // steer the Host (a permissive reverse proxy, a wildcard vhost, |
| + // a raw request) has the page render text of their choosing as |
| + // the instance's name — "Acme Security, verify your password at |
| + // ...". idear escapes nothing on the app's behalf either; the |
| + // renderer owns that. The default is honest about where the |
| + // request landed, which is useful in development and is not a |
| + // name you should show a stranger. |
| Site string |
| // MembersPath is where a successful mutation redirects. Default |
| @@ -124,9 +134,15 @@ type HandlerConfig struct { |
| // When it is nil idear puts the LINK ITSELF in the flash notice, |
| // so a bare mount is usable: the token is shown exactly once, to |
| // the admin who minted it, and can then be pasted into whatever |
| - // channel the team uses. That is also a credential in a cookie |
| - // for sixty seconds, so an app that can send mail should set this |
| - // and keep the token out of the browser entirely. |
| + // channel the team uses. |
| + // |
| + // That fallback puts a live credential in a cookie, and |
| + // rastrillo/flash sets HttpOnly, SameSite=Lax and MaxAge=60 but |
| + // NOT Secure — so on a plain-http origin the token crosses the |
| + // wire in clear text, and it is written by a package idear does |
| + // not own, so idear cannot add the flag. An app that can send |
| + // mail should set this and keep the token out of the browser |
| + // entirely; NewHandlers logs a warning when it is nil. |
| Deliver func(r *http.Request, inv *Invitation, link string) error |
| // RateLimit bounds the two public routes. See RateLimit — the |
| @@ -172,6 +188,12 @@ func NewHandlers(cfg HandlerConfig) (*Handlers, error) { |
| if cfg.ClientKey == nil { |
| cfg.ClientKey = clientIP |
| } |
| + if cfg.Deliver == nil { |
| + // Once, at boot, rather than on every invitation: the |
| + // fallback is usable and it is not what a deployed app should |
| + // be doing. See HandlerConfig.Deliver. |
| + cfg.Roster.cfg.Logger.Warn("idear: no HandlerConfig.Deliver, so invitation LINKS will be flashed to the inviting admin's browser in a cookie that rastrillo/flash does not mark Secure; set Deliver to mail them instead") |
| + } |
| return &Handlers{cfg: cfg, limit: newLimiter(cfg.RateLimit)}, nil |
| } |
| @@ -228,12 +250,16 @@ func (h *Handlers) Routes() []Route { |
| // Grantable is the roles actor may hand out, highest first — what a |
| // role selector must be built from. |
| // |
| -// It is derived from checkInviteRole, the same predicate Invite and |
| -// SetRole enforce inside their transactions, so the form and the store |
| -// cannot disagree. RoleOwner is never in it for anybody: ownership |
| -// moves only by Transfer. |
| +// It is derived from the store's OWN predicates and never from a |
| +// second copy of the rule: mayManage is the authority floor Invite and |
| +// SetRole check first (active, and at least Admin), and |
| +// checkInviteRole is the rank rule they check next, both inside their |
| +// transactions. So the form and the store cannot disagree, and a |
| +// deactivated actor — whose row outlives their privileges, because |
| +// removal is never a delete — is offered nothing at all. RoleOwner is |
| +// never in it for anybody: ownership moves only by Transfer. |
| func Grantable(actor *Member) []Role { |
| - if actor == nil { |
| + if actor == nil || mayManage(actor) != nil { |
| return nil |
| } |
| var out []Role |
| @@ -756,7 +782,9 @@ func (h *Handlers) allow(w http.ResponseWriter, r *http.Request) bool { |
| return false |
| } |
| -// site names the instance on the invitation page. |
| +// site names the instance on the invitation page. With no configured |
| +// Site this is the request's Host — a client-supplied header. See |
| +// HandlerConfig.Site. |
| func (h *Handlers) site(r *http.Request) string { |
| if h.cfg.Site != "" { |
| return h.cfg.Site |
| @@ -783,11 +811,33 @@ func field(r *http.Request, name string) string { |
| if r.PostForm == nil { |
| // An unparseable body leaves an empty form behind, which every |
| // caller here treats as a missing field: 400, not a default. |
| - _ = r.ParseForm() |
| + // |
| + // MULTIPART IS PARSED EXPLICITLY. ParseForm does not populate |
| + // PostForm for multipart/form-data — it parses only the query |
| + // — so an app whose members form carries a file input would |
| + // otherwise 400 on every submit with nothing to explain it. |
| + // The failure direction was right and the diagnosis was |
| + // impossible. |
| + // |
| + // The memory bound is small because nothing here reads a file: |
| + // these are three short text fields. Anything larger spills to |
| + // a temp file, which is why this runs only on the |
| + // admin-guarded mutations — the public routes read no fields |
| + // at all. |
| + if strings.HasPrefix(r.Header.Get("Content-Type"), "multipart/form-data") { |
| + _ = r.ParseMultipartForm(multipartMemory) |
| + } else { |
| + _ = r.ParseForm() |
| + } |
| } |
| return r.PostForm.Get(name) |
| } |
| +// multipartMemory bounds what a multipart mutation may hold in memory. |
| +// idear reads only short text fields; the limit exists so a body that |
| +// is not that costs nothing. |
| +const multipartMemory = 1 << 20 |
| + |
| // pathID is the {id} wildcard, as an int64. Ids come from the URL on |
| // every route that has one. |
| func pathID(r *http.Request) (int64, error) { |
diff --git a/handlers_test.go b/handlers_test.go| index 69f7ce2..b7e43d3 100644 |
| --- a/handlers_test.go |
| +++ b/handlers_test.go |
| @@ -4,6 +4,7 @@ import ( |
| "fmt" |
| "net/http" |
| "net/url" |
| + "reflect" |
| "strings" |
| "sync" |
| "testing" |
| @@ -199,6 +200,23 @@ func memberBySubject(t *testing.T, app *ideartest.App, subject string) *idear.Me |
| return m |
| } |
| +// standing asserts the two flags the invitation page carries about the |
| +// viewer. The harness renderer prints every field of InvitationPage by |
| +// reflection, so these read what the HANDLER decided rather than what |
| +// a template chose to show. |
| +func standing(t *testing.T, res *ideartest.Result, who string, signedIn, reconcile bool) { |
| + t.Helper() |
| + if res.Status != http.StatusOK { |
| + t.Fatalf("%s: GET the invitation → %d; body %q", who, res.Status, res.Body) |
| + } |
| + for field, want := range map[string]bool{"signedin": signedIn, "reconcile": reconcile} { |
| + line := fmt.Sprintf("%s %v", field, want) |
| + if !strings.Contains(res.Body, line) { |
| + t.Errorf("%s: the invitation page does not say %q; body %q", who, line, res.Body) |
| + } |
| + } |
| +} |
| + |
| // --------------------------------------------------------------- |
| // §7.1 — a non-member is refused read and write on every route. |
| // --------------------------------------------------------------- |
| @@ -338,12 +356,10 @@ func TestAdminCannotActOnAdminOrOwner(t *testing.T) { |
| // self-management is how an instance ends up with nobody able to |
| // administer it. |
| targets := map[string]*idear.Member{ |
| - "the owner": owner, |
| - "a peer admin": peer, |
| - "a deactivated admin": goneAdmin, |
| - "themselves": actor, |
| - "themselves, demotion": actor, |
| - "the owner, restoration": owner, |
| + "the owner": owner, |
| + "a peer admin": peer, |
| + "a deactivated admin": goneAdmin, |
| + "themselves": actor, |
| } |
| // The three target-shaped mutations, derived from the route table |
| // rather than listed: every management route that carries an {id}. |
| @@ -381,6 +397,21 @@ func TestAdminCannotActOnAdminOrOwner(t *testing.T) { |
| t.Errorf("the acting admin acted on themselves: now %s/%v", got.Role, got.Active()) |
| } |
| + // A member id that resolves to NOTHING is answered by the same |
| + // hook, with the same bytes, as a non-member's refusal — which is |
| + // what refuse()'s "same hook, same bytes" comment claims and |
| + // nothing else asserted. |
| + for _, rt := range byID { |
| + path := instantiate(rt.Pattern, "999999999", "0") |
| + res := c.Post(path, url.Values{"role": {string(idear.RoleMember)}}) |
| + if res.Status != http.StatusNotFound { |
| + t.Errorf("%s on a nonexistent member → %d, want 404; body %q", path, res.Status, res.Body) |
| + } |
| + if res.Body != ideartest.AppNotFound { |
| + t.Errorf("%s answered 404 with %q, want the app's own 404 page", path, res.Body) |
| + } |
| + } |
| + |
| // What the admin CAN do, so the test is not green because the |
| // admin can do nothing at all. |
| plain := app.H.Member(idear.RoleMember) |
| @@ -793,6 +824,14 @@ func TestOrphanIs404edEverywhereUntilReconciled(t *testing.T) { |
| // needs a valid token, which is the spec's corrected wording. |
| token := invite(t, app, col, oc, "orphan@example.test", idear.RoleAdmin) |
| + // The invitation page has to SAY they are the orphan, because |
| + // SignedIn and Reconcile are what a template keys the accept |
| + // control off. Stuck at false, item 9's healing path is |
| + // unreachable from any real UI while the redemption below still |
| + // answers 303 — so the flags are asserted, not assumed. |
| + standing(t, orphan.Get("/invitations/"+token), "the orphan", true, true) |
| + standing(t, app.Visitor().Get("/invitations/"+token), "a signed-out visitor", false, false) |
| + |
| res := orphan.Post("/invitations/"+token, url.Values{}) |
| if res.Status != http.StatusSeeOther { |
| t.Fatalf("reconciliation → %d, want 303; body %q", res.Status, res.Body) |
| @@ -817,6 +856,12 @@ func TestOrphanIs404edEverywhereUntilReconciled(t *testing.T) { |
| // And afterwards they have exactly the access their role earns — |
| // derived, so "exactly" means every route. |
| checkAccess(t, app, orphan, m, "a healed orphan") |
| + |
| + // A member is signed in and has nothing left to reconcile, so the |
| + // accept control must be gone. Their own spent token is no longer |
| + // a valid invitation, so this asks about a fresh one. |
| + fresh := invite(t, app, col, oc, "somebody.else@example.test", idear.RoleMember) |
| + standing(t, orphan.Get("/invitations/"+fresh), "a healed member", true, false) |
| } |
| func TestReconciliationClaimsAnUnclaimedInstance(t *testing.T) { |
| @@ -1113,6 +1158,81 @@ func TestInvitationPageDoesNotDiscloseTheAddress(t *testing.T) { |
| // The role selector, CSRF, and the wiring. |
| // --------------------------------------------------------------- |
| +// TestGrantableMatchesTheDesign pins WHAT the rule is, which no |
| +// amount of derivation can do for itself. |
| +// |
| +// TestGrantableIsTheAllowList below checks that the page, the store |
| +// and Grantable agree. They agree by construction — Grantable asks |
| +// checkInviteRole, the test asks Grantable — so relaxing the rule |
| +// moves the expectation with it: changing checkInviteRole's |
| +// `rank(role) >= rank(actor.Role)` to `>`, which lets an Admin mint a |
| +// PEER ADMIN and puts the new admin beyond every other admin's reach, |
| +// left every test in this file green. That is the brief's anti-pattern |
| +// in mirror form: total derivation buys drift-resistance and loses |
| +// rule-change detection. |
| +// |
| +// So this states the matrix from |
| +// docs/superpowers/specs/2026-08-23-idear-design.md §5 — "Admins |
| +// manage Members only", "the target's rank must be strictly below the |
| +// actor's", "ownership moves only by Transfer" — and compares it in |
| +// BOTH directions, exactly as TestRoutesMatchTheDesign does for the |
| +// route table. It quotes the spec, never the implementation. |
| +func TestGrantableMatchesTheDesign(t *testing.T) { |
| + design := map[idear.Role][]idear.Role{ |
| + // An Owner may grant Admin and Member. Not Owner: ownership |
| + // moves only by Transfer. |
| + idear.RoleOwner: {idear.RoleAdmin, idear.RoleMember}, |
| + // An Admin may grant Member ONLY. An Admin who could mint a |
| + // peer Admin has escalated — MayActOn refuses acting on an |
| + // equal rank, so the new Admin is beyond the granter's reach |
| + // and beyond every other Admin's too. |
| + idear.RoleAdmin: {idear.RoleMember}, |
| + // A Member manages nothing. |
| + idear.RoleMember: nil, |
| + } |
| + |
| + for actorRole, want := range design { |
| + actor := &idear.Member{ID: 1, Role: actorRole} |
| + got := idear.Grantable(actor) |
| + |
| + inGot := map[idear.Role]bool{} |
| + for _, role := range got { |
| + inGot[role] = true |
| + } |
| + inWant := map[idear.Role]bool{} |
| + for _, role := range want { |
| + inWant[role] = true |
| + } |
| + for _, role := range want { |
| + if !inGot[role] { |
| + t.Errorf("a %s may grant %s by the design, and Grantable does not offer it", actorRole, role) |
| + } |
| + } |
| + for _, role := range got { |
| + if !inWant[role] { |
| + t.Errorf("Grantable offers %s to a %s; the design says %v", role, actorRole, want) |
| + } |
| + } |
| + if len(got) != len(want) { |
| + t.Errorf("Grantable(%s) = %v, the design says %v", actorRole, got, want) |
| + } |
| + } |
| + |
| + // A deactivated actor grants nothing, whatever rank the row still |
| + // carries: removal is never a delete, so the row outlives the |
| + // privileges. |
| + past := time.Now().UTC() |
| + for actorRole := range design { |
| + gone := &idear.Member{ID: 1, Role: actorRole, DeactivatedAt: &past} |
| + if got := idear.Grantable(gone); len(got) != 0 { |
| + t.Errorf("a deactivated %s is offered %v", actorRole, got) |
| + } |
| + } |
| + if got := idear.Grantable(nil); len(got) != 0 { |
| + t.Errorf("Grantable(nil) = %v, want nothing", got) |
| + } |
| +} |
| + |
| // TestGrantableIsTheAllowList checks the role selector against the |
| // store, in both directions, for every actor. |
| // |
| @@ -1409,3 +1529,63 @@ func TestFieldsComeFromTheBodyNotTheQuery(t *testing.T) { |
| t.Fatalf("the target is %s; the query string completed a bodyless post", got.Role) |
| } |
| } |
| + |
| +// TestMultipartFormsAreRead covers the encoding an app reaches the |
| +// moment its members form grows a file input. ParseForm alone does not |
| +// populate PostForm for multipart/form-data, so every field would read |
| +// empty and every submit would 400 with nothing to explain it — a |
| +// fail-closed failure, and an undiagnosable one. |
| +func TestMultipartFormsAreRead(t *testing.T) { |
| + app, _ := newApp(t) |
| + target := app.H.Member(idear.RoleMember) |
| + c := app.As(app.H.Owner()) |
| + |
| + res := c.PostMultipart(fmt.Sprintf("/members/%d/role", target.ID), |
| + url.Values{"role": {string(idear.RoleAdmin)}}) |
| + if res.Status != http.StatusSeeOther { |
| + t.Fatalf("a multipart role change → %d, want 303; body %q", res.Status, res.Body) |
| + } |
| + if got := app.H.Reload(target.ID); got.Role != idear.RoleAdmin { |
| + t.Fatalf("the target is %s; the multipart body was not read", got.Role) |
| + } |
| + |
| + // And a multipart body with no role in it is still a 400, not a |
| + // path around the field check. |
| + if res := c.PostMultipart(fmt.Sprintf("/members/%d/role", target.ID), url.Values{}); res.Status != http.StatusBadRequest { |
| + t.Errorf("a multipart post with no role → %d, want 400", res.Status) |
| + } |
| +} |
| + |
| +// TestEveryHandlerIsMounted closes the hole under the whole suite: |
| +// Routes() is what every derived test walks, so an exported handler |
| +// added WITHOUT a Routes() entry is invisible to all of them — it |
| +// would ship with no membership gate and no test would notice. |
| +// |
| +// The count is the assertion because the names cannot be: a func value |
| +// in a Route carries no method name to compare against. Reflection |
| +// over *Handlers finds every exported method with a handler's |
| +// signature; there must be exactly as many as there are routes. |
| +func TestEveryHandlerIsMounted(t *testing.T) { |
| + app := ideartest.NewApp(t) |
| + |
| + handlerish := reflect.TypeOf(func(http.ResponseWriter, *http.Request) {}) |
| + typ := reflect.TypeOf(app.Handlers) |
| + handlers := 0 |
| + var names []string |
| + for i := range typ.NumMethod() { |
| + m := typ.Method(i) |
| + // A method's own type has the receiver first; compare what is |
| + // left against the handler signature. |
| + if m.Type.NumIn() == handlerish.NumIn()+1 && m.Type.NumOut() == 0 && |
| + m.Type.In(1) == handlerish.In(0) && m.Type.In(2) == handlerish.In(1) { |
| + handlers++ |
| + names = append(names, m.Name) |
| + } |
| + } |
| + if handlers == 0 { |
| + t.Fatal("reflection found no handler methods at all; the check is broken") |
| + } |
| + if got := len(app.Handlers.Routes()); got != handlers { |
| + t.Fatalf("*Handlers has %d handler methods (%v) but Routes() mounts %d; an unmounted handler is invisible to every derived test in this file", handlers, names, got) |
| + } |
| +} |
diff --git a/internal/ideartest/app.go b/internal/ideartest/app.go| index 3242f0c..d9c20cf 100644 |
| --- a/internal/ideartest/app.go |
| +++ b/internal/ideartest/app.go |
| @@ -1,8 +1,10 @@ |
| package ideartest |
| import ( |
| + "bytes" |
| "fmt" |
| "io" |
| + "mime/multipart" |
| "net/http" |
| "net/http/cookiejar" |
| "net/http/httptest" |
| @@ -299,6 +301,40 @@ func (c *Client) TryPost(path string, form url.Values) (*Result, error) { |
| return c.do(http.MethodPost, path, form, c.App.Origin) |
| } |
| +// PostMultipart submits the same fields as a multipart/form-data body |
| +// — the encoding a browser uses the moment a form grows a file input. |
| +func (c *Client) PostMultipart(path string, fields url.Values) *Result { |
| + c.App.T.Helper() |
| + var body bytes.Buffer |
| + w := multipart.NewWriter(&body) |
| + for name, values := range fields { |
| + for _, v := range values { |
| + if err := w.WriteField(name, v); err != nil { |
| + c.App.T.Fatalf("writing multipart field %q: %v", name, err) |
| + } |
| + } |
| + } |
| + if err := w.Close(); err != nil { |
| + c.App.T.Fatalf("closing the multipart body: %v", err) |
| + } |
| + req, err := http.NewRequest(http.MethodPost, c.App.Origin+path, &body) |
| + if err != nil { |
| + c.App.T.Fatalf("POST %s: %v", path, err) |
| + } |
| + req.Header.Set("Content-Type", w.FormDataContentType()) |
| + req.Header.Set("Origin", c.App.Origin) |
| + res, err := c.HTTP.Do(req) |
| + if err != nil { |
| + c.App.T.Fatalf("POST %s: %v", path, err) |
| + } |
| + defer res.Body.Close() |
| + b, err := io.ReadAll(res.Body) |
| + if err != nil { |
| + c.App.T.Fatalf("reading %s: %v", path, err) |
| + } |
| + return &Result{Status: res.StatusCode, Body: string(b), Location: res.Header.Get("Location"), Header: res.Header} |
| +} |
| + |
| // PostFrom is TryPost with a chosen Origin header: the cross-origin |
| // form submission a CSRF attack actually looks like. |
| func (c *Client) PostFrom(origin, path string, form url.Values) *Result { |
diff --git a/ratelimit.go b/ratelimit.go| index 024038f..8710388 100644 |
| --- a/ratelimit.go |
| +++ b/ratelimit.go |
| @@ -3,6 +3,7 @@ package idear |
| import ( |
| "net" |
| "net/http" |
| + "net/netip" |
| "sync" |
| "time" |
| ) |
| @@ -160,22 +161,60 @@ func (l *limiter) size() int { |
| return len(l.buckets) |
| } |
| -// clientIP is the default rate-limit key: the IP half of RemoteAddr, |
| -// so a client behind one address shares one budget across ports. |
| +// clientIP is the default rate-limit key: the client's NETWORK, not |
| +// its address. |
| // |
| // It is "per-IP-ish" and not per-person on purpose — there is nobody |
| -// to identify on an unauthenticated route. Behind a reverse proxy |
| -// EVERY request arrives from the proxy's address and they would share |
| -// a single bucket, which turns the limiter into a global one and locks |
| -// out real invitees; an app in that shape must set |
| -// HandlerConfig.ClientKey to read its own trusted forwarding header. |
| -// idear does not read X-Forwarded-For itself, because a header idear |
| -// cannot verify is a header an attacker can spoof to mint unlimited |
| -// budgets. |
| +// to identify on an unauthenticated route. |
| +// |
| +// THE FOLD TO /64 IS THE WHOLE POINT, and keying on the bare address |
| +// was a defect rather than a simplification. Every IPv6 host is handed |
| +// a /64: that is 2^64 source addresses, free to rotate, one per |
| +// request. Keyed on the address, each one is an unseen client with a |
| +// fresh burst, so the per-client limit does not exist at all — and it |
| +// is worse than a limit that merely leaks. Max distinct keys fill the |
| +// table, at which point allow FAILS CLOSED and refuses every UNSEEN |
| +// client: real invitees, and orphans, whose only healing path is |
| +// POST /invitations/{token}. One machine can hold an instance in that |
| +// state indefinitely. Failing closed is still the right direction on |
| +// a guessing-slowdown; the key was the bug. Folding to the /64 gives |
| +// that machine one bucket, which is what it should always have had. |
| +// |
| +// IPv4 is used whole (a /32), which is the same rule read the same |
| +// way: the smallest unit an operator is routinely handed. Folding |
| +// IPv4 further would put a whole CGNAT or campus behind one bucket. |
| +// IPv4-mapped v6 addresses (::ffff:203.0.113.9) are unmapped first, |
| +// so one client cannot hold two budgets by switching representation. |
| +// |
| +// Behind a reverse proxy EVERY request arrives from the proxy's |
| +// address and they would share a single bucket, which turns the |
| +// limiter into a global one and locks out real invitees; an app in |
| +// that shape must set HandlerConfig.ClientKey to read its own TRUSTED |
| +// forwarding header. idear never reads X-Forwarded-For (or any other |
| +// forwarding header) itself, because a header idear cannot verify is |
| +// a header an attacker can spoof to mint unlimited budgets — the |
| +// exact failure the /64 fold exists to prevent, handed over for free. |
| +// TestClientIPIgnoresForwardingHeaders pins that. |
| func clientIP(r *http.Request) string { |
| host, _, err := net.SplitHostPort(r.RemoteAddr) |
| if err != nil { |
| - return r.RemoteAddr |
| + // No port: a unix socket, or a test. Use it whole rather than |
| + // dropping it, so it still keys to something. |
| + host = r.RemoteAddr |
| + } |
| + addr, err := netip.ParseAddr(host) |
| + if err != nil { |
| + return host |
| + } |
| + // The zone ("fe80::1%eth0") is the local interface, not the |
| + // client, and would split one client's budget in two. |
| + addr = addr.WithZone("") |
| + if addr.Is4() || addr.Is4In6() { |
| + return addr.Unmap().String() |
| + } |
| + prefix, err := addr.Prefix(64) |
| + if err != nil { |
| + return addr.String() |
| } |
| - return host |
| + return prefix.String() |
| } |
diff --git a/ratelimit_internal_test.go b/ratelimit_internal_test.go| index 71c4872..2f9eb86 100644 |
| --- a/ratelimit_internal_test.go |
| +++ b/ratelimit_internal_test.go |
| @@ -112,22 +112,112 @@ func TestLimiterIsConcurrencySafe(t *testing.T) { |
| } |
| } |
| -func TestClientIPKeysByAddressNotPort(t *testing.T) { |
| - r := httptest.NewRequest("GET", "/invitations/x", nil) |
| - r.RemoteAddr = "203.0.113.9:51234" |
| - if got := clientIP(r); got != "203.0.113.9" { |
| +func TestClientIPKeysByNetworkNotAddress(t *testing.T) { |
| + key := func(remote string) string { |
| + r := httptest.NewRequest("GET", "/invitations/x", nil) |
| + r.RemoteAddr = remote |
| + return clientIP(r) |
| + } |
| + |
| + // IPv4: the address, without the port. A browser making a second |
| + // request from a new source port must share the first one's |
| + // budget, or the limit is no limit at all. |
| + if got := key("203.0.113.9:51234"); got != "203.0.113.9" { |
| t.Errorf("clientIP = %q, want the address without the port", got) |
| } |
| - // A browser making a second request from a new source port must |
| - // share the first one's budget, or the limit is no limit at all. |
| - r.RemoteAddr = "203.0.113.9:51235" |
| - if got := clientIP(r); got != "203.0.113.9" { |
| + if got := key("203.0.113.9:51235"); got != "203.0.113.9" { |
| t.Errorf("clientIP = %q for a second port, want the same key", got) |
| } |
| + // ...and a DIFFERENT IPv4 host is a different client. Folding v4 |
| + // further would put a whole CGNAT behind one bucket. |
| + if key("203.0.113.9:1") == key("203.0.113.10:1") { |
| + t.Error("two IPv4 hosts share a key; the fold is too coarse") |
| + } |
| + |
| + // IPv6: the /64, because every host is handed one and can rotate |
| + // addresses inside it for free. Three addresses in one /64 are |
| + // one client. |
| + sixtyFour := key("[2001:db8:1:2::5]:443") |
| + if sixtyFour != "2001:db8:1:2::/64" { |
| + t.Errorf("clientIP = %q, want the /64 prefix", sixtyFour) |
| + } |
| + for _, other := range []string{"[2001:db8:1:2:ffff::9]:443", "[2001:db8:1:2:dead:beef:cafe:1]:80"} { |
| + if got := key(other); got != sixtyFour { |
| + t.Errorf("clientIP(%s) = %q, want the same /64 key %q", other, got, sixtyFour) |
| + } |
| + } |
| + // A different /64 is a different client, so a shared limit does |
| + // not fall out of the fold either. |
| + if got := key("[2001:db8:1:3::5]:443"); got == sixtyFour { |
| + t.Errorf("a neighbouring /64 shares the key %q", got) |
| + } |
| + // An IPv4-mapped address is unmapped first: one client must not |
| + // hold two budgets by switching representation. |
| + if got := key("[::ffff:203.0.113.9]:80"); got != "203.0.113.9" { |
| + t.Errorf("clientIP = %q for an IPv4-mapped address, want %q", got, "203.0.113.9") |
| + } |
| + // The zone is the local interface, not the client. |
| + if key("[fe80::1%eth0]:80") != key("[fe80::1%eth1]:80") { |
| + t.Error("the interface zone splits one client's budget in two") |
| + } |
| // An address with no port at all (a unix socket, a test) is used |
| // whole rather than dropped, so it still keys to something. |
| - r.RemoteAddr = "@" |
| - if got := clientIP(r); got != "@" { |
| + if got := key("@"); got != "@" { |
| t.Errorf("clientIP = %q, want the raw RemoteAddr when it has no port", got) |
| } |
| } |
| + |
| +// TestClientIPIgnoresForwardingHeaders pins the claim clientIP's doc |
| +// comment makes outright: a header idear cannot verify is a header an |
| +// attacker can spoof to mint unlimited budgets, so idear never reads |
| +// one. Without this test, teaching clientIP to prefer X-Forwarded-For |
| +// left the whole suite green while handing every prober an unlimited |
| +// supply of fresh buckets. |
| +func TestClientIPIgnoresForwardingHeaders(t *testing.T) { |
| + r := httptest.NewRequest("GET", "/invitations/x", nil) |
| + r.RemoteAddr = "203.0.113.9:51234" |
| + for _, header := range []string{"X-Forwarded-For", "X-Real-IP", "Forwarded", "CF-Connecting-IP", "True-Client-IP"} { |
| + r.Header.Set(header, "1.2.3.4") |
| + } |
| + if got := clientIP(r); got != "203.0.113.9" { |
| + t.Errorf("clientIP = %q with forwarding headers set, want the RemoteAddr host %q", got, "203.0.113.9") |
| + } |
| +} |
| + |
| +// TestIPv6RotationCannotLockOutOtherClients is the scenario the /64 |
| +// fold exists for, run end to end against the limiter. |
| +// |
| +// One machine rotates source addresses inside its own /64 — free, on |
| +// any IPv6 host — and spends far more than the table can hold. Keyed |
| +// on the bare address that fills the table, and allow then fails |
| +// closed for every UNSEEN client: invitees, and orphans, whose only |
| +// healing path is POST /invitations/{token}. Failing closed is right; |
| +// the key was the bug. |
| +func TestIPv6RotationCannotLockOutOtherClients(t *testing.T) { |
| + const max = 8 |
| + l := newLimiter(RateLimit{Burst: 2, Every: time.Hour, Max: max}) |
| + |
| + key := func(remote string) string { |
| + r := httptest.NewRequest("POST", "/invitations/x", nil) |
| + r.RemoteAddr = remote |
| + return clientIP(r) |
| + } |
| + |
| + for i := range max * 20 { |
| + l.allow(key(fmt.Sprintf("[2001:db8:1:2::%x]:443", i))) |
| + } |
| + if got := l.size(); got != 1 { |
| + t.Fatalf("one attacker's /64 filled %d buckets, want 1", got) |
| + } |
| + // The attacker is throttled on the budget their whole /64 shares. |
| + if l.allow(key("[2001:db8:1:2::ffff]:443")) { |
| + t.Error("an address rotation inside one /64 bought a fresh burst") |
| + } |
| + // And everybody else is still served — including the orphan whose |
| + // only way back into the instance is the route being defended. |
| + for _, invitee := range []string{"198.51.100.7:1", "[2001:db8:9:9::1]:443", "[2001:db8:aa::5]:443"} { |
| + if !l.allow(key(invitee)) { |
| + t.Errorf("a real client (%s) was locked out by one machine's address rotation", invitee) |
| + } |
| + } |
| +} |