WP_Query::parse_query() does not handle invalid query arg values

[xknown]

The code in WP_Query::parse_query assumes that most query arguments have the correct type. However, this doesn't seem to be the case for the following arguments:

Things that expect arrays, but the code that call them pass (unintended) invalid data types

					'author__in'          => 'string',
					'author__not_in'      => 'string',
					'category__and'       => 'string',
					'category__in'        => 'string',
					'category__not_in'    => 'string',
					'post__in'            => 'string',
					'post__not_in'        => 'string',
					'post_name__in'       => 'string',
					'post_parent__in'     => 'string',
					'post_parent__not_in' => 'string',
					'tag__and'            => 'string',
					'tag__in'             => 'string',
					'tag__not_in'         => 'string',
					'tag_slug__and'       => 'string',
					'tag_slug__in'        => 'string',

Using most of the above query args in a WP_Query::get_posts() call result in a PHP fatal.

Things that expect scalars:

					'attachment'      => array(),
					'author_name'     => array(),
					'feed'            => array(),

Using any of the above query args result in a PHP fatal on a default WP installation:

alex@wayra core % cat .wp-env.json
{
	"core": null
}

​http://localhost:8888/?attachment[]=admin

Fatal error: Uncaught TypeError: urlencode(): Argument #1 ($string) must be of type string, array given in /var/www/html/wp-includes/formatting.php:5683 Stack trace: #0 /var/www/html/wp-includes/formatting.php(5683): urlencode(Array) #1 /var/www/html/wp-includes/class-wp-query.php(2183): wp_basename(Array) #2 /var/www/html/wp-includes/class-wp-query.php(3824): WP_Query->get_posts() #3 /var/www/html/wp-includes/class-wp.php(696): WP_Query->query(Array) #4 /var/www/html/wp-includes/class-wp.php(816): WP->query_posts() #5 /var/www/html/wp-includes/functions.php(1336): WP->main('') #6 /var/www/html/wp-blog-header.php(16): wp() #7 /var/www/html/index.php(17): require('/var/www/html/w...') #8 {main} thrown in /var/www/html/wp-includes/formatting.php on line 5683

​http://localhost:8888/?author_name[]=admin

Fatal error: Uncaught TypeError: str_contains(): Argument #1 ($haystack) must be of type string, array given in /var/www/html/wp-includes/class-wp-query.php:2358 Stack trace: #0 /var/www/html/wp-includes/class-wp-query.php(2358): str_contains(Array, '/') #1 /var/www/html/wp-includes/class-wp-query.php(3824): WP_Query->get_posts() #2 /var/www/html/wp-includes/class-wp.php(696): WP_Query->query(Array) #3 /var/www/html/wp-includes/class-wp.php(816): WP->query_posts() #4 /var/www/html/wp-includes/functions.php(1336): WP->main('') #5 /var/www/html/wp-blog-header.php(16): wp() #6 /var/www/html/index.php(17): require('/var/www/html/w...') #7 {main} thrown in /var/www/html/wp-includes/class-wp-query.php on line 2358

​http://localhost:8888/?feed[]=admin

Fatal error: Uncaught TypeError: str_contains(): Argument #1 ($haystack) must be of type string, array given in /var/www/html/wp-includes/class-wp-query.php:1018 Stack trace: #0 /var/www/html/wp-includes/class-wp-query.php(1018): str_contains(Array, 'comments-') #1 /var/www/html/wp-includes/class-wp-query.php(1868): WP_Query->parse_query() #2 /var/www/html/wp-includes/class-wp-query.php(3824): WP_Query->get_posts() #3 /var/www/html/wp-includes/class-wp.php(696): WP_Query->query(Array) #4 /var/www/html/wp-includes/class-wp.php(816): WP->query_posts() #5 /var/www/html/wp-includes/functions.php(1336): WP->main('') #6 /var/www/html/wp-blog-header.php(16): wp() #7 /var/www/html/index.php(17): require('/var/www/html/w...') #8 {main} thrown in /var/www/html/wp-includes/class-wp-query.php on line 1018

Add is_scalar/is_array checks.

Trac ticket: https://core.trac.wordpress.org/ticket/60745

[jrf]

@xknown PHP errors like these exist for a reason: to warn developers and allow them to fix their code.

Hiding these errors as per your proposal (and as previously clearly applied in part as well), is often the cause of insidious bugs and makes debugging faulty queries much harder as some "mystery default" gets used instead of the query throwing an error which the developer can fix.

[peterwilsoncc]

I agree with @jrf that these shouldn't be fixed in WP_Query.

For the use cases provided of visitors using the incorrect data type in URLs, eg localhost?attachment[]=foobar, data sanitization should take place in WP::parse_request() before calling WP_Query. The former being a user API, the latter a developer API.

Related #56311.

[josephscott]

This is a bug in WordPress core code. You can easily demonstrate this fatal error using the WordPress Playground ( I tested these steps in Chrome ).

• Visit ​https://playground.wordpress.net/ to spin up a fresh site ( new incognito can be helpful )
• Open one of the page links in a new tab to get a fresh scoped URL, like ​https://playground.wordpress.net/scope:0.2459814000732259/
• Add ?feed[]=admin to the scoped URL, like ​https://playground.wordpress.net/scope:0.2459814000732259/?feed[]=admin and request that URL
• Get a fatal error back
• Go to the original tab, in the three-bar menu click on View logs
• Review the stack traces for the fatal error

PHP Fatal error: Uncaught TypeError: str_contains(): Argument #1 ($haystack) must be of type string, array given in /wordpress/wp-includes/class-wp-query.php:3
Stack trace:
#0 /wordpress/wp-includes/class-wp-query.php(3): str_contains(Array, 'comments-')
#1 /wordpress/wp-includes/class-wp-query.php(3): WP_Query->parse_query()
#2 /wordpress/wp-includes/class-wp-query.php(13): WP_Query->get_posts()
#3 /wordpress/wp-includes/class-wp.php(3): WP_Query->query(Array)
#4 /wordpress/wp-includes/class-wp.php(3): WP->query_posts()
#5 /wordpress/wp-includes/functions.php(2): WP->main('')
#6 /wordpress/wp-blog-header.php(2): wp()
#7 /wordpress/index.php(2): require('/wordpress/wp-b...')
#8 {main}
thrown in /wordpress/wp-includes/class-wp-query.php on line 3

These types of ?feed[]=admin requests happen regularly, often from those attempting to scan sites for vulnerabilities. No theme or plugin changes were made. No settings or code of any kind were done. It is WordPress core not validating data structures before using them.

As a general principle WordPress core code should be stable and resilient. This change helps accomplish that goal.

[dmsnell]

I agree with @jrf that it's valuable to surface errors like these, especially if they are mistakes by developers. I also agree with @xknown and @josephscott that mistakes or malicious probing should not break a website. In any case where a site owner doesn't have control over the code, I don't think WordPress should punish them for something a develop or attacker did.

Is this not why we have _doing_it_wrong()? I could easily imagine adding a method to parse the argument type, which allows us to declare the expected type of each argument, and that method could notify developers while allowing sites to continue in potentially-degraded form.

I will point out that the proposed patch is not introducing any change in how invalid values are handled. The diff view makes it look bigger because of the required whitespace changes from the linting rules, but this patch merely continues examining parameters that previously were overlooked, following the exact pattern. So if we say it's wrong to validate, we should probably open a ticket and propose changing what was proposed in [53891].

[ironprogrammer]

+1 for this update. This ticket relates to site requests that are not the result of user or developer mistakes, and which are likely automated and can occur at high volume. I feel it's a benefit to the WordPress community to address this.

As Dennis mentioned, validation for other params has been added previously to WP_Query::parse_query, and the proposed changes augment that validation and lend support to the ​documented expectations of this class.

Another consideration is that as of today, ​over 50% of reporting sites are running PHP 7.4 or lower, where this situation results in a 404 and logs an error (try this URL for a Playground test under PHP 7.4: ​https://playground.wordpress.net/?php=7.4&wp=6.6&url=/?author_name[]=admin). But as more sites gradually move to PHP 8+ (which ​we encourage), this issue will become more prevalent and draw greater attention. I think it's better for WordPress to proactively get ahead of this.

[dd32]

I tend to agree that this "doesn't belong in WP_Query" but the only better place is within the WP class before it passes parameters to WP_Query.
Due to the how WP_Query is used in the wild, often passing query variables into it, it doesn't make sense to me to put it into WP.

There's a loooong history of requests to resolve these notices, then warnings, and now fatals. #17737 is the primary one I can find.

Most of the scalar-only query_vars were handled in [53891], but that hasn't added any "validate the array-only items are arrays". Some of the arrays that only accept ID's are 'protected' via wp_parse_id_list() deeper in.

That all being said; I feel like if the answer was "This shouldn't be fixed in WP_Query" then WP_Query should return a WP_Error for invalid inputs, but [53891] has already been merged which can be used as a good reason to add array-validation here too.

Another one worth looking at for inspiration, is ​WP_Tax_Query::clean_query().

I'm milestoning this for 6.7, because I'm not seeing a good reason not to move forward with the PR in some form or another.

[peterwilsoncc]

I've come around to putting this in WP_Query in some form.

With the introduction of the query block, it's possible for user input to be passed to WP_Query in an unexpected form that will also result in a fatal error due through no fault of the developers. From memory, the block query builder protects against it but doing validation at a lower level can't hurt.

[peterwilsoncc]

I'm moving this to the next release and marking it for early attention as the changes are significant enough to make me nervous modifying WP_Query to such an extent at this stage of the release cycle.