20 KiB
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-18T20:15:00Z | standard | 72 |
|
|
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:
- Update an album that already has
year=1991with{"name": "Nevermind"}(the pathTestAlbumArtistsOrderRoundTripuses, but only because those albums have a nil year) → values["year"] is*int→"The year must be an integer." - A JSON body
{"name":"x","year":1991}→ Fill convertsfloat64→*intsuccessfully viaConvertibleTo, then the overlay putsfloat64(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:
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:
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_sourceis cleared (request usually omits the key; AlbumFillFields cannot set it anyway).market_price_checked_atis 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:
// 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:
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:
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:
if err := lagoon.Fill(album, AlbumFillFields, requested, production); err != nil {
return err
}
WR-02: Fill of Encrypted columns Scans 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):
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:
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:
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:
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