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

docs(05): add code review fix report

Browse Source
This commit is contained in:
Jakub Zych
2026-09-18 23:53:21 +02:00
parent c84869270e
commit bbee45c5f4
1 changed files with 110 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -0,0 +1,110 @@
---
phase: 05-data-layer-full-fidelity
fixed_at: 2026-09-18T22:05:00Z
review_path: .planning/phases/05-data-layer-full-fidelity/05-REVIEW.md
iteration: 1
findings_in_scope: 10
fixed: 9
skipped: 1
status: partial
---
# Phase 5: Code Review Fix Report
**Fixed at:** 2026-09-18T22:05:00Z
**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`.
## Fixed Issues
### CR-01: Integer validation rejects `*int` and JSON numbers, so SaveAlbum year checks fail
**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.
### CR-02: `stampMarketPrice` has no dirty check
**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).
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.
### CR-03: `User.Password` is neither `Hidden()` nor `json:"-"`
**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-01: Write services hardcode `production=false`
**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-02: `Fill` of `Encrypted` columns `Scan`s ciphertext, not plaintext
**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-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_<id>_` 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_<id>_` 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.
---
_Fixed: 2026-09-18T22:05:00Z_
_Fixer: Claude (gsd-code-fixer)_
_Iteration: 1_