WordPress.org

Make WordPress Core

Opened 4 months ago

Closed 4 months ago

Last modified 2 weeks ago

#38446 closed task (blessed) (fixed)

Deprecate the rest_enabled filter

Reported by: pento Owned by: pento
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: needs-docs needs-dev-note commit has-unit-tests has-patch
Focuses: Cc:

Description

With the introduction of Core functionality that relies on the REST API existing (#38342 and #38343), being able to turn off the REST API is roughly the equivalent of being able to turn off admin-ajax.php - something that will break your site.

As more Dashboard (and plugin!) functionality comes to rely on the REST API, it's irresponsible of us to provide a method for disabling the REST API entirely.

I'd also be inclined to throw a _doing_it_wrong if anything is actually attached to the rest_enabled filter.

Attachments (2)

38446.diff (1.1 KB) - added by jorbin 4 months ago.
38446.2.diff (2.0 KB) - added by pento 4 months ago.

Download all attachments as: .zip

Change History (23)

#1 @johnbillion
4 months ago

Devil's advocate for the purposes of considering the impact of removing this filter: What will people who really want to disable the REST API start doing instead in order to disable it? What will break in the process? Will this cause other headaches to core or site owners?

#2 @pento
4 months ago

There are still options to disable it, but it will hopefully encourage a more considered approach. For example, returning an error on the rest_authentication_errors filter will prevent all REST API calls, but would hopefully encourage a little bit of nuance - returning an error when ! is_user_logged_in(), for example.

I'm inclined to think that an off switch like rest_enabled is too much of a blunt instrument.

@jorbin
4 months ago

#3 @jorbin
4 months ago

First pass at some language and removal of code. Still needs some unit tests.

#4 @pento
4 months ago

  • Keywords needs-docs needs-dev-note added

@dmchale: Hi there! It looks like yours is the only WordPress.org plugin that makes use of this filter - as the REST API is going to become a dependency for wp-admin, disabling the REST API won't work anymore.

I'd recommend replacing it with a check that the user is logged in, something like:

function only_allow_logged_in_rest_access( $access ) {
        if( ! is_user_logged_in() ) {
                return new WP_Error( 'rest_cannot_access', __( 'Only authenticated users can access the REST API.', 'disable-json-api' ), array( 'status' => rest_authorization_required_code() ) );
        }

        return $access;
}
add_filter( 'rest_authentication_errors', 'only_allow_logged_in_rest_access' );

Ticket note: I'm inclined to do a make/core dev note for this change, too, as there are several snippets floating around to do the same thing.

We can document rest_authentication_errors as the alternative, and include this example on docs page.

#5 @dmchale
4 months ago

@pento thanks for the heads up. Is it merely being deprecated during the 4.7 cycle, or will the filter already be removed entirely then? I agree a make/core post would be great regardless since I know I've seen Ryan post it as a solution for people numerous times, so I'm sure there's many installs out there NOT using my plugin to achieve the same thing.

#6 @pento
4 months ago

The current plan is 38446.diff - the filter will run, but the return value will be ignored.

I'm waiting for input from the REST API team before we go ahead with any changes, though.

#7 follow-up: @rmccue
4 months ago

I am -1 on removing this, for a few reasons.

  1. I think we should give people enough rope to hang themselves if they really want. The fact is that the REST API does introduce a new attack surface (the Flash XSS exploit for example).
  2. The admin should be designed to work without JavaScript, and hence without the REST API. For people that disable the API, the progressive enhancement should drop back to standard interactions.

That said, there is a way to remove the endpoints without needing to use this filter:

remove_action( 'rest_api_init', 'create_initial_rest_routes', 99 );

I think we should start publicising this method rather than the sledgehammer that is rest_enabled.

@dmchale Maybe you could switch your plugin to that method? Unsure if that's changing the functionality too much.

#8 in reply to: ↑ 7 @pento
4 months ago

Replying to rmccue:

  1. I think we should give people enough rope to hang themselves if they really want.

Conversely, they already have plenty of ways to explode their site, they don't really need one more.

The fact is that the REST API does introduce a new attack surface (the Flash XSS exploit for example).

Everything we do introduces a new attack surface, but we don't add an off button for each feature. If a REST API vulnerability is discovered, a Core auto update is far more efficient at protecting sites than a filter to disable the REST API.

  1. The admin should be designed to work without JavaScript, and hence without the REST API. For people that disable the API, the progressive enhancement should drop back to standard interactions.

There are already significant parts that either require JS (the Customizer), or the no-js fallback is barely usable (the post editor). As more development moves to a JS first (or JS only) model, I expect the balance to tip more in a Customizer direction. Indeed, as the Customizer moves to using the REST API, disabling the API (or particular endpoints) will cause all sorts of exciting behaviour.

That said, there is a way to remove the endpoints without needing to use this filter:

remove_action( 'rest_api_init', 'create_initial_rest_routes', 99 );

I think we should start publicising this method rather than the sledgehammer that is rest_enabled.

Per discussion on #38339, removing the core endpoints is just as bad, we shouldn't be encouraging that.

#9 @dmchale
4 months ago

I am fine with whatever updates are needed for the plugin. I will probably need to add a Settings page no matter what happens, so that site admins can configure whatever solution is deemed best practice.

I'll keep an eye on things from the background until a decision has been made, and figure out the plugin's best reaction to the ecosystem from there.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


4 months ago

#11 @johnbillion
4 months ago

  • Version set to trunk

#12 @jorbin
4 months ago

  • Type changed from enhancement to task (blessed)

This decision is less of an enhancement, and more of a task for us to decide if we want to do it, changing the type accordingly.

The admin should be designed to work without JavaScript, and hence without the REST API. For people that disable the API, the progressive enhancement should drop back to standard interactions.

There are parts of the admin that need to always work without JavaScript, namely the ability to change themes and the ability to activate/deactivate plugins. Beyond that, JavaScript is a foundational part of the web that we all should learn. deeply. And it's something that features can and should use.

@pento
4 months ago

#13 @pento
4 months ago

  • Keywords commit has-unit-tests has-patch added; needs-patch needs-unit-tests removed

38446.2.diff updates the deprecated message, and adds a unit test.

#14 @pento
4 months ago

  • Owner set to pento
  • Status changed from new to assigned

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


4 months ago

#16 @pento
4 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 38947:

REST API: Deprecate the rest_enabled filter.

As the REST API becomes more integral to WordPress Core, turning it off will cause a... suboptimal experience. If we don't want it to be turned off, the off switch needs to be removed.

Props jorbin, pento.
Fixes #38446.

This ticket was mentioned in Slack in #core-restapi by schlessera. View the logs.


2 months ago

#18 @jnylen0
3 weeks ago

Noting that rest_request_before_callbacks (ref) is probably a better filter to use than rest_authentication_errors, because it gets access to the $request object.

This makes it really easy to only restrict, for example, the built-in wp/v2 endpoints by checking the beginning of $request->get_route().

#19 @SergeyBiryukov
3 weeks ago

In 40038:

REST API: After [38947], improve the wording of the message to clarify that rest_authentication_errors is a filter.

See #38446.

#20 @wturrell
2 weeks ago

Any case for revisiting this, given recent events?

#21 @dmchale
2 weeks ago

Not sure what needs to be revisited? If you'd like to disable the REST API to forward-facing site visitors you can use the code that @pento posted above which is nearly identical to the example found in the REST API documentation, or you can install the plugin that does it for you. You just can't use the old rest_enabled filter anymore.

Note: See TracTickets for help on using tickets.