Invalid cookie headers being returned

[Ovid]

Hi there,

I think this might belong against HTTP::Headers::Fast, but it's popping up in a client's Plack stack, so I thought I would start here first. This is a small test case:

#!/usr/bin/env perl

use Test::Most;
use Plack::Response;
use Plack::Test;

my $response = Plack::Response->new(200);
$response->content('Hello World');
$response->cookies->{foo} = {
    value   => 'test',
    expires => time + 24 * 60 * 60,
    secure  => 1,
};
$response->cookies->{bar} = {
    value   => 'test2',
    expires => time + 24 * 60 * 60,
    secure  => 1,
};
$response->headers->push_header( 'Set-Cookie', 'foo=that' );

# traditional - named params
test_psgi
  app    => $response->to_app,
  client => sub {
    my $cb  = shift;
    my $req = HTTP::Request->new( GET => "http://localhost/hello" );
    my $res = $cb->($req);
    like $res->content, qr/Hello World/;
    explain scalar $res->headers->header('Set-Cookie');
  };

done_testing;

That final line prints:

foo=that, bar=test2; expires=Wed, 06-Jul-2022 12:27:13 GMT; secure, foo=test; expires=Wed, 06-Jul-2022 12:27:13 GMT; secure

Per the IETF spec, we have a couple of violations:

• Section 3

Origin servers SHOULD NOT fold multiple Set-Cookie header fields into a single header field. The usual mechanism for folding HTTP headers fields (i.e., as defined in [RFC2616]) might change the semantics of the Set-Cookie header field because the %x2C (",") character is used by Set-Cookie in a way that conflicts with such folding.

As a consequence of the above, in the last sentence of section 4.1.2, we find the following:

User agents ignore unrecognized cookie attributes (but not the entire cookie).

Because the header fields are joined on a comma, we have an invalid secure, attribute, which suggests that strict cookie parsers might accept the cookie, but ignore the strict attribute. This might be a serious security concern.

• Section 4.1.1

Servers SHOULD NOT include more than one Set-Cookie header field in the same response with the same cookie-name. (See Section 5.2 for how user agents handle this case.)

In the above example, we have the cookie foo being set twice, with different values and attributes. This caused a serious authentication issue in our client's code.

Admittedly, this code is being used extensively and I'm unsure about a decent approach to solving it.

[haarg]

It's not clear what a fix to this could even be. Combining the headers into one only happens on the final step when requesting the Set-Cookie header as a single entity. There is no other way that could be returned without throwing away data or throwing an error. Possibly a documentation note that not all headers can be represented as a single value.

[Ovid]

@haarg the header spec allows this:

Set-Cookie: bar=test2; expires=Wed, 06-Jul-2022 12:27:13 GMT; secure
Set-Cookie: foo=test; expires=Wed, 06-Jul-2022 12:27:13 GMT; secure

So long as each cookie-name is unique, it's valid.

So maybe warn if cookies are fetched in scalar context, or provide a new method which canonically returns multiple Set-Cookie lines?

[haarg]

The step that combines the headers into a single entry is the final scalar $res->headers->header('Set-Cookie') call. Until this, they are kept as separate headers. And if you use list context, you correctly get the separate values, not combined with a comma. So a warning could be issued, but there isn't really anything else it could be doing.

[neilb]

Looking at the code for HTTP::Headers::header(), you could do something like:

# after initial arg check
my $want_set_cookie = @_ == 1 && lc($_[0]) eq 'set-cookie';
...
# before the final join
Carp::croak("multiple cookies cannot be folded into a single Set-Cookie: header") if $want_set_cookie;

I don't know if there are other field names that should apply to?