From c1c9c690401bff43b350241cb58b6017ca5fe632 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Thu, 31 Mar 2016 17:47:00 -0700 Subject: [PATCH] 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. --- actionpack/CHANGELOG.md | 22 +++++++ .../metal/conditional_get.rb | 59 +++++++++++++---- actionpack/lib/action_dispatch/http/cache.rb | 64 ++++++++++++++----- actionpack/test/controller/render_test.rb | 58 ++++++++++++----- actionpack/test/dispatch/request_test.rb | 35 +++++----- actionpack/test/dispatch/response_test.rb | 35 ++++++++-- 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 @@ def etag(&etagger) # # === Parameters: # - # * :etag. - # * :last_modified. + # * :etag Sets a "weak" ETag validator on the response. See the + # +:weak_etag+ option. + # * :weak_etag 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. + # * :strong_etag 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. + # * :last_modified 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. # * :public 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). # * :template By default, the template digest for the current @@ -86,12 +101,16 @@ def etag(&etagger) # # 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 @@ def fresh_when(object = nil, etag: object, last_modified: nil, public: false, te # # === Parameters: # - # * :etag. - # * :last_modified. + # * :etag Sets a "weak" ETag validator on the response. See the + # +:weak_etag+ option. + # * :weak_etag 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. + # * :strong_etag 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. + # * :last_modified 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. # * :public 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). # * :template By default, the template digest for the current @@ -180,8 +214,8 @@ def fresh_when(object = nil, etag: object, last_modified: nil, public: false, te # 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 @@ def http_cache_forever(public: false) 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 @@ def if_none_match 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 @@ def not_modified?(modified_at) 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 @@ def date=(utc_time) 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 @@ def array 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 @@ def test_request_not_modified 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 @@ def test_request_not_modified_with_record 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 @@ def test_request_not_modified_with_collection_of_records 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 @@ def test_last_modified_works_with_less_than_too 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 @@ def test_multiple_etags 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 @@ def test_etag_reflects_template_digest 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 @@ def cache_me_forever 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 @@ def test_cache_response_code_with_if_modified_since 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 @@ def test_only_set_charset_still_defaults_to_text_html 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 @@ def test_only_set_charset_still_defaults_to_text_html } 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 @@ def test_only_set_charset_still_defaults_to_text_html 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 -- GitLab