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