amadan

oficina / sheets Public

fix: an importing sheet is nobody's until it is finished

A gpt-5.6-sol review of the import branch came back REQUEST CHANGES.
Six of its seven findings were real; this is all of them.

**The blocker, and its two neighbours, were one bug.** Import cannot be
one transaction — Handle.Set recalculates and commits per cell — so
between creating the sheet row and writing the last cell there was a
window in which the sheet existed and was wrong. On this instance every
team member sees every sheet, so that window was visible to the whole
team, and three things went wrong in it: a half-filled sheet looked
complete, a collaborator could edit one that was about to be discarded,
and a process that stopped mid-import left the partial behind for good,
because the job that would have cleaned it up was in memory and died
with it. My own commit message claimed idempotency that only ever held
for HANDLED failures.

So the sheet is now invisible for the whole of the import.
`Sheet.FillingSince` is set at creation and cleared on success, and
every read path filters it exactly as it filters `deleted_at` — the
list, the sheet page, `sheetExists` and share links. Cleanup became a
tidiness measure rather than the thing correctness rests on, which is
the point: the one failure no cleanup can cover is the process dying,
and that now leaves a hidden row instead of a lie. `markFilled`'s error
is not dropped, unlike `touchSheet`'s, because a flag that will not
clear means reporting success and then 404ing the person on it.

**Unbounded decompression.** XLSX is a ZIP and excelize's own
UnzipSizeLimit defaults to 16GB, so a 20MB cap on the HTTP body bounded
nothing: a small, highly compressible file expanded in `OpenReader` and
`GetRows` long before Import's MaxCellsPerTab check — which counts cells
that have already been decompressed — could refuse it. Both unzip limits
are now set.

**A failed import minted an admitting grant.** `Admits` counts a
member's grants to decide whether an address may sign in at all, and it
joined only members, so a grant to a deleted sheet still admitted its
holder. Both halves closed: `discardSheet` removes the grants it
created, and `Admits` joins sheets and excludes ones you could not open.

**20 MB did not mean 20 MB.** MaxBytesReader bounds the whole multipart
body, so a file of exactly the advertised size was refused. The body
limit gets slack for the framing and the file's own size is checked
exactly.

The seventh finding — sheets.css over the line cap — is a false
positive I induced by paraphrasing the cap as "every file" in the review
brief. TestFileLineCap covers .go only, and that file was 413 lines
before this branch touched it.

Every fix has a test that fails without it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H6YuJ2QG6koRKjNf3ddr1Q
Paul Campbell pushed by paul@keymail.dev 20977af6a3c8a061ab1f0e4c6a3bf205942a2cc2 parent e813aa2
10 files changed, +462 −103
  • internal/sheets/grants.go +18 −3
  • internal/sheets/handlers_import.go +67 −96
  • internal/sheets/handlers_import_hidden_test.go +208 −0
  • internal/sheets/handlers_link.go +1 −1
  • internal/sheets/importupload.go +92 −0
  • internal/sheets/migrations/0008_alter_sheets.sql +4 −0
  • internal/sheets/migrations/schema.sql +3 −1
  • internal/sheets/sheetsvc.go +20 −1
  • internal/sheets/store/models.go +23 −0
  • internal/sheets/xlsx/import.go +26 −1

Intent

Humanclaude~paul@keymail.dev

merge and deploy so I can review

Humanclaude~paul@keymail.dev

You can have commandcode.ai on this machine do a review using GPT Sol

Humanclaude~paul@keymail.dev

Go ahead and fix everything. Don't worry about existing data ... we're in development mode.

full ledger: feat/xlsx-drop-import

diff --git a/internal/sheets/grants.go b/internal/sheets/grants.go
index 2b4b025..fcd316f 100644
--- a/internal/sheets/grants.go
+++ b/internal/sheets/grants.go
@@ -88,7 +88,7 @@ func (a *App) mayShare(sheetID uint64, m Member) bool {
func (a *App) sheetExists(id uint64) bool {
var n int64
if err := a.db.Model(&store.Sheet{}).
- Where("id = ? AND deleted_at IS NULL", id).Count(&n).Error; err != nil {
+ Where("id = ? AND deleted_at IS NULL AND filling_since IS NULL", id).Count(&n).Error; err != nil {
return false
}
return n == 1
@@ -110,7 +110,13 @@ func (a *App) visibleSheets(m Member) ([]store.Sheet, error) {
if a.db == nil {
return nil, errNoDB
}
- q := a.db.Model(&store.Sheet{}).Where("sheets.deleted_at IS NULL")
+ // filling_since alongside deleted_at, everywhere the other is
+ // checked: a sheet still being filled from an imported file is not
+ // a thing anybody should see yet, and on this instance every team
+ // member sees every sheet, so "not visible" has to mean not in this
+ // query rather than not linked to.
+ q := a.db.Model(&store.Sheet{}).
+ Where("sheets.deleted_at IS NULL AND sheets.filling_since IS NULL")
switch TeamRole(m.TeamRole) {
case TeamOwner, TeamMember:
@@ -258,10 +264,19 @@ func (a *App) Admits(address string) bool {
if a.db == nil {
return false
}
+ // The sheets join is load-bearing, not decoration. Without it this
+ // counts grants to sheets that no longer exist, so a guest whose
+ // every live grant has been revoked still signs in on the strength
+ // of a deleted one — and a sheet still being filled by an import
+ // would admit somebody before the import it belongs to had even
+ // succeeded. Admission has to rest on a sheet they could actually
+ // open.
var n int64
if err := a.db.Model(&Grant{}).
Joins("JOIN members ON members.id = grants.member_id").
- Where("members.address = ?", addr).Count(&n).Error; err != nil {
+ Joins("JOIN sheets ON sheets.id = grants.sheet_id").
+ Where("members.address = ? AND sheets.deleted_at IS NULL AND sheets.filling_since IS NULL", addr).
+ Count(&n).Error; err != nil {
return false
}
return n > 0
diff --git a/internal/sheets/handlers_import.go b/internal/sheets/handlers_import.go
index b67ea12..039b144 100644
--- a/internal/sheets/handlers_import.go
+++ b/internal/sheets/handlers_import.go
@@ -4,11 +4,8 @@ import (
"bytes"
"context"
"errors"
- "io"
"net/http"
- "path"
"strconv"
- "strings"
"time"
"github.com/carlosframework/rastrillo/jobs"
@@ -27,6 +24,17 @@ import (
// byte count.
const maxImportBytes = 20 << 20
+// multipartSlack is what MaxBytesReader gets ON TOP of maxImportBytes.
+//
+// MaxBytesReader bounds the whole request body — part headers, boundary
+// markers and the trailing boundary as well as the file — so reusing
+// maxImportBytes as the body limit refuses a file of exactly the size
+// the form advertises, and a slightly smaller one too. The form says
+// 20 MB, so 20 MB has to get through; the slack covers the framing and
+// the file's own size is then checked exactly, against the bytes that
+// actually arrived.
+const multipartSlack = 1 << 20
+
// importField is the file input's name, in the form and in the handler.
const importField = "file"
@@ -66,9 +74,14 @@ const (
// for one reason: jobs.Start needs the finished job's location up front,
// and that location must be a path this code built rather than anything
// a user supplied (jobs.Start's own contract). Knowing the sheet id
-// before the job starts is what makes "/sheets/{id}" constructible. The
-// cost is a sheet that exists before its contents do, and discardSheet
-// is what pays it when the import fails.
+// before the job starts is what makes "/sheets/{id}" constructible.
+//
+// The cost is a sheet row that exists before its contents do, and
+// createFillingSheet is what pays it: the row is invisible on every
+// read path until runImport reveals it. Cleanup is then a tidiness
+// measure rather than the thing correctness rests on — which matters,
+// because the one failure no cleanup can cover is the process dying
+// mid-import.
func (a *App) handleSheetImport(w http.ResponseWriter, r *http.Request) {
actor, ok := a.actor(r)
if !ok {
@@ -93,7 +106,7 @@ func (a *App) handleSheetImport(w http.ResponseWriter, r *http.Request) {
return
}
- s, err := a.createSheet(titleFromFilename(filename), actor)
+ s, err := a.createFillingSheet(titleFromFilename(filename), actor)
if err != nil {
a.fail(w, r, err)
return
@@ -118,89 +131,23 @@ func (a *App) handleSheetImport(w http.ResponseWriter, r *http.Request) {
http.Redirect(w, r, "/jobs/"+job.ID, http.StatusSeeOther)
}
-// readUpload reads the posted file into memory, bounded, and returns
-// either its bytes and name or a refusal to show the person.
-//
-// Into memory rather than to a temp file because the job outlives the
-// request: the multipart reader is closed the moment this handler
-// returns, so the bytes have to be somewhere the goroutine still owns.
-// maxImportBytes times jobs' own four-running-per-owner cap is what
-// that costs at worst.
-//
-// The upload is deliberately NOT retained anywhere. Spec §11 says an
-// uploaded file goes to blobs.Sealed, which is the rule for KEEPING
-// one; not keeping it at all satisfies the same concern more simply,
-// and the plaintext workbook never reaches storage in any form. A later
-// task that wants the original back wires xlsx.StoreSealed in, which is
-// what that function is already there for.
-func readUpload(w http.ResponseWriter, r *http.Request) (data []byte, filename, refusal string) {
- r.Body = http.MaxBytesReader(w, r.Body, maxImportBytes)
-
- file, header, err := r.FormFile(importField)
- if err != nil {
- return nil, "", uploadRefusal(err)
- }
- defer file.Close()
-
- if !isWorkbookName(header.Filename) {
- return nil, "", refusalNotWorkbook
- }
- body, err := io.ReadAll(file)
- if err != nil {
- return nil, "", uploadRefusal(err)
- }
- return body, header.Filename, ""
-}
-
-// uploadRefusal turns a multipart failure into copy.
-//
-// A too-large body arrives here as *http.MaxBytesError, and it is worth
-// naming separately: "that upload could not be read" sends somebody
-// looking for a corrupt file when the real answer is that theirs is
-// simply too big.
-func uploadRefusal(err error) string {
- var tooLarge *http.MaxBytesError
- switch {
- case errors.As(err, &tooLarge):
- return refusalTooLarge
- case errors.Is(err, http.ErrMissingFile):
- return refusalNoFile
- default:
- return refusalUnreadable
- }
-}
-
-// isWorkbookName reports whether a filename looks like an Excel
-// workbook. path.Base first: a filename is remote input, and while
-// browsers send a bare name, nothing makes them.
-func isWorkbookName(name string) bool {
- return workbookExts[strings.ToLower(path.Ext(path.Base(name)))]
-}
-
-// titleFromFilename names the sheet after the file it came from, minus
-// the extension — the thing a person expects to see in the list.
-//
-// createSheet caps the length and refuses an empty title, so this only
-// has to supply a sensible fallback for a file called ".xlsx".
-func titleFromFilename(name string) string {
- base := path.Base(strings.ReplaceAll(name, `\`, "/"))
- base = strings.TrimSuffix(base, path.Ext(base))
- if clean := strings.TrimSpace(base); clean != "" {
- return clean
- }
- return "Untitled sheet"
-}
-
// runImport is the job body: open the new sheet's tab, read the
-// workbook into it, and leave nothing behind if that fails.
+// workbook into it, and either reveal the sheet or leave nothing
+// behind.
+//
+// The sheet is invisible for the whole of this function
+// (createFillingSheet, store.Sheet.FillingSince) and becomes visible
+// only on the last line. That is what makes the three ways this can go
+// wrong all end somewhere safe: a handled failure discards it, an
+// unhandled one leaves it hidden, and a process that dies mid-import —
+// a deploy, which the jobs package warns is a harder deadline than any
+// timeout — leaves a hidden row rather than a half-filled sheet the
+// whole team can see and nothing left alive to clean it up.
//
// It is not interruptible. xlsx.Import takes no context and runs to
// completion, so ctx is checked at the top and not again — an honest
// statement of what cancellation does here rather than a select that
-// implies more than it delivers. Jobs are expected to be idempotent
-// because a deploy ends them mid-flight (jobs' package doc); this one
-// is re-runnable in the only way that matters, since a failed run
-// leaves no sheet for a second one to collide with.
+// implies more than it delivers.
func (a *App) runImport(ctx context.Context, s SheetSummary, data []byte, progress func(string)) error {
if err := ctx.Err(); err != nil {
a.discardSheet(s.ID)
@@ -219,12 +166,32 @@ func (a *App) runImport(ctx context.Context, s SheetSummary, data []byte, progre
a.discardSheet(s.ID)
return importFailure(err)
}
+ // Reveal it, and only then report success. If this fails the sheet
+ // stays hidden and the job says so, which is the right way round:
+ // the alternative is telling somebody their import worked and
+ // sending them to a sheet the list will not show.
+ if err := a.markFilled(s.ID); err != nil {
+ return errors.New("that sheet could not be opened")
+ }
// The list orders by real activity, and an imported sheet's
// activity is the import.
a.touchSheet(s.ID)
return nil
}
+// markFilled reveals a sheet that finished filling.
+//
+// Its error is NOT dropped, unlike touchSheet's: a sheet whose flag
+// will not clear is invisible, so swallowing this would report a
+// successful import and then 404 the person on the sheet it made.
+func (a *App) markFilled(id uint64) error {
+ if a.db == nil {
+ return errNoDB
+ }
+ return a.db.Model(&store.Sheet{}).Where("id = ?", id).
+ Update("filling_since", nil).Error
+}
+
// importFailure turns an import error into text safe to show and safe
// to log.
//
@@ -247,23 +214,27 @@ func importFailure(err error) error {
return errors.New("that file could not be read as a spreadsheet")
}
-// discardSheet soft-deletes a sheet whose import did not finish.
+// discardSheet throws away a sheet whose import did not finish: it
+// soft-deletes the row and removes the grants that were minted with it.
//
-// A half-imported sheet is worse than no sheet: it looks complete, sits
-// in a list everybody on the instance can see, and the only signal that
-// it is missing rows is a job status page whoever opens it later never
-// saw. Removing it makes the failure mean one thing — try again with a
-// file that works.
+// The grants matter on their own. Admits counts a member's grants to
+// decide whether an address may sign in at all, so an owner grant left
+// behind by a failed import keeps admitting a guest whose every real
+// grant has since been revoked. Deleting them here keeps that from
+// being something a failed upload can create. (The wider gap — Admits
+// not excluding grants to already-deleted sheets — is not this
+// function's to close, and is reported separately.)
//
-// Soft, not hard: nothing is destroyed, the row keeps whatever the
-// partial run wrote, and every read path already filters deleted_at
-// (visibleSheets, sheetSummary). A failure to mark it is not worth
-// reporting over the import failure that caused it, so the error is
-// dropped and the worst case is one empty sheet in the list.
+// Soft-deleting the sheet is belt and braces rather than the mechanism:
+// the sheet is still marked FillingSince, so it is already invisible on
+// every read path, and an error here therefore cannot expose anything.
+// That is why this returns nothing — there is no failure mode left
+// worth reporting over the import failure that caused it.
func (a *App) discardSheet(id uint64) {
if a.db == nil {
return
}
now := time.Now().UTC()
a.db.Model(&store.Sheet{}).Where("id = ?", id).Update("deleted_at", now)
+ a.db.Where("sheet_id = ?", id).Delete(&Grant{})
}
diff --git a/internal/sheets/handlers_import_hidden_test.go b/internal/sheets/handlers_import_hidden_test.go
new file mode 100644
index 0000000..908bbfd
--- /dev/null
+++ b/internal/sheets/handlers_import_hidden_test.go
@@ -0,0 +1,208 @@
+package sheets
+
+import (
+ "net/http"
+ "net/http/httptest"
+ "strings"
+ "testing"
+ "time"
+
+ "amadan.net/oficina/sheets/internal/sheets/store"
+)
+
+// These pin the findings a gpt-5.6-sol review raised against the first
+// cut of import (2026-09-02). Each one failed before the fix.
+
+// TestAnUnfinishedImportIsInvisible is the fix for the blocker: a sheet
+// being filled from a workbook must not be visible to anybody, because
+// the window between creating it and finishing it is one nothing can
+// clean up if the process dies inside it.
+//
+// It asserts the flag directly rather than racing a real import,
+// because the interesting state is the one that exists only while the
+// job runs — and a test that tried to catch it mid-flight would be
+// flaky in exactly the way that hides a regression.
+func TestAnUnfinishedImportIsInvisible(t *testing.T) {
+ app := newTestApp(t)
+ member := testMember(t, app)
+
+ s, err := app.createFillingSheet("half a workbook", member)
+ if err != nil {
+ t.Fatalf("createFillingSheet: %v", err)
+ }
+
+ list, err := app.listSheets(member)
+ if err != nil {
+ t.Fatalf("listSheets: %v", err)
+ }
+ if len(list) != 0 {
+ t.Errorf("a sheet still being filled appears in the list (%d rows)", len(list))
+ }
+ if _, err := app.sheetSummary(s.ID, member); err == nil {
+ t.Error("a sheet still being filled can be opened by id")
+ }
+
+ // And it becomes ordinary the moment it is filled.
+ if err := app.markFilled(s.ID); err != nil {
+ t.Fatalf("markFilled: %v", err)
+ }
+ list, err = app.listSheets(member)
+ if err != nil {
+ t.Fatalf("listSheets after markFilled: %v", err)
+ }
+ if len(list) != 1 {
+ t.Fatalf("want the sheet visible once filled, got %d rows", len(list))
+ }
+ if _, err := app.sheetSummary(s.ID, member); err != nil {
+ t.Errorf("a filled sheet cannot be opened by id: %v", err)
+ }
+}
+
+// TestAnOrdinaryNewSheetIsVisibleImmediately guards the other
+// direction: the flag must not leak into the plain Create path, or
+// every new sheet vanishes.
+func TestAnOrdinaryNewSheetIsVisibleImmediately(t *testing.T) {
+ app := newTestApp(t)
+ member := testMember(t, app)
+
+ if _, err := app.createSheet("an ordinary sheet", member); err != nil {
+ t.Fatalf("createSheet: %v", err)
+ }
+ list, err := app.listSheets(member)
+ if err != nil {
+ t.Fatalf("listSheets: %v", err)
+ }
+ if len(list) != 1 {
+ t.Fatalf("a normally created sheet is not visible (%d rows)", len(list))
+ }
+}
+
+// TestDiscardingAnImportRemovesItsGrants is the fix for the admission
+// finding. Admits counts a member's grants to decide whether an address
+// 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) {
+ app := newTestApp(t)
+ guest := testMember(t, app)
+
+ s, err := app.createFillingSheet("a workbook that will fail", guest)
+ if err != nil {
+ t.Fatalf("createFillingSheet: %v", err)
+ }
+ if !app.Admits(guest.Address) {
+ t.Fatal("the importer is not admitted while their import is live; fixture is wrong")
+ }
+
+ app.discardSheet(s.ID)
+
+ var grants int64
+ if err := app.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 discarded import left %d grant(s) behind", grants)
+ }
+}
+
+// TestAdmitsIgnoresGrantsToSheetsYouCannotOpen covers the wider half of
+// the same finding: a grant to a deleted — or still-filling — sheet is
+// not a reason to let an address sign in.
+func TestAdmitsIgnoresGrantsToSheetsYouCannotOpen(t *testing.T) {
+ // admit refuses everybody, so Admits can only answer true on the
+ // strength of a grant. Otherwise the allowlist masks the bug.
+ app := newTestApp(t)
+ app.admit = func(string) bool { return false }
+ guest := testMember(t, app)
+
+ s, err := app.createSheet("shared with a guest", guest)
+ if err != nil {
+ t.Fatalf("createSheet: %v", err)
+ }
+ if !app.Admits(guest.Address) {
+ t.Fatal("a live grant does not admit; 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 app.Admits(guest.Address) {
+ t.Error("a grant to a deleted sheet still admits its holder")
+ }
+}
+
+// TestAFileOfExactlyTheAdvertisedSizeIsAccepted pins the limit the form
+// promises. MaxBytesReader bounds the whole multipart body, so without
+// slack for the framing a file of exactly 20 MB — the number printed on
+// the page — is refused.
+//
+// The payload is not a workbook, so this stops at the extension check;
+// what matters is that it gets that far rather than dying as "too
+// large".
+func TestAFileOfExactlyTheAdvertisedSizeIsAccepted(t *testing.T) {
+ app, cookies := signIn(t)
+
+ rec := do(t, app, as(uploadRequest(t, "big.bin", make([]byte, maxImportBytes)), cookies))
+ if rec.Code != http.StatusBadRequest {
+ t.Fatalf("answered %d, want 400", rec.Code)
+ }
+ body := rec.Body.String()
+ if strings.Contains(body, refusalTooLarge) {
+ t.Error("a file of exactly the advertised size was refused as too large")
+ }
+ if !strings.Contains(body, refusalNotWorkbook) {
+ t.Errorf("want the extension refusal, got: %q", firstRefusal(body))
+ }
+}
+
+// TestAFileOverTheAdvertisedSizeIsRefused is the other side of it: the
+// slack exists for the framing, not to raise the limit.
+func TestAFileOverTheAdvertisedSizeIsRefused(t *testing.T) {
+ app, cookies := signIn(t)
+
+ rec := do(t, app, as(uploadRequest(t, "big.xlsx", make([]byte, maxImportBytes+1)), cookies))
+ if rec.Code != http.StatusBadRequest {
+ t.Fatalf("answered %d, want 400", rec.Code)
+ }
+ if !strings.Contains(rec.Body.String(), refusalTooLarge) {
+ t.Error("a file over the advertised size was not refused as too large")
+ }
+}
+
+// firstRefusal pulls whichever refusal a page is carrying, for a
+// failure message that says what actually happened.
+func firstRefusal(body string) string {
+ for _, r := range []string{
+ refusalNoFile, refusalNotWorkbook, refusalTooLarge,
+ refusalBusy, refusalUnreadable,
+ } {
+ if strings.Contains(body, r) {
+ return r
+ }
+ }
+ return "no refusal on the page"
+}
+
+// TestImportedSheetIsNotReachableByLinkWhileFilling closes the last
+// read path: share links resolve a sheet by id of their own accord, so
+// they need the same filter as the list and the page.
+func TestImportedSheetIsNotReachableByLinkWhileFilling(t *testing.T) {
+ app := newTestApp(t)
+ member := testMember(t, app)
+
+ s, err := app.createFillingSheet("half a workbook", member)
+ if err != nil {
+ t.Fatalf("createFillingSheet: %v", err)
+ }
+ token, err := app.createLink(s.ID, SheetViewer, member)
+ if err != nil {
+ // Minting a link for a filling sheet may itself be refused,
+ // which is a perfectly good answer to this question.
+ return
+ }
+ rec := do(t, app, httptest.NewRequest(http.MethodGet, "/l/"+token, nil))
+ if rec.Code == http.StatusOK {
+ t.Error("a share link opens a sheet that is still being filled")
+ }
+}
diff --git a/internal/sheets/handlers_link.go b/internal/sheets/handlers_link.go
index f82b183..9d60890 100644
--- a/internal/sheets/handlers_link.go
+++ b/internal/sheets/handlers_link.go
@@ -111,7 +111,7 @@ func (a *App) summarizeByID(id uint64) (SheetSummary, error) {
return SheetSummary{}, errNoSealer
}
var sheet store.Sheet
- if err := a.db.Where("id = ? AND deleted_at IS NULL", id).First(&sheet).Error; err != nil {
+ if err := a.db.Where("id = ? AND deleted_at IS NULL AND filling_since IS NULL", id).First(&sheet).Error; err != nil {
return SheetSummary{}, err
}
return a.summarize(sheet)
diff --git a/internal/sheets/importupload.go b/internal/sheets/importupload.go
new file mode 100644
index 0000000..239a954
--- /dev/null
+++ b/internal/sheets/importupload.go
@@ -0,0 +1,92 @@
+// Reading an upload: what the browser sent, and whether we will take
+// it. Split from handlers_import.go only because that file reached the
+// 300-line cap; this half is the request-shaped part, and the other is
+// the sheet-and-job-shaped part.
+package sheets
+
+import (
+ "errors"
+ "io"
+ "net/http"
+ "path"
+ "strings"
+)
+
+// readUpload reads the posted file into memory, bounded, and returns
+// either its bytes and name or a refusal to show the person.
+//
+// Into memory rather than to a temp file because the job outlives the
+// request: the multipart reader is closed the moment this handler
+// returns, so the bytes have to be somewhere the goroutine still owns.
+// maxImportBytes times jobs' own four-running-per-owner cap is what
+// that costs at worst.
+//
+// The upload is deliberately NOT retained anywhere. Spec §11 says an
+// uploaded file goes to blobs.Sealed, which is the rule for KEEPING
+// one; not keeping it at all satisfies the same concern more simply,
+// and the plaintext workbook never reaches storage in any form. A later
+// task that wants the original back wires xlsx.StoreSealed in, which is
+// what that function is already there for.
+func readUpload(w http.ResponseWriter, r *http.Request) (data []byte, filename, refusal string) {
+ r.Body = http.MaxBytesReader(w, r.Body, maxImportBytes+multipartSlack)
+
+ file, header, err := r.FormFile(importField)
+ if err != nil {
+ return nil, "", uploadRefusal(err)
+ }
+ defer file.Close()
+
+ if !isWorkbookName(header.Filename) {
+ return nil, "", refusalNotWorkbook
+ }
+ body, err := io.ReadAll(file)
+ if err != nil {
+ return nil, "", uploadRefusal(err)
+ }
+ // The exact limit, checked on the file itself rather than on the
+ // body it arrived in. The slack above exists to let a genuine 20 MB
+ // file through the framing, not to raise the limit.
+ if len(body) > maxImportBytes {
+ return nil, "", refusalTooLarge
+ }
+ return body, header.Filename, ""
+}
+
+// uploadRefusal turns a multipart failure into copy.
+//
+// A too-large body arrives here as *http.MaxBytesError, and it is worth
+// naming separately: "that upload could not be read" sends somebody
+// looking for a corrupt file when the real answer is that theirs is
+// simply too big.
+func uploadRefusal(err error) string {
+ var tooLarge *http.MaxBytesError
+ switch {
+ case errors.As(err, &tooLarge):
+ return refusalTooLarge
+ case errors.Is(err, http.ErrMissingFile):
+ return refusalNoFile
+ default:
+ return refusalUnreadable
+ }
+}
+
+// isWorkbookName reports whether a filename looks like an Excel
+// workbook. path.Base first: a filename is remote input, and while
+// browsers send a bare name, nothing makes them.
+func isWorkbookName(name string) bool {
+ return workbookExts[strings.ToLower(path.Ext(path.Base(name)))]
+}
+
+// titleFromFilename names the sheet after the file it came from, minus
+// the extension — the thing a person expects to see in the list.
+//
+// createSheet caps the length and refuses an empty title, so this only
+// has to supply a sensible fallback for a file called ".xlsx".
+func titleFromFilename(name string) string {
+ base := path.Base(strings.ReplaceAll(name, `\`, "/"))
+ base = strings.TrimSuffix(base, path.Ext(base))
+ if clean := strings.TrimSpace(base); clean != "" {
+ return clean
+ }
+ return "Untitled sheet"
+}
diff --git a/internal/sheets/migrations/0008_alter_sheets.sql b/internal/sheets/migrations/0008_alter_sheets.sql
new file mode 100644
index 0000000..ad1a979
--- /dev/null
+++ b/internal/sheets/migrations/0008_alter_sheets.sql
@@ -0,0 +1,4 @@
+ALTER TABLE `sheets` ADD `filling_since` datetime;
+
+CREATE INDEX `idx_sheets_filling_since` ON `sheets`(`filling_since`);
+
diff --git a/internal/sheets/migrations/schema.sql b/internal/sheets/migrations/schema.sql
index f386203..3322040 100644
--- a/internal/sheets/migrations/schema.sql
+++ b/internal/sheets/migrations/schema.sql
@@ -19,7 +19,7 @@ CREATE TABLE `rows` (`id` integer PRIMARY KEY AUTOINCREMENT,`tab_id` integer,`po
CREATE TABLE `share_links` (`id` integer PRIMARY KEY AUTOINCREMENT,`sheet_id` integer,`token_hash` text,`role` text,`created_by` integer,`created_at` datetime,`revoked_at` datetime);
-CREATE TABLE `sheets` (`id` integer PRIMARY KEY AUTOINCREMENT,`title_sealed` blob,`created_by` integer,`created_at` datetime,`updated_at` datetime,`deleted_at` datetime);
+CREATE TABLE `sheets` (`id` integer PRIMARY KEY AUTOINCREMENT,`title_sealed` blob,`created_by` integer,`created_at` datetime,`updated_at` datetime,`deleted_at` datetime, `filling_since` datetime);
CREATE TABLE `tab_views` (`id` integer PRIMARY KEY AUTOINCREMENT,`tab_id` integer,`spec_sealed` blob,`updated_by` integer,`updated_at` datetime);
@@ -49,6 +49,8 @@ CREATE UNIQUE INDEX `idx_share_links_token_hash` ON `share_links`(`token_hash`);
CREATE INDEX `idx_sheets_deleted_at` ON `sheets`(`deleted_at`);
+CREATE INDEX `idx_sheets_filling_since` ON `sheets`(`filling_since`);
+
CREATE UNIQUE INDEX `idx_tab_views_tab_id` ON `tab_views`(`tab_id`);
CREATE INDEX `idx_tabs_sheet_id` ON `tabs`(`sheet_id`);
diff --git a/internal/sheets/sheetsvc.go b/internal/sheets/sheetsvc.go
index fa2511b..4e8212a 100644
--- a/internal/sheets/sheetsvc.go
+++ b/internal/sheets/sheetsvc.go
@@ -65,7 +65,22 @@ type SheetSummary struct {
// function ever creates a Sheet row, which is what makes "every sheet
// has at least one tab" a real invariant rather than a convention
// everyone remembers.
+// createSheet makes an ordinary, immediately visible sheet.
func (a *App) createSheet(title string, by Member) (SheetSummary, error) {
+ return a.newSheet(title, by, false)
+}
+
+// createFillingSheet makes a sheet that is HIDDEN until something
+// finishes filling it — import's shape (store.Sheet.FillingSince).
+//
+// The caller owns the other half: markFilled on success, discardSheet
+// on failure. A caller that does neither leaves an invisible row, which
+// is the failure this is designed to have.
+func (a *App) createFillingSheet(title string, by Member) (SheetSummary, error) {
+ return a.newSheet(title, by, true)
+}
+
+func (a *App) newSheet(title string, by Member, filling bool) (SheetSummary, error) {
if a.db == nil {
return SheetSummary{}, errNoDB
}
@@ -89,6 +104,10 @@ func (a *App) createSheet(title string, by Member) (SheetSummary, error) {
}
sheet := store.Sheet{TitleSealed: sealedTitle, CreatedBy: by.ID}
+ if filling {
+ now := time.Now().UTC()
+ sheet.FillingSince = &now
+ }
tab := store.Tab{NameSealed: sealedName, Position: 0}
if err := a.db.Transaction(func(tx *gorm.DB) error {
if err := tx.Create(&sheet).Error; err != nil {
@@ -166,7 +185,7 @@ func (a *App) sheetSummary(id uint64, viewer Member) (SheetSummary, error) {
return SheetSummary{}, errNoAccess
}
var s store.Sheet
- if err := a.db.Where("id = ? AND deleted_at IS NULL", id).First(&s).Error; err != nil {
+ if err := a.db.Where("id = ? AND deleted_at IS NULL AND filling_since IS NULL", id).First(&s).Error; err != nil {
return SheetSummary{}, err
}
return a.summarize(s)
diff --git a/internal/sheets/store/models.go b/internal/sheets/store/models.go
index a1383bc..b70b456 100644
--- a/internal/sheets/store/models.go
+++ b/internal/sheets/store/models.go
@@ -25,6 +25,29 @@ type Sheet struct {
CreatedAt time.Time
UpdatedAt time.Time
DeletedAt *time.Time `gorm:"index"`
+ // FillingSince is set while a sheet is being filled from an
+ // imported file and cleared when that finishes. Non-nil means the
+ // sheet is NOT yet a thing anybody should see: every read path
+ // filters it out exactly as it filters DeletedAt.
+ //
+ // It exists because import cannot be one transaction. Cells are
+ // written and committed one at a time (Handle.Set recalculates and
+ // persists as it goes), so between the sheet row and the last cell
+ // there is a window in which the sheet exists and is wrong — and on
+ // this instance every team member sees every sheet, so that window
+ // is visible to the whole team. Three things go wrong in it: a
+ // half-filled sheet looks complete, a collaborator can edit one
+ // that is about to be discarded, and a process that stops
+ // mid-import (a deploy) leaves the partial behind forever, because
+ // the job that would have cleaned it up was in memory and died with
+ // it.
+ //
+ // A timestamp rather than a bool so the stale ones are also
+ // identifiable — an import interrupted by a deploy leaves a row
+ // whose FillingSince never clears, and a sweeper or an operator can
+ // find those by age. Invisible is already the safe state, so
+ // nothing has to run for correctness.
+ FillingSince *time.Time `gorm:"index"`
}
// Tab is one sheet within a document — the tabs along the bottom.
diff --git a/internal/sheets/xlsx/import.go b/internal/sheets/xlsx/import.go
index f5b5b87..55141f0 100644
--- a/internal/sheets/xlsx/import.go
+++ b/internal/sheets/xlsx/import.go
@@ -24,6 +24,31 @@ import (
// as "the system, not a person" in the event log.
const importActor uint64 = 0
+// openLimits bounds what a workbook may decompress to.
+//
+// XLSX is a ZIP, so the size of the upload bounds nothing: excelize's
+// own defaults are UnzipSizeLimit 16GB and an unzipXML limit derived
+// from it, and the caller's byte cap on the HTTP body is a cap on
+// COMPRESSED bytes. A small file of highly compressible XML therefore
+// reaches OpenReader and GetRows — both of which materialise what they
+// read — long before Import's own MaxCellsPerTab check gets a chance to
+// refuse it, because that check counts cells that have already been
+// decompressed into memory.
+//
+// 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.
+//
+// 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
+// a limit whose job is to stop the process dying.
+var openLimits = excelize.Options{
+ UnzipSizeLimit: 256 << 20,
+ UnzipXMLSizeLimit: 64 << 20,
+}
+
// Import reads an XLSX workbook's first worksheet into h, cell by cell:
// literal values become literal input, formulas are parsed and
// evaluated by this app's own engine (never excelize's — see the package
@@ -35,7 +60,7 @@ const importActor uint64 = 0
// platform logs this app cannot grep, so "cannot parse cell B7" is what
// gets said — never what B7 actually contained.
func Import(r io.Reader, h *store.Handle) error {
- f, err := excelize.OpenReader(r)
+ f, err := excelize.OpenReader(r, openLimits)
if err != nil {
return fmt.Errorf("xlsx: open workbook: %w", err)
}