amadan
fix: a grant was a way round the does-this-sheet-exist check
Second gpt-5.6-sol pass, again REQUEST CHANGES, and it found a real one the first fix had walked straight past. **roleFor honoured a grant without asking whether the sheet existed.** The check sat in the team-standing branch alone, so an explicit Grant row skipped it entirely. Every import mints an owner grant for whoever started it, and openTab and the stream gate on roleFor and nothing else — so the importer had direct read and WRITE access, over the grid JSON API and SSE, to a sheet no list would show. Hiding the sheet from visibleSheets and sheetSummary had closed the front door and left this one open. It predates import: the same hole meant a grant holder kept their role on a soft-deleted sheet. The check moved to the top of roleFor, where both sources of access pass through it, so a route added later cannot forget it. Two tests, one per shape, both 200-instead-of-404 before it. **Stale imports are now reclaimed.** A sheet still marked filling when a process starts is stale by construction — the jobs registry is in-memory, so the goroutine that was filling it died with the process that held it — which makes boot the one moment a sweep cannot race live work. Hidden was already the safe state, so this is reclamation, not correctness. **Two comments corrected rather than defended.** UnzipXMLSizeLimit is not a second rejection threshold: excelize uses it to decide when to stream a part through temporary storage. UnzipSizeLimit is the hard bound and the one doing the work. **Two of my own tests were weaker than they read.** One asserted admission through the allowlist rather than through the grant it claimed to be testing, and the deleted-sheet case would have stayed green with the filling condition removed. Both fixed; the SSE assertion also grew a deadline, because the regression shape there is a hang rather than a failure. The seventh finding stays rejected, and the reviewer now agrees: the cap is on Go files and JS modules, and no 300-line CSS rule exists.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H6YuJ2QG6koRKjNf3ddr1Q
6 files changed,
+210
−7
internal/sheets/app.go+5 −0internal/sheets/grants.go+15 −3internal/sheets/grants_sheetstate_test.go+93 −0internal/sheets/handlers_import.go+28 −0internal/sheets/handlers_import_hidden_test.go+60 −1internal/sheets/xlsx/import.go+9 −3
diff --git a/internal/sheets/app.go b/internal/sheets/app.go| index 6dd6071..ebac371 100644 |
| --- a/internal/sheets/app.go |
| +++ b/internal/sheets/app.go |
| @@ -103,6 +103,11 @@ func New(cfg Config) *App { |
| panic("sheets: wiring job handlers: " + err.Error()) |
| } |
| a.jobPages = h |
| + // Any sheet still marked mid-import belongs to a process that is |
| + // gone, so this is the moment to reclaim them — before this one |
| + // starts an import of its own and the "stale by construction" |
| + // reasoning stops holding. See sweepStaleImports. |
| + a.sweepStaleImports() |
| return a |
| } |
diff --git a/internal/sheets/grants.go b/internal/sheets/grants.go| index fcd316f..ffbbf01 100644 |
| --- a/internal/sheets/grants.go |
| +++ b/internal/sheets/grants.go |
| @@ -37,6 +37,16 @@ func (a *App) roleFor(sheetID uint64, m Member) (SheetRole, bool) { |
| if a.db == nil { |
| return "", false |
| } |
| + // FIRST, covering both sources below. This check used to sit in the |
| + // team-standing branch alone, which made an explicit Grant row a |
| + // way round it: a grant holder kept their role on a soft-deleted |
| + // sheet, and — once import existed — on one still being filled. |
| + // openTab and the stream gate on roleFor and nothing else, so that |
| + // was read and WRITE access to a sheet no list would show, for the |
| + // one person every import gives an owner grant to. |
| + if !a.sheetExists(sheetID) { |
| + return "", false |
| + } |
| var g Grant |
| err := a.db.Where("sheet_id = ? AND member_id = ?", sheetID, m.ID).First(&g).Error |
| if err == nil { |
| @@ -58,9 +68,11 @@ func (a *App) roleFor(sheetID uint64, m Member) (SheetRole, bool) { |
| // owner: sheet ownership is about who may change sharing, and |
| // handing that to every colleague by default makes the grant |
| // table decorative. |
| - if a.sheetExists(sheetID) { |
| - return SheetEditor, true |
| - } |
| + // |
| + // The sheet is already known to exist — that is checked at the |
| + // top, for both sources. It was checked here instead, once, and |
| + // the grant path above went unguarded as a result. |
| + return SheetEditor, true |
| } |
| return "", false |
| } |
diff --git a/internal/sheets/grants_sheetstate_test.go b/internal/sheets/grants_sheetstate_test.go| new file mode 100644 |
| index 0000000..b4fbb6b |
| --- /dev/null |
| +++ b/internal/sheets/grants_sheetstate_test.go |
| @@ -0,0 +1,93 @@ |
| +package sheets |
| + |
| +// roleFor is the single place both sources of access to a sheet are |
| +// decided — an explicit grant, and team standing — so it is also the |
| +// only place the "does this sheet exist for anybody" question has to |
| +// be asked. These cover the two ways it can be wrong. |
| + |
| +import ( |
| + "context" |
| + "net/http" |
| + "net/http/httptest" |
| + "strconv" |
| + "testing" |
| + "time" |
| + |
| + "amadan.net/oficina/sheets/internal/sheets/store" |
| +) |
| + |
| +// TestAFillingSheetIsClosedToItsOwnImporter is the blocker the |
| +// follow-up review found: every import mints an explicit owner grant |
| +// for the person who started it, and roleFor used to hand back that |
| +// grant's role without ever asking whether the sheet was one anybody |
| +// could open. The grid API and the live stream gate on roleFor alone, |
| +// so that was direct read and write access to a sheet no list shows. |
| +func TestAFillingSheetIsClosedToItsOwnImporter(t *testing.T) { |
| + app, cookies := signIn(t) |
| + importer := signedInMember(t, app) |
| + |
| + s, err := app.createFillingSheet("half a workbook", importer) |
| + if err != nil { |
| + t.Fatalf("createFillingSheet: %v", err) |
| + } |
| + if _, has := app.roleFor(s.ID, importer); has { |
| + t.Error("the importer holds a role on a sheet that is still filling") |
| + } |
| + |
| + sheetID := strconv.FormatUint(s.ID, 10) |
| + tabID := strconv.FormatUint(s.TabID, 10) |
| + base := "/api/sheets/" + sheetID + "/tabs/" + tabID |
| + |
| + for _, tc := range []struct{ name, path string }{ |
| + {"grid cells", base + "/cells"}, |
| + {"live stream", base + "/stream"}, |
| + } { |
| + // A deadline, because the stream route is the one that does NOT |
| + // return when it lets you in: refused it 404s at once, allowed |
| + // it holds the connection open by design. Without this the |
| + // regression shape is a test that hangs until CI gives up, |
| + // which is a worse signal than a red line. Measured: removing |
| + // the roleFor check turns this from a 404 into a hang. |
| + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) |
| + req := as(httptest.NewRequest(http.MethodGet, tc.path, nil), cookies).WithContext(ctx) |
| + rec := do(t, app, req) |
| + cancel() |
| + if rec.Code != http.StatusNotFound { |
| + t.Errorf("%s answered %d on a filling sheet, want 404", tc.name, rec.Code) |
| + } |
| + } |
| + |
| + // Once filled it is ordinary again, so this is a gate and not a |
| + // wall. |
| + if err := app.markFilled(s.ID); err != nil { |
| + t.Fatalf("markFilled: %v", err) |
| + } |
| + if _, has := app.roleFor(s.ID, importer); !has { |
| + t.Error("the importer lost their role on a sheet that finished filling") |
| + } |
| +} |
| + |
| +// TestADeletedSheetIsClosedToAGrantHolder is the same hole in the shape |
| +// it had before import existed: a grant outlived the sheet it pointed |
| +// at, and roleFor honoured it. |
| +func TestADeletedSheetIsClosedToAGrantHolder(t *testing.T) { |
| + app := newTestApp(t) |
| + owner := testMember(t, app) |
| + |
| + s, err := app.createSheet("a sheet about to go", owner) |
| + if err != nil { |
| + t.Fatalf("createSheet: %v", err) |
| + } |
| + if _, has := app.roleFor(s.ID, owner); !has { |
| + t.Fatal("the owner holds no role on a live sheet; fixture is wrong") |
| + } |
| + |
| + now := time.Now().UTC() |
| + if err := app.db.Model(&store.Sheet{}).Where("id = ?", s.ID). |
| + Update("deleted_at", now).Error; err != nil { |
| + t.Fatalf("soft delete: %v", err) |
| + } |
| + if _, has := app.roleFor(s.ID, owner); has { |
| + t.Error("a grant holder keeps their role on a deleted sheet") |
| + } |
| +} |
diff --git a/internal/sheets/handlers_import.go b/internal/sheets/handlers_import.go| index 039b144..4ab6cb4 100644 |
| --- a/internal/sheets/handlers_import.go |
| +++ b/internal/sheets/handlers_import.go |
| @@ -179,6 +179,34 @@ func (a *App) runImport(ctx context.Context, s SheetSummary, data []byte, progre |
| return nil |
| } |
| +// sweepStaleImports discards every sheet left mid-import by a previous |
| +// process. |
| +// |
| +// At startup, a sheet still marked FillingSince is stale BY |
| +// CONSTRUCTION, with no timeout to guess at: the job that was filling |
| +// it was a goroutine in a process that is no longer running (the jobs |
| +// package is in-memory and unpersisted by design), so nothing is coming |
| +// back to finish it. That makes this the one sweep that cannot race |
| +// live work — within a running process every filling sheet has a live |
| +// job, and this only ever runs before there is one. |
| +// |
| +// Hidden was already the safe state, so this is reclamation rather than |
| +// correctness: without it a deploy mid-import leaves the row, its |
| +// grants and its sealed cells behind for good. |
| +func (a *App) sweepStaleImports() { |
| + if a.db == nil { |
| + return |
| + } |
| + var stale []store.Sheet |
| + if err := a.db.Where("filling_since IS NOT NULL AND deleted_at IS NULL"). |
| + Find(&stale).Error; err != nil { |
| + return |
| + } |
| + for _, s := range stale { |
| + a.discardSheet(s.ID) |
| + } |
| +} |
| + |
| // markFilled reveals a sheet that finished filling. |
| // |
| // Its error is NOT dropped, unlike touchSheet's: a sheet whose flag |
diff --git a/internal/sheets/handlers_import_hidden_test.go b/internal/sheets/handlers_import_hidden_test.go| index 908bbfd..7d7f4e9 100644 |
| --- a/internal/sheets/handlers_import_hidden_test.go |
| +++ b/internal/sheets/handlers_import_hidden_test.go |
| @@ -82,15 +82,27 @@ func TestAnOrdinaryNewSheetIsVisibleImmediately(t *testing.T) { |
| // may sign in at all, so an owner grant left behind by a failed import |
| // would keep admitting somebody who has no live access to anything. |
| func TestDiscardingAnImportRemovesItsGrants(t *testing.T) { |
| + // The allowlist has to be off, or Admits answers true from it and |
| + // the assertion below proves nothing about grants at all — which is |
| + // exactly what the first version of this test did. |
| app := newTestApp(t) |
| + app.admit = func(string) bool { return false } |
| guest := testMember(t, app) |
| s, err := app.createFillingSheet("a workbook that will fail", guest) |
| if err != nil { |
| t.Fatalf("createFillingSheet: %v", err) |
| } |
| + // Filled first, so the grant is one that genuinely admits. A |
| + // still-filling sheet does not admit its own importer — that is |
| + // TestAdmitsIgnoresGrantsToSheetsYouCannotOpen's subject — so |
| + // starting there would make the assertion below pass for the wrong |
| + // reason. |
| + if err := app.markFilled(s.ID); err != nil { |
| + t.Fatalf("markFilled: %v", err) |
| + } |
| if !app.Admits(guest.Address) { |
| - t.Fatal("the importer is not admitted while their import is live; fixture is wrong") |
| + t.Fatal("a live grant does not admit; fixture is wrong") |
| } |
| app.discardSheet(s.ID) |
| @@ -102,6 +114,9 @@ func TestDiscardingAnImportRemovesItsGrants(t *testing.T) { |
| if grants != 0 { |
| t.Errorf("a discarded import left %d grant(s) behind", grants) |
| } |
| + if app.Admits(guest.Address) { |
| + t.Error("a discarded import still admits the person who started it") |
| + } |
| } |
| // TestAdmitsIgnoresGrantsToSheetsYouCannotOpen covers the wider half of |
| @@ -130,6 +145,50 @@ func TestAdmitsIgnoresGrantsToSheetsYouCannotOpen(t *testing.T) { |
| if app.Admits(guest.Address) { |
| t.Error("a grant to a deleted sheet still admits its holder") |
| } |
| + |
| + // And the filling case on its own, which the deleted case above |
| + // does not cover: without this, dropping filling_since from Admits |
| + // would leave this test green. |
| + if err := app.db.Model(&store.Sheet{}).Where("id = ?", s.ID). |
| + Updates(map[string]any{"deleted_at": nil, "filling_since": now}).Error; err != nil { |
| + t.Fatalf("mark filling: %v", err) |
| + } |
| + if app.Admits(guest.Address) { |
| + t.Error("a grant to a sheet still being filled admits its holder") |
| + } |
| +} |
| + |
| +// TestStaleImportsAreSweptAtBoot covers the reclamation path. A sheet |
| +// still marked filling when a process starts belongs to a process that |
| +// is gone — the jobs registry is in-memory — so nothing is coming back |
| +// to finish it. |
| +func TestStaleImportsAreSweptAtBoot(t *testing.T) { |
| + app := newTestApp(t) |
| + member := testMember(t, app) |
| + |
| + s, err := app.createFillingSheet("interrupted by a deploy", member) |
| + if err != nil { |
| + t.Fatalf("createFillingSheet: %v", err) |
| + } |
| + |
| + // A second App over the same database is what a restart looks like. |
| + restarted := New(Config{DB: app.db, Sealer: app.sealer, Admit: app.admit}) |
| + |
| + var sheet store.Sheet |
| + if err := restarted.db.Where("id = ?", s.ID).First(&sheet).Error; err != nil { |
| + t.Fatalf("reload sheet: %v", err) |
| + } |
| + if sheet.DeletedAt == nil { |
| + t.Error("a stale import survived a restart undiscarded") |
| + } |
| + var grants int64 |
| + if err := restarted.db.Model(&Grant{}).Where("sheet_id = ?", s.ID). |
| + Count(&grants).Error; err != nil { |
| + t.Fatalf("count grants: %v", err) |
| + } |
| + if grants != 0 { |
| + t.Errorf("a stale import kept %d grant(s) through a restart", grants) |
| + } |
| } |
| // TestAFileOfExactlyTheAdvertisedSizeIsAccepted pins the limit the form |
diff --git a/internal/sheets/xlsx/import.go b/internal/sheets/xlsx/import.go| index 55141f0..24a07f8 100644 |
| --- a/internal/sheets/xlsx/import.go |
| +++ b/internal/sheets/xlsx/import.go |
| @@ -35,11 +35,17 @@ const importActor uint64 = 0 |
| // refuse it, because that check counts cells that have already been |
| // decompressed into memory. |
| // |
| +// UnzipSizeLimit is the hard bound and the one doing the work here: |
| // 256MB unzipped is generous for a workbook that has to fit in |
| // MaxCellsPerTab (200,000 cells) and small enough that several |
| -// concurrent imports cannot exhaust an instance. The XML limit is |
| -// smaller again and applies per worksheet part, which is the one an |
| -// expansion attack actually inflates. |
| +// concurrent imports cannot exhaust an instance. |
| +// |
| +// UnzipXMLSizeLimit is NOT a second rejection threshold, whatever its |
| +// name suggests — excelize uses it to decide when a worksheet or shared |
| +// string table is large enough to stream through temporary storage |
| +// instead of memory (its default is StreamChunkSize). Setting it lower |
| +// than the total therefore bounds resident memory rather than refusing |
| +// anything, which is worth having and is not the protection. |
| // |
| // This is a bound, not validation: a legitimate workbook past it is |
| // refused the same as a malicious one, and that is the right trade for |