* Support for Rack 3 headers formatted as arrays
@ 2022-12-09 21:52 Martin Posthumus
2022-12-10 2:42 ` Eric Wong
0 siblings, 1 reply; 8+ messages in thread
From: Martin Posthumus @ 2022-12-09 21:52 UTC (permalink / raw)
To: unicorn-public
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Support for Rack 3 headers formatted as arrays
2022-12-09 21:52 Support for Rack 3 headers formatted as arrays Martin Posthumus
@ 2022-12-10 2:42 ` Eric Wong
2022-12-12 15:52 ` Martin Posthumus
0 siblings, 1 reply; 8+ messages in thread
From: Eric Wong @ 2022-12-10 2:42 UTC (permalink / raw)
To: Martin Posthumus; +Cc: unicorn-public
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 :<
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: Support for Rack 3 headers formatted as arrays
2022-12-10 2:42 ` Eric Wong
@ 2022-12-12 15:52 ` Martin Posthumus
2022-12-25 22:56 ` Eric Wong
0 siblings, 1 reply; 8+ messages in thread
From: Martin Posthumus @ 2022-12-12 15:52 UTC (permalink / raw)
To: Eric Wong; +Cc: unicorn-public
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Support for Rack 3 headers formatted as arrays
2022-12-12 15:52 ` Martin Posthumus
@ 2022-12-25 22:56 ` Eric Wong
2022-12-27 8:17 ` Jean Boussier
2023-01-11 11:12 ` Jean Boussier
0 siblings, 2 replies; 8+ messages in thread
From: Eric Wong @ 2022-12-25 22:56 UTC (permalink / raw)
To: Martin Posthumus; +Cc: unicorn-public, Jean Boussier
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)
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: Support for Rack 3 headers formatted as arrays
2022-12-25 22:56 ` Eric Wong
@ 2022-12-27 8:17 ` Jean Boussier
2023-01-11 11:12 ` Jean Boussier
1 sibling, 0 replies; 8+ messages in thread
From: Jean Boussier @ 2022-12-27 8:17 UTC (permalink / raw)
To: Eric Wong, Martin Posthumus; +Cc: unicorn-public
> 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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Support for Rack 3 headers formatted as arrays
2022-12-25 22:56 ` Eric Wong
2022-12-27 8:17 ` Jean Boussier
@ 2023-01-11 11:12 ` Jean Boussier
2023-01-11 11:40 ` Eric Wong
1 sibling, 1 reply; 8+ messages in thread
From: Jean Boussier @ 2023-01-11 11:12 UTC (permalink / raw)
To: Eric Wong, Martin Posthumus; +Cc: unicorn-public
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Support for Rack 3 headers formatted as arrays
2023-01-11 11:12 ` Jean Boussier
@ 2023-01-11 11:40 ` Eric Wong
2023-01-11 11:44 ` Jean Boussier
0 siblings, 1 reply; 8+ messages in thread
From: Eric Wong @ 2023-01-11 11:40 UTC (permalink / raw)
To: Jean Boussier; +Cc: Martin Posthumus, unicorn-public
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...
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Support for Rack 3 headers formatted as arrays
2023-01-11 11:40 ` Eric Wong
@ 2023-01-11 11:44 ` Jean Boussier
0 siblings, 0 replies; 8+ messages in thread
From: Jean Boussier @ 2023-01-11 11:44 UTC (permalink / raw)
To: Eric Wong; +Cc: Martin Posthumus, unicorn-public
> 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.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-01-11 11:44 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-09 21:52 Support for Rack 3 headers formatted as arrays Martin Posthumus
2022-12-10 2:42 ` Eric Wong
2022-12-12 15:52 ` Martin Posthumus
2022-12-25 22:56 ` Eric Wong
2022-12-27 8:17 ` Jean Boussier
2023-01-11 11:12 ` Jean Boussier
2023-01-11 11:40 ` Eric Wong
2023-01-11 11:44 ` Jean Boussier
Code repositories for project(s) associated with this public inbox
https://yhbt.net/unicorn.git/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).