WordPress.org

Make WordPress Core

Opened 6 months ago

Last modified 13 days ago

#39061 reopened enhancement

REST API pagination: Large INT passed to `paged` query arg doesn't fail properly

Reported by: morganestes Owned by: joehoyle
Milestone: 4.8.1 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: needs-patch needs-unit-tests
Focuses: Cc:

Description

When an absurdly large value is passed to the REST API (e.g. /wp/v2/pages?page=23924321212413345333), it returns the first page of results instead of an error. The problem is during validation and sanitization of the value, where the passed value is run through absint, which returns another absurdly large value, which then gets nullified by PHP, which becomes 1.

wp> print_r( rest_sanitize_value_from_schema( 23452345346346345456567356, array( 'type' => 'integer' ), 'page' ) );
3481259413623275520
=> bool(true)
wp> print_r( rest_validate_value_from_schema( 23452345346346345456567356, array( 'type' => 'integer' ), 'page' ) );
1
=> bool(true)

wp> absint(23924321212413345333);
=> int(5477577138703794176)

Edge case, but worth noting since smaller values that are larger than the number of pages return an empty array (like if there are only 2 pages of posts, but 3 are requested).

Related: #19728.

Attachments (4)

39061.diff (827 bytes) - added by morganestes 6 months ago.
39061.2.diff (1.8 KB) - added by morganestes 6 months ago.
39061.3.diff (1.8 KB) - added by morganestes 6 months ago.
39061-comments.diff (2.8 KB) - added by joehoyle 5 weeks ago.

Download all attachments as: .zip

Change History (21)

#1 @rachelbaker
6 months ago

  • Keywords needs-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to Future Release
  • Type changed from defect (bug) to enhancement

Related #28559

@morganestes want to work on a patch?

#2 @morganestes
6 months ago

@rachelbaker yep, I'll work on "smaller values that are larger than the number of pages return an empty array (like if there are only 2 pages of posts, but 3 are requested)."

@morganestes
6 months ago

#3 @morganestes
6 months ago

39061.diff is a first pass at returning an error for an out-of-bounds page request for posts. If this looks right, I'll do the same for the other endpoints.

#4 @morganestes
6 months ago

  • Keywords has-patch added; needs-patch removed

#5 @morganestes
6 months ago

  • Keywords dev-feedback added

@morganestes
6 months ago

#6 @morganestes
6 months ago

Added a test for OOB posts pagination in 39061.2.diff.

@morganestes
6 months ago

#7 @morganestes
6 months ago

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

In 39061.3.diff: make sure to check for no posts in a response before returning an error. This prevents unintended failure on empty arrays when none are expected.

#8 @rmccue
4 months ago

It seems like the cutoff value here is PHP_INT_MAX, so could we just check $value > PHP_INT_MAX?

#9 @joehoyle
4 months ago

  • Owner set to joehoyle
  • Resolution set to fixed
  • Status changed from new to closed

In 39967:

REST API: Return an error if the page number is out of bounds.

Return an error from the REST API if a page number larger than the total pages count is requested.

Props morganestes.
Fixes #39061.

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


4 months ago

#11 @joehoyle
4 months ago

In 39991:

REST API: Fix unit tests for posts out of bounds errors

Previously we were assuming pagination headers would be sent when the request for posts is out of bounds. Instead presume it will return an error.

See #39061.

#12 @swissspidy
4 months ago

  • Milestone changed from Future Release to 4.8

#13 @ocean90
3 months ago

  • Keywords needs-patch needs-unit-tests added; has-patch dev-feedback has-unit-tests removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

We should do the same for all endpoints (terms, comments and users) as mentioned in comment:3.

#14 @joehoyle
5 weeks ago

Attached a patch for comments endpoint with the same functionality:

  1. Handle page query var that is bigger than PHP_MAX_INT.
  2. Show an out of bounds error if the page is larger than X-TotalPages

Like /wp/v2/posts we also no longer return pagination headers in the case of the out of bounds error.

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


2 weeks ago

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


13 days ago

#17 @jbpaul17
13 days ago

  • Milestone changed from 4.8 to 4.8.1

Punting to 4.8.1 per today's bug scrub.

Note: See TracTickets for help on using tickets.