diff --git a/.planning/phases/07-deploy-v1/07-REVIEW-FIX.md b/.planning/phases/07-deploy-v1/07-REVIEW-FIX.md new file mode 100644 index 0000000..6ce52eb --- /dev/null +++ b/.planning/phases/07-deploy-v1/07-REVIEW-FIX.md @@ -0,0 +1,71 @@ +--- +phase: 07-deploy-v1 +fixed_at: 2026-05-15T00:00:00Z +review_path: .planning/phases/07-deploy-v1/07-REVIEW.md +iteration: 1 +findings_in_scope: 6 +fixed: 6 +skipped: 0 +status: all_fixed +--- + +# Phase 07: Code Review Fix Report + +**Fixed at:** 2026-05-15T00:00:00Z +**Source review:** .planning/phases/07-deploy-v1/07-REVIEW.md +**Iteration:** 1 + +**Summary:** +- Findings in scope: 6 (CR-02, WR-01, WR-02, WR-03, WR-04, WR-05) +- Fixed: 6 +- Skipped: 0 + +Note: CR-01 and CR-03 were pre-fixed per fix guidance and not re-applied. + +## Fixed Issues + +### CR-02: File-Upload Race Condition — defer cancel() placement + +**Files modified:** `backend/internal/web/handlers_files.go` +**Commit:** fbda7cb +**Applied fix:** Removed `defer cancel()` and replaced with an explicit `cancel()` call immediately after `deps.Files.Delete` returns, so the context timeout is released right after the cleanup operation rather than when the handler function returns. + +### WR-01: NewRouter returns error instead of panicking on bad static FS + +**Files modified:** `backend/internal/web/router.go`, `backend/cmd/web/main.go`, `backend/internal/web/csrf_test.go`, `backend/internal/web/handlers_auth_test.go`, `backend/internal/web/handlers_files_test.go`, `backend/internal/web/handlers_tablos_test.go`, `backend/internal/web/handlers_tasks_test.go`, `backend/internal/web/handlers_test.go` +**Commit:** b61f36f +**Applied fix:** Changed `NewRouter` signature from `http.Handler` to `(http.Handler, error)`. The `fs.Sub` panic is replaced with an error return using `fmt.Errorf`. `main.go` handles the error with `slog.Error` + `os.Exit(1)`. All test helper functions and inline test calls updated to handle the error return (test helpers use `panic`, inline test calls use `t.Fatalf`). Also adds `"fmt"` to router.go imports. + +### WR-02: Rate limit check moved before validation in LoginPostHandler + +**Files modified:** `backend/internal/web/handlers_auth.go` +**Commit:** ab12bf0 +**Applied fix:** Moved the rate-limit check (`deps.Limiter.Allow`) to before the email/password validation block. The key is derived from the already-trimmed `email` variable (lowercased) plus IP. Malformed inputs now consume rate-limit tokens, preventing timing-based enumeration via invalid payloads. Updated function docstring step order accordingly. + +### WR-03: SetMaxOpenConns(2) added in migrate.go + +**Files modified:** `backend/internal/db/migrate.go` +**Commit:** e7a66c4 +**Applied fix:** Added `sqlDB.SetMaxOpenConns(2)` immediately after `sql.Open`, before `defer sqlDB.Close()`. Goose runs migrations sequentially so a pool of 2 is sufficient, preventing the default unlimited pool from opening unnecessary connections. + +### WR-04: Both tests pass "localhost" as trusted origin + +**Files modified:** `backend/internal/web/handlers_test.go` (included in WR-01 commit) +**Commit:** b61f36f +**Applied fix:** Both `TestIndex_UnauthRedirects` and `TestDemoTime_Fragment` now pass `"localhost"` as a trusted origin to `NewRouter`. Also fixed `TestRequestID_HeaderSet` for consistency. The fix was applied as part of the WR-01 commit since all three tests required the same NewRouter signature update. + +### WR-05: Filename sanitized with filepath.Base and length cap + +**Files modified:** `backend/internal/web/handlers_files.go` +**Commit:** 4ea4d28 +**Applied fix:** Added `"path/filepath"` import. After the existing empty-filename check, added: `filename = filepath.Base(filename)` to strip path components, a length cap at 255 bytes, and a check for empty-or-dot result (returns 400). This prevents path-separator filenames from being stored in the database. + +## Skipped Issues + +None — all in-scope findings were fixed. + +--- + +_Fixed: 2026-05-15T00:00:00Z_ +_Fixer: Claude (gsd-code-fixer)_ +_Iteration: 1_ diff --git a/.planning/phases/07-deploy-v1/07-REVIEW.md b/.planning/phases/07-deploy-v1/07-REVIEW.md index c81c8d9..c6f7da3 100644 --- a/.planning/phases/07-deploy-v1/07-REVIEW.md +++ b/.planning/phases/07-deploy-v1/07-REVIEW.md @@ -23,7 +23,14 @@ findings: warning: 5 info: 3 total: 11 -status: issues_found +status: fixed +fixed_findings: + - CR-02 + - WR-01 + - WR-02 + - WR-03 + - WR-04 + - WR-05 --- # Phase 07: Code Review Report