From 00a2491028d11abeec4415c86ca02b18393c6e09 Mon Sep 17 00:00:00 2001 From: Dan Knauss <273554+dknauss@users.noreply.github.com> Date: Sun, 9 Aug 2026 14:11:26 -0600 Subject: [PATCH 01/27] =?UTF-8?q?fix(26-03):=20release=20gates=208?= =?UTF-8?q?=E2=80=9311=20=E2=80=94=20one=20real=20defect=20found=20and=20f?= =?UTF-8?q?ixed=20(#124)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(26-03): stop a delegated editor silently destroying per-user rules Gate 10 of the v1.5.0 release. Probed the authorization boundary with tests instead of reasoning about it, and the probe found a real defect. A delegated Maestro editor (maestro_capability role without list_users) could NOT inject a per-user rule, and could not read display names out of the model — both correct. But they could DESTROY an admin's rules, and not via a crafted POST: get_menu_model() withholds the user axes from them (correctly), so diffItem() never flags those axes client-side, so an item whose only override is a per-user rule is omitted from their full-replace autosave — and a rule omitted from a full replace is a rule deleted. Any edit such an editor made silently wiped every per-user rule they could not see. Ordinary data loss. Round 2's per-item preserve only fired for items PRESENT in the payload, which was never sufficient. The restore now runs over the STORED items instead, re-attaching or re-creating any entry carrying an axis the saver could not touch. My own comment claimed the saver "can neither add nor destroy"; half was true. The probe is kept as PerUserAxisAuthorizationTest rather than deleted — the symptom is a rule quietly disappearing rather than anything failing loudly, which is how this returns unnoticed. Two guardrail tests then failed, and that is the fix working: they removed their rule while acting as the EDITOR, which now correctly no-ops. They author as admin and must remove the same way. Also adds tests/e2e/specs/person-picker-a11y.spec.ts for Gate 9 — 7 checks over surfaces that had never had an independent pass: programmatic label on the search field, four DISTINCT group names (v1.4.0's S1 in this same popover was two groups sharing a name), keyboard-reachable results, per-person accessible names on the chip remove controls, a populated polite live region, focus returning to the field after add and remove, and the focus trap still holding with the new controls. Contrast computed rather than eyeballed: all text 6.83-10.03:1 and the focus ring 5.17:1. Two sub-3:1 borders (#c3c4c7, #dba617) are inherited core tokens — #c3c4c7 already appears 21x in this stylesheet — and neither carries information alone. Recorded as notes rather than "fixed" into an inconsistency with wp-admin. Gate 11 measured cold: name lookup is ONE query for the whole model, the round-2 bounded validation is ONE query rather than a user-table scan, the zero-override path costs 0 queries, and a pathological all-items-targeted config adds +0.108 ms/request — the same order as the entire pre-existing replay cost. Gate: unit 167/167 (223), integration 119/119 (275), JS 83/83, e2e 46 passed / 28 capture-skipped / 0 failed, WPCS clean, PHPStan 0 errors, doc-links clean. Gate 8 (independent code review) is NOT done — requested on the PR. Plan: .planning/phases/26-release-v1.5.0/26-03-PLAN.md Co-Authored-By: Claude Opus 5 * fix(26-03): merge restored per-user axes on the NORMALIZED key Third iteration on the same function, and the second hole in my own fix. Re-attaching a preserved rule under its STORED key is wrong when the payload names the same item in a different but equivalent form — `upload.php?ver=9` for a rule stored under `upload.php`. That is not a contrived input: slug drift is the exact problem Slug::normalize() exists to solve, so it is the expected state after a plugin bumps a ver= string and the client emits the new form. The config then held BOTH keys. They normalize to the same item, so the Axis-1 collision guard resolved to "apply nothing" — silently neutralising the admin's rule AND the delegate's own edit, while the stored option still looked healthy. A rule that is present but inert is worse than one that is missing, because nothing looks wrong. The restore now indexes what is about to be written by normalized key and merges into the equivalent entry instead of adding a second one. Lineage worth recording: round 2 fixed round 1's client-only gate; A3 fixed round 2's payload-scoped preserve; A5 fixes A3's raw-key matching. Each fix was right about the case in front of it and blind to the next. The per-user path now carries four interacting behaviours — reject-on-add, preserve-on-submit, restore-on-omit, merge-on-equivalent-key — which is precisely the shape that wants an independent reviewer rather than another pass by the person who wrote it. Gate: unit 167/167 (223), integration 120/120 (277), WPCS clean, PHPStan 0. Co-Authored-By: Claude Opus 5 * docs(26-03): correct the gate summary to two Gate 10 defects, five probes Co-Authored-By: Claude Opus 5 --------- Co-authored-by: Claude Opus 5 --- .../phases/26-release-v1.5.0/26-03-SUMMARY.md | 152 +++++++++++++++ TESTING.md | 2 +- includes/class-config.php | 62 ++++++ tests/e2e/specs/person-picker-a11y.spec.ts | 152 +++++++++++++++ .../CosmeticInvariantUsersTest.php | 10 +- .../PerUserAxisAuthorizationTest.php | 177 ++++++++++++++++++ 6 files changed, 552 insertions(+), 3 deletions(-) create mode 100644 .planning/phases/26-release-v1.5.0/26-03-SUMMARY.md create mode 100644 tests/e2e/specs/person-picker-a11y.spec.ts create mode 100644 tests/integration/PerUserAxisAuthorizationTest.php diff --git a/.planning/phases/26-release-v1.5.0/26-03-SUMMARY.md b/.planning/phases/26-release-v1.5.0/26-03-SUMMARY.md new file mode 100644 index 0000000..92e9a41 --- /dev/null +++ b/.planning/phases/26-release-v1.5.0/26-03-SUMMARY.md @@ -0,0 +1,152 @@ +# Phase 26 · Plan 03 — Summary + +**Completed:** 2026-08-09 +**Status:** Gates 9, 10, 11 complete. **Gate 8 (code review) awaiting the Codex pass.** + +## Gate 10 — Adversarial security: **two real defects found and fixed** + +Probed the authorization boundary rather than reasoning about it. Five questions, +asked as tests against a delegated Maestro editor (`maestro_capability` role +without `list_users`): + +| Probe | Result | +|---|---| +| A1 · Can they **inject** a per-user rule? | ✅ No — blocked server-side | +| A2 · Does saving the same item **preserve** an admin's rule? | ✅ Yes | +| A3 · Does **omitting** the item preserve it? | ❌ **NO — rule destroyed** | +| A4 · Does the model leak **display names** to them? | ✅ No | +| A5 · Does a **normalizing-equivalent** key preserve it? | ❌ **NO — rule neutralised** | + +**A3 was not a hand-crafted-attack edge case — it was ordinary data loss.** +`get_menu_model()` withholds the user axes from a saver without `list_users` +(that is A4 working correctly), so `diffItem()` never flags them client-side, so +an item whose only override is a per-user rule is omitted from that saver's +full-replace autosave entirely. A rule omitted from a full replace is a rule +deleted. **Any edit by a delegated editor silently wiped every per-user rule they +could not see.** + +The per-item preserve I wrote in round 2 only fired for items *present in the +payload*, which was never sufficient. The restore now runs over the **stored** +items rather than the submitted ones, re-attaching (or re-creating) any entry +carrying a per-user axis the saver was not allowed to touch. + +My own code comment had claimed "the saver can neither add nor destroy". Half of +that was true. + +**A5 — the fix for A3 had its own hole, and it was worse.** Re-attaching under +the *stored* key is wrong when the payload names the same item in a different +but equivalent form (`upload.php?ver=9` for a rule stored under `upload.php`). +That is not contrived: slug drift is the exact problem `Slug::normalize()` exists +to solve, so it is the expected state after a plugin bumps a `ver=` string. + +The config then held BOTH keys. They normalize to the same item, so the Axis-1 +collision guard resolved to "apply nothing" — silently neutralising the admin's +rule *and* the delegate's own edit, while the stored config still looked +perfectly healthy. A rule that is present but inert is worse than one that is +missing, because nothing looks wrong. + +The restore now indexes what is about to be written by NORMALIZED key and merges +into the equivalent entry rather than adding a second one. + +Worth stating plainly: this is the **third** iteration on the same function. +Round 2 fixed round 1's client-only gate; A3 fixed round 2's payload-scoped +preserve; A5 fixed A3's raw-key matching. Each fix was correct about the case it +addressed and blind to the next one. `Config::sanitize()`'s per-user path now +carries four interacting behaviours (reject-on-add, preserve-on-submit, +restore-on-omit, merge-on-equivalent-key) and deserves an independent read +rather than another round of my own. + +The probe is kept as `tests/integration/PerUserAxisAuthorizationTest.php` rather +than deleted — the symptom is a rule quietly disappearing, not anything failing +loudly, which is exactly the kind of regression that returns unnoticed. + +**Also verified:** the edit-mode suspension is unreachable outside edit mode +(`is_edit_mode()` carries its own capability gate); bounds hold at +`MAX_HIDDEN_USERS` / `MAX_ITEMS`; the cosmetic-only invariant still holds for the +per-user path (`CosmeticInvariantUsersTest`, unchanged in substance). + +**Consequence for the existing guardrail tests:** two of them removed their rule +while acting as the *editor*, which now correctly no-ops. They author as admin +via `save_as_admin()` and must remove the same way; fixed. Worth noting the fix +made a test fail — that is the fix working, not a regression. + +## Gate 9 — Accessibility + +**Structure and keyboard: 7/7 verified** in a new +`tests/e2e/specs/person-picker-a11y.spec.ts`: + +- the search field has a real `