Fix Undefined offset: 0

[e1111o]

No description provided.

[sagikazarmark]

Can you please provide some examples how you met this issue? Examining the code this should not be possible at all. array_filter reindexes the array and the empty check should catch the case when the first index does not exist.

[e1111o]

In the fromString method of Cookie/SetCookie.php, the return value of $pieces is as follows.

Array: 2 [
   1 => "path = /"
   2 => "HttpOnly"
]

An error occurred because the 0th array did not exist.
I encountered this error while creating and using the CookieJar class as shown below.

$cookie_jar = new CookieJar ();
$json = $ this-> client-> request ('POST', '[[URL]]',
             'Cookies' => $cookie_jar,
             'Headers' => $headers,
             ...
         ]);

[rtek]

Related: #1895

http://php.net/manual/en/function.array-filter.php

Iterates over each value in the array passing them to the callback function. If the callback function returns true, the current value from array is returned into the result array. Array keys are preserved.

[Nyholm]

It would be easy to write a test for this. Could you provide a test that is proving the correctness of this patch?

[e1111o]

Test cases added.
This is an error when passing the argument from the fromString method to ; path = /; HttpOnly.
The array_filter function removes the 0th index and accesses the 0th index of the $pieces array in the 40th line.

[sagikazarmark]

Array keys are preserved.

Oh my.... of course it does.

Thanks a lot for the fix. Will merge and tag once the build passes.

[Nyholm]

Thank you @e1111o. I like that you added a test. But your test does not prove the correctness of the patch.

Actually, your patch does not do anything with this input date.

https://3v4l.org/nD4l3

[e1111o]

@Nyholm Thank you for your care.
Yet, in this case, i think code should be $cookie = '; Path=/; HttpOnly';, not $cookie = 'path=/; Path=/; HttpOnly';

[Nyholm]

I think you miss the point. Im saying that the test you provided will pass with and without you changes to SetCookie.php.

I need you to provide a test that would fail without your changes to SetCookie.php. Such test will prove the correctness of your patch.

[sagikazarmark]

What's the status of this PR?

[e1111o]

Sorry. I have been busy with my company business these days and have not progressed since. I'll commit it soon after the fix.

[curtisdf]

I encountered this bug myself just now.

I've looked at the proposed changes in the aforementioned pull request, but I don't believe they address the real issue. This error is triggered when the first KVP (key-value pair) in a Set-Cookie string is missing. This KVP is pretty vital to the cookie because it provides the cookie's name and value. I'm not familiar with the relevant RFCs or other standards regarding cookie handling, but I would guess this isn't legal. But what action should Guzzle take in this case? It seems to me that a cookie without a name and value is dead on arrival and should be ignored.

The code in question already has a comment indicating that it expects the cookie to have that first KVP. See line 38:

    // The name of the cookie (first kvp) must include an equal sign.

The proposed fix only re-indexes the $pieces array by calling array_values() on it. But this obfuscates the fact that the cookie is malformed. It also doesn't clean up the inline comment, which would be obsoleted by such a fix.

So I believe this pull request should indeed be rejected, even without looking at the tests. I'm considering submitting my own pull request which would fix this more properly, if I can find time today or tomorrow.

[curtisdf]

I've submitted a pull request. See #1998. I saw that the function in question already returns an empty cookie object if the cookie is missing a name, so I made a trivial fix which also does that if the whole first key-value pair is missing.

For clarification, this bug is only triggered if the $cookie string begins with a leading semicolon (";") and has at least one additional key-value pair behind it.

[sagikazarmark]

Accepted the fix in #1998

Thanks for your contribution though