oficina / docs Public

The re-review's blockers: a redirect is not an answer to "store this"

The re-review refused c8415ce and was right twice over. The worst defect
on this branch was one I introduced while fixing the previous worst one.

**requireAdmission redirected every method.** fetch() follows redirects by
default, so the editor's POST /updates became a GET /signin, came back 200
with HTML, and save-queue.mjs read res.ok and reported the batch saved.
Alice revokes Bob mid-sentence and Bob types for an hour into a page that
tells him everything is fine — no failure indicator, ever.

That is the same failure class as the reader-editor bug this branch was
already fixing (a person told their writing is safe when it is not), made
worse: that one at least lit "save failed". rastrillo's own refuseSession
draws the line where I did not — redirect GET/HEAD, 403 everything else —
and now so does this. A redirect answers "show me a page"; it is never an
answer to "store this".

**And the previous commit's mutation claim was false.** The reviewer
deleted the member guard from sharePost and my test still passed: Bob does
not own Alice's document, so the owner check 404s him anyway, and the test
only asserted "not 303". Three of the four guarded routes had never been
exercised at all. The assertions are now 403 exactly, and the mutations
re-run: removing the guard from sharePost, shareRevoke or shareLink each
fail now, as does restoring the every-method redirect and restoring the
create controls. Five for five, verified rather than asserted.

That is the second time on this branch that "the mutations are caught" was
a claim I made without the evidence supporting it as widely as I said.

Also: guests were shown a "New document" form in the list header and in
the File menu, both of which could only ever answer them 403 — the same
defect as the reader's editor in a smaller place, so both are absent for a
guest rather than present and refused. Cache-Control: no-store on the
render that prints the one-shot URL, since "shown once" was only a
server-side truth while the browser's disk cache and bfcache kept it. The
self-grant check no longer swallows a lookup error. Two more comments that
still described owner-scoped code.

Left as the reviewer rated them, non-blocking: ejection on a transient DB
error (a guest is 303'd on a HasGrant failure and the sign-in page cannot
explain why), the concurrent-republish race that can show a dead URL, the
five-minute window an already-open SSE stream survives revocation, and the
inert formatting bar a reader still sees.

Note for anyone with a dev database built from an earlier revision of this
branch: 0005 and 0006 were edited in place rather than superseded, so an
existing db has links.token and no grants.created_by, and CREATE TABLE IF
NOT EXISTS will not repair it. Delete it and let migrations rebuild.

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 1 month ago 7cdba6317e0870753b85a88139b7752cc0b30781 parent c8415ce

8 files changed, +179 −32

internal/docs/auth.go +21 −2
internal/docs/grants.go +5 −1
internal/docs/guest_test.go +97 −7
internal/docs/handlers.go +9 −2
internal/docs/menu.go +33 −17
internal/docs/menu_test.go +4 −1
internal/docs/share.go +8 −0
internal/docs/views/list.html +2 −2

CI passed — run details

Intent

full ledger: sharing

diff --git a/internal/docs/auth.go b/internal/docs/auth.go

index 254660b..d3c0de1 100644

--- a/internal/docs/auth.go

+++ b/internal/docs/auth.go

@@ -368,11 +368,30 @@ func requireAdmission(list allowlist, signinPath string) func(http.Handler) http

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) {

+ if !ok || list.allows(id.Address) {

+ next.ServeHTTP(w, r)

+ return

+ }

+ // A redirect answers "show me a page". It is never an answer

+ // to "store this".

+ //

+ // The first version of this middleware 303'd every method,

+ // which was the worst defect on this branch and was introduced

+ // by the fix for the previous one. fetch() follows redirects by

+ // default, so the editor's POST became a GET of the sign-in

+ // page, came back 200 with HTML, and save-queue.mjs read res.ok

+ // and reported the batch saved. A revoked writer typed into a

+ // page that told them everything was fine — strictly worse than

+ // the reader-editor bug, which at least lit "save failed".

+ //

+ // rastrillo's own refuseSession draws the line in exactly this

+ // place and for exactly this reason. (Re-review, finding 1.)

+ if r.Method == http.MethodGet || r.Method == http.MethodHead {

http.Redirect(w, r, signinPath, http.StatusSeeOther)

return

}

- next.ServeHTTP(w, r)

+ http.Error(w, rastrillo.T(r, "docs.error.generic"), http.StatusForbidden)

})

}

diff --git a/internal/docs/grants.go b/internal/docs/grants.go

index 8723c45..bebba5a 100644

--- a/internal/docs/grants.go

+++ b/internal/docs/grants.go

@@ -177,7 +177,11 @@ func (s *Store) Grant(ctx context.Context, owner int64, sqid, address string, ro

// Granting to yourself is refused: you already reach it by owning it,

// and the row would put the owner on their own share list as though

// somebody had been invited.

- if ownerAddress, err := s.addressOf(ctx, owner); err == nil && ownerAddress == subject {

+ ownerAddress, err := s.addressOf(ctx, owner)

+ if err != nil {

+ return fmt.Errorf("docs: resolve owner address: %w", err)

+ }

+ if ownerAddress == subject {

return errEmptyAddress

}

diff --git a/internal/docs/guest_test.go b/internal/docs/guest_test.go

index dfa1a1b..2c04687 100644

--- a/internal/docs/guest_test.go

+++ b/internal/docs/guest_test.go

@@ -46,14 +46,53 @@ func TestAGuestCannotCreateOrShare(t *testing.T) {

t.Fatalf("the guest cannot read the document he was granted: %d", rec.Code)

}

- // But he is a guest. He cannot mint documents of his own...

- if rec := post(t, app, bob, "/documents", "title=Bob%27s+own"); rec.Code == http.StatusSeeOther {

- t.Error("a guest created a document; a grant is not a membership")

+ // But he is a guest. He cannot mint documents of his own — 403

+ // exactly, so a 500 cannot pass for a refusal.

+ if rec := post(t, app, bob, "/documents", "title=Bob%27s+own"); rec.Code != http.StatusForbidden {

+ t.Errorf("guest create: status %d, want 403", rec.Code)

}

// ...and he cannot admit anybody else, which is the transitive half

// and the one that makes the first unbounded.

- if rec := post(t, app, bob, "/d/"+sqid+"/share", "address=mallory@example.com&role=writer"); rec.Code == http.StatusSeeOther {

- t.Error("a guest shared a document; admission would be transitive")

+ //

+ // Asserting 403 specifically, not merely "not 303". The first version

+ // of this test accepted any non-303, which the OWNER check already

+ // produces for Bob on Alice's document — so deleting the member guard

+ // from sharePost left it green. The re-review caught that by mutation:

+ // three of the four guarded routes had never been exercised at all.

+ for _, c := range []struct{ what, path, body string }{

+ {"share", "/d/" + sqid + "/share", "address=mallory@example.com&role=writer"},

+ {"revoke", "/d/" + sqid + "/share/revoke", "address=alice@example.com"},

+ {"publish", "/d/" + sqid + "/share/link", "on=1"},

+ } {

+ if rec := post(t, app, bob, c.path, c.body); rec.Code != http.StatusForbidden {

+ t.Errorf("guest %s: status %d, want 403 from the member guard", c.what, rec.Code)

+ }

+// A guest is not shown a control they cannot use. Two of them: the create

+// form in the list header, and File → New document in the editor's menu

+// bar. Both post /documents, which answers a guest 403 with a bare text

+// page. (Re-review, finding 3.)

+func TestAGuestIsNotShownCreateControls(t *testing.T) {

+ app := members(t, "alice@example.com")

+ alice := signInCookies(t, app, "alice@example.com")

+ sqid := createDoc(t, app, alice, "Minutes")

+ post(t, app, alice, "/d/"+sqid+"/share", "address=bob@example.com&role=writer")

+ 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"`) {

+ t.Errorf("the guest's %s offers a create control they are refused", page)

+ }

+ // Alice keeps both, or the check above passes on an app that offers

+ // nobody a way to create a document.

+ for _, page := range []string{"/", "/d/" + sqid} {

+ if body := get(t, app, alice, page).Body.String(); !strings.Contains(body, `action="/documents"`) {

+ t.Errorf("the member's %s lost its create control", page)

+ }

}

@@ -89,9 +128,10 @@ func TestRevokingTheLastGrantEndsAdmission(t *testing.T) {

post(t, app, alice, "/d/"+sqid+"/share/revoke", "address=bob@example.com")

// The same cookies, still cryptographically valid, now reach nothing.

+ // 303 exactly: "not 200" would let a 500 pass for a refusal.

for _, path := range []string{"/", "/d/" + sqid} {

- if rec := get(t, app, bob, path); rec.Code == http.StatusOK {

- t.Errorf("a revoked guest still reads %s with a live session", path)

+ if rec := get(t, app, bob, path); rec.Code != http.StatusSeeOther {

+ t.Errorf("a revoked guest's GET %s answered %d, want 303", path, rec.Code)

}

@@ -102,6 +142,56 @@ func TestRevokingTheLastGrantEndsAdmission(t *testing.T) {

}

+// A revoked guest's writes are REFUSED, not redirected.

+//

+// The worst defect on this branch, and I introduced it while fixing the

+// last one. requireAdmission answered every method with 303 → /signin.

+// fetch() follows redirects by default, so the editor's POST became a GET

+// of the sign-in page, came back 200 with HTML, and save-queue.mjs read

+// res.ok and reported the batch saved. Alice revokes Bob mid-sentence and

+// Bob types for an hour into a page that says everything is fine.

+//

+// That is strictly worse than the reader-editor bug this branch already

+// fixed: that one at least lit the "save failed" indicator. rastrillo's

+// own refuseSession redirects GET/HEAD and 403s everything else, for

+// exactly this reason, and requireAdmission now does the same.

+//

+// A redirect is a fine answer to "show me a page". It is never an answer

+// to "store this". (Re-review, finding 1.)

+func TestARevokedGuestsWritesAreRefusedNotRedirected(t *testing.T) {

+ app := members(t, "alice@example.com")

+ alice := signInCookies(t, app, "alice@example.com")

+ sqid := createDoc(t, app, alice, "Minutes")

+ post(t, app, alice, "/d/"+sqid+"/share", "address=bob@example.com&role=writer")

+ bob := signInCookies(t, app, "bob@example.com")

+ // He really can write, first — or the assertion below passes on an

+ // app where nobody can.

+ if rec := postBytes(t, app, bob, "/d/"+sqid+"/updates", docs.ReadFragmentFixture(t, "hello")); rec.Code != http.StatusNoContent {

+ t.Fatalf("precondition: the guest writer cannot write (%d)", rec.Code)

+ }

+ post(t, app, alice, "/d/"+sqid+"/share/revoke", "address=bob@example.com")

+ // Now every write must be refused with a status the browser reports

+ // as a failure. A 3xx here is the bug: fetch follows it and the save

+ // queue reads the sign-in page's 200 as success.

+ rec := postBytes(t, app, bob, "/d/"+sqid+"/updates", docs.ReadFragmentFixture(t, "hello"))

+ if rec.Code >= 300 && rec.Code < 400 {

+ 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)

+ }

+ // GETs still redirect — that is what a redirect is for, and a person

+ // following a link deserves the sign-in page rather than a bare 403.

+ if rec := get(t, app, bob, "/"); rec.Code != http.StatusSeeOther {

+ t.Errorf("a revoked guest's GET / answered %d, want 303 to sign-in", rec.Code)

+ }

// The wiring, end to end through the real sign-in, rather than allows()

// and HasGrant each tested against a stand-in for the other.

func TestAGrantAdmitsThroughTheRealSignIn(t *testing.T) {

diff --git a/internal/docs/handlers.go b/internal/docs/handlers.go

index 67a7cd9..6c14a80 100644

--- a/internal/docs/handlers.go

+++ b/internal/docs/handlers.go

@@ -109,6 +109,12 @@ type handlers struct {

// (rastrillo/auth.DefaultSessionTTL); a page with no way to end one

// on purpose is a real gap on something public, not a cosmetic one.

type listPage struct {

+ // CanCreate gates the create form in the list header. A guest is

+ // answered 403 by POST /documents, and a form whose only outcome is a

+ // bare error page is a control that exists to fail — the same defect

+ // as the editor a reader could type into. (Re-review, finding 3.)

+ CanCreate bool

Title string

Groups []docGroup

EmptyTitle string

@@ -268,6 +274,7 @@ func (h *handlers) list(w http.ResponseWriter, r *http.Request) {

query := strings.TrimSpace(r.URL.Query().Get("q"))

data := listPage{

+ CanCreate: h.member(r),

Title: rastrillo.T(r, "docs.list.title"),

EmptyTitle: rastrillo.T(r, "docs.list.empty.title"),

EmptyBody: rastrillo.T(r, "docs.list.empty.body"),

@@ -601,7 +608,7 @@ func (h *handlers) show(w http.ResponseWriter, r *http.Request) {

JumpToScript: jumpToScriptPath(),

Toolbar: newToolbarView(r),

- Menu: newMenuView(r, doc.Sqid),

+ Menu: newMenuView(r, doc.Sqid, h.member(r)),

FontFaces: fontFaces(),

ViewMode: viewMode(r),

PageLabel: rastrillo.T(r, "docs.editor.page.number"),

@@ -623,7 +630,7 @@ const maxLogBytes = 8 << 20

// getDocumentForUpdates is the reader-scoped lookup both

// updatesGet and updatesPost share: it resolves the signed-in

-// person, then loads the document through Store.Get — already

+// person, then loads the document through Store.GetFor — already

// reader-scoped and already 404-not-403 (its own doc comment) — so a

// stranger's sqid returns exactly the same outcome as one that does

// not exist. ok is false once a response has already been written

diff --git a/internal/docs/menu.go b/internal/docs/menu.go

index ea91fe5..2c39144 100644

--- a/internal/docs/menu.go

+++ b/internal/docs/menu.go

@@ -160,7 +160,38 @@ func viewMode(r *http.Request) string {

return ViewPaged

}

-func newMenuView(r *http.Request, sqid string) menuView {

+// 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.

+//

+// "New document" is a plain form posting /documents, which a guest is

+// answered 403 for. A control that exists only to fail is the same defect

+// as the editor a reader could type into, in a smaller place — so it is

+// absent for them rather than present and refused.

+func fileItems(r *http.Request, sqid string, canCreate bool) []menuItem {

+ items := make([]menuItem, 0, 8)

+ if canCreate {

+ items = append(items, menuItem{Kind: menuForm, Action: "/documents", Label: rastrillo.T(r, "docs.editor.menu.file.new")})

+ }

+ return append(items,

+ menuItem{Kind: menuLink, Href: "/", Label: rastrillo.T(r, "docs.editor.menu.file.open")},

+ menuItem{Kind: menuDivider},

+ // The two exports, moved here from [docs-doc-actions]. Still plain

+ // links with the download attribute, so they behave identically to

+ // before for anyone who had learned where they were — the route

+ // did not change, only where it is named.

+ menuItem{Kind: menuLink, Href: "/d/" + sqid + "/export.pdf", Download: true, Label: rastrillo.T(r, "docs.export.pdf.label")},

+ menuItem{Kind: menuLink, Href: "/d/" + sqid + "/export.md", Download: true, Label: rastrillo.T(r, "docs.export.markdown.label")},

+ menuItem{Kind: menuCommand, Command: "print", Label: rastrillo.T(r, "docs.editor.menu.file.print"), Shortcut: "Ctrl+P"},

+ menuItem{Kind: menuDivider},

+ menuItem{Kind: menuForm, Action: "/signout", Label: rastrillo.T(r, "docs.signout.label")},

+ )

+func newMenuView(r *http.Request, sqid string, canCreate bool) menuView {

mode := viewMode(r)

return menuView{

Label: rastrillo.T(r, "docs.editor.menu.label"),

@@ -168,22 +199,7 @@ func newMenuView(r *http.Request, sqid string) menuView {

{

ID: "file",

Title: rastrillo.T(r, "docs.editor.menu.file"),

- Items: []menuItem{

- {Kind: menuForm, Action: "/documents", Label: rastrillo.T(r, "docs.editor.menu.file.new")},

- {Kind: menuLink, Href: "/", Label: rastrillo.T(r, "docs.editor.menu.file.open")},

- {Kind: menuDivider},

- // The two exports, moved here from

- // [docs-doc-actions]. Still plain links with the

- // download attribute, so they behave identically to

- // before for anyone who had learned where they were

- // — the route did not change, only where it is

- // named.

- {Kind: menuLink, Href: "/d/" + sqid + "/export.pdf", Download: true, Label: rastrillo.T(r, "docs.export.pdf.label")},

- {Kind: menuLink, Href: "/d/" + sqid + "/export.md", Download: true, Label: rastrillo.T(r, "docs.export.markdown.label")},

- {Kind: menuCommand, Command: "print", Label: rastrillo.T(r, "docs.editor.menu.file.print"), Shortcut: "Ctrl+P"},

- {Kind: menuDivider},

- {Kind: menuForm, Action: "/signout", Label: rastrillo.T(r, "docs.signout.label")},

- },

+ Items: fileItems(r, sqid, canCreate),

{

ID: "edit",

diff --git a/internal/docs/menu_test.go b/internal/docs/menu_test.go

index f341463..7e5444f 100644

--- a/internal/docs/menu_test.go

+++ b/internal/docs/menu_test.go

@@ -14,7 +14,10 @@ import (

// clicked.

func newMenuViewForTest() menuView {

- return newMenuView(httptest.NewRequest("GET", "/d/abc123", nil), "abc123")

+ // 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)

}

// Every command the menu names is implemented by one of the two modules

diff --git a/internal/docs/share.go b/internal/docs/share.go

index 7abc56e..1ee4c20 100644

--- a/internal/docs/share.go

+++ b/internal/docs/share.go

@@ -144,6 +144,14 @@ func (h *handlers) renderShare(w http.ResponseWriter, r *http.Request, doc *Docu

page.Error = rastrillo.T(r, errKey)

}

+ // no-store whenever this render carries the plaintext capability.

+ // "Shown once" was a server-side truth only: the browser's disk cache

+ // and bfcache would keep the URL, and any intermediary might, so on a

+ // shared machine the next person could page back to it.

+ // (Re-review, finding 5.)

+ if page.PublicURL != "" {

+ w.Header().Set("Cache-Control", "no-store")

+ }

if err := h.render.page(w, r, "share", page); err != nil {

h.logger.Error("docs: render share", "sqid", doc.Sqid, "err", err)

http.Error(w, rastrillo.T(r, "docs.error.generic"), http.StatusInternalServerError)

diff --git a/internal/docs/views/list.html b/internal/docs/views/list.html

index fe99b7b..7caaae0 100644

--- a/internal/docs/views/list.html

+++ b/internal/docs/views/list.html

@@ -311,10 +311,10 @@

placeholder="{{.SearchPlace}}" aria-label="{{.SearchLabel}}" autocomplete="off">

<button rst-btn="ghost" type="submit">{{.SearchLabel}}</button>

</form>

- <form docs-create-form method="post" action="/documents">

+{{if .CanCreate}} <form docs-create-form method="post" action="/documents">

<button rst-btn="primary" type="submit">{{.CreateLabel}}</button>

- </form>

+ </form>{{end}}

</div>

</header>

{{/* Searching renders a ranked, UNGROUPED result list. Recency