Make WordPress Core

Opened 3 years ago

Closed 22 months ago

Last modified 22 months ago

#52252 closed defect (bug) (fixed)

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

Reported by: dd32's profile dd32 Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 6.1 Priority: normal
Severity: normal Version: 4.3
Component: Rewrite Rules Keywords: has-patch early needs-unit-tests commit
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 3 years ago.
adding array_key_exists check

Download all attachments as: .zip

Change History (31)

#1 @SergeyBiryukov
3 years ago

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

@ovidiul
3 years ago

adding array_key_exists check

#2 @ovidiul
3 years ago

  • Keywords has-patch added; needs-patch removed

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


3 years ago

#4 @johnbillion
3 years ago

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

#5 @johnbillion
3 years ago

  • Version set to 4.3

#6 @johnbillion
3 years 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
3 years 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
3 years 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
3 years ago

  • Keywords needs-unit-tests added

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


3 years ago
#11

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

#12 follow-up: @peterwilsoncc
3 years 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
3 years 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 years ago

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


3 years ago

#16 @desrosj
3 years 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.


3 years ago

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


3 years ago

#19 @tremidkhar
3 years ago

Tested the patch and it seems to be working.

Last edited 2 years ago by tremidkhar (previous) (diff)

#20 @desrosj
3 years 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.

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


3 years ago

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


3 years ago

#23 @hellofromTonya
3 years ago

  • Milestone changed from 5.9 to Future Release

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.

#24 @johnbillion
2 years ago

  • Owner johnbillion deleted

#25 @peterwilsoncc
22 months ago

  • Keywords commit added
  • Milestone changed from Future Release to 6.1
  • Owner set to peterwilsoncc
  • Status changed from reviewing to accepted

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.

#26 @peterwilsoncc
22 months ago

  • Component changed from Query to Rewrite Rules

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

#27 @peterwilsoncc
22 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

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.

#29 @SergeyBiryukov
22 months ago

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.

#30 @TimothyBlynJacobs
22 months ago

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

Note: See TracTickets for help on using tickets.