Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cookies with numeric/integer names cause exception. #845

Open
1 task done
nosilver4u opened this issue Nov 30, 2023 · 3 comments · May be fixed by #847
Open
1 task done

Cookies with numeric/integer names cause exception. #845

nosilver4u opened this issue Nov 30, 2023 · 3 comments · May be fixed by #847

Comments

@nosilver4u
Copy link

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?

  • I have verified the issue still exists in the develop branch of Requests.
@nosilver4u
Copy link
Author

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() (

public function __construct($name, $value, $attributes = [], $flags = [], $reference_time = null) {
), 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 ) );
}

nosilver4u added a commit to nosilver4u/Requests that referenced this issue Nov 30, 2023
Fixes WordPress#845 so that an exception isn't thrown for legitimate numeric cookie names.
@jrfnl
Copy link
Member

jrfnl commented Nov 30, 2023

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
Copy link
Author

nosilver4u commented Nov 30, 2023

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!

@nosilver4u nosilver4u linked a pull request Dec 2, 2023 that will close this issue
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants