fix(26-03): release gates 8–11 — one real defect found and fixed - #124
Merged
Conversation
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 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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 <noreply@anthropic.com>
…obes Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
26-03 of Phase 26. Gate 10 found a genuine data-loss defect; gates 9 and 11 pass with notes; gate 8 needs an independent pass.
Gate 10 — the defect
Probed the authorization boundary with tests rather than reasoning about it, against a delegated Maestro editor (
maestro_capabilityrole withoutlist_users):This wasn't an exotic crafted POST — it was ordinary use.
get_menu_model()correctly withholds the user axes from such a saver, sodiffItem()never flags them, so an item whose only override is a per-user rule is omitted from their full-replace autosave. A rule omitted from a full replace is deleted. Any edit a delegated editor made silently wiped every per-user rule they couldn't see.Round 2's per-item preserve only fired for items present in the payload. The restore now runs over the stored items instead. My own comment had claimed the saver "can neither add nor destroy" — half was true.
Two guardrail tests then failed, which is the fix working: they removed their rule while acting as the editor, which now correctly no-ops.
Gate 9 — a11y
7 new checks over surfaces that had never had an independent pass: programmatic label, four distinct group names (v1.4.0's S1 here was two groups sharing one), keyboard-reachable results, per-person accessible names on remove controls, populated live region, focus returning after add/remove, focus trap intact.
Contrast computed, not eyeballed: text 6.83–10.03:1, focus ring 5.17:1. Two sub-3:1 borders are inherited core tokens (
#c3c4c7already appears 21× in this stylesheet;#dba617is core's notice amber) and neither carries information alone — recorded as notes rather than "fixed" into an inconsistency with wp-admin.Not covered: what a screen reader actually announces. The structure it depends on is proven; the announcement isn't.
Gate 11 — performance, measured cold
Same order as the entire pre-existing replay cost, and lost in admin TTFB.
Gate 8 — needs you
@codex review — please look at
v1.4.1..main, and particularly atConfig::sanitize()'s per-user authorization path, which now has three interacting behaviours: reject-on-add, preserve-on-submit, and restore-on-omit. That is exactly the shape where a fourth case hides. Every pass so far has found something real.Gate: unit 167/167 · integration 119/119 · JS 83/83 · e2e 46 passed/0 failed · WPCS clean · PHPStan 0.
🤖 Generated with Claude Code