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-18 21:11:11 +02:00
parent 38cd4c6f50
commit cd546dae1d
1 changed files with 391 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

391
.planning/phases/05-data-layer-full-fidelity/05-REVIEW.md Normal file
View File

@@ -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_<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.
## 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_