Support for Rack 3 headers formatted as arrays

Support for Rack 3 headers formatted as arrays

[Martin Posthumus]

Hello,

As of Rack v3, the internal representation of headers containing 
multiple values appears to have been changed to use an array of strings 
rather than a newline-separated string. Pull request visible here: 
https://github.com/rack/rack/pull/1793

The Rack changelog (https://github.com/rack/rack/blob/main/CHANGELOG.md) 
also includes this entry under 3.0.0:
"Response header values can be an Array to handle multiple values (and 
no longer supports \n encoded headers)."



I'm running into some serialization issues with this sort of header when 
trying to run an application on Unicorn that makes use of multiple 
cookies. The `set-cookie` header is returning what appears to be a 
stringified array:

set-cookie: ["my.cookie=.....; domain=......; path=/", 
"rack.session=......; path=/; httponly"]

Which in turn results in cookies getting registered with names like 
`["rack.session` rather than `rack.session`.


I'm not especially familiar with Unicorn's internals, but poking around 
a little bit, it looks like this might be related to the definition of 
`http_response_write` in lib/unicorn/http_response.rb, where it handles 
newline-separated strings, but not arrays. I can confirm that the 
set-cookie header value appears to be an array in this method, not a string.


At present I'm only seeing this issue when running on a Unicorn server. 
When I swap out something like webrick, it appears the array values are 
handled as expected.


Thank you,

Martin

Re: Support for Rack 3 headers formatted as arrays

[Eric Wong]

Martin Posthumus <martin.posthumus@gmail.com> wrote:
> Hello,
> 
> As of Rack v3, the internal representation of headers containing multiple
> values appears to have been changed to use an array of strings rather than a
> newline-separated string. Pull request visible here:
> https://github.com/rack/rack/pull/1793
> 
> The Rack changelog (https://github.com/rack/rack/blob/main/CHANGELOG.md)
> also includes this entry under 3.0.0:
> "Response header values can be an Array to handle multiple values (and no
> longer supports \n encoded headers)."

OK.  unicorn has no choice to support all Rack as long as Rack <= 2
applications exist...

> I'm running into some serialization issues with this sort of header when
> trying to run an application on Unicorn that makes use of multiple cookies.
> The `set-cookie` header is returning what appears to be a stringified array:
> 
> set-cookie: ["my.cookie=.....; domain=......; path=/", "rack.session=......;
> path=/; httponly"]
> 
> Which in turn results in cookies getting registered with names like
> `["rack.session` rather than `rack.session`.
> 
> I'm not especially familiar with Unicorn's internals, but poking around a
> little bit, it looks like this might be related to the definition of
> `http_response_write` in lib/unicorn/http_response.rb, where it handles
> newline-separated strings, but not arrays. I can confirm that the set-cookie
> header value appears to be an array in this method, not a string.

Does this work for you?

diff --git a/lib/unicorn/http_response.rb b/lib/unicorn/http_response.rb
index b23e521..3308c9b 100644
--- a/lib/unicorn/http_response.rb
+++ b/lib/unicorn/http_response.rb
@@ -40,7 +40,10 @@ def http_response_write(socket, status, headers, body,
           # key in Rack < 1.5
           hijack = value
         else
-          if value =~ /\n/
+          case value
+          when Array # Rack 3
+            value.each { |v| buf << "#{key}: #{v}\r\n" }
+          when /\n/ # Rack 2
             # avoiding blank, key-only cookies with /\n+/
             value.split(/\n+/).each { |v| buf << "#{key}: #{v}\r\n" }
           else

> At present I'm only seeing this issue when running on a Unicorn server. When
> I swap out something like webrick, it appears the array values are handled
> as expected.

Yeah, I haven't looked deeply at Rack 3 support and hate dealing
with the culture of breaking changes prevalent in the Ruby world :<

Re: Support for Rack 3 headers formatted as arrays

[Martin Posthumus]

> OK.  unicorn has no choice to support all Rack as long as Rack <= 2
> applications exist...

Understood, I just wanted to show that I'm reasonably confident this 
isn't due to something weird I was doing in application code, and that 
ultimately I couldn't find any workaround that didn't involve 
monkeypatching either Rack or Unicorn.

> Does this work for you?

Indeed it does!

> Yeah, I haven't looked deeply at Rack 3 support and hate dealing
> with the culture of breaking changes prevalent in the Ruby world :<

Yeah, I get it. To be fair the array representation probably makes more 
sense for a header with multiple values, but downstream in practice it 
means you have to deal with both.


Thanks

Re: Support for Rack 3 headers formatted as arrays

[Eric Wong]

Martin Posthumus <martin.posthumus@gmail.com> wrote:
> > OK.  unicorn has no choice to support all Rack as long as Rack <= 2
> > applications exist...
> 
> Understood, I just wanted to show that I'm reasonably confident this isn't
> due to something weird I was doing in application code, and that ultimately
> I couldn't find any workaround that didn't involve monkeypatching either
> Rack or Unicorn.
> 
> > Does this work for you?
> 
> Indeed it does!

Thanks for the confirmation and good to know.

> > Yeah, I haven't looked deeply at Rack 3 support and hate dealing
> > with the culture of breaking changes prevalent in the Ruby world :<
> 
> Yeah, I get it. To be fair the array representation probably makes more
> sense for a header with multiple values, but downstream in practice it means
> you have to deal with both.

Yes, and Rack 3 also breaks the array of arrays `[ [ k, v1 ], [ k, v2 ] ]'
representation I was relying on :<

In any case, I'm thinking about hoisting out append_header()
into its own method to reuse between normal responses and
rack.early_hints 103 responses.

Cc-ing Jean to see if anything is amiss with the following
since they wrote the original code.

Original Rack 3 headers patch at:
   https://yhbt.net/unicorn-public/20221210024246.GA8584@dcvr/


diff --git a/lib/unicorn/http_response.rb b/lib/unicorn/http_response.rb
index 3308c9be..19469b47 100644
--- a/lib/unicorn/http_response.rb
+++ b/lib/unicorn/http_response.rb
@@ -19,6 +19,18 @@ def err_response(code, response_start_sent)
       "#{code} #{STATUS_CODES[code]}\r\n\r\n"
   end
 
+  def append_header(buf, key, value)
+    case value
+    when Array # Rack 3
+      value.each { |v| buf << "#{key}: #{v}\r\n" }
+    when /\n/ # Rack 2
+      # avoiding blank, key-only cookies with /\n+/
+      value.split(/\n+/).each { |v| buf << "#{key}: #{v}\r\n" }
+    else
+      buf << "#{key}: #{value}\r\n"
+    end
+  end
+
   # writes the rack_response to socket as an HTTP response
   def http_response_write(socket, status, headers, body,
                           req = Unicorn::HttpRequest.new)
@@ -40,15 +52,7 @@ def http_response_write(socket, status, headers, body,
           # key in Rack < 1.5
           hijack = value
         else
-          case value
-          when Array # Rack 3
-            value.each { |v| buf << "#{key}: #{v}\r\n" }
-          when /\n/ # Rack 2
-            # avoiding blank, key-only cookies with /\n+/
-            value.split(/\n+/).each { |v| buf << "#{key}: #{v}\r\n" }
-          else
-            buf << "#{key}: #{value}\r\n"
-          end
+          append_header(buf, key, value)
         end
       end
       socket.write(buf << "\r\n".freeze)
diff --git a/lib/unicorn/http_server.rb b/lib/unicorn/http_server.rb
index 3416808f..cad515b9 100644
--- a/lib/unicorn/http_server.rb
+++ b/lib/unicorn/http_server.rb
@@ -589,22 +589,11 @@ def handle_error(client, e)
   end
 
   def e103_response_write(client, headers)
-    response = if @request.response_start_sent
-      "103 Early Hints\r\n"
-    else
-      "HTTP/1.1 103 Early Hints\r\n"
-    end
-
-    headers.each_pair do |k, vs|
-      next if !vs || vs.empty?
-      values = vs.to_s.split("\n".freeze)
-      values.each do |v|
-        response << "#{k}: #{v}\r\n"
-      end
-    end
-    response << "\r\n".freeze
-    response << "HTTP/1.1 ".freeze if @request.response_start_sent
-    client.write(response)
+    rss = @request.response_start_sent
+    buf = rss ? "103 Early Hints\r\n" : "HTTP/1.1 103 Early Hints\r\n"
+    headers.each { |key, value| append_header(buf, key, value) }
+    buf << (rss ? "\r\nHTTP/1.1 ".freeze : "\r\n".freeze)
+    client.write(buf)
   end
 
   def e100_response_write(client, env)

Re: Support for Rack 3 headers formatted as arrays

[Jean Boussier]

> Cc-ing Jean to see if anything is amiss with the following
> since they wrote the original code.
>

Looks good to me. 1XX responses don't have any special rules regarding 
headers, so sharing the code make sense.

Re: Support for Rack 3 headers formatted as arrays

[Jean Boussier]

Interestingly, this patch would also help with Ruby 3.2 compatibility.

Ruby 3.2 removed `Kernel#=~`, so when misbehaving app return header 
values in e.g. Integer
unicorn now choke on it:

app error: undefined method `=~' for 43747171631368:Integer (NoMethodError)
unicorn-6.1.0/lib/unicorn/http_response.rb:43:in `block in 
http_response_write'
unicorn-6.1.0/lib/unicorn/http_response.rb:34:in `each'
unicorn-6.1.0/lib/unicorn/http_response.rb:34:in `http_response_write'
unicorn-6.1.0/lib/unicorn/http_server.rb:645:in `process_client'

Whereas on 3.1 and older, `42 =~ /\n/` would simply not match and cast 
the header as a String.

Re: Support for Rack 3 headers formatted as arrays

[Eric Wong]

Jean Boussier <jean.boussier@shopify.com> wrote:
> Interestingly, this patch would also help with Ruby 3.2 compatibility.
> 
> Ruby 3.2 removed `Kernel#=~`, so when misbehaving app return header values
> in e.g. Integer
> unicorn now choke on it:

Thanks for that note.  2 things:

1. Ugh.  That sort disregard for compatibility at the expense of making
   things "better" is why I and most people I've known no longer do new
   projects in Ruby :<  Heck, I started rewriting many of unicorn's tests
   in Perl5 a few weeks ago since I want to be able to trust them decades
   from now.

2. I suppose unicorn will continue letting misbehaving apps have
   their way.  It would be nice if developers consistently
   tested with Rack::Lint, of course

> Whereas on 3.1 and older, `42 =~ /\n/` would simply not match and cast the
> header as a String.

Now I wonder if case/when === behavior will break someday...

Re: Support for Rack 3 headers formatted as arrays

[Jean Boussier]

> It would be nice if developers consistently tested with Rack::Lint, of course

Indeed. I'm planning to make Rack::Lint a default middleware on Rails apps.
> Now I wonder if case/when === behavior will break someday...
Unlikely, by convention `===` should never raise.