diff --git a/.planning/phases/05-data-layer-full-fidelity/05-REVIEW.md b/.planning/phases/05-data-layer-full-fidelity/05-REVIEW.md new file mode 100644 index 0000000..ee5e12c --- /dev/null +++ b/.planning/phases/05-data-layer-full-fidelity/05-REVIEW.md @@ -0,0 +1,391 @@ +--- +phase: 05-data-layer-full-fidelity +reviewed: 2026-09-18T20:15:00Z +depth: standard +files_reviewed: 72 +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/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 +findings: + critical: 3 + warning: 7 + info: 3 + total: 13 +status: issues_found +--- + +# Phase 5: Code Review Report + +**Reviewed:** 2026-09-18T20:15:00Z +**Depth:** standard +**Files Reviewed:** 72 +**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. + +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. + +## Narrative Findings (AI reviewer) + +## Critical Issues + +### CR-01: Integer validation rejects `*int` and JSON numbers, so SaveAlbum year checks fail + +**File:** `lagoon/validate.go:172-190` +**Also:** `fonoteka.go/plugins/golem15/fonoteka/classes/album_write_service.go:57-77` + +**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. + +`albumRuleValues` always puts `album.Year` (`*int`) into the validation map, then overlays **every** requested key except `market_price_stored`. That means: + +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. + +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. + +**Fix:** Dereference pointers and accept whole JSON numbers. Stop overlaying raw request values over already-converted model fields: + +```go +func isIntegerValue(val any) bool { + if val == nil { + return true + } + rv := reflect.ValueOf(val) + for rv.Kind() == reflect.Ptr || rv.Kind() == reflect.Interface { + if rv.IsNil() { + return true + } + 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 + } + _, 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"` +} + +func (User) Hidden() []string { return []string{"password"} } +``` + +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. + +## Warnings + +### WR-01: Write services hardcode `production=false`, so dropped-key logs fire in production + +**File:** `fonoteka.go/plugins/golem15/fonoteka/classes/album_write_service.go:37` +**Also:** `fonoteka.go/plugins/golem15/fonoteka/classes/collection_write_service.go:25` + +**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`. + +**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: + +```go +if err := lagoon.Fill(album, AlbumFillFields, requested, production); err != nil { + return err +} +``` + +### WR-02: `Fill` of `Encrypted` columns `Scan`s ciphertext, not plaintext + +**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` + +**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. + +**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`): + +```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 +} +``` + +Do not `Scan` request input as ciphertext on a write path. + +### WR-03: `CollectionInvitation.token_hash` is not hidden + +**File:** `fonoteka.go/plugins/golem15/fonoteka/models/collection_invitation.go:10-28` + +**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()`. + +**Fix:** + +```go +TokenHash string `gorm:"column:token_hash" json:"-"` + +func (CollectionInvitation) Hidden() []string { return []string{"token_hash"} } +``` + +Remove `token_hash` from `Fillable()`; InvitationService should set it explicitly, as PHP already does with `hash('sha256', $rawToken)`. + +### WR-04: `Thumb` interpolates unsanitized `mode`/`ext` into fileblob keys + +**File:** `lagoon/attach/thumb.go:18-20` +**Also:** `lagoon/attach/thumb.go:92-98` + +**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. + +**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__` 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__` 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__` as a full filename prefix (next rune is a digit of the width/height field, but not another ID). Safer: list `thumb__` 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. + +## Info + +### IN-01: `boolean` validation is a no-op + +**File:** `lagoon/validate.go:88-90` + +**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). + +**Fix:** Accept only `bool`, `nil`, and the strings/ints Laravel treats as boolean (`"1"`/`"0"`/`"true"`/`"false"`), and reject everything else. + +### IN-02: Ciphertext key-id is written and never read + +**File:** `lagoon/encrypted.go:146-154` +**Also:** `lagoon/encrypted.go:274, 292-300` + +**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. + +**Fix:** Select the candidate by `raw[0] & 0x0F` first, then fall back; refuse to publish more than 15 previous keys instead of clamping. + +### IN-03: `albumBeforeSaveCallback` is a no-op registration seam + +**File:** `fonoteka.go/plugins/golem15/fonoteka/classes/artist_resolver.go:34-43` + +**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. + +**Fix:** Drop the empty callback until it has a body, or document it as intentionally vacant in one comment next to `RegisterHook`. + +--- + +_Reviewed: 2026-09-18T20:15:00Z_ +_Reviewer: Claude (gsd-code-reviewer)_ +_Depth: standard_