Make WordPress Core

Opened 17 months ago

Last modified 2 months ago

#56311 assigned defect (bug)

Week query variable is not being sanitized correctly

Reported by: domainsupport's profile domainsupport Owned by: audrasjb's profile audrasjb
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Query Keywords: has-patch needs-testing
Focuses: Cc:

Description

If you add a "w" query variable to the URL of a WordPress website, for example ...

http://localhost/?w=1234

... the following PHP error notice is generated ...

PHP Notice: Function WP_Date_Query was called <strong>incorrectly</strong>. Invalid value <code>1234</code> for <code>week</code>. Expected value should be between <code>1</code> and <code>53</code>. Please see <a href="https://wordpress.org/support/article/debugging-in-wordpress/">Debugging in WordPress</a> for more information. (This message was added in version 4.1.0.) in /wp-includes/functions.php on line 5831

This is because the w query string is not being correctly sanitized before being added to WP_Query. It should be checked that it is a number between 1 and 53 before being added to the query.

Oliver

Attachments (2)

56311.diff (799 bytes) - added by domainsupport 16 months ago.
56311.2.diff (3.1 KB) - added by domainsupport 11 months ago.

Download all attachments as: .zip

Change History (31)

#1 @audrasjb
17 months ago

  • Milestone changed from Awaiting Review to 6.1
  • Owner set to audrasjb
  • Status changed from new to assigned
  • Version trunk deleted

Good catch, I can reproduce the issue as well.

validate_date_values() should contain a weeks per year function, just like other date queries do.

See https://github.com/WordPress/wordpress-develop/blob/3fdce9eb4b13ab9e9f52c5636b730aaa43740b12/src/wp-includes/class-wp-date-query.php#L281

#2 @audrasjb
17 months ago

  • Keywords reporter-feedback added
  • Milestone changed from 6.1 to Awaiting Review

Hmm, by the way, throwing a _doing_it_wrong() message is expected here.
Just like ?day=1234 does for a Day query.

#3 @domainsupport
17 months ago

  • Keywords reporter-feedback removed

Yes, I noticed that at the same time whilst looking at that function ... but a PHP error notice shouldn't be generated from a third party injection of invalid date values in the query variables? (which is how we found this issue)

Surely the 404 page should be quietly shown without littering the debug.log?

Otherwise a PHP error notice would be shown every time a page / post slug or ID wasn't found?

Thanks,

Oliver

#4 @peterwilsoncc
16 months ago

There are two cases in which an out of range date value could trigger the error message:

  • a visitor to the site enters an out of range number in the request, ?w=1234, as mentioned above
  • a developer enters an out of range number in a custom query new WP_Query( [ 'w' => 1234 ] )

As the first case is out of the developers control, I think it should be checked in WP::parse_request() and trigger a File Not Found error as suggested. This avoids bloating the error logs with unfixable errors.

As the second case is within the developers control, I think logging an error serves a purpose and the they should continue to be logged.

Is that an acceptable resolution for each of you?

#5 @domainsupport
16 months ago

That would be perfect!

#6 @audrasjb
16 months ago

Yeah, that's perfect 👌

#7 @domainsupport
16 months ago

I may be wrong, but could this be very easily achieved by changing line 397 of /wp-includes/class-wp-date-query.php to ... ?

<?php
                                if ( ! isset($_GET[$key]) && ( ! is_numeric( $_value ) || ! $is_between ) ) {

If so, would you like me to provide a patch?

Thanks,

Oliver

#8 @domainsupport
16 months ago

Actually I've just seen that the $_GET keys don't all match the $date_query keys so actually that wouldn't work. I suggest replacing lines 387 -> 391 of /wp-includes/class-wp-date-query.php with this instead ... ?

<?php
                $date_variable_keys = array(
                        'month' => 'monthnum',
                        'week' => 'w',
                        'day' => 'day',
                        'hour' => 'hour',
                        'minute' => 'minute',
                        'second' => 'second'
                );

                // Concatenate and throw a notice for each invalid value.
                foreach ( $min_max_checks as $key => $check ) {
                        if ( 
                                ( isset($date_variable_keys[$key]) && ! isset($_GET[$date_variable_keys[$key]]) ) ||
                                ! array_key_exists( $key, $date_query )
                        ) {
                                continue;
                        }

If that's acceptable I can produce a patch ... ?

Thanks,

Oliver

#9 @xParham
16 months ago

I wanted to add that the same issue exists for the day parameter and perhaps for the other date parameters. Just recently, we have found a bot requesting some random URLs with parameters like ?type=day&day=2021-02-09, causing a lot of noise in the logs: Function WP_Date_Query was called <strong>incorrectly</strong>. Invalid value <code>2021</code> for <code>day</code>.

#10 @domainsupport
16 months ago

@xParham that’s precisely how I found this issue!! The code above should fix the problem for the day query variable too.

Oliver

#11 @domainsupport
16 months ago

Testing Instructions

I've added a patch to this ticket ...

Steps to Reproduce

  1. Go to /?day=1234 in any WordPress installation
  2. 🐞 Bug occurs.

Expected Results

After installing the patch

  • ✅ No PHP warning should be seen in the log when you visit ...

/?day=1234
/?w=1234
/?monthnum=1234
/?hour=1234
/?minute=1234
/?second=1234

When reproducing a bug:

  • PHP Notice: Function WP_Date_Query was called <strong>incorrectly</strong>. Invalid value <code>1234</code> for <code>week</code>. Expected value should be between <code>1</code> and <code>53</code>. Please see <a href="https://wordpress.org/support/article/debugging-in-wordpress/">Debugging in WordPress</a> for more information. (This message was added in version 4.1.0.) in /wp-includes/functions.php on line 5831

Test Report Icons:
🐞 <= Indicates where issue ("bug") occurs.
✅ <= Behavior is expected.
❌ <= Behavior is NOT expected.

#12 @domainsupport
13 months ago

This is still an issue in v6.1 ...

[14-Nov-2022 08:42:13 UTC] PHP Notice:  Function WP_Date_Query was called <strong>incorrectly</strong>. Invalid value <code>6029465263</code> for <code>week</code>. Expected value should be between <code>1</code> and <code>53</code>. Please see <a href="https://wordpress.org/support/article/debugging-in-wordpress/">Debugging in WordPress</a> for more information. (This message was added in version 4.1.0.) in /wp-includes/functions.php on line 5835

Oliver

#13 @domainsupport
12 months ago

Hi guys, just keeping this one alive ... still getting the notices in v6.1.1 ...

[16-Dec-2022 13:00:23 UTC] PHP Notice:  Function WP_Date_Query was called <strong>incorrectly</strong>. Invalid value <code>95404</code> for <code>week</code>. Expected value should be between <code>1</code> and <code>53</code>. Please see <a href="https://wordpress.org/support/article/debugging-in-wordpress/">Debugging in WordPress</a> for more information. (This message was added in version 4.1.0.) in /wp-includes/functions.php on line 5835
[16-Dec-2022 13:00:23 UTC] PHP Notice:  Function WP_Date_Query was called <strong>incorrectly</strong>. Invalid value <code>95404</code> for <code>week</code>. Expected value should be between <code>1</code> and <code>53</code>. Please see <a href="https://wordpress.org/support/article/debugging-in-wordpress/">Debugging in WordPress</a> for more information. (This message was added in version 4.1.0.) in /wp-includes/functions.php on line 5835

Oliver

#14 @johnbillion
12 months ago

  • Keywords has-patch needs-testing added
  • Milestone changed from Awaiting Review to Future Release

#15 @johnbillion
12 months ago

  • Milestone changed from Future Release to 6.2

#16 @audrasjb
12 months ago

Thanks for moving this to 6.2 John!
I'll give it a quick test and share a feedback soon.

#17 @peterwilsoncc
11 months ago

I'd rather validate this in src/wp-includes/class-wp.php than in the date query.

The WP class is intended to validate the user input via the URL whereas WP_Date_Query is intended as a developer API. Adding checks for $_GET values in the latter seems a little sub-optimal.

I think a method similar to WP_Date_Query::validate_date_values() could be created in the WP class to validate the query strings parameters. This can silently discard (or throw a 404 for) invalid values.

Unfortunately WP_Date_Query doesn't know if it's being called for the main query so it doesn't appear possible to mute the warnings for the main query only.

#18 @domainsupport
11 months ago

Hi @peterwilsoncc,

Are you saying that you would like the entire patched WP_Date_Query->validate_date_values() method relocated to the WP class or that you would like an additional method in WP class which would validate the date values for a second time?

The latter would still result in superfluous entires in the logs without checking for the existence of the corresponding $_GET element unless the notice in line 397 were moved to the WP method.

Oliver

#19 @peterwilsoncc
11 months ago

Yes, that's what I am thinking.

The WP class is used to validate user input (in this case via the URL). An example of this is ensuring post type queries are public. see source code

The WP class would then drop or modify any invalid date queries before they are passed to WP_Query which in turn would prevent them from being passed to WP_Date_Query.

My thought been that if a developer writes the code WP_Query( ['monthnum' => 36, /* etc */ ] ) then the notices ought to be thrown. If a visitor enters the URL /2023/36/15 then the WP class should handle the invalid data gracefully.

#20 @domainsupport
11 months ago

Done.

Please see attached patch.

What do you think?

Oliver

#22 @peterwilsoncc
11 months ago

Thanks Oliver, I'll review and test the code.

Before committing it to WordPress it will needs some additional tests to help ensure that it's error free. I've created a pull request with your code so I can add tests.

If you've got experience writing unit tests, feel free to create a PR on your account and I'll close mine off.

#23 @domainsupport
11 months ago

I've not run GitHub pull request unit tests on WordPress before and it looks like your tests have already run.

I see these failures ... not sure if I should address them here on on GitHub but ...

1) Tests_Canonical::test_canonical with data set #40 ('/2012/13/', '/2012/')
Failed asserting that two strings are identical.
2) Tests_Canonical::test_canonical with data set #41 ('/2012/11/51/', '/2012/11/', 0, array('WP_Date_Query'))
Failed asserting that two strings are identical.

I'm not sure these tests are relevant here? Or, if they are I guess if invalid data is found then we should be forcing a 404 to prevent duplicate content? Is that best done with handle_404() as well as or instead of unsetting the query_vars?

3) Tests_Query_Conditionals::test_bad_dates
4) Tests_Query_VerbosePageRules::test_bad_dates
Failed to assert that WP_Date_Query triggered an incorrect usage notice.

This is a good thing, right :)

Failed asserting that an array is empty.

Do we have more details for this?

Also, please delete line 439 as it seems to have slipped through the net and is not required ...

<?php
                $day_month_year_error_msg = '';

And thanks for adding in the missing [ $key ]!

Oliver

#24 @peterwilsoncc
11 months ago

Yeah, I noticed the failures.

It looks like there are a mix of assertions failing (ie, back-compat breaks) and expected errors not throwing (a good thing for user input). I'll need to figure it out.

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


10 months ago

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


10 months ago

#27 @costdev
10 months ago

  • Milestone changed from 6.2 to Future Release

This ticket was discussed in the bug scrub and it was agreed to move this ticket to Future Release.

@peterwilsoncc If the PR is ready in time, feel free to pull it back into the 6.2 milestone for commit.

Jankyz commented on PR #3821:


2 months ago
#28

Hi, how can I help with that? I've noticed a lot of errors in our organization related to this solution.

@peterwilsoncc commented on PR #3821:


2 months ago
#29

Hi, how can I help with that? I've noticed a lot of errors in our organization related to this solution.

Unfortunately it ended up breaking a canonical redirects, http://wp-dev.local/2023/09/51 is meant to redirect to http://wp-dev.local/2023/09/ but on this branch it no longer does.

I think what is needed is to prevent WP_Date_Query from logging notices on the main query (ie, the query formed by a site visitors request to an invalid URL) but continue to logging them for developers calling WP_Query with invalid date parameters.

Unfortunately, WP_Date_Query doesn't have access to whether it's the main query. This could be done by changing the constructor but it seems a little bit of overkill.

Are the errors you are seeing coming from visitors entering incorrect values in the URL or due to calls within your code to WP_Query?

Note: See TracTickets for help on using tickets.