diff --git a/.planning/phases/13-p-ytarium-api-wishlist-notifications-csv-credentials-public/13-REVIEW.md b/.planning/phases/13-p-ytarium-api-wishlist-notifications-csv-credentials-public/13-REVIEW.md new file mode 100644 index 0000000..196f9e2 --- /dev/null +++ b/.planning/phases/13-p-ytarium-api-wishlist-notifications-csv-credentials-public/13-REVIEW.md @@ -0,0 +1,254 @@ +--- +phase: 13-p-ytarium-api-wishlist-notifications-csv-credentials-public +reviewed: 2026-10-03T00:00:00Z +depth: standard +files_reviewed: 67 +files_reviewed_list: + - summercms.go/modules/surf/overlap.go + - summercms.go/modules/surf/router.go + - summercms.go/modules/conga/conga.go + - summercms.go/modules/lagoon/validate_rules.go + - summercms.go/modules/tide/diff.go + - summercms.go/modules/tide/centrifugo_golden.go + - summercms.go/scripts/check-phase13.sh + - fonoteka.go/plugins/golem15/fonoteka/classes/ai_config_resolver.go + - fonoteka.go/plugins/golem15/fonoteka/classes/album_search.go + - fonoteka.go/plugins/golem15/fonoteka/classes/album_write_service.go + - fonoteka.go/plugins/golem15/fonoteka/classes/credential_write_service.go + - fonoteka.go/plugins/golem15/fonoteka/classes/csv_export.go + - fonoteka.go/plugins/golem15/fonoteka/classes/csv_import_service.go + - fonoteka.go/plugins/golem15/fonoteka/classes/gates.go + - fonoteka.go/plugins/golem15/fonoteka/classes/invitation_service.go + - fonoteka.go/plugins/golem15/fonoteka/classes/job_contract.go + - fonoteka.go/plugins/golem15/fonoteka/classes/notification_service.go + - fonoteka.go/plugins/golem15/fonoteka/classes/notifications.go + - fonoteka.go/plugins/golem15/fonoteka/classes/onboarding.go + - fonoteka.go/plugins/golem15/fonoteka/classes/php_values.go + - fonoteka.go/plugins/golem15/fonoteka/classes/public_share.go + - fonoteka.go/plugins/golem15/fonoteka/classes/reservations.go + - fonoteka.go/plugins/golem15/fonoteka/classes/serialize_album.go + - fonoteka.go/plugins/golem15/fonoteka/classes/serialize_public_album.go + - fonoteka.go/plugins/golem15/fonoteka/classes/share_service.go + - fonoteka.go/plugins/golem15/fonoteka/classes/similarity_finder.go + - fonoteka.go/plugins/golem15/fonoteka/classes/wishlist_notifications.go + - fonoteka.go/plugins/golem15/fonoteka/classes/wishlist_resolver.go + - fonoteka.go/plugins/golem15/fonoteka/classes/wishlist_subscriptions.go + - fonoteka.go/plugins/golem15/fonoteka/classes/csv/canonical_id.go + - fonoteka.go/plugins/golem15/fonoteka/classes/csv/contract.go + - fonoteka.go/plugins/golem15/fonoteka/classes/csv/detector.go + - fonoteka.go/plugins/golem15/fonoteka/classes/csv/errors.go + - fonoteka.go/plugins/golem15/fonoteka/classes/csv/fgetcsv.go + - fonoteka.go/plugins/golem15/fonoteka/classes/csv/mapper.go + - fonoteka.go/plugins/golem15/fonoteka/classes/csv/ordered.go + - fonoteka.go/plugins/golem15/fonoteka/classes/csv/parser.go + - fonoteka.go/plugins/golem15/fonoteka/classes/csv/php.go + - fonoteka.go/plugins/golem15/fonoteka/classes/csv/pipe_codec.go + - fonoteka.go/plugins/golem15/fonoteka/classes/csv/writer.go + - fonoteka.go/plugins/golem15/fonoteka/controllers/api/albums_controller.go + - fonoteka.go/plugins/golem15/fonoteka/controllers/api/credentials_controller.go + - fonoteka.go/plugins/golem15/fonoteka/controllers/api/csv_export_controller.go + - fonoteka.go/plugins/golem15/fonoteka/controllers/api/csv_import_controller.go + - fonoteka.go/plugins/golem15/fonoteka/controllers/api/invitation_inspect_controller.go + - fonoteka.go/plugins/golem15/fonoteka/controllers/api/notifications_controller.go + - fonoteka.go/plugins/golem15/fonoteka/controllers/api/onboarding_controller.go + - fonoteka.go/plugins/golem15/fonoteka/controllers/api/public_share_controller.go + - fonoteka.go/plugins/golem15/fonoteka/controllers/api/request.go + - fonoteka.go/plugins/golem15/fonoteka/controllers/api/wishlist_albums_controller.go + - fonoteka.go/plugins/golem15/fonoteka/controllers/api/wishlist_peer_controller.go + - fonoteka.go/plugins/golem15/fonoteka/controllers/api/wishlist_reservations_controller.go + - fonoteka.go/plugins/golem15/fonoteka/controllers/api/wishlist_share_settings_controller.go + - fonoteka.go/plugins/golem15/fonoteka/controllers/api/wishlist_subscriptions_controller.go + - fonoteka.go/plugins/golem15/fonoteka/jobs.go + - fonoteka.go/plugins/golem15/fonoteka/mail.go + - fonoteka.go/plugins/golem15/fonoteka/plugin.go + - fonoteka.go/plugins/golem15/fonoteka/realtime.go + - fonoteka.go/plugins/golem15/fonoteka/routes.go + - fonoteka.go/plugins/golem15/fonoteka/middleware/public_share_headers.go + - fonoteka.go/plugins/golem15/fonoteka/models/csv_import.go + - fonoteka.go/plugins/golem15/fonoteka/models/csv_import_row.go + - fonoteka.go/parity/check_corpus.go + - fonoteka.go/parity/php_parity.sh + - fonoteka.go/plugins/golem15/user/classes/events.go + - fonoteka.go/plugins/golem15/user/controllers/api_controller.go + - fonoteka.go/plugins/golem15/user/controllers/registration.go +findings: + critical: 0 + warning: 4 + info: 9 + total: 13 +status: issues_found +--- + +# Phase 13: Code Review Report + +**Reviewed:** 2026-10-03 +**Depth:** standard +**Files Reviewed:** 67 +**Status:** issues_found + +## Summary + +I reviewed the Phase 13 framework changes in summercms.go: surf overlap families, the conga unregistered-kind client, the lagoon `prohibited` rule, the tide masks and the gate script. I also reviewed the application changes in fonoteka.go (diff base `1cfe501`) and the sm-user-plugin registration refactor (diff base `c258e9f`). + +The security-critical paths hold up when traced: +- **Public token handling:** the shape check comes before any lookup, then a case-insensitive lookup followed by a constant-time exact compare. `public_enabled` and the kind are part of the query. The pubfail slot is reserved before resolution. +- **Public serializer:** the DTO is a positive allow-list, and the public text-field set is separate. Rating and price are not sorts on the public surface. +- **Credentials:** they are encrypted at rest and never selected into a response. The fill list holds no owner FK. +- **CSV import:** storage keys are generated on the server under a private bucket, and lookups are scoped by user and collection. +- **Notifications:** every query is scoped by `user_id`. +- **Onboarding:** an advisory lock is taken and the users are recounted under it. + +The sm-user-plugin change is additive: +- `RegisterEvent` gains a `Payload` field. +- `RegisterUser`, `FireRegisterEvent`, `IssueToken` and `APIArray` are new exports. +- `/register` behaviour is unchanged. The token is minted from the same `JWTSecret` value, and the payload drops `password` and `password_confirmation`. + +Go 1.27's `encoding/json` enforces a nesting depth limit, so `csv.DecodeOrdered`'s recursion cannot be driven into stack exhaustion. I verified this empirically. + +The defects found are concurrency gaps in multi-request state transitions, which matter more once Phase 14 starts the workers. The worst is that the CSV commit compare-and-swap does not cover the other writers. There is also a latent boot failure in the surf overlap families, and one residual risk that the security review under-states. I reproduced the surf finding with a `go test -overlay` probe; the source tree was not modified. + +## Warnings + +### WR-01: A surf overlap family's method-less pattern conflicts with any method-specific catch-all and fails boot + +**File:** `summercms.go/modules/surf/overlap.go:224-258` (fold loop), `:276-288` (the rejection), `:325-329` and `:501` (method-less pattern) +**Issue:** `generaliseFamily` registers each family under a pattern with no method, such as `/shelves/{surfOverlap1}/{surfOverlap2}`. ServeMux treats a method-less pattern and a pattern like `GET /{slug...}` as conflicting: the first has the more specific path and the second the more specific method, so neither is more specific overall. The fold loop at lines 237-247 then pulls the catch-all into the family, and `generaliseFamily` rejects it with "overlapping routes must use literal and single-segment {name} wildcards only". + +Without the family, the same table (the catch-all beside the original member routes) registers fine on ServeMux. The probe I ran confirms both behaviours: +``` +GET /{slug...} + GET /shelves/items/similar + GET /shelves/items/{id} (where id [0-9]+) + GET /shelves/{shelfId}/items (where shelfId [0-9]+) +-> surf: route conflict for GET /{slug...} ... overlapping routes must use literal and single-segment {name} wildcards only +``` +Any application that combines one routes.php overlap pair with a CMS-style catch-all fails to boot, and the error names the wrong cause. The pages plugin is a core plugin that will need such a catch-all. Today's fonoteka table has no catch-all under the family prefix, so nothing is broken yet. + +A second, wider method-specific route such as `POST /{a}/{b}/{c}` gets pulled into the family as well. It still dispatches correctly, but the family membership is broader than the conflict set. +**Fix:** Register one family pattern per method that the members use, for example `GET /shelves/{surfOverlap1}/{surfOverlap2}` and `DELETE /shelves/...`. Then compute 405 from the method-less view only inside `allowed()`, or register a method-less fallback only when no catch-all conflicts with it. Alternatively, limit the fold loop to routes of a method present in the family. Then add a regression test: +```go +table := append([]overlapRoute{{name: "catch", method: "GET", path: "/{slug...}"}}, overlapTable...) +if _, err := buildOverlapRouter(t, table).compile(); err != nil { t.Fatal(err) } +``` + +### WR-02: The CSV commit compare-and-swap is bypassed by mapping, row and cancel writes, which allows a second import job + +**File:** `fonoteka.go/plugins/golem15/fonoteka/classes/csv_import_service.go:519-587` (`UpdateCsvMapping`, status write at `:562`), `:639-705` (`UpdateCsvRow`, `save` at `:658-665`), `:889-901` (`CancelCsvImport`) +**Issue:** Only `CommitCsvImport` makes its state transition atomic (`UPDATE ... WHERE id = ? AND status = 'preview'`, line 845). The other writers check `csvBeforeCommit(imp.Status)` against the row that `visibleCsvImport` read before any lock, then write with `WHERE id = ?` only. This race leads to a double import: + +1. Request A (`PATCH mapping`) reads the import in status `preview`. +2. Request B (`POST commit`) wins the swap, sets the status to `importing`, dispatches import job X and stores `import_job_id = X`. +3. Request A's transaction deletes and recreates every row while job X is queued for them. It then writes `status = preview` (canonical) or `uploaded`, and leaves `import_job_id = X`. +4. Request C (`POST commit`) wins the swap again and dispatches import job Y. + +Two import jobs are now queued for one import, and job X is never cancelled. When Phase 14 registers the worker, both jobs write the rows and the albums are duplicated. + +`CancelCsvImport` has the matching gap: it cancels the job ids from its stale read. If a commit lands between that read and the write, the new import job stays queued for an import marked `canceled`. PHP has the same race (`updateMapping` checks `isBeforeCommit` and then calls `update()`). But the security review lists T-13-17 ("double commit / double writer") as mitigated with no known residual risk, and that only holds for commit against commit. +**Fix:** Do the check and the write under one lock. The response shapes do not change. +```go +err = lagoon.Transaction(ctx, db, func(ctx context.Context, tx *gorm.DB) error { + var cur models.CsvImport + if err := cleanSession(tx, ctx).Clauses(clause.Locking{Strength: "UPDATE"}). + Where("id = ?", imp.ID).Take(&cur).Error; err != nil { + return err + } + if !csvBeforeCommit(cur.Status) { + return ErrCsvAlreadyCommitted + } + // cancel cur.MatchJobID, replace rows, write status ... +}) +``` +For `CancelCsvImport`, lock the row in the same way, then cancel `cur.MatchJobID` and `cur.ImportJobID` from the locked row. Add a test that interleaves a commit with a mapping update and requires at most one import job. + +### WR-03: A double-submitted "Kupione" (purchase) sends every purchase notification and mail twice + +**File:** `fonoteka.go/plugins/golem15/fonoteka/controllers/api/wishlist_albums_controller.go:366-383`, `fonoteka.go/plugins/golem15/fonoteka/classes/album_write_service.go:395-414` +**Issue:** `findAlbum` loads the album from the caller's wishlist outside the transaction, without a lock. `MoveToCollection` then saves it with `Save(a)` and takes `from` from that stale snapshot. + +Two concurrent purchase requests, such as a double click, both pass `findAlbum`. The second one waits on the first one's row lock. After that it re-saves the album to the same target and finds no reservation to release. It still calls `NotifyWishlistItemPurchased(from = wishlist)`, so every subscriber gets a second `wishlist_item_purchased` bell row and a second purchase mail job, and a second realtime event is emitted. + +Separately, `saveAlbumRow` writes every column from the snapshot taken before the transaction. PHP's Eloquent save writes only the dirty `collection_id` and `updated_at`, so the Go version can overwrite a concurrent edit to other columns. +**Fix:** Inside the transaction, lock the album and re-check that it is still in the wishlist. If it moved, answer the same 404 JSON as `findAlbum`: +```go +var cur models.Album +if err := cleanSession(tx, ctx).Clauses(clause.Locking{Strength: "UPDATE"}). + Where("id = ? AND collection_id = ?", a.ID, from).Take(&cur).Error; err != nil { + return err // ErrRecordNotFound -> writeAlbumNotFound +} +``` +Alternatively, move the album with a conditional `UPDATE ... SET collection_id = ?, updated_at = NOW() WHERE id = ? AND collection_id = ?` and send notifications only when `RowsAffected == 1`. + +### WR-04: Token subscribers keep read and reserve access after the owner disables or regenerates the share, and the security review does not record this + +**File:** `fonoteka.go/plugins/golem15/fonoteka/classes/wishlist_resolver.go:171-186`, `fonoteka.go/plugins/golem15/fonoteka/controllers/api/wishlist_peer_controller.go:243-279`, `fonoteka.go/plugins/golem15/fonoteka/controllers/api/wishlist_reservations_controller.go:217-249` +**Issue:** `ReservableWishlistIDs` includes every wishlist the caller has a subscription row for, and checks only that the wishlist is live. The share state is not checked. A logged-in user who subscribed through `wishlist/token/{token}/subscribe` keeps all of this after the owner disables sharing or regenerates the token: +- the full `AlbumDTO` (notes, shelf, prices and so on) through `wishlist/{collectionId}/albums[/{albumId}]`; +- the ability to reserve albums; +- purchase and item-added notifications. + +The owner has no route to remove a subscriber. This matches PHP (`AlbumReservationService::reservableWishlistIds`, lines 33-55), so it is not a porting defect. But T-13-03 ("disabled or regenerated share") records "None known" as residual risk, and only the anonymous public routes are revoked. +**Fix:** At minimum, record the residual risk under T-13-03 and T-13-06 in `13-SECURITY-REVIEW.md`. If the product owner decides that revocation should apply, gate only the subscription branch on the share state. This changes PHP behaviour, so it needs a decision note: +```sql +OR c.id IN (SELECT s.collection_id FROM golem15_fonoteka_wishlist_subscriptions s + WHERE s.user_id = ?) AND c.public_enabled = true +``` +Alternatively, add an owner-side "remove subscriber" route in a later phase. + +## Info + +### IN-01: The conga unregistered-kind guard only covers a worker in the same process + +**File:** `summercms.go/modules/conga/conga.go:469-494` +**Issue:** `clientFor` checks `m.worker != nil`. In a web process with `queue.work_in_serve: false`, or in any process while a separate `queue:work` process serves the default queue, an unregistered kind dispatched without a queue is inserted without complaint, then fetched and discarded by the other process. The lock is also released between the check (lines 470-478) and the insert-only client lookup (lines 491-493), so a worker that starts in that gap is not seen. The Phase 13 contract always names unserved queues, so there is no live bug. +**Fix:** Require a non-empty queue that no configured worker serves for unregistered kinds regardless of `m.worker`, and compute `served` from settings and jobs, not from worker state. Alternatively, document the in-process scope in the conga README. + +### IN-02: A panic between `Begin` and `done` leaks a pubfail slot for good + +**File:** `fonoteka.go/plugins/golem15/fonoteka/controllers/api/public_share_controller.go:67-89` +**Issue:** `done` is called on every return path but not deferred. A panic in `publicDB` or `ResolvePublic` leaves `inflight` raised. `window()` resets `hits` but never `inflight`, so ten such panics lock that IP out until the process restarts. +**Fix:** Defer `done(failed)` with a `failed` flag that is set before each return. + +### IN-03: Huge `page` values overflow the OFFSET computation on the new paginated routes + +**File:** `fonoteka.go/plugins/golem15/fonoteka/controllers/api/wishlist_albums_controller.go:87-98`, `fonoteka.go/plugins/golem15/fonoteka/controllers/api/wishlist_peer_controller.go:310-322` +**Issue:** `phpInt` saturates at int64, so `(page - 1) * perPage` wraps. A negative result is dropped by GORM, which serves page 1 data while the meta reports the huge page. A positive result is an arbitrary offset. `ShowCsvImport` already guards against this (`page > math.MaxInt32`). +**Fix:** Clamp `page`, or return an empty page when `int64(page-1)*int64(perPage) >= total`. + +### IN-04: The `householdPeerSQL` comment gives the wrong bind count + +**File:** `fonoteka.go/plugins/golem15/fonoteka/classes/wishlist_resolver.go:103-106` +**Issue:** The comment says the query "binds the user id three times". It has four placeholders, and every caller correctly passes four (five in `ReservableWishlistIDs`). A future caller who follows the comment would get a bind error. +**Fix:** Change the comment to "four times". + +### IN-05: `ResolveAIConfig` can return `(nil, nil)`, and `AIConfig` has no log redaction + +**File:** `fonoteka.go/plugins/golem15/fonoteka/classes/ai_config_resolver.go:82-84`, `:32-37` +**Issue:** The site-admin path returns `AdminVisionModel(ctx)`, which is `(nil, nil)` until Phase 14. A Phase 14 caller that checks only `err` will dereference nil. `AIConfig` carries the decrypted key and uses `json:"-"` tags, but has no `String`/`LogValue` method, so `%v` or slog would print the key. +**Fix:** Return `ErrNoAICredential` when the admin model is nil, and add `func (AIConfig) LogValue() slog.Value` and a `String()` that redact `APIKey`. + +### IN-06: Uploaded CSV files are never removed + +**File:** `fonoteka.go/plugins/golem15/fonoteka/classes/csv_import_service.go:144-175`, `:889-901` +**Issue:** The blob is written before the database transaction, so a failed insert leaves an orphaned file. Cancel and completion never delete the file. Personal collection data stays in `storage/app/fonoteka-csv//` indefinitely. +**Fix:** Delete the key when the transaction fails. Decide on a retention step (on cancel, or in the Phase 14 import job) and record it. + +### IN-07: The first credential store under concurrency answers 500 + +**File:** `fonoteka.go/plugins/golem15/fonoteka/classes/credential_write_service.go:54-58`, `:84-105`, `:219-230` +**Issue:** `lockedCredentialID` runs `SELECT ... FOR UPDATE`, which locks nothing when no row exists. Two concurrent first stores both take the create path, and the second fails on the UNIQUE `user_id` / `organisation_id` constraint with the opaque 500. No data is corrupted. +**Fix:** Use `INSERT ... ON CONFLICT (user_id) DO UPDATE` for the create path, or retry once on a unique violation. + +### IN-08: The gate's required-test check matches test names without their package + +**File:** `summercms.go/scripts/check-phase13.sh:69`, `:100` +**Issue:** `passed` stores bare test names, so a required test is satisfied by a test with the same name in any package of the run. Lines that do not start with `{` are skipped silently, although the header comment says non-JSON output exits 4. +**Fix:** Key `passed` by `(Package, Test)` and require package-qualified names. Either count non-JSON lines or correct the comment. + +### IN-09: The case-status check accepts any status the route recorded in the fixture + +**File:** `fonoteka.go/parity/check_corpus.go:736`, `:756-777` +**Issue:** `caseStatuses` collects the status of every step with the route's id, so a case's manifest status passes when any call to the route in that fixture recorded it. Example: a 404 case passes if another step of the same route recorded a 404. This weakens the "manifest status differs from fixture" guard for fixtures that call one route several times. +**Fix:** Prefer the step whose `ID` equals the case id, and fall back to route-id matching only when no such step exists. + +--- + +_Reviewed: 2026-10-03_ +_Reviewer: Claude (gsd-code-reviewer)_ +_Depth: standard_