amadan
edge: read a login POST's body only once the host is known to be ours
Review finding on the round-1 fix. The form was parsed BEFORE lookupUpstreamPath, so it ran for hosts this box does not serve. When the lookup missed, e.backhaul reverse-proxied the same request to the box that homes it — with the body already drained and ContentLength untouched, so the home box saw an empty body and the hop 502'd with "ContentLength=41 with Body length 0". The ordinary lookup and its !ok branch are now byte-for-byte what they were. Only once the lookup has hit does a POST to /.carlos/gate parse its form and re-resolve by next; nothing reads the body before the host is known to be ours. The re-resolution stays ahead of the meter so every step below, billing included, sees one settled up. A body that will not parse is now answered 413 at the edge and never reaches Admit, so it spends no limiter token and no bcrypt comparison — where the round-1 fallback left PostForm non-nil-empty, Admit's own ParseForm then returned nil and the junk fell through to the limiter and the compare. Refusing junk must not cost a real visitor their attempts. Tests: a login POST for an unknown host leaves its body unread (a strings.Reader whose Len is unchanged after ServeHTTP) and still gets the unchanged unknown-host answer; an over-4 KiB login POST is 413 and the ten wrong-password attempts that follow from the same address are all 401, never 429, which is what proves no token was spent. Also drains the response body before Drain in the meter test — the login page is over 2 KiB, so headers flush before the deferred Record runs.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
3 files changed,
+106
−26
internal/edge/edge.go+41 −25internal/edge/gate_test.go+64 −0internal/gate/gate_test.go+1 −1
diff --git a/internal/edge/edge.go b/internal/edge/edge.go| index 4a213f7bb..94d010222 100644 |
| --- a/internal/edge/edge.go |
| +++ b/internal/edge/edge.go |
| @@ -1200,31 +1200,7 @@ func (e *Edge) ServeHTTP(w http.ResponseWriter, r *http.Request) { |
| // nothing else. |
| r, hopped := e.acceptHop(r, host) |
| - // A login POST resolves its upstream by the path in `next`, not by its |
| - // own (spec §4.2). On a hostname that routes by path prefix, |
| - // lookupUpstreamPath keys on the first segment; "/.carlos/gate" has no |
| - // segment that can ever be a prefix, so a login POST would fall through |
| - // to the FRONT host's route — the password would be judged by the wrong |
| - // row (or proxied to an app that never asked for it, which the |
| - // ungated-route branch below then refuses), and the cookie would be |
| - // signed for the wrong host. `next` is the path the visitor was refused |
| - // on, so resolving by it lands on the route that put up the page. |
| - // |
| - // Sanitised with gate.SafeNext before it is used, so the path routed by |
| - // and the path redirected to are the same string. A form that will not |
| - // parse (a truncated body, a bad encoding) falls back to today's |
| - // behaviour; Admit then answers it. On a host with no prefix map this |
| - // resolves to exactly the route r.URL.Path would have. |
| - loginPost := r.Method == http.MethodPost && r.URL.Path == gate.LoginPath |
| - lookupPath := r.URL.Path |
| - if loginPost { |
| - r.Body = http.MaxBytesReader(w, r.Body, gate.MaxFormBody) |
| - if err := r.ParseForm(); err == nil { |
| - lookupPath = gate.SafeNext(r.PostForm.Get("next")) |
| - } |
| - } |
| - |
| - up, ok := e.lookupUpstreamPath(host, lookupPath) |
| + up, ok := e.lookupUpstreamPath(host, r.URL.Path) |
| if !ok { |
| if hopped { |
| // A PROVEN hop from a sibling edge, for a host that has moved (or |
| @@ -1248,6 +1224,46 @@ func (e *Edge) ServeHTTP(w http.ResponseWriter, r *http.Request) { |
| return |
| } |
| + // A login POST re-resolves its upstream by the path in `next`, not by |
| + // its own (spec §4.2). On a hostname that routes by path prefix, |
| + // lookupUpstreamPath keys on the first segment; "/.carlos/gate" has no |
| + // segment that can ever be a prefix, so a login POST falls through to |
| + // the FRONT host's route — the password would be judged by the wrong |
| + // row (or proxied to an app that never asked for it, which the |
| + // ungated-route branch below then refuses), and the cookie would be |
| + // signed for the wrong host. `next` is the path the visitor was refused |
| + // on, so resolving by it lands on the route that put up the page. On a |
| + // host with no prefix map it resolves to the same route either way. |
| + // |
| + // Placed AFTER the lookup above and its !ok branch, which stay |
| + // byte-for-byte what they were, because this reads the request BODY: a |
| + // host this box does not serve may be backhauled to the box that homes |
| + // it (e.backhaul, above), and that reverse-proxies this very request — |
| + // draining its body here would send the home box a ContentLength it |
| + // cannot fill, and the hop would 502. Nothing is read until the host is |
| + // known to be ours. Placed before the meter so every step below, |
| + // billing included, sees one settled up. |
| + // |
| + // A body that will not parse — over gate.MaxFormBody, or malformed — is |
| + // answered 413 here and never reaches Admit, so it spends no limiter |
| + // token and no bcrypt comparison. Refusing junk must not cost a real |
| + // visitor their attempts. |
| + loginPost := r.Method == http.MethodPost && r.URL.Path == gate.LoginPath |
| + if loginPost { |
| + r.Body = http.MaxBytesReader(w, r.Body, gate.MaxFormBody) |
| + if err := r.ParseForm(); err != nil { |
| + http.Error(w, http.StatusText(http.StatusRequestEntityTooLarge), http.StatusRequestEntityTooLarge) |
| + return |
| + } |
| + // A miss cannot happen — SafeNext yields a path on this host, and |
| + // the worst case "/" resolves to the host's own route, the one |
| + // already in up — but keeping the first up is the honest answer if |
| + // the table changed underneath. |
| + if target, hit := e.lookupUpstreamPath(host, gate.SafeNext(r.PostForm.Get("next"))); hit { |
| + up = target |
| + } |
| + } |
| + |
| // Meter every route this box serves except the platform's own status |
| // host (not billable) — wrapping w here, once, before the upgrade/cache |
| // branching below, is what makes all three dispatch shapes (hibernating, |
diff --git a/internal/edge/gate_test.go b/internal/edge/gate_test.go| index 59b83cd06..82c723e36 100644 |
| --- a/internal/edge/gate_test.go |
| +++ b/internal/edge/gate_test.go |
| @@ -317,6 +317,10 @@ func TestGatedRefusalIsMeteredToTheRoute(t *testing.T) { |
| if resp.StatusCode != http.StatusUnauthorized { |
| t.Fatalf("status = %d, want 401", resp.StatusCode) |
| } |
| + // Drain only once the whole body is in hand: the login page is over |
| + // 2 KiB, so the headers reach the client before the handler returns and |
| + // the meter's deferred Record has run. |
| + io.ReadAll(resp.Body) |
| row, ok := m.Drain()["gated.example.com"] |
| if !ok { |
| t.Fatal("no usage row for the gated route — the refusal was not metered") |
| @@ -329,6 +333,66 @@ func TestGatedRefusalIsMeteredToTheRoute(t *testing.T) { |
| } |
| } |
| +// TestLoginPostForAnUnknownHostLeavesTheBodyUnread is the backhaul pin. A |
| +// host this box does not serve may be homed on a sibling edge, and |
| +// e.backhaul reverse-proxies THIS request there — so nothing on the way to |
| +// that branch may read the body. Parsing the login form before the route |
| +// lookup drained it while leaving ContentLength alone, and the hop 502'd |
| +// with "ContentLength=41 with Body length 0". The form is parsed only once |
| +// the host is known to be ours. |
| +func TestLoginPostForAnUnknownHostLeavesTheBodyUnread(t *testing.T) { |
| + e, _, _, _ := gatedEdge(t, &fakeActivator{}, false) |
| + form := url.Values{"password": {"correct horse"}, "next": {"/docs"}}.Encode() |
| + body := strings.NewReader(form) |
| + req := httptest.NewRequest(http.MethodPost, gate.LoginPath, body) |
| + req.Host = "not.served.here.example" |
| + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") |
| + rec := httptest.NewRecorder() |
| + e.ServeHTTP(rec, req) |
| + if body.Len() != len(form) { |
| + t.Fatalf("%d of %d body bytes were consumed before the route lookup — a backhauled hop would 502", |
| + len(form)-body.Len(), len(form)) |
| + } |
| + if rec.Code != http.StatusNotFound { |
| + t.Fatalf("unknown host = %d, want the unchanged unknown-host answer (404)", rec.Code) |
| + } |
| +} |
| + |
| +// TestOversizedLoginPostIs413AndSpendsNoAttempt: a body over |
| +// gate.MaxFormBody is refused at the edge, before Admit is ever called, so |
| +// it costs neither a bcrypt comparison nor a limiter token. The proof that |
| +// no token was spent is the ten wrong-password attempts that follow from |
| +// the same address: the bucket holds exactly ten, so if the junk body had |
| +// taken one, the last would be 429 instead of 401. |
| +func TestOversizedLoginPostIs413AndSpendsNoAttempt(t *testing.T) { |
| + _, srv, _, seen := gatedEdge(t, &fakeActivator{}, false) |
| + c := &http.Client{CheckRedirect: func(*http.Request, []*http.Request) error { return http.ErrUseLastResponse }} |
| + post := func(t *testing.T, form string) int { |
| + t.Helper() |
| + req, _ := http.NewRequest(http.MethodPost, srv.URL+gate.LoginPath, strings.NewReader(form)) |
| + req.Host = "gated.example.com" |
| + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") |
| + resp, err := c.Do(req) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + defer resp.Body.Close() |
| + io.Copy(io.Discard, resp.Body) |
| + return resp.StatusCode |
| + } |
| + if code := post(t, "password="+strings.Repeat("a", 64<<10)); code != http.StatusRequestEntityTooLarge { |
| + t.Fatalf("oversized login POST = %d, want 413", code) |
| + } |
| + if *seen != "" { |
| + t.Fatal("the oversized body reached the upstream") |
| + } |
| + for i := range 10 { |
| + if code := post(t, url.Values{"password": {"wrong horse"}}.Encode()); code != http.StatusUnauthorized { |
| + t.Fatalf("wrong-password attempt %d = %d, want 401 — the 413 spent a token", i+1, code) |
| + } |
| + } |
| +} |
| + |
| func TestGatedRouteCacheIsNeverConsultedBeforeTheGate(t *testing.T) { |
| e, srv, _, _ := gatedEdge(t, &fakeActivator{}, false) |
| e.EnableCache(NewCache(DefaultCacheMaxEntries, DefaultCacheMaxBodyBytes)) |
diff --git a/internal/gate/gate_test.go b/internal/gate/gate_test.go| index 76e8e5fd7..8c78c3551 100644 |
| --- a/internal/gate/gate_test.go |
| +++ b/internal/gate/gate_test.go |
| @@ -261,7 +261,7 @@ func TestStripCookieDropsTheHeaderWhenTheGateCookieIsTheOnlyOne(t *testing.T) { |
| } |
| } |
| -// A body over maxFormBody fails to parse; that must cost no bcrypt and no |
| +// A body over MaxFormBody fails to parse; that must cost no bcrypt and no |
| // limiter token. The bucket is keyed on the sender's own address, so a junk |
| // flood only ever starves the flooder — except behind the backhaul fold, |
| // where a sibling edge's address stands in for every visitor through it, |