amadan
fix: two findings from an independent review, and the leak that mattered
Reviewed by Codex (codex-cli 0.153.0) on 2026-09-05 — the first review of this branch by a model that did not write it, which is what §Review has been asking for. Three findings; two are fixed here, the third is recorded below. **High — a revoked guest kept receiving live cells.** The stream checked the grant once, at subscribe, and never again. A connection lives up to maxStreamLife and every edit resets the idle timer that would otherwise end it, so a guest whose access was revoked kept receiving every cell written afterwards. The owner had done the one thing that should have stopped it and saw no sign that it had not — the quietest shape a leak can take. The grant is now re-checked before every delivery, and the connection closes with a named reason. One indexed read per delivered message, not per edit on the instance. **Medium — an oversized format body was read from the query instead.** r.FormValue swallows ParseForm's error, including the one MaxBytesReader raises, and Go still fills r.Form from the URL query. So a post whose body was refused had its patch taken from the query and the write landed: the cap was enforced on the body and bypassed by moving the fields. sizeFromForm has always checked ParseForm; formatFromForm had not, so two decoders disagreed about what a malformed form is. Both were reproduced with a failing test before being fixed. The stream test asserts the guest receives an edit BEFORE the revoke, so that silence afterwards means something rather than proving the stream never worked. The third finding — revoking a teammate's explicit grant leaves them with team-standing editor access — is issue #8, already found by Paul and already ruled on. A second reviewer calling it High is new information about a settled decision, so it goes to the discussion rather than getting changed here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018RN4HhHR4rHH3Fxh4LMds5
4 files changed,
+138
−0
internal/sheets/formatform.go+13 −0internal/sheets/formatform_parse_test.go+34 −0internal/sheets/handlers_stream.go+21 −0internal/sheets/stream_revoke_test.go+70 −0
CI passed — run details
diff --git a/internal/sheets/formatform.go b/internal/sheets/formatform.go| index 0dbf5b7..23fc57b 100644 |
| --- a/internal/sheets/formatform.go |
| +++ b/internal/sheets/formatform.go |
| @@ -27,6 +27,19 @@ import ( |
| func formatFromForm(r *http.Request) (formatPatch, bool) { |
| var p formatPatch |
| + // ParseForm's error is checked, and that is not defensive noise. |
| + // r.FormValue swallows it — including the one MaxBytesReader raises |
| + // on an oversized body — and Go still populates r.Form from the URL |
| + // QUERY. So without this, a post whose body was refused had its |
| + // patch read from the query string instead and the write landed: |
| + // the cap was enforced on the body and bypassed by moving the |
| + // fields. sizeFromForm has always checked; this one had not, so the |
| + // two decoders disagreed about what a malformed form is. Found by |
| + // an independent review (Codex, 2026-09-05). |
| + if err := r.ParseForm(); err != nil { |
| + return p, false |
| + } |
| + |
| rect := []struct { |
| name string |
| into *int |
diff --git a/internal/sheets/formatform_parse_test.go b/internal/sheets/formatform_parse_test.go| new file mode 100644 |
| index 0000000..b7e2f3d |
| --- /dev/null |
| +++ b/internal/sheets/formatform_parse_test.go |
| @@ -0,0 +1,34 @@ |
| +package sheets |
| + |
| +import ( |
| + "fmt" |
| + "net/http" |
| + "net/http/httptest" |
| + "strings" |
| + "testing" |
| +) |
| + |
| +// r.FormValue swallows the error from ParseForm — including the one |
| +// MaxBytesReader raises on an oversized body — and Go still populates |
| +// r.Form from the URL QUERY. So a form post whose body is refused can |
| +// have its patch read from the query instead, and the write lands. |
| +// |
| +// sizeFromForm checks ParseForm explicitly; formatFromForm did not, so |
| +// the two decoders disagreed about what a malformed form is. Found by |
| +// an independent review (Codex, 2026-09-05). |
| +func TestAnOversizedFormBodyIsRefusedNotReadFromTheQuery(t *testing.T) { |
| + app, cookies := signIn(t) |
| + s, _ := newSheet(t, app, cookies, "Budget") |
| + |
| + path := fmt.Sprintf( |
| + "/api/sheets/%d/tabs/%d/format?r1=0&c1=0&r2=0&c2=0&bold=set&page=1", |
| + s.ID, s.TabID) |
| + // Well past the handler's 8 KiB cap, so ParseForm fails on the body. |
| + req := httptest.NewRequest(http.MethodPost, path, strings.NewReader("pad="+strings.Repeat("x", 32<<10))) |
| + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") |
| + rec := do(t, app, as(req, cookies)) |
| + |
| + if rec.Code == http.StatusSeeOther || rec.Code == http.StatusOK { |
| + t.Fatalf("an oversized form body was accepted (%d); the patch came from the query", rec.Code) |
| + } |
| +} |
diff --git a/internal/sheets/handlers_stream.go b/internal/sheets/handlers_stream.go| index 937e41c..f9f0eb9 100644 |
| --- a/internal/sheets/handlers_stream.go |
| +++ b/internal/sheets/handlers_stream.go |
| @@ -126,6 +126,27 @@ func (a *App) handleStream(w http.ResponseWriter, r *http.Request) { |
| } |
| case m := <-sub.C(): |
| + // The grant is re-checked before every delivery, not only |
| + // at subscribe. |
| + // |
| + // Authorization on a long-lived connection is a question |
| + // about NOW, and this stream lives for up to |
| + // maxStreamLife with every edit resetting the idle timer |
| + // that would otherwise end it. Checked once, a revoked |
| + // guest keeps receiving cells written after their access |
| + // was taken away — which is the quietest possible leak, |
| + // because the owner has done the thing that should have |
| + // stopped it and sees no sign that it did not. Found by an |
| + // independent review (Codex, 2026-09-05); the gate at open |
| + // was real and ran exactly once. |
| + // |
| + // One indexed read per delivered message, not per edit on |
| + // the instance: a subscriber only reaches this line when a |
| + // message is already addressed to its tab. |
| + if !a.mayRead(sheetID, viewer) { |
| + writeEvent(w, rc, "bye", `{"reason":"access-changed"}`) |
| + return |
| + } |
| if sub.Lost() { |
| // This subscriber overran its buffer and has missed at |
| // least one message, so everything after it would paint |
diff --git a/internal/sheets/stream_revoke_test.go b/internal/sheets/stream_revoke_test.go| new file mode 100644 |
| index 0000000..af6d639 |
| --- /dev/null |
| +++ b/internal/sheets/stream_revoke_test.go |
| @@ -0,0 +1,70 @@ |
| +package sheets |
| + |
| +import ( |
| + "bufio" |
| + "io" |
| + "strings" |
| + "testing" |
| + "time" |
| +) |
| + |
| +// A grant is checked when the stream opens and never again, so a |
| +// revoked guest keeps receiving cells on the connection they already |
| +// hold — for up to maxStreamLife, and every edit resets the idle timer |
| +// that would otherwise end it. |
| +// |
| +// Found by an independent review (Codex, 2026-09-05). It is the quiet |
| +// shape of leak the stream gate was written to prevent: the gate was |
| +// real, and it ran once. |
| +func TestARevokedGuestStopsReceivingCells(t *testing.T) { |
| + app, cookies := signIn(t) |
| + s, cells := newSheet(t, app, cookies, "Budget") |
| + ada := guestOn(t, app, s.ID, "ada@elsewhere.example", SheetViewer) |
| + adaCookies := signInAs(t, app, "ada@elsewhere.example") |
| + |
| + srv := liveServer(t, app) |
| + resp := openStream(t, srv, s, adaCookies) |
| + waitForSubscriber(t, app, s.TabID) |
| + |
| + // Still granted: this edit must arrive. Without it a later silence |
| + // would prove nothing — the stream might simply never have worked. |
| + postCell(t, app, cookies, cells, `{"r":0,"c":0,"raw":"before"}`) |
| + if line := readEventWithin(t, resp.Body, 5*time.Second); !strings.Contains(line, "before") { |
| + t.Fatalf("the guest did not receive the first edit: %q", line) |
| + } |
| + |
| + if err := app.revoke(s.ID, ada.ID); err != nil { |
| + t.Fatalf("revoke: %v", err) |
| + } |
| + postCell(t, app, cookies, cells, `{"r":0,"c":1,"raw":"CONFIDENTIAL"}`) |
| + |
| + if got := readDataOrSilence(resp.Body, 3*time.Second); strings.Contains(got, "CONFIDENTIAL") { |
| + t.Fatalf("a revoked guest received a cell written after the revoke: %q", got) |
| + } |
| +} |
| + |
| +// readDataOrSilence returns the next SSE data line, or "" if the stream |
| +// says nothing in time. |
| +// |
| +// Unlike readEventWithin it does not fail on silence, because silence |
| +// is the passing case here. It reads in a goroutine and abandons it on |
| +// timeout rather than blocking the test. |
| +func readDataOrSilence(body io.Reader, d time.Duration) string { |
| + ch := make(chan string, 1) |
| + go func() { |
| + sc := bufio.NewScanner(body) |
| + for sc.Scan() { |
| + if strings.HasPrefix(sc.Text(), "data: ") { |
| + ch <- sc.Text() |
| + return |
| + } |
| + } |
| + ch <- "" |
| + }() |
| + select { |
| + case line := <-ch: |
| + return line |
| + case <-time.After(d): |
| + return "" |
| + } |
| +} |