diff --git a/.planning/phases/05-files/05-REVIEW.md b/.planning/phases/05-files/05-REVIEW.md new file mode 100644 index 0000000..9e96acd --- /dev/null +++ b/.planning/phases/05-files/05-REVIEW.md @@ -0,0 +1,200 @@ +--- +phase: 05-files +reviewed: 2026-05-15T00:00:00Z +depth: standard +files_reviewed: 11 +files_reviewed_list: + - backend/cmd/web/main.go + - backend/compose.yaml + - backend/go.mod + - backend/internal/db/queries/files.sql + - backend/internal/files/store.go + - backend/internal/web/handlers_files.go + - backend/internal/web/handlers_tablos.go + - backend/internal/web/router.go + - backend/migrations/0005_files.sql + - backend/templates/files_helpers.go + - backend/templates/files.templ + - backend/templates/tablos.templ +findings: + critical: 1 + warning: 4 + info: 2 + total: 7 +status: issues_found +--- + +# Phase 05: Code Review Report + +**Reviewed:** 2026-05-15 +**Depth:** standard +**Files Reviewed:** 11 +**Status:** issues_found + +## Summary + +Phase 5 implements file upload, listing, download (presigned URL), and delete for tablo owners. The ownership model is consistently applied via `loadOwnedTablo` / `loadOwnedTabloForFile`, content-type sniffing is server-side, and S3 keys are UUID-based with no user-controlled path component. These are solid foundations. + +However, one critical data-integrity bug exists: when a successful S3 upload is followed by a failing DB insert, the uploaded object is silently orphaned with no logging, no cleanup, and no client error that accurately reflects what happened. Four warnings cover the global 15 s `WriteTimeout` killing large-file uploads, an un-validated empty filename stored to the DB, a dead code variable left in production, and a bespoke `itoa` helper that reimplements `strconv.Itoa`. Two informational notes cover the missing `UNIQUE` constraint on `s3_key` and the upload error path not checking `HX-Request` before writing a 422. + +--- + +## Critical Issues + +### CR-01: S3 object orphaned silently when DB insert fails after successful upload + +**File:** `backend/internal/web/handlers_files.go:178-196` + +**Issue:** `deps.Files.Upload` streams the object to S3 successfully (lines 178-183). If the subsequent `deps.Queries.InsertTabloFile` call fails (lines 185-196), the handler logs the DB error and returns HTTP 500 — but the S3 object is never deleted and no log entry records the orphaned key. The caller receives a 500 and believes the upload failed; the file persists in S3 with no DB record and no way to reconcile it (Phase 6 worker does not exist yet). This is a data-integrity bug: storage leaks on every DB error after a successful upload. + +**Fix:** Delete the S3 object before returning on DB insert failure. The context may already be cancelled if the DB error was a timeout, so use a fresh background context with a short deadline: + +```go +_, err = deps.Queries.InsertTabloFile(r.Context(), sqlc.InsertTabloFileParams{ + TabloID: tablo.ID, + S3Key: s3Key, + Filename: header.Filename, + ContentType: contentType, + SizeBytes: bytesWritten, +}) +if err != nil { + slog.Default().Error("files upload: InsertTabloFile failed", "tablo_id", tablo.ID, "s3_key", s3Key, "err", err) + // Best-effort cleanup — orphan prevention until Phase 6 reconciler exists. + cleanupCtx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + if delErr := deps.Files.Delete(cleanupCtx, s3Key); delErr != nil { + slog.Default().Error("files upload: S3 cleanup after DB failure", "s3_key", s3Key, "err", delErr) + } + http.Error(w, "internal server error", http.StatusInternalServerError) + return +} +``` + +--- + +## Warnings + +### WR-01: Global 15 s `WriteTimeout` will abort large file uploads in flight + +**File:** `backend/cmd/web/main.go:122-123` + +**Issue:** `http.Server.WriteTimeout` applies to the entire response write, including the request body read that happens during `ParseMultipartForm`. For a 25 MB file on a slow connection (e.g. 2 Mbit/s), reading the body alone takes ~100 s, which exceeds the 15 s timeout. The server will cut the connection mid-upload, leaving the client with a TCP reset and no meaningful error. The `MaxBytesReader` limit (lines 146-147 of `handlers_files.go`) only bounds _size_, not _time_. + +**Fix:** Either increase `WriteTimeout` to a value that accommodates `MAX_UPLOAD_SIZE_MB` at a minimum expected upload speed, or (better) set a dedicated per-route timeout for upload endpoints using `http.TimeoutHandler` or by upgrading to `http.Server.WriteTimeout = 0` and relying on `ReadTimeout` + handler-level deadlines: + +```go +// Option A: raise to accommodate MAX_UPLOAD_SIZE_MB at worst-case bandwidth. +// 25 MB at 256 KB/s = ~100 s; choose a generous multiple. +WriteTimeout: 120 * time.Second, + +// Option B: keep the default low global timeout and wrap upload routes: +// r.With(http.TimeoutHandler(nil, 120*time.Second, "upload timeout")). +// Post("/tablos/{id}/files", FileUploadHandler(fileDeps)) +``` + +--- + +### WR-02: Empty `header.Filename` is accepted and stored to DB + +**File:** `backend/internal/web/handlers_files.go:168-173, 185-192` + +**Issue:** `r.FormFile("file")` returns a `*multipart.FileHeader` whose `Filename` field can be an empty string when a client submits a multipart upload without a `filename` parameter in the `Content-Disposition` header (e.g. raw `curl` calls). `header.Filename` is stored directly in `InsertTabloFileParams.Filename` (line 188) without any validation. A DB row is written with an empty `filename`, which the template then renders as a blank filename in the file list, degrading the UI and making the file unidentifiable. + +**Fix:** Validate `header.Filename` before the S3 upload: + +```go +filename := strings.TrimSpace(header.Filename) +if filename == "" { + http.Error(w, "bad request: file must have a filename", http.StatusBadRequest) + return +} +// Use filename (not header.Filename) in InsertTabloFileParams. +``` + +--- + +### WR-03: Dead variable `errMsg` left in production handler + +**File:** `backend/internal/web/handlers_files.go:159-161` + +**Issue:** Lines 159 and 161 declare `errMsg` as a format string and then immediately discard it with `_ = errMsg`. The actual error message is constructed by `formatMBError` on line 160 and passed correctly to `UploadErrorFragment`. The `errMsg` variable is leftover from a draft or earlier approach and is never used. In Go, an unused local variable that is explicitly silenced with `_ =` is dead code and misleading to readers — it suggests the variable serves a purpose. + +**Fix:** Delete lines 159 and 161: + +```go +// Before: +errMsg := "File too large (max %d MB)." +_ = templates.UploadErrorFragment(tablo, fileList, csrf.Token(r), formatMBError(deps.MaxUploadMB)).Render(r.Context(), w) +_ = errMsg + +// After: +_ = templates.UploadErrorFragment(tablo, fileList, csrf.Token(r), formatMBError(deps.MaxUploadMB)).Render(r.Context(), w) +``` + +--- + +### WR-04: Custom `itoa` reimplements `strconv.Itoa` without benefit + +**File:** `backend/internal/web/handlers_files.go:223-242` + +**Issue:** The `itoa` helper (lines 223-242) is a hand-rolled integer-to-string converter. The comment on line 223 states it exists "without importing strconv in this file." `strconv` is a standard library package with zero external dependencies; there is no valid reason to avoid it. The custom implementation introduces a subtle bug: it silently produces an incorrect result for `math.MinInt` (negating `math.MinInt` overflows to itself in two's-complement arithmetic), though this edge case is unreachable given `maxMB` is validated as positive. More importantly, the bespoke code is maintenance surface with no upside. + +**Fix:** Remove `itoa` and `formatMBError`, replace with a `strconv.Itoa` call directly in the one call site: + +```go +// In FileUploadHandler, at the UploadErrorFragment call: +errMsg := "File too large (max " + strconv.Itoa(deps.MaxUploadMB) + " MB)." +_ = templates.UploadErrorFragment(tablo, fileList, csrf.Token(r), errMsg).Render(r.Context(), w) +``` + +--- + +## Info + +### IN-01: No `UNIQUE` constraint on `s3_key` in `tablo_files` + +**File:** `backend/migrations/0005_files.sql:9` + +**Issue:** `s3_key` is declared `NOT NULL` but has no `UNIQUE` constraint. The S3 key is constructed from a `uuid.New()` per upload, making collisions astronomically unlikely in practice. However, the DB provides no hard guarantee. If a duplicate key were somehow inserted (e.g. via a test fixture or a future migration that replays records), `Delete` would remove the S3 object but two DB rows would still reference it, or a re-upload to the same key could silently overwrite existing data. + +**Fix:** Add the constraint to the migration: + +```sql +s3_key text NOT NULL UNIQUE, +``` + +--- + +### IN-02: Upload error path (file-too-large) does not differentiate HTMX vs non-HTMX + +**File:** `backend/internal/web/handlers_files.go:151-162` + +**Issue:** When `ParseMultipartForm` fails with `*http.MaxBytesError`, the handler always renders the `UploadErrorFragment` HTML with a 422 status, regardless of whether the request came from HTMX or a plain form submit. For a non-HTMX browser submit, this renders a bare fragment (no surrounding layout) into the full page response, producing a broken page. The success path at line 207 correctly branches on `HX-Request`, but the error path at line 151 does not. + +**Fix:** Mirror the HTMX branch check used elsewhere: + +```go +if errors.As(err, &mbErr) { + fileList, _ := deps.Queries.ListFilesByTablo(r.Context(), tablo.ID) + if fileList == nil { + fileList = []sqlc.TabloFile{} + } + errMsg := formatMBError(deps.MaxUploadMB) + w.Header().Set("Content-Type", "text/html; charset=utf-8") + w.WriteHeader(http.StatusUnprocessableEntity) + if r.Header.Get("HX-Request") == "true" { + _ = templates.UploadErrorFragment(tablo, fileList, csrf.Token(r), errMsg).Render(r.Context(), w) + return + } + // Non-HTMX: render full page with error embedded. + // (requires passing user through; consider extracting from loadOwnedTablo return) + http.Redirect(w, r, "/tablos/"+tablo.ID.String()+"/files?err=too_large", http.StatusSeeOther) + return +} +``` + +--- + +_Reviewed: 2026-05-15_ +_Reviewer: Claude (gsd-code-reviewer)_ +_Depth: standard_