golem15/summercms
3
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(11-08): record T-11-31 and T-11-32 in the security review

Browse Source 
- check-phase11.sh --evidence refused the 11-08 threats missing from the
  review; adds both rows, the RC-14 removal check and the CR-01 fix row
This commit is contained in:
Jakub Zych
2026-09-30 21:00:20 +02:00
parent d4fc957f8f
commit e6777bda97
1 changed files with 9 additions and 3 deletions
Download Patch File Download Diff File Expand all files Collapse all files

12
.planning/phases/11-jobs-realtime-and-search-infrastructure/11-SECURITY-REVIEW.md
View File

@@ -1,7 +1,7 @@
---
phase: "11"
reviewed: "2026-09-30"
reviewer: "gsd-executor, plan 11-07 (code-and-test review of plans 11-01 to 11-07)"
reviewer: "gsd-executor, plan 11-07 (code-and-test review of plans 11-01 to 11-07); T-11-31, T-11-32 and RC-14 added in the 11-08 close-out"
threats_open: 0
gate: "scripts/check-phase11.sh --all"
removal_harness: "scripts/check-phase11.sh --removal"
@@ -9,7 +9,7 @@ removal_harness: "scripts/check-phase11.sh --removal"
# Phase 11 Security Review
This is a fresh code-and-test review of every threat in the registers of Plans 11-01 to 11-07 (T-11-01 to T-11-30 and T-11-SC). Severity and disposition are copied from the originating plan. A high threat counts as mitigated only when its named test or gate stage fails with the protection removed. `scripts/check-phase11.sh --removal` does this for every high mitigated threat: it applies an anchor-exact mutation that removes the protection, runs the named test, requires it to fail on an assertion (a build failure does not count), and restores the file byte for byte, checked with `cmp`. The results are recorded under "Removal checks". The accepted threat keeps its rationale verbatim from its originating plan.
This is a fresh code-and-test review of every threat in the registers of Plans 11-01 to 11-08 (T-11-01 to T-11-32 and T-11-SC; T-11-31 and T-11-32 come from gap plan 11-08). Severity and disposition are copied from the originating plan. A high threat counts as mitigated only when its named test or gate stage fails with the protection removed. `scripts/check-phase11.sh --removal` does this for every high mitigated threat: it applies an anchor-exact mutation that removes the protection, runs the named test, requires it to fail on an assertion (a build failure does not count), and restores the file byte for byte, checked with `cmp`. The results are recorded under "Removal checks". The accepted threat keeps its rationale verbatim from its originating plan.
The review found three defects in Phase 11 code, all fixed in plan 11-07 with a failing-when-broken test:
@@ -17,6 +17,8 @@ The review found three defects in Phase 11 code, all fixed in plan 11-07 with a
- lagoon handed after-commit callbacks a handle that carried the written model's statement (deferred from 11-05);
- beachcomber and lighthouse released their savepoint whenever the inner function reported no error, so a read failure that a Gate or channel function swallowed left the caller's Postgres transaction aborted (T-11-20, T-11-26).
Gap plan 11-08 closed verifier gap CR-01: `lagoon.AfterCommit` ran search sync immediately inside a plain GORM transaction, and the Cabana admin and `SaveAlbum` write paths used plain transactions, so Typesense received uncommitted, pre-pivot or rolled-back Album state (T-11-31, T-11-32).
Commands run from `summercms.go`. `../fonoteka.go` tests run inside that repository. Gate stages are modes of `scripts/check-phase11.sh`.
| Threat | Category | Component | Severity | Disposition | Production mitigation | Test or gate stage | Observed result | Residual risk |
@@ -51,11 +53,13 @@ Commands run from `summercms.go`. `../fonoteka.go` tests run inside that reposit
| T-11-28 | Information Disclosure | goldens and route fixtures | high | mitigate | `tide/centrifugo.go` records only whether `Authorization` matched; fonoteka `parity/check_corpus.go` fails on the test-only Centrifugo values and on an `X-Centrifugo-Secret` that is not a `{{var}}` or the documented deny literal | `TestCentrifugoRecorder` (value never stored), `TestCentrifugoRecorderRecordsPublishAndBroadcast`, parity `TestUniqueAndSecretScan` | pass; removal check RC-12 fails TestUniqueAndSecretScan | The corpus scan knows only the test-only values; live secrets never reach the isolated PHP |
| T-11-29 | Spoofing | recorder listener and parity:broadcasts target | medium | mitigate | `CentrifugoRecorder.ListenAndServe` and `RecordBroadcasts` require loopback addresses (tide's T-02-01 rule) | `TestCentrifugoRecorderRefusesNonLoopback`, `TestCentrifugoRecorder` (loopback serve and shutdown), `TestRecordBroadcastsStep` | pass | None known |
| T-11-30 | Repudiation | golden normalisation hiding regressions | medium | mitigate | `NormalizePublications` masks only `+00:00` timestamps, an exact `{user_id, name}` actor and captured ids under id keys; `DiffPublications` compares count, path, authorization and body; pending goldens skip, never pass | `TestNormalizePublications`, `TestDiffPublications`, parity `TestBroadcastGoldens` (created/updated must skip with "pending: Phase 12", enforced by `--named` and `--postgres`) | pass | Phase 12 must turn created/updated into assertions |
| T-11-31 | Information Disclosure | lagoon.AfterCommit / beachcomber sync | high | mitigate | `modules/lagoon/transaction.go` `AfterCommit` buffers inside `lagoon.Transaction` and GORM's implicit single-statement transaction, and inside a foreign plain GORM `*sql.Tx` logs a warning and skips the callback; a nested `lagoon.Transaction` given a root handle returns an error; Cabana create/update/delete/bulk-delete and relation Link/Unlink and fonoteka `SaveAlbum` run in `lagoon.Transaction` | `TestTransactionAfterCommit` (`plain_gorm_transaction_is_refused`), `TestTransactionEdges` (nested root handle), beachcomber `TestSyncAfterCommit` (`plain_gorm_transaction_rollback_sends_nothing`), fonoteka `TestAlbumSearchDeleteAndFailures` (foreign transaction rollback never reaches Typesense); stages `--go`, `--postgres` | pass; removal check RC-14 fails TestTransactionAfterCommit | Code that writes inside its own plain GORM transaction gets no search sync (warned, not silent) until it adopts `lagoon.Transaction` or calls `Sync` after commit |
| T-11-32 | Tampering | Cabana and SaveAlbum ordered artist pivots | medium | mitigate | The Album row and its ordered artist pivots commit in one `lagoon.Transaction` (Cabana `CRUDService.save`, `RelationService.Link`/`Unlink`, fonoteka `SaveAlbum`), and the buffered sync reloads the committed row and pivots | fonoteka `TestAlbumsAdminSearchUsesCommittedArtists` (assembled admin router; indexed `artist_ids` equal the committed pivot order), `TestSaveAlbumDefersAfterCommitUntilArtistsSync` (callback sees the committed order; none on rollback) | pass; with Cabana `save` reverted to a plain transaction the admin test fails (no import), and with `SaveAlbum` reverted the SaveAlbum test fails (no callback), both checked by hand in the 11-08 close-out | Concurrent commits for the same Album are not serialized; the Phase 12 SQL re-gate of `SearchIDs` results remains the boundary |
| T-11-SC | Tampering | Go module installs (River v0.47.0 and sub-modules) | high | mitigate | River is pinned at v0.47.0 with go.sum checksums; no Centrifugo, Typesense or Web Push client and no cron library was added in any plan | `check-phase11.sh --hygiene` (client libraries in `go list -m all` of both repositories, direct cron requirements, River version), `--self-test` (plants) | pass; removal check RC-13 (client rule disabled) fails `--self-test` | `robfig/cron/v3` is in the module graph only as River's test dependency, not a requirement |
## Removal checks
Each row is one anchor-exact mutation from `scripts/check-phase11.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 and compared with `cmp` against the copy saved before the mutation. All thirteen were run on 2026-09-30 and all failed as required; `git status` was clean in both repositories afterwards.
Each row is one anchor-exact mutation from `scripts/check-phase11.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 and compared with `cmp` against the copy saved before the mutation. All thirteen were run on 2026-09-30 and all failed as required; `git status` was clean in both repositories afterwards. RC-14 was added and run in the 11-08 close-out on 2026-09-30 and failed as required.
| Check | Threat | File | Anchor removed or changed | Replacement | Test run | Observed |
|-------|--------|------|---------------------------|-------------|----------|----------|
@@ -72,6 +76,7 @@ Each row is one anchor-exact mutation from `scripts/check-phase11.sh --removal`.
| RC-11 | T-11-22 | `modules/flare/flare.go` | `if !HostAllowed(host, p.cfg.AllowedHosts) {` | `if false {` | `go test ./modules/flare -run '^(TestSendAllowlist\|TestSendRefusesDisallowedEndpoint)$'` | fails: TestSendAllowlist, TestSendRefusesDisallowedEndpoint; restored, cmp ok |
| RC-12 | T-11-28 | `../fonoteka.go/parity/check_corpus.go` | `if strings.Contains(text, v) {` (centrifugo test value scan) | `if false && strings.Contains(text, v) {` | `go test ./parity -run '^TestUniqueAndSecretScan$'` | fails: TestUniqueAndSecretScan; restored, cmp ok |
| RC-13 | T-11-SC | `scripts/check-phase11.sh` (mutated copy) | `hits="$(grep -iE "$CLIENT_RE" "$modlist" \| head -n1 \|\| true)"` | `hits=""` | `bash <copy> --self-test` | fails: "refuse: self-test hygiene_11 accepted a planted client-gocent"; original untouched, cmp ok |
| RC-14 | T-11-31 | `modules/lagoon/transaction.go` | `if transactionalHandle(db) {` (foreign-transaction refusal in `AfterCommit`) | `if false {` | `go test ./modules/lagoon -run '^TestTransactionAfterCommit$'` | fails: TestTransactionAfterCommit/plain_gorm_transaction_is_refused, callback_handle_has_a_clean_statement; restored, cmp ok |
## Fixes made during the review
@@ -80,3 +85,4 @@ Each row is one anchor-exact mutation from `scripts/check-phase11.sh --removal`.
| A single-statement write enqueued its broadcast job after GORM's own commit, outside the write transaction | T-11-05 | `lighthouse` after-write callbacks also declare `Before("gorm:commit_or_rollback_transaction")` | `TestBroadcastTx/single_statement_write_enqueues_in_its_own_transaction` (RED: channels ran on the pool, not the write's `*sql.Tx`) | summercms.go `6f50b6c` |
| After-commit callbacks received a handle carrying the written model's statement; a `WithContext` query read the written model's table | T-11-26 | `lagoon` passes a clean handle (`Session{NewDB, Context}`, `Clauses()`, `Session{NewDB}`) on all three after-commit paths | `TestTransactionAfterCommit/callback_handle_has_a_clean_statement` (RED: `SELECT ... "lagoon_ac_items"."label"` and an aborted plain transaction) | summercms.go `c544319` |
| A read failure swallowed inside the sync or broadcast savepoint left the caller's transaction aborted (25P02) | T-11-20, T-11-26 | `beachcomber` and `lighthouse` roll back to the savepoint when its release fails | `TestSyncFailuresNonFatal/failed_gate_read_keeps_the_callers_transaction`, `TestBroadcastSwallowedReadFailure` (both RED with 25P02) | summercms.go `6df43d4` |
| Search sync ran inside plain GORM transactions, before the Album's artist pivots were written, and survived a rollback (verifier gap CR-01) | T-11-31, T-11-32 | Cabana writes and `SaveAlbum` use `lagoon.Transaction`; `lagoon.AfterCommit` refuses a foreign `*sql.Tx`; a nested `lagoon.Transaction` over a root handle errors | `TestAlbumsAdminSearchUsesCommittedArtists`, `TestSaveAlbumDefersAfterCommitUntilArtistsSync`, `TestTransactionAfterCommit/plain_gorm_transaction_is_refused`, `TestSyncAfterCommit/plain_gorm_transaction_rollback_sends_nothing` | summercms.go `f7b6b0c`, `a33b1ad`, `2766f34`; fonoteka.go `1c88199` |