Make WordPress Core

Opened 12 months ago

Closed 5 months ago

Last modified 5 months ago

#58566 closed defect (bug) (fixed)

WP_Http::normalize_cookies does not convert cookie names to strings

Reported by: nosilver4u's profile nosilver4u Owned by: swissspidy's profile swissspidy
Milestone: 6.5 Priority: normal
Severity: normal Version: 6.2.2
Component: HTTP API Keywords: has-patch has-unit-tests commit
Focuses: Cc:

Description

We ran into an issue where, if a cookie with an integer name is submitted via wp_remote_post(), it triggers a fatal error/exception in Requests\Cookie::construct(). Looking at the stack trace, the cookies go through WP_Http::normalize_cookies(), which already casts the values to strings. Unfortunately, it just sends the cookie names straight through rather than casting the names to strings as is required by Requests\Cookies::construct().

Partial stack trace here:

PHP Fatal error: Uncaught WpOrg\Requests\Exception\InvalidArgument: WpOrg\Requests\Cookie::__construct(): Argument #1 ($name) must be of type string, integer given in [...]public_html/wp-includes/Requests/src/Exception/InvalidArgument.php:29
Stack trace:
#0 [...]public_html/wp-includes/Requests/src/Cookie.php(83): WpOrg\Requests\Exception\InvalidArgument::create(1, '$name', 'string', 'integer')
#1 [...]public_html/wp-includes/class-wp-http.php(472): WpOrg\Requests\Cookie->__construct(135370, '')
#2 [...]public_html/wp-includes/class-wp-http.php(352): WP_Http::normalize_cookies(Array)
#3 [...]public_html/wp-includes/class-wp-http.php(616): WP_Http->request('https://allegor...', Array)
#4 [...]public_html/wp-includes/http.php(179): WP_Http->post('https://allegor...', Array)
#5 [...]public_html/wp-content/plugins/ewww-image-optimizer/common.php(11271): wp_remote_post('https://allegor...', Array)

Change History (13)

#1 @nosilver4u
12 months ago

This bug also seems to be affecting the Site Health page in certain cases: https://wordpress.org/support/topic/php-fatal-error-class-wp-site-health-php/

#2 @engahmeds3ed
9 months ago

To add more context here, plz check this PHP core feature/bug :) here: https://github.com/php/php-src/issues/9029

So if we even try to cast the cookie key to be string, this won't happen because PHP will detect that this key is numeric and change it back to integer so the error will occur in all cases.

I used the following code:

<?php
add_filter( 'http_request_args', function( $args ){
    if ( empty( $args['cookies'] ) || ! is_array( $args['cookies'] ) ) {
        return $args;
    }
    
    foreach ( $args['cookies'] as $cookie_key => $cookie_value ) {
        if ( is_string( $cookie_key ) ) {
            continue;
        }

        unset( $args['cookies'][$cookie_key] );
    }

    return $args;
}, 1000 );

Just to remove the integer cookie keys not to fall in this WP fatal error trap :)

Can we simply remove the thrown exception from here:
https://github.com/WordPress/wordpress-develop/blob/b6b6ded79a54535b9ea80ae9c395dfa90262a4f9/src/wp-includes/Requests/src/Cookie.php#L83-L85

What do u think?

#3 @nosilver4u
9 months ago

Ah, that's a good catch on the PHP "feature", I hadn't thought of that happening. So there's no sense in casting the array indices to string, since they won't be preserved.
Instead, the Cookie() constructor should accept string OR integer values for the $name parameter. I think it's good for to be validating the "input", so I don't think dropping the exception would be appropriate.

#4 @barry.hughes
9 months ago

Can we simply remove the thrown exception from here:

https://github.com/WordPress/wordpress-develop/blob/b6b6ded79a54535b9ea80ae9c395dfa90262a4f9/src/wp-includes/Requests/src/Cookie.php#L83-L85

Or widen it to allow integers? That would still offer some protection (against something truly unexpected, like a bool or object) while still accommodating both valid key types.

if ( is_string( $name ) === false && is_integer( $name ) === false ) {
        throw InvalidArgument::create( 1, '$name', 'string|int', gettype( $name ) );
}

#5 follow-up: @schlessera
7 months ago

The above should be fixed by adding (string) casts to the first argument of the Cookie constructor in lines 470 & 472 here:

https://github.com/WordPress/wordpress-develop/blob/ac4f768222ee9583357d44059df6c61940f7b2a5/src/wp-includes/class-wp-http.php#L470-L472

This ticket was mentioned in PR #5836 on WordPress/wordpress-develop by darssen.


6 months ago
#6

  • Keywords has-patch has-unit-tests added

This pull request addresses an issue encountered when integer-named cookies are submitted using wp_remote_post and wp_remote_get . The underlying problem stems from the Requests\Cookie::construct() method, which requires cookie names to be of type string.

The primary change in this pull request is the addition casting within the WP_Http::normalize_cookies() function. This update explicitly casts cookie names to strings.

Unit tests have been added to test the offending scenario.

#7 in reply to: ↑ 5 @darssen
6 months ago

The above should be fixed by adding (string) casts to the first argument of the Cookie constructor in lines 470 & 472 here:

https://github.com/WordPress/wordpress-develop/blob/ac4f768222ee9583357d44059df6c61940f7b2a5/src/wp-includes/class-wp-http.php#L470-L472

I created a PR with the changes suggested above and added a couple of unit tests for it.

#8 @kraftbj
6 months ago

  • Keywords commit added

I left a review on the PR. The PR itself looks good to me. I have a question about the unit test, but not enough of a concern to suggest blocking if others are happy.

#9 @kraftbj
6 months ago

My concerns were unfounded so the hesitancy mentioned above is not merited. I support this PR.

#10 @swissspidy
5 months ago

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

In 57501:

HTTP API: Ensure cookie names are cast to strings.

Props nosilver4u, darssen, kraftbj, engahmeds3ed, barry.hughes, schlessera.
Fixes #58566.

#11 @swissspidy
5 months ago

  • Milestone changed from Awaiting Review to 6.5

#13 @engahmeds3ed
5 months ago

Many thanks, I tested it and it's working properly, nice job.

Note: See TracTickets for help on using tickets.