Make ConditionalGet follow RFC 7232 precedence

RFC 7232 specifies that if both If-None-Match and If-Modified-Since
are present, then If-Modified-Since should be ignored. The
previous behavior considered both if both were provided (it
predated RFC 7232).

While here, refactor and add documentation.  Remove some checks
from private methods where the caller performs the same check.

3 files changed, 30 insertions, 14 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 40566c40..14ee89dd 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -23,7 +23,8 @@ All notable changes to this project will be documented in this file. For info on
 
 ### Changed
 
-- `.ru` files now support the `frozen-string-literal` magic comment. ([@eregon](https://github.com/eregon))
+- `ConditionalGet` follows RFC 7232 precedence if both If-None-Match and If-Modified-Since headers are provided. ([@jeremyevans](https://github.com/jeremyevans))
+- `.ru` files supports the `frozen-string-literal` magic comment. ([@eregon](https://github.com/eregon))
 - Rely on autoload to load constants instead of requiring internal files, make sure to require 'rack' and not just 'rack/...'. ([@jeremyevans](https://github.com/jeremyevans))
 - `Etag` will continue sending ETag even if the response should not be cached. ([@henm](https://github.com/henm))
 - `Request#host_with_port` no longer includes a colon for a missing or empty port. ([@AlexWayfer](https://github.com/AlexWayfer))
diff --git a/lib/rack/conditional_get.rb b/lib/rack/conditional_get.rb
index 24200053..0a2d787b 100644
--- a/lib/rack/conditional_get.rb
+++ b/lib/rack/conditional_get.rb
@@ -19,6 +19,8 @@ module Rack
       @app = app
     end
 
+    # Return empty 304 response if the response has not been
+    # modified since the last request.
     def call(env)
       case env[REQUEST_METHOD]
       when "GET", "HEAD"
@@ -41,28 +43,32 @@ module Rack
 
   private
 
+    # Return whether the response has not been modified since the
+    # last request.
     def fresh?(env, headers)
-      modified_since = env['HTTP_IF_MODIFIED_SINCE']
-      none_match     = env['HTTP_IF_NONE_MATCH']
-
-      return false unless modified_since || none_match
-
-      success = true
-      success &&= modified_since?(to_rfc2822(modified_since), headers) if modified_since
-      success &&= etag_matches?(none_match, headers) if none_match
-      success
+      # If-None-Match has priority over If-Modified-Since per RFC 7232
+      if none_match = env['HTTP_IF_NONE_MATCH']
+        etag_matches?(none_match, headers)
+      elsif (modified_since = env['HTTP_IF_MODIFIED_SINCE']) && (modified_since = to_rfc2822(modified_since))
+        modified_since?(modified_since, headers)
+      end
     end
 
+    # Whether the ETag response header matches the If-None-Match request header.
+    # If so, the request has not been modified.
     def etag_matches?(none_match, headers)
-      etag = headers['ETag'] and etag == none_match
+      headers['ETag'] == none_match
     end
 
+    # Whether the Last-Modified response header matches the If-Modified-Since
+    # request header.  If so, the request has not been modified.
     def modified_since?(modified_since, headers)
       last_modified = to_rfc2822(headers['Last-Modified']) and
-        modified_since and
         modified_since >= last_modified
     end
 
+    # Return a Time object for the given string (which should be in RFC2822
+    # format), or nil if the string cannot be parsed.
     def to_rfc2822(since)
       # shortest possible valid date is the obsolete: 1 Nov 97 09:55 A
       # anything shorter is invalid, this avoids exceptions for common cases
@@ -71,8 +77,6 @@ module Rack
         # NOTE: there is no trivial way to write this in a non exception way
         #   _rfc2822 returns a hash but is not that usable
         Time.rfc2822(since) rescue nil
-      else
-        nil
       end
     end
   end
diff --git a/test/spec_conditional_get.rb b/test/spec_conditional_get.rb
index f64faf41..5d517be4 100644
--- a/test/spec_conditional_get.rb
+++ b/test/spec_conditional_get.rb
@@ -42,6 +42,17 @@ describe Rack::ConditionalGet do
     response.body.must_be :empty?
   end
 
+  it "set a 304 status and truncate body when If-None-Match hits but If-Modified-Since is after Last-Modified" do
+    app = conditional_get(lambda { |env|
+      [200, { 'Last-Modified' => (Time.now + 3600).httpdate, 'Etag' => '1234', 'Content-Type' => 'text/plain' }, ['TEST']] })
+
+    response = Rack::MockRequest.new(app).
+      get("/", 'HTTP_IF_MODIFIED_SINCE' => Time.now.httpdate, 'HTTP_IF_NONE_MATCH' => '1234')
+
+    response.status.must_equal 304
+    response.body.must_be :empty?
+  end
+
   it "not set a 304 status if If-Modified-Since hits but Etag does not" do
     timestamp = Time.now.httpdate
     app = conditional_get(lambda { |env|