amadan
🤖 fix: the sign-in guard read the request differently from what it guarded
An adversarial mutation pass by a reviewer who did not write gate 27 found the gate green over TWO LIVE BYPASSES in the guard shipped four commits ago. Not mutations — the actual code. POST /auth/begin?a;b=1 with an ordinary body -> reached the classifier address only in the query string, empty body -> reached the classifier The first is the worse one and it is one character. The guard called r.ParseForm() and passed the request through on error. url.ParseQuery keeps every well-formed pair AND returns the first error, ParseForm reports a query-string error even when the body parsed perfectly, and rastrillo's FormValue ignores the error and uses what parsed. So a semicolon in the URL skipped the admission check entirely and the instance dialled the attacker's domain. Confirmed: DNS 192.168.1.1:53 asked for _keymail.attacker.example. The second is the PostFormValue/FormValue seam: the guard read the body, rastrillo reads body-or-query. Both are one mistake. A guard that reads a request differently from the thing it guards is not guarding it. The guard now reads the address with the same accessor rastrillo uses and has no error branch, because FormValue has no error to branch on — the two calls cannot disagree. Three more green mutations closed with it. SigninPath was a second literal that could drift from the guard's constant, giving a clean oracle by Location; they share the constant now. Dropping csrf.Protect from MountAuth — plausible, since Begin re-checks origin itself — made cross-origin answer 403 for an owner and 303 for a stranger, readable across sites through an opaque redirect; the guard now hands cross-origin straight to rastrillo, which refuses it uniformly before dialling. And the nil-predicate fail-closed branch was untested prose; it is asserted. ONE RESIDUAL IS NOT CLOSABLE FROM HERE, and the guard's comment claimed the opposite. It sits in front of signin's per-address rate limiter, so six submissions at an owner's address yield ?err=rate and six at a stranger's yield ?sent=1 every time — an owners-list oracle at six requests per candidate, with enumeration unthrottled because refused addresses consume no per-IP budget. The budget lives inside signin's flow. Closing it means duplicating a constant we do not own, or fixing the ordering upstream, which is the same fix the SSRF needs. Every new subtest was confirmed red against the old guard before being kept.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H3VKtXnq6fSmM2F11Bjcsy
4 files changed,
+202
−26
docs/superpowers/specs/2026-09-03-memoria-design.md+20 −3internal/memoria/app.go+1 −1internal/memoria/auth.go+60 −12internal/memoriatest/signindial_test.go+121 −10
diff --git a/docs/superpowers/specs/2026-09-03-memoria-design.md b/docs/superpowers/specs/2026-09-03-memoria-design.md| index 67c8586..66df086 100644 |
| --- a/docs/superpowers/specs/2026-09-03-memoria-design.md |
| +++ b/docs/superpowers/specs/2026-09-03-memoria-design.md |
| @@ -2139,9 +2139,26 @@ renders on its own origin. The home never sees `q`. |
| exact `?sent=1` destination, because a distinct rejection would make |
| the guard an owners-list oracle; the owner case asserts the dial does |
| happen, without which a guard that refuses everybody, an unwired auth, |
| - a missing route and an uninstalled recorder are all green. Five |
| - mutations, each verified red — author-chosen, which AGENTS.md rates as |
| - the weak kind, so it is going to a reviewer for its own. |
| + a missing route and an uninstalled recorder are all green. |
| + **The adversarial pass found the gate green over two live bypasses**, |
| + not mutations but the shipped guard: `POST /auth/begin?a;b=1` with an |
| + ordinary body reached the classifier, because the guard called |
| + `ParseForm` and passed the request through on error while rastrillo's |
| + `FormValue` ignores that error and uses what parsed; and an address |
| + given only in the query string was invisible to the guard's |
| + `PostFormValue` and visible to rastrillo's `FormValue`. **One mistake |
| + twice: a guard that reads a request differently from the thing it |
| + guards is not guarding it.** The guard now reads the address with the |
| + same accessor and has no error branch, because `FormValue` has no |
| + error to branch on. Three further green mutations closed with it — |
| + `SigninPath` drifting from the guard's constant (now shared), dropping |
| + `csrf.Protect` (cross-origin now answers identically for both), and a |
| + nil predicate falling open (asserted). |
| + **One residual stays open and §3.2 records it:** the guard sits in |
| + front of signin's per-address rate limiter, so an owner and a stranger |
| + are distinguishable after five submissions, and a refused address |
| + consumes no per-IP budget either. The budget is inside signin and not |
| + reachable from here; it closes with the upstream fix. |
| ## 11. Sequencing |
diff --git a/internal/memoria/app.go b/internal/memoria/app.go| index f14dd70..52c6f6c 100644 |
| --- a/internal/memoria/app.go |
| +++ b/internal/memoria/app.go |
| @@ -78,7 +78,7 @@ func (a *app) MountAuth(mux *http.ServeMux, origin string) { |
| r.Use(csrf.Protect(origin)) |
| // Guarded, not bare: the classifier inside Begin dials the submitted |
| // address's domain before Authorize runs. beginGuard says why. |
| - r.Post("/auth/begin", a.beginGuard(a.auth.Begin)) |
| + r.Post("/auth/begin", a.beginGuard(origin, a.auth.Begin)) |
| r.Get("/auth/callback", a.auth.Callback) |
| r.Get("/auth/verify", a.auth.Verify) |
| r.Post("/signout", a.auth.Signout) |
diff --git a/internal/memoria/auth.go b/internal/memoria/auth.go| index 1225506..bdd4a4a 100644 |
| --- a/internal/memoria/auth.go |
| +++ b/internal/memoria/auth.go |
| @@ -43,13 +43,35 @@ const signinPath = "/signin" |
| // since the dial only confirms the instance exists while an oracle |
| // confirms who owns it. |
| // |
| -// Residual, stated rather than left to be discovered: an owner whose |
| -// domain delegates to keymail is redirected to keymail's host rather than |
| -// to ?sent=1, so for keymail-delegated domains the two paths still |
| -// differ. That is strictly narrower than the status quo, where the dial |
| -// happens for every address at every domain, and it closes when the |
| -// upstream fix lands. |
| -func (a *app) beginGuard(next http.HandlerFunc) http.HandlerFunc { |
| +// THREE RESIDUALS, and the second one means the sentence above is an aim |
| +// rather than an achievement. Stated here because an earlier version of |
| +// this comment claimed the guard was not an oracle, and it is one. |
| +// |
| +// 1. An owner whose domain delegates to keymail is redirected to |
| +// keymail's host rather than to ?sent=1, so those two paths differ. |
| +// |
| +// 2. **Rate limiting distinguishes them after five submissions.** This |
| +// guard sits in front of rastrillo, so a refused address never |
| +// reaches signin's per-address budget while an admitted one does: |
| +// six tries at an owner's address yields ?err=rate, six at a |
| +// stranger's yields ?sent=1 every time. Enumeration is also |
| +// unthrottled, because a refused address consumes no per-IP budget |
| +// either. The budget lives inside signin's flow and is not reachable |
| +// from here, so closing this means either duplicating upstream's |
| +// window and count — coupling to a constant we do not own — or |
| +// fixing the ordering upstream, which is the same fix the SSRF needs. |
| +// Found by an adversarial mutation pass, 2026-09-05. |
| +// |
| +// 3. A `force=1` submission skips the classifier, so no dial happens — |
| +// but rastrillo still mails a magic link to whatever address was |
| +// given. The guard refuses those before Begin, so memoria is covered; |
| +// it is listed because "no dial" and "no outbound anything" are not |
| +// the same claim. |
| +// |
| +// All three are narrower than the status quo, where the dial happens for |
| +// every address at every domain, and all three close when the upstream |
| +// fix lands. |
| +func (a *app) beginGuard(origin string, next http.HandlerFunc) http.HandlerFunc { |
| return func(w http.ResponseWriter, r *http.Request) { |
| // Fail closed. A missing predicate is a wiring bug, and the |
| // version of this that defaults to allowing is the version that |
| @@ -59,13 +81,39 @@ func (a *app) beginGuard(next http.HandlerFunc) http.HandlerFunc { |
| http.Error(w, "sign-in is not configured", http.StatusInternalServerError) |
| return |
| } |
| - if err := r.ParseForm(); err != nil { |
| - // Let rastrillo produce its own error for a malformed body |
| - // rather than inventing a second shape for one. |
| + |
| + // Cross-origin goes straight to rastrillo, which refuses it |
| + // uniformly with a 403 before it dials anything. Deciding it here |
| + // would answer differently for an owner and a stranger — a |
| + // cross-site page can read that difference through an opaque |
| + // redirect — and this guard exists to remove distinguishable |
| + // outcomes, not to add one on another axis. |
| + if o := r.Header.Get("Origin"); o != "" && o != origin { |
| next(w, r) |
| return |
| } |
| - if !a.admits(r.PostFormValue("address")) { |
| + |
| + // Read the address EXACTLY as rastrillo does. This is the whole |
| + // correctness argument of the guard, and the first version got it |
| + // wrong twice in one line. |
| + // |
| + // It called r.ParseForm() and passed the request through on error. |
| + // url.ParseQuery keeps every well-formed pair AND returns the |
| + // first error, and ParseForm reports a query-string error even |
| + // when the body parsed perfectly — while rastrillo's FormValue |
| + // ignores the error and uses what parsed. So `POST /auth/begin?a;b=1` |
| + // with an ordinary body skipped the check entirely and dialled. |
| + // One character. |
| + // |
| + // It also read PostFormValue where rastrillo reads FormValue, so |
| + // an address supplied only in the query string was invisible here |
| + // and visible there. |
| + // |
| + // Both are the same mistake: a guard that reads a request |
| + // differently from the thing it guards is not guarding it. There |
| + // is no error branch now, because FormValue has no error to |
| + // branch on — the two calls cannot disagree. |
| + if !a.admits(r.FormValue("address")) { |
| // No address in the log line, ever: §3.2, gate 4. |
| a.logger.Info("sign-in refused before classifier dial") |
| http.Redirect(w, r, signinPath+"?sent=1", http.StatusSeeOther) |
| @@ -132,7 +180,7 @@ func NewAuth(sqlDB *sql.DB, env AuthEnv, logger *slog.Logger, authorize func(str |
| InstanceKey: env.InstanceKey, |
| Mailer: env.Mailer, |
| Authorize: authorize, |
| - SigninPath: "/signin", |
| + SigninPath: signinPath, |
| SignedInPath: "/", |
| Subject: "Your memoria sign-in link", |
| Logger: logger, |
diff --git a/internal/memoriatest/signindial_test.go b/internal/memoriatest/signindial_test.go| index ea96cfa..807667c 100644 |
| --- a/internal/memoriatest/signindial_test.go |
| +++ b/internal/memoriatest/signindial_test.go |
| @@ -72,6 +72,16 @@ const dialTestOwner = "owner@example.test" |
| // real auth, real owners list — and points the whole process's HTTP |
| // transport at a recorder. |
| func signinApp(t *testing.T) (http.Handler, *dialRecorder) { |
| + t.Helper() |
| + return signinAppWithPredicate(t, memoria.Admits([]string{dialTestOwner})) |
| +} |
| + |
| +// signinAppWithPredicate is signinApp with the admission predicate under |
| +// the test's control, so the fail-closed branch can be reached. NewAuth |
| +// still gets a real predicate — it refuses to construct without one — and |
| +// only the guard's copy varies, which is exactly the wiring mistake the |
| +// nil case is about. |
| +func signinAppWithPredicate(t *testing.T, admits func(string) bool) (http.Handler, *dialRecorder) { |
| t.Helper() |
| logger := slog.New(slog.NewTextHandler(io.Discard, nil)) |
| d, err := db.Open(filepath.Join(t.TempDir(), "app.db"), logger) |
| @@ -94,7 +104,7 @@ func signinApp(t *testing.T) (http.Handler, *dialRecorder) { |
| if err != nil { |
| t.Fatalf("NewAuth: %v", err) |
| } |
| - a.UseAuth(au, memoria.Admits(owners)) |
| + a.UseAuth(au, admits) |
| a.MountAuth(mux, testOrigin) |
| rec := &dialRecorder{} |
| @@ -147,7 +157,10 @@ func beginSignin(t *testing.T, h http.Handler, address string) *httptest.Respons |
| // runs for an admitted address, and the guard discriminates rather than |
| // blocks. |
| // |
| -// MUTATION-TESTED 2026-09-05, five, each verified red and reverted: |
| +// MUTATION-TESTED 2026-09-05, in two passes, and the second is why this |
| +// file is worth reading. |
| +// |
| +// The author's five, each verified red: |
| // |
| // M1 register a.auth.Begin bare, no guard -> non-owner case red |
| // M2 guard admits everyone -> non-owner case red |
| @@ -155,15 +168,33 @@ func beginSignin(t *testing.T, h http.Handler, address string) *httptest.Respons |
| // M4 guard returns 403 instead of ?sent=1 -> non-owner case red |
| // M5 neither recorder installed -> owner case red |
| // |
| -// M4 is the oracle case and the reason the non-owner half asserts a |
| -// Location and not merely a status. M5 is the check on the gate itself: |
| -// with no recorder there is nothing to observe, and only the owner half |
| -// notices — a one-sided version of this test would have passed M3 and M5 |
| -// both, while measuring nothing at all. |
| +// Then an adversarial pass by someone who did not write the gate, per |
| +// AGENTS.md. It found the five above were the cases already defended |
| +// against, and that **the gate was green over two live bypasses in the |
| +// shipped guard** — not mutations, the actual code: |
| +// |
| +// - `POST /auth/begin?a;b=1` with an ordinary body reached the |
| +// classifier. The guard called ParseForm and passed the request |
| +// through on error; ParseForm reports a query-string error even when |
| +// the body parsed fine, and rastrillo's FormValue ignores it and uses |
| +// what parsed. One character defeated the check. |
| +// - An address supplied only in the query string was invisible to the |
| +// guard's PostFormValue and visible to rastrillo's FormValue. |
| +// |
| +// Both are one mistake — a guard reading the request differently from the |
| +// thing it guards — and the subtests below are the four payloads that |
| +// proved it, each confirmed to go red against the old code with the |
| +// resolver being asked for the attacker's domain. |
| // |
| -// These five were chosen by the author, which AGENTS.md says is the weak |
| -// kind: they are the cases already defended against while writing it. The |
| -// gate has been handed to a reviewer to pick its own. |
| +// The same pass found three more green mutations, now closed: SigninPath |
| +// drifting from the guard's own constant (they share it), dropping |
| +// csrf.Protect from MountAuth (cross-origin now answers identically), and |
| +// a nil predicate falling open (asserted). |
| +// |
| +// STILL OPEN, and stated rather than quietly carried: rate limiting makes |
| +// owner and non-owner distinguishable after five submissions, because the |
| +// guard sits in front of signin's per-address budget. See beginGuard's |
| +// residual 2. Not closable from here. |
| func TestSigninDoesNotDialForANonOwner(t *testing.T) { |
| t.Run("a non-owner never reaches the classifier", func(t *testing.T) { |
| h, rec := signinApp(t) |
| @@ -195,4 +226,84 @@ func TestSigninDoesNotDialForANonOwner(t *testing.T) { |
| "half of this gate is measuring nothing") |
| } |
| }) |
| + |
| + // A guard that reads the request differently from the thing it guards |
| + // is not guarding it. Every case below defeated the first version, |
| + // which called ParseForm and passed the request through on error — |
| + // and ParseForm reports a query-string error even when the body is |
| + // fine, while rastrillo's FormValue ignores it and uses what parsed. |
| + // |
| + // Found by an adversarial mutation pass, 2026-09-05. These are not |
| + // mutations: they were live bypasses of shipped code. |
| + t.Run("a malformed request does not skip the check", func(t *testing.T) { |
| + const stranger = "stranger@attacker.example" |
| + cases := []struct { |
| + name, target, body, contentType string |
| + }{ |
| + {"semicolon in the query string", "/auth/begin?a;b=1", |
| + "address=" + stranger, "application/x-www-form-urlencoded"}, |
| + {"semicolon in the body", "/auth/begin", |
| + "x;y=1&address=" + stranger, "application/x-www-form-urlencoded"}, |
| + {"bad percent-escape in the body", "/auth/begin", |
| + "x=%zz&address=" + stranger, "application/x-www-form-urlencoded"}, |
| + {"malformed Content-Type, address in the query", |
| + "/auth/begin?address=" + url.QueryEscape(stranger), |
| + "", "application/x-www-form-urlencoded; charset"}, |
| + {"address only in the query string, empty body", |
| + "/auth/begin?address=" + url.QueryEscape(stranger), |
| + "", "application/x-www-form-urlencoded"}, |
| + } |
| + for _, c := range cases { |
| + t.Run(c.name, func(t *testing.T) { |
| + h, rec := signinApp(t) |
| + req := httptest.NewRequest(http.MethodPost, c.target, strings.NewReader(c.body)) |
| + req.Header.Set("Content-Type", c.contentType) |
| + req.Header.Set("Origin", testOrigin) |
| + h.ServeHTTP(httptest.NewRecorder(), req) |
| + if rec.dialed() { |
| + t.Errorf("an unadmitted address reached the classifier via %s: dialed %s", |
| + c.name, rec.where()) |
| + } |
| + }) |
| + } |
| + }) |
| + |
| + // A cross-site page can read the difference between an opaque redirect |
| + // and an opaque response, so owner and stranger must be identical |
| + // cross-origin too. Without this, removing csrf.Protect from MountAuth |
| + // — plausible, since Begin re-checks the origin itself — turns the |
| + // guard into an oracle: 403 for an owner, 303 for a stranger. |
| + t.Run("cross-origin answers the same for both", func(t *testing.T) { |
| + submit := func(address string) (int, string) { |
| + h, _ := signinApp(t) |
| + form := url.Values{"address": {address}} |
| + req := httptest.NewRequest(http.MethodPost, "/auth/begin", strings.NewReader(form.Encode())) |
| + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") |
| + req.Header.Set("Origin", "https://evil.example") |
| + rr := httptest.NewRecorder() |
| + h.ServeHTTP(rr, req) |
| + return rr.Code, rr.Header().Get("Location") |
| + } |
| + ownerCode, ownerLoc := submit(dialTestOwner) |
| + strangerCode, strangerLoc := submit("stranger@attacker.example") |
| + if ownerCode != strangerCode || ownerLoc != strangerLoc { |
| + t.Errorf("cross-origin is distinguishable: owner %d %q, stranger %d %q", |
| + ownerCode, ownerLoc, strangerCode, strangerLoc) |
| + } |
| + }) |
| + |
| + // The fail-closed branch was untested prose until this existed: the |
| + // gate always installed a predicate, so making the nil case fall |
| + // through to rastrillo stayed green while dialing for everyone. |
| + t.Run("a nil predicate refuses rather than falling open", func(t *testing.T) { |
| + h, rec := signinAppWithPredicate(t, nil) |
| + rr := beginSignin(t, h, "stranger@attacker.example") |
| + if rec.dialed() { |
| + t.Errorf("a nil admission predicate dialed %s", rec.where()) |
| + } |
| + if rr.Code != http.StatusInternalServerError { |
| + t.Errorf("status = %d, want %d: a missing predicate is a wiring "+ |
| + "bug and must say so", rr.Code, http.StatusInternalServerError) |
| + } |
| + }) |
| } |