WP_Http::normalize_cookies does not convert cookie names to strings

[nosilver4u]

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)

[nosilver4u]

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/

[engahmeds3ed]

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?

[nosilver4u]

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.

[barry.hughes]

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 ) );
}

[schlessera]

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 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.

[darssen]

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.

[kraftbj]

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.

[kraftbj]

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

[swissspidy]

In 57501:

HTTP API: Ensure cookie names are cast to strings.

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

Committed in https://core.trac.wordpress.org/changeset/57501

[engahmeds3ed]

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