Strong ETag validators

* Introduce `Response#strong_etag=` and `#weak_etag=` and analogous options
  for `fresh_when` and `stale?`. `Response#etag=` sets a weak ETag.

  Strong ETags are desirable when you're serving byte-for-byte identical
  responses that support Range requests, like PDFs or videos (typically
  done by reproxying the response from a backend storage service).
  Also desirable when fronted by some CDNs that support strong ETags
  only, like Akamai.

* No longer strips quotes (`"`) from ETag values before comparing them.
  Quotes are significant, part of the ETag. A quoted ETag and an unquoted
  one are not the same entity.

* Support `If-None-Match: *`. Rarely useful for GET requests; meant
  to provide some optimistic concurrency control for PUT requests.

6 files changed, 210 insertions, 63 deletions

diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
index 4f5b81d04a..7d400eaa22 100644
--- a/actionpack/CHANGELOG.md
+++ b/actionpack/CHANGELOG.md
@@ -1,3 +1,25 @@
+*   ETags: Introduce `Response#strong_etag=` and `#weak_etag=` and analogous
+    options for `fresh_when` and `stale?`. `Response#etag=` sets a weak ETag.
+
+    Strong ETags are desirable when you're serving byte-for-byte identical
+    responses that support Range requests, like PDFs or videos (typically
+    done by reproxying the response from a backend storage service).
+    Also desirable when fronted by some CDNs that support strong ETags
+    only, like Akamai.
+
+    *Jeremy Daer*
+
+*   ETags: No longer strips quotes (") from ETag values before comparing them.
+    Quotes are significant, part of the ETag. A quoted ETag and an unquoted
+    one are not the same entity.
+
+    *Jeremy Daer*
+
+*   ETags: Support `If-None-Match: *`. Rarely useful for GET requests; meant
+    to provide some optimistic concurrency control for PUT requests.
+
+    *Jeremy Daer*
+
 *   `ActionDispatch::ParamsParser` is deprecated and was removed from the middleware
     stack. To configure the parameter parsers use `ActionDispatch::Request.parameter_parsers=`.
 
diff --git a/actionpack/lib/action_controller/metal/conditional_get.rb b/actionpack/lib/action_controller/metal/conditional_get.rb
index 35befc05e1..00b380d3dc 100644
--- a/actionpack/lib/action_controller/metal/conditional_get.rb
+++ b/actionpack/lib/action_controller/metal/conditional_get.rb
@@ -36,8 +36,23 @@ module ActionController
     #
     # === Parameters:
     #
-    # * <tt>:etag</tt>.
-    # * <tt>:last_modified</tt>.
+    # * <tt>:etag</tt> Sets a "weak" ETag validator on the response. See the
+    #   +:weak_etag+ option.
+    # * <tt>:weak_etag</tt> Sets a "weak" ETag validator on the response.
+    #   requests that set If-None-Match header may return a 304 Not Modified
+    #   response if it matches the ETag exactly. A weak ETag indicates semantic
+    #   equivalence, not byte-for-byte equality, so they're a good for caching
+    #   HTML pages in browser caches. They can't be used for responses that
+    #   must be byte-identical, like serving Range requests within a PDF file.
+    # * <tt>:strong_etag</tt> Sets a "strong" ETag validator on the response.
+    #   Requests that set If-None-Match header may return a 304 Not Modified
+    #   response if it matches the ETag exactly. A strong ETag implies exact
+    #   equality: the response must match byte for byte. This is necessary for
+    #   doing Range requests within a large video or PDF file, for example, or
+    #   for compatibility with some CDNs that don't support weak ETags.
+    # * <tt>:last_modified</tt> Sets a "weak" last-update validator on the
+    #   response. Subsequent requests that set If-Modified-Since may return a
+    #   304 Not Modified response if last_modified <= If-Modified-Since.
     # * <tt>:public</tt> By default the Cache-Control header is private, set this to
     #   +true+ if you want your application to be cacheable by other devices (proxy caches).
     # * <tt>:template</tt> By default, the template digest for the current
@@ -86,12 +101,16 @@ module ActionController
     #
     #   before_action { fresh_when @article, template: 'widgets/show' }
     #
-    def fresh_when(object = nil, etag: object, last_modified: nil, public: false, template: nil)
+    def fresh_when(object = nil, etag: nil, weak_etag: nil, strong_etag: nil, last_modified: nil, public: false, template: nil)
+      weak_etag ||= etag || object unless strong_etag
       last_modified ||= object.try(:updated_at) || object.try(:maximum, :updated_at)
 
-      if etag || template
-        response.etag = combine_etags(etag: etag, last_modified: last_modified,
-          public: public, template: template)
+      if strong_etag
+        response.strong_etag = combine_etags strong_etag,
+          last_modified: last_modified, public: public, template: template
+      elsif weak_etag || template
+        response.weak_etag = combine_etags weak_etag,
+          last_modified: last_modified, public: public, template: template
       end
 
       response.last_modified = last_modified if last_modified
@@ -107,8 +126,23 @@ module ActionController
     #
     # === Parameters:
     #
-    # * <tt>:etag</tt>.
-    # * <tt>:last_modified</tt>.
+    # * <tt>:etag</tt> Sets a "weak" ETag validator on the response. See the
+    #   +:weak_etag+ option.
+    # * <tt>:weak_etag</tt> Sets a "weak" ETag validator on the response.
+    #   requests that set If-None-Match header may return a 304 Not Modified
+    #   response if it matches the ETag exactly. A weak ETag indicates semantic
+    #   equivalence, not byte-for-byte equality, so they're a good for caching
+    #   HTML pages in browser caches. They can't be used for responses that
+    #   must be byte-identical, like serving Range requests within a PDF file.
+    # * <tt>:strong_etag</tt> Sets a "strong" ETag validator on the response.
+    #   Requests that set If-None-Match header may return a 304 Not Modified
+    #   response if it matches the ETag exactly. A strong ETag implies exact
+    #   equality: the response must match byte for byte. This is necessary for
+    #   doing Range requests within a large video or PDF file, for example, or
+    #   for compatibility with some CDNs that don't support weak ETags.
+    # * <tt>:last_modified</tt> Sets a "weak" last-update validator on the
+    #   response. Subsequent requests that set If-Modified-Since may return a
+    #   304 Not Modified response if last_modified <= If-Modified-Since.
     # * <tt>:public</tt> By default the Cache-Control header is private, set this to
     #   +true+ if you want your application to be cacheable by other devices (proxy caches).
     # * <tt>:template</tt> By default, the template digest for the current
@@ -180,8 +214,8 @@ module ActionController
     #     super if stale? @article, template: 'widgets/show'
     #   end
     #
-    def stale?(object = nil, etag: object, last_modified: nil, public: nil, template: nil)
-      fresh_when(object, etag: etag, last_modified: last_modified, public: public, template: template)
+    def stale?(object = nil, **freshness_kwargs)
+      fresh_when(object, **freshness_kwargs)
       !request.fresh?(response)
     end
 
@@ -231,9 +265,8 @@ module ActionController
     end
 
     private
-      def combine_etags(options)
-        etags = etaggers.map { |etagger| instance_exec(options, &etagger) }.compact
-        etags.unshift options[:etag]
+      def combine_etags(validator, options)
+        [validator, *etaggers.map { |etagger| instance_exec(options, &etagger) }].compact
       end
   end
 end
diff --git a/actionpack/lib/action_dispatch/http/cache.rb b/actionpack/lib/action_dispatch/http/cache.rb
index 4bd727c14e..9fa2e38ae3 100644
--- a/actionpack/lib/action_dispatch/http/cache.rb
+++ b/actionpack/lib/action_dispatch/http/cache.rb
@@ -17,9 +17,7 @@ module ActionDispatch
         end
 
         def if_none_match_etags
-          (if_none_match ? if_none_match.split(/\s*,\s*/) : []).collect do |etag|
-            etag.gsub(/^\"|\"$/, "")
-          end
+          if_none_match ? if_none_match.split(/\s*,\s*/) : []
         end
 
         def not_modified?(modified_at)
@@ -28,8 +26,8 @@ module ActionDispatch
 
         def etag_matches?(etag)
           if etag
-            etag = etag.gsub(/^\"|\"$/, "")
-            if_none_match_etags.include?(etag)
+            validators = if_none_match_etags
+            validators.include?(etag) || validators.include?('*')
           end
         end
 
@@ -80,27 +78,63 @@ module ActionDispatch
           set_header DATE, utc_time.httpdate
         end
 
-        # This method allows you to set the ETag for cached content, which
-        # will be returned to the end user.
+        # This method sets a weak ETag validator on the response so browsers
+        # and proxies may cache the response, keyed on the ETag. On subsequent
+        # requests, the If-None-Match header is set to the cached ETag. If it
+        # matches the current ETag, we can return a 304 Not Modified response
+        # with no body, letting the browser or proxy know that their cache is
+        # current. Big savings in request time and network bandwidth.
+        #
+        # Weak ETags are considered to be semantically equivalent but not
+        # byte-for-byte identical. This is perfect for browser caching of HTML
+        # pages where we don't care about exact equality, just what the user
+        # is viewing.
         #
-        # By default, Action Dispatch sets all ETags to be weak.
-        # This ensures that if the content changes only semantically,
-        # the whole page doesn't have to be regenerated from scratch
-        # by the web server. With strong ETags, pages are compared
-        # byte by byte, and are regenerated only if they are not exactly equal.
-        def etag=(etag)
-          key = ActiveSupport::Cache.expand_cache_key(etag)
-          super %(W/"#{Digest::MD5.hexdigest(key)}")
+        # Strong ETags are considered byte-for-byte identical. They allow a
+        # browser or proxy cache to support Range requests, useful for paging
+        # through a PDF file or scrubbing through a video. Some CDNs only
+        # support strong ETags and will ignore weak ETags entirely.
+        #
+        # Weak ETags are what we almost always need, so they're the default.
+        # Check out `#strong_etag=` to provide a strong ETag validator.
+        def etag=(weak_validators)
+          self.weak_etag = weak_validators
+        end
+
+        def weak_etag=(weak_validators)
+          set_header 'ETag', generate_weak_etag(weak_validators)
+        end
+
+        def strong_etag=(strong_validators)
+          set_header 'ETag', generate_strong_etag(strong_validators)
         end
 
         def etag?; etag; end
 
+        # True if an ETag is set and it's a weak validator (preceded with W/)
+        def weak_etag?
+          etag? && etag.starts_with?('W/"')
+        end
+
+        # True if an ETag is set and it isn't a weak validator (not preceded with W/)
+        def strong_etag?
+          etag? && !weak_etag?
+        end
+
       private
 
         DATE          = 'Date'.freeze
         LAST_MODIFIED = "Last-Modified".freeze
         SPECIAL_KEYS  = Set.new(%w[extras no-cache max-age public private must-revalidate])
 
+        def generate_weak_etag(validators)
+          "W/#{generate_strong_etag(validators)}"
+        end
+
+        def generate_strong_etag(validators)
+          %("#{Digest::MD5.hexdigest(ActiveSupport::Cache.expand_cache_key(validators))}")
+        end
+
         def cache_control_segments
           if cache_control = _cache_control
             cache_control.delete(' ').split(',')
diff --git a/actionpack/test/controller/render_test.rb b/actionpack/test/controller/render_test.rb
index 82fc8b0f8a..f42efd35af 100644
--- a/actionpack/test/controller/render_test.rb
+++ b/actionpack/test/controller/render_test.rb
@@ -16,6 +16,10 @@ class TestControllerWithExtraEtags < ActionController::Base
     render plain: "stale" if stale?(etag: %w(1 2 3), template: false)
   end
 
+  def strong
+    render plain: "stale" if stale?(strong_etag: 'strong')
+  end
+
   def with_template
     if stale? template: 'test/hello_world'
       render plain: 'stale'
@@ -385,7 +389,7 @@ class LastModifiedRenderTest < ActionController::TestCase
 
   def test_request_not_modified_but_etag_differs
     @request.if_modified_since = @last_modified
-    @request.if_none_match = "234"
+    @request.if_none_match = '"234"'
     get :conditional_hello
     assert_response :success
   end
@@ -414,7 +418,7 @@ class LastModifiedRenderTest < ActionController::TestCase
 
   def test_request_not_modified_but_etag_differs_with_record
     @request.if_modified_since = @last_modified
-    @request.if_none_match = "234"
+    @request.if_none_match = '"234"'
     get :conditional_hello_with_record
     assert_response :success
   end
@@ -442,7 +446,7 @@ class LastModifiedRenderTest < ActionController::TestCase
 
   def test_request_not_modified_but_etag_differs_with_collection_of_records
     @request.if_modified_since = @last_modified
-    @request.if_none_match = "234"
+    @request.if_none_match = '"234"'
     get :conditional_hello_with_collection_of_records
     assert_response :success
   end
@@ -477,8 +481,26 @@ end
 class EtagRenderTest < ActionController::TestCase
   tests TestControllerWithExtraEtags
 
+  def test_strong_etag
+    @request.if_none_match = strong_etag(['strong', 'ab', :cde, [:f]])
+    get :strong
+    assert_response :not_modified
+
+    @request.if_none_match = '*'
+    get :strong
+    assert_response :not_modified
+
+    @request.if_none_match = '"strong"'
+    get :strong
+    assert_response :ok
+
+    @request.if_none_match = weak_etag(['strong', 'ab', :cde, [:f]])
+    get :strong
+    assert_response :ok
+  end
+
   def test_multiple_etags
-    @request.if_none_match = etag(["123", 'ab', :cde, [:f]])
+    @request.if_none_match = weak_etag(["123", 'ab', :cde, [:f]])
     get :fresh
     assert_response :not_modified
 
@@ -488,7 +510,7 @@ class EtagRenderTest < ActionController::TestCase
   end
 
   def test_array
-    @request.if_none_match = etag([%w(1 2 3), 'ab', :cde, [:f]])
+    @request.if_none_match = weak_etag([%w(1 2 3), 'ab', :cde, [:f]])
     get :array
     assert_response :not_modified
 
@@ -523,9 +545,14 @@ class EtagRenderTest < ActionController::TestCase
     end
   end
 
-  def etag(record)
-    %(W/"#{Digest::MD5.hexdigest(ActiveSupport::Cache.expand_cache_key(record))}")
-  end
+  private
+    def weak_etag(record)
+      "W/#{strong_etag record}"
+    end
+
+    def strong_etag(record)
+      %("#{Digest::MD5.hexdigest(ActiveSupport::Cache.expand_cache_key(record))}")
+    end
 end
 
 class MetalRenderTest < ActionController::TestCase
@@ -713,20 +740,24 @@ class HttpCacheForeverTest < ActionController::TestCase
 
   def test_cache_with_public
     get :cache_me_forever, params: {public: true}
+    assert_response :ok
     assert_equal "max-age=#{100.years}, public", @response.headers["Cache-Control"]
     assert_not_nil @response.etag
+    assert @response.weak_etag?
   end
 
   def test_cache_with_private
     get :cache_me_forever
+    assert_response :ok
     assert_equal "max-age=#{100.years}, private", @response.headers["Cache-Control"]
     assert_not_nil @response.etag
-    assert_response :success
+    assert @response.weak_etag?
   end
 
   def test_cache_response_code_with_if_modified_since
     get :cache_me_forever
-    assert_response :success
+    assert_response :ok
+
     @request.if_modified_since = @response.headers['Last-Modified']
     get :cache_me_forever
     assert_response :not_modified
@@ -734,13 +765,10 @@ class HttpCacheForeverTest < ActionController::TestCase
 
   def test_cache_response_code_with_etag
     get :cache_me_forever
-    assert_response :success
-    @request.if_modified_since = @response.headers['Last-Modified']
-    @request.if_none_match = @response.etag
+    assert_response :ok
 
+    @request.if_none_match = @response.etag
     get :cache_me_forever
     assert_response :not_modified
-    @request.if_modified_since = @response.headers['Last-Modified']
-    @request.if_none_match = @response.etag
   end
 end
diff --git a/actionpack/test/dispatch/request_test.rb b/actionpack/test/dispatch/request_test.rb
index 0edad72fd9..a4cb8ce449 100644
--- a/actionpack/test/dispatch/request_test.rb
+++ b/actionpack/test/dispatch/request_test.rb
@@ -1152,36 +1152,41 @@ class RequestParameterFilter < BaseRequestTest
 end
 
 class RequestEtag < BaseRequestTest
-  test "if_none_match_etags none" do
+  test "always matches *" do
+    request = stub_request('HTTP_IF_NONE_MATCH' => '*')
+
+    assert_equal '*', request.if_none_match
+    assert_equal ['*'], request.if_none_match_etags
+
+    assert request.etag_matches?('"strong"')
+    assert request.etag_matches?('W/"weak"')
+    assert_not request.etag_matches?(nil)
+  end
+
+  test "doesn't match absent If-None-Match" do
     request = stub_request
 
     assert_equal nil, request.if_none_match
     assert_equal [], request.if_none_match_etags
-    assert !request.etag_matches?("foo")
-    assert !request.etag_matches?(nil)
-  end
 
-  test "if_none_match_etags single" do
-    header = 'the-etag'
-    request = stub_request('HTTP_IF_NONE_MATCH' => header)
-
-    assert_equal header, request.if_none_match
-    assert_equal [header], request.if_none_match_etags
-    assert request.etag_matches?("the-etag")
+    assert_not request.etag_matches?("foo")
+    assert_not request.etag_matches?(nil)
   end
 
-  test "if_none_match_etags quoted single" do
+  test "matches opaque ETag validators without unquoting" do
     header = '"the-etag"'
     request = stub_request('HTTP_IF_NONE_MATCH' => header)
 
     assert_equal header, request.if_none_match
-    assert_equal ['the-etag'], request.if_none_match_etags
-    assert request.etag_matches?("the-etag")
+    assert_equal ['"the-etag"'], request.if_none_match_etags
+
+    assert request.etag_matches?('"the-etag"')
+    assert_not request.etag_matches?("the-etag")
   end
 
   test "if_none_match_etags multiple" do
     header = 'etag1, etag2, "third etag", "etag4"'
-    expected = ['etag1', 'etag2', 'third etag', 'etag4']
+    expected = ['etag1', 'etag2', '"third etag"', '"etag4"']
     request = stub_request('HTTP_IF_NONE_MATCH' => header)
 
     assert_equal header, request.if_none_match
diff --git a/actionpack/test/dispatch/response_test.rb b/actionpack/test/dispatch/response_test.rb
index cd385982d9..658e0d004b 100644
--- a/actionpack/test/dispatch/response_test.rb
+++ b/actionpack/test/dispatch/response_test.rb
@@ -189,7 +189,7 @@ class ResponseTest < ActiveSupport::TestCase
     assert_equal({"user_name" => "david", "login" => nil}, @response.cookies)
   end
 
-  test "read cache control" do
+  test "read ETag and Cache-Control" do
     resp = ActionDispatch::Response.new.tap { |response|
       response.cache_control[:public] = true
       response.etag = '123'
@@ -197,6 +197,9 @@ class ResponseTest < ActiveSupport::TestCase
     }
     resp.to_a
 
+    assert resp.etag?
+    assert resp.weak_etag?
+    assert_not resp.strong_etag?
     assert_equal('W/"202cb962ac59075b964b07152d234b70"', resp.etag)
     assert_equal({:public => true}, resp.cache_control)
 
@@ -204,6 +207,20 @@ class ResponseTest < ActiveSupport::TestCase
     assert_equal('W/"202cb962ac59075b964b07152d234b70"', resp.headers['ETag'])
   end
 
+  test "read strong ETag" do
+    resp = ActionDispatch::Response.new.tap { |response|
+      response.cache_control[:public] = true
+      response.strong_etag = '123'
+      response.body = 'Hello'
+    }
+    resp.to_a
+
+    assert resp.etag?
+    assert_not resp.weak_etag?
+    assert resp.strong_etag?
+    assert_equal('"202cb962ac59075b964b07152d234b70"', resp.etag)
+  end
+
   test "read charset and content type" do
     resp = ActionDispatch::Response.new.tap { |response|
       response.charset = 'utf-16'
@@ -446,11 +463,19 @@ class ResponseIntegrationTest < ActionDispatch::IntegrationTest
     assert_equal('application/xml; charset=utf-16', @response.headers['Content-Type'])
   end
 
-  test "we can set strong ETag by directly adding it as header" do
-    @response = ActionDispatch::Response.create
-    @response.add_header "ETag", '"202cb962ac59075b964b07152d234b70"'
+  test "strong ETag validator" do
+    @app = lambda { |env|
+      ActionDispatch::Response.new.tap { |resp|
+        resp.strong_etag = '123'
+        resp.body = 'Hello'
+        resp.request = ActionDispatch::Request.empty
+      }.to_a
+    }
+
+    get '/'
+    assert_response :ok
 
-    assert_equal('"202cb962ac59075b964b07152d234b70"', @response.etag)
     assert_equal('"202cb962ac59075b964b07152d234b70"', @response.headers['ETag'])
+    assert_equal('"202cb962ac59075b964b07152d234b70"', @response.etag)
   end
 end