From 44a25dd3801326c4e4c258f6f7db606ce9ba73dc Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Fri, 2 Oct 2026 17:08:18 +0200 Subject: [PATCH] docs(12): add code review report --- .../12-REVIEW.md | 269 ++++++++++++++++++ 1 file changed, 269 insertions(+) create mode 100644 .planning/phases/12-p-ytarium-api-collections-and-albums/12-REVIEW.md diff --git a/.planning/phases/12-p-ytarium-api-collections-and-albums/12-REVIEW.md b/.planning/phases/12-p-ytarium-api-collections-and-albums/12-REVIEW.md new file mode 100644 index 0000000..9ffa579 --- /dev/null +++ b/.planning/phases/12-p-ytarium-api-collections-and-albums/12-REVIEW.md @@ -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= +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 `