From c97cc094d5de5d624799e5ac66bcb2c235c19e0d Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Fri, 2 Oct 2026 16:56:55 +0200 Subject: [PATCH] docs(12-05): complete the Phase 12 security proof, coverage and gate plan --- .../12-05-SUMMARY.md | 279 ++++++++++++++++++ 1 file changed, 279 insertions(+) create mode 100644 .planning/phases/12-p-ytarium-api-collections-and-albums/12-05-SUMMARY.md diff --git a/.planning/phases/12-p-ytarium-api-collections-and-albums/12-05-SUMMARY.md b/.planning/phases/12-p-ytarium-api-collections-and-albums/12-05-SUMMARY.md new file mode 100644 index 0000000..613857b --- /dev/null +++ b/.planning/phases/12-p-ytarium-api-collections-and-albums/12-05-SUMMARY.md @@ -0,0 +1,279 @@ +--- +phase: 12-p-ytarium-api-collections-and-albums +plan: 05 +subsystem: testing +tags: [fonoteka, security, search-leak, fuzz, route-table, coverage, gate, lagoon, attach, tide, beachcomber, parity] + +requires: + - phase: 12-p-ytarium-api-collections-and-albums + provides: "12-01..12-04: lagoon.ValidateRequest, attach URLs/Thumb, tide multipart, beachcomber.SearchPage/RegisterEngine, Resolve/AccessibleBy/AlbumsAccessibleBy, SearchAlbums, the collection, household and album handlers on both auth groups, 99 ported parity routes" + - phase: 11-jobs-realtime-and-search-infrastructure + provides: "scripts/check-phase11.sh gate pattern (stages, --self-test, --named, --removal, --evidence) and 11-SECURITY-REVIEW.md format" +provides: + - "TestSearchLeak: poisoned fake engine through the real albums/search route on both groups (stale-moved, mis-scoped, soft-deleted, removed-editor and its resolve race, token-pin, short-page, recount, cap, engine-error), page ids and meta.total compared exactly" + - "TestRouteTablePhase12: one inv.scope per token route equal to routes.php, JWT-only routes absent from the token group, [0-9]+ id constraints, D-01 throttles on exactly their routes" + - "FuzzWriteEndpoints over all 41 Phase 12 write routes from the route table, Postgres column snapshots, 41 committed seeds (82 seed runs)" + - "TestPhase12Threats: 26 subtests, one per mitigated T-12 threat not covered by the dedicated tests" + - "Full unit coverage (>= 80%) of lagoon, lagoon/attach, tide, beachcomber, beachcomber/typesense, fonoteka classes, controllers/api, sm-user-plugin classes and updates" + - "scripts/check-phase12.sh with --self-test, --go, --parity, --named, --removal (25 anchor-exact mutations), --coverage, --evidence, --all" + - "12-SECURITY-REVIEW.md, validated 12-VALIDATION.md, API-01 and API-02 Complete" +affects: [13, 14, 12.2] + +actuals: + tokens: 139512 + tasks: 3 + commits: 15 +# Measured: summercms.go `git rev-list --count 37595fb..1f4e1e0` = 12, of which 6 are 12-05's +# (the other 6 are the concurrent Phase 12.2 planning session's docs commits on the same branch); +# fonoteka.go `git rev-list --count 3b8304e..f60c3af` = 8; sm-user-plugin c258e9f = 1. +# 6 + 8 + 1 = 15, before this SUMMARY's own commit. +plan_head_before: "summercms.go 37595fb6b2c446e65aea5e25d1703fb8d297dba4; fonoteka.go 3b8304ea8cf6fe06af0972066e40e36e3ef2fe9a" +plan_head_after: "summercms.go 1f4e1e01c40a71621db1d9b6bfaa1a23aa266990; fonoteka.go f60c3af06f573cc6f98c60363eeea0a747ef1a8b" + +tech-stack: + added: [] + patterns: + - "Test-only beachcomber engine registered with RegisterEngine from _test.go files and selected by search.driver; it scripts ids/found/errors per call and records every Query" + - "Write-endpoint fuzz enumerates routes from surf's route table and diffs Postgres row snapshots against a per-route allowed-column set" + - "Removal harness: anchor must occur exactly once, the named test must fail on an assertion (build failure does not count), restore checked with cmp, refuses dirty files" + - "PHP truth tables recorded from WinterCMS/PHP 8 drive the validator and request-cast tests (in/not_in, (string) of floats, (int) casts, FILTER_VALIDATE_BOOLEAN, egulias e-mail domains)" + +key-files: + created: + - scripts/check-phase12.sh + - .planning/phases/12-p-ytarium-api-collections-and-albums/12-SECURITY-REVIEW.md + - ../fonoteka.go/plugins/golem15/fonoteka/fake_engine_test.go + - ../fonoteka.go/plugins/golem15/fonoteka/search_leak_test.go + - ../fonoteka.go/plugins/golem15/fonoteka/routes_table_phase12_test.go + - ../fonoteka.go/plugins/golem15/fonoteka/write_endpoints_fuzz_test.go + - ../fonoteka.go/plugins/golem15/fonoteka/testdata/fuzz/FuzzWriteEndpoints/ (41 seeds) + - ../fonoteka.go/plugins/golem15/fonoteka/phase12_security_test.go + - ../fonoteka.go/plugins/golem15/fonoteka/album_side_routes_test.go + - ../fonoteka.go/plugins/golem15/fonoteka/handler_failures_test.go + - ../fonoteka.go/plugins/golem15/fonoteka/handler_paths_test.go + - ../fonoteka.go/plugins/golem15/fonoteka/classes/phase12_classes_test.go + - ../fonoteka.go/plugins/golem15/fonoteka/classes/album_helpers_test.go + - ../fonoteka.go/plugins/golem15/fonoteka/classes/search_test.go + - ../fonoteka.go/plugins/golem15/fonoteka/controllers/api/phase12_controllers_test.go + - ../fonoteka.go/plugins/golem15/user/classes/user_groups_test.go (sm-user-plugin) + modified: + - modules/lagoon/attach/thumb.go + - modules/lagoon/validate_rules.go + - modules/lagoon/README.md + - docs/database/attachments.md + - modules/lagoon/validate_request_test.go + - modules/lagoon/validate_rules_test.go + - modules/lagoon/attach/url_test.go + - modules/tide/multipart_test.go + - modules/tide/normalize_upload_test.go + - ../fonoteka.go/plugins/golem15/fonoteka/classes/php_values.go + - ../fonoteka.go/plugins/golem15/fonoteka/classes/laravel_email.go + - ../fonoteka.go/plugins/golem15/fonoteka/controllers/api/request.go + - .planning/phases/12-p-ytarium-api-collections-and-albums/12-VALIDATION.md + - .planning/REQUIREMENTS.md + +key-decisions: + - "An unusable stored original (missing, undecodable, over 4096x4096) makes attach Thumb store WinterCMS's broken-image picture and return its URL instead of an error, as File::makeThumb's catch branch does (T-12-16: one 100-byte PNG used to 500 every later listing)" + - "--removal is not part of --all: it edits tracked source and refuses dirty files, so it runs as its own stage; the review records all 25 RC rows" + - "RC-18 removes CreateAlbum's server-set collection_id rather than making collection_id fillable, because setAlbumField has no setter for it and the fillable change alone never reaches the database" + - "The security review is self-performed by the executor (08-10 precedent); every high mitigated threat must have a removal row, enforced by --evidence" + +patterns-established: + - "Phase gate per phase: scripts/check-phaseNN.sh with fail-closed detectors proven by --self-test" + - "One subtest per threat id (t.Run(\"T-12-NN\")) so the removal harness can target a single threat" + +requirements-completed: [API-01, API-02] + +coverage: + - id: D1 + description: "A poisoned search index leaks no album and no count to a JWT user or a pinned token (ROADMAP SC-3, D-18, D-19)" + requirement: API-02 + verification: + - kind: integration + ref: "../fonoteka.go/plugins/golem15/fonoteka/search_leak_test.go#TestSearchLeak (-race)" + status: pass + - kind: other + ref: "scripts/check-phase12.sh --removal RC-02, RC-03, RC-04" + status: pass + human_judgment: false + - id: D2 + description: "Every token route carries PHP's one scope, JWT-only routes are absent from the token group, ids are constrained and throttles sit on exactly their routes (D-10, D-26)" + requirement: API-01 + verification: + - kind: integration + ref: "../fonoteka.go/plugins/golem15/fonoteka/routes_table_phase12_test.go#TestRouteTablePhase12" + status: pass + - kind: other + ref: "scripts/check-phase12.sh --removal RC-07, RC-08" + status: pass + human_judgment: false + - id: D3 + description: "No write endpoint of Phase 12 changes a server-owned column whatever extra keys it receives (ROADMAP SC-5, C-02)" + requirement: API-02 + verification: + - kind: integration + ref: "../fonoteka.go/plugins/golem15/fonoteka/write_endpoints_fuzz_test.go#FuzzWriteEndpoints (82 seed runs plus 60 s fuzzing)" + status: pass + - kind: other + ref: "scripts/check-phase12.sh --removal RC-18, RC-19" + status: pass + human_judgment: false + - id: D4 + description: "Every mitigated T-12 threat has a named test that fails when its protection is removed" + requirement: API-01 + verification: + - kind: integration + ref: "../fonoteka.go/plugins/golem15/fonoteka/phase12_security_test.go#TestPhase12Threats" + status: pass + - kind: other + ref: "scripts/check-phase12.sh --removal (25/25 fail as required) and --evidence" + status: pass + human_judgment: false + - id: D5 + description: "Phase 12 packages in both repositories at or above 80% statement coverage" + verification: + - kind: unit + ref: "scripts/check-phase12.sh --coverage (lowest: tide 81.4%, controllers/api 81.7%)" + status: pass + human_judgment: false + - id: D6 + description: "Fail-closed Phase 12 gate" + verification: + - kind: other + ref: "scripts/check-phase12.sh --self-test && scripts/check-phase12.sh --all (phase12 all passed, 7 min)" + status: pass + human_judgment: false + - id: D7 + description: "Security review, validation sign-off and requirement traceability" + verification: + - kind: other + ref: "scripts/check-phase12.sh --evidence" + status: pass + human_judgment: true + rationale: "The review text is written by the executor itself; a human should read 12-SECURITY-REVIEW.md before /gsd-verify-work closes the phase" + +duration: 2h15m +completed: 2026-10-02 +status: complete +--- + +# Phase 12 Plan 05: Phase 12 Security Proof, Unit Coverage and Gate Summary + +**Poisoned-index leak test, route-table scope test, a 41-route write fuzz and one test per T-12 threat, each proven by 25 anchor-exact removal mutations, plus 80%+ coverage in nine packages and a fail-closed `check-phase12.sh` gate; six parity and safety bugs found and fixed on the way.** + +## Performance + +- **Duration:** about 2h15m (including one interruption, see below) +- **Started:** 2026-10-02T12:43:34Z (after the 12-04 SUMMARY commit) +- **Completed:** 2026-10-02T14:57Z +- **Tasks:** 3/3 +- **Files modified:** 41 code/test files and 41 fuzz seeds across both repositories and the sm-user-plugin submodule, plus 3 planning docs + +## Accomplishments + +- `TestSearchLeak` drives the real `albums/search` route on both auth groups with a scripted test-only engine. It shows that stale, mis-scoped, soft-deleted, removed-editor and other-collection ids never show up in `data` or in `meta.total`/`last_page`. The recount pages by 250 and stops at 1000, and an engine error falls back to SQL. +- `TestRouteTablePhase12` checks the assembled router against a table of every Phase 12 route transcribed from routes.php (with line numbers). It covers scopes, JWT-only absences, `[0-9]+` constraints and throttles. +- `FuzzWriteEndpoints` gets its 41 write routes from the route table and sends every server-owned key with hostile values. It snapshots Postgres rows before and after and allows only each route's own columns to change. There is one committed seed per route, and 60 s of fuzzing came back clean. +- `TestPhase12Threats` has 26 subtests named by threat id. Together with the dedicated tests above, every mitigated T-12 threat has a named test. +- Unit coverage, framework: beachcomber 84.3%, beachcomber/typesense 95.9%, lagoon 84.4%, lagoon/attach 87.7%, tide 81.4%. +- Unit coverage, application: fonoteka classes 83.6%, controllers/api 81.7%, sm-user-plugin classes 88.7%, sm-user-plugin updates 86.2%. +- `scripts/check-phase12.sh`: `--all` passes (vet and tests in both repos, parity 99 ported and 0 failing, recorded 171/171, named tests, coverage, evidence). `--removal` reports all 25 RC mutations failing their named test, and both trees are clean afterwards. `--self-test` shows that every detector fails closed. +- 12-SECURITY-REVIEW.md, the validated 12-VALIDATION.md (`nyquist_compliant: true`, no pending rows), and API-01 and API-02 set to Complete. + +## Task Commits + +1. **Task 1: poisoned search index leak test** (fonoteka.go): `7962bfc` (test) +2. **Task 2: token scopes, write-endpoint fuzz, T-12 threat tests** (fonoteka.go): `e4122ae` (test) +3. **Task 3: unit coverage, gate, sign-off** + - summercms.go `1307060` fix: broken-image thumbnail for an unusable original (T-12-16) + - summercms.go `36983bc` fix: Laravel in/not_in on array values + - summercms.go `3ac1d64` fix: float to string with PHP's 14-digit precision + - summercms.go `6e30624` test: framework packages to full unit coverage + - summercms.go `6f4386c` chore: the fail-closed Phase 12 gate + - fonoteka.go `64f5496` fix: PHP 8 int/string/numeric casts + - fonoteka.go `817867d` fix: egulias domain part in the Laravel email rule + - fonoteka.go `2869747` fix: Laravel int/bool query parameters + - fonoteka.go `40b2c65` test: album_added recipients matched by the payload's album id + - fonoteka.go `1ac1911` test: fonoteka classes and API handlers to full unit coverage + - sm-user-plugin `c258e9f` test: user group codes and class helpers; fonoteka.go `f60c3af` chore: bump the submodule + - summercms.go `1f4e1e0` docs: security review, validation sign-off, API-01/API-02 + +## Files Created/Modified + +Listed in the `key-files` frontmatter. The production changes are limited to `modules/lagoon/attach/thumb.go`, `modules/lagoon/validate_rules.go` (plus its README and `docs/database/attachments.md`), and in fonoteka.go `classes/php_values.go`, `classes/laravel_email.go` and `controllers/api/request.go`. Each change is its own fix commit with a test that fails without it. + +## Decisions Made + +See `key-decisions`. The most significant: when `attach.File.Thumb` gets an unusable original, it now follows WinterCMS and stores the broken-image picture instead of returning an error. + +## Deviations from Plan + +### Auto-fixed Issues + +**1. [Rule 1 - Bug] A header-only 30000x30000 PNG made every later listing answer 500 (T-12-16)** +- **Found during:** Task 2 (T-12-16 subtest) +- **Fix:** `Thumb` logs the reason, stores `attach.BrokenImagePNG` and returns its URL +- **Files:** modules/lagoon/attach/thumb.go, README, docs/database/attachments.md +- **Verification:** TestThumbBrokenSourceServesPlaceholder (RED: three errors), TestPhase12Threats/T-12-16 +- **Commit:** summercms.go 1307060 + +**2. [Rule 1 - Bug] `in`/`not_in` on arrays diverged from Laravel** +- **Found during:** Task 3 (validator truth table) +- **Fix:** follow `array_diff` and `!validateIn` +- **Files:** modules/lagoon/validate_rules.go +- **Verification:** TestValidateRulesMatchLaravel (162 recorded cases) +- **Commit:** summercms.go 36983bc + +**3. [Rule 1 - Bug] Float-to-string cast used 17 digits instead of PHP's 14** +- **Found during:** Task 3 +- **Files:** modules/lagoon/validate_rules.go +- **Verification:** TestPHPFloatStringMatchesPHPCast +- **Commit:** summercms.go 3ac1d64 + +**4. [Rule 1 - Bug] PHP 8 (int), is_numeric and float-string casts diverged** +- **Found during:** Task 3 +- **Files:** classes/php_values.go +- **Verification:** TestPHPValueCasts (RED: nineteen values) +- **Commit:** fonoteka.go 64f5496 + +**5. [Rule 1 - Bug] The Laravel email rule's domain part diverged from egulias** +- **Found during:** Task 3 +- **Files:** classes/laravel_email.go +- **Verification:** TestValidLaravelEmailRFC +- **Commit:** fonoteka.go 817867d + +**6. [Rule 1 - Bug] `per_page=1e2` and `boolean()` parsing diverged from Laravel** +- **Found during:** Task 3 +- **Files:** controllers/api/request.go +- **Verification:** TestRequestCasts +- **Commit:** fonoteka.go 2869747 + +**7. [Rule 1 - Bug] Flaky test: album_added recipients were matched by position, not by the payload's album id** +- **Found during:** Task 3 (coverage runs) +- **Commit:** fonoteka.go 40b2c65 + +**Total deviations:** 7 auto-fixed (Rule 1). **Impact on plan:** each fix is needed for API parity or safety and is pinned by a test that fails without it. No scope creep and no new dependency. + +## Issues Encountered + +- **Interruption:** an API usage limit stopped the first executor in the middle of Task 3, while it was running `scripts/check-phase12.sh --all`. At that point all code commits existed, and the security review, validation file and REQUIREMENTS.md edits were still uncommitted. A continuation executor checked the commits and reviewed the uncommitted docs. It re-ran `--all` (passed, 7 min) and `--removal` (25/25 fail as required, 3 min, both trees clean afterwards). It then committed the docs as `1f4e1e0` and wrote this summary. No code was redone. +- **sm-user-plugin not pushed:** the plan asked for the submodule's master to be pushed. This run was told not to push, so `plugins/golem15/user` is still 3 commits ahead of origin/master (c65ab3c, 8697007, c258e9f). fonoteka.go's pointer `f60c3af` refers to c258e9f, so the submodule has to be pushed before anyone else clones fonoteka.go. +- **RC-04 is partial by design:** with `AlbumsAccessibleBy` removed from `ScopedAlbums`, stale-moved, mis-scoped and soft-deleted still pass, because the active-collection filter and GORM's `deleted_at` filter still exclude those rows. Only token-pin and removed-editor fail. This is documented in the review. + +## User Setup Required + +None. Push sm-user-plugin master when convenient (see above). + +## Next Phase Readiness + +Phase 12 is complete: all 5 plans have summaries, the gate is green, and API-01 and API-02 are Complete. Next steps are `/gsd-verify-work 12`, including a human read of 12-SECURITY-REVIEW.md and the manual-only fixture-recording row in 12-VALIDATION.md. Phase 12.2 is being planned in a parallel session. + +--- +*Phase: 12-p-ytarium-api-collections-and-albums* +*Completed: 2026-10-02* + +## Self-Check: PASSED + +- Files exist: scripts/check-phase12.sh, 12-SECURITY-REVIEW.md, 12-VALIDATION.md, search_leak_test.go, fake_engine_test.go, routes_table_phase12_test.go, write_endpoints_fuzz_test.go, phase12_security_test.go, 41 fuzz seeds +- Commits exist: 1307060 36983bc 3ac1d64 6e30624 6f4386c 1f4e1e0 (summercms.go); 7962bfc e4122ae 64f5496 817867d 2869747 40b2c65 1ac1911 f60c3af (fonoteka.go); c258e9f (sm-user-plugin) +- Acceptance: t.Run count in search_leak_test.go 21 (>= 9); T-12 subtests 26 (>= 15); RegisterEngine only in _test.go; nyquist_compliant true 1, TBD rows 0; T-12 ids in review equal the union of the plan registers; API-01/API-02 Complete; nine coverage lines >= 80%