Allow header value to be an Array of String instances.

[ioquatix]

Addresses #1598

[ioquatix]

@jeremyevans @tenderlove Looking at the RFC, there should be no need to delete an existing cookie by scanning the list of set-cookie headers before adding one with value= and expires in the past. The browser will simply add and then remove a cookie.

I believe that there would be very few cases where this is actually useful behaviour. i.e. someone would have to add and then delete a cookie with the same details to hit this case.

Bearing in mind, this means that every time someone deletes the cookie, it's O(N) scan, and even worse on the current implementation which has to unpack, parse and then recombine all the cookies.

I'd personally like to simplify this in this PR.

If someone deletes a cookie, we simply append the appropriate deletion (expires in past, value=) without trying to unpack or parse existing cookies.

The only disadvantage I can see with this is if someone sets, say, a 1MiB cookie and then deletes it, in my proposed implementation it would send still 1MiB of cookie data.

I think the simplicity (and probably performance) win is big enough to justify the difference. That being said, if there are buggy behaviours that don't process set-cookie in order, it could be an issue... but then it's a bug in the browser.

[ioquatix]

By the way, I've also removed a bunch of Utils methods that don't really make sense and were very specific to the newline encoded format. We can try to add compatibility shims, not sure if it's necessary?

[ioquatix]

Doing some digging, found that it looks like we did previously support header value being an Array:

rack/lib/rack/response.rb

Lines 84 to 96 in f23e833

	def delete_cookie(key, value={})
	unless Array === self["Set-Cookie"]
	self["Set-Cookie"] = [self["Set-Cookie"]].compact
	end
	self["Set-Cookie"].reject! { |cookie|
	cookie =~ /\A#{Utils.escape(key)}=/
	}
	set_cookie(key,
	{:value => '', :path => nil, :domain => nil,
	:expires => Time.at(0) }.merge(value))
	end

[ioquatix]

@leahneukirchen I see your first implementation here: 7ed819a#diff-cfcc6d2645b6f77c5ec05acd448f280ce3004d3dba85b8aef3b3c81faf6d1373R53-R55

It looks like you decided to delete cookies with the given name using a regexp. While I understand that it's a fairly "complete" implementation, my impression of reading the RFC is that it's unnecessary, as adding a later header with value= and expires=1970 is sufficient as the user agent should process it in order. Do you remember why you implemented it this way?

[tenderlove]

This is definitely better than splitting on \n, but I have a few concerns. First, we should make the webrick handler backwards compatible. Second, it's very very bad if an outgoing header contains a newline as it can lead to header splitting attacks. If an attacker is able to control header values, they can use \n to set cookies or whatever. The fact that the Rack SPEC requires webservers to split on \n surprisingly helps us avoid that particular issue as it means output header values can never have a newline.

I realize the spec says values below 037 are not allowed but it looks like we're not verifying that in the webrick server.

I'm not sure what we should do exactly, but the spec needs to be quite strongly worded with regard to the data allowed in headers, and we might want to add exceptions and checking in Rack::Response.

[jeremyevans]

I'm OK with the protocol change. I think we need to properly deprecate the Utils methods, unless doing so is not feasible.

[ioquatix]

@tenderlove I actually had a similar discussion and we introduced checks here as a result (which is used by Falcon):

https://github.com/socketry/protocol-http1/blob/main/lib/protocol/http1/connection.rb#L160

This problem doesn’t apply to HTTP/2 w.r.t. potential security issues so I don’t bother with a similar check there.

[ioquatix]

@tenderlove I checked this and Webrick already fails if you give it a header containing an invalid value:

e.g.

# config.ru

run lambda{|env| [200, {"test" => "a\nb: c"}, {}]}

Result:

> curl -v http://localhost:9292
*   Trying 127.0.0.1:9292...
* Connected to localhost (127.0.0.1) port 9292 (#0)
> GET / HTTP/1.1
> Host: localhost:9292
> User-Agent: curl/7.80.0
> Accept: */*
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 500 Internal Server Error
< Content-Type: text/html; charset=ISO-8859-1
* no chunk, no close, no size. Assume close to signal end
< 
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0//EN">
<HTML>
  <HEAD><TITLE>Internal Server Error</TITLE></HEAD>
  <BODY>
    <H1>Internal Server Error</H1>
    WEBrick::HTTPResponse::InvalidHeader
    <HR>
    <ADDRESS>
     WEBrick/1.7.0 (Ruby/3.0.3/2021-11-24) at
     localhost:9292
    </ADDRESS>
  </BODY>
</HTML>
* Closing connection 0

So, I don't think we need to do anything here, as it's already correctly failing. WDYT?

[ioquatix]

This is definitely better than splitting on \n, but I have a few concerns. First, we should make the webrick handler backwards compatible

As @jeremyevans mentioned, if someone includes Rack 2 they will get the Rack 2 implementation, and if they include Rack 3 they will get the Rack 3 implementation. I don't think backwards compatibility is needed here?

I realize the spec says values below 037 are not allowed but it looks like we're not verifying that in the webrick server.

Webrick appears to validate this internally now.

I'm not sure what we should do exactly, but the spec needs to be quite strongly worded with regard to the data allowed in headers, and we might want to add exceptions and checking in Rack::Response.

My feeling is it's better to check this in the server since it reduces the performance cost (doesn't double up on checks).

[jeremyevans]

Looks mostly good, just need to modify the warn calls.