* 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).