fix(config): store one key per normalized slug, closing a fifth sanitize hole - #153
Merged
Merged
Conversation
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>
…load 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>
…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>
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>
…ize 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>
dknauss
added a commit
that referenced
this pull request
Aug 12, 2026
Version strings bumped via bin/prep-release.sh. Changelog derived from the `v1.5.1..main` DIFF, which is one shippable file: includes/class-config.php (+46/-3), the normalizing-equivalent-key dedupe from #153.⚠️ DO NOT TAG YET. A second ultrareview is still running in another session and may confirm further bugs. If it does, they belong in THIS patch: tagging now would either ship a fix whose changelog omits its siblings, or force a v1.5.3 days later. v1.4.0's gate 8 caught a changelog overclaim by diffing the real commits — the same discipline says do not finalise the changelog until the commit set is. Two changelog entries, both from the one fix, split because they are different audiences: - The general one: an override could silently stop applying when a single save carried two spellings of the same item's address. Written to name the symptom a user would actually notice — a rename or visibility rule that stops working while still showing in settings — rather than the mechanism. - The authorization one: it also closes a way for a saver who cannot list users to undo an administrator's per-person rule. Stated plainly, with the cosmetic-only guarantee restated, because that is the claim people will check. NOT framed as a security release. Hiding is cosmetic: no capability was granted or removed and the page stayed URL-reachable throughout. Same framing as 1.4.1. Also riding along, not changelog material: the readme listing copy rewritten in #152 (D4 non-autoloaded differentiator, tags, description). It publishes with the release but is directory copy, not plugin behaviour. Upgrade Notice is 271 chars (Plugin Check limit 300). Gate (Docker-free lanes): unit 167/167 (223 assertions), WPCS clean, PHPStan 0, doc-links clean. Integration/e2e ran green on #153 and re-run here in CI. 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.
The ultrareview against #149 returned one finding. It is real, and it is a fifth hole in
Config::sanitize().The bug
Two payload keys that normalize alike —
upload.phpandupload.php?ver=1— were both stored.The protected-map merge matches on the normalized key, claims the entry, and
unset()s 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 under its own raw key.Two stored keys normalizing to one is precisely the ambiguity
Replay::normalized_items()resolves to "apply nothing". Result: a saver withoutlist_userscould neutralize 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.I verified both halves of the trace against the code before touching anything: the
unset()atclass-config.php:510, the store-under-raw-slug at:532, and the drop atclass-replay.php:604-606.Why four prior rounds missed it
The comment directly above the merge already argued this couldn't 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 sends 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 only on the no-
list_userspath. The attack existed because the collision was constructible at all; blocking it on one path leaves the same seam for a sixth round. First spelling in incoming object order wins, matching the existing MAX_ITEMS rule. The key is recorded only when an entry is actually stored, so an empty first entry can't shadow a real later one.Three replay tests re-seeded — read this part
test_collision_noop_ambiguous_stored_keys_apply_nothing,test_axis1_guard_extends_to_qualified_keysandtest_child_hidden_roles_does_not_fire_on_ambiguous_parentbuilt their fixtures by saving an ambiguous pair, which sanitize now refuses to write. They seed viaupdate_option()instead.The replay-side guard is untouched and still load-bearing. The ambiguity stays reachable from:
Seeding through
save()would now assert sanitize's behaviour while claiming to assert replay's, and would go quietly vacuous.All three re-verified falsifiable: replacing the guard with first-wins fails all three. Worth recording — my first probe used last-wins and only failed two, because the third fixture's entry order lets last-wins mask it. That shape of probe can't be trusted here.
Tests
PerUserAxisAuthorizationTest)test_save_stores_one_key_per_normalized_slug(RestConfigTest)Gate
integration 129/129 single-site (291 assertions) + 129/129 multisite (296), unit 167/167 (223), JS 83/83, WPCS clean, PHPStan 0.
Not a security release
Hiding is cosmetic, so this cannot widen or revoke access — the affected page was always URL-reachable for anyone holding its capability. It's an authorization-boundary correctness bug: a delegate could undo an admin's intent. Same framing as 1.4.1.
STATE.md updated to close the "#128 unreviewed" gap and record that the review found a fifth hole.
🤖 Generated with Claude Code