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

HTTP_VERSION vs SERVER_PROTOCOL and Rack::Lint #2141

Closed
lloeki opened this issue Jan 2, 2024 · 5 comments
Closed

HTTP_VERSION vs SERVER_PROTOCOL and Rack::Lint #2141

lloeki opened this issue Jan 2, 2024 · 5 comments

Comments

@lloeki
Copy link

lloeki commented Jan 2, 2024

Since HTTP_VERSION is a client supplied header, can HTTP_VERSION and SERVER_PROTOCOL differ in practice, or am I misunderstanding #970 #1691 #969 ?

Found out via this lint check that blowed up in Thin specs while attempting to move it to Rack 3.0.

@jeremyevans
Copy link
Contributor

For reference, the Lint check was added in caed808

I think the Lint check was added so that apps/middleware that were using HTTP_VERSION as an alias to SERVER_PROTOCOL don't get client-provided information that they might use incorrectly. Maybe that Lint check can be removed? Servers that want to support both Rack 2 and 3 should do: env['SERVER_PROTOCOL'] || env['HTTP_VERSION']

@ioquatix
Copy link
Member

@jeremyevans are you proposing to remove caed808#diff-2db485952a203ded9d5ae1301e35d82601b1c2410f914e767b02c4933b7e8a57R302-R305

If so, I'm also okay with that.

@lloeki
Copy link
Author

lloeki commented Jan 31, 2024

I think such a lint may still have value, as it pointed me to (maybe) fix Thin:

macournoyer/thin@b4d7946

Thin was blindly overwriting curl -H 'Version: foo' http://1.2.3.4 with the value at the end of the HTTP request line instead of allowing HTTP_VERSION to be foo.

@jeremyevans
Copy link
Contributor

@jeremyevans are you proposing to remove caed808#diff-2db485952a203ded9d5ae1301e35d82601b1c2410f914e767b02c4933b7e8a57R302-R305

If so, I'm also okay with that.

Yep, I think that can be removed. Servers should not have to deal with request Version headers specially.

@ioquatix
Copy link
Member

ioquatix commented Feb 1, 2024

Thin was blindly overwriting curl -H 'Version: foo' http://1.2.3.4 with the value at the end of the HTTP request line instead of allowing HTTP_VERSION to be foo.

IMHO, HTTP_VERSION should not be set by the server going forward. I'll add a rack-conform test for this.

As we merged #2154 I believe this issue is resolved, but feel free to re-open it if not.

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

No branches or pull requests

3 participants