Repository navigation
Multisite: Require delete_sites capability to delete a site via REST - #5
Merged
lloc merged 5 commits intoSep 30, 2026
Conversation
… refine sites permission checks. `WP_REST_Sites_Controller::delete_item_permissions_check()` only checked the `delete_site` meta capability, which core maps unconditionally to `manage_options` and is not scoped to the target site. This let any per-site Administrator delete an arbitrary site network-wide, unlike core's own network admin Sites screen and this controller's own create/update checks, which correctly require the super-admin-only `create_sites`/`manage_sites` capabilities. - Require `delete_sites` (in addition to the existing `delete_site` meta cap check) before a site can be deleted, matching the layering `wp-admin/network/sites.php` already uses. - Return a specific `rest_cannot_create` error from `create_item_permissions_check()` instead of a bare boolean, matching `WP_REST_Posts_Controller` and this class's own `rest_cannot_edit`/ `rest_cannot_delete` errors. - Extract `check_multisite_support()`, `check_delete_permission()`, and reuse `check_edit_permission()` to de-duplicate capability checks that were repeated across every permission-check method. - `get_item_permissions_check()`: a site's default/view-context data is now readable by members of that site (or a super admin), not the general public; other contexts still require `manage_sites`. - Add regression tests for the capability gap, the multisite-check ordering, and the view/edit context permission boundaries. Props spacedmonkey. Fixes #40365. Co-Authored-By: Claude Sonnet 5 <[email protected]>
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
…ller.php. phpcs flagged 34 WordPress.Arrays.MultipleStatementAlignment.DoubleArrowNotAligned violations across the new test methods, auto-fixed via phpcbf. No behavioral change. Co-Authored-By: Claude Sonnet 5 <[email protected]>
…default. The REST API index schema fixture was missing "default": 1 on the sites endpoint's `network` arg (both the create and update route args), which current_user_can()/get_current_network_id() output but the committed fixture predates. Pre-existing drift on this branch, unrelated to the permission-check changes in this PR, caught by CI's generated-file diff check. Co-Authored-By: Claude Sonnet 5 <[email protected]>
…s REST controller. A wrapido-based manual QA pass against 4aa1947 (which fixed the network-wide-delete and view-context info-disclosure bugs) surfaced four smaller inconsistencies introduced or left by that refactor: - `get_item_permissions_check()`'s membership bypass only matched the literal `view` context, so a site member got 200 on the default view context but 403 on `context=embed` - stricter than `view` even though embed is normally a strict subset of it. Now also allows `embed`. - `get_items_permissions_check()`'s fallback error said "not allowed to edit sites" for what is a read (GET) endpoint - copy-pasted from the manage_sites-gated edit check it now reuses. Reworded to match the single-item route's "not allowed to view sites". - `is_own_user_filter()` returned `true` for `user=me` unconditionally, so a logged-out `GET /sites?user=me` returned 200 (empty) instead of 401 - an unintentional anonymous-access bypass. Now requires being logged in. - `update_item()` unconditionally forced the response context to `edit`, while `create_item()` (changed by the same commit) now respects an explicit `context` request param, defaulting to `view`. Made `update_item()` consistent with `create_item()`. Updates test_get_items_me_filter_is_readable_when_logged_out to assert the corrected (403) behavior, and adds a regression test for the new embed-context allowance. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01UGF7E1FnyB4fpDGdwoDMyc
…ilter. Code review of the previous commit found the same class of bug it had just fixed for `user=me` also affects any non-numeric `user` value: `is_own_user_filter()` cast `$user` to `(int)` and compared it against `get_current_user_id()` with no logged-in guard, so a logged-out request (current user id 0) with e.g. `?user=xyz` satisfied `0 === (int) 'xyz'` and was treated as an authorized own-sites request. Require `is_user_logged_in()` for the general (non-"me") comparison too, matching the guard already added for the literal `me` case. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01UGF7E1FnyB4fpDGdwoDMyc
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
WP_REST_Sites_Controller::delete_item_permissions_check()only checked thedelete_sitemeta capability, which core maps unconditionally tomanage_options(capabilities.php) and is not scoped to the target site ID. This let any per-site Administrator — not just a network super admin — delete an arbitrary site network-wide viaDELETE /wp/v2/sites/{id}, unlike core's own network admin Sites screen (wp-admin/network/sites.php) and this controller's owncreate_item_permissions_check()/update_item_permissions_check(), which correctly require the super-admin-onlycreate_sites/manage_sitescapabilities.delete_sites(in addition to the existingdelete_sitemeta cap check) before a site can be deleted, matching the layeringwp-admin/network/sites.phpalready uses for its own delete action.rest_cannot_createerror fromcreate_item_permissions_check()instead of a bare boolean, matchingWP_REST_Posts_Controllerand this class's ownrest_cannot_edit/rest_cannot_deleteerrors.check_multisite_support()andcheck_delete_permission(), and reusecheck_edit_permission(), to de-duplicate capability checks that were repeated across every permission-check method.get_item_permissions_check(): a site's default/view-context data is now readable by members of that site (or a super admin), not the general public; any other context still requiresmanage_sites.Test plan
WP_Test_REST_Sites_Controllerpasses against a single-site install (phpunit --filter WP_Test_REST_Sites_Controller)WP_Test_REST_Sites_Controllerpasses against a multisite install (phpunit -c tests/phpunit/multisite.xml --filter WP_Test_REST_Sites_Controller)composer lint(phpcs) andcomposer phpstanclean on the changed fileDELETEa site they had no relationship to; confirmed the same request now returns403🤖 Generated with Claude Code