docs(14): add code review report
This commit is contained in:
Jakub Zych
@@ -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/<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:
|
||||
```go
|
||||
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/test` sends a "ping" with `max_tokens` 4096 to OpenAI or Anthropic.
|
||||
- `discogs-credential/test` sends `GET /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:**
|
||||
```go
|
||||
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) `CompleteJob` fails 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) `AttachFile` fails after `CreateTask` succeeded. The retry creates a second task. PHP has the same weakness (parity).
|
||||
**Fix:** Return early when `sub.Status == models.StatusSent && sub.G15OfficeTaskID != nil`. Persist the task id as soon as `CreateTask` returns, before attaching. On a retry with a stored task id, only re-attempt the attachment. In `jobs.go`, log a `CompleteJob` failure 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_
|
||||
Reference in New Issue
Block a user