Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Puma forces casing of certain response headers #3250

Open
skipkayhil opened this issue Oct 5, 2023 · 18 comments · May be fixed by #3305
Open

Puma forces casing of certain response headers #3250

skipkayhil opened this issue Oct 5, 2023 · 18 comments · May be fixed by #3305

Comments

@skipkayhil
Copy link

Describe the bug

While most headers have casing determined by what's in the app's response headers, Puma forces the casing of certain headers in the response it returns. When using Rack 2, the forced Content-Length type casing was likely not noticed because (most) apps would use this for their header casing. As Rails is close to a release that supports Rack 3, this behavior will likely be surprising to users who expect the same casing in the response returned by Puma as is returned by their app.

Puma config:

No config, just rackup with the config.ru below

To Reproduce

# config.ru
App = ->(e) { [200, { "content-type" => "text/plain", "content-length" => "5" }, ["12345"]] }

run App

Run it with:

$ rackup
$ curl -I localhost:9292
HTTP/1.1 200 OK
content-type: text/plain
Content-Length: 5

Expected behavior

$ curl -I localhost:9292
HTTP/1.1 200 OK
content-type: text/plain
content-length: 5

Desktop (please complete the following information):

  • OS: any
  • Puma Version 6.4.0
@MSP-Greg
Copy link
Member

MSP-Greg commented Oct 5, 2023

I haven't been in the various specs for several weeks. See https://www.rfc-editor.org/rfc/rfc9110.html#name-content-length. The spec also refers to Transfer-Encoding, etc.

The Rack spec specifies that header field names/keys should be lower case, but that isn't referring to the HTTP syntax, it's referring to what the app returns.

Also, most browsers' 'dev tools' show capitalized header field names...

@skipkayhil
Copy link
Author

skipkayhil commented Oct 5, 2023

Right, my understanding is either casing is valid in HTTP/1.1 spec, and the Rack spec doesn't actually say anything about how the server should respond.

However, I believe the current behavior is confusing (at least for me, and the user that opened an issue on Rails) because only certain headers have their casing changed. With a Rack 2 version of the App in config.ru above, both headers will be mixed case in the app and the Puma response will have all mixed case headers. With the Rack 3 version, all headers in the app will be downcased, but only some of the headers in the Puma response will be downcased.

This confusion seems especially important as apps will be migrating to Rack 3 and trying to determine whether all of their libraries/middleware are behaving as expected. While using Rack::Lint is probably the best way for them to know for sure, I predict libraries will see issues raised because users will be looking at the actual response and seeing unexpected casing.

Also, most browsers' 'dev tools' show capitalized header field names...

image

Firefox shows me the exact casing, but I do recognize I'm not part of the Chrome majority.

@dentarg
Copy link
Member

dentarg commented Oct 5, 2023

This can't happen to that many headers, right? Only those Puma are responsible for. I think we can see all of them in https://github.com/puma/puma/blob/7a1237b4215e86020ed8cb3080c2958d850877c7/lib/puma/const.rb

I get the point you're making... but wouldn't it be equally confusing if you started to see lowercase headers mixed in when you still on Rack 2 but a newer Puma? (If this was adjusted) (at least for some time, until the whole world is on Rack 3)

Is it a breaking change for Puma to change to downcased headers?

It is possible to make it user configurable?

Investigation's welcomed.

@nateberkopec
Copy link
Member

Puma's contract is to produce legal, valid HTTP responses. Since header field names are case insensitive, it isn't a breaking change for us to do anything regarding the casing of a header field name.

Because this is cosmetic (or, at least, a dev UX issue) and not an issue with Puma doing the right/wrong thing, I don't want to introduce new configs here.

@dentarg
Copy link
Member

dentarg commented Oct 28, 2023

So should we change all headers to lowercase or do nothing in Puma?

@MSP-Greg
Copy link
Member

While working on the test framework, some of the work involved removing the use of net/http. Its header API converts all header keys to lowercase.

So, the API has one method that returns the response headers as an array of lines (unconverted), and another method that returns the headers as a hash, with keys converted to lowercase.

Messy subject. I checked dev tools with Chrome, Edge, & FireFox, and I recall that all had capitalized header keys.

Or, not sure what to do...

@dentarg
Copy link
Member

dentarg commented Nov 13, 2023

So should we change all headers to lowercase or do nothing in Puma?

I think yes, we should use lowercase.

From rack/rack#1592

HTTP/2 makes it clear that headers should be lower-case (https://datatracker.ietf.org/doc/html/rfc7540#section-8.1.2):

Just as in HTTP/1.x, header field names are strings of ASCII characters that are compared
in a case-insensitive fashion. However, header field names MUST be converted to lowercase
prior to their encoding in HTTP/2. A request or response containing uppercase header field
names MUST be treated as malformed (Section 8.1.2.6).

@MSP-Greg
Copy link
Member

MSP-Greg commented Nov 13, 2023

If think we should change them all to lower case. But, doing so may break any CI looking at header fields. So, it should either be a minor or a major version change.

@MSP-Greg
Copy link
Member

@dentarg
Copy link
Member

dentarg commented Dec 31, 2023

If we go lowercase, these should change too

[404, {'Content-Type' => "text/plain", "X-Cascade" => "pass"}, ["Not Found: #{path}"]]

@nateberkopec
Copy link
Member

. But, doing so may break any CI looking at header fields.

That's their problem IMO. Both are valid HTTP 1.1. If you're relying on Puma to return specific casing in a header, you're relying on behavior we never specified.

@nateberkopec
Copy link
Member

Action item here: Puma should return lowercase headers fields in all circumstances.

@skipkayhil
Copy link
Author

If we go lowercase, these should change too

(unrelated to this issue which is more about puma -> client header casing)

Those should definitely be downcased for Rack 3 compliance since UrlMap is in the Rack layer. The current casing would be incompatible if any Rack 3 middleware were to be above it in the stack.

@nikhilbhatt
Copy link

nikhilbhatt commented Jan 6, 2024

I was working on this and updated headers in const.rb and urlmap.rb.

I have a doubt regarding CI pipelines. For example, in this line, it only sends a request. Should this also be changed to lowercase, or do I just need to update the response with corrected case.

socket = send_http "GET / HTTP/1.0\r\nConnection: Keep-Alive\r\n\r\n"

In most test cases, Content-Type is used in uppercase when a request is sent. Since Rack::Lint does not allow uppercase headers, should this Content-Type be updated to lowercase?

@dentarg
Copy link
Member

dentarg commented Jan 6, 2024

I don't think we need to update our tests. That's not the change we're talking about here.

(We could change the tests too, for consistency, but it should be done in a separate PR for easier review)

@nikhilbhatt
Copy link

nikhilbhatt commented Jan 6, 2024

I don't think we need to update our tests. That's not the change we're talking about here.

Ok, consistency of test cases part can be covered in separate issue.

But when we update the headers in puma to be returned always in lowercase(in const.rb file) . we need to update our tests as well. otherwise pipelines will fail.

@dentarg
Copy link
Member

dentarg commented Jan 6, 2024

Sure, such changes we need to do

@nikhilbhatt nikhilbhatt linked a pull request Jan 6, 2024 that will close this issue
7 tasks
@dentarg dentarg closed this as completed Feb 15, 2024
@dentarg dentarg reopened this Feb 15, 2024
@dentarg
Copy link
Member

dentarg commented Feb 15, 2024

(Apparently gotta be careful how you reference public issues from private repos when you're maintainer)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants