golem15/summercms
3
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

docs(05): add code review report

Browse Source
This commit is contained in:
Jakub Zych
2026-09-19 00:20:17 +02:00
parent bbee45c5f4
commit ab4ac856da
1 changed files with 132 additions and 301 deletions
Download Patch File Download Diff File Expand all files Collapse all files

433
.planning/phases/05-data-layer-full-fidelity/05-REVIEW.md
View File

@@ -1,391 +1,222 @@
---
phase: 05-data-layer-full-fidelity
reviewed: 2026-09-18T20:15:00Z
reviewed: 2026-09-19T16:30:00Z
depth: standard
files_reviewed: 72
files_reviewed: 34
files_reviewed_list:
- lagoon/fill.go
- lagoon/fill_test.go
- lagoon/fill_fuzz_test.go
- lagoon/hidden_marshal_test.go
- lagoon/lifecycle.go
- lagoon/lifecycle_test.go
- lagoon/paginate.go
- lagoon/paginate_test.go
- lagoon/relations.go
- lagoon/validate.go
- lagoon/validate_test.go
- lagoon/jsonable.go
- lagoon/jsonable_test.go
- lagoon/encrypted.go
- lagoon/encrypted_test.go
- lagoon/keygen.go
- lagoon/laravel_decrypt.go
- lagoon/laravel_decrypt_test.go
- lagoon/commands.go
- lagoon/migrations.go
- lagoon/connection.go
- lagoon/order.go
- lagoon/attach/file.go
- lagoon/attach/thumb.go
- lagoon/attach/thumb_test.go
- 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
- fonoteka.go/plugins/golem15/fonoteka/plugin.go
- fonoteka.go/plugins/golem15/fonoteka/models/album.go
- fonoteka.go/plugins/golem15/fonoteka/models/collection.go
- fonoteka.go/plugins/golem15/fonoteka/models/genre.go
- fonoteka.go/plugins/golem15/fonoteka/models/artist.go
- fonoteka.go/plugins/golem15/fonoteka/models/style.go
- fonoteka.go/plugins/golem15/fonoteka/models/money_string.go
- fonoteka.go/plugins/golem15/fonoteka/models/user_ai_credential.go
- fonoteka.go/plugins/golem15/fonoteka/models/org_ai_credential.go
- fonoteka.go/plugins/golem15/fonoteka/models/user_discogs_credential.go
- fonoteka.go/plugins/golem15/fonoteka/models/org_discogs_credential.go
- fonoteka.go/plugins/golem15/fonoteka/models/api_token.go
- fonoteka.go/plugins/golem15/fonoteka/models/oauth_client.go
- fonoteka.go/plugins/golem15/fonoteka/models/oauth_auth_code.go
- fonoteka.go/plugins/golem15/fonoteka/models/oauth_refresh_token.go
- fonoteka.go/plugins/golem15/fonoteka/models/collection_invitation.go
- fonoteka.go/plugins/golem15/fonoteka/models/registry.go
- fonoteka.go/plugins/golem15/fonoteka/models/settings.go
- fonoteka.go/plugins/golem15/fonoteka/models/notification.go
- fonoteka.go/plugins/golem15/fonoteka/models/csv_import.go
- fonoteka.go/plugins/golem15/fonoteka/models/csv_import_row.go
- fonoteka.go/plugins/golem15/fonoteka/models/slug.go
- fonoteka.go/plugins/golem15/fonoteka/classes/album_write_service.go
- fonoteka.go/plugins/golem15/fonoteka/classes/collection_write_service.go
- fonoteka.go/plugins/golem15/fonoteka/classes/join_tables.go
- fonoteka.go/plugins/golem15/fonoteka/classes/serialize.go
- fonoteka.go/plugins/golem15/fonoteka/classes/artist_resolver.go
- fonoteka.go/plugins/golem15/fonoteka/classes/album_write_service_fuzz_test.go
- fonoteka.go/plugins/golem15/fonoteka/classes/collection_write_service_fuzz_test.go
- fonoteka.go/plugins/golem15/fonoteka/classes/credential_write_service_fuzz_test.go
- fonoteka.go/plugins/golem15/fonoteka/classes/hidden_marshal_test.go
- fonoteka.go/plugins/golem15/fonoteka/updates/10_album_slice.go
- fonoteka.go/plugins/golem15/fonoteka/updates/11_secrets_slice.go
- fonoteka.go/plugins/golem15/fonoteka/updates/20_remaining.go
- fonoteka.go/plugins/golem15/user/plugin.go
- fonoteka.go/plugins/golem15/user/models/organisation.go
- fonoteka.go/plugins/golem15/user/models/user.go
- fonoteka.go/plugins/golem15/user/updates/10_organisations.go
- fonoteka.go/config/app.yaml
- fonoteka.go/config/storage.yaml
- fonoteka.go/parity/schema_diff_test.go
- fonoteka.go/parity/album_write_service_test.go
- fonoteka.go/parity/fixtureplugin/plugin.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: 3
warning: 7
info: 3
total: 13
critical: 1
warning: 5
info: 4
total: 10
status: issues_found
---
# Phase 5: Code Review Report
**Reviewed:** 2026-09-18T20:15:00Z
**Reviewed:** 2026-09-19T16:30:00Z
**Depth:** standard
**Files Reviewed:** 72
**Files Reviewed:** 34
**Status:** issues_found
## Summary
Phase 5's mass-assignment primitive (`lagoon.Fill`) and at-rest encryptor (`lagoon.Encrypted`) are the right shape: allow-list copy, redacted `MarshalJSON`/`String`/`GoString`, AES-256-GCM with HKDF, loud `app.key` failure, and `DecryptLaravelPayload` off the live Scan/Value path. `StaticHandler` rebuilds blob keys from `disk_name` and rejects `..`. Those boundaries hold.
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 shipped write path does not. `SaveAlbum` feeds `*int` (and JSON `float64`) into `lagoon.Validate`'s integer rule, so a year-bearing album cannot be name-updated and typical JSON bodies fail year checks. `Album.stampMarketPrice` has no dirty check, so a name-only save wipes `market_price_source` and bumps `market_price_checked_at`, contradicting PHP D-4/D-6. `models.User.Password` is neither `Hidden()` nor `json:"-"`; the 05-06 hidden-marshal walk does not populate it and would not catch a leak.
Fill/Encrypted footguns remain for Phase 6 handlers: write services hardcode `production=false`, `Fill` of `Encrypted` runs `Scan` (ciphertext, not plaintext), and credential `Fillable()` still lists owner FKs — the narrowing list lives only in a test helper.
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: Integer validation rejects `*int` and JSON numbers, so SaveAlbum year checks fail
### CR-01: `StaticHandler` cannot serve the URLs `File.Thumb` returns
**File:** `lagoon/validate.go:172-190`
**Also:** `fonoteka.go/plugins/golem15/fonoteka/classes/album_write_service.go:57-77`
**File:** `lagoon/attach/static.go:32-37`
**Also:** `lagoon/attach/thumb.go:117-126`, `lagoon/attach/static.go:72-91`
**Issue:** `isIntegerValue` only accepts untyped `nil`, non-pointer integer kinds, and digit strings. Pointers (`*int`, the type of `Album.Year`) fall through to `return false` unless the pointer itself is nil. `float64` (encoding/json into `map[string]any`) also fails.
**Issue:** Thumbnails are stored as `PartitionDirectory(originalDiskName) + ThumbFilename(...)`, and `Thumb` returns that key under `PublicPathPrefix()`:
`albumRuleValues` always puts `album.Year` (`*int`) into the validation map, then overlays **every** requested key except `market_price_stored`. That means:
```
/storage/uploads/abc/123/xyz/thumb_42_200_200_0_0_crop.jpg
```
1. Update an album that already has `year=1991` with `{"name": "Nevermind"}` (the path `TestAlbumArtistsOrderRoundTrip` uses, but only because those albums have a nil year) → values["year"] is `*int` → `"The year must be an integer."`
2. A JSON body `{"name":"x","year":1991}` → Fill converts `float64`→`*int` successfully via `ConvertibleTo`, then the overlay puts `float64(1991)` back → integer rule fails.
`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)`:
Existing tests pass integer literals in Go maps and never re-save a row whose year is set. FuzzSaveAlbum treats `saveErr != nil` as acceptable, so this never crashes a fuzz target.
- last segment = `thumb_42_200_200_0_0_crop.jpg`
- `PartitionDirectory` of that name = `thu/mb_/42_`
- request partition = `abc/123/xyz` → mismatch → 404
**Fix:** Dereference pointers and accept whole JSON numbers. Stop overlaying raw request values over already-converted model fields:
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 isIntegerValue(val any) bool {
if val == nil {
return true
func parsePublicBlobPath(p string) (key string, ok bool) {
if p == "" || strings.Contains(p, "\\") || strings.Contains(p, "..") || strings.Contains(p, "//") {
return "", false
}
rv := reflect.ValueOf(val)
for rv.Kind() == reflect.Ptr || rv.Kind() == reflect.Interface {
if rv.IsNil() {
return true
parts := strings.Split(p, "/")
if len(parts) != 4 {
return "", false
}
for _, part := range parts {
if part == "" || part == "." || part == ".." {
return "", false
}
rv = rv.Elem()
}
switch rv.Kind() {
case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
return true
case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64:
return true
case reflect.Float32, reflect.Float64:
f := rv.Float()
return f == float64(int64(f))
case reflect.String:
s := rv.String()
if s == "" {
return true
filename := parts[3]
got := strings.Join(parts[:3], "/")
if !strings.HasPrefix(filename, "thumb_") {
want := strings.TrimSuffix(PartitionDirectory(filename), "/")
if got != want {
return "", false
}
_, ok := new(big.Int).SetString(s, 10)
return ok
default:
return false
}
}
```
In `albumRuleValues`, do not range-overlay `requested` onto rule fields; validate the filled model (dereference `*int` / `*string`) the same way `market_price_stored` is already special-cased. Add a regression test: create with `year: 1991`, then `SaveAlbum(..., {"name": "x"}, nil)` must succeed and keep 1991.
### CR-02: `stampMarketPrice` has no dirty check — name-only saves wipe provenance and bump `checked_at`
**File:** `fonoteka.go/plugins/golem15/fonoteka/models/album.go:141-164`
**Also:** `fonoteka.go/plugins/golem15/fonoteka/classes/album_write_service.go:36`
**Issue:** PHP `Album::stampMarketPrice` returns immediately unless `market_price_stored` is dirty (D-6: name/currency-only edits must not move `market_price_checked_at`; D-4: source is cleared only when the amount changed and source was not also set on that save).
Go runs the full stamp on every `BeforeSave`:
```141:164:fonoteka.go/plugins/golem15/fonoteka/models/album.go
func (a *Album) stampMarketPrice() error {
raw := strings.TrimSpace(string(a.MarketPriceStored))
formatted, err := formatMoney4(raw)
if raw == "" || err != nil {
a.MarketPriceStored = ""
a.MarketPriceCurrency = nil
a.MarketPriceSource = nil
a.MarketPriceCheckedAt = nil
return nil
}
a.MarketPriceStored = MoneyString(formatted)
// ...
if !a.KeepMarketPriceSource {
a.MarketPriceSource = nil
}
now := time.Now()
a.MarketPriceCheckedAt = &now
return nil
}
```
`KeepMarketPriceSource` only approximates `isDirty('market_price_source')` from request-key presence. There is no equivalent for `market_price_stored`. Consequences on a name-only `SaveAlbum` of a priced album:
- `market_price_source` is cleared (request usually omits the key; AlbumFillFields cannot set it anyway).
- `market_price_checked_at` is rewritten to now, so the SPA's "when was this amount established" is wrong.
`FuzzSaveAlbum` always injects `market_price_source` into `requested`, which forces `KeepMarketPriceSource=true` and hides the wipe. PHP AlbumWriteService never puts source in FILL_FIELDS; a price-changing hand edit is supposed to clear source, and a name-only edit is supposed to leave source and `checked_at` alone.
**Fix:** Track whether this save actually changed the stored amount (and whether source was assigned on the model, not merely present in the HTTP map). One approach:
```go
// on Album, gorm:"-"
PriceTouched bool
SourceTouched bool
// SaveAlbum:
_, album.SourceTouched = requested["market_price_source"]
_, album.PriceTouched = requested["market_price_stored"]
func (a *Album) stampMarketPrice() error {
if !a.PriceTouched {
if a.CurrencyTouched { /* normalize currency only; do not touch checked_at */ }
return nil
}
// existing blank/format/source/checked_at logic, using SourceTouched
}
```
Add a test: seed price `"12.0000"` + source `"suggestion"` + a frozen `checked_at`, then `SaveAlbum` with only `name`. Assert source, currency, stored amount, and `checked_at` are unchanged.
### CR-03: `User.Password` is neither `Hidden()` nor `json:"-"`
**File:** `fonoteka.go/plugins/golem15/user/models/user.go:3-9`
**Issue:** PHP `User::$hidden` includes `password`. The Go stub used by the JWT guard is:
```go
type User struct {
ID uint `gorm:"column:id;primaryKey"`
Email string `gorm:"column:email"`
Password string `gorm:"column:password"`
MustChangePassword bool `gorm:"column:must_change_password"`
}
```
`json.Marshal` of a loaded user emits the password hash under `"Password"`. `TestHiddenNeverMarshals` walks `user/models.All()` but only plants the sentinel on columns listed by `Hidden()`; User implements neither `HasHidden` nor `json:"-"`, so the 05-06 backstop that claimed to close T-05-03/`T-05-21` does not cover the most sensitive column in the registry. Widen-users is Phase 7; hiding `password` is not.
**Fix:**
```go
type User struct {
ID uint `gorm:"column:id;primaryKey"`
Email string `gorm:"column:email"`
Password string `gorm:"column:password" json:"-"`
MustChangePassword bool `gorm:"column:must_change_password"`
return got + "/" + filename, true
}
func (User) Hidden() []string { return []string{"password"} }
// StaticHandler: key, ok := parsePublicBlobPath(rel); reader, err := bucket.NewReader(r.Context(), key, nil)
```
Extend `TestHiddenNeverMarshals` so a registered model with a `password` / `token_hash` / `*_hash` gorm column must be in `Hidden()` even if the type does not yet implement other secrets.
Add a test that writes a thumb at `part+thumbName`, calls `StaticHandler`, and GETs the URL `Thumb` returns.
## Warnings
### WR-01: Write services hardcode `production=false`, so dropped-key logs fire in production
### WR-01: `Thumb` has no bounds on dimensions or decoded image size
**File:** `fonoteka.go/plugins/golem15/fonoteka/classes/album_write_service.go:37`
**Also:** `fonoteka.go/plugins/golem15/fonoteka/classes/collection_write_service.go:25`
**File:** `lagoon/attach/thumb.go:99-140`
**Issue:** `lagoon.Fill`'s fourth argument is `production bool`. `true` is silent (D-06, `TestFillProductionSilent`); `false` logs each type+key once. Both write services pass `false` unconditionally. A production `SaveAlbum`/`SaveCollection` will `slog.Warn` every probed mass-assignment key (`collection_id`, `owner_id`, `public_token`, …) into application logs. Fuzz tests correctly pass `true`.
**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.
**Fix:** Thread the boot env (e.g. `app.Config` `debug` / `SUMMER_ENV=production`) into the service, or default to silent (`true`) and only log when debug is on:
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
if err := lagoon.Fill(album, AlbumFillFields, requested, production); err != nil {
return err
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: `Fill` of `Encrypted` columns `Scan`s ciphertext, not plaintext
### WR-02: A failed thumb encode still commits the blob, which then caches forever
**File:** `lagoon/fill.go:95-134`
**Also:** `lagoon/encrypted.go:62-92`, `fonoteka.go/plugins/golem15/fonoteka/classes/credential_write_service_fuzz_test.go:142-147`
**File:** `lagoon/attach/thumb.go:148-156`
**Issue:** Credential `Fillable()` lists `api_key` / `token`. `setField` cannot convert `string` → `Encrypted`, so it falls through to `sql.Scanner.Scan`. `Encrypted.Scan` decrypts. The fuzz helper documents this and **deletes** those keys before Fill; that helper is not production code. A Phase 6 handler that does `Fill(cred, cred.Fillable(), req)` will (a) fail on plaintext secrets, (b) succeed if `req` contains another row's ciphertext, copying the secret.
**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:** Teach `Fill` to construct plaintext Encrypted values, and drop encrypted/hash columns from `Fillable()` (or add a credential write-service allow-list that excludes them, matching `AlbumFillFields`):
**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.
```go
if destType == reflect.TypeOf(Encrypted{}) {
s, ok := val.(string)
if !ok {
return reflect.Value{}, fmt.Errorf("encrypted value must be a string")
}
return reflect.ValueOf(NewEncrypted(s)), nil
}
```
### WR-03: Laravel `between`/`min`/`max` are translated as go-playground length tags; `nullable` plus `omitempty` lets numeric `0` skip the range
Do not `Scan` request input as ciphertext on a write path.
**File:** `lagoon/validate.go:54-72` and `lagoon/validate.go:109-117`
### WR-03: `CollectionInvitation.token_hash` is not hidden
**Issue:** Two Laravel-parity holes remain after the `*int`/float64 integer fix:
**File:** `fonoteka.go/plugins/golem15/fonoteka/models/collection_invitation.go:10-28`
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`.
**Issue:** `token_hash` is fillable, has no `json:"-"`, and the model implements no `Hidden()`. `json.Marshal` of an invitation leaks the SHA-256 of the raw invite token. PHP also omits `$hidden` and relies on the Invitation API serializer (`InvitationApiTest` asserts `token_hash` is absent). Go's D-08 contract is the accidental-marshal backstop, and every other hash column (`ApiToken.token_hash`, `OAuthClient.client_secret_hash`, `OAuthAuthCode.code_hash`, `OAuthRefreshToken.token_hash`) already uses `json:"-"` + `Hidden()`.
`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
TokenHash string `gorm:"column:token_hash" json:"-"`
func (CollectionInvitation) Hidden() []string { return []string{"token_hash"} }
IsPublic bool `gorm:"column:is_public;default:true"`
```
Remove `token_hash` from `Fillable()`; InvitationService should set it explicitly, as PHP already does with `hash('sha256', $rawToken)`.
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-04: `Thumb` interpolates unsanitized `mode`/`ext` into fileblob keys
### WR-05: `StaticHandler` serves every matching blob and never consults `is_public`
**File:** `lagoon/attach/thumb.go:18-20`
**Also:** `lagoon/attach/thumb.go:92-98`
**File:** `lagoon/attach/static.go:16-53`
**Issue:** `ThumbFilename` formats `mode` and `ext` directly into the blob key. `File.Thumb` defaults empty mode to `"auto"` but does not restrict the alphabet. With `fileblob`, a key like `abc/123/xyz/thumb_42_200_200_0_0_../../secret.jpg` is a filesystem path. StaticHandler is not mounted yet (T-05-16), but `Thumb` is a public API that Winter thumb URLs will call with a mode string.
**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:** Reject anything other than `[a-z0-9]+` for mode and ext before building the key:
```go
func ThumbFilename(id uint, w, h int, offsetX, offsetY int, mode, ext string) string {
if !thumbToken.MatchString(mode) || !thumbToken.MatchString(ext) {
mode, ext = "auto", "jpg"
}
return fmt.Sprintf("thumb_%d_%d_%d_%d_%d_%s.%s", id, w, h, offsetX, offsetY, mode, ext)
}
```
Prefer returning an error over silently coercing if the caller is HTTP-facing.
### WR-05: `DeleteKeys` prefix `thumb_<id>_` collides with later IDs in the same partition
**File:** `lagoon/attach/file.go:60-65`
**Also:** `lagoon/attach/file.go:105-126`
**Issue:** Force-delete lists `partition + "thumb_" + id + "_"`. Prefix `thumb_4_` also matches `thumb_40_…`, `thumb_41_…`, … whenever two originals share the first 9 characters of `disk_name` (same partition). Random hashes make that rare, but the match is by string prefix, not `thumb_<id>_` as a delimited token (`thumb_4_` vs `thumb_4/` is fine; `thumb_4_` vs `thumb_42_` is not).
**Fix:** List the partition and only delete keys that match `^thumb_<id>_` as a full filename prefix (next rune is a digit of the width/height field, but not another ID). Safer: list `thumb_<id>_` and require the remainder to match `ThumbFilename`'s pattern `^\d+_\d+_\d+_\d+_[a-z0-9]+\.[a-z0-9]+$`, or store exact thumb keys instead of a prefix.
### WR-06: `SaveCollection` validates the raw request, not the filled model
**File:** `fonoteka.go/plugins/golem15/fonoteka/classes/collection_write_service.go:25-34`
**Issue:** `SaveAlbum` builds rule values from the model (then overlays the request). `SaveCollection` passes `requested` straight into `Validate`. `Rules()` is `name: required`. An update that only sends `description` fails even when `collection.Name` is already set. `FuzzSaveCollection` always injects `name`, so this is untested.
**Fix:** Validate filled fields, same as album:
```go
values := map[string]any{"name": collection.Name, "description": collection.Description}
if err := lagoon.Fill(collection, CollectionFillFields, requested, production); err != nil {
return err
}
values["name"] = collection.Name
values["description"] = collection.Description
```
### WR-07: Credential `Fillable()` includes owner FKs; the only narrowing list is a test helper
**File:** `fonoteka.go/plugins/golem15/fonoteka/models/user_ai_credential.go:23-24`
**Also:** `org_ai_credential.go:23-24`, `user_discogs_credential.go:20-21`, `org_discogs_credential.go:20-21`, `classes/credential_write_service_fuzz_test.go:160-170`
**Issue:** D-05 is two-layer: model `Fillable()` is the backstop, write services pass a narrower list (`AlbumFillFields` drops `collection_id`). Credential models have no write service. `Fillable()` includes `user_id` / `organisation_id` and the Encrypted columns. `credentialWriteFields` in the fuzz test is the only place those keys are stripped. A Phase 6 handler that copies `Fillable()` as the working allow-list is an owner-takeover.
PHP `$fillable` is the same set; PHP also has a dedicated credentials writer. Go currently has only the backstop.
**Fix:** Add `CredentialFillFields` (provider/model/base_url only; secrets via `NewEncrypted`; owner FK set by the service from the auth context) in `classes/`, and point the fuzz helper at that production list so the narrowing cannot drift.
**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: `boolean` validation is a no-op
### IN-01: `HasFillable` / `HasHidden` are never called
**File:** `lagoon/validate.go:88-90`
**File:** `lagoon/fill.go:15-23`
**Issue:** `Settings.Rules()` is `search_use_typesense: boolean`, but the token does not type-check. `"yes"`, `2`, and maps all pass. Fill of a `bool` field from a non-bool still errors, so the hole is only if Validate is run on the raw request map (as `SaveCollection` does today).
**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:** Accept only `bool`, `nil`, and the strings/ints Laravel treats as boolean (`"1"`/`"0"`/`"true"`/`"false"`), and reject everything else.
**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: Ciphertext key-id is written and never read
### IN-02: `TestNoAutoMigrate` does not scan `attach/`
**File:** `lagoon/encrypted.go:146-154`
**Also:** `lagoon/encrypted.go:274, 292-300`
**File:** `lagoon/migrations_test.go:79-96`
**Issue:** `primaryID` is `len(previous)+1` capped at `0x0F` (IDs collide after 15 previous keys). `decryptCiphertext` ignores the nibble and tries primary + all previous keys. Harmless for GCM, but the version byte is documentation-only and the cap is a silent footgun for long rotation histories.
**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:** Select the candidate by `raw[0] & 0x0F` first, then fall back; refuse to publish more than 15 previous keys instead of clamping.
**Fix:** Walk recursively (or `filepath.Glob("../*.go")` plus `attach/*.go` from the lagoon directory), still skipping `_test.go`.
### IN-03: `albumBeforeSaveCallback` is a no-op registration seam
### IN-03: `migrate:status` omits the framework `system_files` set
**File:** `fonoteka.go/plugins/golem15/fonoteka/classes/artist_resolver.go:34-43`
**File:** `lagoon/migrations.go:148-176`
**Also:** `lagoon/commands.go:52-72`
**Issue:** The GORM create/update callback is registered on every Boot and does nothing. Dead callbacks add cost and imply artist resolution happens here (it does not; `ResolveArtists` + `syncArtists` do). Fine as a seam, but easy to mistake for a security boundary.
**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:** Drop the empty callback until it has a body, or document it as intentionally vacant in one comment next to `RegisterHook`.
**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-18T20:15:00Z_
_Reviewed: 2026-09-19T16:30:00Z_
_Reviewer: Claude (gsd-code-reviewer)_
_Depth: standard_