-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add SERVER_PROTOCOL to SPEC #1883
Conversation
SPEC currently does not currently specify a way to get the HTTP version in use. However, both Chunked and CommonLogger need access to the http version for correct functioning, and other users in the rack ecosystem need it as well (Roda needs it, and I've just identified a need for it in rack-test). Unicorn, Webrick, and Puma all currently set SERVER_PROTOCOL. However, Puma currently sets SERVER_PROTOCOL statically to HTTP/1.1, unlike Unicorn and Webrick, which set it to the protocol used by the client. Unicorn and Puma set HTTP_VERSION to the protocol used by the client. This specifies that SERVER_PROTOCOL should match the protocol used by the client, that it should be a valid protocol matching HTTP/\d(\.\d)?, and that if HTTP_VERSION is provided, it must match SERVER_PROTOCOL. This will require minor changes to Puma to be compliant with the new SPEC. Set SERVER_PROTOCOL to HTTP/1.1 by default in Rack::MockRequest, allowing it to be set by the :http_version option. Update CommonLogger specs to include the version. This removes a spec in Chunked for usage without SERVER_PROTOCOL. A comment in the removed lines indicate unicorn will not set SERVER_PROTOCOL for HTTP/0.9 requests, but that is incorrect, as unicorn has set SERVER_PROTOCOL to HTTP/0.9 since 2009 (see unicorn commit bd0599c4ac91d95cae1f34df3ae99c92f3225391). The related comment was correct when added in 2009 (rack commit 895beec), but has been incorrect since the code was changed from HTTP_VERSION to SERVER_PROTOCOL in 2015 (rack commit e702d31).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work.
Question about 'if HTTP_VERSION is provided, it must match SERVER_PROTOCOL' Not sure about how to handle an edge case here. If the client sets a VERSION header (or has more than one), HTTP_VERSION Also, see puma/puma#2870 |
That's actually a good question, since at least in theory |
I edited my message, see the Puma issue. When that was posted, I looked around for mention of a VERSION/HTTP_VERSION header, and found essentially nothing...
I might say 'may be' rather than 'is', which is what's causing the whole issue... |
I posted comments regarding |
Thank you for the response. In your summary of some current Ruby app servers, you don't mention how they handle a client provided VERSION header, which is the issue here. Possibly, the rack spec could be changed to something similar to 'if HTTP_VERSION is provided, its first value must match SERVER_PROTOCOL'? EDIT: mentioned in Puma comment -
I think dropping any HTTP_VERSION requirements (ignoring it) in Rack4 is a good idea. |
Rack app: [200, {}, [env.values_at('SERVER_PROTOCOL', 'HTTP_VERSION').inspect+"\n"]] HTTP/1.0 requests without version header (
HTTP/1.0 requests with version header (
I think the problem with that is that many applications already rely on HTTP_VERSION as the version of HTTP in use. Older versions of Rack used HTTP_VERSION, if you wanted to differentiate between HTTP/1.0 and HTTP/1.1 when using Puma (the most popular Ruby web server), you have to use HTTP_VERSION. If we are going to change the rack 3 SPEC regarding this, we should specify that HTTP_VERSION must be the value of the |
Thank you for the data, and I had yet to look at Rails, thanks for that info. From the perspective of current compatibility, I think Unicorn seems to provide the best implementation, even though it ignores a client's value for VERSION. Puma is easily patched to fix this, not sure about whether it would be considered a breaking change. Pinging @nateberkopec |
…test-sprint Add rack-session gem when testing rack-attack
SPEC currently does not currently specify a way to get the HTTP
version in use. However, both Chunked and CommonLogger need access
to the http version for correct functioning, and other users in
the rack ecosystem need it as well (Roda needs it, and I've just
identified a need for it in rack-test).
Unicorn, Webrick, and Puma all currently set SERVER_PROTOCOL.
However, Puma currently sets SERVER_PROTOCOL statically to
HTTP/1.1, unlike Unicorn and Webrick, which set it to the
protocol used by the client. Unicorn and Puma set HTTP_VERSION
to the protocol used by the client.
This specifies that SERVER_PROTOCOL should match the protocol
used by the client, that it should be a valid protocol matching
HTTP/\d(.\d)?, and that if HTTP_VERSION is provided, it must
match SERVER_PROTOCOL. This will require minor changes to Puma
to be compliant with the new SPEC.
Set SERVER_PROTOCOL to HTTP/1.1 by default in Rack::MockRequest,
allowing it to be set by the :http_version option. Update
CommonLogger specs to include the version.
This removes a spec in Chunked for usage without SERVER_PROTOCOL.
A comment in the removed lines indicate unicorn will not set
SERVER_PROTOCOL for HTTP/0.9 requests, but that is incorrect, as
unicorn has set SERVER_PROTOCOL to HTTP/0.9 since 2009 (see unicorn
commit bd0599c4ac91d95cae1f34df3ae99c92f3225391). The related
comment was correct when added in 2009 (rack commit
895beec), but has been incorrect
since the code was changed from HTTP_VERSION to SERVER_PROTOCOL in
2015 (rack commit e702d31).