fix: address ultrareview findings — collapse the per-user authorization path - #128
Merged
Conversation
…on path
The review found the invariant my own PR description asked reviewers to hunt
for ("the saver can neither add nor destroy") was false on two more paths. Both
confirmed by test before fixing.
PATH A — MAX_ITEMS starvation, and silent. The restore admitted only on
`count($out['items']) < MAX_ITEMS`, so a crafted payload of 200 title-only junk
entries filled every slot and left the protected rules nowhere to land. One POST
from a saver with no authority to write per-user rules destroyed all of them,
with no UI involved.
PATH B — the DELETE endpoint bypassed the gate entirely. Rest::reset_config() is
gated on capability(), not list_users, and Config::reset() wiped
unconditionally. Every sanitize-side round lived on the POST path. A boundary
enforced on save but not on reset is not a boundary.
RATHER THAN PATCH, COLLAPSED. Preserve-on-submit, restore-on-omit and
merge-on-equivalent-key were three mechanisms doing one job, each added to cover
a path the previous missed — which is precisely why a fourth path still got
through. They are now ONE map, keyed by normalized slug, built before the
payload loop by protected_user_axes(), with item-cap capacity RESERVED for it.
Starvation is impossible by construction rather than by another guard, and the
stored total stays bounded by MAX_ITEMS exactly as before: abusive filler is
what gets dropped, never an administrator's rules.
Config::reset() uses the same map, so "Reset All" means "reset everything you
can affect". An admin is unaffected — they hold list_users, the map is empty,
and it stays the plain wipe it always was. Asserted both ways.
Also two nits from the review, both real:
resetItem() had drifted behind resetSelected() — four fields against six. No
production consumer, but its docblock claims to mirror it. The round-trip test
passed VACUOUSLY because diffItem() short-circuits on absent keys, so the test
could never have caught the drift; it now seeds all four axes and asserts the
returned SHAPE, which is what actually pins the claim.
Group 1's setSet cleared .maestro-has-hidden on hiddenRoles alone, so unchecking
the last role while a person rule remained made the row look untouched until
reload. Now matches initModel() and the user group.
Gate: unit 167/167 (223), integration 126/126 single-site (285) + 126/126
multisite (290), JS 83/83, e2e 47 passed/0 failed, WPCS clean, PHPStan 0.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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. |
dknauss
added a commit
that referenced
this pull request
Aug 10, 2026
The ultrareview ran against #127 and returned three real findings, all fixed in #128 (707d9b6). It caught the two paths I had missed on the exact invariant the review request asked it to attack: MAX_ITEMS starvation (silent, one crafted POST destroyed every protected rule) and the DELETE endpoint bypassing the gate entirely. Records the outcome rather than just flipping a flag: four consecutive holes in one function, every one found by review rather than by me, and after each fix I believed the path was settled. Also carries forward the caveats that survive — the #128 fixes are themselves unreviewed, there has been no human screen-reader pass, and 21-05's browser verification never happened. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
dknauss
added a commit
that referenced
this pull request
Aug 10, 2026
The ultrareview ran against #127 and returned three real findings, all fixed in #128 (707d9b6). It caught the two paths I had missed on the exact invariant the review request asked it to attack: MAX_ITEMS starvation (silent, one crafted POST destroyed every protected rule) and the DELETE endpoint bypassing the gate entirely. Records the outcome rather than just flipping a flag: four consecutive holes in one function, every one found by review rather than by me, and after each fix I believed the path was settled. Also carries forward the caveats that survive — the #128 fixes are themselves unreviewed, there has been no human screen-reader pass, and 21-05's browser verification never happened. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
dknauss
added a commit
that referenced
this pull request
Aug 10, 2026
Tag v1.5.0 on 694b1bf; GitHub Release published; SVN deploy dispatched manually and verified FROM SVN — trunk Stable tag 1.5.0, tags/1.5.0/ present and carrying the per-user code (not merely the right version string), assets intact. Marks REL-11 and Phase 26 complete, flips the milestone to shipped, and adds the v1.5 entry to MILESTONES.md. The milestone entry records what this release should actually be remembered for: NINE defects found by verification and review, almost none by the test suite as first written. Most instructive is the run of FOUR consecutive holes in Config::sanitize()'s per-user authorization path — client-only gate, then payload-scoped preserve, then raw-key matching, then item-cap starvation, plus a DELETE endpoint that bypassed all of them. Each fix was correct about the case in front of it and blind to the next, and after each one the path looked settled. It only resolved by COLLAPSING three mechanisms into one, not by adding a fifth guard. The transferable lesson: when a fix keeps needing another fix, the shape is wrong, not the coverage. Also corrects ROLE-02's known-limitation text, which still claimed the multisite exempt branch was untested — the dedicated multisite CI lane closed that. Caveats carried forward rather than dropped at the finish line: the #128 fixes shipped unreviewed, there was no human screen-reader pass, and 21-05's browser verification never happened. A milestone record that quietly drops its caveats is how the next cycle inherits a false premise — which this project has already had to correct once. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
dknauss
added a commit
that referenced
this pull request
Aug 10, 2026
Session Continuity was badly stale — it still read "Last session 2026-08-02, stopped at Phase 21 context gathered, resume from 21-CONTEXT.md", which predates Phase 21 executing, v1.5.0 shipping, Phase 25, the correctness fixes, and the planning of Phases 27 and 28. Clearing context against that would have lost real continuity, which is the whole thing this section exists to prevent. Rewritten as a START HERE paragraph a fresh session can act on without reconstructing the history: - v1.5.0 is live, but main carries UNRELEASED work (Phase 25 + the three Phase 20 correctness fixes). A v1.5.1 patch is the obvious next release and clears the decks before new phases land on top — this is the single most important fact and was not recorded anywhere a resuming session would look. - Phases 27 and 28 are planned and unblocked, with a recommended order and the reasoning for it (28 is lighter, 28-01 ships alone, and it proves the shared modal shell with a scalar before 27 puts CRUD in one). - Both 2026-08-10 decisions are named with their files, flagged as read-before- reopening: the settings surface, and the fold story — including that the fold-versus-width "conflict" was mis-framed and is withdrawn, so nobody re-derives a constraint that does not exist. - The three carried caveats are restated rather than left only in the milestone entry: #128 shipped unreviewed, no human screen-reader pass, and 21-05's checkpoint accepted on automated evidence. - The wp-env port gotcha is written down, including that Playwright reads WP_ENV_TESTS_PORT independently, and that the other project's containers are not ours to stop. Also updates stopped_at and last_updated, both of which still described the state immediately after the v1.5.0 release.
dknauss
added a commit
that referenced
this pull request
Aug 10, 2026
#148) Reviewed the three items carried out of v1.5.0 rather than restating them, and two of the three moved. 21-05 TASK 5 — STRUCK AS SUPERSEDED. Phase 25 performed a genuine human pass in a live editor (25-VERIFICATION.md, 2026-08-09), and recorded itself as such precisely because this project had one checkpoint recorded the other way. It covered the toolbar and the locked-checkbox row. It did not cover the person picker or the four-group popover — so the residue is real, but it is the SAME residue as the screen-reader item, not a separate one. THE SCREEN-READER ITEM slightly WIDENED and now owns that residue. Phase 25's M2 change altered the same popover AFTER the v1.5.0 axe scan: the derived-locked checkbox went from natively disabled to aria-disabled, so a control that focus mode used to skip is now reachable and refuses its own toggle. That was the right fix — the lock reason was written for assistive technology and could never be heard while the row was skipped — but it changes tab order and announcement sequence in exactly the component that has never had a human pass, and axe passes over it either way. Both now live as one todo, with a concrete six-point script. Neither was ever a todo before; they were prose in STATE.md, which is how they survived two releases without moving. THE #128 GAP IS UNCHANGED AND VERIFIED STILL LIVE. 707d9b6 changed 192 lines of class-config.php plus the logic modules, and that mechanism is intact in main, touched since only by #138's 20-line entity-collision fix. It is the same sanitize path in which the ultrareview found four consecutive holes, and the one round no adversarial pass has seen. Closing it needs /code-review ultra over 1a32f08..707d9b6, which is user-triggered. Also struck one stale entry found while editing: the v1.5.0 milestone's "open, carried deliberately" list still said Phase 25 remains open. It shipped in v1.5.1. Struck in the same change, per the rule the 2026-08-09 reconciliation established. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 10, 2026
dknauss
added a commit
that referenced
this pull request
Aug 12, 2026
…ize hole (#153) * docs(compat): record the non-autoloaded option as differentiator D4 Dan observed on live sites that Admin Menu Editor's option row can dominate wp_options and slow admin loads. It is a known, acknowledged issue upstream, and Maestro's storage model is the direct counter-position -- but that was recorded nowhere except one cell of the 2026-08-01 architecture table. - PRIOR-ART: add D4 to Differentiate. AME's `ws_menu_editor` is autoloaded, so every request -- front end included, where the data is unused -- pays to fetch and unserialize it, and it grows with the whole admin menu rather than with what the user edited. Evidence is upstream, not inferred: Elsts confirms the autoload behavior in a wordpress.org thread and declines to flip it (an extra admin query), plus three size mitigations across 2.5 / 2.11 / 2.27 -- which is what makes it structural. Maestro's side is stated with the measured numbers and with the trade named honestly (1 extra admin-page query, exactly Elsts' objection; zero front-end cost, which is the bulk of real traffic). Framed as the storage-row consequence of the existing V2 finding, not a new claim. Notes the evidence caveat: the autoload flag comes from the reply and the changelog, not from the 1.15.1 source read. - Feature-sweep todo: the sweep had storage entirely out of scope and would have missed this. Carve out the footprint consequences -- format stays out, but AME's two Settings-tab toggles ("Compress menu configuration data", "Optimize menu configuration size") are user-facing features and earn rows, landing `deliberately-not`. Folds in the open evidence question so the zip read confirms the `update_option` call and upgrades D4 to `source-read`. - class-config.php: the MAX_CONFIG_BYTES docblock said the cap protects "every autoloaded read of it", contradicting the design two methods below it. Say admin-request read, and state that the option is never in `alloptions`. Comment-only code change: php -l and phpcs both clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(readme): sharpen the footprint copy, and pin down where the autoload cost lands Lead the Performance section with the alloptions point instead of burying it fourth, since it is the claim that distinguishes Maestro and the one a user can verify themselves -- `wp option list --autoload=on` does not list maestro_config. Adds the sparse-delta framing (storage tracks edits, not installed-plugin count) and the measured 0.1 ms / 1 ms figures with a link to the method. No competitor is named, and no comparative claim is made about any other plugin -- the copy describes the WordPress mechanism and what Maestro does with it. Also tightens D4 in the prior-art note. "Every request pays to fetch and unserialize it" was loose: wp_load_alloptions() pulls every autoloaded row in one query and holds it in memory, but the option's own maybe_unserialize() runs only when get_option() asks for it. The front-end tax is the bundle -- transfer, memory, and with a persistent object cache a per-request fetch and unserialize of the whole alloptions object, which is where a fat row hurts most. Notes that full-page caching spares requests that never boot WP. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(readme): lead the footprint section with the competitive claim, and retag Per Dan: state the performance advantage head-on rather than leaving the reader to infer it from the mechanism. Wording is his. The "nothing to compress, optimize, or tune" clause moves up into it, so it is dropped from the sparse-delta bullet to avoid saying it twice. Tags: drop "admin menu editor" -- guideline 12 treats a competitor's plugin name as spam, and the phrase is already in our plugin TITLE, which the directory weights far above tags, so the SEO cost is close to nil. (Evidence that tags are weak: Admin Menu Editor ranks #1 for "hide admin menu", "menu editor", "admin menu icons" and "hide menu items" while tagging none of them -- it tags only admin/dashboard/menu/security/wpmu.) Filled the slot with "rename menu items" rather than the requested "hide admin menu": Hide Admin Menu is itself a plugin (20k installs), so that swap would have reproduced the exact problem being fixed. "rename menu items" collides with no plugin name, pairs with the existing "hide menu items", and covers the one core capability the tag set had no term for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(readme): work the searched phrases into the description prose The directory indexes the whole readme and weights the title and body far above tags -- AME ranks #1 for "hide admin menu", "menu editor", "admin menu icons" and "hide menu items" while tagging none of them. So the description, not the tag line, is where discoverability is won, and ours led with "orchestrate the appearance of the WordPress admin menu": true to the product, but containing almost none of the literal phrases people type. - Short description now leads with "Hide admin menu items per user role", the highest-intent phrase we can claim. 146 chars, same as before, still under the 150 limit. - Opening bold line names the four capabilities in searchable form (rename / reorder / change icons / hide admin menu items) instead of only "orchestrate". - New second paragraph frames the jobs -- declutter a client site, rename cryptic plugin labels, reorder, hide per role -- so the phrases appear in prose a human wants to read rather than as a keyword list. Guideline 12 bars keyword stuffing; this stays on the right side of that line by describing real use cases. - Adds a one-line performance tease pointing at the footprint section, so the differentiator is visible above the fold rather than only near the bottom. The inline-editing paragraph is unchanged apart from a closing line, since that premise is still the lead story. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(config): store one key per normalized slug, closing a fifth sanitize hole Found by ultrareview against #149 — the review-only PR opened precisely because the #128 round had never been adversarially reviewed. It was the only finding, and it is real. THE BUG Two payload keys that normalize alike (`upload.php` and `upload.php?ver=1`) were both stored. The protected-map merge matches on the NORMALIZED key, claims the entry, and unsets it — so the SECOND equivalent spelling in the same payload found no match, took the no-match branch, and wrote itself as a fresh entry. Two stored keys normalizing to one is exactly the ambiguity Replay's Axis-1 guard resolves to "apply nothing". So a saver WITHOUT list_users could neutralise an administrator's per-user rule with a single POST: the rule stays visibly present in storage and applies nowhere. Same outcome as deletion, invisible to anyone auditing the config. WHY IT SURVIVED FOUR PRIOR ROUNDS The comment directly above the merge already argued this could not happen — matching on the normalized key so an equivalent spelling "lands here rather than producing a second entry later". That reasoning only ever covered the FIRST equivalent spelling. The seam was the unset() after the match. The A5 test covers exactly one equivalent key, so it could never have caught the two-key case. THE FIX Dedupe at the point of write, keyed by normalized slug, UNCONDITIONALLY — not just on the no-list_users path. The attack existed because the collision was constructible at all; blocking it on one path would leave the same seam for a sixth round. First spelling in incoming object order wins, matching the MAX_ITEMS break. Recorded only when an entry is actually stored, so an empty first entry does not shadow a real later one. THREE REPLAY TESTS RE-SEEDED, NOT WEAKENED test_collision_noop_ambiguous_stored_keys_apply_nothing, test_axis1_guard_extends_to_qualified_keys and test_child_hidden_roles_does_not_fire_on_ambiguous_parent all built their fixture by SAVING an ambiguous pair, which sanitize now refuses to write. They now seed via update_option(). The replay-side guard is untouched and still load-bearing: the ambiguity remains reachable from configs written by any version before this fix (i.e. every install upgrading into it), from slug drift where two keys did not normalize alike when saved but do now, and from direct DB edits. Seeding through save() would have asserted sanitize's behaviour while claiming to assert replay's, and gone quietly vacuous. All three re-verified FALSIFIABLE: replacing the guard with first-wins fails all three. A first probe using last-wins only failed two — the third fixture happens to order its entries such that last-wins masks it, which is worth knowing before anyone trusts that shape of probe again. TESTS - A8 (PerUserAxisAuthorizationTest) — the adversarial two-key case. Written first, watched fail, stored keys printed in the failure message. - test_save_stores_one_key_per_normalized_slug (RestConfigTest) — the ordinary admin half, since the dedupe is unconditional. Written after the code, so falsifiability was proven explicitly by disabling the guard. Gate: integration 129/129 single-site (291 assertions) + 129/129 multisite (296), unit 167/167 (223), JS 83/83, WPCS clean, PHPStan 0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- 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.
Addresses all three findings from the ultrareview of #127. Both substantive paths were confirmed by test before fixing.
The invariant was false on two more paths
My own PR description asked reviewers to hunt for holes in "the saver can neither add nor destroy". They found two.
Path A — MAX_ITEMS starvation, and silent. The restore admitted only on
count($out['items']) < MAX_ITEMS, so a crafted payload of 200 title-only junk entries filled every slot and left the protected rules nowhere to land. One POST from a saver with no authority to write per-user rules destroyed all of them, no UI involved.Path B — the DELETE endpoint bypassed the gate entirely.
Rest::reset_config()is gated oncapability(), notlist_users, andConfig::reset()wiped unconditionally. Every sanitize-side round lived on the POST path. A boundary enforced on save but not on reset is not a boundary.Collapsed rather than patched
Preserve-on-submit, restore-on-omit and merge-on-equivalent-key were three mechanisms doing one job, each added to cover a path the previous one missed — which is exactly why a fourth path still got through.
They are now one map, keyed by normalized slug, built before the payload loop by
protected_user_axes(), with item-cap capacity reserved for it. Starvation is impossible by construction rather than by another guard, and the stored total stays bounded byMAX_ITEMSexactly as before — abusive filler is what gets dropped, never an administrator's rules.Config::reset()uses the same map, so "Reset All" means "reset everything you can affect". An admin is unaffected: they holdlist_users, the map is empty, and it stays the plain wipe it always was. Asserted both ways.Both nits were real
resetItem()had drifted behindresetSelected()— four fields against six. The round-trip test passed vacuously, becausediffItem()short-circuits on absent keys, so it could never have caught the drift. It now seeds all four axes and asserts the returned shape, which is what actually pins the docblock's mirror-of-resetSelectedclaim..maestro-has-hiddenon Group 1's setSet testedhiddenRolesalone, so unchecking the last role while a person rule remained made the row look untouched until reload. Now matchesinitModel()and the user group.Gate
unit 167/167 (223) · integration 126/126 single-site (285) + 126/126 multisite (290) · JS 83/83 · e2e 47 passed / 0 failed · WPCS clean · PHPStan 0
Once this merges, #127 should be closed and
review-base/v1.4.1deleted — it exists only to host that review diff.🤖 Generated with Claude Code