Do not reference HTTP_VERSION internally #969
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
HTTP_VERSION is supposed to be a client supplied header. This usage inside Rack is conflating it with SERVER_PROTOCOL, which imo is instead also conflating it with the client's HTTP version from the request line. In any of these cases, HTTP_VERSION is set when an existing Version header doesn't already exist. So it's possible to send a Version header to conflict with the expected behaviors. According to the CGI spec (https://tools.ietf.org/html/draft-robinson-www-interface-00) > Environment variables with names beginning with "HTTP_" contain header data read from the client, if the protocol used was HTTP. This is an anscillary issue with Rack, but will leave that open for discussion since this behavior already exists.
This was referenced Nov 6, 2015
|
Looks good. It makes sense to me that the code should not reference the HTTP_VERSION, especially if it results in Rack setting an environment variable that other apps may expect to come from the client. |
|
@tenderlove this looks like a good change to me. Can you confirm you are happy to merge? |
|
I will add entry to changelog if so. |
rafaelfranca
approved these changes
Nov 15, 2019
|
Rebased and merged by hand. |
|
Happy 4 year anniversary all! 🎉 |
lloeki
added a commit
to lloeki/thin
that referenced
this pull request
Jan 2, 2024
This spec failed:
Thin::Request parser should not fuck up on stupid fucked IE6 headers
with:
should validate with Rack Lint: env[HTTP_VERSION] does not equal env[SERVER_PROTOCOL]
where:
"HTTP_VERSION"=>"HTTP/1.0",
"SERVER_PROTOCOL"=>"HTTP/1.1",
Indeed:
> Rack::HTTP_VERSION has been removed and the HTTP_VERSION env setting is no longer set in the CGI and Webrick handlers
See:
- rack/rack#970
- rack/rack#969
- rack/rack#1691
The version is necessary to negotiate `persistent?`, so here we punt on
a further refactoring to find a good way to get the request HTTP version
from the first line, instead shoving it in a Thin-specific
`thin.request_http_header`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
HTTP_VERSION is supposed to be a client supplied header. This usage
inside Rack is conflating it with SERVER_PROTOCOL, which imo is instead
also conflating it with the client's HTTP version from the request line.
In any of these cases, HTTP_VERSION is set when an existing Version
header doesn't already exist. So it's possible to send a Version header
to conflict with the expected behaviors.
According to the CGI spec
(https://tools.ietf.org/html/draft-robinson-www-interface-00)
This is an anscillary issue with Rack, but will leave that open for
discussion since this behavior already exists.
Notes
It's worth noting that this incorrect behavior is happening as a result of https://github.com/rack/rack/blob/028438f/lib/rack/handler/cgi.rb#L29 (and in all other handlers). I believe this behavior should be changed and not set to HTTP_VERSION in the first place. See my follow up: #970
Lastly, I'm not super familiar with Rack, but this seems problematic to rely on
SERVER_PROTOCOLforTransfer-Encodingbehaviors since what you really want is the client's HTTP version. I've looked into WEBrick only, but theirSERVER_PROTOCOLdoes NOT mean the protocol of the request. WEBrick has a separaterequest.http_versionas opposed torequest.env_meta['SERVER_PROTOCOL']. Not sure about the others.