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

fix(11-08): make transaction gates fail closed

Browse Source
This commit is contained in:
Jakub Zych
2026-09-30 22:24:12 +02:00
parent 9526b6b638
commit 93c735b8a2
3 changed files with 22 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
.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); T-11-31, T-11-32 and RC-14 added in the 11-08 close-out"
reviewer: "gsd-executor, plan 11-07 (code-and-test review of plans 11-01 to 11-07); T-11-31, T-11-32, RC-14 and RC-15 added in the 11-08 close-out/review"
threats_open: 0
gate: "scripts/check-phase11.sh --all"
removal_harness: "scripts/check-phase11.sh --removal"
@@ -53,13 +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-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; nested calls must use the exact parent transaction connection, so root and unrelated transaction handles are rejected; Cabana create/update/delete/bulk-delete and relation Link/Unlink and fonoteka `SaveAlbum` run in `lagoon.Transaction` | `TestTransactionAfterCommit` (`plain_gorm_transaction_is_refused`, `nested_rejects_unrelated_transaction_handle`), `TestTransactionEdges` (nested root handle), beachcomber `TestSyncAfterCommit` (`plain_gorm_transaction_rollback_sends_nothing`), fonoteka `TestAlbumSearchDeleteAndFailures` (foreign transaction rollback never reaches Typesense); stages `--go`, `--postgres`, `--named` | pass; removal checks RC-14 and RC-15 fail 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. RC-14 was added and run in the 11-08 close-out on 2026-09-30 and failed as required.
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 fifteen were run on 2026-09-30 and all failed as required; `git status` was clean in both repositories afterwards. RC-14 and RC-15 were added and run in the 11-08 close-out/review and failed as required.
| Check | Threat | File | Anchor removed or changed | Replacement | Test run | Observed |
|-------|--------|------|---------------------------|-------------|----------|----------|
@@ -77,6 +77,7 @@ Each row is one anchor-exact mutation from `scripts/check-phase11.sh --removal`.
| 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 |
| RC-15 | T-11-31 | `modules/lagoon/transaction.go` | `if !parent.owns(gdb) {` (exact parent transaction identity) | `if false {` | `go test ./modules/lagoon -run '^TestTransactionAfterCommit$'` | fails: TestTransactionAfterCommit/nested_rejects_unrelated_transaction_handle; restored, cmp ok |
## Fixes made during the review