Requests_Cookie::parse() unable to parse WP_Http_Cookie object coming from WP_Http::request()

[stephdau]

If you pass an array of cookie objects via WP_Http::request()'s cookie arg, which is the format it expects for cookies, Requests_Cookie::parse() will fail, as it only expects to parse strings.

Example:

    $response = wp_remote_get(
        'https://myawesome.site' . $path,
        array(
            'cookies' => array(
                new WP_Http_Cookie( array( 'name' => AUTH_COOKIE, 'value' => wp_generate_auth_cookie( $user_id, $expiration, 'auth' ) ) ),
                new WP_Http_Cookie( array( 'name' => SECURE_AUTH_COOKIE, 'value' => wp_generate_auth_cookie( $user_id, $expiration, 'secure_auth' ) ) ),
                new WP_Http_Cookie( array( 'name' => LOGGED_IN_COOKIE, 'value' => wp_generate_auth_cookie( $user_id, $expiration, 'logged_in' ) ) ),
            ),
            'headers' => array(
                'Host' => $hostname
            ),
            'sslverify' => false,
        )
    );

Will lead to:

Warning: explode() expects parameter 2 to be string, object given in ~/public_html/wp-includes/Requests/Cookie.php on line 383
wp-includes/Requests/Cookie.php:383 explode(),
wp-includes/Requests/Cookie/Jar.php:43 parse(),
wp-includes/Requests/Cookie/Jar.php:144 normalize_cookie(), before_request(),
wp-includes/Requests/Hooks.php:62 call_user_func_array(),
wp-includes/class-requests.php:365 dispatch(),
wp-includes/class-http.php:369 request(),
wp-includes/class-http.php:566 request(),
wp-includes/http.php:170 get(),
wp_remote_get(), 

From:

class Requests_Cookie {
    [...]
    public static function parse($string, $name = '', $reference_time = null) {
        $parts = explode(';', $string);

Not submitting a patch because there are many ways to handle this one, which should be decided.

[stephdau]

Note: I wasn’t sure how we’d prefer to fix it: take the value key out of the object if it’s a WP_Http_Cookie instance. Or do so higher up (normalize_cookies()?) and just have Requests_Cookie::parse() fail if not a string, etc.

[stephdau]

Also, the value of auth cookies shouldn't really hit that parse() method as stored in the object in the 1st place, since their format does not match. Hence my thoughts about "fixing" it higher (but that explode() call should be hardened anyways).

If you look at the structure of the method's code, at the moment, there are more related things that have not yet been handled, like:

elseif (strpos($kvparts, '=') === false) {

or

        else {
            list($name, $value) = explode('=', $kvparts, 2);
        }

which would fail if $parts is empty.

Or if (!empty($parts)) {, further down, having no else clause, leading to return new Requests_Cookie($name, $value, $attributes, array(), $reference_time); potentially being called without the expected values.

So, one thing that would help me provide a patch would be decisions on how we would like the parse() method to gracefully fail when it does not receive the data it requires to provide a proper output. I have no directions to base such a decision myself in that method, at the moment, besides arbitrarily returning a WP_Error object, which other functions/methods are currently not written to handle higher up.

[rmccue]

I think we need to add a normalize_cookies() to WP_Http that maps WP_Http_Cookie instances to Requests_Cookie. We can then pass in an array of these instances instead.

Replying to stephdau:

Also, the value of auth cookies shouldn't really hit that parse() method as stored in the object in the 1st place, since their format does not match. Hence my thoughts about "fixing" it higher (but that explode() call should be hardened anyways).

The parse() call is used when receiving cookies (hence the static-ness), we shouldn't need it when sending. We can just instantiate Requests_Cookie directly instead.

[ocean90]

@stephdau Do you have time to work on a patch which adds normalize_cookies() to WP_Http?

[stephdau]

Replying to ocean90:

@stephdau Do you have time to work on a patch which adds normalize_cookies() to WP_Http?

In the middle of a lot of things (isn't everybody? :) ), but I'll see what I can do.

[ocean90]

37437.diff​ adds two tests which passes in 4.5 but not in trunk.

[ocean90]

37437.2.diff​:

• Converts WP_Http_Cookie() and key => value to Requests_Cookie.
• Adds WP_Http_Cookie::get_attributes() to retrieve the cookie attributes which is used when converting WP_Http_Cookie() to Requests_Cookie.
• Adds isset() checks to WP_HTTP_Requests_Response::get_cookies() for cookie attributes.

[stephdau]

Awesome! Yours is better than what I had.

I've just tested it on a vanilla local install, as well as on WPCOM, and all seems to be working as intended.

[rmccue]

@ocean90 Patch looks good I think, but I'd break it out into a new function rather than keeping it inline.

[ocean90]

In 38164:

HTTP API: Normalize cookies before passing them to Requests.

Requests has its own cookie object in form of Requests_Cookie. Therefore we have to convert WP_Http_Cookie objects to Requests_Cookie.
This introduces WP_Http_Cookie::get_attributes() to retrieve cookie attributes of a WP_Http_Cookie object and WP_Http::normalize_cookies() to convert the cookie objects.

Fixes #37437.

[stephdau]

:happy_dance: