Cookies with numeric/integer names cause exception.

[nosilver4u]

Summary

As reported in https://core.trac.wordpress.org/ticket/58566 when a cookie with a numeric name is used in a request, an exception is thrown. Though a cookie with a numeric name seems odd to me, it is permitted by the spec, and happens "in the wild".

Given the following code sample

$result = WpOrg\Requests\Requests::post(
        'https://example.com/submit/',
        array(),
        array(  
                'test_verify' => 'abc123',
        ),      
        array(  
                'cookies' => array(
                        'stuff' => 'good one!',
                        5       => 'stuff',
                )       
        )       
);

I'd expect the following behaviour

The request to succeed.

Instead this happened

PHP Fatal error:  Uncaught WpOrg\Requests\Exception\InvalidArgument: WpOrg\Requests\Cookie::parse(): Argument #2 ($name) must be of type string, integer given in rmccue/requests/src/Exception/InvalidArgument.php:29
Stack trace:
#0 rmccue/requests/src/Cookie.php(422): WpOrg\Requests\Exception\InvalidArgument::create(2, '$name', 'string', 'integer')
#1 rmccue/requests/src/Cookie/Jar.php(61): WpOrg\Requests\Cookie::parse('stuff', 5)
#2 rmccue/requests/src/Cookie/Jar.php(156): WpOrg\Requests\Cookie\Jar->normalize_cookie('stuff', 5)
#3 rmccue/requests/src/Hooks.php(93): WpOrg\Requests\Cookie\Jar->before_request(Object(WpOrg\Requests\Iri), Array, Array, 'POST', Array)
#4 rmccue/requests/src/Requests.php(455): WpOrg\Requests\Hooks->dispatch('requests.before...', Array)
#5 rmccue/requests/src/Requests.php(342): WpOrg\Requests\Requests::request('https://example...', Array, Array, 'POST', Array)
#6 functions.php(82): WpOrg\Requests\Requests::post('https://example...', Array, Array, Array)
...
#13 {main}
  thrown in /Users/sbishop/Downloads/ewwwio-silo/vendor/rmccue/requests/src/Exception/InvalidArgument.php on line 29

Additional context

To my knowledge, this affects several plugins making async requests in WordPress, including EWWW Image Optimizer and WooCommerce, and also possibly WP core (site health check).

Your environment

Environment	Answer
Operating system and version (e.g. Ubuntu 20.04, Windows 10)	Linux (Ubuntu)/MacOS 13.6.1
PHP version	8.0/8.2
Requests version	2.0.9
Autoloader used	Composer autoload (for testing, but also affects WordPress)

Tested against develop branch?

• [X] I have verified the issue still exists in the develop branch of Requests.

[nosilver4u]

On Trac, I had originally proposed casting the array keys to strings, but apparently PHP will just convert them back to integers, so the fix is either to cast the the cookie name to a string when calling the constructor for Cookie() (https://github.com/WordPress/Requests/blob/422612952ff3bd5163039f8889eaaaab95a432eb/src/Cookie.php#L82), or just modify the check in __construct() to this:

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

[jrfnl]

Though a cookie with a numeric name seems odd to me, it is permitted by the spec,

Please back up this statement as I cannot, for the life of me, find where in the specs it allows for an integer name.

As far as I can see, the name should always be a string, though a numeric string would be allowed.

[nosilver4u]

Agreed, the spec doesn't specifically state integer names are allowed. https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#Directives says it should be ASCII:

A cookie-name can contain any US-ASCII characters except for: control characters (ASCII characters 0 up to 31 and ASCII character 127) or separator characters (space, tab and the characters: ( ) < > @ , ; : \ " / [ ] ? = { })

The trouble is, even if one changes the numeric value to a string (in my example), the error is thrown, because PHP converts numeric array indices to integers (if possible). If I knew which WP plugin was setting such odd cookies, I'd beg them to stop. But it can and does happen, and it's technically permissible, so they might just say, "not a bug, go away..."

*Nevermind the unit tests, I think I figured that out, just had to learn some new things in the phpunit docs!

[jrfnl]

I've added two extra test cases too as the test update did check that integers were now being accepted, but did not test that strings which didn't comply with RFC 2616 were correctly being rejected.

[jrfnl]

Oops... ☝🏻 Sorry, that comment should have been posted on the PR. My bad. (too many open tabs)