Make WordPress Core

Opened 4 months ago

Last modified 6 hours ago

#60745 new defect (bug)

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

Reported by: xknown's profile xknown Owned by:
Milestone: 6.7 Priority: normal
Severity: normal Version:
Component: Query Keywords: has-patch has-unit-tests php80
Focuses: Cc:

Description

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

Change History (8)

This ticket was mentioned in PR #6246 on WordPress/wordpress-develop by @xknown.


4 months ago
#1

  • Keywords has-patch has-unit-tests added

Add is_scalar/is_array checks.

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

#2 @xknown
3 months ago

  • Keywords php80 added

#3 @jrf
3 months ago

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

#4 @peterwilsoncc
3 months ago

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.

#5 @josephscott
14 hours ago

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

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.

#6 @dmsnell
13 hours ago

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

#7 @ironprogrammer
7 hours ago

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

#8 @dd32
6 hours ago

  • Milestone changed from Awaiting Review to 6.7

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.

Note: See TracTickets for help on using tickets.