Make WordPress Core

Opened 23 months ago

Closed 23 months ago

Last modified 23 months ago

#56336 closed defect (bug) (fixed)

Fatal Error/Warning when invalid `sitemap` query var presented.

Reported by: dd32's profile dd32 Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 6.1 Priority: normal
Severity: normal Version:
Component: Sitemaps Keywords: php8 has-patch has-unit-tests
Focuses: Cc:

Description

A request such as https://example.org/?sitemap[foo]=bar will result in a fatal error being generated under PHP8, and a warning under PHP7.

PHP 7.4:
E_WARNING: Illegal offset type in isset or empty in wp-includes/sitemaps/class-wp-sitemaps-registry.php:68


PHP8.1:
Fatal error: Uncaught Error: Illegal offset type in isset or empty
in wp-includes/sitemaps/class-wp-sitemaps-registry.php on line 68

Call stack:
WP_Sitemaps_Registry::get_provider()
wp-includes/sitemaps.php:114
get_sitemap_url()
wp-includes/canonical.php:428
redirect_canonical()
wp-includes/class-wp-hook.php:307
...

This is partially related to #17737

Attachments (1)

56336.diff (1.0 KB) - added by costdev 23 months ago.
Add type checks to WP_Sitemaps_Registry::get_provider() and taxonomy_exists().

Download all attachments as: .zip

Change History (9)

#1 @costdev
23 months ago

  • Keywords reporter-feedback added

A quick attempt to reproduce this by appending ?sitemap[foo]=bar to the home url on a local (alpha, PHP 7.2) and remote (6.0.1, PHP 7.4) site failed to produce a warning/error.

Are there any other settings/steps required to produce this?

Edit: Nevermind, got it. Hadn't realised the sitemap plugin was deactivated.

Last edited 23 months ago by costdev (previous) (diff)

#2 @costdev
23 months ago

  • Keywords reporter-feedback removed

I'm getting the warning for illegal offset type when appending ?sitemap[foo]=bar, but in my case it's wp-includes/taxonomy.php:

Warning: Illegal offset type in isset or empty in src/wp-includes/taxonomy.php on line 340

Looks like both may need an is_string() check on the value being passed to isset(). Both are documented as a string.

Last edited 23 months ago by costdev (previous) (diff)

@costdev
23 months ago

Add type checks to WP_Sitemaps_Registry::get_provider() and taxonomy_exists().

#3 @dd32
23 months ago

Thanks @costdev!

Adding the type check to WP_Sitemaps_Registry::get_provider() appears to work for me, the taxonomy_exists() change didn't seem to be required in my environment but I can't see any harm in it or reason not to include it in the same change.

This ticket was mentioned in PR #3065 on WordPress/wordpress-develop by costdev.


23 months ago
#4

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

#5 @peterwilsoncc
23 months ago

  • Owner set to peterwilsoncc
  • Resolution set to fixed
  • Status changed from new to closed

In 53838:

Sitemaps: Prevent invalid provider names throwing errors.

Validate the requested sitemap is a string before attempting to use it in a provider. This prevents WP_Sitemaps_Registry::get_provider() from triggering a fatal error in more recent versions of PHP.

The errors can be triggered by items outside the site owner or developers control (such as a user visiting ?sitemap[foo]=bar) so the code fails silently to avoid filling error logs with unfixable errors.

Props costdev, dd32.
Fixes #56336.

#7 @peterwilsoncc
23 months ago

  • Milestone changed from Awaiting Review to 6.1

#8 @audrasjb
23 months ago

In 53869:

Taxonomy: Prevent non string taxonomy names generating warnings or errors.

This changeset adds an is_string( $taxonomy ) check to the condition in taxonomy_exists(), to ensure false is returned when the $taxonomy is not a string.

Follow-up to [35718].

Props costdev, peterwilsoncc, mukesh27.
Fixes #56338.
See #56336.

Note: See TracTickets for help on using tickets.