diff --git a/TESTING.md b/TESTING.md index fae9627..02542eb 100644 --- a/TESTING.md +++ b/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) diff --git a/assets/maestro-logic.js b/assets/maestro-logic.js index 9f96529..f56a01d 100644 --- a/assets/maestro-logic.js +++ b/assets/maestro-logic.js @@ -133,15 +133,23 @@ 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 = { @@ -149,6 +157,8 @@ function resetItem( item, pristine, isSub ) { hiddenRoles: [], icon: isSub ? '' : ( pristine.icon || '' ), childHiddenRoles: [], + hiddenUsers: [], + childHiddenUsers: [], }; return result; } diff --git a/assets/maestro.js b/assets/maestro.js index a3210cd..47c7944 100644 --- a/assets/maestro.js +++ b/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 + ); } }, { diff --git a/includes/class-config.php b/includes/class-config.php index 5a944ac..7d8e7b4 100644 --- a/includes/class-config.php +++ b/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 + */ + 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'] ) ) { diff --git a/tests/integration/PerUserAxisAuthorizationTest.php b/tests/integration/PerUserAxisAuthorizationTest.php index 85f96b1..2b6db8c 100644 --- a/tests/integration/PerUserAxisAuthorizationTest.php +++ b/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(); diff --git a/tests/js/reset-item.test.mjs b/tests/js/reset-item.test.mjs index 5748f95..12ab52e 100644 --- a/tests/js/reset-item.test.mjs +++ b/tests/js/reset-item.test.mjs @@ -63,13 +63,34 @@ test( 'resetItem does not mutate the input item', () => { } ); // Round-trip invariant: diffItem(resetItem(item), pristine) === modified:false +// +// The seed carries ALL FOUR visibility axes deliberately. diffItem() checks +// `current. && current..length > 0`, so an axis MISSING from the +// reset result short-circuits to false and the round-trip passes vacuously — +// which is exactly how resetItem() silently drifted behind resetSelected() when +// the user axes were added. Seeding every axis makes the round-trip able to +// fail if a future axis is added here and forgotten there. test( 'round-trip: top-level item after reset is not modified per diffItem', () => { - const item = { title: 'My Posts', icon: 'dashicons-admin-home', hiddenRoles: [ 'editor' ] }; + const item = { + title: 'My Posts', + icon: 'dashicons-admin-home', + hiddenRoles: [ 'editor' ], + childHiddenRoles: [ 'author' ], + hiddenUsers: [ { id: 4, name: 'Ada' } ], + childHiddenUsers: [ { id: 9, name: 'Grace' } ], + }; const pristine = { title: 'Posts', icon: 'dashicons-admin-post' }; const reset = resetItem( item, pristine, false ); const diff = diffItem( reset, pristine ); assert.equal( diff.modified, false ); assert.deepEqual( diff.fields, [] ); + + // Assert the SHAPE too, not just the diff verdict: the diff would stay clean + // if an axis were dropped entirely, so shape is what actually pins the + // mirror-of-resetSelected claim in the docblock. + assert.deepEqual( Object.keys( reset ).sort(), [ + 'childHiddenRoles', 'childHiddenUsers', 'hiddenRoles', 'hiddenUsers', 'icon', 'title', + ] ); } ); test( 'round-trip: submenu item after reset is not modified per diffItem', () => {