From 78374b2d49c14efc94b009a733420ae55da26c0d Mon Sep 17 00:00:00 2001 From: agent Date: Wed, 9 Sep 2026 09:18:42 -0500 Subject: [PATCH] fix: atomic burn-after-read claim (#58) SoftDelete now reports whether it performed the delete (conditional UPDATE ... WHERE deleted_at IS NULL checked via RowsAffected). RegisterRead returns an admitted flag: legacy burn pastes admit exactly one reader (the atomic soft-delete winner), and burn-after-N pastes increment reads_used via a conditional UPDATE guarded on reads_used < reads_limit, so concurrent readers cannot both consume the final read. API, HTML, and raw read paths return 404 when the reader loses the burn claim; content is never served twice. OpenStore pins the SQLite pool to one connection: concurrent writes on separate pooled connections surfaced SQLITE_BUSY as spurious 500s instead of serializing. Adds concurrency regression tests: 24 parallel readers of a burn paste (exactly one receives content, none of the others leak it) and 30 parallel readers vs a 3-read budget (exactly 3 admitted, then 404). --- internal/api/burnrace_test.go | 103 ++++++++++++++++++++++++++++++++++ internal/api/server.go | 28 ++++++--- internal/store/burn.go | 45 +++++++++++---- internal/store/store.go | 19 ++++++- internal/web/web.go | 6 +- 5 files changed, 178 insertions(+), 23 deletions(-) create mode 100644 internal/api/burnrace_test.go diff --git a/internal/api/burnrace_test.go b/internal/api/burnrace_test.go new file mode 100644 index 0000000..5386787 --- /dev/null +++ b/internal/api/burnrace_test.go @@ -0,0 +1,103 @@ +package api + +import ( + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" +) + +// TestBurnAfterReadConcurrentRace is the #58 regression test: N concurrent +// readers of a burn-after-read paste must receive exactly one success with +// content; every other reader must get 404 and never any content. +func TestBurnAfterReadConcurrentRace(t *testing.T) { + s := testServer(t) + h := s.routes() + id := createBurnReads(t, h, 1) + + const readers = 24 + var wg sync.WaitGroup + var mu sync.Mutex + wins, losses := 0, 0 + for i := 0; i < readers; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + req := httptest.NewRequest("GET", "/api/pastes/"+id, nil) + req.AddCookie(&http.Cookie{Name: "vwr", Value: fmt.Sprintf("racer-%d", i)}) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + mu.Lock() + defer mu.Unlock() + if rec.Code == 200 { + wins++ + var got struct { + Content string `json:"content"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &got); err != nil || got.Content != "limited" { + t.Errorf("winning read returned wrong content: %v %q", err, got.Content) + } + } else if rec.Code == 404 { + losses++ + if strings.Contains(rec.Body.String(), "limited") { + t.Errorf("losing read leaked content: %s", rec.Body.String()) + } + } else { + t.Errorf("unexpected status %d: %s", rec.Code, rec.Body.String()) + } + }(i) + } + wg.Wait() + + if wins != 1 { + t.Fatalf("expected exactly 1 winning read of burn paste, got %d (losses=%d)", wins, losses) + } + if losses != readers-1 { + t.Fatalf("expected %d losing reads, got %d", readers-1, losses) + } +} + +// TestBurnAfterNReadsConcurrentBudget hammers a burn-after-N paste with many +// more concurrent distinct readers than the budget: total admissions must +// equal exactly N, and no losing read may see content. +func TestBurnAfterNReadsConcurrentBudget(t *testing.T) { + s := testServer(t) + h := s.routes() + const budget = 3 + id := createBurnReads(t, h, budget) + + const readers = 30 + var wg sync.WaitGroup + var mu sync.Mutex + wins := 0 + for i := 0; i < readers; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + req := httptest.NewRequest("GET", "/api/pastes/"+id, nil) + req.AddCookie(&http.Cookie{Name: "vwr", Value: fmt.Sprintf("racer-%d", i)}) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + mu.Lock() + defer mu.Unlock() + if rec.Code == 200 { + wins++ + } else if rec.Code != 404 { + t.Errorf("unexpected status %d: %s", rec.Code, rec.Body.String()) + } + }(i) + } + wg.Wait() + + if wins != budget { + t.Fatalf("expected exactly %d admitted reads, got %d", budget, wins) + } + + // After the race, the paste is burned for everyone. + if rec := getWithCookie(t, h, id, "after-the-fact"); rec.Code != 404 { + t.Fatalf("paste should be burned after budget exhausted, got %d", rec.Code) + } +} diff --git a/internal/api/server.go b/internal/api/server.go index f9de994..57f9037 100644 --- a/internal/api/server.go +++ b/internal/api/server.go @@ -29,11 +29,11 @@ type Config struct { } type apiServer struct { - store *store.Store - cfg Config - ui *web.UI - settings *settingsStore - adminKey string + store *store.Store + cfg Config + ui *web.UI + settings *settingsStore + adminKey string } func NewServer(st *store.Store, cfg Config, ui *web.UI, ss *settingsStore, adminKey string) *apiServer { @@ -216,7 +216,12 @@ func (a *apiServer) handleGetPaste(w http.ResponseWriter, r *http.Request) { return } } - rem, _ := a.store.RegisterRead(row, currentViewerID(r), a.burnViewerWindow()) // #49 (also covers legacy burn) + // #58: only the reader that wins the atomic burn claim may see content. + rem, admitted := a.store.RegisterRead(row, currentViewerID(r), a.burnViewerWindow()) + if !admitted { + writeErr(w, 404, "paste not found") + return + } writeJSON(w, 200, map[string]any{ "id": row.ID, "content": row.Content, "content_type": row.ContentType, "language": store.NullStrPtr(row.Language), "title": store.NullStrPtr(row.Title), "created_at": row.CreatedAt, @@ -240,7 +245,7 @@ func (a *apiServer) handleDeletePaste(w http.ResponseWriter, r *http.Request) { writeErr(w, 403, "not your paste") return } - if err := a.store.SoftDelete(row.ID); err != nil { + if _, err := a.store.SoftDelete(row.ID); err != nil { writeErr(w, 500, "db error") return } @@ -319,8 +324,13 @@ func (a *apiServer) handleRaw(w http.ResponseWriter, r *http.Request) { return } // #49 decision: raw reads count against the read budget too, with the - // same per-viewer dedupe window as page views. - a.store.RegisterRead(row, currentViewerID(r), a.burnViewerWindow()) + // same per-viewer dedupe window as page views. #58: a reader that loses + // the burn claim must not receive the content. + _, admitted := a.store.RegisterRead(row, currentViewerID(r), a.burnViewerWindow()) + if !admitted { + http.Error(w, "not found", 404) + return + } // #34: content_type is attacker-controlled via the create API. Serving it // verbatim let a paste be stored with text/html (or image/svg+xml) and // render as active content on this origin when fetched from /raw — diff --git a/internal/store/burn.go b/internal/store/burn.go index bb7f377..8935374 100644 --- a/internal/store/burn.go +++ b/internal/store/burn.go @@ -17,23 +17,34 @@ func genDeletionToken() string { // TimeNow is overridable in tests to inject the clock. var TimeNow = time.Now -// registerRead applies the burn-after-read budget for one view (#49). +// RegisterRead applies the burn-after-read budget for one view (#49). // For pastes with reads_limit set: the viewer's paste_views row is checked; // a view within the burn viewer window of the viewer's last view is deduped // (count=false). Otherwise reads_used is incremented, and the paste is // soft-deleted (burned) once reads_used reaches reads_limit. Viewers without // a cookie (plain API clients) count as their own viewer id "". // For legacy plain burn_after_read pastes (no reads_limit), any read burns. -// Returns the number of reads remaining (0 when burned), or nil when no -// budget is set. view_count is tracked separately and unaffected. -func (s *Store) RegisterRead(row *PasteRow, viewerID string, burnWindowMinutes int) (remaining *int, count bool) { +// #58: admission is atomic. Returns (remaining, admitted). admitted is true +// only when this caller may serve the content: for legacy burn pastes the +// caller wins exactly when its conditional soft delete flipped deleted_at +// (RowsAffected), and for read-budget pastes the caller wins exactly when its +// conditional UPDATE (reads_used < reads_limit) incremented the counter - so +// concurrent readers can never both consume the last read. Losing callers +// must treat the paste as gone. view_count is tracked separately and +// unaffected. +func (s *Store) RegisterRead(row *PasteRow, viewerID string, burnWindowMinutes int) (remaining *int, admitted bool) { if !row.ReadsLimit.Valid { if row.BurnAfterRead { - s.SoftDelete(row.ID) + // #58: atomic claim - only the caller whose UPDATE actually + // flips deleted_at from NULL may serve the content. + ok, err := s.SoftDelete(row.ID) r := 0 + if err != nil || !ok { + return &r, false + } return &r, true } - return nil, false + return nil, true } now := TimeNow().Unix() var last sql.NullInt64 @@ -44,14 +55,28 @@ func (s *Store) RegisterRead(row *PasteRow, viewerID string, burnWindowMinutes i if r < 0 { r = 0 } - return &r, false + return &r, true } s.db.Exec(`INSERT INTO paste_views (paste_id, viewer_id, last_viewed) VALUES (?,?,?) ON CONFLICT(paste_id, viewer_id) DO UPDATE SET last_viewed = excluded.last_viewed`, row.ID, viewerID, now) - used := row.ReadsUsed + 1 - s.db.Exec(`UPDATE pastes SET reads_used=? WHERE id=?`, used, row.ID) - if int64(used) >= row.ReadsLimit.Int64 { + // #58: conditional increment - only succeeds while budget remains, so + // concurrent readers cannot both consume the final read. + res, err := s.db.Exec(`UPDATE pastes SET reads_used = reads_used + 1 + WHERE id = ? AND deleted_at IS NULL AND reads_used < ?`, row.ID, row.ReadsLimit.Int64) + if err != nil { + r := 0 + return &r, false + } + if n, _ := res.RowsAffected(); n == 0 { + // Lost the race: budget exhausted (or paste already burned). + r := 0 + return &r, false + } + var used int64 + s.db.QueryRow(`SELECT reads_used FROM pastes WHERE id = ?`, row.ID).Scan(&used) + if used >= row.ReadsLimit.Int64 { + // Atomic burn; either way the paste is gone for future readers. s.SoftDelete(row.ID) } r := int(row.ReadsLimit.Int64) - int(used) diff --git a/internal/store/store.go b/internal/store/store.go index a323038..0cc1512 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -81,6 +81,11 @@ func OpenStore(path string) (*Store, error) { if err != nil { return nil, err } + // #58: a single write connection. SQLite allows only one writer at a + // time; with multiple pooled connections concurrent writes surface as + // SQLITE_BUSY errors ("database is locked") instead of serializing, and + // the burn-after-read race tests saw spurious 500s under parallel reads. + db.SetMaxOpenConns(1) s := &Store{db: db} if err := s.migrate(); err != nil { return nil, err @@ -320,9 +325,17 @@ func (s *Store) MineOwner(id string) (string, error) { return vid.String, nil } -func (s *Store) SoftDelete(id string) error { - _, err := s.db.Exec(`UPDATE pastes SET deleted_at=? WHERE id=? AND deleted_at IS NULL`, time.Now().Unix(), id) - return err +// SoftDelete marks a paste deleted (burned) atomically (#58): the deleted_at +// IS NULL guard means only the first caller flips the row. Returns true when +// this call performed the delete (RowsAffected > 0), false when the paste was +// already deleted - callers use this to decide read admission atomically. +func (s *Store) SoftDelete(id string) (bool, error) { + res, err := s.db.Exec(`UPDATE pastes SET deleted_at=? WHERE id=? AND deleted_at IS NULL`, time.Now().Unix(), id) + if err != nil { + return false, err + } + n, err := res.RowsAffected() + return n > 0, err } func (s *Store) IncrementViews(id string) { diff --git a/internal/web/web.go b/internal/web/web.go index 59a33e0..9f36e8e 100644 --- a/internal/web/web.go +++ b/internal/web/web.go @@ -255,7 +255,11 @@ func (h *Handlers) HandlePasteView(w http.ResponseWriter, r *http.Request) { // #49: burn-after-N-reads budget (per-viewer dedupe window). // Just-created first render does not count as a read for the creator. if !justCreated { - rem, _ := h.Store.RegisterRead(row, h.ViewerID(r), h.BurnWindowMin()) + rem, admitted := h.Store.RegisterRead(row, h.ViewerID(r), h.BurnWindowMin()) + if !admitted { // #58: lost the burn claim; do not render content + http.NotFound(w, r) + return + } h.renderPaste(w, row, false, "", rem) return } -- 2.54.0