WordPress.org

Make WordPress Core

Opened 4 months ago

Closed 7 weeks ago

#40704 closed defect (bug) (fixed)

REST API request includes possibly unintended numeric parameters from regex parsing

Reported by: flixos90 Owned by: flixos90
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.4
Component: REST API Keywords: has-patch has-unit-tests commit needs-dev-note
Focuses: rest-api Cc:

Description

When processing a route such as /wp/v2/users/(?P<id>[\d]+), the route is matched through a regular expression and its matches are stored in a variable $args. These $args are then passed to the request as URL parameters without any further processing. See the WP_REST_Server::dispatch() method.

This means that some unnecessary parameters are included that naturally appear in a matches array when parsing a regular expression. In the above example the $args array would be array( 'id' => 10, 1 => '10' ) due to the described bug.

This isn't very problematic when accessing specific request parameters, but causes unexpected results when calling the WP_REST_Request::get_params() method, since the additional numeric parameters that are a result of regex parsing are included in there.

I came across this bug while working on #40263, which uses WP_REST_Request::get_params(). Unless it was an intended decision to leave the regex result untouched, we should work on a fix soon.

Attachments (3)

40704.diff (829 bytes) - added by flixos90 4 months ago.
40704.2.diff (1.8 KB) - added by jnylen0 2 months ago.
Add unit test; remove unneeded continue
40704.3.diff (1.7 KB) - added by flixos90 7 weeks ago.

Download all attachments as: .zip

Change History (14)

@flixos90
4 months ago

#1 @flixos90
4 months ago

  • Keywords has-patch added; needs-patch removed

40704.diff fixes this bug for the patch in #40263.

I'm not exactly sure why this bug happens as it only seems to occur under very specific conditions. I was not able to replicate it when trying to write unit tests for the actual bug in tests/rest-api/rest-server.php. More eyes on this would help determining the actual cause for the problem.

The above patch might however be already sufficient to move forward. Let's wait for some reviews on how to proceed here.

#2 @flixos90
4 months ago

  • Milestone changed from Awaiting Review to 4.8.1

Let's try to find a proper solution for this bug in the next minor release, as it's blocking #40263 which should be fixed sooner than later.

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


2 months ago

@jnylen0
2 months ago

Add unit test; remove unneeded continue

#4 follow-up: @jnylen0
2 months ago

  • Keywords has-unit-tests commit added; 2nd-opinion needs-unit-tests removed
  • Version changed from 4.7 to 4.4

This means that some unnecessary parameters are included that naturally appear in a matches array when parsing a regular expression. In the above example the $args array would be array( 'id' => 10, 1 => '10' ) due to the described bug.

This appears to happen whenever PCRE matches against a named subpattern (in our case, any URL parameters), and it's documented in the PHP docs for preg_match:

Example #4 Using named subpattern

<?php
$str = 'foobar: 2008';
preg_match('/(?P<name>\w+): (?P<digit>\d+)/', $str, $matches);
print_r($matches);

?>
Array
(
    [0] => foobar: 2008
    [name] => foobar
    [1] => foobar
    [digit] => 2008
    [2] => 2008
)

40704.2.diff adds a unit test and a minor cleanup to the logic in the original patch. I think this is good to ship in 4.8.1.

#5 in reply to: ↑ 4 @rmccue
2 months ago

Replying to jnylen0:

40704.2.diff adds a unit test and a minor cleanup to the logic in the original patch. I think this is good to ship in 4.8.1.

The problem with this is that we're giving people full regular expressions, and they might not necessarily expect that regular (numeric) matches don't appear.

For example, they might be using behaviour like:

register_rest_route( '/ns', '/route/(\d+)/(\d+)', array(
	'callback' => function ( $matches ) {
		$post = $matches[1];
		$page = $matches[2];
		// ...
	}
));

If we want to drop support for that, we should make sure we're doing so intentionally, and that it's well-documented in the release notes. IMO, it should also go into a major not a minor.

#6 @flixos90
2 months ago

@rmccue

If we want to drop support for that, we should make sure we're doing so intentionally, and that it's well-documented in the release notes. IMO, it should also go into a major not a minor.

Good point, I agree. I think it's fine to drop support since all the examples (from which people are hopefully copy/paste/modifying) did not use numeric matches. But that is indeed a change that should be highlighted in a dev-note. I'd move this to 4.9.

#7 @jnylen0
2 months ago

  • Milestone changed from 4.8.1 to 4.9

Ok, moving back to 4.9. I'll do a scan of the plugin repo and try to figure out if people are actually doing this.

#8 @jnylen0
2 months ago

I checked for potentially problematic code in the plugin directory and I didn't see anything that made me hesitant to proceed with this change (in 4.9, with a dev note):

ag --php --skip-vcs-ignores '\$(req|request)\s*\[\s*[0-9]+' | tee scans/access-numeric.txt

Instances of $request[index] and $req[index] that use numeric indices. None appeared to be related to REST API code.

ag --php --skip-vcs-ignores register_rest_route | tee scans/register_rest_route.txt
scans/register_rest_route-strings.php # https://gist.github.com/nylen/866603c7a910d63f59f6bfbd917bc10f

register_rest_route calls with non-named match parameters. All of these are inside a named match expression (and there are no accesses to parameters with numeric indices here either):

plugins/auto-video-youtube-poster/index.php:633:        register_rest_route('video-producer/v1', '/images_from_post/(?P<post_id>(.*)+)', array(
plugins/auto-video-youtube-poster/index.php:638:        register_rest_route('video-producer/v1', '/data_from_post/(?P<post_id>(.*)+)', array(
plugins/woo-infoplus-connect/includes/api/rest/class-wc-infoplus-rest-orders-controller.php:56:         register_rest_route( $this->namespace, '/' . $this->rest_base . '/(?P<orderNo>[\d]+(\.[0-9][0-9][0-9])?)', array(
plugins/wp-rest-api-v2-menus/wp-rest-api-v2-menus.php:44:    register_rest_route( 'menus/v1', '/menus/(?P<id>[a-zA-Z(-]+)', array(
Last edited 7 weeks ago by jnylen0 (previous) (diff)

@flixos90
7 weeks ago

#9 @flixos90
7 weeks ago

  • Owner set to flixos90
  • Status changed from new to reviewing

I just reviewed the changes, things work fine. 40704.3.diff adds a missing ticket annotation to the test.

Let's get this in.

#10 @flixos90
7 weeks ago

  • Keywords needs-dev-note added

#11 @jnylen0
7 weeks ago

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

In 41223:

REST API: Exclude numeric parameters from regex parsing

The list of endpoint parameters should only include explicitly named and requested parameters.

Props flixos90, rmccue, jnylen0.
Fixes #40704.

Note: See TracTickets for help on using tickets.