t/request.t fails due to change in HTTP::XSCookies?

[whosgonna]

t/request.t fails with Use of uninitialized value $value in split at /root/.cpanm/work/1520488903.54892/Dancer2-0.205002/lib/Dancer2/Core/Request.pm line 599.

t/request.t sets cookies as follows:
HTTP_COOKIE => 'cookie.a=foo=bar; cookie.b=1234abcd; no.value.cookie;',
Where no.value.cookie looks to be an intentionally bad value, with a possible expectation that HTTP::XSCookies will discard it (see #1087) .

Given the following snippet:

use strict;
use warnings;
use Data::Dumper;
use HTTP::XSCookies 'crush_cookie';

my $str = 'cookie.a=foo=bar; cookie.b=1234abcd; no.value.cookie;'; #undef; #'lalala';
my $cookies = crush_cookie($str);

print Dumper $cookies;

Using HTTP::XSCookies version 0.000014 will return:

$VAR1 = {
          'cookie.a' => 'foo=bar',
          'cookie.b' => '1234abcd'
        };

And using HTTP::XSCookies version 0.000016 will return:

$VAR1 = {
          'cookie.b' => '1234abcd',
          'no.value.cookie' => undef,
          'cookie.a' => 'foo=bar'
        };

The change in HTTP::XSCookies is from issue crush_cookie bug? #5

I'm not entirely sure if it is more appropriate to open the request here or against HTTP::XSCookies, but the documentation for the crush_cookies sub says, "Parse a (properly encoded) cookie string into a hashref with the individual values" which would indicate that it shouldn't be used to validate the cookie string.

[gonzus]

Hey, sorry about the breakage. Your diagnosis is spot on.

From a technical POV, it seems to me the new behaviour is more comprehensive, since it allows the caller to know whether a value was set or not in a cookie. It also supports no-value fields like HttpOnly and Secure (which I think were just being skipped before, OMG!).

OTOH, I am also willing to accept that we should follow the standard, should there be a clear definition for this.

What do you think? What would be best?

• summons @SysPete *

[whosgonna]

I agree - very clearly HttpOnly and Secure should be supported without values. I do think that this problem is in Dancer2::Core::Request. I'm don't know:

• How Dancer2 would react if there was an HttpOnly or Secure field used. Even if HTTP::XSCookie were updated to only allow HttpOnly and Secure without values (I think those are the only defined valueless fields in the RFC) would Dancer2 still fail?
• What type of failure Dancer2 would encounter if it received the improperly formatted cookie? If the entire application crashes, etc, then it is a fairly critical issue to fix. If the request just gets a 5xx response, is that a problem? It would seem to me that 5xx is a valid response for an improper request.

I think there are people more qualified than I am to weigh in on this issue, though.

[gonzus]

IOW, this shows the upside of the change:

my $str = 'cookie.a=foo=bar; cookie.b=1234abcd; HttpOnly; no.value.cookie;';
...
$VAR1 = {
          'HttpOnly' => undef,
          'no.value.cookie' => undef,
          'cookie.a' => 'foo=bar',
          'cookie.b' => '1234abcd'
        };

If those undef values are a problem, maybe we could keep the new behaviour but only for known no-value fields (HttpOnly and Secure, AFAIK)? I still believe it is technically better to receive those undefs, it lets you choose what to do about them.

[veryrusty]

RFC6265 provides the grammars for cookies; they are different for Cookie and Set-Cookie headers.

@gonzus would having crush_cookie handle Cookie headers, and another method, say crush_set_cookie handle the Set-Cookie headers be a reasonable approach ?

Note that one downside of including HttpOnly => undef in the hashref returned by crush_set_cookie, is that crush_set_cookie and bake_cookie don't round-trip; HttpOnly is boolean, undef is considered false. It may be preferable to have the attribute-values (such as HttpOnly) be a "true" value.

[bigpresh]

Cookie headers should not contain HttpOnly etc, should they? I'm fairly sure they only make sense in a Set-Cookie header, i.e. when the server is telling the client to set a cookie and how to use it, and don't make sense when the client is sending a cookie back to the server?

[bigpresh]

Also, RFC 6265 says:

Output the cookie's name, the %x3D ("=") character, and the cookie's value.

To me, that says that a cookie name, alone, without an equals sign, is not valid. cookiename= may be valid, for a cookie that exists but doesn't have a value, maybe?

[gonzus]

How about we allow passing an optional parameter to crush_cookie that will tell it to put those undef values for any items with no value? By default it could skip those (which seems to be what the standard AND Dancer2 need). Thoughts?

[whosgonna]

Would this change in Dancer2::Core::Request within _build_cookie work:

598a599,602
>         unless (defined $value) {
>             delete $cookies->{$name};
>             next;
>         }

Alternately, what would be the impact of returning a boolean instead of undef from HTTP::XSCookies here?

[gonzus]

I liked the idea of returning an explicit undef, to differentiate HttpOnly=0;foo=bar from HttpOnly;foo=bar.

Anyway, I already implemented the optional parameter, with a default according to what Dancer2 needs. Would that suit everyone?

[gonzus]

I uploaded HTTP-XSCookies-0.000017.tar.gz, containing these fixes.

[eserte]

Dancer2's t/cookie.t fails with HTTP-XSCookies-0.000017:

#   Failed test at t/cookie.t line 188.
#     Structures begin differing at:
#          $got->[0] = 'ARRAY(0x46a23f18)'
#     $expected->[0] = 'bar'

#   Failed test at t/cookie.t line 188.
#     Structures begin differing at:
#          $got->[0] = 'ARRAY(0x4448f3f0)'
#     $expected->[0] = 'bar'
# Looks like you failed 2 tests of 118.
t/cookie.t ............................................. 
Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/118 subtests 

[gonzus]

Can you provide the exact call to bake_cookie? I have a test t/20_cookie_baker_bake.t that looks like this (and is the closest I could find to t/cookie.t:188), and is successful.

[ 't140', 'foo', { value => [qw/bar baz/], Secure => 1 }, 'foo=bar%26baz; Secure' ],

[eserte]

A debugging session revealed the following:

• HTTP::XSCookies is called here in Dancer: https://github.com/PerlDancer/Dancer2/blob/master/lib/Dancer2/Core/Request.pm#L590
• the returned $cookies looks like this: { foo => ['bar', 'baz'] }
• but Dancer2 apparently does not expect the values to be arrayrefs and tries to split the stringified arrayref, see https://github.com/PerlDancer/Dancer2/blob/master/lib/Dancer2/Core/Request.pm#L601

[perlpunk]

@gonzus Can you please also update the Changes file in the future? I see no changes after 0.000005
https://metacpan.org/source/GONZUS/HTTP-XSCookies-0.000017/Changes

[gonzus]

@perlpunk LOL, I had forgotten that file even existed... :-) I will give it a go tomorrow.

[gonzus]

@eserte aha! Maybe Dancer2 could use is_arrayref() and take the value as is, or otherwise split it itself? I would much rather return the object myself.

[gonzus]

@perlpunk I just pushed the updated Changes file. Starting with the next release in CPAN, I'll remember to keep it up to date. Thanks!

[veryrusty]

@gonzus Any chance you could document that flag in the POD for HTTP::XSCookies ?
( I concur, the flag was a better option than a seperate method! )

D2 optionally uses HTTP::XSCookies as it provides XS speedups for both bake_cookie and crush_cookie. Otherwise Cookie::Baker is the default which only has XS speedups for crush_cookie.
I'll try and get some benchmarking done over the weekend; provided (as I expect) there is sufficient speed gain from having crush_cookie do the splitting of the cookie value, I'd be happy for D2 to accomodate getting an arrayref back. If anyone else want to do that benchmarking, please feel free to do so :)

[gonzus]

@veryrusty Good idea, just did that, it will go out on the next release.