69 lines
4.2 KiB
Markdown
69 lines
4.2 KiB
Markdown
---
|
||
phase: 05-data-layer-full-fidelity
|
||
fixed_at: 2026-09-19T13:37:26Z
|
||
review_path: .planning/phases/05-data-layer-full-fidelity/05-REVIEW.md
|
||
iteration: 1
|
||
findings_in_scope: 6
|
||
fixed: 6
|
||
skipped: 0
|
||
status: all_fixed
|
||
---
|
||
|
||
# Phase 5: Code Review Fix Report
|
||
|
||
**Fixed at:** 2026-09-19T13:37:26Z
|
||
**Source review:** .planning/phases/05-data-layer-full-fidelity/05-REVIEW.md
|
||
**Iteration:** 1
|
||
|
||
**Summary:**
|
||
- Findings in scope: 6 (1 critical, 5 warning; Info findings out of scope)
|
||
- Fixed: 6
|
||
- Skipped: 0
|
||
|
||
## Fixed Issues
|
||
|
||
### CR-01: `StaticHandler` cannot serve the URLs `File.Thumb` returns
|
||
|
||
**Files modified:** `lagoon/attach/static.go`, `lagoon/attach/static_test.go`
|
||
**Commit:** 44126cc
|
||
**Applied fix:** `parsePublicBlobPath` now returns the validated 4-segment path as the blob key. The partition-matches-filename check is skipped for `thumb_*` names, which Winter stores beside the original. `StaticHandler` opens that key directly instead of rebuilding via `BlobKey(last-segment)`. `TestStaticHandlerServesThumbURL` generates a thumb, then GETs the URL `Thumb` returns through the handler.
|
||
|
||
### WR-01: `Thumb` has no bounds on dimensions or decoded image size
|
||
|
||
**Files modified:** `lagoon/attach/thumb.go`, `lagoon/attach/thumb_test.go`
|
||
**Commit:** c211822
|
||
**Applied fix:** Reject `w`/`h` that are non-positive or larger than 4096 before any bucket I/O. Cap the decoder with `io.LimitReader` at 32MiB and reject decoded images above 4096×4096 pixels. `TestFileThumbRejectsOutOfRangeSize` asserts rejected sizes never touch the bucket.
|
||
|
||
### WR-02: A failed thumb encode still commits the blob, which then caches forever
|
||
|
||
**Files modified:** `lagoon/attach/thumb.go`, `lagoon/attach/thumb_test.go`
|
||
**Commit:** e54f9d7
|
||
**Applied fix:** After `encodeImage` / `Writer.Close`, a failure deletes the thumb key (NotFound ignored) so the next `Thumb` regenerates instead of serving a partial object. `TestFileThumbEncodeFailureDoesNotCache` injects an encode error, asserts the blob is gone, then retries successfully.
|
||
|
||
### WR-03: Laravel `between`/`min`/`max` are translated as length tags; `nullable` plus `omitempty` lets numeric `0` skip the range
|
||
|
||
**Files modified:** `lagoon/validate.go`, `lagoon/validate_test.go`
|
||
**Commit:** fb12b22
|
||
**Status:** fixed: requires human verification
|
||
**Applied fix:** `nullable` no longer emits go-playground `omitempty`; empty is gated only by `isEmptyValue` (nil / empty string, not numeric 0). `integer`/`numeric` `between`/`min`/`max` compare with `big.Rat` via `numericString` (dereferences `*int` and accepts digit strings) instead of go-playground length `min`/`max`. String-length `between`/`min`/`max` are unchanged. Tests: digit string `"1991"` passes; `0` / `float64(0)` / `"0"` fail `integer|between:1889,2100`; numeric `0` still passes `min:0`.
|
||
|
||
### WR-04: `File.IsPublic` zero value writes `false`, opposite Winter and the SQL default
|
||
|
||
**Files modified:** `lagoon/attach/file.go`, `lagoon/attach/file_test.go`, `lagoon/attach/lifecycle_test.go`
|
||
**Commit:** c12e657
|
||
**Status:** fixed: requires human verification
|
||
**Applied fix:** `IsPublic` is `*bool` with `gorm:"column:is_public;not null;default:true"` plus `File.Public()` (nil → true). A non-pointer `bool` with `default:true` cannot persist false because GORM replaces the zero value with the default. `Create` without the field stores true; explicit `&false` stores false (`TestFileCreateDefaultsIsPublic`).
|
||
|
||
### WR-05: `StaticHandler` serves every matching blob and never consults `is_public`
|
||
|
||
**Files modified:** `lagoon/attach/static.go`, `lagoon/attach/file.go`, `lagoon/attach/static_test.go`, `lagoon/attach/file_test.go`, `lagoon/attach/lifecycle_test.go`
|
||
**Commit:** 56156ae
|
||
**Status:** fixed: requires human verification
|
||
**Applied fix:** Documented `StaticHandler` as public-disk-only (must not hold protected files; do not mount ungated on the app origin). Added `StaticHandlerPublic`, which looks up `system_files` by `disk_name` (or file ID parsed from `thumb_<id>_…`) **before** `NewReader` and 404s on missing rows and `is_public=false`. Stub test proves the gate runs before the blob is opened; postgres test covers public/private originals and thumbs.
|
||
|
||
---
|
||
|
||
_Fixed: 2026-09-19T13:37:26Z_
|
||
_Fixer: Claude (gsd-code-fixer)_
|
||
_Iteration: 1_
|