Make WordPress Core

Opened 10 years ago

Closed 10 years ago

Last modified 8 years ago

#29736 closed enhancement (fixed)

Proposal to check for is_scalar() in WP_Query::fill_query_vars()

Reported by: tivnet's profile tivnet Owned by: boonebgorges's profile boonebgorges
Milestone: 4.1 Priority: normal
Severity: normal Version:
Component: Query Keywords: commit
Focuses: Cc:

Description

The Symptom:

PHP Warning: strlen() expects parameter 1 to be string, array given in .../wp-includes/query.php on line 1577

The line:

if ( ! empty( $qv['s'] ) && strlen( $qv['s'] ) > 1600 )

The cause (hacker's script):

http://www.example.com/?s[]=something

The 's' becomes an array, causing a sequence of warnings

My quick patch in the core:

if ( ! is_scalar( $array['s'] ) ) {
	$array['s'] = '';
}

A patch outside the core, until there is a upgrade:

/** Hooked to 'parse_request' */
function action_parse_request(WP $query_args) {
	if ( ! is_scalar( $query_args->query_vars['s'] ) ) {
		$query_args->query_vars['s'] = '';
	}
}

The proposal:

To check all (or most of) variables in the fill_query_vars()'s first loop for is_scalar()

Attachments (1)

29736.patch (1.8 KB) - added by boonebgorges 10 years ago.

Download all attachments as: .zip

Change History (6)

#1 @tivnet
10 years ago

Of course, isset or ! empty should be checked before is_scalar, for example:

if ( isset( $query_args->query_vars['s'] ) && ! is_scalar( $query_args->query_vars['s'] ) ) {
        $query_args->query_vars['s'] = '';
}

#2 @boonebgorges
10 years ago

  • Keywords commit added
  • Milestone changed from Awaiting Review to 4.1

Yes, the change looks good. 29736.patch makes it more concise and adds unit tests.

Of course, isset or ! empty should be checked before is_scalar,

This shouldn't be necessary. By the time we've gotten to this point in parse_query(), the query vars have either been run through fill_query_vars(), which guarantees that 's' will be isset(). An equivalent ! empty() check is done later on: https://core.trac.wordpress.org/browser/tags/4.0/src/wp-includes/query.php#L2655

@boonebgorges
10 years ago

#3 @boonebgorges
10 years ago

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

In 29912:

Check that search value is scalar before parsing.

Prevents PHP notices when non-scalar values are passed.

Includes unit tests.

Props tivnet.
Fixes #29736.

#5 @swissspidy
8 years ago

For some reason the condition added in [29912] does not work on HHVM as the tests fail because of a strlen() expects parameter 1 to be string, array given error

Note: See TracTickets for help on using tickets.