diff --git a/.planning/phases/11-jobs-realtime-and-search-infrastructure/11-SECURITY-REVIEW.md b/.planning/phases/11-jobs-realtime-and-search-infrastructure/11-SECURITY-REVIEW.md index 0fec1ac..cee6d72 100644 --- a/.planning/phases/11-jobs-realtime-and-search-infrastructure/11-SECURITY-REVIEW.md +++ b/.planning/phases/11-jobs-realtime-and-search-infrastructure/11-SECURITY-REVIEW.md @@ -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 --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 diff --git a/scripts/check-phase10.sh b/scripts/check-phase10.sh index 405ecdc..2ea4114 100755 --- a/scripts/check-phase10.sh +++ b/scripts/check-phase10.sh @@ -54,6 +54,7 @@ require = set(os.environ.get("PHASE10_REQUIRE", "").split()) expect_skip = set(os.environ.get("PHASE10_EXPECT_SKIP", "").split()) skip_text = os.environ.get("PHASE10_SKIP_TEXT", "") skip_output = {} +skipped = set() passed = set() failed_tests = {} failed_pkgs = [] @@ -79,6 +80,7 @@ with open(path, encoding="utf-8", errors="replace") as fh: # A later phase's documented pending test (for example the Phase # 12 broadcast goldens) may skip, but only with its pending text. if test in expect_skip and skip_text and any(skip_text in o for o in skip_output.get(test, [])): + skipped.add(test) continue print(f"refuse: skipped {pkg} {test}", file=sys.stderr) sys.exit(2) @@ -117,6 +119,14 @@ missing = sorted(name for name in require if name not in passed) if missing: print("refuse: required tests did not pass: " + ", ".join(missing), file=sys.stderr) sys.exit(5) +unexpected_passes = sorted(expect_skip & passed) +missing_skips = sorted(expect_skip - skipped) +if unexpected_passes: + print("refuse: expected pending tests passed: " + ", ".join(unexpected_passes), file=sys.stderr) + sys.exit(7) +if missing_skips: + print("refuse: expected pending skips did not occur: " + ", ".join(missing_skips), file=sys.stderr) + sys.exit(7) if not passed: print("refuse: zero tests", file=sys.stderr) sys.exit(3) @@ -185,6 +195,10 @@ run_self_test() { PHASE10_EXPECT_SKIP="TestG/created" PHASE10_SKIP_TEXT="pending: Phase 12" expect_detect pending-skip-without-text 2 \ '{"Action":"skip","Package":"p","Test":"TestG/created"} {"Action":"pass","Package":"p","Test":"TestG"}' + PHASE10_EXPECT_SKIP="TestG/created" PHASE10_SKIP_TEXT="pending: Phase 12" expect_detect pending-skip-missing 7 \ + '{"Action":"pass","Package":"p","Test":"TestOther"}' + PHASE10_EXPECT_SKIP="TestG/created" PHASE10_SKIP_TEXT="pending: Phase 12" expect_detect pending-skip-passes 7 \ + '{"Action":"pass","Package":"p","Test":"TestG/created"}' expect_detect zero 3 '{"Action":"pass","Package":"git.golem15.com/golem15/summercms/modules/cabana"}' expect_detect nonjson 4 '{"Action":"pass",' expect_detect build 1 '{"Action":"build-fail","ImportPath":"p"} diff --git a/scripts/check-phase11.sh b/scripts/check-phase11.sh index 0cb8f69..65ffc7f 100755 --- a/scripts/check-phase11.sh +++ b/scripts/check-phase11.sh @@ -335,6 +335,9 @@ removal_table() { "hits=\"\"", "", "--self-test"], ["RC-14", "T-11-31", "root", "modules/lagoon/transaction.go", "if transactionalHandle(db) {", + "if false {", "./modules/lagoon", "^TestTransactionAfterCommit$"], + ["RC-15", "T-11-31", "root", "modules/lagoon/transaction.go", + "if !parent.owns(gdb) {", "if false {", "./modules/lagoon", "^TestTransactionAfterCommit$"] ] EOF @@ -646,7 +649,7 @@ run_named() { TestScheduledEntryMismatchSkipped TestScheduleUniqueByPeriod TestScheduleRunForeground TestScheduleValidation \ TestScheduleOrdering TestScheduleMissingCatalog TestScheduleLogWriter TestScheduleDueAt phase11_tests "$ROOT" ./modules/bonfire TestCall TestCallEdges - phase11_tests "$ROOT" ./modules/lagoon TestOnDatabaseAfterActivate TestQueueMigrationsUpDown TestTransactionAfterCommit + phase11_tests "$ROOT" ./modules/lagoon TestOnDatabaseAfterActivate TestQueueMigrationsUpDown TestTransactionAfterCommit TestTransactionEdges phase11_tests "$ROOT" ./modules/lighthouse TestBroadcastTx TestSuppression TestBulkEmitsOnce TestBroadcastEdges \ TestBroadcastSwallowedReadFailure TestMountSurfaces TestChannelIDMatchesPHP TestParseChannel TestRegistry phase11_tests "$ROOT" ./modules/lighthouse/centrifugo TestTokenClaims TestTokenHandler TestClientRequests TestProxy TestHealthCommand