PHP Notice when `monthnum` query var is set without the `year` QV

[dd32]

E_NOTICE: Undefined index: year in wp-includes/rewrite.php:413 / E_NOTICE: Undefined index: monthnum in wp-includes/rewrite.php:413

It looks like [32648] assumes the permalink structures will always include both year & monthnum or monthnum & day https://core.trac.wordpress.org/browser/trunk/src/wp-includes/rewrite.php?marks=400-403#L393

But a request such as ?monthnum=1 will cause it to check for the year query var which might be unset.

(Props to the pentester hitting WordPress.org with many junk requests for bringing this to light)

[ovidiul]

adding array_key_exists check

[johnbillion]

I'm so far unable to reproduce this problem. Requesting ?monthnum=2 on my local installation doesn't trigger a notice. There's a guard condition at the beginning of wp_resolve_numeric_slug_conflicts() that protects against accessing the year, monthnum, or day indexes of $query_vars.

@dd32 Is anything else needed to trigger this notice? Is w.org running trunk?

[dd32]

Thanks for looking into it @johnbillion!

This notice was caused by a fuzzer hitting WordPress.org, 500+ query vars in a POST request against a news post, makes it hard to narrow down the cause, so sorry for the bad report.

There's a guard condition at the beginning of wp_resolve_numeric_slug_conflicts() that protects against accessing the year, monthnum, or day indexes of $query_vars.

The guard condition protects against the case of all 3 being not set, but not against situations where only one out of the expected vars is set.

Looking at the code, I can clearly see that on line 400 monthnum being set without year results in year being looked for as an index on line 413, It seems that this happens early enough that the normal plugins such as Query monitor & Debug Bar don't pick it up the notice.

Is anything else needed to trigger this notice? Is w.org running trunk?

It looks like to trigger it with year you need to have the permalink structure set to /%postname%/ and request https://example.com/?monthnum=1

It looks like to trigger it with monthnum you need to have the permalink structure set to /%year%/%postname%/ and request https://example.com/?day=1

I can't see that day is possible as a notice here, only year/monthnum. I'm guessing I either copy-paste wrong or caused the day case in testing.

I pared it back as much as possible, and here's some debugging output for the year case:

wp-includes/rewrite.php

@@ -412 +412 @@
         // This is the potentially clashing slug.
         $value = $query_vars[ $compare ];
 
+var_dump( compact( 'query_vars', 'permastructs', 'postname_index', 'compare', 'value' ) ); die();
         $post = get_page_by_path( $value, OBJECT, 'post' );
         if ( ! ( $post instanceof WP_Post ) ) {
                 return $query_vars;

results in:

GET https://wordpress.org/?monthnum=1

( ! ) Notice: Undefined index: year in wp-includes/rewrite.php on line 413
Call Stack
#	Time	Memory	Function	Location
1	0.0002	369824	{main}( )	.../index.php:0
2	0.0002	372720	require( 'wp-blog-header.php )	.../index.php:26
3	0.2574	6280864	wp( $query_vars = ??? )	.../wp-blog-header.php:16
4	0.2574	6280896	WP->main( $query_args = '' )	.../functions.php:1291
5	0.2574	6280896	WP->parse_request( $extra_query_vars = '' )	.../class-wp.php:750
6	0.2579	6304120	wp_resolve_numeric_slug_conflicts( $query_vars = ['monthnum' => '1'] )	.../class-wp.php:360

wp-includes/rewrite.php:415:
array (size=5)
  'query_vars' => 
    array (size=1)
      'monthnum' => string '1' (length=1)
  'permastructs' => 
    array (size=1)
      0 => string '%postname%' (length=10)
  'postname_index' => int 0
  'compare' => string 'year' (length=4)
  'value' => null

[SergeyBiryukov]

Related: #45513

[johnbillion]

Confirmed. Moving back to 5.7. That condition does indeed only guard against none of the three indexes being present.

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

[peterwilsoncc]

With RC this week, I'm inclined to move this to 5.8 early to avoid making Query related changes 2 days out.

I've started on some tests in the linked ​pull request but at this point don't have any assertions figured out. It's showing the notices.

[dd32]

Replying to peterwilsoncc:

I'm inclined to move this to 5.8 early to avoid making Query related changes 2 days out.

I agree, this isn't urgent and has existed for long enough not to rush any kind of fix.

[desrosj]

Adjusting the keywords here to more accurately represent the state of tests. The PR touches a PHPUnit file, which triggers the has-unit-tests keyword, but the needed assertions are not yet added.

[tremidkhar]

Tested the patch and it seems to be working.

[desrosj]

Unfortunately this one has run out of time as 5.8 beta 1 is being packaged in less than an hour. Punting to 5.9.

[hellofromTonya]

Query changes by design need careful consideration and plenty of soak time to ensure nothing breaks. With 5.9 feature freeze ~2 days away, it's too late in the release cycle for a change to query. Punting this out of 5.9. As the next milestone is not yet available, moving to Future Release. Once available, feel free to move into the next release.

[peterwilsoncc]

I've updated the tests in ​the linked pull request and merged in @dd32's patch.

The tests are weird because they assert true is true (which is true), however they're dummy tests as the real test is to assert that there are no notices thrown.

In Tests_Privacy_wpPrivacyDeleteOldExportFiles::test_non_existent_folders_should_not_cause_errors() there is a similar test that does the same thing. Thus I decided to follow precedent rather than add an annotation marking the test not risky.

I think this is good for commit.

[peterwilsoncc]

Moving this to the Rewrite Rules component as that is where the notice is thrown and the fix applied.

[peterwilsoncc]

In 53857:

Rewrite rules: Prevent malformed date requests throwing notices.

Ensure that date queries contain each component before attempting to use them. This prevents http requests triggering notices if a portion of the date is missing. For example a request containing a day and year but no month, a request containing a month bu not year.

Props ovidiul, dd32, peterwilsoncc, costdev, johnbillion, SergeyBiryukov, desrosj, tremidkhar, hellofromTonya.
Fixes #52252.

Merged https://core.trac.wordpress.org/changeset/53857 / 3e866f8b8f

[SergeyBiryukov]

In 53861:

Tests: Improve the test for not throwing a warning on malformed date queries.

• Make it specifically about wp_resolve_numeric_slug_conflicts(), the function that was throwing an Undefined array key "year" PHP warning for malformed date requests.
• Move the test under the rewrite component and make its name a bit more descriptive.
• Check the return result of the function instead of performing a dummy assertion.
• Use named array keys in the data provider for clarity.
• Add missing @covers tag.

Follow-up to [32648], [53857].

Props costdev, peterwilsoncc, 1naveengiri, mukesh27, SergeyBiryukov.
See #52252, #45513.

[TimothyBlynJacobs]

Instead of assertTrue( true ) we could potentially use addToAssertionCount( 1 ). Not sure if that is preferred, but thought I'd mention it.