amadan
The two mutations that survived, and a test claim that was false
The fourth review approved the branch and, approving it, caught a false claim in the previous commit message: it said the menu structure test now runs the guest's File menu. newMenuViewFor(false) was never called. That is the fifth time on this branch a test has claimed something it did not do, and the first time the claim lived in a commit message rather than a comment. Two mutations also survived the whole suite, both on the authorization path, and neither is cosmetic. **gateBegin flipped to fail-open passed everything.** Under it a database stall turns POST /signin into an open relay: every address a stranger types reaches Begin and is mailed a magic link. Config.AllowedAddresses' own comment calls gateBegin "the wiring that actually matters" for exactly this. admits() would still refuse the session at callback, but by then we have sent mail to an address we chose to accept. **HasGrant returning (false, nil) on a query failure passed everything.** The 503 chain added in the last commit was tested from the stub downward and never from the store up — so the fix for the data-loss path had no test holding up its foundation. Both are now pinned, and both new tests were checked against the mutation they exist for rather than assumed to work. A correction to the last commit message while I am here. It said the 503 fixes the guest's dropped-batch path; that is broader than what holds. rastrillo's sessions.From folds a lookup error into "no session", so a real database outage still 403s the POST in the session layer before this middleware runs, and the batch still drops. What this branch fixes is the narrower window where the session row resolves and the grants query fails or exceeds its 2s cap. Worth having, not the whole thing, and the rest is upstream. Full suite green, gofmt clean, vet clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RVcEQ8NgrPCpRTb6JtBz1Q
2 files changed,
+88
−1
internal/docs/admission_test.go+72 −0internal/docs/menu_test.go+16 −1
CI passed — run details
Intent
diff --git a/internal/docs/admission_test.go b/internal/docs/admission_test.go| index 3e04e29..2c11100 100644 |
| --- a/internal/docs/admission_test.go |
| +++ b/internal/docs/admission_test.go |
| @@ -1,9 +1,12 @@ |
| package docs |
| import ( |
| + "context" |
| "errors" |
| "net/http" |
| "net/http/httptest" |
| + "net/url" |
| + "strings" |
| "testing" |
| ) |
| @@ -143,3 +146,72 @@ func TestAdmitsFailsClosed(t *testing.T) { |
| t.Error("a configured member was refused because of a grant check that is not about them") |
| } |
| } |
| + |
| +// gateBegin fails closed too, and nothing pinned it. |
| +// |
| +// A mutation flipping its condition to `err != nil || admitted` passed the |
| +// whole suite. Under it, a database stall turns POST /signin into an open |
| +// relay: every address a stranger types reaches Begin and gets a magic |
| +// link mailed. Config.AllowedAddresses' own comment calls gateBegin "the |
| +// wiring that actually matters" for exactly that, and admits() would still |
| +// refuse the session at callback — but the mail is already out, sent by us, |
| +// to an address we chose to accept. (Re-review 4, finding 1.) |
| +func TestGateBeginFailsClosed(t *testing.T) { |
| + list := allowlist{ |
| + addrs: map[string]struct{}{"alice@example.com": {}}, |
| + granted: func(string) (bool, error) { |
| + return false, errors.New("database is stalled") |
| + }, |
| + } |
| + |
| + for _, c := range []struct { |
| + name string |
| + address string |
| + want bool |
| + }{ |
| + {"a stranger during a failed check is refused", "mallory@example.com", false}, |
| + {"a member is unaffected, the check never runs for them", "alice@example.com", true}, |
| + } { |
| + t.Run(c.name, func(t *testing.T) { |
| + reached := false |
| + h := gateBegin(func(w http.ResponseWriter, r *http.Request) { |
| + reached = true |
| + }, list, "/signin") |
| + |
| + req := httptest.NewRequest(http.MethodPost, "/signin", |
| + strings.NewReader(url.Values{"address": {c.address}}.Encode())) |
| + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") |
| + h(httptest.NewRecorder(), req) |
| + |
| + if reached != c.want { |
| + t.Errorf("reached Begin = %v, want %v — a mail %s sent", reached, c.want, |
| + map[bool]string{true: "was not", false: "was"}[reached]) |
| + } |
| + }) |
| + } |
| +} |
| + |
| +// HasGrant reports its error rather than folding it into false, which is |
| +// the whole reason the 503 above can exist. A mutation returning |
| +// (false, nil) on a query failure passed the suite: the chain was tested |
| +// from the stub downward and never from the store up. (Re-review 4, |
| +// finding 2.) |
| +func TestHasGrantReportsItsError(t *testing.T) { |
| + app, err := NewForTest(t) |
| + if err != nil { |
| + t.Fatalf("NewForTest: %v", err) |
| + } |
| + |
| + // A cancelled context is the cheapest real failure: the query cannot |
| + // run, and the honest answer is "could not tell", never "no". |
| + ctx, cancel := context.WithCancel(context.Background()) |
| + cancel() |
| + |
| + held, err := app.Store().HasGrant(ctx, "bob@example.com") |
| + if err == nil { |
| + t.Error("a failed grant check reported no error; the caller cannot tell a refusal from an outage") |
| + } |
| + if held { |
| + t.Error("a failed grant check answered true") |
| + } |
| +} |
diff --git a/internal/docs/menu_test.go b/internal/docs/menu_test.go| index 577e6d4..5b726fa 100644 |
| --- a/internal/docs/menu_test.go |
| +++ b/internal/docs/menu_test.go |
| @@ -97,7 +97,22 @@ func TestMenuCommandsExistInTheBundle(t *testing.T) { |
| // malformed in a manner the Go compiler is happy with and a person is |
| // not. |
| func TestMenuStructureIsWellFormed(t *testing.T) { |
| - v := newMenuViewForTest() |
| + // Both variants. The guest's File menu is one item shorter, and a |
| + // shorter list is exactly where a leading divider appears — so the |
| + // rules below have to hold for it, not only for a member's. |
| + // |
| + // The previous commit added newMenuViewFor and its comment saying so, |
| + // and then never called it with false. The claim sat in a commit |
| + // message for one round. (Re-review 4, finding 3.) |
| + for _, canCreate := range []bool{true, false} { |
| + t.Run(map[bool]string{true: "member", false: "guest"}[canCreate], func(t *testing.T) { |
| + assertMenuWellFormed(t, newMenuViewFor(canCreate)) |
| + }) |
| + } |
| +} |
| + |
| +func assertMenuWellFormed(t *testing.T, v menuView) { |
| + t.Helper() |
| if len(v.Menus) == 0 { |
| t.Fatal("the menu bar has no menus") |