20 KiB
phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status
| phase | reviewed | depth | files_reviewed | files_reviewed_list | findings | status | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 13-p-ytarium-api-wishlist-notifications-csv-credentials-public | 2026-10-03T00:00:00Z | standard | 67 |
|
|
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_enabledand 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:
RegisterEventgains aPayloadfield.RegisterUser,FireRegisterEvent,IssueTokenandAPIArrayare new exports./registerbehaviour is unchanged. The token is minted from the sameJWTSecretvalue, and the payload dropspasswordandpassword_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:
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:
- Request A (
PATCH mapping) reads the import in statuspreview. - Request B (
POST commit) wins the swap, sets the status toimporting, dispatches import job X and storesimport_job_id = X. - Request A's transaction deletes and recreates every row while job X is queued for them. It then writes
status = preview(canonical) oruploaded, and leavesimport_job_id = X. - 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.
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:
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) throughwishlist/{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:
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/<uid>/ 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