Make WordPress Core

Opened 6 months ago

Closed 6 months ago

Last modified 4 months ago

#60059 closed defect (bug) (fixed)

Warning / Error in wp-includes/canonical.php when $_GET['author'] is an array

Reported by: davidbinda's profile david.binda Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.5 Priority: normal
Severity: normal Version: 6.5
Component: Canonical Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

On a WordPress install with pretty permalinks turned on, there is a PHP warning or error (depends on the PHP version) when viewing an author on frontend, while the $_GET['author'] is an array. For instance:

/author/canonical-author/?author[1]=hello

produces following fatal error on PHP 8+:

Fatal error: Uncaught TypeError: preg_match(): Argument #2 ($subject) must be of type string, array given

and following warning on lower versions of PHP:

Warning: preg_match() expects parameter 2 to be string, array given

Related code line: https://core.trac.wordpress.org/browser/trunk/src/wp-includes/canonical.php?rev=56738#L319

A fix seems to be to check if is_string( $_GET['author'] ) before performing the preg_match.

Attachments (1)

60059.diff (1.2 KB) - added by david.binda 6 months ago.

Download all attachments as: .zip

Change History (9)

@david.binda
6 months ago

#1 @antonvlasenko
6 months ago

I don't have the time to test this now, but I have 2 concerns:

  1. I'm wondering why there is an array in the GET parameters. Did it work like that before? If not (i.e. is this a recent bug), what caused an array to appear in the GET parameters?

A fix seems to be to check if is_string( $_GET['author'] ) before performing the preg_match.

  1. In the example you provided, hello seems to be a valid author name. If this bug was not caused by some recent change, perhaps it's worth considering getting the first element of the author array and using it as an author name (in case it's a string), as opposed to ignoring arrays.

#2 @antonvlasenko
6 months ago

  • Keywords needs-testing reporter-feedback added

#3 @david.binda
6 months ago

I'm wondering why there is an array in the GET parameters. Did it work like that before? If not (i.e. is this a recent bug), what caused an array to appear in the GET parameters?

The concern raised in terms of this ticket is the PHP 8 compatibility of the code. While the code under PHP 7 "works" and "just" produces a warning while displaying the author page (ignoring the GET param), under PHP 8 it produces a fatal error.

So it's not the functionality of an array passed to the author GET param, sorry for not being clear.

#4 follow-up: @azaozz
6 months ago

Makes sense to check for and ignore arrays there but don't seem to be able to reproduce this (in trunk). Unless I'm missing something it seems a non-scalar query value for author is ignored and is_author() returns false. See: https://core.trac.wordpress.org/browser/tags/6.4.2/src/wp-includes/class-wp-query.php#L825.

#5 in reply to: ↑ 4 @SergeyBiryukov
6 months ago

Replying to azaozz:

Makes sense to check for and ignore arrays there but don't seem to be able to reproduce this (in trunk). Unless I'm missing something it seems a non-scalar query value for author is ignored and is_author() returns false.

I can reproduce the issue as described.

It appears that the preg_match( '|^[0-9]+$|', $_GET['author'] ) check in redirect_canonical() runs after the validation in WP_Query::parse_query(), and is unaffected by that validation because it checks the $_GET['author'] value directly, not the parsed value.

#6 @SergeyBiryukov
6 months ago

  • Milestone changed from Awaiting Review to 6.5
  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

#7 @SergeyBiryukov
6 months ago

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

In 57232:

Canonical: Check if the author parameter is a string in redirect_canonical().

This avoids a PHP warning or error when viewing an author on the front end, while an array is passed as $_GET['author'].

Follow-up to [12034], [12040], [12202].

Props david.binda, antonvlasenko, azaozz, SergeyBiryukov.
Fixes #60059.

#8 @ironprogrammer
6 months ago

  • Keywords has-patch has-unit-tests added; needs-testing reporter-feedback removed

Though this has already been merged (thanks, @davidbinda and @SergeyBiryukov!), I wanted to share separate confirmation that the patch addresses the issue.

Test Report

Patch tested: attachment:60059.diff

Environment

  • Hardware: MacBook Pro Apple M1 Pro
  • OS: macOS 14.2.1
  • Browser: Safari 17.2.1
  • Server: nginx/1.25.3
  • PHP: 8.2.13
  • WordPress: 6.5-alpha-56966-src
  • Theme: twentytwentyfour v1.0

Actual Results

  • ✅ Viewing /author/admin/?author[1]=hello (or variations) does not produce errors outlined in ticket description.
Note: See TracTickets for help on using tickets.