Skip to content

Commit

Permalink
Make HTTP version handling compatible with Rack 3 SPEC
Browse files Browse the repository at this point in the history
Add RodaRequest#http_version for determining the HTTP version in use.
Use this in the chunked, common_logger, and status_303 plugins.

For the status_303 plugin, allow use of the 303 status for HTTP/2
and higher versions.  Only call super (302) for HTTP/1.0 and HTTP/0.9
versions.

For Rack 3 compatibility, prefer SERVER_PROTOCOL since it is required
to be set in Rack 3.  Otherwise, prefer HTTP_VERSION, since that is
what is used previously.  Puma, the most common Ruby webserver, sets
HTTP_VERSION accurately, but SERVER_PROTOCOL is fixed at 1.1, so
switching to always using SERVER_PROTOCOL could result in incorrect
behavior when using Puma (I'm sure Puma will fix this in their changes
to support Rack 3).
  • Loading branch information
jeremyevans committed May 4, 2022
1 parent 88b6ad4 commit 50f0ddf
Show file tree
Hide file tree
Showing 7 changed files with 47 additions and 17 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
= master

* Make status_303 plugin use 303 responses for HTTP/2 and higher versions (jeremyevans)

* Add RodaRequest#http_version for determining the HTTP version in use (jeremyevans)

* Do not set a body for 405 responses when using the verb methods in the not_allowed plugin (jeremyevans) (#267)

* Support status_handler method :keep_headers option in status_handler plugin (jeremyevans) (#267)
Expand Down
2 changes: 1 addition & 1 deletion lib/roda/plugins/chunked.rb
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ def view(*a)
# an overview. If a block is given, it is passed to #delay.
def chunked(template, opts=OPTS, &block)
unless defined?(@_chunked)
@_chunked = !self.opts[:force_chunked_encoding] || env['HTTP_VERSION'] == "HTTP/1.1"
@_chunked = !self.opts[:force_chunked_encoding] || @_request.http_version == "HTTP/1.1"
end

if block
Expand Down
2 changes: 1 addition & 1 deletion lib/roda/plugins/common_logger.rb
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ def _roda_after_90__common_logger(result)

env = @_request.env

opts[:common_logger_meth].call("#{env['HTTP_X_FORWARDED_FOR'] || env["REMOTE_ADDR"] || "-"} - #{env["REMOTE_USER"] || "-"} [#{Time.now.strftime("%d/%b/%Y:%H:%M:%S %z")}] \"#{env["REQUEST_METHOD"]} #{env["SCRIPT_NAME"]}#{env["PATH_INFO"]}#{"?#{env["QUERY_STRING"]}" if ((qs = env["QUERY_STRING"]) && !qs.empty?)} #{env["HTTP_VERSION"]}\" #{result[0]} #{((length = result[1]['Content-Length']) && (length unless length == '0')) || '-'} #{elapsed_time}\n")
opts[:common_logger_meth].call("#{env['HTTP_X_FORWARDED_FOR'] || env["REMOTE_ADDR"] || "-"} - #{env["REMOTE_USER"] || "-"} [#{Time.now.strftime("%d/%b/%Y:%H:%M:%S %z")}] \"#{env["REQUEST_METHOD"]} #{env["SCRIPT_NAME"]}#{env["PATH_INFO"]}#{"?#{env["QUERY_STRING"]}" if ((qs = env["QUERY_STRING"]) && !qs.empty?)} #{@_request.http_version}\" #{result[0]} #{((length = result[1]['Content-Length']) && (length unless length == '0')) || '-'} #{elapsed_time}\n")
end

# Create timer instance used for timing
Expand Down
9 changes: 6 additions & 3 deletions lib/roda/plugins/status_303.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,13 @@ module RequestMethods
private

def default_redirect_status
if env['HTTP_VERSION'] == 'HTTP/1.1' && !is_get?
303
else
return super if is_get?

case http_version
when 'HTTP/1.0', 'HTTP/0.9', nil
super
else
303
end
end
end
Expand Down
21 changes: 21 additions & 0 deletions lib/roda/request.rb
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,27 @@ def inspect
"#<#{self.class.inspect} #{@env["REQUEST_METHOD"]} #{path}>"
end

# :nocov:
if Rack.release >= '2.3'
def http_version
# Prefer SERVER_PROTOCOL as it is required in Rack 3.
# Still fall back to HTTP_VERSION if SERVER_PROTOCOL
# is not set, in case the server in use is not Rack 3
# compliant.
@env['SERVER_PROTOCOL'] || @env['HTTP_VERSION']
end
else
# :nocov:
# What HTTP version the request was submitted with.
def http_version
# Prefer HTTP_VERSION as it is backwards compatible
# with previous Roda versions. Fallback to
# SERVER_PROTOCOL for servers that do not set
# HTTP_VERSION.
@env['HTTP_VERSION'] || @env['SERVER_PROTOCOL']
end
end

# Does a terminal match on the current path, matching only if the arguments
# have fully matched the path. If it matches, the match block is
# executed, and when the match block returns, the rack response is
Expand Down
24 changes: 12 additions & 12 deletions spec/plugin/common_logger_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@ def cl_app(&block)
it 'logs requests to given logger/stream' do
cl_app(&:path_info)

body.must_equal '/'
body("HTTP_VERSION"=>'HTTP/1.0').must_equal '/'
@logger.rewind
@logger.read.must_match(/\A- - - \[\d\d\/[A-Z][a-z]{2}\/\d\d\d\d:\d\d:\d\d:\d\d [-+]\d\d\d\d\] "GET \/ " 200 1 0.\d\d\d\d\n\z/)
@logger.read.must_match(/\A- - - \[\d\d\/[A-Z][a-z]{2}\/\d\d\d\d:\d\d:\d\d:\d\d [-+]\d\d\d\d\] "GET \/ HTTP\/1.0" 200 1 0.\d\d\d\d\n\z/)

@logger.rewind
@logger.truncate(0)
Expand All @@ -37,9 +37,9 @@ def cl_app(&block)
@app.plugin :common_logger, Logger.new(@logger)
@logger.rewind
@logger.truncate(0)
body.must_equal '/'
body("HTTP_VERSION"=>'HTTP/1.0').must_equal '/'
@logger.rewind
@logger.read.must_match(/\A- - - \[\d\d\/[A-Z][a-z]{2}\/\d\d\d\d:\d\d:\d\d:\d\d [-+]\d\d\d\d\] "GET \/ " 200 1 0.\d\d\d\d\n\z/)
@logger.read.must_match(/\A- - - \[\d\d\/[A-Z][a-z]{2}\/\d\d\d\d:\d\d:\d\d:\d\d [-+]\d\d\d\d\] "GET \/ HTTP\/1.0" 200 1 0.\d\d\d\d\n\z/)
end

it 'skips timer information if not available' do
Expand All @@ -48,19 +48,19 @@ def cl_app(&block)
r.path_info
end

body.must_equal '/'
body("HTTP_VERSION"=>'HTTP/1.0').must_equal '/'
@logger.rewind
@logger.read.must_match(/\A- - - \[\d\d\/[A-Z][a-z]{2}\/\d\d\d\d:\d\d:\d\d:\d\d [-+]\d\d\d\d\] "GET \/ " 200 1 -\n\z/)
@logger.read.must_match(/\A- - - \[\d\d\/[A-Z][a-z]{2}\/\d\d\d\d:\d\d:\d\d:\d\d [-+]\d\d\d\d\] "GET \/ HTTP\/1.0" 200 1 -\n\z/)
end

it 'skips length information if not available' do
cl_app do |r|
r.halt [500, {}, []]
end

body.must_equal ''
body("HTTP_VERSION"=>'HTTP/1.0').must_equal ''
@logger.rewind
@logger.read.must_match(/\A- - - \[\d\d\/[A-Z][a-z]{2}\/\d\d\d\d:\d\d:\d\d:\d\d [-+]\d\d\d\d\] "GET \/ " 500 - 0.\d\d\d\d\n\z/)
@logger.read.must_match(/\A- - - \[\d\d\/[A-Z][a-z]{2}\/\d\d\d\d:\d\d:\d\d:\d\d [-+]\d\d\d\d\] "GET \/ HTTP\/1.0" 500 - 0.\d\d\d\d\n\z/)
end

it 'does not log if an error is raised' do
Expand All @@ -84,9 +84,9 @@ def cl_app(&block)
"bad"
end

body.must_equal 'bad'
body("HTTP_VERSION"=>'HTTP/1.0').must_equal 'bad'
@logger.rewind
@logger.read.must_match(/\A- - - \[\d\d\/[A-Z][a-z]{2}\/\d\d\d\d:\d\d:\d\d:\d\d [-+]\d\d\d\d\] "GET \/ " 500 3 0.\d\d\d\d\n\z/)
@logger.read.must_match(/\A- - - \[\d\d\/[A-Z][a-z]{2}\/\d\d\d\d:\d\d:\d\d:\d\d [-+]\d\d\d\d\] "GET \/ HTTP\/1.0" 500 3 0.\d\d\d\d\n\z/)
end

def cl_app_meth(&block)
Expand All @@ -104,8 +104,8 @@ def debug(str)
r.halt [500, {}, []]
end

body.must_equal ''
body("HTTP_VERSION"=>'HTTP/1.0').must_equal ''
@logger.rewind
@logger.read.must_match(/\ADEBUG - - - \[\d\d\/[A-Z][a-z]{2}\/\d\d\d\d:\d\d:\d\d:\d\d [-+]\d\d\d\d\] "GET \/ " 500 - 0.\d\d\d\d\n\z/)
@logger.read.must_match(/\ADEBUG - - - \[\d\d\/[A-Z][a-z]{2}\/\d\d\d\d:\d\d:\d\d:\d\d [-+]\d\d\d\d\] "GET \/ HTTP\/1.0" 500 - 0.\d\d\d\d\n\z/)
end
end
2 changes: 2 additions & 0 deletions spec/spec_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,8 @@ def _req(app, env)
env = {"REQUEST_METHOD" => "GET", "PATH_INFO" => "/", "SCRIPT_NAME" => ""}.merge(env)
if ENV['LINT']
env['SERVER_NAME'] ||= 'example.com'
env['SERVER_PROTOCOL'] ||= env['HTTP_VERSION'] || 'HTTP/1.0'
env['HTTP_VERSION'] ||= env['SERVER_PROTOCOL']
env['QUERY_STRING'] ||= ''
env['rack.input'] ||= rack_input
env['rack.errors'] ||= StringIO.new
Expand Down

0 comments on commit 50f0ddf

Please sign in to comment.