From 500ba0c10a680b006c6509b3a9c1b17be6a0d720 Mon Sep 17 00:00:00 2001 From: Dan Knauss <273554+dknauss@users.noreply.github.com> Date: Tue, 11 Aug 2026 19:14:36 -0600 Subject: [PATCH 1/5] 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 --- .../compat/PRIOR-ART-admin-menu-editor.md | 30 +++++++++++++++++++ ...2026-08-10-ame-feature-surface-research.md | 20 +++++++++++++ includes/class-config.php | 7 +++-- 3 files changed, 55 insertions(+), 2 deletions(-) diff --git a/.planning/compat/PRIOR-ART-admin-menu-editor.md b/.planning/compat/PRIOR-ART-admin-menu-editor.md index 521a26e..00a64de 100644 --- a/.planning/compat/PRIOR-ART-admin-menu-editor.md +++ b/.planning/compat/PRIOR-ART-admin-menu-editor.md @@ -99,6 +99,36 @@ solved — or paid for the sharp corners of — most of Maestro's hard problems. drag-between-levels (reparenting), and per-role deny are all either Pro-gated or fragile in AME. Maestro's own backlog (`config-presets-export-import`, reparenting v2) lines up to undercut these for free. +- **D4 — Zero front-end footprint: one sparse, NON-AUTOLOADED row (added 2026-08-11).** The + storage-row consequence of V2, and a **key differentiator / USP** — the one Maestro claim that is + measurable by a third party (`wp option list --autoload=on`) without installing either plugin. + - **AME's row is autoloaded**, so every request the site serves — front end included, where the + data is never used — pays to fetch and unserialize it. Its size grows with the *whole* admin + menu (every plugin installed adds to the stored full tree) and with every role/user rule, not + with what the user edited. + - **This is acknowledged upstream, not inferred.** A wordpress.org thread reports `ws_menu_editor` + as the site's largest autoloaded row; Elsts confirms the autoload behavior and declines to flip + it because non-autoloaded "would mean an additional SQL query on every admin page", pointing to + compression instead ([support thread](https://wordpress.org/support/topic/wp_option-table-ws_menu_editor-auotload-yes/)). + Three separate size mitigations over eight years ([Pro changelog](https://adminmenueditor.com/documentation/changelog/)): + **2.5** (2017) compress-menu-config, **2.11** (2020) + zlib ("greatly decreases the amount of + data stored… but increases decompression overhead"), **2.27** (2025) "Optimize menu + configuration size" for the `ws_menu_editor_pro` entry. Three rounds ⇒ structural, not a bug. + - **Maestro's position:** one `maestro_config` row written `update_option( …, false )` + (`includes/class-config.php`), never created until first save; sparse delta so size tracks + *edits*, not installed-plugin count; hard 1 MB aggregate cap; every hook admin-gated (`Replay` + on `admin_menu`, `Admin_Bar::node()` bails on `! is_admin()`) so a front-end request reads + nothing at all. Measured: **0 extra front-end queries**; ~0.1 ms added per admin page at a + realistic 5–15 KB config, ~1 ms at the 1 MB ceiling + (`docs/performance/config-size-and-page-load.md`). + - **The trade, stated honestly:** non-autoloaded costs **1 extra admin-page query** — exactly + Elsts' objection. Maestro takes it because that query is admin-only, memoized once per request + by `Config::get()`, and zero with a persistent object cache, while public traffic — the bulk of + a live site's requests — pays nothing. AME's arrangement inverts the tax. + - **Evidence caveat:** the autoload flag comes from the author's own reply and the changelog, not + from the 1.15.1 source read (that spike recorded the storage *format*, not the autoload + argument). Confirm the `update_option` call when the free zip is next opened — the feature + sweep todo now carries that as a check. --- diff --git a/.planning/todos/pending/2026-08-10-ame-feature-surface-research.md b/.planning/todos/pending/2026-08-10-ame-feature-surface-research.md index 76d76e7..3eba5e0 100644 --- a/.planning/todos/pending/2026-08-10-ame-feature-surface-research.md +++ b/.planning/todos/pending/2026-08-10-ame-feature-surface-research.md @@ -86,6 +86,20 @@ The sweep is not done until each of these is enumerated and every item lands in 4. **The settings/UI surface** — what AME exposes where. Relevant because `DECISION-settings-surface.md` locked Maestro's answer and AME is the counter-example (a dedicated editor screen). +5. **The database-footprint features** — AME's Settings tab ships "Compress menu + configuration data" (2.5, zlib in 2.11) and "Optimize menu configuration size" + (2.27). These are *user-facing toggles*, so they are feature rows despite the + storage carve-out in Out of scope below, and they are the rows where Maestro's + **D4 differentiator** (single sparse non-autoloaded row — see + [`PRIOR-ART-admin-menu-editor.md`](../../compat/PRIOR-ART-admin-menu-editor.md) + § Differentiate) states itself: status `deliberately-not`, because a config that + is sparse and out of `alloptions` never needs a compression toggle. Expected to + be among the strongest `have`/`deliberately-not` rows in the matrix for + competitive-positioning purposes. + **Also settle the open evidence question while the zip is open:** confirm + whether `ws_menu_editor` is written with an autoload argument at all. D4 + currently rests on the author's forum reply plus the changelog, not on a source + read — grep the `update_option` call and record the answer as `source-read`. **Completion check** (mirroring SCHEMA.md's): every file in AME's `modules/` accounted for in at least one row; every bullet on the Pro pricing page accounted @@ -117,6 +131,12 @@ The architecture comparison — apply model, hook ordering, storage format, menu identity, submenu targeting — all answered 2026-08-01. If a feature row needs an architectural answer, note the dependency in its Note cell and move on. +**One carve-out: the options-table footprint is in scope** (Coverage item 5). The +storage *format* is architecture and stays out, but its consequences — an +autoloaded row that every front-end request pays for, and the two Settings-tab +toggles AME ships to manage it — are user-visible and are Maestro's D4 selling +point. They earn rows. + Other prior art is also out: "Wider Admin Menu" and "Hide Admin Menu" belong to `SPEC.md` items 9 and 10. AME-only keeps this bounded. diff --git a/includes/class-config.php b/includes/class-config.php index f5dbfcb..212c8b6 100644 --- a/includes/class-config.php +++ b/includes/class-config.php @@ -133,8 +133,11 @@ class Config { * under this — five 128 KB icons plus titles/roles is ~0.65 MB. The ceiling * exists only to refuse the pathological multi-MB payload (e.g. 200 max * items each carrying a 128 KB icon ≈ 25 MB) that would bloat the option - * and every autoloaded read of it. An over-ceiling save is rejected whole - * rather than truncated — a partial config is worse than the prior one. + * and every admin-request read of it. (The option is stored non-autoloaded — + * see save() — so it is never part of the `alloptions` bundle; the cost the + * ceiling bounds is the per-admin-request get_option + unserialize, not a + * front-end tax.) An over-ceiling save is rejected whole rather than + * truncated — a partial config is worse than the prior one. * * @var int */ From 608c226b89067ec480fdac8d2028e0c602c823ee Mon Sep 17 00:00:00 2001 From: Dan Knauss <273554+dknauss@users.noreply.github.com> Date: Tue, 11 Aug 2026 19:20:18 -0600 Subject: [PATCH 2/5] 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 --- .planning/compat/PRIOR-ART-admin-menu-editor.md | 16 ++++++++++++---- readme.txt | 12 +++++++----- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/.planning/compat/PRIOR-ART-admin-menu-editor.md b/.planning/compat/PRIOR-ART-admin-menu-editor.md index 00a64de..2885c44 100644 --- a/.planning/compat/PRIOR-ART-admin-menu-editor.md +++ b/.planning/compat/PRIOR-ART-admin-menu-editor.md @@ -102,10 +102,18 @@ solved — or paid for the sharp corners of — most of Maestro's hard problems. - **D4 — Zero front-end footprint: one sparse, NON-AUTOLOADED row (added 2026-08-11).** The storage-row consequence of V2, and a **key differentiator / USP** — the one Maestro claim that is measurable by a third party (`wp option list --autoload=on`) without installing either plugin. - - **AME's row is autoloaded**, so every request the site serves — front end included, where the - data is never used — pays to fetch and unserialize it. Its size grows with the *whole* admin - menu (every plugin installed adds to the stored full tree) and with every role/user rule, not - with what the user edited. + - **AME's row is autoloaded**, so its bytes ride in the `alloptions` bundle on every request that + boots WP — front end and logged-out included, where the data is never read. Its size grows with + the *whole* admin menu (every plugin installed adds to the stored full tree) and with every + role/user rule, not with what the user edited. + - **Where that cost actually lands** (state it precisely; the loose version invites a correction): + `wp_load_alloptions()` fetches every autoloaded row in one query and holds them in memory for + the request. The option's own `maybe_unserialize()` runs only when something calls + `get_option()` for it — so the front-end tax is the **bundle**, not that call: DB→PHP transfer, + memory, and — with a persistent object cache, where `alloptions` is a single cache object — a + per-request fetch and unserialize of the *whole* bundle, which is precisely where a fat row + hurts most. A full-page cache spares requests that never boot WP; a logged-in or otherwise + uncacheable front-end request pays in full. - **This is acknowledged upstream, not inferred.** A wordpress.org thread reports `ws_menu_editor` as the site's largest autoloaded row; Elsts confirms the autoload behavior and declines to flip it because non-autoloaded "would mean an additional SQL query on every admin page", pointing to diff --git a/readme.txt b/readme.txt index ba93285..2a95852 100644 --- a/readme.txt +++ b/readme.txt @@ -130,13 +130,15 @@ their current parent. Maestro is built to stay out of the way: -* **Zero extra database queries on the front end.** Every hook is admin-only, so the plugin is completely inert for public page loads and logged-out visitors. -* **One extra query on an admin page** — a single, *non-autoloaded* option (`maestro_config`), read once per request and cached. With a persistent object cache (Redis / Memcached) that drops to zero. -* **Nothing added to `alloptions`.** Because the option is not autoloaded, it adds no weight to the bundle WordPress loads on every request. +* **Nothing added to `alloptions` — the one that matters.** WordPress loads every *autoloaded* option into memory on every request that boots it, including front-end pages served to logged-out visitors. A menu configuration is admin-only data, so an autoloaded one is a tax on traffic that will never read it — and a bloated autoloaded bundle is a classic cause of a sluggish site. Maestro's option is explicitly non-autoloaded. **Check it on your own site:** `wp option list --autoload=on` will not list `maestro_config`. +* **Zero extra database queries on the front end.** Not merely "light" — inert. Every hook is admin-gated, so a public page load reads nothing at all. +* **One extra query on an admin page** — a single, *non-autoloaded* option (`maestro_config`), read once per request and cached, no matter how many parts of the plugin ask for it. With a persistent object cache (Redis / Memcached) that drops to zero. This is a deliberate trade: one query where the data is actually used, so that public traffic — the bulk of what a live site serves — pays nothing. +* **Storage tracks your edits, not your plugin count.** Maestro stores a sparse delta: three renamed items are three renamed items, whether the site runs five plugins or fifty. A typical configuration is 5–15 KB, with a hard 1 MB ceiling — small enough that there is nothing to compress, optimize, or tune. * **Minimal storage.** One `wp_options` row, created only when you first save a change — a fresh install stores nothing. No custom tables, no post or user meta, no transients, no cron jobs. Uninstalling deletes that single row. -* **Small install** — roughly a 115 KB download. Menu changes are applied in memory during the `admin_menu` pass, not through extra queries. +* **Measured, not asserted.** At a typical configuration size Maestro adds roughly **0.1 ms** to an admin page load; even a pathological config at the 1 MB ceiling adds about **1 ms**. Menu changes are applied in memory during the `admin_menu` pass, not through extra queries. Method and full numbers: [config size vs. page-load cost](https://github.com/dknauss/Maestro/blob/main/docs/performance/config-size-and-page-load.md). +* **Small install** — roughly a 115 KB download. -(Figures are a v1.4.1 snapshot.) +(Size figures are a v1.4.1 snapshot; timings were measured 2026-08-03 on WordPress 7.0 / PHP 8.3.) == Known limits / deferred to v2 == From 4c28f8bcfb6a211a99ff56c8f149e251b37ec78a Mon Sep 17 00:00:00 2001 From: Dan Knauss <273554+dknauss@users.noreply.github.com> Date: Tue, 11 Aug 2026 19:28:57 -0600 Subject: [PATCH 3/5] 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 --- readme.txt | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/readme.txt b/readme.txt index 2a95852..878f75a 100644 --- a/readme.txt +++ b/readme.txt @@ -1,7 +1,7 @@ === Maestro: The Inline Admin Menu Editor === Contributors: dpknauss Donate link: https://github.com/sponsors/dknauss -Tags: admin menu, admin menu editor, menu editor, hide menu items, menu icons +Tags: admin menu, menu editor, hide menu items, rename menu items, menu icons Requires at least: 6.4 Tested up to: 7.0 Stable tag: 1.5.1 @@ -128,12 +128,14 @@ their current parent. == Performance & footprint == +**Unlike other admin menu editors, Maestro won't hurt your site's performance. It's small enough that there is nothing to compress, optimize, or tune.** + Maestro is built to stay out of the way: * **Nothing added to `alloptions` — the one that matters.** WordPress loads every *autoloaded* option into memory on every request that boots it, including front-end pages served to logged-out visitors. A menu configuration is admin-only data, so an autoloaded one is a tax on traffic that will never read it — and a bloated autoloaded bundle is a classic cause of a sluggish site. Maestro's option is explicitly non-autoloaded. **Check it on your own site:** `wp option list --autoload=on` will not list `maestro_config`. * **Zero extra database queries on the front end.** Not merely "light" — inert. Every hook is admin-gated, so a public page load reads nothing at all. * **One extra query on an admin page** — a single, *non-autoloaded* option (`maestro_config`), read once per request and cached, no matter how many parts of the plugin ask for it. With a persistent object cache (Redis / Memcached) that drops to zero. This is a deliberate trade: one query where the data is actually used, so that public traffic — the bulk of what a live site serves — pays nothing. -* **Storage tracks your edits, not your plugin count.** Maestro stores a sparse delta: three renamed items are three renamed items, whether the site runs five plugins or fifty. A typical configuration is 5–15 KB, with a hard 1 MB ceiling — small enough that there is nothing to compress, optimize, or tune. +* **Storage tracks your edits, not your plugin count.** Maestro stores a sparse delta: three renamed items are three renamed items, whether the site runs five plugins or fifty. A typical configuration is 5–15 KB, with a hard 1 MB ceiling. * **Minimal storage.** One `wp_options` row, created only when you first save a change — a fresh install stores nothing. No custom tables, no post or user meta, no transients, no cron jobs. Uninstalling deletes that single row. * **Measured, not asserted.** At a typical configuration size Maestro adds roughly **0.1 ms** to an admin page load; even a pathological config at the 1 MB ceiling adds about **1 ms**. Menu changes are applied in memory during the `admin_menu` pass, not through extra queries. Method and full numbers: [config size vs. page-load cost](https://github.com/dknauss/Maestro/blob/main/docs/performance/config-size-and-page-load.md). * **Small install** — roughly a 115 KB download. From 5cfb75811e8f081a3d27c49105794b35f0284eb0 Mon Sep 17 00:00:00 2001 From: Dan Knauss <273554+dknauss@users.noreply.github.com> Date: Tue, 11 Aug 2026 19:32:19 -0600 Subject: [PATCH 4/5] 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 --- readme.txt | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/readme.txt b/readme.txt index 878f75a..d65a532 100644 --- a/readme.txt +++ b/readme.txt @@ -9,13 +9,17 @@ Requires PHP: 7.4 License: GPLv2 or later License URI: https://www.gnu.org/licenses/gpl-2.0.html -Rename, reorder, change icons, and hide WordPress admin menu items per user role — an inline admin menu editor you drive right on the menu itself. +Hide admin menu items per user role, rename them, reorder them, swap their icons — an inline admin menu editor you drive right on the menu itself. == Description == -**Maestro lets you orchestrate the appearance of the WordPress admin menu.** +**Maestro lets you orchestrate the appearance of the WordPress admin menu — rename admin menu items, reorder them, change their icons, and hide admin menu items from the roles and people who don't need them.** -Instead of a separate settings screen, Maestro turns the admin menu into something you edit *in place* — right where it lives. Toggle **Edit Menu** from the admin bar, and the menu becomes editable. Click a menu item to rename it, hide it from selected user roles, or change its icon. Drag submenu items and whole menu groups to reorder them. +Every WordPress install accumulates menu clutter. A dozen plugins each claim a top-level slot, submenus sit in an order nobody chose, and clients meet a dashboard full of tools that aren't theirs. Maestro is how you customize the admin menu back into shape: **declutter** it for a client site, **rename** cryptic plugin labels into the words your team actually uses, **reorder** the admin menu so what you touch daily is at the top, and **hide admin menu items per user role** — or from one named person — so everyone sees only their own work. + +Instead of a separate settings screen, Maestro turns the admin menu into something you edit *in place* — right where it lives. Toggle **Edit Menu** from the admin bar, and the menu becomes editable. Click a menu item to rename it, hide it from selected user roles, or change its icon. Drag submenu items and whole menu groups to reorder them. Nothing to configure first, and nothing to learn: the menu you are editing *is* the menu you are looking at. + +And it stays out of the way. Maestro adds **no autoloaded options** and does no work at all on the front end, so a menu you tidied for the dashboard costs your visitors nothing (see **Performance & footprint** below). **Try Maestro right here.** Launch a demo in [WordPress Playground](https://playground.wordpress.net/?blueprint-url=https://raw.githubusercontent.com/dknauss/Maestro/main/playground/blueprint-hosted.json) — it boots a throwaway site with the plugin active in edit mode, plus User Switching and test users (editor, author, contributor, subscriber; password `password`) so you can try per-role visibility by switching to another user's account. From ff22735ee32f5ed60e66567f0d15e2fd1bc37845 Mon Sep 17 00:00:00 2001 From: Dan Knauss <273554+dknauss@users.noreply.github.com> Date: Tue, 11 Aug 2026 19:41:37 -0600 Subject: [PATCH 5/5] fix(config): store one key per normalized slug, closing a fifth sanitize hole MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .planning/STATE.md | 42 ++++++++++----- includes/class-config.php | 42 ++++++++++++++- .../PerUserAxisAuthorizationTest.php | 52 +++++++++++++++++++ tests/integration/ReplayTest.php | 40 +++++++++++--- tests/integration/RestConfigTest.php | 43 +++++++++++++++ 5 files changed, 198 insertions(+), 21 deletions(-) diff --git a/.planning/STATE.md b/.planning/STATE.md index 96c5e9c..33ff375 100644 --- a/.planning/STATE.md +++ b/.planning/STATE.md @@ -62,9 +62,10 @@ settled. The gate earned its place; the hold was correct. ### Caveats that survive into the milestone record -- The #128 fixes are themselves unreviewed — the ultrareview ran against - `1a32f08`. Accepted deliberately: the collapse removed a class of seam rather - than adding another guard. +- ~~The #128 fixes are themselves unreviewed — the ultrareview ran against + `1a32f08`.~~ **CLOSED 2026-08-10.** Reviewed via #149 (review-only PR, base + pinned at `1a32f08`, so the diff was everything no adversarial pass had seen). + **It found a fifth hole in the same function** — see the entry below. - No human screen-reader pass on the person picker. axe is clean across empty and populated states, which is not the same thing. **Consolidated 2026-08-10 into `todos/pending/2026-08-10-person-picker-screen-reader-pass.md`** — see the @@ -199,10 +200,10 @@ not automatic and required a manual `workflow_dispatch`. Plan the step in. (`todos/pending/2026-08-02-cloned-role-hiding-profiles.md`) - Phase 22 (Playground demo) remains open. ~~Phase 25 (toolbar polish)~~ — ✅ COMPLETE 2026-08-09, SHIPPED in v1.5.1 -- The #128 fixes shipped unreviewed (the ultrareview ran against the prior commit). - **Still true as of v1.5.1** — the mechanism is intact in `main`, touched since - only by #138's 20-line entity-collision fix. Closing it needs `/code-review ultra` - over `1a32f08..707d9b6`, which is user-triggered. +- ~~The #128 fixes shipped unreviewed (the ultrareview ran against the prior + commit).~~ **CLOSED 2026-08-10** via #149 — which found a fifth hole in the same + sanitize path (two normalizing-equivalent payload keys). Fixed; see the + reassessment near the end of this file. - No human screen-reader pass on the person picker → consolidated into `todos/pending/2026-08-10-person-picker-screen-reader-pass.md` - ~~21-05 Task 5 (human browser verification) was never performed~~ — **STRUCK @@ -525,13 +526,26 @@ re-opening either question:** **withdrawn**: `160px` is just hardcoded in three places. **Reviewed 2026-08-10 — three carried items are now TWO, and both are actionable:** -1. **The #128 fixes shipped unreviewed.** 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 lines). It is the same - sanitize path in which the ultrareview found four consecutive holes, and it is - the one round no adversarial pass has ever seen. **Close with `/code-review - ultra` over `1a32f08..707d9b6`** — user-triggered and billed, so it needs Dan. - Highest value of the three, and the cheapest. +1. ~~**The #128 fixes shipped unreviewed.**~~ **CLOSED 2026-08-10 — and it was + worth doing.** The ultrareview ran against #149 (`1a32f08..main`) and returned + **one finding, confirmed real: a FIFTH hole in `Config::sanitize()`.** + + Two payload keys that normalize alike (`upload.php` + `upload.php?ver=1`) both + got stored: the first claimed and `unset()` the protected map entry, so the + second found no match and wrote itself as a fresh entry. Two stored keys + normalizing to one is exactly what Replay's Axis-1 guard resolves to "apply + nothing" — so a saver WITHOUT `list_users` could neutralise an administrator's + per-user rule with one POST, leaving it visibly present in storage and applying + nowhere. + + **The pattern held for a fifth time**: the seam was the `unset()` after the + first match, and the code comment directly above it claimed the merge existed + to prevent precisely this ambiguity — true only of the FIRST equivalent + spelling. Fixed by deduping at the point of write, unconditionally, so the + collision is unconstructible rather than blocked on one path. Reproduced with a + failing test first; three replay-side tests had to be re-seeded because + sanitize can no longer create the state they assert on (they still can, and + must, via legacy configs and slug drift — all three re-verified falsifiable). 2. **The screen-reader pass**, now a real todo rather than prose: `todos/pending/2026-08-10-person-picker-screen-reader-pass.md`. It absorbed item 3 and slightly widened — Phase 25's M2 change made a previously-skipped diff --git a/includes/class-config.php b/includes/class-config.php index 212c8b6..e366ae0 100644 --- a/includes/class-config.php +++ b/includes/class-config.php @@ -408,6 +408,11 @@ public function sanitize( array $raw ) { // than an administrator's rules. $item_budget = self::MAX_ITEMS - count( $protected ); + // Normalized keys already written to $out['items'] this pass. See the + // dedupe guard in the loop — this is what makes the sanitized output + // collision-free BY CONSTRUCTION rather than collision-detected later. + $emitted_norm = array(); + if ( ! empty( $raw['items'] ) && is_array( $raw['items'] ) ) { foreach ( $raw['items'] as $slug => $item ) { // Qualified `parent>child` submenu keys: clean each half @@ -445,6 +450,31 @@ public function sanitize( array $raw ) { $slug = $this->clean_slug( $slug ); } + // ONE stored key per NORMALIZED identity. + // + // Two payload keys that normalize alike (`upload.php` and + // `upload.php?ver=1`) would otherwise BOTH be stored, and the + // Axis-1 guard in Replay::normalized_items() resolves that + // ambiguity to "apply nothing" — dropping every override on that + // item, including a protected per-user rule merged in below. The + // config still looks healthy; the rule simply stops applying. + // + // That was reachable by a saver WITHOUT `list_users`: the protected + // map is claimed and unset on the first matching spelling, so a + // second equivalent spelling in the same payload found no match, + // stored itself as a fresh entry, and neutralised an administrator's + // rule with one POST. Deduping at the point of write closes it for + // every caller at once, rather than special-casing that path — the + // seam only existed because the collision was constructible at all. + // + // First spelling in incoming object order wins, matching the + // MAX_ITEMS break below. Recorded only when an entry is actually + // stored, so an empty first entry does not shadow a real later one. + $nk = Slug::normalize_qualified( (string) $slug, $base ); + if ( '' !== $nk && isset( $emitted_norm[ $nk ] ) ) { + continue; + } + $entry = array(); if ( isset( $item['title'] ) && '' !== trim( (string) $item['title'] ) ) { @@ -504,7 +534,14 @@ public function sanitize( array $raw ) { // item are ambiguous, and the Axis-1 guard resolves ambiguity // to "apply nothing", which would leave the rule stored but // inert while the config still looked healthy. - $nk = Slug::normalize_qualified( (string) $slug, $base ); + // + // That reasoning only ever covered the FIRST equivalent spelling: + // this branch claims and unsets the protected entry, so a second + // equivalent key in the same payload found no match and stored + // itself anyway — recreating the exact ambiguity the comment above + // says it prevents. The dedupe guard earlier in the loop is what + // actually closes it; $nk is computed once up there and reused + // here, since a second local copy is how the two would drift. if ( '' !== $nk && isset( $protected[ $nk ] ) ) { $entry = array_merge( $entry, $protected[ $nk ]['axes'] ); unset( $protected[ $nk ] ); // Claimed; do not re-add below. @@ -530,6 +567,9 @@ public function sanitize( array $raw ) { if ( $entry ) { $out['items'][ $slug ] = $entry; + if ( '' !== $nk ) { + $emitted_norm[ $nk ] = true; + } if ( count( $out['items'] ) >= $item_budget ) { break; // Deterministic: first N slugs in incoming object order win. } diff --git a/tests/integration/PerUserAxisAuthorizationTest.php b/tests/integration/PerUserAxisAuthorizationTest.php index 2b6db8c..1f443e2 100644 --- a/tests/integration/PerUserAxisAuthorizationTest.php +++ b/tests/integration/PerUserAxisAuthorizationTest.php @@ -153,6 +153,58 @@ public function test_delegate_save_with_a_normalizing_equivalent_key() { ); } + /** + * A8: TWO normalizing-equivalent keys in ONE payload. + * + * A5 covers a single equivalent spelling: it matches the protected map, + * merges, and claims the entry. The seam is what happens to the SECOND + * equivalent key in the same payload — the first iteration `unset()`s the + * protected entry, so the second finds no match, takes the no-match branch, + * and is stored under its own raw key. + * + * That leaves two stored keys normalizing to one item, which is precisely + * the ambiguity the Axis-1 guard resolves to "apply nothing" — so the + * admin's rule survives in storage and applies NOWHERE. Same outcome as + * outright deletion, reached by a saver with no authority to write it, and + * invisible to anyone auditing the stored config because the rule is still + * sitting there under `upload.php`. + * + * Found by ultrareview against #149, on the exact question the review was + * pointed at: can a crafted payload cause a protected rule to be dropped? + */ + public function test_delegate_cannot_neutralize_a_rule_with_two_equivalent_keys() { + $this->seed_admin_rule(); + wp_set_current_user( $this->delegate ); + + ( new Config() )->save( + array( + 'items' => array( + 'upload.php' => array( 'title' => 'Files1' ), + 'upload.php?ver=1' => array( 'title' => 'Files2' ), + ), + ) + ); + + $cfg = ( new Config() )->get(); + $items = $cfg['items']; + + global $menu, $submenu; + $menu = array( + 10 => array( 'Media', 'upload_files', 'upload.php', '', 'menu-top', 'menu-media', '' ), + ); + $submenu = array(); + + wp_set_current_user( $this->victim ); + ( new Replay( new Config() ) )->replay(); + + $this->assertNotContains( + 'upload.php', + wp_list_pluck( $menu, 2 ), + 'A8: two normalizing-equivalent keys in one payload must not be able to ' + . 'neutralise the rule. Stored keys: ' . implode( ', ', array_keys( $items ) ) + ); + } + /** * A6: MAX_ITEMS starvation. A crafted payload of MAX_ITEMS title-only junk * entries used to fill every slot, leaving the restore nowhere to land — so diff --git a/tests/integration/ReplayTest.php b/tests/integration/ReplayTest.php index 04b9811..dfb7d1f 100644 --- a/tests/integration/ReplayTest.php +++ b/tests/integration/ReplayTest.php @@ -554,9 +554,27 @@ public function test_collision_noop_ambiguous_stored_keys_apply_nothing() { ); // Store two DISTINCT keys that normalize to the same key. - ( new Config() )->save( + // + // Seeded DIRECTLY rather than through Config::save(), which since the + // two-equivalent-keys fix refuses to write an ambiguous pair at all + // (first spelling wins). That is the sanitize-side guarantee; this test + // pins the REPLAY-side one, which is still load-bearing because the + // ambiguity remains reachable by other routes: + // + // - configs written by any version BEFORE that fix, i.e. every install + // upgrading into it + // - slug drift: two keys that did NOT normalize alike when saved but do + // now, after a host move or a plugin changing its menu URL — the exact + // scenario Slug::normalize() exists for + // - direct DB or WP-CLI edits + // + // Seeding through save() would now assert sanitize's behaviour while + // claiming to assert replay's, and would go quietly vacuous. + update_option( + 'maestro_config', array( - 'items' => array( + 'schema_version' => Config::SCHEMA_VERSION, + 'items' => array( $slug_amp => array( 'title' => 'Ambiguous A' ), $slug_plain => array( 'title' => 'Ambiguous B' ), ), @@ -886,9 +904,14 @@ public function test_qualified_key_parent_half_miss_skips_silently() { public function test_axis1_guard_extends_to_qualified_keys() { $this->seed_shared_slug_menu(); - ( new Config() )->save( + // Seeded directly — Config::save() no longer writes an ambiguous pair. + // See test_collision_noop_ambiguous_stored_keys_apply_nothing() for why + // the replay-side guard still has to hold regardless. + update_option( + 'maestro_config', array( - 'items' => array( + 'schema_version' => Config::SCHEMA_VERSION, + 'items' => array( 'edit.php?post_type=product>edit.php?post_type=product&ver=1.0' => array( 'title' => 'Ambiguous A' ), 'edit.php?post_type=product>edit.php?post_type=product&ver=2.0' => array( 'title' => 'Ambiguous B' ), ), @@ -1594,9 +1617,14 @@ public function test_child_hidden_roles_does_not_fire_on_ambiguous_parent() { // Two DISTINCT stored keys collide to one normalized parent key (Axis-1 // norm_skip). One carries child_hidden_roles for the current (editor) user; // because the parent override is ambiguous, child-hiding must NOT fire. - ( new Config() )->save( + // Seeded directly — Config::save() no longer writes an ambiguous pair. + // See test_collision_noop_ambiguous_stored_keys_apply_nothing() for why + // the replay-side guard still has to hold regardless. + update_option( + 'maestro_config', array( - 'items' => array( + 'schema_version' => Config::SCHEMA_VERSION, + 'items' => array( $slug_amp => array( 'child_hidden_roles' => array( 'editor' ) ), $slug_plain => array( 'title' => 'Ambiguous' ), ), diff --git a/tests/integration/RestConfigTest.php b/tests/integration/RestConfigTest.php index 018a241..7600170 100644 --- a/tests/integration/RestConfigTest.php +++ b/tests/integration/RestConfigTest.php @@ -198,6 +198,49 @@ public function test_save_sanitizes_bad_icon_and_unknown_role() { $this->assertSame( 'Bold Posts', $item['title'], 'Markup must be stripped from titles.' ); } + /** + * Sanitize stores ONE key per normalized identity. + * + * Two spellings of the same slug used to be stored as two entries, and + * Replay's Axis-1 guard then resolved that ambiguity to "apply nothing" — + * so a config that looked healthy silently applied none of its own + * overrides for that item. + * + * This is the ordinary-admin half of the fix. The adversarial half, where a + * saver without `list_users` used the same collision to neutralise an + * administrator's per-user rule, is A8 in PerUserAxisAuthorizationTest. + * Deduping unconditionally is what makes that attack unconstructible rather + * than merely blocked on one path. + */ + public function test_save_stores_one_key_per_normalized_slug() { + wp_set_current_user( self::factory()->user->create( array( 'role' => 'administrator' ) ) ); + + $req = new WP_REST_Request( 'POST', self::ROUTE ); + $req->set_param( + 'config', + array( + 'items' => array( + 'upload.php' => array( 'title' => 'Files One' ), + 'upload.php?ver=1' => array( 'title' => 'Files Two' ), + ), + ) + ); + + $items = $this->server->dispatch( $req )->get_data()['config']['items']; + + $this->assertCount( + 1, + $items, + 'Two normalizing-equivalent keys must collapse to one stored entry. Got: ' + . implode( ', ', array_keys( $items ) ) + ); + $this->assertSame( + array( 'upload.php' => array( 'title' => 'Files One' ) ), + $items, + 'First spelling in incoming object order wins, matching the MAX_ITEMS rule.' + ); + } + public function test_save_accepts_all_native_icon_forms() { wp_set_current_user( self::factory()->user->create( array( 'role' => 'administrator' ) ) );