amadan

oficina / sheets Public

merge: five review fixes that were never brought back to core

The branch review recorded these as fixed, with SHAs, on 2026-09-02 —
the link token out of the URL, safeReturn's backslash spelling, undo
past an unsealable event, a link mint that stopped blaming the role,
and the revoke notice that claimed more than it did. Every one of them
was committed to its own branch and none reached core, so the deployed
canary has been running without the lot.

Found while acting on an independent review: Codex reported
sharenotice.go absent and the revoke notice still reading "Access
removed", which is exactly what a stranded fix branch looks like from
the inside.

They are a linear stack, so one merge takes all five.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018RN4HhHR4rHH3Fxh4LMds5
Paul Campbell pushed by paul@keymail.dev a8a27921bc2e8f9d8d5e3dfc025ed77fd146e541 parent bad02cbparent 9b723a5
13 files changed, +496 −51
  • internal/sheets/grants_revoke_notice_test.go +104 −0
  • internal/sheets/handlers_history.go +19 −2
  • internal/sheets/handlers_history_test.go +45 −0
  • internal/sheets/handlers_link.go +30 −8
  • internal/sheets/handlers_share.go +30 −10
  • internal/sheets/members.go +18 −0
  • internal/sheets/sharelink.go +11 −1
  • internal/sheets/sharelink_failure_test.go +50 −0
  • internal/sheets/sharelink_test.go +15 −27
  • internal/sheets/sharelink_url_test.go +73 −0
  • internal/sheets/sharenotice.go +73 −0
  • internal/sheets/theme.go +20 −3
  • internal/sheets/theme_test.go +8 −0
diff --git a/internal/sheets/grants_revoke_notice_test.go b/internal/sheets/grants_revoke_notice_test.go
new file mode 100644
index 0000000..c85cf06
--- /dev/null
+++ b/internal/sheets/grants_revoke_notice_test.go
@@ -0,0 +1,104 @@
+package sheets
+
+import (
+ "fmt"
+ "net/http"
+ "net/http/httptest"
+ "net/url"
+ "strings"
+ "testing"
+)
+
+func httpGET(path string) *http.Request {
+ return httptest.NewRequest(http.MethodGet, path, nil)
+}
+
+// teammateOn is somebody with BOTH an explicit grant and team standing
+// — a colleague who was also shared with directly. Team standing is
+// what makes revoking the grant not the same as removing their access,
+// so it is the whole point of the fixture.
+func teammateOn(t *testing.T, app *App, sheetID uint64, address string, role SheetRole) Member {
+ t.Helper()
+ m := guestOn(t, app, sheetID, address, role)
+ if err := app.db.Model(&Member{}).Where("id = ?", m.ID).
+ Update("team_role", string(TeamMember)).Error; err != nil {
+ t.Fatalf("put %s on the team: %v", address, err)
+ }
+ m.TeamRole = string(TeamMember)
+ return m
+}
+
+// Removing a teammate's explicit grant removes the grant and does not
+// remove their access: team standing reaches every sheet on the
+// instance as an editor, by design (spec §8.2). The fallback is right
+// and stays. Saying "Access removed." on top of it is not.
+//
+// An owner who reads that and walks away believes they have shut
+// somebody out of a sealed sheet. They have not, and nothing on the
+// screen tells them.
+func TestRevokingATeammatesGrantDoesNotClaimTheyLostAccess(t *testing.T) {
+ app, cookies := signIn(t)
+ s, _ := newSheet(t, app, cookies, "Budget")
+ m := teammateOn(t, app, s.ID, "ada@example.com", SheetViewer)
+
+ form := url.Values{"member": {fmt.Sprint(m.ID)}}
+ rec := do(t, app, as(postForm(sharePath(s)+"/revoke", form), cookies))
+ if rec.Code != http.StatusSeeOther {
+ t.Fatalf("revoke = %d: %s", rec.Code, rec.Body.String())
+ }
+
+ // The premise. If this ever stops holding, the notice below is not
+ // the thing to change — the fallback is.
+ if !app.mayWrite(s.ID, m) {
+ t.Fatal("a team member lost access when their explicit grant went; " +
+ "team standing is supposed to reach every sheet")
+ }
+
+ loc := rec.Header().Get("Location")
+ if strings.HasSuffix(loc, "notice=revoked") {
+ t.Fatalf("Location = %q — told the owner access was removed while "+
+ "team standing still admits the person", loc)
+ }
+ if !strings.Contains(loc, "notice=revoked-team") {
+ t.Fatalf("Location = %q, want the notice that says they still reach it", loc)
+ }
+
+ // An email address must not travel in the query — that is the same
+ // access-log leak the share token was just moved out of. The id
+ // does, and the address is read back from it server-side.
+ if strings.Contains(loc, "ada@example.com") ||
+ strings.Contains(loc, url.QueryEscape("ada@example.com")) {
+ t.Fatalf("Location = %q carries an email address", loc)
+ }
+
+ // The sentence has to name them, because revoking the grant also
+ // takes them off the list below: without a name the notice refers
+ // to somebody the owner can no longer see on the screen.
+ page := do(t, app, as(httpGET(loc), cookies)).Body.String()
+ if !strings.Contains(page, "ada@example.com") {
+ t.Fatal("the notice does not name the person it is about")
+ }
+ if !strings.Contains(page, "on the team") {
+ t.Fatalf("the notice does not say why they still reach it")
+ }
+}
+
+// The plain case has to keep the plain sentence: a guest with no team
+// standing really does lose the sheet, and a fix that hedged every
+// revoke would be its own kind of lie.
+func TestRevokingAGuestsGrantStillReportsAPlainRemoval(t *testing.T) {
+ app, cookies := signIn(t)
+ s, _ := newSheet(t, app, cookies, "Budget")
+ m := guestOn(t, app, s.ID, "ada@elsewhere.example", SheetViewer)
+
+ form := url.Values{"member": {fmt.Sprint(m.ID)}}
+ rec := do(t, app, as(postForm(sharePath(s)+"/revoke", form), cookies))
+
+ if app.mayRead(s.ID, m) {
+ t.Fatal("a guest kept access after their only grant was revoked")
+ }
+ if loc := rec.Header().Get("Location"); !strings.Contains(loc, "notice=revoked") ||
+ strings.Contains(loc, "revoked-team") {
+ t.Fatalf("Location = %q, want the plain removal notice", loc)
+ }
+}
diff --git a/internal/sheets/handlers_history.go b/internal/sheets/handlers_history.go
index fb6bb24..e595191 100644
--- a/internal/sheets/handlers_history.go
+++ b/internal/sheets/handlers_history.go
@@ -157,7 +157,24 @@ func (a *App) handleRestore(w http.ResponseWriter, r *http.Request) {
a.historyRedirect(w, r, s, "restored")
}
-// handleUndo reverts the viewer's own most recent edit.
+// undoScan is how far back undo will look for an edit it can read.
+//
+// It asked for one row and took what came back, which is wrong because
+// History skips an event it cannot unseal: one unreadable newest edit
+// turned into "nothing to undo" while readable older ones sat behind
+// it. Asking for a window and taking the first that survives is the
+// fix, and the window is bounded rather than open because a tab full of
+// undecryptable events would otherwise turn one undo into a scan of the
+// whole log, unsealing every row on the way.
+//
+// Twenty is a judgement, not a measurement: far enough that the case
+// this exists for — an event or two under a key we no longer hold — is
+// covered, short enough that the pathological tab costs twenty AEAD
+// opens and stops. Past that, undo reports nothing to undo, which by
+// then is the honest answer.
+const undoScan = 20
+
+// handleUndo reverts the viewer's own most recent readable edit.
//
// Own-edits-only, cell-level (spec §7). Multi-user undo has no correct
// answer; this one is predictable and explainable, which is the next
@@ -179,7 +196,7 @@ func (a *App) handleUndo(w http.ResponseWriter, r *http.Request) {
return
}
records, err := store.History(a.db, a.sealer, store.HistoryQuery{
- TabID: s.TabID, ActorID: viewer.ID, Limit: 1,
+ TabID: s.TabID, ActorID: viewer.ID, Limit: undoScan,
})
if err != nil {
a.fail(w, r, err)
diff --git a/internal/sheets/handlers_history_test.go b/internal/sheets/handlers_history_test.go
index 92f4deb..c5c7217 100644
--- a/internal/sheets/handlers_history_test.go
+++ b/internal/sheets/handlers_history_test.go
@@ -122,6 +122,51 @@ func TestUndoWithNothingToUndoSaysSo(t *testing.T) {
}
}
+// History skips an event this key cannot open rather than failing the
+// whole read — one unreadable row must not take the log with it. Undo
+// asked for exactly one row and then skipped it, so a single unreadable
+// newest edit reported that the person had nothing to undo at all,
+// while older edits of theirs sat right there.
+//
+// The shape is what makes it worth a test: it does not read as an
+// error. It reads as a calm, correct answer about their own work, given
+// to somebody who has just made a mistake and is trying to take it
+// back.
+func TestUndoLooksPastAnEventItCannotUnseal(t *testing.T) {
+ app, cookies := signIn(t)
+ s, cells := newSheet(t, app, cookies, "Budget")
+
+ postCell(t, app, cookies, cells, `{"r":0,"c":0,"raw":"first"}`)
+ postCell(t, app, cookies, cells, `{"r":0,"c":0,"raw":"second"}`)
+ spoil(t, app, newestEventID(t, app, s))
+
+ rec := do(t, app, as(postForm(fmt.Sprintf("/sheets/%d/undo", s.ID), url.Values{}), cookies))
+ if loc := rec.Header().Get("Location"); strings.Contains(loc, "nothing") {
+ t.Fatal("undo said there was nothing to undo while a readable older edit " +
+ "of the viewer's own was in the log")
+ }
+
+ // The readable edit set A1 to "first" from empty, so undoing it
+ // empties A1 — and in particular leaves it not showing "second".
+ grid := decodeGrid(t, do(t, app, as(httptest.NewRequest(http.MethodGet, cells, nil), cookies)))
+ for _, c := range grid.Cells {
+ if c.R == 0 && c.C == 0 && c.Display == "second" {
+ t.Fatal("undo changed nothing; it stopped at the event it could not open")
+ }
+ }
+}
+
+// spoil makes one event's payload undecryptable, which is what an event
+// sealed under a key this instance no longer holds looks like from
+// here: AEAD rejects it and openInput reports not-ok.
+func spoil(t *testing.T, app *App, id uint64) {
+ t.Helper()
+ if err := app.db.Exec("UPDATE events SET payload_sealed = ? WHERE id = ?",
+ []byte("not a sealed payload"), id).Error; err != nil {
+ t.Fatalf("spoil event %d: %v", id, err)
+ }
+}
+
// An event id from another sheet must not be applicable here — it comes
// from a URL, and without the tab in the lookup a caller could name any
// event on the instance.
diff --git a/internal/sheets/handlers_link.go b/internal/sheets/handlers_link.go
index 9d60890..3f512ca 100644
--- a/internal/sheets/handlers_link.go
+++ b/internal/sheets/handlers_link.go
@@ -1,6 +1,7 @@
package sheets
import (
+ "errors"
"net/http"
"strconv"
@@ -125,6 +126,24 @@ func (a *App) linkURL(token string) string {
}
// handleLinkCreate mints a link and shows the token once.
+//
+// This is the one POST on the share screen that answers with a page
+// instead of a 303, and the exception is deliberate. The token exists
+// in readable form for exactly this moment — only its SHA-256 is
+// stored — so it has to reach the person in this response or not at
+// all. Every way of carrying it through a redirect puts it somewhere we
+// do not control: a query string is written into the platform edge's
+// access logs, which AGENTS.md names as storage we cannot grep, and a
+// server-side stash would have to survive a redirect landing on another
+// instance, failing by showing nothing where the token is already gone.
+// Rendering here keeps it in this process, on this goroutine, in one
+// response body.
+//
+// The cost is that a reload re-submits the form and mints a second
+// link. That is visible in the list below and revocable from it, which
+// is the trade the other options do not offer: a token in an access log
+// cannot be taken back, and a token lost between instances cannot be
+// recovered.
func (a *App) handleLinkCreate(w http.ResponseWriter, r *http.Request) {
s, viewer, ok := a.sheetForSharing(w, r)
if !ok {
@@ -132,17 +151,20 @@ func (a *App) handleLinkCreate(w http.ResponseWriter, r *http.Request) {
}
role := SheetRole(r.FormValue("role"))
token, err := a.createLink(s.ID, role, viewer)
- if err != nil {
+ if errors.Is(err, errBadLinkRole) {
+ // The only failure the person can act on, and the only one they
+ // are told about in these terms.
a.shareRedirect(w, r, s, "bad-link-role")
return
}
- // The token travels in the redirect because this is the only moment
- // it exists in readable form: it is not stored, so a reload of the
- // share screen cannot show it again. Putting it in the URL is a
- // real trade — it lands in browser history — and it is the smaller
- // cost against storing the token or showing it never.
- http.Redirect(w, r, "/sheets/"+strconv.FormatUint(s.ID, 10)+
- "/share?notice=link&token="+token, http.StatusSeeOther)
+ if err != nil {
+ // Ours, not theirs: the same 500 every other broken write on
+ // this screen gives, rather than a sentence blaming a role
+ // choice that was fine.
+ a.fail(w, r, err)
+ return
+ }
+ a.renderShare(w, r, s, viewer, "link", token)
}
// handleLinkRevoke turns a link off.
diff --git a/internal/sheets/handlers_share.go b/internal/sheets/handlers_share.go
index dcd2795..77e51a6 100644
--- a/internal/sheets/handlers_share.go
+++ b/internal/sheets/handlers_share.go
@@ -55,10 +55,15 @@ func (a *App) handleShare(w http.ResponseWriter, r *http.Request) {
if !ok {
return
}
- a.renderShare(w, r, s, viewer, r.URL.Query().Get("notice"))
+ a.renderShare(w, r, s, viewer, r.URL.Query().Get("notice"), "")
}
-func (a *App) renderShare(w http.ResponseWriter, r *http.Request, s SheetSummary, _ Member, notice string) {
+// renderShare draws the screen. newLink is a token in readable form,
+// and it arrives as an argument rather than out of the request for the
+// reason handleLinkCreate gives: the only caller that has one is the
+// handler that just minted it, in the same process, on the same
+// goroutine. Nothing that crossed a URL can reach this parameter.
+func (a *App) renderShare(w http.ResponseWriter, r *http.Request, s SheetSummary, _ Member, notice, newLink string) {
people, err := a.grantsOn(s.ID)
if err != nil {
a.fail(w, r, err)
@@ -78,14 +83,11 @@ func (a *App) renderShare(w http.ResponseWriter, r *http.Request, s SheetSummary
Sheet: s, People: people, Roles: shareableRoles,
LinkRoles: linkRoles, Links: rows, Notice: noticeText(notice),
}
- // Only ever from this request's own query, and only right after
- // minting one. It is echoed into a page, so it is checked against
- // the token that was actually just created rather than trusted:
- // hashing it and finding a live link for this sheet is the test.
- if tok := r.URL.Query().Get("token"); tok != "" {
- if l, ok := a.linkFor(tok); ok && l.SheetID == s.ID {
- view.NewLink = a.linkURL(tok)
- }
+ if notice == noticeRevokedTeam {
+ view.Notice = a.revokedTeamNotice(r, s.ID)
+ }
+ if newLink != "" {
+ view.NewLink = a.linkURL(newLink)
}
a.render(w, r, "share", "Sharing "+s.Title, http.StatusOK, view)
}
@@ -102,6 +104,11 @@ func noticeText(code string) string {
return "Shared. They can sign in with that address to open it."
case "revoked":
return "Access removed."
+ case noticeRevokedTeam:
+ // renderShare replaces this with the named version when it can
+ // resolve who the revoke was about. This is the honest sentence
+ // when it cannot.
+ return revokedTeamText
case "bad-address":
return "That does not look like an email address."
case "bad-role":
@@ -174,6 +181,10 @@ func (a *App) handleShareRevoke(w http.ResponseWriter, r *http.Request) {
a.fail(w, r, err)
return
}
+ if a.stillReaches(s.ID, memberID) {
+ a.shareRedirectAbout(w, r, s, noticeRevokedTeam, memberID)
+ return
+ }
a.shareRedirect(w, r, s, "revoked")
}
@@ -230,6 +241,15 @@ func (a *App) sheetForSharing(w http.ResponseWriter, r *http.Request) (SheetSumm
// uint64 rather than passed through — both end up in a Location header,
// and a number that came back out as a number cannot carry anything
// else.
+// shareRedirectAbout is shareRedirect for a notice that has to name
+// somebody. It carries the member id and never the address: an address
+// in a request line is an address in the platform's access logs.
+func (a *App) shareRedirectAbout(w http.ResponseWriter, r *http.Request, s SheetSummary,
+ notice string, memberID uint64) {
+ http.Redirect(w, r, "/sheets/"+strconv.FormatUint(s.ID, 10)+"/share?notice="+notice+
+ "&who="+strconv.FormatUint(memberID, 10), http.StatusSeeOther)
+}
+
func (a *App) shareRedirect(w http.ResponseWriter, r *http.Request, s SheetSummary, notice string) {
http.Redirect(w, r, "/sheets/"+strconv.FormatUint(s.ID, 10)+"/share?notice="+notice,
http.StatusSeeOther)
diff --git a/internal/sheets/members.go b/internal/sheets/members.go
index 60c187c..13c2ac1 100644
--- a/internal/sheets/members.go
+++ b/internal/sheets/members.go
@@ -92,3 +92,21 @@ func (a *App) actor(r *http.Request) (Member, bool) {
}
return m, true
}
+
+// memberByID reads one member row.
+//
+// It exists for the share screen's revoke path, which has an id from a
+// form and needs the person's TEAM standing — the field that decides
+// whether removing their grant actually removed anything. Reading the
+// row is the only way to know: team standing is not in the grants
+// table, which is exactly why revoke could not see it.
+func (a *App) memberByID(id uint64) (Member, bool) {
+ if a.db == nil || id == 0 {
+ return Member{}, false
+ }
+ var m Member
+ if err := a.db.Where("id = ?", id).First(&m).Error; err != nil {
+ return Member{}, false
+ }
+ return m, true
+}
diff --git a/internal/sheets/sharelink.go b/internal/sheets/sharelink.go
index 1c88967..0cbe327 100644
--- a/internal/sheets/sharelink.go
+++ b/internal/sheets/sharelink.go
@@ -54,6 +54,16 @@ func hashLinkToken(token string) string {
return hex.EncodeToString(sum[:])
}
+// errBadLinkRole is the one createLink failure the person asking can do
+// anything about.
+//
+// It is a sentinel rather than an anonymous error because the handler
+// has to tell it apart from the rest. Every other way this function
+// fails — the token generator, the insert, no database at all — is ours
+// and not theirs, and reporting those as a rejected role choice tells
+// somebody to fix the one thing that was not broken.
+var errBadLinkRole = errors.New("sheets: a link can only view or comment, never edit")
+
// createLink mints a link for a sheet and returns the token, which the
// caller must show once and then forget.
func (a *App) createLink(sheetID uint64, role SheetRole, by Member) (string, error) {
@@ -64,7 +74,7 @@ func (a *App) createLink(sheetID uint64, role SheetRole, by Member) (string, err
// Named rather than silently downgraded: somebody asking for an
// editor link has a expectation this refuses, and refusing
// quietly would leave them thinking they made one.
- return "", errors.New("sheets: a link can only view or comment, never edit")
+ return "", errBadLinkRole
}
token, hash, err := newLinkToken()
if err != nil {
diff --git a/internal/sheets/sharelink_failure_test.go b/internal/sheets/sharelink_failure_test.go
new file mode 100644
index 0000000..bb17739
--- /dev/null
+++ b/internal/sheets/sharelink_failure_test.go
@@ -0,0 +1,50 @@
+package sheets
+
+import (
+ "net/http"
+ "net/url"
+ "strings"
+ "testing"
+)
+
+// Refusing an editor link is a real refusal with a real reason, and the
+// person can act on it by picking another role. It keeps its own
+// notice.
+func TestAnEditorLinkIsRefusedByName(t *testing.T) {
+ app, cookies := signIn(t)
+ s, _ := newSheet(t, app, cookies, "Budget")
+
+ form := url.Values{"role": {string(SheetEditor)}}
+ rec := do(t, app, as(postForm(sharePath(s)+"/link", form), cookies))
+ if loc := rec.Header().Get("Location"); !strings.Contains(loc, "bad-link-role") {
+ t.Fatalf("asking for an editor link = %q, want the role refusal", loc)
+ }
+}
+
+// Everything else that can go wrong inside createLink — the token
+// generator, the insert, a missing database — was reported with that
+// same notice. So an infrastructure fault was presented to the person
+// as their own input mistake: they retry the identical form, get the
+// identical sentence, and have no way to tell that the thing they are
+// being blamed for is not the thing that went wrong.
+func TestAFailedLinkMintDoesNotBlameTheRole(t *testing.T) {
+ app, cookies := signIn(t)
+ s, _ := newSheet(t, app, cookies, "Budget")
+
+ // A real failure rather than a mocked one: with the table gone the
+ // insert fails inside createLink, past the role check, on the same
+ // path a disk or migration fault would take.
+ if err := app.db.Exec("DROP TABLE share_links").Error; err != nil {
+ t.Fatalf("drop share_links: %v", err)
+ }
+
+ form := url.Values{"role": {string(SheetViewer)}}
+ rec := do(t, app, as(postForm(sharePath(s)+"/link", form), cookies))
+ if loc := rec.Header().Get("Location"); strings.Contains(loc, "bad-link-role") {
+ t.Fatalf("a database failure was reported as the person's role choice: %q", loc)
+ }
+ if rec.Code != http.StatusInternalServerError {
+ t.Fatalf("a failed mint = %d, want 500 — the same answer every other "+
+ "broken write on this screen gives", rec.Code)
+ }
+}
diff --git a/internal/sheets/sharelink_test.go b/internal/sheets/sharelink_test.go
index 21782af..efb9d8b 100644
--- a/internal/sheets/sharelink_test.go
+++ b/internal/sheets/sharelink_test.go
@@ -188,21 +188,16 @@ func TestTheTokenIsShownOnceAndNotAgain(t *testing.T) {
form := url.Values{"role": {string(SheetViewer)}}
rec := do(t, app, as(postForm(sharePath(s)+"/link", form), cookies))
- if rec.Code != http.StatusSeeOther {
+ if rec.Code != http.StatusOK {
t.Fatalf("creating a link = %d: %s", rec.Code, rec.Body.String())
}
- loc := rec.Header().Get("Location")
- if !strings.Contains(loc, "token=") {
- t.Fatalf("Location %q carries no token", loc)
- }
- // Following the redirect shows it.
- shown := do(t, app, as(httptest.NewRequest(http.MethodGet, loc, nil), cookies)).Body.String()
- if !strings.Contains(shown, "/l/") {
- t.Fatal("the share screen did not show the new link")
+ // The response to the POST is the one place it appears.
+ if !strings.Contains(rec.Body.String(), "/l/") {
+ t.Fatal("minting a link did not show the new link")
}
- // Loading the screen again, without the token, cannot.
+ // Loading the screen again cannot.
plain := do(t, app, as(httptest.NewRequest(http.MethodGet, sharePath(s), nil), cookies)).Body.String()
if strings.Contains(plain, "/l/") {
t.Fatal("the share screen showed a link URL without being handed the token — " +
@@ -213,23 +208,16 @@ func TestTheTokenIsShownOnceAndNotAgain(t *testing.T) {
}
}
-// A token for another sheet must not be echoed onto this sheet's share
-// screen just because it is valid.
-func TestTheShareScreenOnlyEchoesItsOwnToken(t *testing.T) {
- app, cookies := signIn(t)
- one, _ := newSheet(t, app, cookies, "One")
- two, _ := newSheet(t, app, cookies, "Two")
-
- token, err := app.createLink(two.ID, SheetViewer, signedInMember(t, app))
- if err != nil {
- t.Fatalf("createLink: %v", err)
- }
- body := do(t, app, as(httptest.NewRequest(http.MethodGet,
- sharePath(one)+"?notice=link&token="+token, nil), cookies)).Body.String()
- if strings.Contains(body, token) {
- t.Fatal("one sheet's share screen echoed another sheet's link token")
- }
-}
+// TestTheShareScreenOnlyEchoesItsOwnToken is deliberately gone.
+//
+// It asserted that a token minted for one sheet is not echoed onto
+// another sheet's share screen — a real property while the screen read
+// a token out of its query string. The screen no longer reads one at
+// all, so no production change could make that test fail: it would have
+// sat in the suite passing and proving nothing, which is the shape this
+// branch has been finding all week. TestTheShareScreenIgnoresATokenInIts
+// Query, in sharelink_url_test.go, asserts the stronger property that
+// replaced it.
// Only an owner may mint or revoke a link — it is the same power as
// granting access, in a different shape.
diff --git a/internal/sheets/sharelink_url_test.go b/internal/sheets/sharelink_url_test.go
new file mode 100644
index 0000000..bbab8f9
--- /dev/null
+++ b/internal/sheets/sharelink_url_test.go
@@ -0,0 +1,73 @@
+package sheets
+
+import (
+ "net/http"
+ "net/http/httptest"
+ "net/url"
+ "regexp"
+ "strings"
+ "testing"
+)
+
+// linkInBody pulls the one /l/<token> address a rendered page shows.
+var linkInBody = regexp.MustCompile(`/l/([A-Za-z0-9_-]{16,})`)
+
+// A share-link token is the whole capability: whoever holds it reads a
+// sealed sheet without signing in, without a member row, and without
+// leaving a trace we can revoke by name. A query string is written into
+// the platform edge's access logs, and AGENTS.md's rule is that those
+// logs are storage we cannot grep — so a token that transits a URL is
+// deposited, in the clear, somewhere we can neither audit nor redact.
+//
+// Browser history was the cost the original code weighed and accepted.
+// The access log is the one it did not, and it is the larger of the two.
+func TestTheLinkTokenNeverReachesAURL(t *testing.T) {
+ app, cookies := signIn(t)
+ s, _ := newSheet(t, app, cookies, "Budget")
+
+ form := url.Values{"role": {string(SheetViewer)}}
+ rec := do(t, app, as(postForm(sharePath(s)+"/link", form), cookies))
+
+ if loc := rec.Header().Get("Location"); loc != "" {
+ t.Fatalf("minting a link redirected to %q — a token cannot travel in a URL", loc)
+ }
+ if rec.Code != http.StatusOK {
+ t.Fatalf("minting a link = %d, want 200 with the screen rendered: %s",
+ rec.Code, rec.Body.String())
+ }
+
+ // Shown once, and it has to be the real thing rather than a
+ // placeholder — so the address on the page is exercised.
+ body := rec.Body.String()
+ m := linkInBody.FindStringSubmatch(body)
+ if m == nil {
+ t.Fatal("minting a link rendered no /l/ address — the token was lost, " +
+ "and only its hash is stored, so it cannot be recovered")
+ }
+ if got := do(t, app, httptest.NewRequest(http.MethodGet, "/l/"+m[1], nil)); got.Code != http.StatusOK {
+ t.Fatalf("the address the screen showed does not open the sheet: %d", got.Code)
+ }
+}
+
+// Closing the channel means the query parameter stops being read at
+// all. While the share screen still echoes a token handed to it, the
+// old URL remains a working way to put one in an access log — a caller
+// only has to build it.
+func TestTheShareScreenIgnoresATokenInItsQuery(t *testing.T) {
+ app, cookies := signIn(t)
+ s, _ := newSheet(t, app, cookies, "Budget")
+
+ token, err := app.createLink(s.ID, SheetViewer, signedInMember(t, app))
+ if err != nil {
+ t.Fatalf("createLink: %v", err)
+ }
+
+ body := do(t, app, as(httptest.NewRequest(http.MethodGet,
+ sharePath(s)+"?notice=link&token="+token, nil), cookies)).Body.String()
+ if strings.Contains(body, token) {
+ t.Fatal("the share screen echoed a token from its query string")
+ }
+ if strings.Contains(body, "/l/") {
+ t.Fatal("the share screen rendered a link address from its query string")
+ }
+}
diff --git a/internal/sheets/sharenotice.go b/internal/sheets/sharenotice.go
new file mode 100644
index 0000000..a0839db
--- /dev/null
+++ b/internal/sheets/sharenotice.go
@@ -0,0 +1,73 @@
+package sheets
+
+import (
+ "net/http"
+ "strconv"
+)
+
+// noticeRevokedTeam is the code for a revoke that removed a grant and
+// did not remove anybody's access.
+//
+// Team standing reaches every sheet on the instance as an editor, by
+// design (spec §8.2), and an explicit grant sits on top of it. So
+// removing a teammate's grant takes the grant away and leaves the
+// person able to open the sheet. The screen used to report the first
+// half and stay silent about the second, which let an owner read
+// "Access removed", walk away, and believe they had shut somebody out
+// of a sealed sheet.
+//
+// RULED by Paul, 2026-09-01: the sentence is what changes here, not the
+// behaviour. Removing a restricting grant from a teammate still widens
+// their access from viewer to editor — see the issue filed against this
+// commit, which records that as a separate, open question rather than
+// letting it disappear behind a notice that now describes it politely.
+const noticeRevokedTeam = "revoked-team"
+
+// revokedTeamText is the sentence when we cannot name the person.
+//
+// Paul's wording, with the name generalised for the fallback below.
+const revokedTeamText = "The explicit permission is removed, but since they are " +
+ "on the team, they still have access."
+
+// stillReaches reports whether somebody can still open a sheet.
+//
+// Asked AFTER the revoke, and asked of roleFor rather than reasoned
+// about separately, so the notice reports what the gate actually says
+// instead of holding a second opinion that can drift from it.
+func (a *App) stillReaches(sheetID, memberID uint64) bool {
+ m, ok := a.memberByID(memberID)
+ if !ok {
+ // The grant is gone and we cannot tell what is left. The plain
+ // sentence is the one that claims less.
+ return false
+ }
+ _, still := a.roleFor(sheetID, m)
+ return still
+}
+
+// revokedTeamNotice names the person the notice is about.
+//
+// Naming them is not decoration: revoking the grant also takes them off
+// the list below, so an unnamed sentence would refer to somebody the
+// owner can no longer see anywhere on the screen.
+//
+// The member ID travels in the query and the address does not, which is
+// the whole reason this reads the row back. An email address in a
+// request line lands in the platform edge's access logs — the same leak
+// the share-link token was moved out of one commit earlier — while an
+// id is already on this screen in every revoke form's hidden field.
+//
+// Checked rather than trusted: the id came from a URL, so the sentence
+// is personalised only for a member this sheet genuinely still admits.
+func (a *App) revokedTeamNotice(r *http.Request, sheetID uint64) string {
+ id, err := strconv.ParseUint(r.URL.Query().Get("who"), 10, 64)
+ if err != nil || id == 0 {
+ return revokedTeamText
+ }
+ m, ok := a.memberByID(id)
+ if !ok || !a.stillReaches(sheetID, m.ID) {
+ return revokedTeamText
+ }
+ return "The explicit permission is removed, but since " + m.Address +
+ " is on the team, they still have access."
+}
diff --git a/internal/sheets/theme.go b/internal/sheets/theme.go
index ac11d0c..3d855d4 100644
--- a/internal/sheets/theme.go
+++ b/internal/sheets/theme.go
@@ -101,10 +101,27 @@ func (a *App) handleMode(w http.ResponseWriter, r *http.Request) {
//
// A form field that becomes a Location header is an open redirect
// unless it is constrained to a path on this origin. Anything with a
-// scheme, a host, or a leading "//" — which a browser reads as
-// scheme-relative and therefore off-site — is discarded for the root.
+// scheme, a host, or a scheme-relative opening is discarded for the
+// root.
+//
+// Scheme-relative has two spellings and only one of them is obvious.
+// "//host" is read off-site by every browser. So is "/\host": browsers
+// normalise the backslash to a forward slash before resolving, which
+// the URL Standard requires of them. url.Parse does neither — it
+// reports an empty Scheme and an empty Host for the second — so the
+// parse below cannot be what rejects it, and the check has to happen
+// before it.
+//
+// Today `u.RequestURI()` happens to percent-encode that backslash, so
+// the header comes out as a same-origin path and no browser is actually
+// sent anywhere. That is an accident of the encoder, not a decision by
+// this function, and it is one refactor away from not being true. The
+// explicit check is what turns it into a decision.
func safeReturn(raw string) string {
- if raw == "" || !strings.HasPrefix(raw, "/") || strings.HasPrefix(raw, "//") {
+ if raw == "" || raw[0] != '/' {
+ return "/"
+ }
+ if len(raw) > 1 && (raw[1] == '/' || raw[1] == '\\') {
return "/"
}
u, err := url.Parse(raw)
diff --git a/internal/sheets/theme_test.go b/internal/sheets/theme_test.go
index c2c7cdc..2a17d42 100644
--- a/internal/sheets/theme_test.go
+++ b/internal/sheets/theme_test.go
@@ -156,6 +156,14 @@ func TestModeReturnIsNotAnOpenRedirect(t *testing.T) {
"//evil.example.com/",
"http://evil.example.com",
"javascript:alert(1)",
+ // The backslash spelling of the same attack. Browsers
+ // normalise "\" to "/" and read these as scheme-relative,
+ // exactly as they read "//" — but url.Parse does not, so it
+ // reports an empty Scheme and an empty Host and every check
+ // below passes. The check has to be made before the parse.
+ `/\evil.example.com/`,
+ `/\/evil.example.com`,
+ `\\evil.example.com`,
} {
if got := safeReturn(hostile); got != "/" {
t.Errorf("safeReturn(%q) = %q, want %q", hostile, got, "/")