Add SERVER_PROTOCOL to SPEC (#1883)

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
895beec0622d3cafdc5fbae20d665c6d5f6c8e7c), but has been incorrect
since the code was changed from HTTP_VERSION to SERVER_PROTOCOL in
2015 (rack commit e702d31335c1a820e99c3acdd9d3368ac25da010).

8 files changed, 71 insertions, 12 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index cc683dcd..76585f8a 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -18,6 +18,7 @@ All notable changes to this project will be documented in this file. For info on
 - Middleware must no longer call `#each` on the body, but they can call `#to_ary` on the body if it responds to `#to_ary`.
 - `rack.input` is no longer required to be rewindable.
 - `rack.multithread/rack.multiprocess/rack.run_once` are no longer required environment keys.
+- `SERVER_PROTOCOL` is now a required key, matching the HTTP protocol used in the request.
 
 ### Removed
 
diff --git a/SPEC.rdoc b/SPEC.rdoc
index 5d89e09f..0739f534 100644
--- a/SPEC.rdoc
+++ b/SPEC.rdoc
@@ -58,6 +58,8 @@ below.
 <tt>SERVER_PORT</tt>:: An optional +Integer+ which is the port the
                        server is running on. Should be specified if
                        the server is running on a non-standard port.
+<tt>SERVER_PROTOCOL</tt>:: A string representing the HTTP version used
+                           for the request.
 <tt>HTTP_</tt> Variables:: Variables corresponding to the
                            client-supplied HTTP request
                            headers (i.e., variables whose
@@ -117,6 +119,8 @@ accepted specifications and must not be used otherwise.
 The <tt>SERVER_PORT</tt> must be an Integer if set.
 The <tt>SERVER_NAME</tt> must be a valid authority as defined by RFC7540.
 The <tt>HTTP_HOST</tt> must be a valid authority as defined by RFC7540.
+The <tt>SERVER_PROTOCOL</tt> must match the regexp <tt>HTTP/\d(\.\d)?</tt>.
+If the <tt>HTTP_VERSION</tt> is present, it must equal the <tt>SERVER_PROTOCOL</tt>.
 The environment must not contain the keys
 <tt>HTTP_CONTENT_TYPE</tt> or <tt>HTTP_CONTENT_LENGTH</tt>
 (use the versions without <tt>HTTP_</tt>).
diff --git a/lib/rack/lint.rb b/lib/rack/lint.rb
index 0f67f296..682fb084 100755
--- a/lib/rack/lint.rb
+++ b/lib/rack/lint.rb
@@ -143,6 +143,9 @@ module Rack
         ##                        server is running on. Should be specified if
         ##                        the server is running on a non-standard port.
 
+        ## <tt>SERVER_PROTOCOL</tt>:: A string representing the HTTP version used
+        ##                            for the request.
+
         ## <tt>HTTP_</tt> Variables:: Variables corresponding to the
         ##                            client-supplied HTTP request
         ##                            headers (i.e., variables whose
@@ -269,7 +272,7 @@ module Rack
         ## accepted specifications and must not be used otherwise.
         ##
 
-        %w[REQUEST_METHOD SERVER_NAME QUERY_STRING
+        %w[REQUEST_METHOD SERVER_NAME QUERY_STRING SERVER_PROTOCOL
            rack.version rack.input rack.errors].each { |header|
           raise LintError, "env missing required key #{header}" unless env.include? header
         }
@@ -290,6 +293,17 @@ module Rack
           raise LintError, "#{env[HTTP_HOST]} must be a valid authority"
         end
 
+        ## The <tt>SERVER_PROTOCOL</tt> must match the regexp <tt>HTTP/\d(\.\d)?</tt>.
+        server_protocol = env['SERVER_PROTOCOL']
+        unless %r{HTTP/\d(\.\d)?}.match?(server_protocol)
+          raise LintError, "env[SERVER_PROTOCOL] does not match HTTP/\\d(\\.\\d)?"
+        end
+
+        ## If the <tt>HTTP_VERSION</tt> is present, it must equal the <tt>SERVER_PROTOCOL</tt>.
+        if env['HTTP_VERSION'] && env['HTTP_VERSION'] != server_protocol
+          raise LintError, "env[HTTP_VERSION] does not equal env[SERVER_PROTOCOL]"
+        end
+
         ## The environment must not contain the keys
         ## <tt>HTTP_CONTENT_TYPE</tt> or <tt>HTTP_CONTENT_LENGTH</tt>
         ## (use the versions without <tt>HTTP_</tt>).
diff --git a/lib/rack/mock.rb b/lib/rack/mock.rb
index 04e69561..d2fcc7f5 100644
--- a/lib/rack/mock.rb
+++ b/lib/rack/mock.rb
@@ -101,6 +101,7 @@ module Rack
     # Options:
     # :fatal :: Whether to raise an exception if request outputs to rack.errors
     # :input :: The rack.input to set
+    # :http_version :: The SERVER_PROTOCOL to set
     # :method :: The HTTP request method to use
     # :params :: The params to use
     # :script_name :: The SCRIPT_NAME to set
@@ -113,6 +114,7 @@ module Rack
       env[REQUEST_METHOD]  = (opts[:method] ? opts[:method].to_s.upcase : GET).b
       env[SERVER_NAME]     = (uri.host || "example.org").b
       env[SERVER_PORT]     = (uri.port ? uri.port.to_s : "80").b
+      env[SERVER_PROTOCOL] = opts[:http_version] || 'HTTP/1.1'
       env[QUERY_STRING]    = (uri.query.to_s).b
       env[PATH_INFO]       = ((!uri.path || uri.path.empty?) ? "/" : uri.path).b
       env[RACK_URL_SCHEME] = (uri.scheme || "http").b
diff --git a/test/spec_chunked.rb b/test/spec_chunked.rb
index 3fcd4af2..ec3e4fb2 100644
--- a/test/spec_chunked.rb
+++ b/test/spec_chunked.rb
@@ -113,9 +113,6 @@ describe Rack::Chunked do
       body.join.must_equal 'Hello World!'
     end
 
-    @env.delete('SERVER_PROTOCOL') # unicorn will do this on pre-HTTP/1.0 requests
-    check.call
-
     @env['SERVER_PROTOCOL'] = 'HTTP/0.9' # not sure if this happens in practice
     check.call
   end
diff --git a/test/spec_common_logger.rb b/test/spec_common_logger.rb
index 3d562ae5..09995dda 100644
--- a/test/spec_common_logger.rb
+++ b/test/spec_common_logger.rb
@@ -30,14 +30,14 @@ describe Rack::CommonLogger do
     res = Rack::MockRequest.new(Rack::CommonLogger.new(app)).get("/")
 
     res.errors.wont_be :empty?
-    res.errors.must_match(/"GET \/ " 200 #{length} /)
+    res.errors.must_match(/"GET \/ HTTP\/1\.1" 200 #{length} /)
   end
 
   it "log to anything with +write+" do
     log = StringIO.new
     Rack::MockRequest.new(Rack::CommonLogger.new(app, log)).get("/")
 
-    log.string.must_match(/"GET \/ " 200 #{length} /)
+    log.string.must_match(/"GET \/ HTTP\/1\.1" 200 #{length} /)
   end
 
   it "work with standard library logger" do
@@ -45,21 +45,21 @@ describe Rack::CommonLogger do
     log = Logger.new(logdev)
     Rack::MockRequest.new(Rack::CommonLogger.new(app, log)).get("/")
 
-    logdev.string.must_match(/"GET \/ " 200 #{length} /)
+    logdev.string.must_match(/"GET \/ HTTP\/1\.1" 200 #{length} /)
   end
 
   it "log - content length if header is missing" do
     res = Rack::MockRequest.new(Rack::CommonLogger.new(app_without_length)).get("/")
 
     res.errors.wont_be :empty?
-    res.errors.must_match(/"GET \/ " 200 - /)
+    res.errors.must_match(/"GET \/ HTTP\/1\.1" 200 - /)
   end
 
   it "log - content length if header is zero" do
     res = Rack::MockRequest.new(Rack::CommonLogger.new(app_with_zero_length)).get("/")
 
     res.errors.wont_be :empty?
-    res.errors.must_match(/"GET \/ " 200 - /)
+    res.errors.must_match(/"GET \/ HTTP\/1\.1" 200 - /)
   end
 
   it "log - records host from X-Forwarded-For header" do
@@ -94,7 +94,7 @@ describe Rack::CommonLogger do
       Rack::MockRequest.new(Rack::CommonLogger.new(app, log)).get("/")
     end
 
-    md = /- - - \[([^\]]+)\] "(\w+) \/ " (\d{3}) \d+ ([\d\.]+)/.match(log.string)
+    md = /- - - \[([^\]]+)\] "(\w+) \/ HTTP\/1\.1" (\d{3}) \d+ ([\d\.]+)/.match(log.string)
     md.wont_equal nil
     time, method, status, duration = *md.captures
     time.must_equal Time.at(0).strftime("%d/%b/%Y:%H:%M:%S %z")
@@ -108,7 +108,7 @@ describe Rack::CommonLogger do
     log = Logger.new(logdev)
     Rack::MockRequest.new(Rack::CommonLogger.new(app, log)).get("/hello")
 
-    logdev.string.must_match(/"GET \/hello " 200 #{length} /)
+    logdev.string.must_match(/"GET \/hello HTTP\/1\.1" 200 #{length} /)
   end
 
   it "log path with SCRIPT_NAME" do
@@ -116,7 +116,15 @@ describe Rack::CommonLogger do
     log = Logger.new(logdev)
     Rack::MockRequest.new(Rack::CommonLogger.new(app, log)).get("/path", script_name: "/script")
 
-    logdev.string.must_match(/"GET \/script\/path " 200 #{length} /)
+    logdev.string.must_match(/"GET \/script\/path HTTP\/1\.1" 200 #{length} /)
+  end
+
+  it "log path with SERVER_PROTOCOL" do
+    logdev = StringIO.new
+    log = Logger.new(logdev)
+    Rack::MockRequest.new(Rack::CommonLogger.new(app, log)).get("/path", http_version: "HTTP/1.0")
+
+    logdev.string.must_match(/"GET \/path HTTP\/1\.0" 200 #{length} /)
   end
 
   def length
diff --git a/test/spec_lint.rb b/test/spec_lint.rb
index 06d7ffa4..bd9d5d9b 100755
--- a/test/spec_lint.rb
+++ b/test/spec_lint.rb
@@ -46,6 +46,26 @@ describe Rack::Lint do
     }.must_raise(Rack::Lint::LintError).
       message.must_match(/missing required key SERVER_NAME/)
 
+    lambda {
+      e = env
+      e.delete("SERVER_PROTOCOL")
+      Rack::Lint.new(nil).call(e)
+    }.must_raise(Rack::Lint::LintError).
+      message.must_match(/missing required key SERVER_PROTOCOL/)
+
+    lambda {
+      e = env
+      e["SERVER_PROTOCOL"] = 'Foo'
+      Rack::Lint.new(nil).call(e)
+    }.must_raise(Rack::Lint::LintError).
+      message.must_match(/env\[SERVER_PROTOCOL\] does not match HTTP/)
+
+    lambda {
+      e = env
+      e["HTTP_VERSION"] = 'HTTP/1.0'
+      Rack::Lint.new(nil).call(e)
+    }.must_raise(Rack::Lint::LintError).
+      message.must_match(/env\[HTTP_VERSION\] does not equal env\[SERVER_PROTOCOL\]/)
 
     lambda {
       Rack::Lint.new(nil).call(env("HTTP_CONTENT_TYPE" => "text/plain"))
diff --git a/test/spec_mock.rb b/test/spec_mock.rb
index 70a7c6f2..48dfde06 100644
--- a/test/spec_mock.rb
+++ b/test/spec_mock.rb
@@ -64,6 +64,7 @@ describe Rack::MockRequest do
     env["REQUEST_METHOD"].must_equal "GET"
     env["SERVER_NAME"].must_equal "example.org"
     env["SERVER_PORT"].must_equal "80"
+    env["SERVER_PROTOCOL"].must_equal "HTTP/1.1"
     env["QUERY_STRING"].must_equal ""
     env["PATH_INFO"].must_equal "/"
     env["SCRIPT_NAME"].must_equal ""
@@ -172,6 +173,18 @@ describe Rack::MockRequest do
     env["REQUEST_METHOD"].must_equal "GET"
   end
 
+  it "accept :script_name option to set SCRIPT_NAME" do
+    res = Rack::MockRequest.new(app).get("/", script_name: '/foo')
+    env = YAML.unsafe_load(res.body)
+    env["SCRIPT_NAME"].must_equal "/foo"
+  end
+
+  it "accept :http_version option to set SERVER_PROTOCOL" do
+    res = Rack::MockRequest.new(app).get("/", http_version: 'HTTP/1.0')
+    env = YAML.unsafe_load(res.body)
+    env["SERVER_PROTOCOL"].must_equal "HTTP/1.0"
+  end
+
   it "accept params and build query string for GET requests" do
     res = Rack::MockRequest.new(app).get("/foo?baz=2", params: { foo: { bar: "1" } })
     env = YAML.unsafe_load(res.body)