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

docs(12): add code review report

Browse Source
This commit is contained in:
Jakub Zych
2026-10-02 17:08:18 +02:00
parent 560a16983c
commit 44a25dd380
1 changed files with 269 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

269
.planning/phases/12-p-ytarium-api-collections-and-albums/12-REVIEW.md Normal file
View File

@@ -0,0 +1,269 @@
---
phase: 12-p-ytarium-api-collections-and-albums
reviewed: 2026-10-02T00:00:00Z
depth: standard
files_reviewed: 85
files_reviewed_list:
- ../fonoteka.go/parity/check_corpus.go
- ../fonoteka.go/parity/fonoteka_reset.php
- ../fonoteka.go/parity/php_parity.sh
- ../fonoteka.go/plugins/golem15/fonoteka/classes/access.go
- ../fonoteka.go/plugins/golem15/fonoteka/classes/active_collection.go
- ../fonoteka.go/plugins/golem15/fonoteka/classes/added_date_parser.go
- ../fonoteka.go/plugins/golem15/fonoteka/classes/album_broadcast.go
- ../fonoteka.go/plugins/golem15/fonoteka/classes/album_files.go
- ../fonoteka.go/plugins/golem15/fonoteka/classes/album_queries.go
- ../fonoteka.go/plugins/golem15/fonoteka/classes/album_search.go
- ../fonoteka.go/plugins/golem15/fonoteka/classes/album_stats.go
- ../fonoteka.go/plugins/golem15/fonoteka/classes/album_sync.go
- ../fonoteka.go/plugins/golem15/fonoteka/classes/album_write_service.go
- ../fonoteka.go/plugins/golem15/fonoteka/classes/artist_resolver.go
- ../fonoteka.go/plugins/golem15/fonoteka/classes/backend_album_collection.go
- ../fonoteka.go/plugins/golem15/fonoteka/classes/collection_provisioner.go
- ../fonoteka.go/plugins/golem15/fonoteka/classes/completeness.go
- ../fonoteka.go/plugins/golem15/fonoteka/classes/cover_importer.go
- ../fonoteka.go/plugins/golem15/fonoteka/classes/duplicate_matcher.go
- ../fonoteka.go/plugins/golem15/fonoteka/classes/fingerprint.go
- ../fonoteka.go/plugins/golem15/fonoteka/classes/gates.go
- ../fonoteka.go/plugins/golem15/fonoteka/classes/image_guard.go
- ../fonoteka.go/plugins/golem15/fonoteka/classes/invitation_service.go
- ../fonoteka.go/plugins/golem15/fonoteka/classes/laravel_email.go
- ../fonoteka.go/plugins/golem15/fonoteka/classes/manual_cover_fetcher.go
- ../fonoteka.go/plugins/golem15/fonoteka/classes/notification_service.go
- ../fonoteka.go/plugins/golem15/fonoteka/classes/org_provisioner.go
- ../fonoteka.go/plugins/golem15/fonoteka/classes/php_values.go
- ../fonoteka.go/plugins/golem15/fonoteka/classes/serialize_album.go
- ../fonoteka.go/plugins/golem15/fonoteka/classes/serialize.go
- ../fonoteka.go/plugins/golem15/fonoteka/classes/share_service.go
- ../fonoteka.go/plugins/golem15/fonoteka/classes/style_resolver.go
- ../fonoteka.go/plugins/golem15/fonoteka/classes/tracklist_text_parser.go
- ../fonoteka.go/plugins/golem15/fonoteka/controllers/api/album_photos_controller.go
- ../fonoteka.go/plugins/golem15/fonoteka/controllers/api/albums_bulk_controller.go
- ../fonoteka.go/plugins/golem15/fonoteka/controllers/api/albums_controller.go
- ../fonoteka.go/plugins/golem15/fonoteka/controllers/api/album_search_controller.go
- ../fonoteka.go/plugins/golem15/fonoteka/controllers/api/album_stats_controller.go
- ../fonoteka.go/plugins/golem15/fonoteka/controllers/api/album_sync_controller.go
- ../fonoteka.go/plugins/golem15/fonoteka/controllers/api/collection_media_controller.go
- ../fonoteka.go/plugins/golem15/fonoteka/controllers/api/collections_controller.go
- ../fonoteka.go/plugins/golem15/fonoteka/controllers/api/collection_share_controller.go
- ../fonoteka.go/plugins/golem15/fonoteka/controllers/api/http_errors.go
- ../fonoteka.go/plugins/golem15/fonoteka/controllers/api/invitations_controller.go
- ../fonoteka.go/plugins/golem15/fonoteka/controllers/api/lookups_controller.go
- ../fonoteka.go/plugins/golem15/fonoteka/controllers/api/me_context_controller.go
- ../fonoteka.go/plugins/golem15/fonoteka/controllers/api/members_controller.go
- ../fonoteka.go/plugins/golem15/fonoteka/controllers/api/ratings_controller.go
- ../fonoteka.go/plugins/golem15/fonoteka/controllers/api/realtime_channels_controller.go
- ../fonoteka.go/plugins/golem15/fonoteka/controllers/api/request.go
- ../fonoteka.go/plugins/golem15/fonoteka/controllers/genre_controller.go
- ../fonoteka.go/plugins/golem15/fonoteka/jobs.go
- ../fonoteka.go/plugins/golem15/fonoteka/mail.go
- ../fonoteka.go/plugins/golem15/fonoteka/middleware/token_scope.go
- ../fonoteka.go/plugins/golem15/fonoteka/models/album.go
- ../fonoteka.go/plugins/golem15/fonoteka/models/collection.go
- ../fonoteka.go/plugins/golem15/fonoteka/models/slug.go
- ../fonoteka.go/plugins/golem15/fonoteka/models/style.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/user/classes/user_groups.go
- ../fonoteka.go/plugins/golem15/user/models/user.go
- ../fonoteka.go/plugins/golem15/user/models/user_group.go
- ../fonoteka.go/plugins/golem15/user/updates/202610020001_create_user_groups.go
- modules/beachcomber/engines.go
- modules/beachcomber/searchable.go
- modules/beachcomber/typesense/engine.go
- modules/lagoon/attach/thumb.go
- modules/lagoon/validate.go
- modules/lagoon/validate_request.go
- modules/lagoon/validate_rules.go
- modules/tide/centrifugo_golden.go
- modules/tide/diff.go
- modules/tide/fixture.go
- modules/tide/flow.go
- modules/tide/multipart.go
- modules/tide/normalize.go
- modules/tide/record.go
- modules/tide/replay.go
- modules/tide/variables.go
- scripts/check-phase10.1.sh
- scripts/check-phase10.sh
- scripts/check-phase11.sh
- scripts/check-phase12.sh
findings:
critical: 1
warning: 5
info: 6
total: 12
status: issues_found
---
# Phase 12: Code Review Report
**Reviewed:** 2026-10-02
**Depth:** standard
**Files Reviewed:** 85
**Status:** issues_found
## Summary
I reviewed the Phase 12 production sources in both repositories: the tenant scoping (`AccessibleBy`, `ScopedAlbums`, `Resolve`, the token pin), every collections, albums, household and invitation handler, the upload and thumbnail path, the cover fetchers, the share and invitation token handling, the Laravel validator port, the search re-gating, and the tide and gate tooling. I read `CLAUDE.md` and the 12-01..12-05 SUMMARYs and the security review first. PHP-faithful quirks recorded in those documents are not reported, unless the Go port makes their consequence worse.
Tenant scoping holds. Every id route goes through `AccessibleBy`, `FindAccessibleAlbum` or attachment-scoped lookups, and foreign ids get the same 404 as missing ones. The token pin is a separate grouped AND. The household and share routes are JWT-only and also refuse a token in the handler. Search ids and totals are re-gated in SQL. Invitation tokens are stored as sha256, the job args are encrypted, and share tokens use rejection sampling over `crypto/rand`. I found no SQL injection: identifiers in `exists:` are checked against `identName`, and the completeness SQL uses only constants.
The one blocker is in the upload path. An upload's file extension comes from the client and is stored in `disk_name` without sanitizing. `attach.File.Thumb` refuses any extension outside `[a-z0-9]+` with an error instead of using the broken-image fallback. So one upload named, for example, `cover.png_` (valid PNG bytes) leaves every later listing that shows that file answering 500. This is the T-12-16 failure mode that plan 12-05 meant to close; it is reached through the extension instead of the pixel count. I confirmed it with a scratch program against the real `attach` package (output below).
The warnings cover a stored-XSS vector that WinterCMS has too (the client extension of a public original), a lock-order inversion that can deadlock invitation acceptance against the resolver on Postgres, non-Latin artist names collapsing into one global artist row, 500 answers after a committed write, and synchronous Discogs fetches multiplied by the bulk row count.
## Critical Issues
### CR-01: Client file extension outside `[a-z0-9]+` poisons every listing with a permanent 500 (T-12-16 bypass)
**File:** `modules/lagoon/attach/thumb.go:153-156`; `../fonoteka.go/plugins/golem15/fonoteka/controllers/api/collection_media_controller.go:251-253`; `../fonoteka.go/plugins/golem15/fonoteka/classes/album_files.go:225-227`; `../fonoteka.go/plugins/golem15/fonoteka/classes/serialize.go:63-66`
**Issue:** `storeUpload` and `StorePublicFile` build `disk_name` as 22 hex characters plus `strings.ToLower(filepath.Ext(upload.Filename))`. Nothing restricts that extension. The `image|mimes:...` rule sniffs content only, and `validateMimes` refuses only PHP-like extensions. So a valid PNG uploaded as `x.png_`, `x.jp-g`, `x.jpeg~` or `x.zdjęcie` is accepted and its row is committed. `File.Thumb` then does:
```go
ext := fileExt(f.DiskName)
if !thumbToken.MatchString(ext) {
return "", fmt.Errorf("attach: invalid thumb extension %q", ext)
}
```
This error is not routed to `brokenThumb`. `SerializePhoto` returns it, `SerializeCollection`, `SerializeAlbums` and `AlbumBroadcastPayload` propagate it, and every handler maps it to `writeOpaque500`. The effects:
- The upload request answers 500 after the row has committed (`collectionUpload` line 123, `AlbumPhotoUpload` line 134).
- From then on, these all answer 500 for every member of the household: GET `collections`, `collections/{id}`, `albums`, `albums/{id}`, `albums/search`, `albums/sync` and `albums/missing`, plus every album update, because `emitAlbumEvent` serializes the photos.
- Any household member can do this, and so can any personal token with write scope. The token photo routes have no throttle.
WinterCMS does not fail here: `getThumb` builds a file name and the `makeThumb` catch branch serves the broken image. The Go behaviour is a regression, not parity.
Scratch reproduction (memblob, a real 10x10 PNG):
```
abcdef0123456789abcdef.png -> "/storage/uploads/abc/def/012/thumb_1_200_200_0_0_crop.png" err=<nil>
abcdef0123456789abcdef.png_ -> "" err=attach: invalid thumb extension "png_"
abcdef0123456789abcdef.jp-g -> "" err=attach: invalid thumb extension "jp-g"
```
**Fix:** Close it at both ends. In `Thumb`, an unusable extension is an unusable original, so it should get the broken picture instead of failing the listing (`ThumbFilename` already coerces to `jpg`):
```go
ext := fileExt(f.DiskName)
if !thumbToken.MatchString(ext) {
ext = "jpg" // keep the key a single safe path element
thumbKey := PartitionDirectory(f.DiskName) + ThumbFilename(f.ID, w, h, 0, 0, mode, ext)
return brokenThumb(ctx, bucket, f, thumbKey, fmt.Errorf("unsupported extension"))
}
```
At upload time, take the stored extension from the sniffed type, as `FetchManualCover` already does (`manualCoverExtensions[SniffImageMIME(data)]`). At minimum, refuse any client extension outside `[a-z0-9]{1,8}`. Do this in one shared helper (see IN-01) so that `storeUpload` and `StorePublicFile` cannot drift apart. Add a regression test: upload `x.png_`, then GET collections and GET albums/{id} must answer 200.
## Warnings
### WR-01: Public originals keep the client extension, so a valid image can be served as HTML (stored XSS on the uploads origin)
**File:** `../fonoteka.go/plugins/golem15/fonoteka/controllers/api/collection_media_controller.go:250-253`; `../fonoteka.go/plugins/golem15/fonoteka/classes/album_files.go:224-227`
**Issue:** Uploads land in a `file://` bucket under `storage/app/uploads/public`. A web server serves them, and it picks the Content-Type from the extension; the blob's `ContentType` metadata does not reach it. A PNG that carries `<script>` in a `tEXt` chunk passes `image|mimes:jpg,jpeg,png,gif,webp` (content sniff) and `IsAllowedImage` (it decodes). If it is uploaded as `x.html`, `x.htm` or `x.xhtml`, it is stored as `<hex>.html` and its URL is returned in the 201 response and in every listing. A browser opening it renders HTML. WinterCMS's `File::getDiskName` has the same behaviour, so this is inherited, not introduced. It is still exploitable by any household member or write-scoped token, and it compounds with CR-01, whose fix touches the same lines.
**Fix:** Derive the stored extension from the sniffed MIME type (`image/png` becomes `png`, and so on), using the shared helper suggested in CR-01. Keep the client name only in `file_name`. This changes the URL only for uploads whose client extension disagrees with their content, a case the parity corpus does not record. As defence in depth, add `X-Content-Type-Options: nosniff` and a types allow-list to the uploads location of the web server.
### WR-02: Lock-order inversion between AcceptInvitation and the resolver can deadlock on Postgres
**File:** `../fonoteka.go/plugins/golem15/fonoteka/classes/invitation_service.go:232-286`; `../fonoteka.go/plugins/golem15/fonoteka/classes/active_collection.go:418-443` and `344-359`
**Issue:** The resolver documents one lock order: the users row, then the context row. `guardPendingInvitation` locks the pending registration row and then the invitation row. `AcceptInvitation` takes the opposite order on both pairs:
1. It locks the invitation `FOR UPDATE` (line 234). Later, `DELETE FROM ..._pending_invitation_registrations` (line 278) locks the registration row. The guard does registration first, then invitation.
2. `persistContext` (line 255) locks the context row through the upsert. Then, for an org-less user, `UPDATE users` (line 265) locks the users row. `Resolve` and `ProvisionCollection` lock users first, then context.
The usual flow triggers this: a user registered through an invitation calls `me/context` (the guard, which answers 409) while POSTing `invitations/{token}/accept`. Postgres detects the cycle and aborts one transaction, and the client gets a 500. The PHP code has the same order but runs on SQLite (database-level locking), so it never deadlocks there. On Postgres it does.
**Fix:** Lock in the documented order at the start of the accept transaction. The answers do not change:
```go
// users -> pending registrations -> invitation, as Resolve/guard do
if _, err := lockUserAndContext(tx, user.ID); err != nil { return err }
if err := tx.Exec(`SELECT 1 FROM golem15_fonoteka_pending_invitation_registrations
WHERE user_id = ? ORDER BY id FOR UPDATE`, user.ID).Error; err != nil { return err }
// then SELECT ... FROM collection_invitations WHERE token_hash = ? FOR UPDATE
```
Add a concurrent test that runs accept and Resolve for the same invited user many times and requires no 500.
### WR-03: Artist names without Latin letters collapse into one global artist row (and genres into one empty-slug genre)
**File:** `../fonoteka.go/plugins/golem15/fonoteka/classes/artist_resolver.go:111-127`; `../fonoteka.go/plugins/golem15/fonoteka/models/slug.go:122-126,136-154`; `../fonoteka.go/plugins/golem15/fonoteka/controllers/api/lookups_controller.go:166`
**Issue:** `ASCII` drops every character outside its Latin table, so `NormalizeNameKey("Кино")` and `NormalizeNameKey("ДДТ")` are both `""`. `resolveArtistInput` looks up `name_key = ''` and reuses the first row it finds (`ON CONFLICT (name_key) DO NOTHING` shows the column is unique). As a result, every Cyrillic or Greek artist after the first is attached as the first one. Artists are a global pool, so one household's input changes another household's albums, `artist_display` and `GET artists`. `GenresStore` has the same pattern: with `slug = Slug(name)` equal to `""`, a second non-Latin genre matches the first. The 12-04 summary documents the missing transliteration as a gap, but not this consequence. Laravel's `Str::ascii` transliterates Cyrillic and Greek, so in PHP these names stay distinct.
**Fix:** When `NormalizeNameKey` returns `""`, fall back to a key that keeps the original letters, such as Unicode lower-case with whitespace collapsed, so distinct names never share a key. Also skip the `OR slug = ?` branch when `Slug(name) == ""`, in `GenresStore`, `StylesStore` and `findOrCreateStyle`. Better still, port voku's Cyrillic and Greek tables into `asciiMap`.
### WR-04: Album writes answer 500 after their transaction has committed
**File:** `../fonoteka.go/plugins/golem15/fonoteka/controllers/api/albums_controller.go:163-176,395-410`; `../fonoteka.go/plugins/golem15/fonoteka/controllers/api/albums_bulk_controller.go:80-120`; `../fonoteka.go/plugins/golem15/fonoteka/controllers/api/album_photos_controller.go:125-139`; `../fonoteka.go/plugins/golem15/fonoteka/controllers/api/collection_media_controller.go:123-127`
**Issue:** Store, update and bulk commit the album, then import covers, emit the event and serialize. Any failure in those later steps returns 500 (`writeAlbumWriteError` or `writeOpaque500`), but the album, and in bulk every row, already exists. Two consequences:
- The client treats a successful create as failed, so a retry creates a duplicate. On the bulk path that is N duplicates.
- Because broadcasting was suppressed during the write, the single `created`/`updated` event and `collection.bulk_updated` are never published, and other household devices miss the change.
The photo upload has the same shape: `StoreAlbumPhoto` commits, then `SyncCompletion` and `SerializePhoto` can fail. CR-01 turns this from theoretical into routine: the serialization step fails every time for a poisoned file.
**Fix:** Once the write has committed, stop turning side-effect failures into a failed request. Log a cover-import, event or completion failure and still answer 201/200 with the committed row, as PHP's `CoverImporter` does for download failures. At least put `StoreAlbumPhoto` and `SyncCompletion` in one transaction, and emit the album event even when the cover import fails.
### WR-05: Bulk create runs up to 5 synchronous Discogs downloads per row, with no per-request cap
**File:** `../fonoteka.go/plugins/golem15/fonoteka/controllers/api/albums_bulk_controller.go:80-84`; `../fonoteka.go/plugins/golem15/fonoteka/classes/cover_importer.go:286-302`
**Issue:** `MaxCovers` (5) applies per album. `AlbumsBulk` imports covers for every created row inside the request, and only the router body cap limits the row count. Each download may take `cover_timeout_seconds` (10 s). A single write-scoped token request with a few hundred rows of five `https://*.discogs.com/...` URLs holds a server goroutine and a database connection for a long time, and sends thousands of outbound requests to Discogs from the server's IP. That risks an upstream ban that would break cover import for every tenant. T-12-24 counts the cap per album, not per request.
**Fix:** Cap the total number of cover fetches per request (for example `MaxCovers` across the whole bulk, or a separate `bulk_max_covers`), or move bulk cover imports to a River job. In either case, give the import loop an overall deadline derived from the request context.
## Info
### IN-01: Two copies of the upload storage code
**File:** `../fonoteka.go/plugins/golem15/fonoteka/controllers/api/collection_media_controller.go:236-277`; `../fonoteka.go/plugins/golem15/fonoteka/classes/album_files.go:216-243`
**Issue:** `storeUpload` and `StorePublicFile` are the same disk-name, content-type and blob-write logic written twice. CR-01 and WR-01 have to be fixed in both places.
**Fix:** Have `collectionUpload` read the bytes and call `classes.StorePublicFile`, and put the extension policy there.
### IN-02: A pre-existing album write path and a no-op GORM callback remain
**File:** `../fonoteka.go/plugins/golem15/fonoteka/classes/album_write_service.go:47-106`; `../fonoteka.go/plugins/golem15/fonoteka/classes/artist_resolver.go:16-35,213-234`
**Issue:** `SaveAlbum`, `ResolveArtists` and `syncArtists` have no production caller now that the handlers use `CreateAlbum` and `UpdateAlbum`. They implement different semantics: no trimming, no completion stamp, artist ids only. `albumBeforeSaveCallback` is registered on every create and update of every model, and does nothing.
**Fix:** Remove them, or route a remaining caller (backend forms) through `CreateAlbum`/`UpdateAlbum`, so only one album write contract exists.
### IN-03: Accept writes the notification inside the accept transaction, unlike PHP and the doc comment
**File:** `../fonoteka.go/plugins/golem15/fonoteka/classes/invitation_service.go:212-222,285`
**Issue:** The doc comment says the notification is "written and published after commit". PHP calls `notifyInvitationAccepted` after `DB::transaction` returns. Go writes the row inside the transaction, so a notification failure rolls back the acceptance and answers 500.
**Fix:** Move `NotifyInvitationAccepted` after the transaction, using `gdb`, or correct the comment if keeping it atomic is intended.
### IN-04: A collection name containing CR or LF silently breaks its invitation mails
**File:** `../fonoteka.go/plugins/golem15/fonoteka/jobs.go:114-121`; `../fonoteka.go/plugins/golem15/fonoteka/controllers/api/collections_controller.go:198-205`
**Issue:** The name rules (`string|max:255`) allow internal newlines, and any editor may rename the collection. The name goes into the mail subject, and postcard's `rejectCRLF("subject", ...)` refuses it. The job then fails all three attempts and no mail is sent, while the owner saw 202.
**Fix:** Collapse whitespace in `collectionName` before handing it to the mailer, or encode the subject instead of refusing it.
### IN-05: The trim and numeric helpers are re-implemented in several places
**File:** `../fonoteka.go/plugins/golem15/fonoteka/controllers/api/lookups_controller.go:190-209`; `../fonoteka.go/plugins/golem15/fonoteka/classes/share_service.go:180-183`; `../fonoteka.go/plugins/golem15/fonoteka/models/collection.go:81-84`; `../fonoteka.go/plugins/golem15/fonoteka/controllers/api/collections_controller.go:145-194`
**Issue:** `classesTrim`, `phpTrimString`, `models.phpTrim`, `classes.phpTrim`, the inline `strings.Trim(..., " \t\n\r\v\x00")` and `phpNumericString` (beside `classes.phpNumericValue`) all re-implement the same PHP primitives. They agree today, but they will drift.
**Fix:** Export one `classes.PHPTrim` and `classes.PHPIsNumeric` and use them everywhere.
### IN-06: Global mutable `models.MarketCurrencyFunc` is set at Boot without synchronization
**File:** `../fonoteka.go/plugins/golem15/fonoteka/plugin.go:88-90`; `../fonoteka.go/plugins/golem15/fonoteka/models/album.go:270-283`
**Issue:** A package-level function variable is reassigned on every `Boot`. Parallel tests that boot apps with different configs race on it (`go test -race`), and the last boot wins for every app in the process.
**Fix:** Store it in an `atomic.Pointer[func() string]`, or pass the currency into `BeforeSave` through the write service.
---
_Reviewed: 2026-10-02_
_Reviewer: Claude (gsd-code-reviewer)_
_Depth: standard_