amadan
fix: undo looks past an event it cannot unseal
Issue oficina/sheets #6, the third of the five Fable findings left open on core. `handleUndo` asked `store.History` for `Limit: 1` and took what came back. But History skips an event it cannot open — deliberately, so one unreadable row cannot take a whole history view with it — so a single unreadable newest edit returned an empty set, and undo reported that the person had nothing to undo while readable older edits of theirs sat directly behind it. The shape is what makes it worth fixing rather than noting. It does not surface as an error. It surfaces as a calm, correct-sounding answer about the person's own work, delivered to somebody who has just made a mistake and is trying to take it back. Undo now asks for a window and takes the newest record that survives unsealing. The window is bounded and the bound is the interesting part: open-ended, a tab full of undecryptable events turns one undo into a scan of the entire log, unsealing every row on the way. Twenty is a judgement and the comment says so — far enough to cover the case this exists for, an event or two under a key we no longer hold, short enough that the pathological tab costs twenty AEAD opens and stops. Past that undo reports nothing to undo, which by then is true enough. The test corrupts a real event's sealed payload rather than mocking the sealer, so it exercises the same not-ok path a key rotation would produce, and asserts on the cell afterwards rather than on the notice — the notice is what was wrong, so trusting it to report the fix would be asking the defect to grade itself. 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
2 files changed,
+64
−2
internal/sheets/handlers_history.go+19 −2internal/sheets/handlers_history_test.go+45 −0
CI passed — run details
diff --git a/internal/sheets/handlers_history.go b/internal/sheets/handlers_history.go| index c3948d9..b369e47 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. |