amadan

oficina / docs Public

app: fix round 1 on document ownership — scope the write, refuse owner 0

Four findings from review of "documents belong to people":

ApplyUpdate's updated_at bump was the one write in store.go that
skipped the owned() seam (bare WHERE id = ?, no owner predicate) --
safe today only because a *Document can only arrive from an
already-scoped Get, and exactly the line the editing plan's next
route would have copied. Routed through owned(doc.OwnerID) instead,
with a regression test proving a mismatched owner no longer bumps it.

owned() now refuses owner <= 0 itself (matches nothing) rather than
trusting every caller to reject it first -- people.id starts at 1, so
0 is never a real person, and it is exactly the value 0002_owners.sql
backfills onto pre-ownership documents.

That backfill's DEFAULT 0 -- permanently orphaning any document that
predates ownership -- is now a documented decision in the migration
itself, not an accidental side effect, and pinned by a test asserting
such a document is visible to nobody, owner 0 included.

owned()'s doc comment now says why it deliberately omits deleted_at:
folding it in would make the function do two unrelated jobs under one
name and foreclose a future caller that wants every row regardless of
deletion state.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011F5CivVm4CmL9ByfSSAaXA
Paul Campbell pushed by paul@keymail.dev 30a148e6dadf5143197582595e71eb91ca64c0c1 parent a7e1e9d
4 files changed, +238 −11
  • internal/docs/migrations/0002_owners.sql +12 −0
  • internal/docs/scope_test.go +129 −6
  • internal/docs/store.go +46 −5
  • internal/docs/store_test.go +51 −0

Intent

full ledger: signin-and-samples

diff --git a/internal/docs/migrations/0002_owners.sql b/internal/docs/migrations/0002_owners.sql
index 4f47586..b8c60af 100644
--- a/internal/docs/migrations/0002_owners.sql
+++ b/internal/docs/migrations/0002_owners.sql
@@ -4,6 +4,18 @@ CREATE TABLE IF NOT EXISTS people (
created_at DATETIME NOT NULL
);
+-- DEFAULT 0 is a deliberate decision, not a placeholder: SQLite's
+-- INTEGER PRIMARY KEY on people.id starts at 1, so owner_id = 0 can
+-- never match a real person. Any document that existed before this
+-- migration ran is backfilled to owner_id = 0 and becomes permanently
+-- invisible through Store (store.go's owned() explicitly matches
+-- nothing for owner <= 0, so no future caller can accidentally expose
+-- these rows either) -- there is no live deployment yet (this plan is
+-- what takes Docs live for the first time), so today that set is
+-- empty in practice, but the behaviour is real and permanent for
+-- anyone who runs this migration against a database that already has
+-- rows: those documents are orphaned on purpose, not lost by oversight.
+-- Pinned by TestOrphanedDocumentsAreVisibleToNobody in scope_test.go.
ALTER TABLE documents ADD COLUMN owner_id INTEGER NOT NULL DEFAULT 0;
CREATE INDEX IF NOT EXISTS idx_documents_owner_id ON documents (owner_id);
diff --git a/internal/docs/scope_test.go b/internal/docs/scope_test.go
index 9387b7c..cb5377a 100644
--- a/internal/docs/scope_test.go
+++ b/internal/docs/scope_test.go
@@ -3,6 +3,7 @@ package docs
import (
"context"
"testing"
+ "time"
)
// TestOwnedScopesByOwner tests store.go's owned function directly —
@@ -53,15 +54,137 @@ func TestOwnedScopesByOwner(t *testing.T) {
t.Errorf("owned(bob) returned %d row(s), want 0 — bob does not own alice's document", len(bobRows))
}
- // An owner nobody has ever used (no Person row, no document) is
- // scoped to nothing, not an error — the same shape a stranger
- // probing gets, never a distinct failure mode of its own.
+ // Owner 0 is scoped to nothing — not an unused id that merely
+ // never happens to match a row (999999 would prove that, but is a
+ // value that never existed), but the one real value
+ // 0002_owners.sql's migration actually writes onto every
+ // pre-ownership document, and the one owned() refuses
+ // structurally regardless of what the table holds (fix round 1,
+ // Finding 2). TestOwnedRejectsOwnerZero and
+ // TestOrphanedDocumentsAreVisibleToNobody below cover the same
+ // refusal against a row that genuinely has owner_id = 0; this
+ // assertion holds even with no such row present at all.
var noRows []*Document
- if err := owned(store.db.WithContext(ctx), 999999).Find(&noRows).Error; err != nil {
- t.Fatalf("owned(unknown owner): %v", err)
+ if err := owned(store.db.WithContext(ctx), 0).Find(&noRows).Error; err != nil {
+ t.Fatalf("owned(0): %v", err)
}
if len(noRows) != 0 {
- t.Errorf("owned(unknown owner) returned %d row(s), want 0", len(noRows))
+ t.Errorf("owned(0) returned %d row(s), want 0", len(noRows))
+ }
+}
+
+// TestOwnedRejectsOwnerZero is fix round 1's own regression test
+// (Finding 2): a reviewer found owned(g, 0), Store.List(ctx, 0) and
+// Store.Get(ctx, 0, ...) all happily returned a row whose owner_id is
+// 0 — exactly what 0002_owners.sql's DEFAULT 0 backfill produces for
+// a document that predates ownership (see that migration's own
+// comment). This inserts such a row directly, bypassing Store.Create
+// (which only ever receives a real Person id from its caller), to
+// reproduce that exact shape, then confirms owned(g, 0) still matches
+// nothing even though a matching row genuinely exists. Negative
+// owners are checked too — the same "not a real person" reasoning,
+// not a special case for the literal value 0.
+func TestOwnedRejectsOwnerZero(t *testing.T) {
+ app, err := NewForTest(t)
+ if err != nil {
+ t.Fatalf("NewForTest: %v", err)
+ }
+ ctx := context.Background()
+ store := app.Store()
+
+ now := time.Now().UTC()
+ orphan := &Document{
+ Sqid: "orphanzero1",
+ Title: "Pre-ownership document",
+ OwnerID: 0,
+ GUID: "orphan-guid-zero",
+ ClientID: 1,
+ CreatedAt: now,
+ UpdatedAt: now,
+ }
+ if err := store.db.WithContext(ctx).Create(orphan).Error; err != nil {
+ t.Fatalf("insert orphaned document: %v", err)
+ }
+
+ var zeroRows []*Document
+ if err := owned(store.db.WithContext(ctx), 0).Find(&zeroRows).Error; err != nil {
+ t.Fatalf("owned(0): %v", err)
+ }
+ if len(zeroRows) != 0 {
+ t.Errorf("owned(0) returned %d row(s), want 0 — owner 0 is never a real person, even when a row with owner_id = 0 exists", len(zeroRows))
+ }
+
+ var negRows []*Document
+ if err := owned(store.db.WithContext(ctx), -1).Find(&negRows).Error; err != nil {
+ t.Fatalf("owned(-1): %v", err)
+ }
+ if len(negRows) != 0 {
+ t.Errorf("owned(-1) returned %d row(s), want 0", len(negRows))
+ }
+}
+
+// TestOrphanedDocumentsAreVisibleToNobody is fix round 1's own
+// regression test (Finding 3): 0002_owners.sql's DEFAULT 0 backfill
+// is a deliberate decision — a document that existed before ownership
+// did becomes permanently invisible — but that property was
+// previously asserted nowhere. This pins it through the same Store
+// API a real request uses (List and Get), not just owned() directly:
+// an orphaned document must not appear in anyone's list, and must not
+// be fetchable by anyone via its sqid, exactly as if it did not
+// exist — spec §4.2's own contract, applied to the migration's own
+// backfill rather than only to a document someone else owns.
+func TestOrphanedDocumentsAreVisibleToNobody(t *testing.T) {
+ app, err := NewForTest(t)
+ if err != nil {
+ t.Fatalf("NewForTest: %v", err)
+ }
+ ctx := context.Background()
+ store := app.Store()
+
+ now := time.Now().UTC()
+ orphan := &Document{
+ Sqid: "orphanzero2",
+ Title: "Another pre-ownership document",
+ OwnerID: 0,
+ GUID: "orphan-guid-visible",
+ ClientID: 2,
+ CreatedAt: now,
+ UpdatedAt: now,
+ }
+ if err := store.db.WithContext(ctx).Create(orphan).Error; err != nil {
+ t.Fatalf("insert orphaned document: %v", err)
+ }
+
+ alice, err := store.EnsurePerson(ctx, "alice-orphan-check@example.com")
+ if err != nil {
+ t.Fatalf("EnsurePerson(alice): %v", err)
+ }
+ bob, err := store.EnsurePerson(ctx, "bob-orphan-check@example.com")
+ if err != nil {
+ t.Fatalf("EnsurePerson(bob): %v", err)
+ }
+
+ for _, owner := range []int64{alice, bob} {
+ list, err := store.List(ctx, owner)
+ if err != nil {
+ t.Fatalf("List(%d): %v", owner, err)
+ }
+ for _, d := range list {
+ if d.Sqid == orphan.Sqid {
+ t.Errorf("owner %d's list contains the orphaned document %q", owner, orphan.Sqid)
+ }
+ }
+
+ if _, err := store.Get(ctx, owner, orphan.Sqid); err == nil {
+ t.Errorf("owner %d fetched the orphaned document %q", owner, orphan.Sqid)
+ }
+ }
+
+ // And owner 0 itself — the value the row actually carries — must
+ // not see it either; owned() refuses it outright rather than
+ // treating it as "the orphan's own owner."
+ if _, err := store.Get(ctx, 0, orphan.Sqid); err == nil {
+ t.Error("owner 0 fetched the orphaned document — owned() must refuse owner 0 unconditionally")
}
}
diff --git a/internal/docs/store.go b/internal/docs/store.go
index 66cc8ef..999969e 100644
--- a/internal/docs/store.go
+++ b/internal/docs/store.go
@@ -86,11 +86,38 @@ const maxSqidAttempts = 5
// pins its current, ownership-only contract so a future edit that
// widens it does so on purpose.
//
-// It does not filter deleted_at: that is not an ownership concern, and
-// callers that care (List, Get) add it themselves, deliberately
-// visible at the call site rather than folded into a function named
-// for something else.
+// owner <= 0 matches nothing, deliberately, rather than trusting every
+// caller to reject it first. people.id starts at 1 (SQLite's INTEGER
+// PRIMARY KEY), so 0 is never a real person — it is exactly the value
+// 0002_owners.sql's migration backfilled onto every document that
+// existed before ownership did (see that file's own comment, and
+// TestOrphanedDocumentsAreVisibleToNobody in scope_test.go, which
+// pins this). Fix round 1 on this task: a reviewer found owned(g, 0),
+// Store.List(ctx, 0) and Store.Get(ctx, 0, ...) all happily returned
+// those orphaned rows — nothing upstream exploited that yet, but nothing
+// prevented a careless future caller from doing so either, and the
+// seam is the one place that can make it structurally impossible
+// rather than merely unexercised today. Matching nothing (a `1 = 0`
+// clause) rather than returning an error keeps every call site's
+// chaining (.Where/.Order/.Find) unchanged, and gives owner 0 the same
+// outcome as any other owner with no visible rows — no distinct error
+// shape for "invalid owner" to leak through a 500 or otherwise
+// distinguish itself from "you just have nothing here" (spec §4.2's
+// own reasoning against a distinguishable failure mode, applied here
+// to the seam itself rather than only to the 404-vs-403 handler
+// boundary).
+//
+// It does not filter deleted_at: that is not an ownership concern.
+// Folding it in here would make `owned` do two unrelated jobs under
+// one name, and would silently change behaviour for any future caller
+// that wants every one of an owner's rows regardless of deletion state
+// (an admin or export path, say) without ever needing to touch this
+// function. List and Get add `deleted_at IS NULL` themselves,
+// deliberately visible at the call site.
func owned(g *gorm.DB, owner int64) *gorm.DB {
+ if owner <= 0 {
+ return g.Where("1 = 0")
+ }
return g.Where("owner_id = ?", owner)
}
@@ -337,7 +364,21 @@ func (s *Store) ApplyUpdate(ctx context.Context, doc *Document, update []byte) e
// to the same `now`), so List ordering by either column looked
// identical and the divergence stayed invisible until something
// — this method — actually moved UpdatedAt away from CreatedAt.
- if err := s.db.WithContext(ctx).
+ //
+ // 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 = ?`."
+ if err := owned(s.db.WithContext(ctx), doc.OwnerID).
Model(&Document{}).
Where("id = ?", doc.ID).
Update("updated_at", time.Now().UTC()).Error; err != nil {
diff --git a/internal/docs/store_test.go b/internal/docs/store_test.go
index d349346..b7c0c16 100644
--- a/internal/docs/store_test.go
+++ b/internal/docs/store_test.go
@@ -185,3 +185,54 @@ func TestUnownedDocumentOverHTTPIs404Not403(t *testing.T) {
t.Errorf("status %d, want 404 — a document you cannot see does not exist (spec §4.2)", rec.Code)
}
}
+
+// 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) {
+ app, err := docs.NewForTest(t)
+ if err != nil {
+ t.Fatalf("NewForTest: %v", err)
+ }
+ ctx := context.Background()
+ owner := app.TestPerson(t, "owner@example.com")
+
+ doc, err := app.Store().Create(ctx, owner, "Real owner's document")
+ if err != nil {
+ t.Fatalf("Create: %v", err)
+ }
+ before, err := app.Store().Get(ctx, owner, doc.Sqid)
+ if err != nil {
+ t.Fatalf("Get (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.
+ time.Sleep(5 * time.Millisecond)
+
+ tampered := *doc
+ 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)
+ }
+
+ after, err := app.Store().Get(ctx, owner, doc.Sqid)
+ if err != nil {
+ t.Fatalf("Get (after): %v", err)
+ }
+ 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)
+ }
+}