diff --git a/.planning/phases/14-domain-jobs-and-external-integrations/14-REVIEW.md b/.planning/phases/14-domain-jobs-and-external-integrations/14-REVIEW.md new file mode 100644 index 0000000..750988e --- /dev/null +++ b/.planning/phases/14-domain-jobs-and-external-integrations/14-REVIEW.md @@ -0,0 +1,288 @@ +--- +phase: 14-domain-jobs-and-external-integrations +reviewed: 2026-10-04T01:03:11Z +depth: standard +files_reviewed: 99 +files_reviewed_list: + - summercms.go/cmd/summer/main.go + - summercms.go/cmd/summer/parity.go + - summercms.go/examples/hello/main.go + - fonoteka.go/app/app.go + - fonoteka.go/main.go + - fonoteka.go/parity/check_corpus.go + - fonoteka.go/plugins.gen.go + - fonoteka.go/plugins/golem15/fonoteka/classes/ai_config_resolver.go + - fonoteka.go/plugins/golem15/fonoteka/classes/album_recognition.go + - fonoteka.go/plugins/golem15/fonoteka/classes/album_write_service.go + - fonoteka.go/plugins/golem15/fonoteka/classes/cover_importer.go + - fonoteka.go/plugins/golem15/fonoteka/classes/csv_canonical_matcher.go + - fonoteka.go/plugins/golem15/fonoteka/classes/csv_import_service.go + - fonoteka.go/plugins/golem15/fonoteka/classes/discogs/applicator.go + - fonoteka.go/plugins/golem15/fonoteka/classes/discogs/client.go + - fonoteka.go/plugins/golem15/fonoteka/classes/discogs/cover_fetcher.go + - fonoteka.go/plugins/golem15/fonoteka/classes/discogs/import_resolver.go + - fonoteka.go/plugins/golem15/fonoteka/classes/discogs/input_parser.go + - fonoteka.go/plugins/golem15/fonoteka/classes/discogs/mapper.go + - fonoteka.go/plugins/golem15/fonoteka/classes/discogs/price_suggestion.go + - fonoteka.go/plugins/golem15/fonoteka/classes/discogs/rate_limiter.go + - fonoteka.go/plugins/golem15/fonoteka/classes/discogs/rate_store.go + - fonoteka.go/plugins/golem15/fonoteka/classes/discogs/scorer.go + - fonoteka.go/plugins/golem15/fonoteka/classes/gates.go + - fonoteka.go/plugins/golem15/fonoteka/console/prune_notifications.go + - fonoteka.go/plugins/golem15/fonoteka/console/reindex.go + - fonoteka.go/plugins/golem15/fonoteka/controllers/api/album_cover_fetch_controller.go + - fonoteka.go/plugins/golem15/fonoteka/controllers/api/credentials_controller.go + - fonoteka.go/plugins/golem15/fonoteka/controllers/api/discogs_import_controller.go + - fonoteka.go/plugins/golem15/fonoteka/controllers/api/inbound_limits.go + - fonoteka.go/plugins/golem15/fonoteka/controllers/api/recognize_controller.go + - fonoteka.go/plugins/golem15/fonoteka/controllers/api/release_match_controller.go + - fonoteka.go/plugins/golem15/fonoteka/controllers/api/wishlist_release_match_controller.go + - fonoteka.go/plugins/golem15/fonoteka/csv_import_job.go + - fonoteka.go/plugins/golem15/fonoteka/csv_match_job.go + - fonoteka.go/plugins/golem15/fonoteka/discogs_wiring.go + - fonoteka.go/plugins/golem15/fonoteka/golem_wiring.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/routes.go + - fonoteka.go/plugins/golem15/fonoteka/updates/22_discogs_rate_windows_table.go + - fonoteka.go/plugins/golem15/fonoteka/wishlist_digest_job.go + - summercms.go/internal/build/build.go + - summercms.go/modules/beachcomber/searchable.go + - summercms.go/modules/beachcomber/typesense/engine.go + - summercms.go/modules/conga/job.go + - summercms.go/modules/fetchguard/client.go + - summercms.go/modules/fetchguard/fetch.go + - summercms.go/modules/fetchguard/ip.go + - summercms.go/modules/fetchguard/policy.go + - summercms.go/modules/sunscreen/sunscreen.go + - summercms.go/modules/surf/cors.go + - summercms.go/modules/tide/normalize.go + - summercms.go/modules/tide/upstream.go + - summercms.go/modules/tide/upstream_proxy.go + - fonoteka.go/plugins/golem15/golem/admin.go + - fonoteka.go/plugins/golem15/golem/plugin.go + - fonoteka.go/plugins/golem15/feedback/assets.go + - fonoteka.go/plugins/golem15/feedback/admin.go + - fonoteka.go/plugins/golem15/feedback/plugin.go + - fonoteka.go/plugins/golem15/feedback/jobs.go + - fonoteka.go/plugins/golem15/feedback/routes.go + - fonoteka.go/plugins/golem15/golem/updates/01_ai_models.go + - fonoteka.go/plugins/golem15/golem/updates/registry.go + - fonoteka.go/plugins/golem15/feedback/updates/01_feedback.go + - fonoteka.go/plugins/golem15/feedback/updates/registry.go + - fonoteka.go/plugins/golem15/golem/controllers/models_admin_controller.go + - fonoteka.go/plugins/golem15/golem/controllers/admin_registry.go + - fonoteka.go/plugins/golem15/golem/console/import_settings.go + - fonoteka.go/plugins/golem15/golem/models/registry.go + - fonoteka.go/plugins/golem15/golem/models/ai_model.go + - fonoteka.go/plugins/golem15/feedback/controllers/submissions_admin_controller.go + - fonoteka.go/plugins/golem15/feedback/controllers/admin_registry.go + - fonoteka.go/plugins/golem15/feedback/classes/submission_store.go + - fonoteka.go/plugins/golem15/feedback/classes/image_guard.go + - fonoteka.go/plugins/golem15/feedback/classes/sync_g15office.go + - fonoteka.go/plugins/golem15/feedback/classes/str_limit.go + - fonoteka.go/plugins/golem15/feedback/classes/g15office_client.go + - fonoteka.go/plugins/golem15/feedback/console/import_settings.go + - fonoteka.go/plugins/golem15/feedback/models/user_preference.go + - fonoteka.go/plugins/golem15/feedback/models/submission.go + - fonoteka.go/plugins/golem15/feedback/models/registry.go + - fonoteka.go/plugins/golem15/feedback/models/settings.go + - fonoteka.go/plugins/golem15/feedback/controllers/api/request.go + - fonoteka.go/plugins/golem15/feedback/controllers/api/feedback_api_controller.go + - fonoteka.go/plugins/golem15/feedback/controllers/api/winter_errors.go + - fonoteka.go/plugins/golem15/feedback/controllers/api/me_hidden_controller.go + - fonoteka.go/plugins/golem15/golem/classes/security/ssrf_guard.go + - fonoteka.go/plugins/golem15/golem/classes/providers/adapter.go + - fonoteka.go/plugins/golem15/golem/classes/providers/anthropic.go + - fonoteka.go/plugins/golem15/golem/classes/providers/openai.go + - fonoteka.go/plugins/golem15/golem/classes/valueobjects/response.go + - fonoteka.go/plugins/golem15/golem/classes/valueobjects/prompt.go + - fonoteka.go/plugins/golem15/golem/classes/factories/prompt_factory.go + - fonoteka.go/plugins/golem15/golem/classes/services/model_config.go + - fonoteka.go/plugins/golem15/golem/classes/services/ai_service.go + - fonoteka.go/plugins/golem15/golem/classes/services/settings.go + - fonoteka.go/plugins/golem15/feedback/assets/js/embed.js +findings: + critical: 1 + warning: 6 + info: 8 + total: 15 +status: issues_found +--- + +# Phase 14: Code Review Report + +**Reviewed:** 2026-10-04T01:03:11Z +**Depth:** standard +**Files Reviewed:** 99 (paths relative to the meta repo `/media/nvme/dev/golem15/summercms.io/summercms/`) +**Status:** issues_found + +## Summary + +I reviewed the Phase 14 source across four repositories: summercms.go (fetchguard client, sunscreen, tide upstream recording and replay, surf CORS, beachcomber, conga), fonoteka.go (the Discogs package, the CSV and digest workers, the AI and Discogs routes, the console commands), sm-golem-plugin and sm-feedback-plugin. I read the SUMMARY deviations, CONTEXT, SECURITY-REVIEW and deferred-items. Where the PHP reference was relevant, I compared against `/media/nvme/dev/golem15/fonoteka`. + +The guarded outbound client, the SSRF guard split, the per-token Postgres limiter, the scoped album lookups and the credential masking in sidecars all hold up. No credential is logged or answered on any path I traced. The main problems are concurrency and write-shape issues the threat register does not cover: + +- Album writes that follow a slow Discogs call save every column from a stale struct. +- The CSV match worker still lets a stale pass rewrite a remapped import, despite the D-10 claim. +- The anonymous feedback upload keeps an extension the attacker chooses in public storage. +- Several smaller non-idempotency and data-import gaps. + +Findings marked "parity" reproduce PHP behaviour. They are listed only where the behaviour is a security hole or contradicts a guarantee the phase claims. + +## Narrative Findings (AI reviewer) + +## Critical Issues + +### CR-01: Album writes after slow Discogs I/O save every column from a stale struct, silently reverting concurrent edits + +**File:** `fonoteka.go/plugins/golem15/fonoteka/classes/discogs/applicator.go:105`, `fonoteka.go/plugins/golem15/fonoteka/classes/discogs/cover_fetcher.go:345` and `:378`, through `fonoteka.go/plugins/golem15/fonoteka/classes/album_write_service.go:269` +**Issue:** `SaveAlbumRow` → `saveAlbumRow` ends in `db.Omit(...).Save(a)`, and GORM `Save` writes every column of the struct. The album passed in is loaded by `findAlbum` at the start of the request (`release_match_controller.go:215/444`, `album_cover_fetch_controller.go:58`). It is saved only after the Discogs work: +- the limiter `Acquire` and 429 sleeps can take up to the 15 s wait budget; +- `GetRelease` and the price suggestions; +- in `attachCover`, a cover download of up to 10 s. + +Any edit committed to the same album in that window is overwritten with the old values: name, notes, shelf, condition, quantity, format and the other columns. Sources of such edits include `PUT albums/{id}` from the Nuxt app, another household member, or the MCP client, which calls `cover-price/discogs` while the user keeps editing. PHP's `$album->save()` writes only dirty attributes, so this lost update is new in the Go port. It is not covered by the "no transaction around apply" deviation (14-03 #7). That deviation is about atomicity, not about clobbering columns the apply never touched. +**Fix:** Write only the columns this operation changed, or re-read and lock the row right before the write: +```go +// applicator: collect the fields actually set, then +cols := append(changed, "updated_at") +if err := db.Model(album).Select(cols).Updates(album).Error; err != nil { ... } + +// cover_fetcher.persistMarketPrice +err := db.Model(c.album).Select("market_price_stored", "market_price_currency", + "market_price_source", "updated_at").Updates(c.album).Error +// cover_fetcher.attachCover +err := db.Model(c.album).Select("cover_import_failures", "updated_at").Updates(c.album).Error +``` +The model rules (`checkAlbumModelRules`) and the broadcast hooks still have to run, so add a `SaveAlbumColumns(ctx, db, a, cols...)` helper next to `saveAlbumRow`. Do not drop the hooks. + +## Warnings + +### WR-01: A stale CSV match pass can cancel, advance or fork a remapped import (the race D-10 claims to close) + +**File:** `fonoteka.go/plugins/golem15/fonoteka/csv_match_job.go:127-128`, `:145-146`, `:196-197`, `:232-252` +**Issue:** The function comment says "a pass never moves a cancelled, remapped or committed import", but only the start update checks `match_job_id`. In this sequence, an old pass is running and the user saves a new mapping: `UpdateCsvMapping` cancels the old job, replaces the rows, sets `uploaded` and queues job B. Then: +1. On its next `CheckIfCanceled`, the old pass runs `UPDATE … SET status = canceled WHERE id = ?` with no condition (line 127). The remapped import becomes `canceled`. Job B then sees a terminal status and skips, so the user's remap destroys the import. This happens whenever River has not cancelled the old job's context yet, and every time in poll-only mode. +2. If B has already moved the import to `matching`, the old pass's final `UPDATE … SET status = preview WHERE id = ? AND status = matching` (line 145) or `fail()` (line 196) moves B's import to `preview` or `failed` while B's rows are still pending. Commit can then import unmatched rows. +3. `pause()` checks only `cur.Status == matching` under the lock (line 232). An old pass can therefore queue a third job and overwrite `match_job_id`, leaving two passes matching the same rows. + +PHP has the same unconditional cancel write (`AlbumCsvMatchJob.php:100-103`). This is still a defect because D-10 and the T-14-10 row present the class as fixed. +**Fix:** Scope every status write in the pass to the job that owns the import: +```sql +... WHERE id = ? AND status = ? AND match_job_id = ? -- final, fail +UPDATE golem15_fonoteka_csv_imports SET status = 'canceled', updated_at = ? + WHERE id = ? AND match_job_id = ? -- cancel branch +``` +In `pause()`, also require `cur.MatchJobID != nil && *cur.MatchJobID == jobID` before dispatching. Otherwise call `lostImport`. + +### WR-02: The anonymous feedback upload keeps an attacker-chosen file extension in public storage + +**File:** `fonoteka.go/plugins/golem15/feedback/classes/submission_store.go:85-87` +**Issue:** `storeScreenshot` builds the disk name from `filepath.Ext(shot.Filename)`, which the client controls, and writes it public. The `image`/`mimes` rules (`lagoon.validateMimes`) and `IsAllowedImage` check only the content: magic bytes plus `image.DecodeConfig` of the header. A valid PNG or GIF named `x.html`, `x.svg` or `x.xhtml`, with HTML or script in a text chunk after IHDR, therefore passes. It is stored as `storage/app/uploads/public/
/
/
/