Fatal error when integer cookie key is added for loggedin user

[engahmeds3ed]

Steps to reproduce:

1. Login as an admin.
2. Open developer tools > Application tab > Cookies
3. Add a cookie with integer key something like:
   
4. Make sure that you have Action Scheduler installed and activated.
5. Open admin dashboard and wait few minutes.
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 as it without any sanitization here:

action-scheduler/lib/WP_Async_Request.php

Line 154 in 44b4fde

'cookies' => $_COOKIE,

Do u think that we need to sanitize it here? or it's out of AS responsibility?
Thanks.

[barryhughes]

Wow, great catch...

Or it's out of AS responsibility?

... And a great question. It's not obvious, because it doesn't live in the vendor directory, but we're actually using a third party library here (WP Background Processing). So, it feels like we should fix this upstream. I'm a little unsure, though, if in this case that means WP Background Processing or if it means WordPress, but I can see a bug report has already been logged for WordPress so perhaps we should wait on the outcome there.

We could also patch this ourselves, as a temporary measure, but that's less ideal. May I ask, did you find this by accident/after exploration of your own, or are real-world users encountering the problem?

[engahmeds3ed]

Many thanks @barryhughes (u r the best)

it doesn't live in the vendor directory, but we're actually using a third party library here (WP Background Processing)

Yes I know that and we already used this package before using Action Scheduler, and I was thinking of opening an issue there.

May I ask, did you find this by accident/after exploration of your own, or are real-world users encountering the problem?
It's with our customers, till now we got about 5 customers and the errors appear randomly, till now I don't know what exactly sets the cookie key with integer value.

We solved it temporary using the following helper code:

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] );
        $args['cookies'][ (string) $cookie_key ] = $cookie_value;
    }

    return $args;
}, 1000 );

I will open an issue and even create a PR there at https://github.com/deliciousbrains/wp-background-processing hope things go faster there and then will notify you here so we can update that here too.

Thanks.

[barryhughes]

Excellent, thank you @engahmeds3ed 👍🏼

[barryhughes]

Placing on hold while we watch for the outcome of the following upstream reports:

• Fatal error when dispatching Async request with not valid cookies deliciousbrains/wp-background-processing#107
• https://core.trac.wordpress.org/ticket/58566

[tdmkld]

@barryhughes

May I ask, did you find this by accident/after exploration of your own, or are real-world users encountering the problem?

Real-world users are encountering the problem, my team and I are an example of users in the wild seeing this problem, our Wordpress website uses third-party plugins that are bundled with ActionScheduler. New Relic alerted us to a series of daily intermittent 500 fatal errors, some identical to the one posted to this thread by @engahmeds3ed and some referencing other plugins bundled with ActionScheduler. We contacted the plugins' support teams and @engahmeds3ed was kind enough to raise the issue here, thank you @engahmeds3ed and @barryhughes

[tdmkld]

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] );
        $args['cookies'][ (string) $cookie_key ] = $cookie_value;
    }

    return $args;
}, 1000 );

@engahmeds3ed should we add this helper code to our Wordpress functions.php file?

[engahmeds3ed]

@tdmkld
You can add this snippet directly to WordPress using any snippets plugin or inside your active theme functions.php or create a dedicated plugin for it.

After doing some tests, I found that removing those integer keys will be better so you can try using the following snippet:

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

Plz try testing it in a staging site before going live.

[tdmkld]

@engahmeds3ed thank you for the updated snippet, much appreciated.

• What dangers, if any, are posed by removing all cookies with keys that are not strings from the $args['cookies'] array?
• I read above how you were able to reproduce the error, should I use the same workflow?

[tdmkld]

@engahmeds3ed I have created an alternative filter that converts any cookie key or value that is not a string into a string representation of that value. I have been testing it in our staging environment:

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

[tdmkld]

@tdmkld You can add this snippet directly to WordPress using any snippets plugin or inside your active theme functions.php or create a dedicated plugin for it.

After doing some tests, I found that removing those integer keys will be better so you can try using the following snippet:

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

Plz try testing it in a staging site before going live.

@engahmeds3ed I've added the hook above as a custom plugin, we've been monitoring our sites' logs all afternoon, the fatal errors involving cookies with integer keys have not resurfaced and as far as I can tell this solution has not introduced any new errors, well done and many thanks!

[barryhughes]

Looks like a fix should land in WP 6.5. Let's keep this open (for visibility) until then—after that we can close.