amadan
app: fix round 2 on Task 3 -- ApplyUpdate refuses mismatched owners up front
The whole-branch review found that fix round 1's supposed close of Finding 1 was wrong, and so was its verification: ApplyUpdate's doc_updates INSERT (and the MAX(seq) query that sizes it) has always been keyed on doc.ID alone, no owner predicate. Only the trailing updated_at bump was ever scoped through owned(doc.OwnerID), and it runs last -- after content has already been replayed, applied and persisted. A gorm UPDATE matching zero rows returns no error, so a *Document carrying a stranger's OwnerID silently persisted the stranger's content into the real document and returned nil; only the timestamp was refused. Reproduced directly: a tampered OwnerID wrote "hello world" into the victim's log while ApplyUpdate reported success. Fixed by refusing the whole operation before anything runs: ApplyUpdate now re-derives ownership from the database itself rather than trusting doc.OwnerID as handed in. It looks up document doc.ID through the same owned(doc.OwnerID) seam every other query in this file uses and requires the row to exist, before replay, before the core is touched, before any write. A mismatched owner (or owner_id <= 0, via owned's existing "owner <= 0 matches nothing" rule -- unchanged, still applies here) makes that lookup return gorm.ErrRecordNotFound, and ApplyUpdate returns a real error instead of a silent partial success. A *Document is a value a caller can construct or mutate freely; a row in the database is not -- Store.Get is owner-scoped and trustworthy, but nothing at this call site enforced that doc actually came from Get, which was the entire defect. Also fixed the test that was named as proving this: it asserted only that UpdatedAt held still, never that content was refused -- a green test certifying a property nobody wanted, sitting on top of the real bug. Renamed to TestApplyUpdateRefusesAnotherOwnersDocument and rewritten to check all four legs: ApplyUpdate returns an error on mismatch, the victim's content is unchanged (via Store.Text, reading the real CRDT replay), the victim's UpdatedAt is unchanged, and the legitimate owner's own ApplyUpdate still works end to end (so the test can't pass by making ApplyUpdate refuse everything). Verified the new test actually fails against the pre-fix code (temporarily reverted store.go, ran it, watched it fail with "ApplyUpdate returned nil for a mismatched owner"; restored the fix, watched it pass). Also closed a narrower related oracle in gateBegin (auth.go): Begin auto-upgrades a permitted, keymail-hosted address to a keymail-URL redirect instead of the plain ?sent=1 every other permitted address gets, so a non-permitted address (?sent=1) was distinguishable from a permitted keymail one -- a membership oracle one level narrower than the one gateBegin already closes. This app never wants the keymail ceremony at all (spec Sec8.1; signin.html's form already hardcodes a hidden force=1 for exactly this reason), so gateBegin now sets force=1 unconditionally on every permitted request, matching what the UI already always sends -- no behavior change for any real caller, and the keymail branch (and its distinct response) is never reached again. Full CI gate (.amadan/ci.d/10-gate) passes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011F5CivVm4CmL9ByfSSAaXA
3 files changed,
+134
−28
internal/docs/auth.go+31 −0internal/docs/store.go+47 −13internal/docs/store_test.go+56 −15
CI passed — run details
diff --git a/internal/docs/auth.go b/internal/docs/auth.go| index bfc5ff1..7fff498 100644 |
| --- a/internal/docs/auth.go |
| +++ b/internal/docs/auth.go |
| @@ -232,6 +232,33 @@ func resolveAllowlist(cfg Config) (allowlist, error) { |
| // TestSigninResponseIdenticalForNonPermittedAddress (auth_test.go), |
| // which asserts the two responses byte-for-byte, not merely "both are |
| // 303". |
| +// |
| +// That guarantee only held for a non-keymail address: on the |
| +// permitted branch, this used to hand the request to Begin exactly as |
| +// received, and Begin auto-upgrades a permitted address that resolves |
| +// to a claimed keymail inbox to the keymail ceremony — a 303 to the |
| +// keymail authorize URL, not signinPath+"?sent=1". A non-permitted |
| +// address never reaches Begin at all, so it always got ?sent=1. That |
| +// reopened the same oracle one level narrower: an attacker who knows |
| +// (or suspects) an address is keymail-hosted could tell "on the |
| +// allowlist" from "not" by whether the response redirected to keymail |
| +// or to ?sent=1. |
| +// |
| +// force=1 is what selects the plain-magic-link path over that |
| +// classification (rastrillo/auth's handlers.go), and this app never |
| +// wants the keymail ceremony in the first place — spec §8.1's ruling, |
| +// this file's own package comment, and signin.html's form, which |
| +// already hardcodes a hidden force=1 field so no request built by |
| +// this app's own UI ever takes the keymail branch. Setting it here |
| +// too, unconditionally, on every permitted request — regardless of |
| +// what the caller's own form said — makes that the actual guarantee |
| +// for any request this handler ever forwards, not just the ones sent |
| +// through this app's form: every permitted address takes the identical |
| +// plain-magic-link path Begin would take for force=1, so the keymail |
| +// branch is never reached and never has a distinct response to leak |
| +// through. This costs nothing a legitimate caller wanted: it matches |
| +// what the UI already always sends, and it is not a behavior change |
| +// this app would ever notice. |
| func gateBegin(next http.HandlerFunc, list allowlist, signinPath string) http.HandlerFunc { |
| return func(w http.ResponseWriter, r *http.Request) { |
| // A malformed body (ParseForm's error) reads back as address |
| @@ -242,6 +269,10 @@ func gateBegin(next http.HandlerFunc, list allowlist, signinPath string) http.Ha |
| // mail could be sent over it. |
| _ = r.ParseForm() |
| if list.allows(r.FormValue("address")) { |
| + if r.Form == nil { |
| + r.Form = url.Values{} |
| + } |
| + r.Form.Set("force", "1") |
| next(w, r) |
| return |
| } |
diff --git a/internal/docs/store.go b/internal/docs/store.go| index 999969e..fde79d1 100644 |
| --- a/internal/docs/store.go |
| +++ b/internal/docs/store.go |
| @@ -326,7 +326,45 @@ func (s *Store) Text(ctx context.Context, doc *Document) (string, error) { |
| // persisted — see the errors.Is-friendly wrapping in replay's doc |
| // comment and Handle.Apply's for why an uncertain outcome must not be |
| // written down as a fact. |
| +// |
| +// Before any of that, doc.OwnerID is verified against the row itself. |
| +// Fix round 2 on this task (Finding 1 again, this time for real): the |
| +// previous fix scoped only the trailing updated_at bump through |
| +// owned(doc.OwnerID), on the theory that it was "the one write in this |
| +// file that skipped the seam." That was wrong — the doc_updates INSERT |
| +// above (and the MAX(seq) query that sizes it) is keyed on doc.ID |
| +// alone and always has been, and it runs long before the bump. Scoping |
| +// only the last write meant a *Document carrying a stranger's OwnerID |
| +// still replayed, applied and persisted the content; only the |
| +// timestamp was refused, and a gorm UPDATE matching zero rows returns |
| +// no error, so the whole call returned nil. Content wrote, silently, |
| +// on a mismatched owner. |
| +// |
| +// The honest fix is not to scope some subset of the writes below — it |
| +// is to refuse the entire operation before any of them run. doc is a |
| +// value a caller can construct or mutate freely (as the regression |
| +// test below does); it is not a database guarantee. Store.Get is |
| +// owner-scoped, so doc.OwnerID is trustworthy *when doc came from |
| +// Get* — but nothing at this call site enforces that it did, which is |
| +// the entire defect. So rather than trust the struct, this re-derives |
| +// ownership from the database itself: look up document doc.ID through |
| +// the same owned(doc.OwnerID) seam every other query in this file |
| +// uses, and require the row to exist. owned's owner <= 0 rule (see |
| +// its own doc comment) still applies unchanged here: an orphaned |
| +// document with owner_id 0 cannot be "owned" by any doc.OwnerID, |
| +// including a tampered 0, so it can never pass this check and this |
| +// route can never make an orphan writable. A mismatched or missing |
| +// owner makes the Where match zero rows, First returns |
| +// gorm.ErrRecordNotFound, and this method returns before replay is |
| +// even called — no core touched, no row written. |
| func (s *Store) ApplyUpdate(ctx context.Context, doc *Document, update []byte) error { |
| + var ownerCheck Document |
| + if err := owned(s.db.WithContext(ctx), doc.OwnerID). |
| + Where("id = ?", doc.ID). |
| + First(&ownerCheck).Error; err != nil { |
| + return fmt.Errorf("docs: apply update to document %d: verify owner %d: %w", doc.ID, doc.OwnerID, err) |
| + } |
| + |
| h, err := s.replay(ctx, doc) |
| if err != nil { |
| return err |
| @@ -365,19 +403,15 @@ func (s *Store) ApplyUpdate(ctx context.Context, doc *Document, update []byte) e |
| // identical and the divergence stayed invisible until something |
| // — this method — actually moved UpdatedAt away from CreatedAt. |
| // |
| - // Routed through owned(doc.OwnerID) — fix round 1 on this task: this |
| - // used to be a bare `Where("id = ?", doc.ID)`, the one write in this |
| - // file that skipped the seam. It was safe only by accident, and only |
| - // today: the sole way to obtain a *Document at all is Store.Get, |
| - // which is already owner-scoped, so doc.OwnerID here always matches |
| - // the caller who legitimately holds doc. But nothing enforced that |
| - // invariant at this call site, and the editing plan is exactly what |
| - // adds a route that calls ApplyUpdate — whoever writes it would have |
| - // copied this line as the established pattern for "how this file |
| - // writes to documents," carrying the gap into a place where it can |
| - // actually be reached. SKILL.md §3, verbatim: "Scope the write, not |
| - // just the read that loaded the row — the SQL must carry |
| - // `WHERE user_id = ? AND id = ?`." |
| + // Still routed through owned(doc.OwnerID), same as every other |
| + // write in this method now — ownership was already verified above |
| + // before replay ran, so by this point the row is guaranteed to |
| + // exist under doc.OwnerID and this can never match zero rows. It |
| + // stays scoped anyway rather than falling back to a bare |
| + // `Where("id = ?", doc.ID)`: SKILL.md §3, verbatim, "Scope the |
| + // write, not just the read that loaded the row," and a defense |
| + // that only holds because of a check earlier in the same function |
| + // is one accidental reordering away from not holding at all. |
| if err := owned(s.db.WithContext(ctx), doc.OwnerID). |
| Model(&Document{}). |
| Where("id = ?", doc.ID). |
diff --git a/internal/docs/store_test.go b/internal/docs/store_test.go| index b7c0c16..b4bbf7a 100644 |
| --- a/internal/docs/store_test.go |
| +++ b/internal/docs/store_test.go |
| @@ -186,19 +186,36 @@ func TestUnownedDocumentOverHTTPIs404Not403(t *testing.T) { |
| } |
| } |
| -// TestApplyUpdateDoesNotBumpAnotherOwnersDocument is fix round 1's own |
| -// regression test (Finding 1): ApplyUpdate's updated_at bump used to |
| -// run `UPDATE documents SET updated_at WHERE id = ?` with no owner |
| -// predicate at all — the one write in store.go that skipped the |
| -// owned() seam. It was safe only because the sole way to obtain a |
| -// *Document is Store.Get, which is already owner-scoped, so in |
| -// practice doc.OwnerID always matched. Nothing enforced that |
| -// invariant at the call site, so this simulates the case where it |
| -// doesn't hold — a *Document whose OwnerID has been tampered with to |
| -// someone else's id — and asserts the write is refused: the real |
| -// row's UpdatedAt must not move. SKILL.md §3, verbatim: "Scope the |
| -// write, not just the read that loaded the row." |
| -func TestApplyUpdateDoesNotBumpAnotherOwnersDocument(t *testing.T) { |
| +// TestApplyUpdateRefusesAnotherOwnersDocument is fix round 2's own |
| +// regression test (Finding 1, corrected): fix round 1's version of |
| +// this test was named as if it proved the write was refused, but it |
| +// only ever asserted that UpdatedAt held still — a green test |
| +// certifying a property nobody wanted, while the actual defect (the |
| +// content write) went unchecked and unfixed underneath it. |
| +// |
| +// The real bug: ApplyUpdate's doc_updates INSERT (and the MAX(seq) |
| +// query that sizes it) has always been keyed on doc.ID alone, with no |
| +// owner predicate at all. Only the trailing updated_at bump was ever |
| +// scoped through owned(doc.OwnerID), and it runs last — long after |
| +// the content has already been replayed, applied and persisted. A |
| +// gorm UPDATE matching zero rows returns no error, so a *Document |
| +// carrying a stranger's OwnerID used to replay and persist the |
| +// stranger's content into the real owner's document, then return nil: |
| +// only the timestamp was refused, silently, while "hello world" |
| +// landed in the victim's log. |
| +// |
| +// This asserts the actual contract: a mismatched owner must refuse |
| +// the whole operation, up front, with a real error — not partially |
| +// apply and then fail silently on the last write. It checks all four |
| +// legs: the call returns an error, the victim's content never changes |
| +// (via Store.Text, reading the real CRDT replay — not a column that |
| +// could look right while the log underneath it is wrong), the |
| +// victim's UpdatedAt never moves (fix round 1's original property, |
| +// kept), and — so this test cannot pass by making ApplyUpdate refuse |
| +// everything — the legitimate owner's own ApplyUpdate still works end |
| +// to end. SKILL.md §3, verbatim: "Scope the write, not just the read |
| +// that loaded the row." |
| +func TestApplyUpdateRefusesAnotherOwnersDocument(t *testing.T) { |
| app, err := docs.NewForTest(t) |
| if err != nil { |
| t.Fatalf("NewForTest: %v", err) |
| @@ -214,6 +231,10 @@ func TestApplyUpdateDoesNotBumpAnotherOwnersDocument(t *testing.T) { |
| if err != nil { |
| t.Fatalf("Get (before): %v", err) |
| } |
| + textBefore, err := app.Store().Text(ctx, before) |
| + if err != nil { |
| + t.Fatalf("Text (before): %v", err) |
| + } |
| // A real gap so a spurious bump would be visible against the |
| // clock, not hidden by two timestamps landing in the same tick. |
| @@ -223,16 +244,36 @@ func TestApplyUpdateDoesNotBumpAnotherOwnersDocument(t *testing.T) { |
| tampered.OwnerID = owner + 1_000_000 // guaranteed to belong to nobody |
| update := docs.ReadFixture(t, "hello") |
| - if err := app.Store().ApplyUpdate(ctx, &tampered, update); err != nil { |
| - t.Fatalf("ApplyUpdate: %v", err) |
| + if err := app.Store().ApplyUpdate(ctx, &tampered, update); err == nil { |
| + t.Fatal("ApplyUpdate returned nil for a mismatched owner — want a real error, before anything is written") |
| } |
| after, err := app.Store().Get(ctx, owner, doc.Sqid) |
| if err != nil { |
| t.Fatalf("Get (after): %v", err) |
| } |
| + textAfter, err := app.Store().Text(ctx, after) |
| + if err != nil { |
| + t.Fatalf("Text (after): %v", err) |
| + } |
| + if textAfter != textBefore { |
| + t.Errorf("victim document's content changed from %q to %q — the stranger's update landed despite the mismatched owner", textBefore, textAfter) |
| + } |
| if !after.UpdatedAt.Equal(before.UpdatedAt) { |
| t.Errorf("UpdatedAt moved from %v to %v even though ApplyUpdate was called with a mismatched owner (%d instead of the document's real owner %d)", |
| before.UpdatedAt, after.UpdatedAt, tampered.OwnerID, owner) |
| } |
| + |
| + // Prove the fix does not break the working path: the legitimate |
| + // owner's own ApplyUpdate must still apply end to end. |
| + if err := app.Store().ApplyUpdate(ctx, doc, update); err != nil { |
| + t.Fatalf("ApplyUpdate (legitimate owner): %v", err) |
| + } |
| + legit, err := app.Store().Text(ctx, doc) |
| + if err != nil { |
| + t.Fatalf("Text (legitimate owner): %v", err) |
| + } |
| + if legit != "hello world" { |
| + t.Errorf("legitimate owner's ApplyUpdate did not persist: Text = %q, want %q", legit, "hello world") |
| + } |
| } |