Add tests for multiple cookies.

[ioquatix]

Types of Changes

• New feature.

Contribution

• I added tests for my changes.
• I tested my changes locally.
• I agree to the Developer's Certificate of Origin 1.1.

[ioquatix]

@MSP-Greg @dentarg is it just me, or is Falcon the only server doing the correct thing here?

Multiple cookie headers should be combined using a semi-colon, right? or am I halucinating?

[ioquatix]

According to https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cookie:

A list of name-value pairs in the form of =. Pairs in the list are separated by a semicolon and a space ('; ').

IIRC, HTTP/1 user agents are supposed to combine all cookies in a single header, however HTTP/2, for the sake of HPACK, recommends separate cookie headers for each cookie: https://datatracker.ietf.org/doc/html/rfc9113#name-compressing-the-cookie-head

[ioquatix]

@jeremyevans do we have any specific guidance in Rack on how to combine cookie headers into HTTP_COOKIE?

The only guidance I can see is:

If multiple header fields with the same field-name are received then the server MUST rewrite them as a single value having the same semantics.

from https://datatracker.ietf.org/doc/html/rfc3875#section-4.1.18

[jeremyevans]

RFC 6265 Section 5.4 states that browsers should not submit more than one Cookie header: https://www.rfc-editor.org/rfc/rfc6265#section-5.4 . Since it isn't allowed by the RFC, I don't think Rack SPEC should mention how to handle that case.

[ioquatix]

RFC 6265 Section 5.4 states "When the user agent generates an HTTP request, the user agent MUST NOT attach more than one Cookie header field."

RFC9113 doesn't change this semantic but does allow multiple cookie headers on the wire:

To allow for better compression efficiency, the Cookie header field MAY be split into separate header fields, each with one or more cookie-pairs. If there are multiple Cookie header fields after decompression, these MUST be concatenated into a single octet string using the two-octet delimiter of 0x3b, 0x20 (the ASCII string "; ") before being passed into a non-HTTP/2 context, such as an HTTP/1.1 connection, or a generic HTTP server application.

Therefore, the following two lists of Cookie header fields are semantically equivalent.

cookie: a=b; c=d; e=f

cookie: a=b
cookie: c=d
cookie: e=f

Therefore, HTTP/2 user agents may emit multiple cookie headers. I agree, this doesn't need to be specified by the spec, however, what servers are currently doing - concatenating cookies using a comma, is wrong. They should either reject the request or concatenate the cookies correctly. Doing otherwise may be a potential security issue.

[MSP-Greg]

what servers are currently doing - concatenating cookies using a comma, is wrong.

Sort of. Both specs imply that an HTTP/1.1 (non-HTTP/2 context) servers should only accept a 'single octet string'.

So, what should non-HTTP/2 servers do? Fail the connection, ignore all cookies lines but the first, or combine them as an HTTP/2 server should?

[ioquatix]

According to the CGI specification:

The header data can be presented as sent by the client, or can be rewritten in ways which do not change its semantics. If multiple header fields with the same field-name are received then the server MUST rewrite them as a single value having the same semantics.

In other words, it's definitely wrong for the server to combine them using a comma, and that could change the semantics of the field, which is in violation of the CGI RFC.

So, what should non-HTTP/2 servers do? Fail the connection, ignore all cookies lines but the first, or combine them as an HTTP/2 server should?

It seems reasonable to me that servers should be robust enough to handle non-compliant requests.

In practice, servers should combine multiple cookie headers into a single header value, separated by semi-colons, rather than rejecting the request outright.

Maybe we can survey other web servers to see what they do?

[ioquatix]

There is a good discussion here from Apache: https://bz.apache.org/bugzilla/show_bug.cgi?id=63434

[ioquatix]

It looks like comma-separated cookie headers might be semantically equivalent, given that:

-- RFC2616
       token          = 1*<any CHAR except CTLs or separators>
       separators     = "(" | ")" | "<" | ">" | "@"
                      | "," | ";" | ":" | "\" | <">
                      | "/" | "[" | "]" | "?" | "="
                      | "{" | "}" | SP | HT

-- RFC6265:
 cookie-pair       = cookie-name "=" cookie-value
 cookie-name       = token
 cookie-value      = *cookie-octet / ( DQUOTE *cookie-octet DQUOTE )
 cookie-octet      = %x21 / %x23-2B / %x2D-3A / %x3C-5B / %x5D-7E
                       ; US-ASCII characters excluding CTLs,
                       ; whitespace DQUOTE, comma, semicolon,
                       ; and backslash
 token             = <token, defined in [[RFC2616], Section 2.2](https://www.rfc-editor.org/rfc/rfc2616#section-2.2)>

Therefore, neither cookie-name nor cookie-value can contain commas. Therefore, it might be safe to split on commas.

[ioquatix]

In summary, unless you use Falcon, a user agent that submits a request to any other server tested here, with multiple cookie headers, will fail to parse correctly by Rack::Request#cookies.

I see three options:

• Servers reject such requests (400 bad request).
• Servers combine the cookie headers using semi-colons.
• Servers combine the cookie header using commas, and Rack::Request.cookies splits based on /[,;]/.

[MSP-Greg]

Instead of /[,;]/, maybe /, |;/? Can't recall if a comma is ok, or it must be a comma & space? I just started on the Apache thread, not even sure how long it is...

[ioquatix]

I don't think it should be sensitive to whitespace, but at least it is one option to consider some kind of "what works in practice", i.e. ", " might be a slightly more robust, non-standard separator. We are not talking about what to do in the correct case, but what to do in the incorrect cases that are presented to us. If we don't want to emit errors, we might need to be more tolerant of how we parse the input.

[MSP-Greg]

Not sure what are considered allowable characters in either a cookie key or value (as in <key>=<value>).

I haven't checked unicorn, but I don't recall many (if any) Puma issues related to cookies.

but what to do in the incorrect cases

The expectation that servers will properly handle malicious or malformed requests, and also handle any failure in upstream code is certainly my idea of fun...

[ioquatix]

This test suite includes test for unicorn and you can see it's failing.

ile test/rack/conform/cookies.rb it can respond with multiple cookie headers test/rack/conform/cookies.rb:37
	expect 200 to
		be == 200
			✓ assertion passed test/rack/conform/cookies.rb:40
	expect #<Protocol::HTTP::Headers [["Date", "Thu, 06 Jun 2024 16:48:13 GMT"], ["Connection", "close"], ["x-http-cookie", "a=1,b=2"], ["Set-Cookie", "a=1%2Cb%3D2"]]> to
		have {key "x-http-cookie" be == ["a=1;b=2"], key "set-cookie" be == ["a=1", "b=2"]}
			key "x-http-cookie" be == ["a=1;b=2"]
				✓ has key test/rack/conform/cookies.rb:41
				expect ["a=1", "b=2"] to
					be == ["a=1;b=2"]
						✗ assertion failed test/rack/conform/cookies.rb:41
			key "set-cookie" be == ["a=1", "b=2"]
				✓ has key test/rack/conform/cookies.rb:41
				expect ["a=1%2Cb%3D2"] to
					be == ["a=1", "b=2"]
						✗ assertion failed test/rack/conform/cookies.rb:41

[ioquatix]

The expectation that servers will properly handle malicious or malformed requests, and also handle any failure in upstream code is certainly my idea of fun...

Evidence here suggests most servers are just combining it together with ,.

What will happen, is that those cookies will get parsed incorrectly.

The request may fail, but only if the cookies are critical (e.g. session cookie will be foobar).

[ioquatix]

Okay, I have one more data point - Node.js appears to do the correct thing - it concatenates cookie headers using a semi-colon.

e.g.

samuel@sakura ~/D/s/r/e/node-cookies (multiple-cookies) [SIGINT]> cat server.js 
const http = require('http');

// Create an HTTP server
const server = http.createServer((req, res) => {
  // Get the cookies from the request headers
  const cookies = req.headers['cookie'];

  // Print the cookies to the console
  console.log('Received cookies:', cookies);

  // Send a response back to the client
  res.writeHead(200, { 'Content-Type': 'text/plain' });
  res.end('Cookies received\n');
});

// Listen on port 3000
const PORT = 3000;
server.listen(PORT, () => {
  console.log(`Server is listening on port ${PORT}`);
});
samuel@sakura ~/D/s/r/e/node-cookies (multiple-cookies)> node server.js
Server is listening on port 3000
Received cookies: cookie1=value1; cookie2=value2

Client:

samuel@Sakura ~/D/s/r/e/node-cookies (multiple-cookies) [7]> curl -v -H "Cookie: cookie1=value1" -H "Cookie: cookie2=value2" http://localhost:3000
* Host localhost:3000 was resolved.
* IPv6: ::1
* IPv4: 127.0.0.1
*   Trying [::1]:3000...
* Connected to localhost (::1) port 3000
> GET / HTTP/1.1
> Host: localhost:3000
> User-Agent: curl/8.7.1
> Accept: */*
> Cookie: cookie1=value1
> Cookie: cookie2=value2
> 
* Request completely sent off
< HTTP/1.1 200 OK
< Content-Type: text/plain
< Date: Fri, 07 Jun 2024 02:44:06 GMT
< Connection: keep-alive
< Keep-Alive: timeout=5
< Transfer-Encoding: chunked
< 
Cookies received
* Connection #0 to host localhost left intact

It's not unreasonable to imagine that this scenario would become more common given HTTP/2, and I could imagine load balancers just appending cookie: ... headers to a request to insert extra state tracking.

[MSP-Greg]

Ok, I think Puma should change its behavior...

[ioquatix]

If you agree, I'll also try to fix webrick.

[ioquatix]

Hmm, it also looks like passenger is doing the correct thing - joining using ;.

[ioquatix]

Okay, WEBrick is fixed but unreleased.