docs(05): add code review report
This commit is contained in:
Arthur Belleville
parent
aab336fa36
commit
f93ef972ad
1 changed files with 200 additions and 0 deletions
200
.planning/phases/05-files/05-REVIEW.md
Normal file
200
.planning/phases/05-files/05-REVIEW.md
Normal file
|
|
@ -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_
|
||||
Loading…
Reference in a new issue