24 KiB
phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status
| phase | reviewed | depth | files_reviewed | files_reviewed_list | findings | status | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 14-domain-jobs-and-external-integrations | 2026-10-04T01:03:11Z | standard | 99 |
|
|
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
Acquireand 429 sleeps can take up to the 15 s wait budget; GetReleaseand 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:
// 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:
- On its next
CheckIfCanceled, the old pass runsUPDATE … SET status = canceled WHERE id = ?with no condition (line 127). The remapped import becomescanceled. 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. - If B has already moved the import to
matching, the old pass's finalUPDATE … SET status = preview WHERE id = ? AND status = matching(line 145) orfail()(line 196) moves B's import toprevieworfailedwhile B's rows are still pending. Commit can then import unmatched rows. pause()checks onlycur.Status == matchingunder the lock (line 232). An old pass can therefore queue a third job and overwritematch_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:
... 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/<p>/<p>/<p>/<hex>.html. That directory is served by the web server from file://./storage/app/uploads/public (fonoteka.go/config/storage.yaml), which maps types by extension. The route needs only the public widget key and no Origin header, so any anonymous visitor can do this. The 88-bit random disk name is the only thing between this and stored XSS on the application origin: any later surface that shows the screenshot URL (an admin preview, an API) makes it exploitable. The framework's own attach.Store refuses this through extPattern plus allowedExtensions, and this code bypasses it. It is probably PHP parity (Winter derives the disk name from file_name too), so it is listed as a parity security hole.
Fix: Derive the extension from the sniffed type, never from the client name:
var shotExt = map[string]string{"image/jpeg": "jpg", "image/png": "png", "image/gif": "gif", "image/webp": "webp"}
ext, ok := shotExt[contentType]
if !ok { return nil, errors.New("feedback: screenshot is not an allowed image") }
diskName := hex.EncodeToString(raw) + "." + ext
Alternatively, store through attach.Store with Limits{Image: true, Extensions: …}. Also run shot.Filename through the same base-name cleaning attach.Store uses before saving it as file_name.
WR-03: Credential-test routes are unthrottled oracles for third-party keys (parity security hole)
File: fonoteka.go/plugins/golem15/fonoteka/routes.go (lines for /ai-credential/test and /discogs-credential/test), fonoteka.go/plugins/golem15/fonoteka/controllers/api/credentials_controller.go:272-323, :459-539
Issue: Both test routes accept an inline secret from any JWT holder, call the vendor from the server's IP, and answer whether the secret works:
ai-credential/testsends a "ping" withmax_tokens4096 to OpenAI or Anthropic.discogs-credential/testsendsGET /oauth/identity.
Neither has a route throttle or an in-handler bucket. The Discogs path also holds the request open for up to the 15 s limiter budget per token. A registered user can bulk-validate stolen OpenAI, Anthropic and Discogs keys through the application, spend the key owners' quota, and get the server's egress IP flagged or banned by the vendors. That would break the Discogs features for everyone. PHP has no throttle either (routes.php:296, :310), so this is a parity security hole. Adding a limit changes no recorded response below the limit.
Fix: Add an in-handler InboundLimits bucket keyed by user id, for example fonoteka-credential-test: at 10 per 60 s, answering PHP's generic 429 shape. Or add throttle:10,1 to both routes, matching the other sensitive JWT routes.
WR-04: The "write-only" admin AI key can be redirected to any host by changing base_url
File: fonoteka.go/plugins/golem15/golem/controllers/models_admin_controller.go:69-90, fonoteka.go/plugins/golem15/golem/classes/services/ai_service.go:104-108
Issue: FormBeforeUpdate keeps the stored key whenever the form sends it back empty or redacted, and allows any change to base_url or adapter in the same save. Admin models are Trusted, which means fetchguard TrustedMode: http allowed, no host list, no dial guard. An operator with golem15.golem.access_settings (the developer and publisher roles) cannot read the key. They can, however, point base_url at a host they control, leave the key redacted, and trigger any admin-tier call (a site admin's recognize). The stored provider key is then sent to them in Authorization/x-api-key. That defeats the write-only guarantee T-14-24 claims. The same TrustedMode lets that role reach internal addresses such as cloud metadata over http (D-05 accepts this for local model servers). The provider error text echoed by the 502 path makes that a partial-read SSRF.
Fix: In FormBeforeUpdate, load the stored base_url and adapter. When either differs from the submitted value and the key was not re-entered, return a validation error on api_key ("Re-enter the API key when changing the endpoint"). Consider limiting TrustedMode to the http and loopback/LAN case the operator explicitly opts into, rather than every admin model.
WR-05: golem:import-settings silently drops models from a repeater saved with non-sequential keys
File: fonoteka.go/plugins/golem15/golem/console/import_settings.go:152-158
Issue: The map[string]any branch exists for "a repeater saved with non-sequential keys" (PHP json_encode of an array after a middle item was deleted, for example {"0":…,"2":…}). It reads only the keys "0" to strconv.Itoa(len(t)-1), so every model whose key is >= len(t) is lost. The import still reports success ("Imported N AI models."). It also refuses to run again because the table is no longer empty, so the missing models stay missing after the one-time migration.
Fix:
case map[string]any:
keys := slices.Collect(maps.Keys(t))
slices.SortFunc(keys, func(a, b string) int {
ai, aerr := strconv.Atoi(a); bi, berr := strconv.Atoi(b)
if aerr == nil && berr == nil { return cmp.Compare(ai, bi) }
return strings.Compare(a, b)
})
for _, k := range keys { list = append(list, t[k]) }
json.Unmarshal into a map does not keep PHP's insertion order. If order matters, decode with an ordered decoder such as fonoteka's csv.DecodeOrdered pattern.
WR-06: The G15Office sync job is not idempotent, and a retry files duplicate tasks
File: fonoteka.go/plugins/golem15/feedback/classes/sync_g15office.go:174-200, fonoteka.go/plugins/golem15/feedback/jobs.go:325-328
Issue: SyncG15Office never checks whether the submission is already sent or already has a g15office_task_id. Every run calls CreateTask first. Two paths repeat it:
- (a)
CompleteJobfails after a successful sync (jobs.go:327). River then retries with up to 3 attempts, and each retry creates another G15Office task for the same report. This path is Go-only; PHP has no completion call. - (b)
AttachFilefails afterCreateTasksucceeded. The retry creates a second task. PHP has the same weakness (parity). Fix: Return early whensub.Status == models.StatusSent && sub.G15OfficeTaskID != nil. Persist the task id as soon asCreateTaskreturns, before attaching. On a retry with a stored task id, only re-attempt the attachment. Injobs.go, log aCompleteJobfailure instead of returning it, so River does not retry a job whose side effect already happened.
Info
IN-01: sunscreen does not scrub the Discogs token shape or struct fields
File: summercms.go/modules/sunscreen/sunscreen.go:54-63, :149-166
Issue: Scrub covers Bearer …, sk-… and x-api-key: …, but not Authorization: Discogs token=<token>, the one credential header this phase adds. redactAny formats structs with %+v and scrubs only those three patterns. A logged discogs.ClientConfig, *http.Request or similar value would print Token:abc… or Discogs token=abc… in clear. No current call site does this, but T-14-03 names sunscreen as the safety net.
Fix: Add {regexp.MustCompile((?i)(Discogs\s+token=)[^\s,"']+), "${1}" + Redacted} and redact exported struct fields whose names match RedactedKeys (via reflection) before falling back to %+v.
IN-02: tide sidecar masking accepts a partly masked credential and prints two unredacted URLs
File: summercms.go/modules/tide/upstream.go:655-666, :207; summercms.go/modules/tide/upstream_proxy.go:442
Issue: credentialResidueRe accepts any run of letters left over after the placeholders are removed. A vars value that matches only a suffix of the token (ABCxyz123 with var xyz123) leaves ABC{{x}}, which is written as "fully masked". The read-body error at upstream.go:207 and the multipart error at upstream_proxy.go:442 print req.URL/rawURL with the query, unlike every other message, which goes through redactURL.
Fix: Allow only a known scheme word (Bearer, Basic, Discogs token=) as residue, and use redactURL in both messages.
IN-03: The match job's re-dispatch delay overflows on a huge Retry-After
File: fonoteka.go/plugins/golem15/fonoteka/csv_match_job.go:245, fonoteka.go/plugins/golem15/fonoteka/classes/discogs/rate_limiter.go:144-147
Issue: RegisterRetryAfter saturates an out-of-range Retry-After to MaxInt. Any value above about 9.2e9 s overflows time.Duration(delay) * time.Second, and the next pass is scheduled in the past, so it runs immediately. The upstream is a fixed, trusted host, so this is unlikely in practice.
Fix: Clamp the delay, for example delay = min(delay, 3600), before building the Duration.
IN-04: security.AllowedHosts ignores a string-valued config key
File: fonoteka.go/plugins/golem15/golem/classes/security/ssrf_guard.go:58-72
Issue: When golem15.golem.ssrf.allowed_hosts arrives as a comma string (for example from an env overlay), neither switch case matches and an empty list is returned. Every base URL override is then refused. This fails closed, but it is surprising.
Fix: Add a case string: that splits on commas, as the env branch does.
IN-05: The G15Office task id from the response is concatenated into the request path unescaped
File: fonoteka.go/plugins/golem15/feedback/classes/g15office_client.go:129
Issue: hashID comes from the vendor's JSON and is pasted into /_support/api/v1/tasks/<id>/attachments. A value containing /, .. or ? changes the path on the locked host. The host lock keeps the impact small. PHP behaves the same.
Fix: url.PathEscape(hashID).
IN-06: embed.js escapeHtml is used in attribute context without escaping quotes (parity)
File: fonoteka.go/plugins/golem15/feedback/assets/js/embed.js:179-184, :313
Issue: escapeHtml (the textContent→innerHTML trick) does not escape ". labels.placeholder is placed inside placeholder="…", so an admin-set label containing " onfocus="… runs script on every embedding site. Only an operator with manage_settings can set it, and the file is served byte for byte from PHP.
Fix: Set the placeholder with textarea.placeholder = … after building the panel, or also replace " with " in escapeHtml. That changes the file's hash, so do it in the PHP original too, or record a deviation.
IN-07: Job cancellation inside the mapping and cancel transactions is not rolled back with them
File: fonoteka.go/plugins/golem15/fonoteka/classes/csv_import_service.go:579-581, :984-988
Issue: cancelCsvJob → conga.Manager.CancelJob writes summer_jobs and cancels the River job on its own connection, so the change commits immediately. If replaceCsvRows or the later updates fail and the transaction rolls back, the old match job stays cancelled while the import still names it in match_job_id with status matching. The import sits in matching until the user remaps.
Fix: Cancel after the transaction commits (record the ids under the lock, cancel after lagoon.Transaction returns nil). Alternatively, give CsvJobs a transactional cancel that writes summer_jobs on tx.
IN-08: wishlistReleaseMatchRules aliases releaseMatchRules' backing array
File: fonoteka.go/plugins/golem15/fonoteka/controllers/api/wishlist_release_match_controller.go:187
Issue: releaseMatchRules[:4] has capacity 9. Any future append to the wishlist rules would overwrite releaseMatchRules[4] (the draft rule) for the album routes.
Fix: var wishlistReleaseMatchRules = slices.Clip(releaseMatchRules[:4]), or declare the four rules explicitly.
Reviewed: 2026-10-04T01:03:11Z Reviewer: Claude (gsd-code-reviewer) Depth: standard