diff --git a/.planning/phases/14-domain-jobs-and-external-integrations/14-SECURITY-REVIEW.md b/.planning/phases/14-domain-jobs-and-external-integrations/14-SECURITY-REVIEW.md new file mode 100644 index 0000000..b8567a3 --- /dev/null +++ b/.planning/phases/14-domain-jobs-and-external-integrations/14-SECURITY-REVIEW.md @@ -0,0 +1,142 @@ +--- +phase: "14" +reviewed: "2026-10-04" +reviewer: "gsd-executor, plan 14-06 (self-performed code-and-test review of plans 14-01 to 14-06; no reviewer agent was spawned, per the 08-10 and 13-06 precedent)" +threats_open: 0 +gate: "scripts/check-phase14.sh --all" +removal_harness: "scripts/check-phase14.sh --removal" +--- + +# Phase 14 Security Review + +This is a code-and-test review of every threat in the registers of Plans 14-01 to 14-06: every id of the form T-14- (T-14-01 to T-14-38) and T-14-SC. Severity and disposition are copied from the originating plan. T-14-SC is declared by every plan, always low and accept, and is listed once. The executor performed the review itself, as plans 08-10 and 13-06 did, because no separate reviewer agent was spawned. + +A threat counts as mitigated only when its named test fails with the protection removed. `scripts/check-phase14.sh --removal` does this for every mitigated threat, framework and application, low severity included: +- it refuses a file with uncommitted changes (the shared plugins' submodule checkouts included); +- it applies an anchor-exact mutation that removes the protection; +- it runs the named test and requires it to fail on an assertion (a build failure does not count); +- it restores the file byte for byte, checked with `cmp`. + +The results are under "Removal checks". The two accepted threats (T-14-28, T-14-SC) keep their rationale from their originating plans. + +Commands run from `summercms.go`; `../fonoteka.go` tests run inside that repository, the shared plugins through its workspace. Gate stages are modes of `scripts/check-phase14.sh`. `TestPhase14Threats/T-14-NN` is the subtest of `../fonoteka.go/plugins/golem15/fonoteka/phase14_security_test.go` for that threat. The sm-feedback-plugin threats (T-14-30 to T-14-36) and T-14-24's admin side are pinned by the plugins' own tests, which live in the plugin repositories (a plugin's tests cannot be run from the application plugin without making it depend on the shared plugin). + +| Threat | Category | Component | Severity | Disposition | Production mitigation | Test or gate stage | Observed result | Residual risk | +|--------|----------|-----------|----------|-------------|-----------------------|--------------------|-----------------|---------------| +| T-14-01 | Tampering | fetchguard.Client guarded modes | high | mitigate | `modules/fetchguard/client.go` `check`: https only and the AllowHosts list before any I/O outside TrustedMode; the dial-time reserved-IP control on every connection; `fetch.go` `newHTTPClient` never follows a redirect | `TestClientSchemeGuard`, `TestClientModes`, `TestClientNeverFollowsRedirects` | pass; removal checks RC-01 (the https check disabled) and RC-02 (redirects followed) fail | None known | +| T-14-02 | Elevation of Privilege | trusted mode and transport override | high | mitigate | `TrustedMode` is set only in Go code; the transport override lives only behind `WithTransport`'s unexported context key; no exported Policy or Client field carries a RoundTripper | `TestTransportSeamIsCodeOnly` | pass; removal check RC-03 (an exported `Client.Transport` field) fails | None known | +| T-14-03 | Information Disclosure | application logs | high | mitigate | `modules/sunscreen` `Wrap` redacts the RedactCredentialsTap keys and scrubs Bearer, `sk-` and `x-api-key:` shapes; every generated main installs it first; surf's 500 hides panic text | `TestRedactHandler`, `TestScrub`, `TestInstallDefault`, `TestGenerateMainInstallsRedactingLogger`, `TestRecoverHidesPanicDetails` | pass; removal check RC-04 (`authorization` dropped from the redacted keys) fails | A credential logged under a key outside the list is caught only by the message scrub patterns | +| T-14-04 | Information Disclosure | committed sidecars | high | mitigate | `modules/tide/upstream.go` `maskUpstream` masks every vars value and refuses to write an Authorization or X-Api-Key header that is not fully masked; `check_corpus --check-secrets` scans the sidecars | `TestWriteUpstreamRefusesUnmaskedCredential`, `TestCheckCorpusUpstreamCredential`; stage `check-phase14.sh --parity` | pass; removal check RC-05 (the unmasked-credential refusal disabled) fails | None known | +| T-14-05 | Spoofing | recording proxy and CA | medium | mitigate | `NewUpstreamProxy` listens on loopback only; the CA key is 0600 outside the fixtures tree; forward mode only by explicit flag | `TestUpstreamProxyRefusesNonLoopback`, `TestEnsureParityCA`, `TestParityCommandContract` | pass; removal check RC-06 (the loopback check skipped) fails | None known | +| T-14-06 | Denial of Service | vendor response bodies | medium | mitigate | `Client.Do` wraps every body in `cappedBody` at the policy's MaxBytes | `TestClientBodyCap` | pass; removal check RC-07 (the cap raised to 2^62) fails | None known | +| T-14-07 | Tampering | beachcomber.DropIndex | medium | mitigate | `modules/beachcomber/typesense/engine.go` `DropIndex` deletes only `/collections/{escaped index}` and reports 2xx as existed, 404 as already absent, anything else as an error | `TestEngineDropIndex`, `TestDropIndex` | pass; removal check RC-08 (a 404 reported as existed) fails | None known | +| T-14-08 | Information Disclosure | Discogs token | high | mitigate | `classes/discogs/rate_limiter.go` `BucketID` is the first 32 hex characters of an HMAC with the app key; job args carry the import id only; client errors and logs carry status and path, never the token | `TestPhase14Threats/T-14-08` (a 32-hex bucket without the token; the queued match job's River args hold no token), `TestBucketID`, `TestRateLimiterPostgresAcquire` | pass; removal check RC-09 (the bucket is the token) fails | Whoever holds the app key can link buckets to tokens they already know | +| T-14-09 | Denial of Service | shared Discogs budget | medium | mitigate | `classes/discogs/rate_store.go` one atomic UNLOGGED upsert per grant (`… WHERE … OR w.hits < ? RETURNING`), threshold 50, 15 s wait budget | `TestPhase14Threats/T-14-09` (50 grants then a refusal on the Postgres store), `TestRateLimiterBudget`, `TestDiscogsRateWindowConcurrent`, `TestPhase14Edges` | pass; removal check RC-10 (every upsert granted) fails | None known | +| T-14-10 | Tampering | racing CSV writes | high | mitigate | `classes/csv_import_service.go` mapping, row save and cancel lock the import row (`lockCsvImport`) and re-check its status under the lock (D-10) | `TestPhase14Threats/T-14-10` (a stale caller cannot edit a committed import's row), `TestCsvWR02`, `TestPhase14Classes` | pass; removal check RC-11 (the re-check under the lock dropped) fails | None known | +| T-14-11 | Tampering | CSV row pick | high | mitigate | `csvAllowedCandidate` allows only the Discogs ids the match pass offered, checked before the fetch and again under the lock | `TestPhase14Threats/T-14-11` (a foreign pick is validation_failed and never fetched), `TestCsvRowPickSeam`, `TestCsvRowPickResolves` | pass; removal check RC-12 (every pick allowed) fails | None known | +| T-14-12 | Tampering | job retries | medium | mitigate | `csv_match_job.go` and `csv_import_job.go` fail the import and the job and return nil; only a timeout or a panic reaches River's retry | `TestPhase14Threats/T-14-12` (a Discogs 500 fails the job and returns nil), `TestCsvMatchJob`, `TestCsvImportJob`, `TestPhase14Jobs` | pass; removal check RC-13 (the pass returns its error) fails | None known | +| T-14-13 | Elevation of Privilege | import writes | high | mitigate | `csv_import_job.go` `importerCanWrite` before the run and before every row; a lost collection cancels the import | `TestPhase14Threats/T-14-13` (a stranger's import writes no album and is canceled), `TestCsvImportJob`, `TestPhase14Jobs` | pass; removal check RC-14 (the write check always true) fails | None known | +| T-14-14 | Repudiation | digest mail | medium | mitigate | `wishlist_digest_job.go` deletes the queue row before mailing and mails only when the delete removed it; `mailWishlistDigest` mails only a wishlist and an existing subscriber | `TestPhase14Threats/T-14-14` (a plain collection's digest mails nothing), `TestWishlistDigestJob`, `TestPhase14Jobs` | pass; removal check RC-15 (the wishlist-kind guard dropped) fails | None known | +| T-14-15 | Information Disclosure | reindex | high | mitigate | `console/reindex.go` aborts on tenantless albums before touching the index, runs the `collection_id:=0` integrity check after the import and drops only the prefixed legacy index | `TestReindexCommand` | pass; removal check RC-16 (the tenantless abort dropped) fails | None known | +| T-14-16 | Elevation of Privilege | album and wishlist match/apply | high | mitigate | `controllers/api/release_match_controller.go` scopes the album (`findAlbum`) before the gate, the bucket or any Discogs request | `TestPhase14Threats/T-14-16` (another user's album is 404 with no Discogs request), `TestDiscogsMatchRoute` | pass; removal check RC-17 (the album taken unscoped from the path) fails | None known | +| T-14-17 | Denial of Service | Discogs routes | medium | mitigate | `controllers/api/inbound_limits.go` fonoteka-discogs-missing 60/60 s shared by album and wishlist routes, fonoteka-discogs-import 20/60 s; `throttle:12,1` on cover-price | `TestPhase14Threats/T-14-17` (61st call 429), `TestDiscogsInboundLimits`, `TestPhase14Edges`, `TestRouteTablePhase14` | pass; removal check RC-18 (the bucket at 6000) fails | The buckets are per process (in memory), as 13-05 decided for the public counter | +| T-14-18 | Tampering | cover download | high | mitigate | `classes/cover_importer.go` `allowedURL` admits https discogs.com and the configured suffix only, before any request; the guarded client is AllowHostsMode too | `TestPhase14Threats/T-14-18` (a foreign and an http cover URL are never fetched), `TestCoverFetcherHostLock` | pass; removal check RC-19 (every host allowed) fails | None known | +| T-14-19 | Tampering | apply-release | medium | mitigate | `classes/discogs/applicator.go` fills only empty fields unless `overwrite_all`; a dry run stages year, label, catalog_number, country and the tracklist and never writes them. PHP's dry run still writes the covers and the other catalog fields; Go matches PHP (14-03 deviation 6), so the prohibition holds as "a dry run never writes a staged field" | `TestPhase14Threats/T-14-19` (a filled label and barcode survive, an empty discogs_id is filled, the staged year and country stay unwritten and appear in the draft), `TestApplyReleaseModes`, `FuzzWriteEndpoints` | pass; removal checks RC-20 (fill-empty skipped) and RC-21 (nothing staged) fail | A dry run writes what PHP's dry run writes | +| T-14-20 | Elevation of Privilege | token cover-price | high | mitigate | `routes.go` mounts cover-price on the token group with exactly `inv.scope:write` and `throttle:12,1` | `TestPhase14Threats/T-14-20` (a read-only token is 403), `TestRouteTablePhase14`, `TestCoverPriceRoute` | pass; removal checks RC-22 and RC-23 (the scope lowered to read) fail | None known | +| T-14-21 | Information Disclosure | discogs-credential/test | medium | mitigate | `controllers/api/credentials_controller.go` `DiscogsCredentialTest` answers `{"ok":true}` or PHP's message only; the token is never answered or logged | `TestPhase14Threats/T-14-21` (an inline and a stored token test answer no token and no failure text), `TestDiscogsCredentialTestRoute` | pass; removal check RC-24 (the failure text answered) fails | None known | +| T-14-22 | Tampering | user/org base_url | high | mitigate | `classes/ai_config_resolver.go` `aiConfigFrom` runs `security.AssertSafeURL` (https, allowlist, resolve-time private check) on every user and org base URL; the guarded client dials in PublicOnlyMode; a refusal is Winter's 500 page | `TestPhase14Threats/T-14-22` (a 169.254.169.254 base URL never reaches a provider), `TestSSRFGuard`, `TestAICredentialTestRoute`, `TestPhase14HandlersFailClosed` | pass; removal check RC-25 (the guard skipped) fails | `GOLEM15_SSRF_ALLOWED_HOSTS` is an operator decision (14-04, open question 1) | +| T-14-23 | Elevation of Privilege | trusted mode | high | mitigate | `AIConfig.Trusted` is set only by `AdminVisionModel` (the admin tier); user and org configs are never trusted | `TestPhase14Threats/T-14-23` (a user's credential resolves untrusted; the admin tier trusted), `TestAdminModelTrusted`, `TestAdminVisionTier` | pass; removal check RC-26 (user configs trusted) fails | None known | +| T-14-24 | Information Disclosure | AI api keys | high | mitigate | sm-golem-plugin `models/ai_model.go`: `lagoon.Encrypted` with `json:"-"`, `Hidden()`, a write-only admin field; the importer encrypts plaintext keys | `TestGolemAdminSchemas`, `TestPhase14Threats/T-14-24`, `TestImportSettings`, `TestAdminModelsForm` | pass; removal checks RC-27 and RC-28 (the key serialized, redacted) fail | Whoever holds the app key can decrypt stored keys | +| T-14-25 | Tampering | model output | medium | mitigate | `classes/album_recognition.go` treats the answer as data: PHP's system prompt, at most 30 albums, years outside 1889-2100 and unknown formats dropped | `TestPhase14Threats/T-14-25` (35 albums answered as 30 with no year), `TestRecognizeAlbums` | pass; removal check RC-29 (the album cap at 3000) fails | A model can still answer wrong album data; nothing is written | +| T-14-26 | Denial of Service | recognize | medium | mitigate | the AI gate, the fonoteka-recognize 10/60 s bucket and the image guard all run before any provider call | `TestPhase14Threats/T-14-26` (11th call 429), `TestRecognizeRoutes`, `TestPhase14Edges` | pass; removal check RC-30 (the bucket at 1000) fails | Per-process bucket (in memory) | +| T-14-27 | Tampering | photo uploads | medium | mitigate | `controllers/api/recognize_controller.go` refuses a photo `classes.IsAllowedImage` does not sniff and decode | `TestPhase14Threats/T-14-27` (a JPEG polyglot is 422 with no provider call), `TestRecognizeRoutes` | pass; removal check RC-31 (the guard skipped) fails | None known | +| T-14-28 | Information Disclosure | provider error echo | low | accept | PHP returns the provider's error text verbatim (the provider masks keys, for example `sk-parit******real`); parity requires it; logs are scrubbed by sunscreen. | none (accepted) | accepted | A provider that echoed a full key would reach the caller, as in PHP | +| T-14-29 | Information Disclosure | sm-golem-plugin push | low | mitigate | The shared plugin repositories hold code, config defaults and docs only; the diff is scanned before every push | `TestPhase14Threats/T-14-29` (a scan of the golem and feedback plugin trees for keys, private keys, DSNs with passwords, AWS ids and long bearer tokens) | pass; removal check RC-32 (a planted key in the golem README) fails | The scan sees the checkout, not the remote's history | +| T-14-30 | Spoofing | config and submit | high | mitigate | sm-feedback-plugin `controllers/api/feedback_api_controller.go` `KeyMatches` compares the widget key in constant time; `OriginAllowed` admits only a listed host and fails closed on an empty list | `TestFeedbackConfig`, `TestKeyMatches`, `TestAllowedOriginHosts`, `TestFeedbackSubmit` | pass; removal checks RC-33 (any key accepted) and RC-43 (any Origin accepted) fail | None known | +| T-14-31 | Denial of Service | public feedback routes | medium | mitigate | sm-feedback-plugin `plugin.go` buckets feedback-config 60/min and feedback-submit 10/min by trusted-proxy client IP | `TestFeedbackEdges` (60 reads then 429; the bucket values), `TestFeedbackSubmit` (11th post 429) | pass; removal check RC-34 (the submit bucket at 1000) fails | Per-process buckets | +| T-14-32 | Tampering | screenshot | medium | mitigate | the rule mimes and 10240 KB, then the copy of ImageContentGuard on the bytes | `TestFeedbackSubmit` (limits, guard-message), `TestImageGuardCopy`, `TestFeedbackEdges` | pass; removal check RC-35 (the content guard skipped) fails | None known | +| T-14-33 | Information Disclosure | G15Office token | high | mitigate | the token comes from config only and is sent only as the Bearer header; the sidecar masks it as `{{secret:g15office-token}}`; the job logs only the submission id | `TestSyncG15Office` (the PHP-recorded exchange replays with the masked bearer), `TestUpstreamSidecarsAreReplayed` | pass; removal check RC-36 (the bearer emptied) fails | None known | +| T-14-34 | Information Disclosure | submission forwarding | medium | mitigate | the G15Office client is AllowHostsMode for the configured host (https only); an unconfigured client sends nothing | `TestSyncG15Office` (http-base-url-is-refused, unconfigured-sends-nothing) | pass; removal check RC-37 (TrustedMode, so http is sent) fails | None known | +| T-14-35 | Elevation of Privilege | me/hidden | medium | mitigate | `controllers/api/me_hidden_controller.go` writes only the JWT principal's preference row | `TestMeHidden`, `TestFeedbackEdges` | pass; removal check RC-38 (the next user's row written) fails | None known | +| T-14-36 | Tampering | admin submissions list | low | mitigate | `models/submission/columns.yaml` renders every column as text; the list has no form and no buttons | `TestFeedbackAdminSchemas` | pass; removal check RC-39 (the message column as a partial) fails | None known | +| T-14-37 | Repudiation | gate counts | medium | mitigate | `check-phase14.sh` reads 175/172/3 from the replay's own coverage line and the manifest, and refuses failing, skipped, zero-test, racy and non-JSON runs | `check-phase14.sh --self-test` (each detector and the counts refused on planted input), `check-phase14.sh --parity` | pass; removal checks RC-40 (the count check disabled) and RC-41 (the skip detector disabled) fail the self-test | None known | +| T-14-38 | Tampering | --removal harness | medium | mitigate | `check-phase14.sh --removal` refuses a dirty target file, mutates by exact anchor, restores in a `finally` and on SIGINT/SIGTERM, checks the restore with `cmp`, and is not part of `--all` | `check-phase14.sh --self-test` (dirty file, non-unique anchor, build failure, surviving mutation, byte-identical restore) | pass; removal check RC-42 (the dirty-file refusal disabled) fails the self-test | None known | +| T-14-SC | Tampering | package installs | low | accept | No npm, pip, cargo or third-party Go module was added in Phase 14 (D-02); sm-golem-plugin and sm-feedback-plugin are in-house modules; `check-phase14.sh --go` refuses a vendor AI SDK in either module graph. | none (accepted); `check-phase14.sh --go` (module hygiene) | accepted | None known | + +## Removal checks + +Each row is one anchor-exact mutation from `scripts/check-phase14.sh --removal`. The anchor occurs exactly once in the file. The test must fail on an assertion, not a build failure. The file is restored byte for byte and checked with `cmp`. The script rows mutate a copy of the gate and run its `--self-test`. The run of 2026-10-04 passed every row (43 of 43). + +| Check | Threat | File | Anchor removed or changed | Replacement | Test run | Observed | +|-------|--------|------|---------------------------|-------------|----------|----------| +| RC-01 | T-14-01 | `modules/fetchguard/client.go` | the https check in `Client.check` | `if false && ...` | `go test ./modules/fetchguard -run '^TestClientSchemeGuard$'` | fails: TestClientSchemeGuard; restored, cmp ok | +| RC-02 | T-14-01 | `modules/fetchguard/fetch.go` | `return http.ErrUseLastResponse` in `CheckRedirect` | `return nil` | `go test ./modules/fetchguard -run '^TestClientNeverFollowsRedirects$'` | fails: TestClientNeverFollowsRedirects; restored, cmp ok | +| RC-03 | T-14-02 | `modules/fetchguard/client.go` | `type Client struct {` | plus an exported `Transport http.RoundTripper` | `go test ./modules/fetchguard -run '^TestTransportSeamIsCodeOnly$'` | fails: TestTransportSeamIsCodeOnly; restored, cmp ok | +| RC-04 | T-14-03 | `modules/sunscreen/sunscreen.go` | `"authorization"` in `redactedKeys` | removed | `go test ./modules/sunscreen -run '^TestRedactHandler$'` | fails: TestRedactHandler; restored, cmp ok | +| RC-05 | T-14-04 | `modules/tide/upstream.go` | the unmasked-credential refusal in `maskUpstream` | `if false && ...` | `go test ./modules/tide -run '^TestWriteUpstreamRefusesUnmaskedCredential$'` | fails: TestWriteUpstreamRefusesUnmaskedCredential; restored, cmp ok | +| RC-06 | T-14-05 | `modules/tide/upstream_proxy.go` | `requireLoopbackAddr(cfg.Listen)` in `NewUpstreamProxy` | a nil error | `go test ./modules/tide -run '^TestUpstreamProxyRefusesNonLoopback$'` | fails: TestUpstreamProxyRefusesNonLoopback; restored, cmp ok | +| RC-07 | T-14-06 | `modules/fetchguard/client.go` | `max: c.maxBytes` in `Client.Do` | `max: 1 << 62` | `go test ./modules/fetchguard -run '^TestClientBodyCap$'` | fails: TestClientBodyCap; restored, cmp ok | +| RC-08 | T-14-07 | `modules/beachcomber/typesense/engine.go` | `return false, nil` on a 404 in `DropIndex` | `return true, nil` | `go test ./modules/beachcomber/typesense -run '^TestEngineDropIndex$'` | fails: TestEngineDropIndex; restored, cmp ok | +| RC-09 | T-14-08 | `../fonoteka.go/plugins/golem15/fonoteka/classes/discogs/rate_limiter.go` | the HMAC return of `BucketID` | `return token` | `go -C ../fonoteka.go test ./plugins/golem15/fonoteka -run '^TestPhase14Threats$/^T-14-08$'` | fails: TestPhase14Threats, TestPhase14Threats/T-14-08; restored, cmp ok | +| RC-10 | T-14-09 | `../fonoteka.go/plugins/golem15/fonoteka/classes/discogs/rate_store.go` | `OR w.hits < ?` in `tryAcquireSQL` | `OR w.hits < ? OR TRUE` | `… -run '^TestPhase14Threats$/^T-14-09$'` | fails: TestPhase14Threats, TestPhase14Threats/T-14-09; restored, cmp ok | +| RC-11 | T-14-10 | `../fonoteka.go/plugins/golem15/fonoteka/classes/csv_import_service.go` | the status re-check under the lock in `saveCsvRowLocked` | removed | `… -run '^TestPhase14Threats$/^T-14-10$'` | fails: TestPhase14Threats, TestPhase14Threats/T-14-10; restored, cmp ok | +| RC-12 | T-14-11 | `../fonoteka.go/plugins/golem15/fonoteka/classes/csv_import_service.go` | the start of `csvAllowedCandidate` | every pick allowed | `… -run '^TestPhase14Threats$/^T-14-11$'` | fails: TestPhase14Threats, TestPhase14Threats/T-14-11; restored, cmp ok | +| RC-13 | T-14-12 | `../fonoteka.go/plugins/golem15/fonoteka/csv_match_job.go` | `return m.d.Jobs.FailJob(...)` in `csvMatcher.fail` | the cause returned | `… -run '^TestPhase14Threats$/^T-14-12$'` | fails: TestPhase14Threats, TestPhase14Threats/T-14-12; restored, cmp ok | +| RC-14 | T-14-13 | `../fonoteka.go/plugins/golem15/fonoteka/csv_import_job.go` | `return n > 0, err` in `importerCanWrite` | `return true, err` | `… -run '^TestPhase14Threats$/^T-14-13$'` | fails: TestPhase14Threats, TestPhase14Threats/T-14-13; restored, cmp ok | +| RC-15 | T-14-14 | `../fonoteka.go/plugins/golem15/fonoteka/wishlist_digest_job.go` | the wishlist-kind condition in `mailWishlistDigest` | removed | `… -run '^TestPhase14Threats$/^T-14-14$'` | fails: TestPhase14Threats, TestPhase14Threats/T-14-14; restored, cmp ok | +| RC-16 | T-14-15 | `../fonoteka.go/plugins/golem15/fonoteka/console/reindex.go` | the tenantless abort | removed | `go -C ../fonoteka.go test ./plugins/golem15/fonoteka/console -run '^TestReindexCommand$'` | fails: TestReindexCommand, TestReindexCommand/a_tenantless_album_aborts_before_the_index_is_touched; restored, cmp ok | +| RC-17 | T-14-16 | `../fonoteka.go/plugins/golem15/fonoteka/controllers/api/release_match_controller.go` | `s.findAlbum(w, r)` in `AlbumReleaseMatch` | the album taken from the path unscoped | `… -run '^TestPhase14Threats$/^T-14-16$'` | fails: TestPhase14Threats, TestPhase14Threats/T-14-16; restored, cmp ok | +| RC-18 | T-14-17 | `../fonoteka.go/plugins/golem15/fonoteka/controllers/api/inbound_limits.go` | `discogsMissingMax = 60` | `6000` | `… -run '^TestPhase14Threats$/^T-14-17$'` | fails: TestPhase14Threats, TestPhase14Threats/T-14-17; restored, cmp ok | +| RC-19 | T-14-18 | `../fonoteka.go/plugins/golem15/fonoteka/classes/cover_importer.go` | the host test of `allowedURL` | any host | `… -run '^TestPhase14Threats$/^T-14-18$'` | fails: TestPhase14Threats, TestPhase14Threats/T-14-18; restored, cmp ok | +| RC-20 | T-14-19 | `../fonoteka.go/plugins/golem15/fonoteka/classes/discogs/applicator.go` | the fill-empty skip in `Applicator.Apply` | removed | `… -run '^TestPhase14Threats$/^T-14-19$'` | fails: TestPhase14Threats, TestPhase14Threats/T-14-19; restored, cmp ok | +| RC-21 | T-14-19 | `../fonoteka.go/plugins/golem15/fonoteka/classes/discogs/applicator.go` | the dry-run staging in `Applicator.Apply` | `if false && ...` | `… -run '^TestPhase14Threats$/^T-14-19$'` | fails: TestPhase14Threats, TestPhase14Threats/T-14-19; restored, cmp ok | +| RC-22 | T-14-20 | `../fonoteka.go/plugins/golem15/fonoteka/routes.go` | `inv.scope:write` on cover-price | `inv.scope:read` | `… -run '^TestPhase14Threats$/^T-14-20$'` | fails: TestPhase14Threats, TestPhase14Threats/T-14-20; restored, cmp ok | +| RC-23 | T-14-20 | `../fonoteka.go/plugins/golem15/fonoteka/routes.go` | `inv.scope:write` on cover-price | `inv.scope:read` | `… -run '^TestRouteTablePhase14$'` | fails: TestRouteTablePhase14, TestRouteTablePhase14/group-middleware-scope-and-throttle; restored, cmp ok | +| RC-24 | T-14-21 | `../fonoteka.go/plugins/golem15/fonoteka/controllers/api/credentials_controller.go` | the unavailable message in `DiscogsCredentialTest` | `err.Error()` | `… -run '^TestPhase14Threats$/^T-14-21$'` | fails: TestPhase14Threats, TestPhase14Threats/T-14-21; restored, cmp ok | +| RC-25 | T-14-22 | `../fonoteka.go/plugins/golem15/fonoteka/classes/ai_config_resolver.go` | `security.AssertSafeURL` in `aiConfigFrom` | skipped | `… -run '^TestPhase14Threats$/^T-14-22$'` | fails: TestPhase14Threats, TestPhase14Threats/T-14-22; restored, cmp ok | +| RC-26 | T-14-23 | `../fonoteka.go/plugins/golem15/fonoteka/classes/ai_config_resolver.go` | the `AIConfig` built by `aiConfigFrom` | plus `Trusted: true` | `… -run '^TestPhase14Threats$/^T-14-23$'` | fails: TestPhase14Threats, TestPhase14Threats/T-14-23; restored, cmp ok | +| RC-27 | T-14-24 | `../fonoteka.go/plugins/golem15/golem/models/ai_model.go` | `json:"-"` on `APIKey` | `json:"api_key"` | `go -C ../fonoteka.go test ./plugins/golem15/golem -run '^TestGolemAdminSchemas$'` | fails: TestGolemAdminSchemas; restored, cmp ok | +| RC-28 | T-14-24 | `../fonoteka.go/plugins/golem15/golem/models/ai_model.go` | `json:"-"` on `APIKey` | `json:"api_key"` | `… -run '^TestPhase14Threats$/^T-14-24$'` | fails: TestPhase14Threats, TestPhase14Threats/T-14-24; restored, cmp ok | +| RC-29 | T-14-25 | `../fonoteka.go/plugins/golem15/fonoteka/classes/album_recognition.go` | `RecognitionMaxAlbums = 30` | `3000` | `… -run '^TestPhase14Threats$/^T-14-25$'` | fails: TestPhase14Threats, TestPhase14Threats/T-14-25; restored, cmp ok | +| RC-30 | T-14-26 | `../fonoteka.go/plugins/golem15/fonoteka/controllers/api/inbound_limits.go` | `recognizeMax = 10` | `1000` | `… -run '^TestPhase14Threats$/^T-14-26$'` | fails: TestPhase14Threats, TestPhase14Threats/T-14-26; restored, cmp ok | +| RC-31 | T-14-27 | `../fonoteka.go/plugins/golem15/fonoteka/controllers/api/recognize_controller.go` | the image guard in `AlbumRecognize` | `if false && ...` | `… -run '^TestPhase14Threats$/^T-14-27$'` | fails: TestPhase14Threats, TestPhase14Threats/T-14-27; restored, cmp ok | +| RC-32 | T-14-29 | `../fonoteka.go/plugins/golem15/golem/README.md` | the title line | plus a planted `sk-` key | `… -run '^TestPhase14Threats$/^T-14-29$'` | fails: TestPhase14Threats, TestPhase14Threats/T-14-29; restored, cmp ok | +| RC-33 | T-14-30 | `../fonoteka.go/plugins/golem15/feedback/controllers/api/feedback_api_controller.go` | the constant-time compare in `KeyMatches` | any key | `go -C ../fonoteka.go test ./plugins/golem15/feedback -run '^TestFeedbackConfig$'` | fails: TestFeedbackConfig, TestFeedbackConfig/key; restored, cmp ok | +| RC-34 | T-14-31 | `../fonoteka.go/plugins/golem15/feedback/plugin.go` | `BucketSubmit: {Max: 10, …}` | `Max: 1000` | `… ./plugins/golem15/feedback -run '^TestFeedbackEdges$'` | fails: TestFeedbackEdges, TestFeedbackEdges/buckets-60-and-10; restored, cmp ok | +| RC-35 | T-14-32 | `../fonoteka.go/plugins/golem15/feedback/controllers/api/feedback_api_controller.go` | the image guard in `Submit` | `if false && ...` | `… ./plugins/golem15/feedback -run '^TestFeedbackSubmit$'` | fails: TestFeedbackSubmit, TestFeedbackSubmit/limits, TestFeedbackSubmit/guard-message; restored, cmp ok | +| RC-36 | T-14-33 | `../fonoteka.go/plugins/golem15/feedback/classes/g15office_client.go` | `fetchguard.Bearer(c.token)` | `fetchguard.Bearer("")` | `… ./plugins/golem15/feedback/classes -run '^TestSyncG15Office$'` | fails: TestSyncG15Office, TestSyncG15Office/php-recorded; restored, cmp ok | +| RC-37 | T-14-34 | `../fonoteka.go/plugins/golem15/feedback/classes/g15office_client.go` | `Mode: fetchguard.AllowHostsMode` | `TrustedMode` | `… ./plugins/golem15/feedback/classes -run '^TestSyncG15Office$'` | fails: TestSyncG15Office; restored, cmp ok | +| RC-38 | T-14-35 | `../fonoteka.go/plugins/golem15/feedback/controllers/api/me_hidden_controller.go` | `user.ID` in the preference write | `user.ID+1` | `… ./plugins/golem15/feedback -run '^TestMeHidden$'` | fails: TestMeHidden; restored, cmp ok | +| RC-39 | T-14-36 | `../fonoteka.go/plugins/golem15/feedback/models/submission/columns.yaml` | `type: text` on the message column | `type: partial` | `… ./plugins/golem15/feedback -run '^TestFeedbackAdminSchemas$'` | fails: TestFeedbackAdminSchemas; restored, cmp ok | +| RC-40 | T-14-37 | `scripts/check-phase14.sh` (mutated copy) | the count comparison of `corpus_coverage` | `if False:` | `bash --self-test` | fails: "refuse: self-test corpus_coverage accepted: recorded 175/175 passing 171 failing 1 …"; original untouched | +| RC-41 | T-14-37 | `scripts/check-phase14.sh` (mutated copy) | the skip detector of `phase14_detect` | `if False:` | `bash --self-test` | fails: "refuse: self-test skip: detector exit 3, want 2"; original untouched | +| RC-42 | T-14-38 | `scripts/check-phase14.sh` (mutated copy) | the dirty-file refusal of `removal_harness` | `if False:` | `bash --self-test` | fails: "refuse: self-test removal harness mutated a dirty file"; original untouched | +| RC-43 | T-14-30 | `../fonoteka.go/plugins/golem15/feedback/controllers/api/feedback_api_controller.go` | the host match in `OriginAllowed` | any host | `… ./plugins/golem15/feedback -run '^TestFeedbackConfig$'` | fails: TestFeedbackConfig, TestFeedbackConfig/origins; restored, cmp ok | + +Not every protection is removable on its own, and the first run showed which: +- `OriginAllowed`'s empty-list check is redundant: an empty list never matches in the loop either, so removing only that check survived. RC-43 removes the host match instead. +- `DropIndex`'s path escaping is redundant for the tested names (Go escapes the path when it sends it), so RC-08 changes the 404 answer, which the test pins. +- T-14-19's first subtest filled only fields that a dry run stages, so the fill-empty mutation survived; the subtest now also pins a filled barcode and an empty discogs_id (fonoteka.go `cd393e4`). +- `TestGolemAdminSchemas` first checked only that the key text was absent, which `lagoon.Encrypted`'s redaction already guarantees; it now refuses any `api_key` key (sm-golem-plugin `6646d10`). +- Removing the whole `AssertSafeURL` block left the `security` import unused, a build failure that does not count; RC-25 now keeps the import. + +## Canon referrals of plans 14-02 to 14-05 + +The plans referred these canon concerns to this review instead of minting prohibitions. Each is covered by a row above. + +| Plan | Concern | Covered by | +|------|---------|------------| +| 14-02 | Token leakage through logs and errors (OWASP logging) | T-14-03, T-14-08, T-14-21 | +| 14-02 | SSRF through the Discogs host | the base URI is a code constant behind fetchguard AllowHostsMode `api.discogs.com` (T-14-01) | +| 14-03 | IDOR on album ids (OWASP access control) | T-14-16 | +| 14-03 | SSRF through cover URLs | T-14-18 | +| 14-04 | SSRF through AI base URLs (OWASP SSRF) | T-14-22, T-14-23 | +| 14-04 | API keys at rest and in logs (OWASP cryptographic storage, logging) | T-14-24, T-14-03 | +| 14-04 | Uploaded-file type spoofing (OWASP file upload) | T-14-27 | +| 14-05 | Cross-origin abuse of the widget (CORS/CSRF) | T-14-30 | +| 14-05 | Screenshot type spoofing (OWASP file upload) | T-14-32 | +| 14-05 | Feedback personal data retention (GDPR) | not a code threat: the port keeps PHP's retention unchanged (no automatic deletion); an operator decision for the cutover | + +## Fixes made during the review + +None in production code. The review found two tests too weak to catch their protection's removal (T-14-19's subtest and `TestGolemAdminSchemas`, above) and strengthened both.