WordPress.org

Make WordPress Core

Opened 6 months ago

Last modified 13 days ago

#52252 reviewing defect (bug)

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

Reported by: dd32 Owned by: johnbillion
Milestone: 5.9 Priority: normal
Severity: normal Version: 4.3
Component: Query Keywords: has-patch early needs-unit-tests
Focuses: Cc:

Description (last modified by 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)

Attachments (1)

52252.patch (571 bytes) - added by ovidiul 5 months ago.
adding array_key_exists check

Download all attachments as: .zip

Change History (21)

#1 @SergeyBiryukov
6 months ago

  • Keywords good-first-bug added
  • Milestone changed from Awaiting Review to 5.7

@ovidiul
5 months ago

adding array_key_exists check

#2 @ovidiul
5 months ago

  • Keywords has-patch added; needs-patch removed

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


4 months ago

#4 @johnbillion
4 months ago

  • Owner set to johnbillion
  • Status changed from new to reviewing

#5 @johnbillion
4 months ago

  • Version set to 4.3

#6 @johnbillion
4 months ago

  • Keywords reporter-feedback added; good-first-bug removed
  • Milestone changed from 5.7 to Future Release

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?

#7 @dd32
4 months ago

  • Description modified (diff)
  • Keywords reporter-feedback removed

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

     
    412412        // This is the potentially clashing slug.
    413413        $value = $query_vars[ $compare ];
    414414
     415var_dump( compact( 'query_vars', 'permastructs', 'postname_index', 'compare', 'value' ) ); die();
    415416        $post = get_page_by_path( $value, OBJECT, 'post' );
    416417        if ( ! ( $post instanceof WP_Post ) ) {
    417418                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

#9 @johnbillion
4 months ago

  • Milestone changed from Future Release to 5.7

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

#10 @johnbillion
4 months ago

  • Keywords needs-unit-tests added

This ticket was mentioned in PR #1027 on WordPress/wordpress-develop by peterwilsoncc.


4 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

#12 follow-up: @peterwilsoncc
4 months ago

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.

#13 in reply to: ↑ 12 @dd32
4 months ago

  • Keywords early added
  • Milestone changed from 5.7 to 5.8

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.

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


3 months ago

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


2 months ago

#16 @desrosj
2 months ago

  • Keywords needs-unit-tests added; has-unit-tests removed

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.

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


2 months ago

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


8 weeks ago

#19 @tremidkhar
8 weeks ago

Tested and the patch and it seems to be working.

#20 @desrosj
13 days ago

  • Milestone changed from 5.8 to 5.9

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.

Note: See TracTickets for help on using tickets.