Fatal error when dispatching Async request with not valid cookies

[engahmeds3ed]

Steps to reproduce:

1. Write a very simple code to dispatch Async task that fires for example with admin_init
2. Open WP admin dashboard.
3. Open developer tools > Application tab > Cookies
4. Add a cookie with integer key something like:

5. Open admin dashboard and refresh any admin page.
6. Make sure that u have at least WP 5.9
7. You will see the following fatal error in your log:

PHP Fatal error: Uncaught WpOrg\Requests\Exception\InvalidArgument: WpOrg\Requests\Cookie::__construct(): Argument #1 ($name) must be of type string, integer given in /wp-includes/Requests/src/Exception/InvalidArgument.php:29#012Stack trace:#012#0 /wp-includes/Requests/src/Cookie.php(84): WpOrg\Requests\Exception\InvalidArgument::create(1, '$name', 'string', 'integer')#012#1 /wp-includes/class-wp-http.php(472): WpOrg\Requests\Cookie->__construct(14, '')#012#2 /wp-includes/class-wp-http.php(352): WP_Http::normalize_cookies(Array)#012#3 /wp-includes/class-wp-http.php(616): WP_Http->request('https://xxx...', Array)#012#4 /wp-includes/http.php(186): WP_Http->post('https://xxx...', Array)

Root cause:

This is happening because we pass the cookies without any sanitization/validation here:
https://github.com/deliciousbrains/wp-background-processing/blob/master/classes/wp-async-request.php#L145

I can create a pull request for that change once u check and validate the bug itself.

Related to woocommerce/action-scheduler#985

[engahmeds3ed]

@ianmjones sorry for bothering, just want to validate the issue and I'll create the PR.

[ianmjones]

Hey @engahmeds3ed, thanks for the ping.

While it seems like it would be best fixed in WordPress Core itself, that could take a while, and wouldn't make it's way to existing WordPress versions.

If you have a nice simple fix, and can include unit tests, I'd definitely be open to reviewing it.

[engahmeds3ed]

Sure I can, will back to you once finishing that.

[kldsr]

@engahmeds3ed thank you for your suggested fix

@engahmeds3ed @ianmjones what do you think of this updated version of original fix proposed by @engahmeds3ed:

add_filter( 'http_request_args', function( $args ) {
    if ( empty( $args['cookies'] ) || ! is_array( $args['cookies'] ) ) {
        return $args;
    }
    
    $sanitized_cookies = [];
    foreach ( $args['cookies'] as $cookie_key => $cookie_value ) {
        $new_key = is_string( $cookie_key ) ? $cookie_key : strval( $cookie_key );
        $new_value = is_string( $cookie_value ) ? $cookie_value : strval( $cookie_value );

        $sanitized_cookies[$new_key] = $new_value;
    }

    $args['cookies'] = $sanitized_cookies;

    return $args;
}, 1000 );

[engahmeds3ed]

Thanks @tdmkld I already started adding the idea of this code here and will create the pull request by tomorrow so cross fingers that this will fix it.
We don't need to use the filter http_request_args here, we will apply the logic directly to the cookies array.
Many thanks for your hard work on that.

[engahmeds3ed]

Unfortunately because of this bug/feature in php core we can't change the numeric key to a string key, check this simple example here: https://onlinephp.io/c/7f6d6

I believe removing the integer keys will be the only solution here, I'll add a comment there in WP Core ticket, hope they fix it from their side.

[ianmjones]

I did wonder how you were going to get around how PHP automatically casts a string that looks like an integer into an integer when used as an associative array key.

The only real way around that is to prefix the integer "string" with a non-empty string value that makes it non-numeric, and remove the prefix later during analysis.

I personally feel that the real solution here isn't so much to get WP's Cookie class to allow a numeric cookie name, but to simply not use a numeric as a cookie name.

You're playing with fire if you ever use an array where the integer keys have significance anyway, it is very easy to use a function that resets the keys back to start from 0.

To my mind, using a cookie name that is an integer without any identifiable source is just asking for trouble, cookie names are in a global namespace per domain/path.

You can use a cookie name that is a real string unique to your plugin that holds an array with ID/Value/Sort values etc, or construct cookie names with a string prefix and trailing integer value, e.g. myplugin-item-123.

Probably best to hunt down whatever WordPress plugin is creating the cookies with integer names and politely ask the author to adjust their cookie names.

[engahmeds3ed]

Many thanks @ianmjones for this valuable discussion.

Probably best to hunt down whatever WordPress plugin is creating the cookies with integer names and politely ask the author to adjust their cookie names.

That'd be hard as cookies can be set with plugins/theme/js and I hope I can see the error in reality without applying this workaround.

[kldsr]

@ianmjones @engahmeds3ed thank you for these replies and valuable insights. Would the best course of action for us (the Wordpress website owners experiencing the 500's thrown by multiple plugins that bundle ActionScheduler) be to wait for the various plugin developers to address the issue on a per plugin basis and wait for new versions of these plugins to be released? Please advise.

[engahmeds3ed]

@tdmkld
Personally I'd suggest using the snippet to remove the integer keys cookies for now till finding out which plugin are doing so.

Waiting for those plugins to fix their code is not an option for me because we need to guard against that because if anyone with admin dashboard access can cause this error 500 with just creating a cookie with integer key in the browser.

[kldsr]

@engahmeds3ed thank you for that advice, is there any danger in using the snippet to remove these integer key cookies? I'm hesitant to remove them because I don't know why or how they are being generated in the first place, are they serving a purpose?