Opened 7 years ago
Closed 6 years ago
#43998 closed enhancement (wontfix)
REST API: Permit unbounded per_page=-1 requests for authorized users
Reported by: | danielbachhuber | Owned by: | |
---|---|---|---|
Milestone: | Priority: | normal | |
Severity: | normal | Version: | |
Component: | REST API | Keywords: | |
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 (17)
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
7 years ago
#4
follow-up:
↓ 5
@
7 years 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:
↓ 6
@
7 years ago
Replying to earnjam:
If we don't pass a
sanitize_callback
as a parameter argument forper_page
, then it ends up just falling back to usingrest_sanitize_value_from_schema()
and using thetype
value passed ofinteger
and then gets typecast as(int)
, so that will work as long as we're ok not explicitly passing asanitize_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
@
7 years 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.
#7
@
7 years ago
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
#8
follow-up:
↓ 15
@
6 years ago
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
#9
@
6 years ago
@adamsilverstein It seems like that pull request only addresses Users? What about the other data types (Categories, Shared Blocks, etc.)?
#10
@
6 years ago
@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.
This ticket was mentioned in Slack in #core by joshuawold. View the logs.
6 years ago
#12
@
6 years ago
- Milestone changed from 4.9.8 to 4.9.9
Punting from 4.9.8, as it doesn't need to happen in the immediate future.
This ticket was mentioned in Slack in #core-restapi by earnjam. View the logs.
6 years ago
#15
in reply to:
↑ 8
@
6 years ago
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.
This ticket was mentioned in Slack in #core-editor by danielbachhuber. View the logs.
6 years ago
#17
@
6 years ago
- Keywords needs-patch needs-unit-tests removed
- Milestone 5.0 deleted
- Resolution set to wontfix
- Status changed from new to closed
As of this morning, I think we have an alternative path forward:
- Keep the existing UI for Categories, Tags, Post Parent, Authors, and Shared Blocks.
- Load data into Gutenberg with
?per_page=100&page=1
,?per_page=100&page=2
. - 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.
Worth noting: there's a bug in
WP_REST_Posts_Controller
right now where passingposts_per_page=-1
will falsely error on:This issue will need to be addressed too.