rails / sprockets

Rack-based asset packaging system
MIT License
932 stars 792 forks source link

Fix header casing compatibility with Rails 7 #790

Closed skipkayhil closed 10 months ago

skipkayhil commented 11 months ago

Ref rails/rails#48874 Fixes rails/rails#47096 for Rails < 7.1

To support Rack 3, response headers in Sprockets::Server were all downcased. However, this has led to issues with Rack 2 applications (ex. Rails 7) since they still expect mixed case (ex. Content-Type) headers.

To ensure compatibility with both Rack 2 and Rack 3 applications, this commit makes the casing of the headers conditional on the Rack version. Rack itself provides constants to do this easily for most of the headers used (Content-Type, Content-Length, Cache-Control, and ETag) and the rest are added as constants under Rack::Server.

As an alternative to this, the responses could instead be wrapped using Rack::Headers (and Rack::Utils::HeaderHash in Rack 2), but making the header casing conditional seems better to me because it is relatively easier to implement and there will be less churn if/when Rack 2 support is eventually removed.

skipkayhil commented 11 months ago

Tests should be fixed by #791

rafaelfranca commented 11 months ago

Wait, what?

Doesn't rack >= 2.2.4 don't care about the casing of the headers? https://github.com/rails/sprockets/pull/758#issuecomment-1244789597

skipkayhil commented 11 months ago

Doesn't rack >= 2.2.4 don't care about the casing of the headers?

For any library that aims to support Rack 2 and Rack 3, the casing of headers matters unless those libraries return Rack::Utils::HeaderHash/Rack::Headers in all responses and all middleware that read/write headers use Rack::Utils::HeadersHash/Rack::Headers as well.

So Rails returns HeadersHash because it uses ActionDispatch::Response, but these responses are plain hashes. This means a Rack 2 middleware immediately above Sprockets::Server that tries to read Content-Type will not work correctly since these responses are hashes that only contain content-type.


So as an alternative to this PR, we could apply Headers to these responses, something like this:

def bad_request_response(env)
  if head_request?(env)
    [ 400, Headers[{ "content-type" => "text/plain", Rack::CONTENT_LENGTH => "0" }], [] ]
  else
    [ 400, Headers[{ "content-type" => "text/plain", Rack::CONTENT_LENGTH => "11" }], [ "Bad Request" ] ]
  end
end

where Headers looks like: https://github.com/rails/rails/blob/main/actionpack/lib/action_dispatch/http/response.rb#L37-L45


I wrote some thoughts on using Headers vs conditional casing here: https://github.com/rails/rails/issues/48874#issuecomment-1662768849 and https://github.com/rails/rails/issues/48874#issuecomment-1662768875