amadan

oficina / docs Public

A failed check is not a refusal, and sign-in fails closed

The third review approved the previous commit and named one RISK I have
fixed rather than deferred, because it is the same data-loss class this
whole branch has been about.

**A failed admission check answered 403.** save-queue.mjs classifies a 4xx
as "the bytes are the problem": it drops the batch permanently, and since
Yjs updates depend on their predecessors, everything after it is poisoned
too. So a two-second database stall cost a guest their paragraph. It is
503 now, which the queue retries.

That required telling "no" apart from "could not tell", so HasGrant
reports its error instead of folding it into false and allows returns
(bool, error). The two callers then choose differently, and the difference
is the point: sign-in fails CLOSED, because there is nothing to lose by
refusing and gateBegin's non-oracle contract survives; a live request
answers retryable, because there is a person's writing in the balance.

**Fixing it found a hole nothing covered.** Mutating admits() to return
true on an error — failing OPEN at the front door, admitting any address
during a database failure — survived the entire suite. Nothing anywhere
pinned the direction. TestAdmitsFailsClosed does now.

**Two more of my own tests were wrong.** TestBreakDatabase broke nothing:
closing that pool left HasGrant answering normally, and rastrillo's
session layer refused first, so the test asserted on somebody else's 403
and never reached the middleware it was named for. Deleted rather than
shipped, and the case moved to the middleware itself through a small
identity seam, since rauth's context key is unexported. My first
replacement mutation for it then failed to compile, which proves nothing
and which I caught only by reading the output rather than the exit code.

That is the fourth round in which a test of mine passed for a reason other
than the one it named.

Also from the review: a status check in the guest create-control loop (it
passed on an unrendered page), == 403 rather than "4xx" in the revoked
writer test, the menu structure test now runs the guest's shorter File
menu as well as the member's, and a duplicated doc comment removed.

Full suite green, gofmt clean, vet clean. Mutations: the 503 folded back
to 403, allows() consulting grants before the configured set, and admits()
failing open are each caught.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RVcEQ8NgrPCpRTb6JtBz1Q
Paul Campbell pushed by paul@keymail.dev 26f01ddf16503bb7194be3ac104a4e0742941e05 parent 7cdba63
9 files changed, +269 −48
  • internal/docs/admission_test.go +145 −0
  • internal/docs/app.go +7 −4
  • internal/docs/auth.go +67 −10
  • internal/docs/auth_allowlist_test.go +5 −5
  • internal/docs/grants.go +14 −6
  • internal/docs/grants_admission_test.go +12 −12
  • internal/docs/guest_test.go +10 −3
  • internal/docs/menu.go +0 −4
  • internal/docs/menu_test.go +9 −4

CI passed — run details

Intent

full ledger: sharing

diff --git a/internal/docs/admission_test.go b/internal/docs/admission_test.go
new file mode 100644
index 0000000..3e04e29
--- /dev/null
+++ b/internal/docs/admission_test.go
@@ -0,0 +1,145 @@
+package docs
+
+import (
+ "errors"
+ "net/http"
+ "net/http/httptest"
+ "testing"
+)
+
+// requireAdmission has to tell three answers apart: admitted, refused, and
+// "could not tell". The third is the one that costs somebody their work if
+// it is folded into the second.
+//
+// A guest's editor posts through fetch. save-queue.mjs classifies a 4xx as
+// "the bytes are the problem" and drops the batch permanently — and
+// because Yjs updates depend on their predecessors, everything after it is
+// poisoned too. A 5xx it retries. So a database stall answered 403 costs a
+// guest their paragraph, while the same stall answered 503 costs them
+// nothing. (Re-review 3, finding 1.)
+//
+// Tested at the middleware rather than over HTTP: the app's own session
+// layer refuses first when the database is genuinely broken, so an
+// end-to-end version would assert on rastrillo's 403 and never reach this
+// code at all. That is exactly the "passes for a reason other than the one
+// it names" failure this branch has produced four times, and the honest
+// fix is to test the unit whose contract this is.
+func TestRequireAdmissionTellsRefusalFromFailure(t *testing.T) {
+ reached := false
+ next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ reached = true
+ w.WriteHeader(http.StatusNoContent)
+ })
+
+ cases := []struct {
+ name string
+ granted func(string) (bool, error)
+ method string
+ want int
+ passes bool
+ }{
+ {"a check that fails is retryable, not a refusal", func(string) (bool, error) {
+ return false, errors.New("database is stalled")
+ }, http.MethodPost, http.StatusServiceUnavailable, false},
+
+ {"a failed check on a GET is also retryable", func(string) (bool, error) {
+ return false, errors.New("database is stalled")
+ }, http.MethodGet, http.StatusServiceUnavailable, false},
+
+ {"a definite refusal of a write is 403", func(string) (bool, error) {
+ return false, nil
+ }, http.MethodPost, http.StatusForbidden, false},
+
+ {"a definite refusal of a read is a redirect to sign in", func(string) (bool, error) {
+ return false, nil
+ }, http.MethodGet, http.StatusSeeOther, false},
+
+ {"a guest who still holds a grant passes through", func(string) (bool, error) {
+ return true, nil
+ }, http.MethodPost, http.StatusNoContent, true},
+ }
+
+ for _, c := range cases {
+ t.Run(c.name, func(t *testing.T) {
+ reached = false
+ list := allowlist{
+ addrs: map[string]struct{}{"alice@example.com": {}},
+ granted: c.granted,
+ }
+ h := requireAdmissionOf(list, "/signin", signedInAs("bob@example.com"))(next)
+
+ req := httptest.NewRequest(c.method, "/d/abc/updates", nil)
+ rec := httptest.NewRecorder()
+ h.ServeHTTP(rec, req)
+
+ if rec.Code != c.want {
+ t.Errorf("status %d, want %d", rec.Code, c.want)
+ }
+ if reached != c.passes {
+ t.Errorf("reached the handler = %v, want %v", reached, c.passes)
+ }
+ })
+ }
+}
+
+// A configured member never touches the grant check, so a broken database
+// cannot eject them. Without this the 503 above could be the answer
+// everybody gets.
+func TestAMemberIsUnaffectedByAFailedGrantCheck(t *testing.T) {
+ called := false
+ list := allowlist{
+ addrs: map[string]struct{}{"alice@example.com": {}},
+ granted: func(string) (bool, error) {
+ called = true
+ return false, errors.New("database is stalled")
+ },
+ }
+
+ reached := false
+ h := requireAdmissionOf(list, "/signin", signedInAs("alice@example.com"))(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ reached = true
+ }))
+
+ h.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest(http.MethodPost, "/documents", nil))
+
+ if !reached {
+ t.Error("a member was stopped by a grant check that is not about them")
+ }
+ if called {
+ t.Error("a member's request consulted the grant store; allows() must answer from the configured set first")
+ }
+}
+
+// signedInAs is the identity lookup requireAdmissionOf takes, standing in
+// for rauth.From, whose context key is unexported.
+func signedInAs(address string) func(*http.Request) (string, bool) {
+ return func(*http.Request) (string, bool) { return address, true }
+}
+
+// admits — the plain-predicate form rauth's Authorize hook takes — fails
+// CLOSED. It cannot distinguish "no" from "could not tell", so it has to
+// choose, and at sign-in the safe choice is refusal: there is no unsaved
+// work to lose and a genuinely admitted person can try again.
+//
+// This is the opposite choice from requireAdmission's 503 above, made for
+// the opposite reason, and neither is right in the other's place. It has
+// its own test because a mutation that made admits() return true on an
+// error survived the whole suite: nothing anywhere pinned the direction,
+// so a database failure would have opened the front door to any address.
+func TestAdmitsFailsClosed(t *testing.T) {
+ list := allowlist{
+ addrs: map[string]struct{}{"alice@example.com": {}},
+ granted: func(string) (bool, error) {
+ return false, errors.New("database is stalled")
+ },
+ }
+
+ if list.admits("mallory@example.com") {
+ t.Error("a failed grant check admitted a stranger; sign-in must fail closed")
+ }
+ // A member is still admitted: the failure must not lock everybody out
+ // either, and allows() answers them without asking the database.
+ if !list.admits("alice@example.com") {
+ t.Error("a configured member was refused because of a grant check that is not about them")
+ }
+}
diff --git a/internal/docs/app.go b/internal/docs/app.go
index 664648a..5308573 100644
--- a/internal/docs/app.go
+++ b/internal/docs/app.go
@@ -390,12 +390,15 @@ func New(cfg Config) (*App, error) {
// visiting, not moving in.
store.seedOnlyMembers = list.configured
- list.granted = func(address string) bool {
+ list.granted = func(address string) (bool, error) {
// A short deadline rather than context.Background(): this runs on
// the sign-in path and now on every request (requireAdmission), so
- // a stalled query must fail rather than hold a request open. A
- // failure answers false, which refuses admission — the safe
- // direction, and the configured allowlist still admits members.
+ // a stalled query must fail rather than hold a request open.
+ //
+ // The error is passed up rather than folded into false. Each
+ // caller wants a different answer to "the check failed": sign-in
+ // refuses, and a live request answers 503 so the editor's save
+ // queue retries instead of dropping the batch.
ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
defer cancel()
return store.HasGrant(ctx, address)
diff --git a/internal/docs/auth.go b/internal/docs/auth.go
index d3c0de1..01118e5 100644
--- a/internal/docs/auth.go
+++ b/internal/docs/auth.go
@@ -180,7 +180,7 @@ type allowlist struct {
// Deliberately one clause in allows(). Sharing widening who may sign in
// is a real decision about this instance's front door, and reversing it
// should be deleting two lines rather than unpicking a design.
- granted func(address string) bool
+ granted func(address string) (bool, error)
}
// configured reports whether address is a MEMBER of this instance — on
@@ -209,20 +209,40 @@ func (a allowlist) configured(address string) bool {
return ok
}
-// allows reports whether address may sign in.
-func (a allowlist) allows(address string) bool {
+// allows reports whether address may sign in, and whether the question
+// could be answered at all.
+//
+// The error is separate from the answer because the two demand different
+// responses on a live request: a definite "no" ejects, an unanswerable
+// question must not. See requireAdmission.
+func (a allowlist) allows(address string) (bool, error) {
if a.addrs == nil {
- return true
+ return true, nil
}
normalized := normalizeAddress(address)
if _, ok := a.addrs[normalized]; ok {
- return true
+ return true, nil
}
// A grant admits its holder, and nobody else — as a guest, not as a
// member: see configured() above for the powers that stay behind the
// configured list, and requireAdmission for why this is now re-asked on
// every request rather than only at sign-in.
- return a.granted != nil && a.granted(normalized)
+ if a.granted == nil {
+ return false, nil
+ }
+ return a.granted(normalized)
+}
+
+// admits is allows for callers that take a plain predicate — rauth's own
+// Authorize hook, which has no way to say "could not tell".
+//
+// A failed check refuses, which is the right direction at sign-in: there
+// is no unsaved work to lose, and a person who is genuinely admitted can
+// try again. requireAdmission is the caller that must NOT do this, and it
+// uses allows directly for exactly that reason.
+func (a allowlist) admits(address string) bool {
+ ok, err := a.allows(address)
+ return err == nil && ok
}
// resolveAllowlist applies Config.AllowedAddresses' own fail-closed
@@ -334,7 +354,13 @@ func gateBegin(next http.HandlerFunc, list allowlist, signinPath string) http.Ha
// malformed request, just reached here instead, before any
// mail could be sent over it.
_ = r.ParseForm()
- if list.allows(r.FormValue("address")) {
+ // A failed check refuses here, unlike requireAdmission's 503. At
+ // sign-in there is nothing to lose by refusing — no unsaved work,
+ // and the person can try again — and gateBegin's whole contract is
+ // that a refusal is indistinguishable from a success, which a 503
+ // would break by telling a stranger the check happened.
+ admitted, err := list.allows(r.FormValue("address"))
+ if err == nil && admitted {
if r.Form == nil {
r.Form = url.Values{}
}
@@ -365,10 +391,41 @@ func gateBegin(next http.HandlerFunc, list allowlist, signinPath string) http.Ha
// with no identity it does nothing and lets RequireSession's own redirect
// stand.
func requireAdmission(list allowlist, signinPath string) func(http.Handler) http.Handler {
+ return requireAdmissionOf(list, signinPath, func(r *http.Request) (string, bool) {
+ id, ok := rauth.From(r)
+ return id.Address, ok
+ })
+}
+
+// requireAdmissionOf is requireAdmission with the identity lookup passed
+// in, which is the only way this middleware can be tested at all: rauth
+// stores the identity under an unexported context key, so a test cannot
+// build a request that carries one.
+//
+// The seam is here rather than the test reaching through HTTP because an
+// end-to-end version of the "database is down" case never reaches this
+// code — rastrillo's own session layer refuses first, and the test would
+// have asserted on its 403 while proving nothing about this middleware.
+// That is the failure mode this branch has now produced four times.
+func requireAdmissionOf(list allowlist, signinPath string, identity func(*http.Request) (string, bool)) func(http.Handler) http.Handler {
return func(next http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
- id, ok := rauth.From(r)
- if !ok || list.allows(id.Address) {
+ address, ok := identity(r)
+ if !ok {
+ next.ServeHTTP(w, r)
+ return
+ }
+ admitted, err := list.allows(address)
+ if err != nil {
+ // The question could not be answered, which is not the
+ // same as "no". 503 rather than 403: save-queue.mjs
+ // retries a 5xx and permanently drops a 4xx, so folding a
+ // database stall into a refusal would cost a guest their
+ // paragraph. Retryable, and honest.
+ http.Error(w, rastrillo.T(r, "docs.error.generic"), http.StatusServiceUnavailable)
+ return
+ }
+ if admitted {
next.ServeHTTP(w, r)
return
}
@@ -417,7 +474,7 @@ func newAuthPlugin(cfg Config, writer *sql.DB, instanceKey string, mailer mail.S
Origin: cfg.Origin,
InstanceKey: instanceKey,
Mailer: mailer,
- Authorize: list.allows,
+ Authorize: list.admits,
Logger: logger,
})
}
diff --git a/internal/docs/auth_allowlist_test.go b/internal/docs/auth_allowlist_test.go
index 215e535..ddb20ef 100644
--- a/internal/docs/auth_allowlist_test.go
+++ b/internal/docs/auth_allowlist_test.go
@@ -216,12 +216,12 @@ func TestResolveAllowlistParsesCommaSeparatedList(t *testing.T) {
t.Fatalf("resolveAllowlist: %v", err)
}
for _, addr := range []string{"alice@example.com", "ALICE@EXAMPLE.COM", "bob@example.com", "carol@example.com"} {
- if !list.allows(addr) {
- t.Errorf("list.allows(%q) = false, want true", addr)
+ if !list.admits(addr) {
+ t.Errorf("list.admits(%q) = false, want true", addr)
}
}
- if list.allows("mallory@example.com") {
- t.Error("list.allows(\"mallory@example.com\") = true, want false — not on the configured list")
+ if list.admits("mallory@example.com") {
+ t.Error("list.admits(\"mallory@example.com\") = true, want false — not on the configured list")
}
}
@@ -253,7 +253,7 @@ func TestResolveAllowlistEmptyInDevelopmentAdmitsEveryone(t *testing.T) {
if err != nil {
t.Fatalf("resolveAllowlist: %v", err)
}
- if !list.allows("anyone@example.com") {
+ if !list.admits("anyone@example.com") {
t.Error("an empty AllowedAddresses in development must admit everyone")
}
}
diff --git a/internal/docs/grants.go b/internal/docs/grants.go
index bebba5a..5574a77 100644
--- a/internal/docs/grants.go
+++ b/internal/docs/grants.go
@@ -205,12 +205,20 @@ func (s *Store) Grant(ctx context.Context, owner int64, sqid, address string, ro
// failure shape. It answers on the address alone, never on which document,
// because admission is not a per-document question.
//
-// A database error answers false: refusing admission on an error is the safe
-// direction, and the address still gets in if it is on the configured list.
-func (s *Store) HasGrant(ctx context.Context, address string) bool {
+// A database error is REPORTED rather than folded into false, and that
+// distinction costs a guest their work if it is lost.
+//
+// requireAdmission runs on every request. Answering false on a failed
+// check ejects a guest mid-session, and their editor's next POST gets a
+// 403 — which save-queue.mjs reads as "the bytes are the problem",
+// dropping the batch permanently and, because Yjs updates depend on their
+// predecessors, poisoning everything after it. One two-second database
+// stall would cost a guest their paragraph. So the caller gets the error
+// and can answer 503, which the queue retries. (Re-review 3, finding 1.)
+func (s *Store) HasGrant(ctx context.Context, address string) (bool, error) {
subject := subjectOf(address)
if subject == "" {
- return false
+ return false, nil
}
// Scoped to grants on documents that still exist. A grant on a
// soft-deleted or orphaned (owner_id = 0) document opens nothing —
@@ -225,9 +233,9 @@ func (s *Store) HasGrant(ctx context.Context, address string) bool {
subject).
Count(&count).Error; err != nil {
s.logger.Error("docs: checking grants for admission", "err", err)
- return false
+ return false, err
}
- return count > 0
+ return count > 0, nil
}
// ListGrants returns who currently reaches a document, oldest first, for its
diff --git a/internal/docs/grants_admission_test.go b/internal/docs/grants_admission_test.go
index 5f12126..c39bac2 100644
--- a/internal/docs/grants_admission_test.go
+++ b/internal/docs/grants_admission_test.go
@@ -22,17 +22,17 @@ func TestAGrantAdmitsAnAddressTheAllowlistDoesNot(t *testing.T) {
"alice@example.com": {},
}}
- if list.allows("bob@example.com") {
+ if list.admits("bob@example.com") {
t.Fatal("bob is admitted before any grant exists; this test proves nothing")
}
granted := map[string]bool{"bob@example.com": true}
- list.granted = func(address string) bool { return granted[address] }
+ list.granted = func(address string) (bool, error) { return granted[address], nil }
- if !list.allows("bob@example.com") {
+ if !list.admits("bob@example.com") {
t.Error("bob holds a grant and is still refused admission — his invitation can never be redeemed")
}
- if list.allows("carol@example.com") {
+ if list.admits("carol@example.com") {
t.Error("carol holds no grant and was admitted; a grant admits its holder, not everybody")
}
}
@@ -40,9 +40,9 @@ func TestAGrantAdmitsAnAddressTheAllowlistDoesNot(t *testing.T) {
// The address is normalised on the way in, the same way the grant stored it.
func TestGrantedAdmissionNormalisesTheAddress(t *testing.T) {
list := allowlist{addrs: map[string]struct{}{}}
- list.granted = func(address string) bool { return address == "bob@example.com" }
+ list.granted = func(address string) (bool, error) { return address == "bob@example.com", nil }
- if !list.allows(" Bob@Example.COM ") {
+ if !list.admits(" Bob@Example.COM ") {
t.Error("a grant made to bob did not admit the same address typed differently")
}
}
@@ -51,9 +51,9 @@ func TestGrantedAdmissionNormalisesTheAddress(t *testing.T) {
// widening, never a narrowing.
func TestGrantsDoNotNarrowAnOpenAllowlist(t *testing.T) {
list := allowlist{} // addrs nil: development's admit-everyone
- list.granted = func(string) bool { return false }
+ list.granted = func(string) (bool, error) { return false, nil }
- if !list.allows("anybody@example.com") {
+ if !list.admits("anybody@example.com") {
t.Error("an open allowlist stopped admitting everybody once grants were wired in")
}
}
@@ -73,13 +73,13 @@ func TestHasGrantAnswersForARealGrant(t *testing.T) {
t.Fatalf("Create: %v", err)
}
- if store.HasGrant(ctx, "bob@example.com") {
- t.Fatal("bob holds a grant before one was made")
+ if held, err := store.HasGrant(ctx, "bob@example.com"); err != nil || held {
+ t.Fatalf("bob holds a grant before one was made (held=%v err=%v)", held, err)
}
if err := store.Grant(ctx, alice, doc.Sqid, "bob@example.com", RoleReader); err != nil {
t.Fatalf("Grant: %v", err)
}
- if !store.HasGrant(ctx, "bob@example.com") {
- t.Error("bob holds a grant and HasGrant says otherwise")
+ if held, err := store.HasGrant(ctx, "bob@example.com"); err != nil || !held {
+ t.Errorf("bob holds a grant and HasGrant says otherwise (held=%v err=%v)", held, err)
}
}
diff --git a/internal/docs/guest_test.go b/internal/docs/guest_test.go
index 2c04687..7524cb3 100644
--- a/internal/docs/guest_test.go
+++ b/internal/docs/guest_test.go
@@ -82,7 +82,14 @@ func TestAGuestIsNotShownCreateControls(t *testing.T) {
bob := signInCookies(t, app, "bob@example.com")
for _, page := range []string{"/", "/d/" + sqid} {
- if body := get(t, app, bob, page).Body.String(); strings.Contains(body, `action="/documents"`) {
+ rec := get(t, app, bob, page)
+ // The status first: an absent control and an unrendered page look
+ // identical to a substring check, so without this the test passes
+ // on a 404 — which is the opposite of what it claims to show.
+ if rec.Code != http.StatusOK {
+ t.Fatalf("the guest's %s answered %d, want 200", page, rec.Code)
+ }
+ if strings.Contains(rec.Body.String(), `action="/documents"`) {
t.Errorf("the guest's %s offers a create control they are refused", page)
}
}
@@ -181,8 +188,8 @@ func TestARevokedGuestsWritesAreRefusedNotRedirected(t *testing.T) {
t.Errorf("a revoked guest's write was redirected (%d → %q); fetch follows it and the save reports ok",
rec.Code, rec.Header().Get("Location"))
}
- if rec.Code < 400 {
- t.Errorf("a revoked guest's write answered %d; want a 4xx", rec.Code)
+ if rec.Code != http.StatusForbidden {
+ t.Errorf("a revoked guest's write answered %d; want 403", rec.Code)
}
// GETs still redirect — that is what a redirect is for, and a person
diff --git a/internal/docs/menu.go b/internal/docs/menu.go
index 2c39144..3779653 100644
--- a/internal/docs/menu.go
+++ b/internal/docs/menu.go
@@ -160,10 +160,6 @@ func viewMode(r *http.Request) string {
return ViewPaged
}
-// canCreate gates the File menu's "New document" form. A guest submitting
-// it gets a 403 and a bare text page — a control that exists only to fail
-// is the same defect as the editor a reader could type into, in a smaller
-// place. (Re-review, finding 3.)
// fileItems is the File menu, whose first entry is the one thing in this
// bar a guest may not do.
//
diff --git a/internal/docs/menu_test.go b/internal/docs/menu_test.go
index 7e5444f..577e6d4 100644
--- a/internal/docs/menu_test.go
+++ b/internal/docs/menu_test.go
@@ -14,10 +14,15 @@ import (
// clicked.
func newMenuViewForTest() menuView {
- // canCreate true: this helper backs the menu-structure tests, which
- // are about a member's full menu. The guest's shorter File menu is
- // covered by TestAGuestIsNotShownCreateControls.
- return newMenuView(httptest.NewRequest("GET", "/d/abc123", nil), "abc123", true)
+ return newMenuViewFor(true)
+}
+
+// newMenuViewFor builds either variant. The guest's File menu is one item
+// shorter, and the structure rules below (no leading divider, every form
+// carries an action) have to hold for it too — a shorter list is exactly
+// where a leading divider would appear.
+func newMenuViewFor(canCreate bool) menuView {
+ return newMenuView(httptest.NewRequest("GET", "/d/abc123", nil), "abc123", canCreate)
}
// Every command the menu names is implemented by one of the two modules