WordPress.org

Make WordPress Core

Opened 3 weeks ago

Last modified 4 days ago

#43998 new enhancement

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

Reported by: danielbachhuber Owned by:
Milestone: 4.9.8 Priority: normal
Severity: normal Version:
Component: REST API Keywords: needs-patch needs-unit-tests
Focuses: rest-api Cc:

Description

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.

Change History (6)

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


3 weeks ago

#2 @danielbachhuber
2 weeks ago

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.

#3 @desrosj
6 days ago

  • Milestone changed from 4.9.7 to 4.9.8

Moving all tickets in 4.9.7 to 4.9.8.

#4 follow-up: @earnjam
4 days ago

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.

#5 in reply to: ↑ 4 ; follow-up: @danielbachhuber
4 days ago

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.

#6 in reply to: ↑ 5 @earnjam
4 days ago

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.

Note: See TracTickets for help on using tickets.