Week query variable is not being sanitized correctly

[domainsupport]

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

[audrasjb]

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

[audrasjb]

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

[domainsupport]

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

[peterwilsoncc]

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?

[domainsupport]

That would be perfect!

[audrasjb]

Yeah, that's perfect 👌

[domainsupport]

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

[domainsupport]

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

[xParham]

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

[domainsupport]

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

Oliver

[domainsupport]

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.

[domainsupport]

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

[domainsupport]

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

[audrasjb]

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

[peterwilsoncc]

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.

[domainsupport]

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

[peterwilsoncc]

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.

[domainsupport]

Done.

Please see attached patch.

What do you think?

Oliver

https://core.trac.wordpress.org/ticket/56311

[peterwilsoncc]

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.

[domainsupport]

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

[peterwilsoncc]

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.

[costdev]

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.

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

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?

[domainsupport]

Any chance we can get this moving ... ? Still seeing notices in our log when sites are being scanned / attacked by bad actors ...

[22-Sep-2024 09:00:57 UTC] E_USER_NOTICE: Function WP_Date_Query was called incorrectly. Invalid value 46680 for week. Expected value should be between 1 and 53. Please see Debugging in WordPress for more information. (This message was added in version 4.1.0.) in /wp-includes/functions.php on line 6085
[22-Sep-2024 09:00:58 UTC] E_USER_NOTICE: Function WP_Date_Query was called incorrectly. Invalid value 46680 for week. Expected value should be between 1 and 53. Please see Debugging in WordPress for more information. (This message was added in version 4.1.0.) in /wp-includes/functions.php on line 6085
[22-Sep-2024 19:38:59 UTC] E_USER_NOTICE: Function WP_Date_Query was called incorrectly. Invalid value 125272522 for month. Expected value should be between 1 and 12. Please see Debugging in WordPress for more information. (This message was added in version 4.1.0.) in /wp-includes/functions.php on line 6085

Thanks.