fix: address ultrareview findings — collapse the per-user authorization path

TESTING.md

@@ -2,7 +2,7 @@
 
 Three layers, smallest and fastest first.
 
-> **Current expected status:** unit 167/167 with 223 assertions, integration 123/123 with 277 assertions single-site + 123/123 with 282 under WP_MULTISITE=1, JavaScript unit tests, phpcs, PHPStan, Plugin Check, and the Playwright E2E suite should pass before release. E2E coverage includes reset-this-item, per-role visibility, icon persistence, keyboard reordering, first-run cues, toolbar accessibility checks, (COMPAT-04) independent shared-slug top-level/submenu editing, (COMPAT-10) the "Hide its sub-items from:" role group's independent child-hiding behavior, and (ROLE-02) per-user hiding asserted in a targeted user's own authenticated session — including a same-role bystander who must keep every row, which is the assertion that proves the rule did not widen to the whole role.
+> **Current expected status:** unit 167/167 with 223 assertions, integration 126/126 with 285 assertions single-site + 126/126 with 290 under WP_MULTISITE=1, JavaScript unit tests, phpcs, PHPStan, Plugin Check, and the Playwright E2E suite should pass before release. E2E coverage includes reset-this-item, per-role visibility, icon persistence, keyboard reordering, first-run cues, toolbar accessibility checks, (COMPAT-04) independent shared-slug top-level/submenu editing, (COMPAT-10) the "Hide its sub-items from:" role group's independent child-hiding behavior, and (ROLE-02) per-user hiding asserted in a targeted user's own authenticated session — including a same-role bystander who must keep every row, which is the assertion that proves the rule did not widen to the whole role.
 
 ## Gotchas (first run)
 
@@ -99,7 +99,7 @@ assert all three halves of the rule: a network super admin is exempt from the
 person axis, an ordinary user on the same network is **not**, and the role axis
 still applies to super admins (the documented asymmetry). They `markTestSkipped`
 on single-site, and the single-site-only self-target case skips under multisite,
-so both lanes report 123 tests. CI runs both.
+so both lanes report 126 tests. CI runs both.
 
 ## 3. End-to-end (Playwright, against live WordPress)
 

assets/maestro-logic.js

@@ -133,22 +133,32 @@ function modeStatusLabel( state, strings ) {
  *   m.hiddenRoles      = [];
  *   if ( ! m.isSub ) m.icon = def.icon || '';
  *   m.childHiddenRoles = [];
+ *   m.hiddenUsers      = [];
+ *   m.childHiddenUsers = [];
  *
- * childHiddenRoles (COMPAT-10 REVISED) has no WP-native pristine state —
- * reset always clears it to [], top-level or submenu (the field is simply
+ * None of the four visibility axes has a WP-native pristine state — reset
+ * always clears them to [], top-level or submenu (the child_* fields are simply
  * unused for submenu items).
  *
- * @param {{ title: string, icon?: string, hiddenRoles: string[], childHiddenRoles?: string[] }} item     Current item state.
+ * KEEP THIS IN STEP WITH resetSelected(). The docblock above claims to mirror
+ * it, and that claim is the only thing making this the readable spec for the
+ * operation — production resets inline, so nothing here fails when the two
+ * drift. The round-trip test does not catch it either: diffItem() short-circuits
+ * on ABSENT keys, so a reset result missing an axis passes vacuously.
+ *
+ * @param {{ title: string, icon?: string, hiddenRoles: string[], childHiddenRoles?: string[], hiddenUsers?: object[], childHiddenUsers?: object[] }} item     Current item state.
  * @param {{ title: string, icon?: string }}                        pristine Pristine default.
  * @param {boolean}                                                 isSub    True for submenu items.
- * @return {{ title: string, hiddenRoles: string[], icon: string, childHiddenRoles: string[] }}
+ * @return {{ title: string, hiddenRoles: string[], icon: string, childHiddenRoles: string[], hiddenUsers: object[], childHiddenUsers: object[] }}
  */
 function resetItem( item, pristine, isSub ) {
 	var result = {
 		title:            pristine.title || '',
 		hiddenRoles:      [],
 		icon:             isSub ? '' : ( pristine.icon || '' ),
 		childHiddenRoles: [],
+		hiddenUsers:      [],
+		childHiddenUsers: [],
 	};
 	return result;
 }

assets/maestro.js

@@ -1122,7 +1122,15 @@
 				model[ slug ].hiddenRoles = arr;
 				var li = liForKey( slug );
 				if ( li ) {
-					li.classList.toggle( 'maestro-has-hidden', model[ slug ].hiddenRoles.length > 0 );
+					// The marker means "this row has a hide rule of its own", which
+					// is EITHER axis. Testing hiddenRoles alone would clear it when
+					// the last role is unchecked while a person rule is still in
+					// force — the row would look untouched until a reload rebuilt
+					// it. Matches initModel() and the user group's own setSet.
+					li.classList.toggle(
+						'maestro-has-hidden',
+						model[ slug ].hiddenRoles.length > 0 || ( model[ slug ].hiddenUsers || [] ).length > 0
+					);
 				}
 			},
 			{

includes/class-config.php

@@ -222,10 +222,91 @@ public function save( array $raw ) {
 	 * @return void
 	 */
 	public function reset() {
+		// ROLE-02 — "Reset All" is still gated only by capability(), so a
+		// delegated editor without `list_users` can reach it. Wiping wholesale
+		// would let them destroy per-user rules they are not permitted to write,
+		// which is the same authorization boundary sanitize() enforces on save —
+		// and a boundary that holds on one endpoint and not the other is not a
+		// boundary. Reset All therefore means "reset everything you can affect".
+		//
+		// An administrator is unaffected: they hold `list_users`, so $protected is
+		// empty and this is the plain wipe it has always been.
+		$protected = current_user_can( 'list_users' )
+			? array()
+			: $this->protected_user_axes( function_exists( 'admin_url' ) ? admin_url( '' ) : '' );
+
+		if ( $protected ) {
+			$items = array();
+			foreach ( $protected as $entry ) {
+				$items[ $entry['slug'] ] = $entry['axes'];
+			}
+			update_option(
+				MAESTRO_OPTION,
+				array(
+					'schema_version' => self::SCHEMA_VERSION,
+					'items'          => $items,
+					'top_order'      => array(),
+					'sub_order'      => array(),
+				),
+				false
+			);
+			$this->cache = null;
+			return;
+		}
+
 		delete_option( MAESTRO_OPTION );
 		$this->cache = array();
 	}
 
+	/**
+	 * Stored per-user axes the CURRENT user has no authority to write, indexed by
+	 * normalized slug. (ROLE-02)
+	 *
+	 * Returns `[ normalized_key => [ 'slug' => stored_slug, 'axes' => [...] ] ]`,
+	 * carrying the original slug so a restored entry lands under the key it was
+	 * stored with rather than a canonicalised one.
+	 *
+	 * Indexed by NORMALIZED key on purpose: a payload may name the same item in
+	 * an equivalent-but-different form (`upload.php?ver=9` for a rule stored as
+	 * `upload.php`), which is the expected state after a plugin bumps a cache
+	 * buster — slug drift is precisely what Slug::normalize() exists for. Keying
+	 * raw would let both spellings survive into storage, and two keys normalizing
+	 * to one item are ambiguous, which the Axis-1 guard resolves to "apply
+	 * nothing": the rule would be present but inert while the config looked fine.
+	 *
+	 * @param string $base Admin base for Slug::normalize_qualified().
+	 * @return array<string, array{slug: string, axes: array}>
+	 */
+	private function protected_user_axes( $base ) {
+		$existing = $this->get();
+		$items    = isset( $existing['items'] ) ? $existing['items'] : array();
+		$out      = array();
+
+		foreach ( $items as $slug => $entry ) {
+			$axes = array();
+			if ( ! empty( $entry['hidden_users'] ) ) {
+				$axes['hidden_users'] = $entry['hidden_users'];
+			}
+			if ( ! empty( $entry['child_hidden_users'] ) ) {
+				$axes['child_hidden_users'] = $entry['child_hidden_users'];
+			}
+			if ( ! $axes ) {
+				continue;
+			}
+
+			$nk = Slug::normalize_qualified( (string) $slug, $base );
+			if ( '' === $nk || isset( $out[ $nk ] ) ) {
+				continue; // Unresolvable or already claimed by an earlier key.
+			}
+			$out[ $nk ] = array(
+				'slug' => $slug,
+				'axes' => $axes,
+			);
+		}
+
+		return $out;
+	}
+
 	/**
 	 * Whether the STORED config predates qualified submenu keys.
 	 *
@@ -294,12 +375,17 @@ public function sanitize( array $raw ) {
 		// autosave is full-replace, so silently dropping the axes would let any
 		// unrelated edit wipe an administrator's per-user rules. Stored values
 		// are preserved verbatim instead — the saver can neither add nor destroy.
+		//
+		// The protection is ONE mechanism, deliberately. It was three (preserve
+		// the submitted item, restore the omitted one, merge an equivalent key)
+		// and each was added to patch a path the previous one missed — which is
+		// exactly why a fourth path (starving the item cap with junk entries)
+		// still got through. A single map, keyed by NORMALIZED slug and given
+		// reserved capacity below, has no seams for the next path to slip
+		// between.
+		$base             = function_exists( 'admin_url' ) ? admin_url( '' ) : '';
 		$can_target_users = current_user_can( 'list_users' );
-		$stored_items     = array();
-		if ( ! $can_target_users ) {
-			$existing     = $this->get();
-			$stored_items = isset( $existing['items'] ) ? $existing['items'] : array();
-		}
+		$protected        = $can_target_users ? array() : $this->protected_user_axes( $base );
 
 		// Validate the incoming IDs with ONE bounded query rather than loading
 		// every user ID on the site. The payload caps each axis at
@@ -310,6 +396,15 @@ public function sanitize( array $raw ) {
 			? $this->valid_user_ids( self::candidate_user_ids( $raw ) )
 			: array();
 
+		// RESERVE capacity for the protected rules before the payload loop runs.
+		// Without this, a payload of MAX_ITEMS junk entries fills every slot and
+		// the protected rules have nowhere to land — silent destruction by a
+		// saver with no authority to write them, achieved with a single crafted
+		// POST. Reserving keeps the stored total bounded by MAX_ITEMS exactly as
+		// before, and simply means abusive filler is what gets dropped rather
+		// than an administrator's rules.
+		$item_budget = self::MAX_ITEMS - count( $protected );
+
 		if ( ! empty( $raw['items'] ) && is_array( $raw['items'] ) ) {
 			foreach ( $raw['items'] as $slug => $item ) {
 				// Qualified `parent>child` submenu keys: clean each half
@@ -382,14 +477,18 @@ public function sanitize( array $raw ) {
 				// scopes (bare top-level and qualified submenu), exactly as
 				// hidden_roles is — only the child_* axis below is parent-only.
 				if ( ! $can_target_users ) {
-					// Saver cannot target users: carry any stored axes through
-					// untouched so an unrelated edit cannot destroy them, and
-					// ignore whatever the payload claims.
-					if ( ! empty( $stored_items[ $slug ]['hidden_users'] ) ) {
-						$entry['hidden_users'] = $stored_items[ $slug ]['hidden_users'];
-					}
-					if ( ! $is_qualified && ! empty( $stored_items[ $slug ]['child_hidden_users'] ) ) {
-						$entry['child_hidden_users'] = $stored_items[ $slug ]['child_hidden_users'];
+					// Saver cannot target users: attach whatever was stored for
+					// this item and ignore what the payload claims. Matched on the
+					// NORMALIZED key so an equivalent spelling of the same slug
+					// (`upload.php?ver=9` vs `upload.php`) lands here rather than
+					// producing a second entry later — two keys normalizing to one
+					// 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 );
+					if ( '' !== $nk && isset( $protected[ $nk ] ) ) {
+						$entry = array_merge( $entry, $protected[ $nk ]['axes'] );
+						unset( $protected[ $nk ] ); // Claimed; do not re-add below.
 					}
 				} else {
 					if ( ! empty( $item['hidden_users'] ) && is_array( $item['hidden_users'] ) ) {
@@ -412,73 +511,18 @@ public function sanitize( array $raw ) {
 
 				if ( $entry ) {
 					$out['items'][ $slug ] = $entry;
-					if ( count( $out['items'] ) >= self::MAX_ITEMS ) {
+					if ( count( $out['items'] ) >= $item_budget ) {
 						break; // Deterministic: first N slugs in incoming object order win.
 					}
 				}
 			}
 		}
 
-		// ROLE-02 — restore per-user axes the saver was not allowed to touch.
-		//
-		// The per-item preserve above only fires for items PRESENT in the
-		// payload, which is not enough. get_menu_model() withholds the user axes
-		// from a saver without `list_users`, so diffItem() never flags them
-		// client-side, so an item whose ONLY override is a per-user rule is
-		// omitted from that saver's full-replace autosave entirely — and a rule
-		// omitted from a full replace is a rule deleted. That made any edit by a
-		// delegated editor silently wipe every per-user rule they could not see:
-		// ordinary data loss, not an exotic hand-crafted POST.
-		//
-		// So the restore runs over the STORED items, not the submitted ones.
-		if ( ! $can_target_users ) {
-			// Index what we are about to write by NORMALIZED key. Matching on the
-			// raw key is not enough: the payload may name the same item in a
-			// different-but-equivalent form (`upload.php?ver=9` for a rule stored
-			// under `upload.php`), which is not contrived — slug drift is the
-			// exact problem Slug::normalize() exists to solve, so it is the
-			// expected state after a plugin update. Re-attaching under the stored
-			// key would then leave TWO keys normalizing to the same item, which
-			// the Axis-1 collision guard resolves to "apply nothing" — silently
-			// neutralising both the preserved rule and the saver's own edit while
-			// the stored config still looks healthy.
-			$base     = function_exists( 'admin_url' ) ? admin_url( '' ) : '';
-			$norm_out = array();
-			foreach ( array_keys( $out['items'] ) as $written_key ) {
-				$nk = Slug::normalize_qualified( (string) $written_key, $base );
-				if ( '' !== $nk && ! isset( $norm_out[ $nk ] ) ) {
-					$norm_out[ $nk ] = $written_key;
-				}
-			}
-
-			foreach ( $stored_items as $stored_slug => $stored_entry ) {
-				$keep = array();
-				if ( ! empty( $stored_entry['hidden_users'] ) ) {
-					$keep['hidden_users'] = $stored_entry['hidden_users'];
-				}
-				if ( ! empty( $stored_entry['child_hidden_users'] ) ) {
-					$keep['child_hidden_users'] = $stored_entry['child_hidden_users'];
-				}
-				if ( ! $keep ) {
-					continue;
-				}
-
-				$nk = Slug::normalize_qualified( (string) $stored_slug, $base );
-
-				if ( '' !== $nk && isset( $norm_out[ $nk ] ) ) {
-					// Same item under either spelling — merge into the key that is
-					// actually being written, never add a second one.
-					$target                  = $norm_out[ $nk ];
-					$out['items'][ $target ] = array_merge( $out['items'][ $target ], $keep );
-				} elseif ( count( $out['items'] ) < self::MAX_ITEMS ) {
-					// Nothing equivalent is being written; the item exists only to
-					// hold the rule, so re-create it rather than let it vanish.
-					$out['items'][ $stored_slug ] = $keep;
-					if ( '' !== $nk ) {
-						$norm_out[ $nk ] = $stored_slug;
-					}
-				}
-			}
+		// Anything still unclaimed names an item the payload never mentioned (in
+		// any spelling). Its entry exists only to hold the rule, so re-create it.
+		// The budget reserved above guarantees room.
+		foreach ( $protected as $unclaimed ) {
+			$out['items'][ $unclaimed['slug'] ] = $unclaimed['axes'];
 		}
 
 		if ( ! empty( $raw['top_order'] ) && is_array( $raw['top_order'] ) ) {

tests/integration/PerUserAxisAuthorizationTest.php

@@ -153,6 +153,71 @@ public function test_delegate_save_with_a_normalizing_equivalent_key() {
 		);
 	}
 
+	/**
+	 * 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
+	 * a single POST from a saver with no authority to write per-user rules
+	 * silently destroyed all of them. Capacity is now reserved BEFORE the payload
+	 * loop, so filler is what gets dropped, never an admin's rules.
+	 */
+	public function test_delegate_cannot_starve_the_item_cap_to_destroy_rules() {
+		$this->seed_admin_rule();
+		wp_set_current_user( $this->delegate );
+
+		$junk = array();
+		for ( $i = 1; $i <= Config::MAX_ITEMS; $i++ ) {
+			$junk[ "junk-$i.php" ] = array( 'title' => 'x' );
+		}
+
+		( new Config() )->save( array( 'items' => $junk ) );
+
+		$cfg = ( new Config() )->get();
+		$this->assertSame(
+			array( $this->victim ),
+			$cfg['items']['upload.php']['hidden_users'] ?? null,
+			'A6: a full-cap junk payload must not be able to evict a protected rule'
+		);
+		$this->assertLessThanOrEqual(
+			Config::MAX_ITEMS,
+			count( $cfg['items'] ),
+			'A6: reserving capacity must not push the stored config past MAX_ITEMS'
+		);
+	}
+
+	/**
+	 * A7: the DELETE endpoint. "Reset All" is gated only on capability(), so a
+	 * delegate can reach it — and an unconditional wipe would destroy per-user
+	 * rules they cannot write. A boundary enforced on save but not on reset is
+	 * not a boundary. Reset All now means "reset everything you can affect".
+	 */
+	public function test_delegate_reset_all_cannot_destroy_protected_rules() {
+		$this->seed_admin_rule();
+		wp_set_current_user( $this->delegate );
+
+		( new Config() )->reset();
+
+		$cfg = ( new Config() )->get();
+		$this->assertSame(
+			array( $this->victim ),
+			$cfg['items']['upload.php']['hidden_users'] ?? null,
+			'A7: a delegate resetting must not destroy rules they cannot author'
+		);
+	}
+
+	/**
+	 * …but an ADMIN's Reset All is still a full wipe. The preservation is an
+	 * authorization boundary, not a new "sticky rules" behaviour — if it applied
+	 * to admins too, Reset All would stop meaning what it says.
+	 */
+	public function test_admin_reset_all_still_wipes_everything() {
+		$this->seed_admin_rule();
+		wp_set_current_user( $this->admin );
+
+		( new Config() )->reset();
+
+		$this->assertSame( array(), ( new Config() )->get(), 'A7b: an admin reset must still clear everything' );
+	}
+
 	/** A4: does the model leak display names to a delegate? Expected: no. */
 	public function test_model_does_not_leak_display_names_to_delegate() {
 		$this->seed_admin_rule();