REST API endpoints for sites

[lloc]

Trac ticket: https://core.trac.wordpress.org/ticket/40365

There are some open questions that we should discuss in the trac ticket.

[github-actions]

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 props-bot label.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @jonnydmg.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

Core Committers: Use this line as a base for the props when committing in SVN:

Props realloc, spacedmonkey, peterwilsoncc, biont, johnjamesjacoby.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[github-actions]

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

• All changes will be lost when closing a tab with a Playground instance.
• All changes will be lost when refreshing the page.
• A fresh instance is created each time the link below is clicked.
• Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
  it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

[jonnydmg]

There is a big PR to review. Good work getting this ready for core. This code a little old and some things have changed in core, so this PR needs to update accordingly.

Biggest things.

• We need to support sites with is_site_meta_supported false. There was some work done there to support it but not complete.
• Site endpoint needs to be registered on single site. So it always exists then return an error on single site.
• WP_REST_Site_Meta_Fields has no test coverage at all.
• Single site needs tests.
• Super admin should be allowed to do everything on the endpoint.
• Code coverage and ticket need needs to be added to every test.

This feel like we are on the right track, if I am nitpicing here, becuase I think this good to go into core and just want to get the final bits to push forward into core.

I will wait until feedback is complete to do some real testing for this PR.

[jonnydmg]

There is a big PR to review. Good work getting this ready for core. This code a little old and some things have changed in core, so this PR needs to update accordingly.

Biggest things.

• We need to support sites with is_site_meta_supported false. There was some work done there to support it but not complete.
• Site endpoint needs to be registered on single site. So it always exists then return an error on single site.
• WP_REST_Site_Meta_Fields has no test coverage at all.
• Single site needs tests.
• Super admin should be allowed to do everything on the endpoint.
• Code coverage and ticket need needs to be added to every test.

This feel like we are on the right track, if I am nitpicing here, becuase I think this good to go into core and just want to get the final bits to push forward into core.

I will wait until feedback is complete to do some real testing for this PR.

[spacedmonkey]

@lloc I have put together a little PR for tests for all the meta changes -lloc#1

[spacedmonkey]

Here is a PR with some unit test improvements.

lloc#2

This is has unit tests that cover single site as well.

[spacedmonkey]

I have more feedback here.

lloc#4
lloc#3

This fixes some issues that were flagged by ai.

[spacedmonkey]

I have spent a long time testing this and I am happy to say, I am happy to commit. Thanks for all the amazing work @lloc, thanks for getting this one across the line.

[peterwilsoncc]

While the changes to checking and getting super admins may be helpful for a programmatic API I don't think that querying one network from another via a public API is a security risk.

For example, this current implimentation exposes cross nmetwork sites when no network is specified.

1. Install and network enable WP Multi Network
2. Create a new network
3. Create a new user (n2admin)
4. Set n2admin as a super user of the second network
5. Log in to the second network as n2admin
6. Observe in the dashboard "My Networks" they only have access to network 2
7. In the browser console run the command wp.apiFetch( { 'path': '/wp/v2/sites' } )
8. Observe the result contains details for both the network they are super admin of and the network

This is an example of why I made my earlier comment that the network should be inferred from the URL on which the request is being made. It's a security risk but, equally significantly, details of one network should not be made available from another. Doing so breaks the isolation multiple networks are intended to maintain.

[jonnydmg]

While the changes to checking and getting super admins may be helpful for a programmatic API I don't think that querying one network from another via a public API is a security risk.

For example, this current implimentation exposes cross nmetwork sites when no network is specified.

1. Install and network enable WP Multi Network
2. Create a new network
3. Create a new user (n2admin)
4. Set n2admin as a super user of the second network
5. Log in to the second network as n2admin
6. Observe in the dashboard "My Networks" they only have access to network 2
7. In the browser console run the command wp.apiFetch( { 'path': '/wp/v2/sites' } )
8. Observe the result contains details for both the network they are super admin of and the network

This is an example of why I made my earlier comment that the network should be inferred from the URL on which the request is being made. It's a security risk but, equally significantly, details of one network should not be made available from another. Doing so breaks the isolation multiple networks are intended to maintain.

That is a bug and sorry that one is me. This commit 959bf3d.

I will fix it.

Fixed in
lloc#10

[peterwilsoncc]

@spacedmonkey @jonnydmg The endpoint just can't allow querying across network, it will introduce data exposure no matter how it's coded up

Each network is entirely independent: logins, network activated plugins, mu-plugins can be network specific via get_current_network_id(). Each site can have a different set of plugins activated too.

One network may hard code super admins via pre_site_option_{$option}, another network may not. Access on the network that does so will bypass the filter on the network that does not.

The same will occur if one network filters the manage_sites permission via the map_meta_cap filter, the other may not.

Even for sites on the same network, the plugins can differ site-to-site so the endpoint can't be available on sub-sites due to different plugins.

This is what I think needs to happen before this can be considered secure enough to commit:

1. No cross-network queries, the network_id parameter is hard coded to the current network in WP_Site_Query()
2. The endpoint is only available on the main site, ie is_main_site() === true -- this matches the network admin behaviour in which /sub-site/wp-admin/network redirects to /wp-admin/network on the main site.

This will ensure that plugins and mu-plugins for the endpoint match what is loaded in the network admin currently.

[spacedmonkey]

@spacedmonkey

1. No cross-network queries, the network_id parameter is hard coded to the current network in WP_Site_Query()
2. The endpoint is only available on the main site, ie is_main_site() === true -- this matches the network admin behaviour in which /sub-site/wp-admin/network redirects to /wp-admin/network on the main site.

This will ensure that plugins and mu-plugins for the endpoint match what is loaded in the network admin currently.

Just a heads up, jonnydmg and spacedmonkey are both me!

1. I agree, I have removed this functionality, but added a single filter, so plugins could add could add this functionality in if they want to. Using filters, rest_site_in_network ( new filter ), rest_site_collection_params ( to add network parameter ) and rest_site_query ( to update query ) and rest_pre_insert_site ( to update crud functions ). Between these filters that could be enough to restore this functionality. See REST API: Limit the sites endpoint to the current network lloc/wordpress-develop#11
2. Limiting to main site is not really an option. One of the other planned use for this endpoint in core is "My sites" section in the admin toolbar. This is why is there is a user_id of "me" to get my sites. If we limit this endpoint to just the main site, then we will have corrs errors.

I know there could be something on the subsite that could tweak the permsissoins. But if you have a plugin doing that, you could do a lot, like regsitering your own sites endpoint or otherwise expose site data. I am not sure this is something we can lock down. You would hope the network maintainer would lock this down.

Thoughts @peterwilsoncc @lloc ?

[lloc]

I checked this against WP Multi Network. Peter's cross-network leak is already fixed: get_items is pinned to the current network, and site_in_network() blocks single sites on other networks.

Proposal:

• Listing, edit context, create/update/delete: main site only (is_main_site(), which is per network) and current network only. These then run with the same plugins as /wp-admin/network. One caveat: is_network_admin() is false in REST, so mu-plugins that check it still won't load.
• user=me in view context: allowed on every site and across networks, which avoids the CORS issue. The toolbar already lists these sites via get_blogs_of_user(), which ignores networks. The endpoint returns a few more fields (dates, post_count, lang_id), all harmless. GET /sites/{id} for member sites would need the same exemption for consistency.

Anyone who wants cross-network management can restore it through the rest_site_query and rest_site_in_network filters. WP Multi Network's own controller shows the pattern: switch_to_network(), then current_user_can( 'manage_network' ).

@peterwilsoncc @spacedmonkey Would that work for both of you?

[spacedmonkey]

@lloc @peterwilsoncc

If we expose data to members of the site, should be consider making register meta only show if context=edit. Site meta would might have senative data in it.

[JJJ]

Thanks everyone for continuing to work through this. @lloc, you’ve done a great job moving a large and complicated change forward, testing it with WP Multi Network, and narrowing the remaining disagreement.

I think @peterwilsoncc is likely right that the safest boundary for this controller is:

1. Current-network access only.
2. Main-site access only, so requests run with the same plugin environment as /wp-admin/network/.

I am hesitant to add a user=me cross-network exception. The core fields appear harmless but the response is extensible through registered site-meta, additional REST fields, and filters such as rest_prepare_site. The site-meta question raised above is one example of how the effective response can differ from the fixed fields in the controller.

I also don’t think the toolbar use-case needs to determine this controller’s security model, or necessarily needs to be implemented in core as part of this change. The toolbar already has a long history of accumulating responsibilities and blog/network queries, and my guess is we'd all prefer not to repeat some of those old design mistakes. 😆

If a client-side, cross-network "My Sites" API is needed later, that would be easier to review as a separate, read-only endpoint with a deliberately small and fixed response schema. (That keeps this controller aligned with network administration while giving the toolbar use case its own security and API-design discussion.)

My personal preference is to keep this PR focused on current-network management from the network's main site, without the cross-network member exception.

Thank you again everyone for all the work getting it this far. 💗

[spacedmonkey]

@lloc @peterwilsoncc @JJJ I've opened lloc#12 against feature/40365-rest-sites-endpoint with the changes from the discussion above. In summary:

Main site only

• Create, update, delete and single-site reads only work from the network's main site. From any other site they return a 403: rest_cannot_{create,edit,delete,view}_not_on_main_site.
• The "my sites" exceptions are kept narrow and only work in the view context: listing your own sites (user=me or your own ID), and a member reading their own site. embed is no longer exempt.

Permissions

• context=edit requires manage_sites on both routes. It is checked before the site is looked up, so it doesn't reveal whether a site exists.
• Listing another user's sites requires manage_sites (rest_forbidden_user).
• Without manage_sites, you can't filter or order by fields you can't see (rest_forbidden_param). That covers archived, mature, spam, deleted, lang_id, lang_id_exclude, before, after and orderby=registered|last_updated. Otherwise the results would reveal those values.

Response and schema

• The status flags, the dates, lang_id, post_count and the registered site meta are now edit context only. This addresses the concern @JJJ and I raised that extensible fields like meta could leak to members.
• New read-only admin_url. home and siteurl now come from get_home_url() and get_site_url(), so their filters apply.
• title duplicated blogname, so it's removed. blogname is now writable when a site is created.

user parameter

• The user filter and the create-only user_id are now a single user parameter. It is validated as a positive user ID or "me", and invalid values return rest_invalid_param.

Other

• If the user filter matches no sites, the site queries are skipped, but rest_site_query still fires.

All sites and site meta tests pass on multisite (254) and single site, and lint and PHPStan are clean.

Feedback welcome, particularly on:

1. Whether keeping the view-context "my sites" exceptions is acceptable, given the main-site-only boundary.
2. The set of edit-only fields.
3. Merging user and user_id into one parameter.

[lloc]

Pushed a fix for the domain length @spacedmonkey flagged on 09-28.

domain now has maxLength: 200 and path has maxLength: 100, matching the varchar(200) and varchar(100) columns on wp_blogs in schema.php. Until now a longer value passed validation, failed in wpdb on insert, and came back as a 500 db_insert_error. It's now a 400 rest_invalid_param. I removed the 253 check in is_valid_domain() because the new limit is stricter. Both validation data providers have boundary cases, which run on create and update, and the QUnit fixture is regenerated.

It's all in one separate commit, so it's easy to revert if you'd rather handle this differently.

[lloc]

@peterwilsoncc @JJJ, could you look at @spacedmonkey's questions in lloc#12? The first one decides whether we keep the "my sites" exception. It's limited to the view context and the current network, but it still works from subsites. If you'd rather require the main site for everything, I'm happy to remove it.