summercms/.planning/phases/05-data-layer-full-fidelity/05-REVIEW.md

phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status

phase	reviewed	depth	files_reviewed	files_reviewed_list	findings	status
05-data-layer-full-fidelity	2026-09-19T16:30:00Z	standard	34	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	critical warning info total 1 5 4 10	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:

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:

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:

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