Rack::Lint on Rails

[skipkayhil]

Meta issue for adding Rack::Lint to Rails middleware tests.

As Rack 3 can now be used with Rails main, it is necessary to ensure that all of the middleware defined in Rails adhere to the Rack SPEC. While existing test coverage was able to find many of the incompatibilities, I've seen multiple examples of mixed case header usage left that need to be fixed. Instead of trying to grep or otherwise manually find these middleware that aren't adhering to the SPEC, I'd like to add Rack::Lint to (every?) middleware's unit test in Rails so that we can be sure they follow the Rack 3 SPEC programmatically.

[skipkayhil]

Supporting Rack 2 and Rack 3 in libraries

As more gems are opening up their version constraints from rack < 3 to rack < 4, it is imperative that these libraries support both Rack 2 and Rack 3 in practice and not just in theory. If the compatibility is not done correctly, users will run into hard to debug errors and generally have a bad experience with the Rack 3 transition.

Some advice has been given that to support both Rack 2 and 3, all response headers should be downcased, and I know of multiple gems that have gone with this approach: rails/sprockets#758, sidekiq/sidekiq@f97d16a. However, while this approach does follow the Rack SPEC as written, I do not believe this is fully backwards compatible in practice. The argument for why this is a good approach is that the Rack 2 SPEC does not specify what casing Response headers should have, so using the casing prescribed by the Rack 3 SPEC is compatible with both. However, libraries have generally agreed that certain headers should have certain casing in Rack 2 (ex. Content-Type), and if a library was written with a different casing it would not be compatible with all other existing libraries.

The incompatibility can be seen in issues raised in Rails with the latest versions of these libraries and Rails < 7.1:

• Lowercase headers returned from Sprockets 4.2.0 not caught within Actionpack router #47096
  • Sprockets 4.2.0's x-cascade is no longer compatible with Rails < 7.1 X-Cascade
• CSP Middleware wrongly uses upper case header names #48815
  • Sidekiq 7's content-security-policy is no longer compatible with Rails < 7.1 content-security-policy

While we could choose to fix forward for these two headers in Rails 7, we can't really fix forward in any older Rails version, and I'm sure there will be plenty of similar issues in the future that end up requiring similar changes.

So what should we do instead?

There have been a few different strategies proposed for handling this:

Return a Rack::Response (or at least return headers as Rack::Headers)

App = ->(env) { Rack::Response.new }
HeaderApp = ->(env) { [200, Rack::Headers.new, []] }

# either of the above Apps would work with either of the following middleware
class TwoMid
  # init
  
  def call(env)
    resp = # call
    resp["Content-Type"] # text/html
  end
end

class ThreeMid
  # init
  
  def call(env)
    resp = # call
    resp["content-type"] # text/html
  end
end

Rails uses this approach for Responses returned as ActionDispatch::Response. The benefit of Rack::Headers is that you can use it with any header casing and it will convert the headers to the Rack 3 casing internally. This allows libraries that want to support Rack 3 to use exclusively downcased headers while allowing exclusively Rack 2 compatible libraries to continue using mixed case headers.

However, this requires that anything creating a Response must use this strategy (ex. Sprockets and Sidekiq too). This means that exclusively Rack 2 compatible libraries will still need to be updated if they return a Rack response themselves.

Since libraries that only support Rack 2 (like Rails < 7.1) need to be updated for this approach to work, it does not feel like the best strategy to maintain backwards compatibility.

All middleware can wrap response in Rack::Response or Rack::Headers

TwoApp = ->(env) { [200, { "Content-Type" => "text/html" }, [] }
ThreeApp = ->(env) { [200, { "content-type" => "text/html" }, [] }

# either of the above Apps would work with this middleware
class Mid
  # init
  
  def call(env)
    resp = Rack::Response[*@app.call(env)]
    resp["content-type"] # text/html
  end
end

This strategy was used in 706fb10 to solve a tangential compatibility issue (setting/deleting cookies), but it could be used to solve the Response header compatibility issue as well. If every Rails middleware ensures that either the Response or Headers are wrapped then we can go forward with using exclusively downcased headers in Rails. The wrapped Response/Headers will handle the casing conversions for us so that Rails doesn't not have to worry about them.

While this will prevent every Rack 2 compatible libraries from having to make changes, this does not really help us solve the compatibility issues with Rails < 7.1 and I'm sure this would come with a performance hit as well. Additionally, if there is not a Rails middleware performing wrapping between a Rack 2-only middleware and a Rack 3 middleware (like Sprockets 4.2.0 or Sidekiq 7) then those middleware will remain incompatible.

Make header casing conditional based on Rack::RELEASE

CONTENT_TYPE = if Rack::RELEASE >= 3
    "content-type"
  else
    "Content-Type"
  end

App = ->(env) { [200, { CONTENT_TYPE => "text/html" }, [] }

# the App and Middleware will be compatible with either Rack version
class Mid
  # init
  
  def call(env)
    resp = # call
    resp[CONTENT_TYPE] # text/html
  end
end

This is the strategy I have been using in some of the above PRs (ex. Location vs location here. Rack also provides some constants that make this especially easy to do, such as Rack::CONTENT_TYPE (Content-Type for Rack 2, content-type for Rack 3). This approach ensures that the headers used will remain the exact same for applications using Rack 2, and only if an app switches to Rack 3 will it start to use the downcased headers, which is why I believe this is the best way to remain backwards compatible.

This prevents any Rack 2-only libraries from having to make any changes to remain compatible with libraries that support both. (ex. Rails < 7.1 should continue to work without any header changes)

The downside to this approach is that libraries may have already committed to downcasing their headers, and this proposal would suggest that they should make them conditional instead. However, I think this has a smaller impact than making Rack 2-only libraries update their code as the Rack 3 supporting libraries are more likely to be actively maintained.

Conclusion

It seems to me that making headers conditional based on the Rack version is the best strategy for libraries that want to fully support both Rack 2 and Rack 3. While temporarily verbose, it ensures that "dual" compatible libraries remain fully compatible with Rack 2 libraries. Additionally, it prevents any existing Rack 2 libraries from having to make any changes to their code (which seems like the primary reason to even support both versions anyways)

[matthewd]

Make header casing conditional based on Rack::RELEASE

If this is the best option available, IMO we should just roll back the gemspec change, and revisit possible compatibility with Rack 4.

Even beyond the mess it creates in our implementation, redefining the Rack 2 spec to retroactively declare existing Rack 3 (and 2) implementations to be non-compliant is prima facie unreasonable.

[skipkayhil]

From #48830 (comment)

Can we not just use lower case everywhere? There shouldn't be any compatibility issue with Rack 2 using lower case.

I think you're suggesting using lowercase everywhere in addition to Rack::Headers? I don't think using lowercase only without Rack::Headers is a viable solution, and am happy to expand on that if that's what you mean.

---

Matthew pointed out to me yesterday that the proper way to use Rack::Headers for libraries allowing Rack 2 or 3 is actually to both return in in Responses and use it in all middleware:

Headers = Rack::Headers || Rack::Utils::HeaderHash # Rack 3 / Rack 2

TwoApp = ->(env) { [200, { "Content-Type" => "text/html" }, [] }
TwoOrThreeApp = ->(env) { [200, Headers.new({ "content-type" => "text/html" }), [] }

class TwoMid
  # init

  def call(env)
    resp = @app.call(env)
    resp[1]["Content-Type"] # text/html
  end
end

class TwoOrThreeMid
  # init
  
  def call(env)
    # we can use Headers directly
    resp = @app.call(env)
    Headers.new(resp[1])["content-type"] # text/html

    # or wrap the whole thing in Response, which uses Headers internally
    resp = Rack::Response[*resp]
    resp.headers["content-type"] # text/html
  end
end

He also pointed out that this has a potential benefit of being more forgiving of any Rack 2 libraries that chose to use downcased headers already.

I'm still not a huge fan of this approach, because I think it would cause significantly more churn in libraries than conditional casing:

• Using Rack::Response would only be necessary while supporting both Rack 2 and 3, so Rack::Headers would be added to all middleware for some time and then probably removed when dropping Rack 2 support.
• The churn with the approach we've been taking with ActionDispatch::Constants will be minimal, since we can just remove the Rack 2 branch once support is dropped.

And while being more compatible with the Rack 2 spec as written seem like a good idea, I think the fact that the de facto header case being mixed means the benefit will not be that large.

[ioquatix]

Thanks for writing down your thoughts and working towards a pattern that will solve the various dimensions of this problem.

Any existing code that looks like this:

status, headers, body = @app.call(env)
if url = headers["Location"] # handle redirect

In Rack 2 is buggy. In Rack 3, it's considered invalid. It may work, but in some cases it may not.

I have personally run into this bug, where middleware expects a specific case. When I wrote the HTTP/2 proxy, it would return lower case headers (because that's what HTTP/2 requires). Returning those headers would break middleware that was looking for a specific Location key.

So, the solutions which avoid this problem:

• When you need to read the response headers, use Rack::Headers or Rack::Utils::HeaderHash (aliased in 3.0 and removed in 3.1).
• When you need to manipulate the response headers, use Rack::Response. This handles the nuances of the array vs string format.
• Use lower case headers everywhere.
• Support downstream gems that don't use Rack::Headers for dealing with case issues.
• Once we drop support for Rack 2, you can drop any usage of Rack::Headers and Rack::Response and the code should become a lot simpler.

The last point is specifically relevant, as I suspect Rails 8 will be Rack 3 only, so we will be able to drop a lot of this code in fairly short order.

So let me make one further concession: if for the sake of compatibility, if it's easier to have a Rack 2 and Rack 3 code path, I think it's okay. However, it may create more surprising behaviour for users, since we would be bifurcating the internal implementation. But as long as we agree to resolve this for Rails 8, I'm okay with it.

[matthewd]

In Rack 2 is buggy. In Rack 3, it's considered invalid. It may work, but in some cases it may not.

Sure, but it exists, and has been working consistently between many interacting middlewares for years. Merely insisting that the code that worked for all that time was wrong does not solve the "worked yesterday, broken today" of it... I think there are some Torvaldsian rants missives I could reference here.

Support downstream gems

I think this means "send them PRs and tell them their seemingly-working code was always broken". That helps the future, for the known (and open source) instances, but doesn't change the fact that we have, in practice, a hard compatibility break between "gems that were written over the last decade using the conventions of the day", and "gems that have been modified to comply with rack 3's strict requirements".

---

The most compatible path forward that I can currently imagine would be to loosen the Rack 3 spec to permit the headers value to be some form of case-forcing hash variant. A middleware that's Rack 3 compliant could then use the correct lowercase forms while providing a headers object that allows collaborating Rack 2 compliant case-mixing middlewares to become retroactively compatible.

if for the sake of compatibility, if it's easier to have a Rack 2 and Rack 3 code path, I think it's okay

For the record, I don't, and only secondarily because of what it does to the Rails codebase: if the only way for any Rack-compatible gem to be functionally compatible with both Rack 2 and Rack 3 is to version-detect which Rack is in use and then branch appropriately, then they are such fundamentally distinct standards that they should have different names.

Tightening the spec such that middleware must be updated to remain compatible is fair, but if becoming conformant with the new spec necessarily breaks compatibility with other previously-compatible middleware, then it's not a refinement, it's just a straight-up different protocol. 😕

[ioquatix]

I don't really agree with your conclusion.

Lots of current Rack (v2) middleware/applications (and perhaps evens servers) do not follow the spec and would probably fail if you tested them with Rack::Lint.

https://github.com/search?q=%22status%2C+headers%2C+body+%3D%22+%22headers%5B%22+language%3ARuby&type=code

It's actually because of the above pattern (assuming headers is a hash) that in Rack 3 we made that a requirement. We could allow it to be non-normalized, but then how would you look up any of the header keys? So we adopted the normalized lower-case which is what is advised by the RFCs.

[rafaelfranca]

Why are we having a relevant discussion to the Rails project in a fork of Rails?

[zzak]

@skipkayhil Is there anything left on this or can we close it out?