docs(07): mark review findings fixed
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
4ea4d28e6e
commit
7d65cb4d94
2 changed files with 79 additions and 1 deletions
71
.planning/phases/07-deploy-v1/07-REVIEW-FIX.md
Normal file
71
.planning/phases/07-deploy-v1/07-REVIEW-FIX.md
Normal file
|
|
@ -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_
|
||||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Reference in a new issue