amadan
fix: the share-link token no longer transits a URL
Issue oficina/sheets #3, the first of the five Fable findings left open on core and the only one of them that is a trust-model defect rather than a correctness or copy one. `handleLinkCreate` minted a token and handed it to the browser in the redirect: `/share?notice=link&token=<plaintext>`. Everything else about that token is careful — 32 bytes from crypto/rand, only the SHA-256 persisted, unique index, constant-time compare, never in an error string or an app log line — and then the request line put it in the clear into the platform edge's access logs, which AGENTS.md names as storage we cannot grep. The one artefact that opens a sealed sheet to somebody holding nothing else, written where we can neither audit nor redact it. The old comment weighed a cost and picked the wrong one. It named browser history, decided that was smaller than storing the token or never showing it, and was right about those two. The access log is the third option it did not consider, and it is larger than the one it accepted. So the token now reaches the person in the response to their own POST. This is the only POST on the share screen that answers with a page instead of a 303 and the exception is written down where the next reader will find it: the token exists in readable form for exactly that moment, and every way of carrying it through a redirect puts it somewhere we do not control. A server-side stash was the other candidate and is worse here — a redirect landing on another instance would show nothing, and since only the hash is stored the token would be gone with no way to get it back. A duplicate link from a resubmitted form is visible in the list below it and revocable from there. That is the trade: a recoverable, visible cost instead of an invisible, permanent one. `renderShare` takes the token as an argument and no longer reads the query at all, which is what actually closes the channel. While it still echoed a token handed to it, the old URL remained a working way to put one in an access log — a caller only had to build it. Two existing tests moved. `TestTheTokenIsShownOnceAndNotAgain` asserted the token WAS in the Location header, so it pinned the bug; it now asserts the same intent against the response body. `TestTheShareScreen OnlyEchoesItsOwnToken` is deleted rather than adapted, and the reason is in a comment where it stood: with the query path gone, no production change could make it fail. It would have sat there passing and proving nothing — the exact shape this branch keeps finding. make test green, including -race. 🤖 Authored by Claude Opus 5, directed by Paul.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0114wZuXnD6aGcDCzL39NpXD
4 files changed,
+116
−44
internal/sheets/handlers_link.go+19 −7internal/sheets/handlers_share.go+9 −10internal/sheets/sharelink_test.go+15 −27internal/sheets/sharelink_url_test.go+73 −0
CI passed — run details
diff --git a/internal/sheets/handlers_link.go b/internal/sheets/handlers_link.go| index f82b183..a7750dc 100644 |
| --- a/internal/sheets/handlers_link.go |
| +++ b/internal/sheets/handlers_link.go |
| @@ -125,6 +125,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 { |
| @@ -136,13 +154,7 @@ func (a *App) handleLinkCreate(w http.ResponseWriter, r *http.Request) { |
| 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) |
| + 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..a429bbc 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,8 @@ 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 newLink != "" { |
| + view.NewLink = a.linkURL(newLink) |
| } |
| a.render(w, r, "share", "Sharing "+s.Title, http.StatusOK, view) |
| } |
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") |
| + } |
| +} |