Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#37437 closed defect (bug) (fixed)

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

Reported by: stephdau's profile stephdau Owned by: rmccue's profile rmccue
Milestone: 4.6 Priority: high
Severity: normal Version: 4.6
Component: HTTP API Keywords: has-unit-tests has-patch
Focuses: Cc:

Description

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.

Attachments (2)

37437.diff (1.6 KB) - added by ocean90 8 years ago.
37437.2.diff (4.2 KB) - added by ocean90 8 years ago.

Download all attachments as: .zip

Change History (14)

#1 @rmccue
8 years ago

  • Milestone changed from Awaiting Review to 4.6
  • Owner set to rmccue
  • Priority changed from normal to high
  • Status changed from new to assigned

#2 @stephdau
8 years ago

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.

#3 follow-up: @stephdau
8 years ago

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.

Last edited 8 years ago by stephdau (previous) (diff)

#4 in reply to: ↑ 3 @rmccue
8 years ago

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.

#5 follow-up: @ocean90
8 years ago

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

#6 in reply to: ↑ 5 @stephdau
8 years ago

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
8 years ago

#7 @ocean90
8 years ago

  • Keywords has-unit-tests added

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

@ocean90
8 years ago

#8 @ocean90
8 years ago

  • Keywords has-patch added; needs-patch removed

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.

#9 @stephdau
8 years ago

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.

#10 @rmccue
8 years ago

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

#11 @ocean90
8 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

#12 @stephdau
8 years ago

:happy_dance:

Note: See TracTickets for help on using tickets.