REST API: Permit unbounded per_page=-1 requests for authorized users

[danielbachhuber]

Currently, the REST API limits GET collection queries (e.g. GET /wp/v2/users) to a per_page value between 1 and 100. The decision protects a WordPress site from a resource-exhaustion DOS attack; unbounded queries can cause significant performance problems.

However, this limit negatively impacts Gutenberg, because there are contexts in which Gutenberg needs access to all items (e.g. the author drop-down). Given the significant accessibility challenges in producing a lazy-load implementation, a reasonable alternative is to permit unbounded per_page=-1 requests for authorized users. In this case, an authorized user can be defined as:

$can_unbounded_request = false;
$types = get_post_types( array( 'show_in_rest' => true ), 'objects' );
foreach ( $types as $type ) {
	if ( current_user_can( $type->cap->edit_posts ) ) {
		$can_unbounded_request = true;
	}
}

See conversation in ​https://github.com/WordPress/gutenberg/issues/6180 for the backstory.

[danielbachhuber]

Worth noting: there's a bug in WP_REST_Posts_Controller right now where passing posts_per_page=-1 will falsely error on:

$max_pages = ceil( $total_posts / (int) $posts_query->query_vars['posts_per_page'] );
if ( $page > $max_pages && $total_posts > 0 ) {
	return new WP_Error( 'rest_post_invalid_page_number', __( 'The page number requested is larger than the number of pages available.' ), array( 'status' => 400 ) );
}

This issue will need to be addressed too.

[desrosj]

Moving all tickets in 4.9.7 to 4.9.8.

[earnjam]

I've been working on a patch for this and am pretty close, but have a few inconsistencies that need to be discussed to determine the proper way to handle them:

1) Sanitize Callback

The parameter args take a sanitize_callback value. Currently that is set to absint in WP_REST_Controller and it works fine. The logical replacement would be intval since we'd now be allowing -1.

However, this throws a PHP Warning Wrong parameter count for intval() because we are passing 3 parameters in the call_user_func() call in WP_Rest_Request. This doesn't throw the warning for absint(), which only accepts 1 parameter, because it's not a built-in PHP function. It basically just stays silent if you pass too many parameters to user-defined functions.

If we don't pass a sanitize_callback as a parameter argument for per_page, then it ends up just falling back to using rest_sanitize_value_from_schema() and using the type value passed of integer and then gets typecast as (int), so that will work as long as we're ok not explicitly passing a sanitize_callback there.

---

2) per_page=0 vs per_page=-1

WP_Term_Query uses 0 for the number argument to get all terms instead of -1 like WP_Query and WP_User_Query do. How should we handle those?

Should we allow either -1 or 0 and treat them both as unbounded?

Should we return an invalid parameter for /wp/v2/posts and /wp/v2/users if someone passes per_page=0? Same with /wp/v2/terms if passed -1.

Or should we just silently fall back to the default of 10 in those cases?

I would lean toward the first option of accepting either value for any of the routes and treating them both as unbounded, but wanted feedback.

We have some divide by zero situations to resolve depending on the direction we go with this.

[danielbachhuber]

Replying to earnjam:

If we don't pass a sanitize_callback as a parameter argument for per_page, then it ends up just falling back to using rest_sanitize_value_from_schema() and using the type value passed of integer and then gets typecast as (int), so that will work as long as we're ok not explicitly passing a sanitize_callback there.

I agree. Let's start with this and we can introduce a dedicated rest_sanitize_intval if we really need it.

WP_Term_Query uses 0 for the number argument to get all terms instead of -1 like WP_Query and WP_User_Query do. How should we handle those?

Should we allow either -1 or 0 and treat them both as unbounded?

In the interest of keeping things simple, we should treat -1 as unbounded and 0 as a nonsensical zero results set. For WP_Term_Query, we should transform -1 into a 0, and ignore 0 early.

I've added you as a collaborator to ​https://github.com/danielbachhuber/wordpress-develop if you want to start a PR and work that way.

[earnjam]

Replying to danielbachhuber:

Thanks! I'll finish up what I've got and submit a PR there tomorrow to work through any other issues you see.

[earnjam]

Based on discussions in the REST API weekly meeting last week, we may be able to avoid this depending on the UI solutions implemented in Gutenberg. That said, I went ahead and did some work on it just in case we end up needing to go this route.

I pushed some working changes up to a new branch on @danielbachhuber 's git fork for anyone who wants to follow or add to it:
​https://github.com/danielbachhuber/wordpress-develop/commit/8ff6e4faf5dd90ac2f8bc0d2e873e64b5673482e

[adamsilverstein]

I don't think adding support for unbound requests to the REST API is a good idea. It breaks the contract and is not scalable or performant. I posted an alternate fix for the Gutenberg author dropdown that avoids the need for adding unbound queries, if this gets merged we can close this. See ​https://github.com/WordPress/gutenberg/pull/7385

[danielbachhuber]

@adamsilverstein It seems like that pull request only addresses Users? What about the other data types (Categories, Shared Blocks, etc.)?

[adamsilverstein]

@danielbachhuber good point, I will work to address those in additional PR's next. I wanted to start with users because I was already very familiar with that component and I want to see if the approach will work.

[danielbachhuber]

Punting from 4.9.8, as it doesn't need to happen in the immediate future.

[rmccue]

Replying to adamsilverstein:

I don't think adding support for unbound requests to the REST API is a good idea. It breaks the contract and is not scalable or performant.

I strongly agree with this. We have already begun seeing fatal errors due to Gutenberg's unbounded queries, and this is blocking our rollout of Gutenberg to sites. If this is included in 5.0, we will need to either force-override the limit (and hence break Gutenberg) or disable Gutenberg entirely.

Unbounded queries should never be allowed in a user-controlled interface. The only places in core where they exist currently are in tightly controlled places with minimal data sets (although I'd argue those should also be bounded in any case).

If the reason to do this is because lazyloading with pagination (i.e. Select2) is not accessible, then the UX and feature need to change, because it is, in my opinion, unacceptable that this should be in core. We cannot compromise on accessibility, but we can't take the site down either.

This should be a blocker for merge.

[danielbachhuber]

​As of this morning, I think we have an alternative path forward:

1. Keep the existing UI for Categories, Tags, Post Parent, Authors, and Shared Blocks.
2. Load data into Gutenberg with ?per_page=100&page=1, ?per_page=100&page=2.
3. Use ​https://github.com/WordPress/gutenberg/issues/6723 to avoid UI inconsistency while the data is loading.

We can use ​https://github.com/WordPress/gutenberg/issues/6694 to track the aforementioned steps through to completion.