-
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
Introduce rack.protocol
header for handling version-agnostic protocol upgrades.
#1954
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -78,8 +78,9 @@ def response | |
end | ||
|
||
## and the *body*. | ||
check_content_type(@status, @headers) | ||
check_content_length(@status, @headers) | ||
check_content_type_header(@status, @headers) | ||
check_content_length_header(@status, @headers) | ||
check_rack_protocol_header(@status, @headers) | ||
@head_request = @env[REQUEST_METHOD] == HEAD | ||
|
||
@lint = (@env['rack.lint'] ||= []) << self | ||
|
@@ -179,6 +180,11 @@ def check_environment(env) | |
## to +call+ that is used to perform a full | ||
## hijack. | ||
|
||
## <tt>rack.protocol</tt>:: If the request is an HTTP/1 upgrade or | ||
## HTTP/2 CONNECT with +:protocol+ pseudo | ||
## header, this is set to an +Array+ | ||
## containing the value(s) of that header. | ||
|
||
## Additional environment specifications have approved to | ||
## standardized middleware APIs. None of these are required to | ||
## be implemented by the server. | ||
|
@@ -671,9 +677,9 @@ def check_header_value(key, value) | |
end | ||
|
||
## | ||
## === The content-type | ||
## ==== The +content-type+ Header | ||
## | ||
def check_content_type(status, headers) | ||
def check_content_type_header(status, headers) | ||
headers.each { |key, value| | ||
## There must not be a <tt>content-type</tt> header key when the +Status+ is 1xx, | ||
## 204, or 304. | ||
|
@@ -687,9 +693,9 @@ def check_content_type(status, headers) | |
end | ||
|
||
## | ||
## === The content-length | ||
## ==== The +content-length+ Header | ||
## | ||
def check_content_length(status, headers) | ||
def check_content_length_header(status, headers) | ||
headers.each { |key, value| | ||
if key == 'content-length' | ||
## There must not be a <tt>content-length</tt> header key when the | ||
|
@@ -714,6 +720,28 @@ def verify_content_length(size) | |
end | ||
end | ||
|
||
## | ||
## ==== The +rack.protocol+ Header | ||
## | ||
def check_rack_protocol_header(status, headers) | ||
## If the +rack.protocol+ header is present, it must be a String, and | ||
## must be one of the values from the +rack.protocol+ array from the | ||
## environment. | ||
protocol = headers['rack.protocol'] | ||
if protocol | ||
request_protocols = Array(@env['rack.protocol']) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the env entry is required to be an array if present, we shouldn't need to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm okay with that, however in HTTP/2, the semantics are that it should only contain a single value, i.e. a string, while in HTTP/1, it can have multiple values. I'm not sure if we want to just specify that it can be a string (single value HTTP/2 semantics) or Array (multiple values HTTP/1 semantics) or an Array with a single value (HTTP/2 wrapped in HTTP/1 semantics). It's probably easier to use if we always assume it's an Array, but 99.9999999999% of the time, this will always be a single value, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it is going to 99.9999999999% be a string in HTTP/1, and 100% in HTTP/2, we probably should just specify it must be a string. If a client submits There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd be okay with this, but
|
||
|
||
if request_protocols.empty? | ||
raise LintError, "rack.protocol header is #{protocol.inspect}, but rack.protocol was not set in request!" | ||
elsif !request_protocols.include?(protocol) | ||
raise LintError, "rack.protocol header is #{protocol.inspect}, but should be one of #{request_protocols.inspect} from the request!" | ||
end | ||
end | ||
end | ||
## | ||
## Setting this value informs the server that it should perform a | ||
## connection upgrade. In HTTP/1, this is done using the +upgrade+ | ||
## header. In HTTP/2, this is done by accepting the request. | ||
## | ||
## === The Body | ||
## | ||
|
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.
I'm not in favor of the
rack.protocol
naming. It seems like it would be which HTTP version is in use (SERVER_PROTOCOL
) orhttp
orhttps
(rack.url_scheme
).I assume the HTTP/1 upgrade header is already available in
HTTP_UPGRADE
. Is the HTTP/2 protocol pseudo header exposed anywhere yet?Absent a better idea, I recommend switching to
rack.upgrade_protocol
, since it looks this this is used for upgrading the protocol used for the connection.Regardless of what we select, I think we want to explicitly specify that this in an array (even if it can only contain one value in certain HTTP versions), since it looks like we specify that the env key should be an array later.
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.
HTTP/2 literally called this the
:protocol
pseudo header. I actually don't have a strong opinion about the name but I've already used this name for my current implementation of websocket simply because I had to choose something and it seemed like the logical approach.I also checked https://datatracker.ietf.org/doc/html/rfc3875 and could find no reference about how to handle
connection: upgrade
requests. It doesn't seem like the CGI specification which we follow for ourenv
keys has any guidance on the matter.rack.upgrade_protocol
- I would suggest we avoid this, or the "upgrade" terminology in general, as HTTP/2 explicitly calls this out in a number of places as being unsupported and I think the word "upgrade" is deprecated in favour of "connect" and "protocol".The specific introduction of the protocol pseudo header is here: https://www.rfc-editor.org/rfc/rfc8441.html#section-4
I'm fine with specifying that it should be an
Array
.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.
rack.connect_protocol
maybe?rack.protocol
is too confusing as a name for this, IMO.I would still like to know how changing the connection protocol is currently handled for HTTP/2 in rack. Is it impossible with the current design, does it require server-specific code, or is it possible and just implemented a different way?
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.
rack.protocol
is the intersection of all the different semantics.connect_protocol
is too specific to HTTP/2 that it makes the HTTP/1 use case clumsy. It doesn't make a lot of sense to haverack.connect_protocol
in the response headers map to an HTTP/1 connection upgrade IMHO. I understand the value of being more specific, but this isn't all or nothing, the intersection between HTTP/1 and HTTP/2 semantics is very wide and doesn't invite more specific naming IMHO. In other words, being too specific in the naming is equally confusing.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.
I probably should have been more specific. There is nothing that ties the
:protocol
pseudo header to theCONNECT
method, so linking them asrack.connect_protocol
would be a bit clumsy if later on another method was introduced that also used the:protocol
pseudo header with a different method.