REST API request includes possibly unintended numeric parameters from regex parsing

[flixos90]

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.

[flixos90]

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.

[flixos90]

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.

[jnylen0]

Add unit test; remove unneeded continue

[jnylen0]

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.

[rmccue]

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.

[flixos90]

@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.

[jnylen0]

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.

[jnylen0]

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(

[flixos90]

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

Let's get this in.

[jnylen0]

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.

[jbpaul17]

Thanks to @flixos90 for publishing the related dev note: ​https://make.wordpress.org/core/2017/10/25/improvements-in-rest-api-request-parameter-regular-expressions/