Skip to content

Commit

Permalink
Do not reference HTTP_VERSION internally
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
mattrobenolt authored and ioquatix committed Nov 20, 2019
1 parent d55193d commit e702d31
Show file tree
Hide file tree
Showing 3 changed files with 7 additions and 7 deletions.
4 changes: 2 additions & 2 deletions lib/rack/chunked.rb
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ def initialize(app)
# a version (nor response headers)
def chunkable_version?(ver)
case ver
when "HTTP/1.0", nil, "HTTP/0.9"
when 'HTTP/1.0', nil, 'HTTP/0.9'
false
else
true
Expand All @@ -73,7 +73,7 @@ def call(env)
status, headers, body = @app.call(env)
headers = HeaderHash.new(headers)

if ! chunkable_version?(env[HTTP_VERSION]) ||
if ! chunkable_version?(env[SERVER_PROTOCOL]) ||
STATUS_WITH_NO_ENTITY_BODY.key?(status.to_i) ||
headers[CONTENT_LENGTH] ||
headers[TRANSFER_ENCODING]
Expand Down
2 changes: 1 addition & 1 deletion lib/rack/common_logger.rb
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ def log(env, status, header, began_at)
env[REQUEST_METHOD],
env[PATH_INFO],
env[QUERY_STRING].empty? ? "" : "?#{env[QUERY_STRING]}",
env[HTTP_VERSION],
env[SERVER_PROTOCOL],
status.to_s[0..3],
length,
Utils.clock_time - began_at ]
Expand Down
8 changes: 4 additions & 4 deletions test/spec_chunked.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ def chunked(app)

before do
@env = Rack::MockRequest.
env_for('/', 'HTTP_VERSION' => '1.1', 'REQUEST_METHOD' => 'GET')
env_for('/', 'SERVER_PROTOCOL' => 'HTTP/1.1', 'REQUEST_METHOD' => 'GET')
end

class TrailerBody
Expand Down Expand Up @@ -81,7 +81,7 @@ def trailers

it 'not modify response when client is HTTP/1.0' do
app = lambda { |env| [200, { "Content-Type" => "text/plain" }, ['Hello', ' ', 'World!']] }
@env['HTTP_VERSION'] = 'HTTP/1.0'
@env['SERVER_PROTOCOL'] = 'HTTP/1.0'
status, headers, body = chunked(app).call(@env)
status.must_equal 200
headers.wont_include 'Transfer-Encoding'
Expand All @@ -97,10 +97,10 @@ def trailers
body.join.must_equal 'Hello World!'
end

@env.delete('HTTP_VERSION') # unicorn will do this on pre-HTTP/1.0 requests
@env.delete('SERVER_PROTOCOL') # unicorn will do this on pre-HTTP/1.0 requests
check.call

@env['HTTP_VERSION'] = 'HTTP/0.9' # not sure if this happens in practice
@env['SERVER_PROTOCOL'] = 'HTTP/0.9' # not sure if this happens in practice
check.call
end

Expand Down

0 comments on commit e702d31

Please sign in to comment.