diff --git a/.planning/phases/05-data-layer-full-fidelity/05-REVIEW-FIX.md b/.planning/phases/05-data-layer-full-fidelity/05-REVIEW-FIX.md index 32b973e..a80ed9e 100644 --- a/.planning/phases/05-data-layer-full-fidelity/05-REVIEW-FIX.md +++ b/.planning/phases/05-data-layer-full-fidelity/05-REVIEW-FIX.md @@ -1,110 +1,68 @@ --- phase: 05-data-layer-full-fidelity -fixed_at: 2026-09-18T22:05:00Z +fixed_at: 2026-09-19T13:37:26Z review_path: .planning/phases/05-data-layer-full-fidelity/05-REVIEW.md iteration: 1 -findings_in_scope: 10 -fixed: 9 -skipped: 1 -status: partial +findings_in_scope: 6 +fixed: 6 +skipped: 0 +status: all_fixed --- # Phase 5: Code Review Fix Report -**Fixed at:** 2026-09-18T22:05:00Z +**Fixed at:** 2026-09-19T13:37:26Z **Source review:** .planning/phases/05-data-layer-full-fidelity/05-REVIEW.md **Iteration:** 1 **Summary:** -- Findings in scope: 10 (3 critical, 7 warning; 3 info findings out of scope) -- Fixed: 9 -- Skipped: 1 (WR-05, reviewer premise disproved by test) - -Commits land in two repositories. `summercms.go` is the framework repo; `fonoteka.go` is the sibling application repo. Commit hashes below are prefixed with the repo they belong to. - -Verification at the final commit of each repo, with Docker/testcontainers available (full suite, not `-short`): -- `summercms.go`: `go vet ./...` clean, `go test ./...` all packages ok. -- `fonoteka.go`: `go vet` and `go test` over `./...`, `./plugins/golem15/user/...`, `./plugins/golem15/fonoteka/...` all ok, including `parity`. - -The fonoteka tests were run against the framework worktree carrying the framework fixes (the `../summercms.go` replace), so `fonoteka.go` master depends on `summercms.go` master containing `3943ea3` and `6d2262d`. +- Findings in scope: 6 (1 critical, 5 warning; Info findings out of scope) +- Fixed: 6 +- Skipped: 0 ## Fixed 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 -**Files modified:** `lagoon/validate.go`, `lagoon/validate_test.go` (summercms.go); `plugins/golem15/fonoteka/classes/album_write_service.go`, `plugins/golem15/fonoteka/classes/album_write_service_db_test.go` (fonoteka.go) -**Commit:** summercms.go `3943ea3`, fonoteka.go `1c4a09d` -**Status:** fixed: requires human verification (logic change) -**Applied fix:** `isIntegerValue` now dereferences pointers/interfaces and accepts whole `float32`/`float64` values (rejecting fractions and infinities). `albumRuleValues` no longer overlays raw request values on top of the filled model; it validates the model fields only, as `market_price_stored` already did. New tests: `TestValidateIntegerPointerAndJSONNumber` (framework), `TestSaveAlbumYearSurvivesNameOnlyUpdate` and `TestSaveAlbumJSONBodyYear` (create with year 1991, name-only update keeps 1991; a `json.Unmarshal`ed body with `year` passes; year 1700 still yields a `year` field error). -**Note:** digit strings under `integer|between:` still hit validator's string-length semantics for `min`/`max`. That is pre-existing, not part of this finding, and unreachable through `SaveAlbum` now that the filled `*int` is validated. +**Files modified:** `lagoon/attach/static.go`, `lagoon/attach/static_test.go` +**Commit:** 44126cc +**Applied fix:** `parsePublicBlobPath` now returns the validated 4-segment path as the blob key. The partition-matches-filename check is skipped for `thumb_*` names, which Winter stores beside the original. `StaticHandler` opens that key directly instead of rebuilding via `BlobKey(last-segment)`. `TestStaticHandlerServesThumbURL` generates a thumb, then GETs the URL `Thumb` returns through the handler. -### CR-02: `stampMarketPrice` has no dirty check +### WR-01: `Thumb` has no bounds on dimensions or decoded image size -**Files modified:** `plugins/golem15/fonoteka/models/album.go`, `plugins/golem15/fonoteka/classes/album_write_service.go` (comment), `plugins/golem15/fonoteka/classes/album_write_service_db_test.go` (fonoteka.go) -**Commit:** fonoteka.go `f092170` -**Status:** fixed: requires human verification (logic change, PHP parity) -**Applied fix:** Instead of request-key flags (which only cover `SaveAlbum`), `Album` now keeps an unexported snapshot of `market_price_stored` / `market_price_currency` / `market_price_source`, taken in `AfterFind` and re-based in `AfterSave` (the Go stand-in for Eloquent `getRawOriginal`/`syncOriginal`). `stampMarketPrice` was rewritten line by line from the PHP original: -- price not dirty: return; if only currency is dirty, run the new `normalizeCurrencyOnlyChange` (normalise, or null currency+source when no amount is stored) without touching `checked_at` (D-6); -- price dirty and blank/non-numeric: null all four columns; -- price dirty: clear source unless this save set it (`KeepMarketPriceSource` or a changed value, D-4), normalise currency, and refresh `checked_at` only when the 4-decimal amount differs from the original (re-sending `12` against `12.0000` is not a fresh check). +**Files modified:** `lagoon/attach/thumb.go`, `lagoon/attach/thumb_test.go` +**Commit:** c211822 +**Applied fix:** Reject `w`/`h` that are non-positive or larger than 4096 before any bucket I/O. Cap the decoder with `io.LimitReader` at 32MiB and reject decoded images above 4096×4096 pixels. `TestFileThumbRejectsOutOfRangeSize` asserts rejected sizes never touch the bucket. -The unexported field is invisible to GORM and `encoding/json`, so no response shape changes. Tests added: name-only save keeps price/currency/source/frozen `checked_at`; currency-only edit keeps `checked_at`; changed amount clears source and moves `checked_at`; same amount in another spelling keeps `checked_at`; a struct reused across two saves without reload starts the second save clean. -**Known limits to confirm:** an `Album` built by hand with an ID and saved without ever being loaded has an empty snapshot, so a non-blank price counts as dirty (previous behaviour). If a transaction rolls back after `AfterSave`, the in-memory snapshot is ahead of the database until the struct is reloaded. +### WR-02: A failed thumb encode still commits the blob, which then caches forever -### CR-03: `User.Password` is neither `Hidden()` nor `json:"-"` +**Files modified:** `lagoon/attach/thumb.go`, `lagoon/attach/thumb_test.go` +**Commit:** e54f9d7 +**Applied fix:** After `encodeImage` / `Writer.Close`, a failure deletes the thumb key (NotFound ignored) so the next `Thumb` regenerates instead of serving a partial object. `TestFileThumbEncodeFailureDoesNotCache` injects an encode error, asserts the blob is gone, then retries successfully. -**Files modified:** `plugins/golem15/user/models/user.go`, `plugins/golem15/fonoteka/classes/hidden_marshal_test.go` (fonoteka.go) -**Commit:** fonoteka.go `2a94e4d` -**Applied fix:** `Password` is tagged `json:"-"` and `User` implements `Hidden() []string{"password"}`. This also closes a leak through `Collection.Editors []User`. `TestHiddenNeverMarshals` now runs a name-based backstop on every registered model: any gorm column named `password`, `token_hash` or `*_hash` must be listed in `Hidden()`. Added `TestSecretColumnNames` and `TestUserPasswordNeverMarshals` (direct marshal and marshal via `Collection.Editors`). +### WR-03: Laravel `between`/`min`/`max` are translated as length tags; `nullable` plus `omitempty` lets numeric `0` skip the range -### WR-01: Write services hardcode `production=false` +**Files modified:** `lagoon/validate.go`, `lagoon/validate_test.go` +**Commit:** fb12b22 +**Status:** fixed: requires human verification +**Applied fix:** `nullable` no longer emits go-playground `omitempty`; empty is gated only by `isEmptyValue` (nil / empty string, not numeric 0). `integer`/`numeric` `between`/`min`/`max` compare with `big.Rat` via `numericString` (dereferences `*int` and accepts digit strings) instead of go-playground length `min`/`max`. String-length `between`/`min`/`max` are unchanged. Tests: digit string `"1991"` passes; `0` / `float64(0)` / `"0"` fail `integer|between:1889,2100`; numeric `0` still passes `min:0`. -**Files modified:** `plugins/golem15/fonoteka/classes/album_write_service.go`, `plugins/golem15/fonoteka/classes/collection_write_service.go`, `plugins/golem15/fonoteka/plugin.go`, `plugins/golem15/fonoteka/classes/album_write_service_test.go` (fonoteka.go) -**Commit:** fonoteka.go `68fe1a4` -**Applied fix:** The write services default to silent. A package-level `atomic.Bool` behind `classes.SetDebug` gates dropped-key logging; `Plugin.Boot` sets it from `app.Config.Bool("app.debug")` (nil-safe). `SaveAlbum`/`SaveCollection` signatures are unchanged. Test `TestWriteServicesSilentUnlessDebug`. +### WR-04: `File.IsPublic` zero value writes `false`, opposite Winter and the SQL default -### WR-02: `Fill` of `Encrypted` columns `Scan`s ciphertext, not plaintext +**Files modified:** `lagoon/attach/file.go`, `lagoon/attach/file_test.go`, `lagoon/attach/lifecycle_test.go` +**Commit:** c12e657 +**Status:** fixed: requires human verification +**Applied fix:** `IsPublic` is `*bool` with `gorm:"column:is_public;not null;default:true"` plus `File.Public()` (nil → true). A non-pointer `bool` with `default:true` cannot persist false because GORM replaces the zero value with the default. `Create` without the field stores true; explicit `&false` stores false (`TestFileCreateDefaultsIsPublic`). -**Files modified:** `lagoon/fill.go`, `lagoon/fill_test.go` (summercms.go) -**Commit:** summercms.go `6d2262d` -**Applied fix:** `convertValue` handles `Encrypted` destinations (value and pointer fields) by wrapping a request string with `NewEncrypted`; non-strings are rejected with `encrypted value must be a string`, and `setField` no longer falls through to `Encrypted.Scan` for that type. `nil` still clears. `TestFillEncryptedTakesPlaintext` proves plaintext round-trips, another row's ciphertext is stored as literal text rather than decrypted, non-string input is rejected, and nil clears. The `Fillable()` half of the reviewer's suggestion is handled under WR-07 (service allow-list), leaving model `Fillable()` as the PHP `$fillable` mirror. +### WR-05: `StaticHandler` serves every matching blob and never consults `is_public` -### WR-03: `CollectionInvitation.token_hash` is not hidden - -**Files modified:** `plugins/golem15/fonoteka/models/collection_invitation.go` (fonoteka.go) -**Commit:** fonoteka.go `7dc65a0` -**Applied fix:** `TokenHash` tagged `json:"-"` and `Hidden()` returns `token_hash`. Committed before CR-03 so the widened hidden-marshal test is green at every commit. -**Deliberately not applied:** `token_hash` was left in `Fillable()`. PHP `$fillable` lists it, and the sibling hash models (`ApiToken`, `OAuthRefreshToken`) keep `token_hash` in their `Fillable()` as line-by-line `$fillable` ports. Removing it from this one model would break that convention and could break a straight port of `InvitationService::create([...'token_hash' => ...])` in Phase 6. The service-level allow-list is the right place to exclude it when that service is written. - -### WR-04: `Thumb` interpolates unsanitized `mode`/`ext` into fileblob keys - -**Files modified:** `lagoon/attach/thumb.go`, `lagoon/attach/thumb_test.go` (summercms.go) -**Commit:** summercms.go `ced87a5` -**Applied fix:** Added `thumbToken` (`^[a-z0-9]+$`). `File.Thumb` lowercases the mode (Winter does the same, and `defaultResizeImage` already compared case-insensitively) and returns an error for a mode or extension outside the alphabet, before any bucket access. `ThumbFilename` keeps its signature and coerces an unsafe mode to `auto` and an unsafe ext to `jpg`, so it can never emit a separator or `..`. Tests `TestThumbFilenameRejectsUnsafeTokens` and `TestFileThumbRejectsTraversalMode` (also asserts nothing is written to the bucket). - -### WR-06: `SaveCollection` validates the raw request, not the filled model - -**Files modified:** `plugins/golem15/fonoteka/classes/collection_write_service.go`, `plugins/golem15/fonoteka/classes/collection_write_service_db_test.go` (fonoteka.go) -**Commit:** fonoteka.go `a02b4e0` -**Status:** fixed: requires human verification (logic change) -**Applied fix:** Validation values are built from the filled collection (`name`, `description`). Tests: a description-only update of a named collection succeeds and keeps the name; a blank name and a create without a name still return a `name` field error. - -### WR-07: Credential `Fillable()` includes owner FKs; the only narrowing list is a test helper - -**Files modified:** `plugins/golem15/fonoteka/classes/credential_write_service.go` (new), `plugins/golem15/fonoteka/classes/credential_write_service_test.go` (new), `plugins/golem15/fonoteka/classes/credential_write_service_fuzz_test.go` (fonoteka.go) -**Commit:** fonoteka.go `df84c21` -**Applied fix:** New production allow-list `classes.CredentialFillFields = {provider, model, base_url}` with a doc comment stating that the owner FK comes from the auth context and secrets are set with `lagoon.NewEncrypted`. The fuzz helper now fills through that list; the test-only `credentialWriteFields` and the pre-Fill key deletion are gone, so the narrowing the four fuzz targets prove is the one handlers will use. `TestCredentialFillFieldsNarrowerThanFillable` pins the exclusions. Model `Fillable()` is unchanged (PHP `$fillable` mirror, backstop layer). New files were required by the finding. A full credential write service (owner from auth context) belongs with the Phase 6 handlers and was not invented here. - -## Skipped Issues - -### WR-05: `DeleteKeys` prefix `thumb__` collides with later IDs in the same partition - -**File:** `lagoon/attach/file.go:60-65` -**Reason:** skipped: false positive, no code change made. The list prefix is `thumb__` including the trailing underscore. `thumb_4_` is not a string prefix of `thumb_40_…` or `thumb_42_…` (the character after `thumb_4` is a digit there, not `_`), so the collision described cannot occur; the finding's own parenthetical contradicts itself. This was verified rather than argued: `TestDeleteKeysThumbPrefixIsIDDelimited` (summercms.go `c848692`, a test-only commit, not counted as a fix) puts originals and thumbs for IDs 4, 14, 40, 41 and 42 in one partition, deletes file 4's keys, and asserts only file 4's original and thumbs are gone. It passes against the unchanged code. The suggested remainder regex (`^\d+_\d+_…`) was not adopted because it would add risk without benefit: any thumb name the regex fails to anticipate (for example a negative offset, which was not checked against Winter) would be orphaned on force-delete. -**Original issue:** Force-delete lists `partition + "thumb_" + id + "_"`; the reviewer believed prefix `thumb_4_` also matches `thumb_40_…`, `thumb_41_…` when two originals share a partition. +**Files modified:** `lagoon/attach/static.go`, `lagoon/attach/file.go`, `lagoon/attach/static_test.go`, `lagoon/attach/file_test.go`, `lagoon/attach/lifecycle_test.go` +**Commit:** 56156ae +**Status:** fixed: requires human verification +**Applied fix:** Documented `StaticHandler` as public-disk-only (must not hold protected files; do not mount ungated on the app origin). Added `StaticHandlerPublic`, which looks up `system_files` by `disk_name` (or file ID parsed from `thumb__…`) **before** `NewReader` and 404s on missing rows and `is_public=false`. Stub test proves the gate runs before the blob is opened; postgres test covers public/private originals and thumbs. --- -_Fixed: 2026-09-18T22:05:00Z_ +_Fixed: 2026-09-19T13:37:26Z_ _Fixer: Claude (gsd-code-fixer)_ _Iteration: 1_