223 lines
11 KiB
Markdown
223 lines
11 KiB
Markdown
---
|
||
phase: 05-data-layer-full-fidelity
|
||
reviewed: 2026-09-19T16:30:00Z
|
||
depth: standard
|
||
files_reviewed: 34
|
||
files_reviewed_list:
|
||
- lagoon/attach/bucket.go
|
||
- lagoon/attach/bucket_test.go
|
||
- lagoon/attach/file.go
|
||
- lagoon/attach/file_test.go
|
||
- lagoon/attach/lifecycle_test.go
|
||
- lagoon/attach/migrations.go
|
||
- lagoon/attach/static.go
|
||
- lagoon/attach/static_test.go
|
||
- lagoon/attach/thumb.go
|
||
- lagoon/attach/thumb_test.go
|
||
- lagoon/commands.go
|
||
- lagoon/connection.go
|
||
- lagoon/encrypted.go
|
||
- lagoon/encrypted_test.go
|
||
- lagoon/fill_fuzz_test.go
|
||
- lagoon/fill.go
|
||
- lagoon/fill_test.go
|
||
- lagoon/hidden_marshal_test.go
|
||
- lagoon/jsonable.go
|
||
- lagoon/jsonable_test.go
|
||
- lagoon/keygen.go
|
||
- lagoon/keygen_test.go
|
||
- lagoon/laravel_decrypt.go
|
||
- lagoon/laravel_decrypt_test.go
|
||
- lagoon/lifecycle.go
|
||
- lagoon/lifecycle_test.go
|
||
- lagoon/migrations.go
|
||
- lagoon/migrations_test.go
|
||
- lagoon/paginate.go
|
||
- lagoon/paginate_test.go
|
||
- lagoon/relations.go
|
||
- lagoon/relations_test.go
|
||
- lagoon/validate.go
|
||
- lagoon/validate_test.go
|
||
findings:
|
||
critical: 1
|
||
warning: 5
|
||
info: 4
|
||
total: 10
|
||
status: issues_found
|
||
---
|
||
|
||
# Phase 5: Code Review Report
|
||
|
||
**Reviewed:** 2026-09-19T16:30:00Z
|
||
**Depth:** standard
|
||
**Files Reviewed:** 34
|
||
**Status:** issues_found
|
||
|
||
## Summary
|
||
|
||
This pass covers the current `lagoon` / `lagoon/attach` sources only (the previous review's fonoteka.go findings were re-checked against this tree and are not restated). Fill, Encrypted, Laravel-decrypt-off-path, plugin-isolated migrations, and the `thumb_<id>_` delete prefix all hold.
|
||
|
||
The attachment public URL path does not. `File.Thumb` stores and advertises keys under the *original* file's partition, while `StaticHandler` rebuilds the blob key from the last path segment via `PartitionDirectory(filename)`. Thumbnail URLs therefore 404. That is a ship-stopper for any consumer of `Thumb`. Remaining issues are resource limits on thumb generation, a GORM bool/default trap on `is_public`, Laravel `between`/`min`/`max` translation mistakes, and a poisonable thumb cache on encode failure.
|
||
|
||
## Narrative Findings (AI reviewer)
|
||
|
||
## Critical Issues
|
||
|
||
### CR-01: `StaticHandler` cannot serve the URLs `File.Thumb` returns
|
||
|
||
**File:** `lagoon/attach/static.go:32-37`
|
||
**Also:** `lagoon/attach/thumb.go:117-126`, `lagoon/attach/static.go:72-91`
|
||
|
||
**Issue:** Thumbnails are stored as `PartitionDirectory(originalDiskName) + ThumbFilename(...)`, and `Thumb` returns that key under `PublicPathPrefix()`:
|
||
|
||
```
|
||
/storage/uploads/abc/123/xyz/thumb_42_200_200_0_0_crop.jpg
|
||
```
|
||
|
||
`StaticHandler` then takes the last segment as `disk_name` and requires `PartitionDirectory(disk_name)` to equal the first three path groups, then reads `BlobKey(disk_name)`:
|
||
|
||
- last segment = `thumb_42_200_200_0_0_crop.jpg`
|
||
- `PartitionDirectory` of that name = `thu/mb_/42_`
|
||
- request partition = `abc/123/xyz` → mismatch → 404
|
||
|
||
Even if the partition check were dropped, `BlobKey("thumb_42_...")` would open `thu/mb_/42_/thumb_42_...`, which is not where the bytes were written.
|
||
|
||
Winter serves thumbs as static files at `partition(original)/thumb_...`. The "rebuild key from filename" check is correct for originals and fatal for thumbs. `TestStaticHandler` only fetches an original; `TestFileThumbResizesOnce` asserts the URL string and never GETs it through the handler.
|
||
|
||
**Fix:** After rejecting `..` / empty / extra segments, use the validated 4-segment path as the blob key. Keep the partition-matches-filename check only for non-thumb names:
|
||
|
||
```go
|
||
func parsePublicBlobPath(p string) (key string, ok bool) {
|
||
if p == "" || strings.Contains(p, "\\") || strings.Contains(p, "..") || strings.Contains(p, "//") {
|
||
return "", false
|
||
}
|
||
parts := strings.Split(p, "/")
|
||
if len(parts) != 4 {
|
||
return "", false
|
||
}
|
||
for _, part := range parts {
|
||
if part == "" || part == "." || part == ".." {
|
||
return "", false
|
||
}
|
||
}
|
||
filename := parts[3]
|
||
got := strings.Join(parts[:3], "/")
|
||
if !strings.HasPrefix(filename, "thumb_") {
|
||
want := strings.TrimSuffix(PartitionDirectory(filename), "/")
|
||
if got != want {
|
||
return "", false
|
||
}
|
||
}
|
||
return got + "/" + filename, true
|
||
}
|
||
|
||
// StaticHandler: key, ok := parsePublicBlobPath(rel); reader, err := bucket.NewReader(r.Context(), key, nil)
|
||
```
|
||
|
||
Add a test that writes a thumb at `part+thumbName`, calls `StaticHandler`, and GETs the URL `Thumb` returns.
|
||
|
||
## Warnings
|
||
|
||
### WR-01: `Thumb` has no bounds on dimensions or decoded image size
|
||
|
||
**File:** `lagoon/attach/thumb.go:99-140`
|
||
|
||
**Issue:** `w` and `h` are interpolated into the blob key and passed straight to `imaging.Fill`/`Resize`/`Fit`. Negative values produce odd filenames (`thumb_1_-1_-1_...`) and empty images; large values (e.g. `100000x100000`) allocate on the order of width×height×4 bytes. `image.Decode` is also unbounded, so a decompression bomb uploaded as an original can OOM the process on the first thumb request.
|
||
|
||
Winter getThumb is typically driven by request parameters. This method is the production primitive those handlers will call.
|
||
|
||
**Fix:** Reject non-positive and oversized dimensions before any bucket I/O, and cap the decoder:
|
||
|
||
```go
|
||
const maxThumbEdge = 4096
|
||
if w <= 0 || h <= 0 || w > maxThumbEdge || h > maxThumbEdge {
|
||
return "", fmt.Errorf("attach: thumb size %dx%d is out of range", w, h)
|
||
}
|
||
r = io.LimitReader(r, 32<<20) // then image.Decode
|
||
```
|
||
|
||
### WR-02: A failed thumb encode still commits the blob, which then caches forever
|
||
|
||
**File:** `lagoon/attach/thumb.go:148-156`
|
||
|
||
**Issue:** `encodeImage` writes into the blob writer, then `wr.Close()` always runs. gocloud `Writer.Close` finalizes the object. If encode fails after some bytes (or Close succeeds with a partial body), the next `Thumb` call hits `bucket.Exists` and returns the public URL of the corrupt object without regenerating.
|
||
|
||
**Fix:** On `encErr != nil`, delete the key after Close (ignore NotFound) and return the encode error. Do not treat Exists as authoritative unless Close of a successful encode is what created it. Prefer write-to-temp-then-rename if the driver supports it; otherwise delete-on-failure is enough.
|
||
|
||
### WR-03: Laravel `between`/`min`/`max` are translated as go-playground length tags; `nullable` plus `omitempty` lets numeric `0` skip the range
|
||
|
||
**File:** `lagoon/validate.go:54-72` and `lagoon/validate.go:109-117`
|
||
|
||
**Issue:** Two Laravel-parity holes remain after the `*int`/float64 integer fix:
|
||
|
||
1. `between:1889,2100` becomes `min=1889,max=2100`. go-playground `min`/`max` on a **string** is length, not numeric value. Form/query input `"1991"` (len 4) fails `integer|between:1889,2100`. Laravel would coerce and accept it.
|
||
2. `nullable` always appends `omitempty`. For a JSON `float64(0)` or `int(0)`, go-playground treats 0 as empty and skips `min`/`max`. Laravel does not treat 0 as empty, so `nullable|integer|between:1889,2100` with `year=0` must fail. `isEmptyValue` correctly says 0 is not empty, but that only gates the early return; the later `validator.Var` still sees `omitempty`.
|
||
|
||
`TestValidateIntegerPointerAndJSONNumber` never sends `0` or a digit string, so both holes are untested. SaveAlbum currently validates a filled `*int` (where `*0` is not omitempty), which hides this from that one caller.
|
||
|
||
**Fix:** For `integer`/`numeric` (and `between` on those types), do not emit go-playground `min`/`max`/`omitempty`. Compare with `big.Rat`/`big.Int` as `numeric|min` already does for money. Keep `omitempty` off numeric values; use only `isEmptyValue` (nil/empty string) for the nullable short-circuit.
|
||
|
||
### WR-04: `File.IsPublic` zero value writes `false`, opposite Winter and the SQL default
|
||
|
||
**File:** `lagoon/attach/file.go:35`
|
||
**Also:** `lagoon/attach/migrations.go:27`
|
||
|
||
**Issue:** The migration is `is_public BOOLEAN NOT NULL DEFAULT TRUE`. The Go field is a non-pointer `bool` with no `default:true` (and no `BeforeCreate`). GORM Create includes zero values, so `gdb.Create(&attach.File{DiskName: ..., FileName: ...})` persists `is_public=false`. Winter `File::create()` is public unless told otherwise. Any upload path that forgets the flag will store protected rows (and, with WR-05, still serve them until an `is_public` gate exists).
|
||
|
||
**Fix:**
|
||
|
||
```go
|
||
IsPublic bool `gorm:"column:is_public;default:true"`
|
||
```
|
||
|
||
and/or set `f.IsPublic = true` in `BeforeCreate` when the caller left it at the zero value. Prefer `*bool` only if you need SQL NULL; Winter does not.
|
||
|
||
### WR-05: `StaticHandler` serves every matching blob and never consults `is_public`
|
||
|
||
**File:** `lagoon/attach/static.go:16-53`
|
||
|
||
**Issue:** There is one bucket (`storage.uploads.bucket_url`) and one handler. `File.IsPublic` is unused outside the struct/migration. Anyone who knows (or can brute a UUID) `disk_name` gets the bytes, including rows saved with `is_public=false`. T-05-16 deferred the gate to route wiring; the exported handler is still unsafe to mount as-is, and the function comment does not say so.
|
||
|
||
**Fix:** Either document `StaticHandler` as public-disk-only and store protected files in a second bucket, or look up `system_files` by `disk_name` and 404 when `!IsPublic`. Do the lookup before `NewReader`. Do not mount this handler on the app origin until that gate exists (same-origin Content-Type from uploads is also XSS-relevant).
|
||
|
||
## Info
|
||
|
||
### IN-01: `HasFillable` / `HasHidden` are never called
|
||
|
||
**File:** `lagoon/fill.go:15-23`
|
||
|
||
**Issue:** `Fill` only honors the `allowed` argument; it never intersects `model.(HasFillable).Fillable()`. `HasHidden` is never read; hiding is entirely `json:"-"` (and `Encrypted`'s own `MarshalJSON`). A caller that passes a superset of `Fillable()` will mass-assign columns the model declared off-limits. That is the opposite of a backstop.
|
||
|
||
**Fix:** If `model` implements `HasFillable`, intersect `allowed` with `Fillable()` inside `Fill`. Keep `json:"-"` as the marshal guarantee; optionally assert `Hidden()` ⊆ json-ignored tags in the existing hidden-marshal test.
|
||
|
||
### IN-02: `TestNoAutoMigrate` does not scan `attach/`
|
||
|
||
**File:** `lagoon/migrations_test.go:79-96`
|
||
|
||
**Issue:** `os.ReadDir(".")` skips directories, so `lagoon/attach/*.go` is never checked. The test claims lagoon production files must not contain `AutoMigrate` and then ignores the only subpackage that owns a schema.
|
||
|
||
**Fix:** Walk recursively (or `filepath.Glob("../*.go")` plus `attach/*.go` from the lagoon directory), still skipping `_test.go`.
|
||
|
||
### IN-03: `migrate:status` omits the framework `system_files` set
|
||
|
||
**File:** `lagoon/migrations.go:148-176`
|
||
**Also:** `lagoon/commands.go:52-72`
|
||
|
||
**Issue:** `Migrate` always applies `summercms.attach` first, but `Status` / `migrate:status` only iterate plugins. Operators cannot see whether `202609180001_create_system_files` is applied, and `migrate:rollback` cannot target it.
|
||
|
||
**Fix:** Prepend a status row for `summercms.attach` / `HistoryTableName("summercms.attach")`. Rollback of that set can stay explicit (`--plugin summercms.attach`) if you do not want it as the default last target.
|
||
|
||
### IN-04: Money/between validator failures always use the `max` message (and `min` params are empty)
|
||
|
||
**File:** `lagoon/validate.go:104-116` and `lagoon/validate.go:318-334`
|
||
|
||
**Issue:** `numeric|min:0` with a negative value reports `"The field may not be greater than ."`. `integer|between` failures take `firstNonOmit` = `min` with `params` nil, so `"The year must be at least ."`. Invalid data is still rejected; the Laravel-shaped message is wrong.
|
||
|
||
**Fix:** Distinguish min vs max vs between when building the message, and pass the bound values in `params`.
|
||
|
||
---
|
||
|
||
_Reviewed: 2026-09-19T16:30:00Z_
|
||
_Reviewer: Claude (gsd-code-reviewer)_
|
||
_Depth: standard_
|