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/.planning/compat/PRIOR-ART-admin-menu-editor.md b/.planning/compat/PRIOR-ART-admin-menu-editor.md index 521a26e..2885c44 100644 --- a/.planning/compat/PRIOR-ART-admin-menu-editor.md +++ b/.planning/compat/PRIOR-ART-admin-menu-editor.md @@ -99,6 +99,44 @@ 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 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 + 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..e366ae0 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 */ @@ -405,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 @@ -442,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'] ) ) { @@ -501,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. @@ -527,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/readme.txt b/readme.txt index ba93285..d65a532 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 @@ -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. @@ -128,15 +132,19 @@ 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: -* **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. * **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 == 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' ) ) );