Merge pull request #52274 from Shopify/http-cache

Prefer ETag over Last-Modified for `fresh_when` and `stale?` according to the HTTP specification

actionpack/CHANGELOG.md

@@ -1,3 +1,13 @@
+*   Add `config.action_dispatch.strict_freshness`.
+
+    When set to `true`, the `ETag` header takes precedence over the `Last-Modified` header when both are present,
+    as specificied by RFC 7232, Section 6.
+
+    Defaults to `false` to maintain compatibility with previous versions of Rails, but is enabled as part of
+    Rails 8.0 defaults.
+
+    *heka1024*
+
 *   Support `immutable` directive in Cache-Control
 
     ```ruby

actionpack/lib/action_dispatch/http/cache.rb

@@ -9,6 +9,8 @@ module Request
         HTTP_IF_MODIFIED_SINCE = "HTTP_IF_MODIFIED_SINCE"
         HTTP_IF_NONE_MATCH     = "HTTP_IF_NONE_MATCH"
 
+        mattr_accessor :strict_freshness, default: false
+
         def if_modified_since
           if since = get_header(HTTP_IF_MODIFIED_SINCE)
             Time.rfc2822(since) rescue nil
@@ -34,19 +36,32 @@ def etag_matches?(etag)
           end
         end
 
-        # Check response freshness (`Last-Modified` and ETag) against request
-        # `If-Modified-Since` and `If-None-Match` conditions. If both headers are
-        # supplied, both must match, or the request is not considered fresh.
+        # Check response freshness (`Last-Modified` and `ETag`) against request
+        # `If-Modified-Since` and `If-None-Match` conditions.
+        # If both headers are supplied, based on configuration, either `ETag` is preferred over `Last-Modified`
+        # or both are considered equally. You can adjust the preference with
+        # `config.action_dispatch.strict_freshness`.
+        # Reference: http://tools.ietf.org/html/rfc7232#section-6
         def fresh?(response)
-          last_modified = if_modified_since
-          etag          = if_none_match
+          if Request.strict_freshness
+            if if_modified_since
+              etag_matches?(response.etag)
+            elsif if_none_match
+              not_modified?(response.last_modified)
+            else
+              false
+            end
+          else
+            last_modified = if_modified_since
+            etag          = if_none_match
 
-          return false unless last_modified || etag
+            return false unless last_modified || etag
 
-          success = true
-          success &&= not_modified?(response.last_modified) if last_modified
-          success &&= etag_matches?(response.etag) if etag
-          success
+            success = true
+            success &&= not_modified?(response.last_modified) if last_modified
+            success &&= etag_matches?(response.etag) if etag
+            success
+          end
         end
       end
 

actionpack/lib/action_dispatch/railtie.rb

@@ -29,6 +29,7 @@ class Railtie < Rails::Railtie # :nodoc:
     config.action_dispatch.request_id_header = ActionDispatch::Constants::X_REQUEST_ID
     config.action_dispatch.log_rescued_responses = true
     config.action_dispatch.debug_exception_log_level = :fatal
+    config.action_dispatch.strict_freshness = false
 
     config.action_dispatch.default_headers = {
       "X-Frame-Options" => "SAMEORIGIN",
@@ -69,6 +70,7 @@ class Railtie < Rails::Railtie # :nodoc:
 
       ActionDispatch::Routing::Mapper.route_source_locations = Rails.env.development?
 
+      ActionDispatch::Http::Cache::Request.strict_freshness = app.config.action_dispatch.strict_freshness
       ActionDispatch.test_app = app
     end
   end

actionpack/test/controller/api/conditional_get_test.rb

@@ -3,6 +3,7 @@
 require "abstract_unit"
 require "active_support/core_ext/integer/time"
 require "active_support/core_ext/numeric/time"
+require "support/etag_helper"
 
 class ConditionalGetApiController < ActionController::API
   before_action :handle_last_modified_and_etags, only: :two
@@ -24,6 +25,7 @@ def handle_last_modified_and_etags
 end
 
 class ConditionalGetApiTest < ActionController::TestCase
+  include EtagHelper
   tests ConditionalGetApiController
 
   def setup
@@ -55,4 +57,54 @@ def test_request_not_modified
     assert_predicate @response.body, :blank?
     assert_equal @last_modified, @response.headers["Last-Modified"]
   end
+
+  def test_if_none_match_is_asterisk
+    @request.if_none_match = "*"
+    get :one
+    assert_response :not_modified
+  end
+
+  def test_etag_matches
+    @request.if_none_match = weak_etag([:foo, 123])
+    get :one
+    assert_response :not_modified
+  end
+
+  def test_etag_precedence_over_last_modified
+    with_strict_freshness(true) do
+      # Not modified because the etag matches
+      @request.if_modified_since = 5.years.ago.httpdate
+      @request.if_none_match = weak_etag([:foo, 123])
+
+      get :one
+      assert_response :not_modified
+
+      # stale because the etag doesn't match
+      @request.if_none_match = weak_etag([:bar, 124])
+      @request.if_modified_since = @last_modified
+
+      get :one
+      assert_response :success
+    end
+  end
+
+  def test_both_should_be_used_when_strict_freshness_is_false
+    with_strict_freshness(false) do
+      # stale because the etag match but the last modified doesn't
+      @request.if_modified_since = 5.years.ago.httpdate
+      @request.if_none_match = weak_etag([:foo, 123])
+
+      get :one
+      assert_response :ok
+    end
+  end
+
+  private
+    def with_strict_freshness(value)
+      old_value = ActionDispatch::Http::Cache::Request.strict_freshness
+      ActionDispatch::Http::Cache::Request.strict_freshness = value
+      yield
+    ensure
+      ActionDispatch::Http::Cache::Request.strict_freshness = old_value
+    end
 end

actionpack/test/controller/render_test.rb

@@ -2,6 +2,7 @@
 
 require "abstract_unit"
 require "controller/fake_models"
+require "support/etag_helper"
 
 class TestControllerWithExtraEtags < ActionController::Base
   self.view_paths = [ActionView::FixtureResolver.new(
@@ -666,6 +667,7 @@ def test_last_modified_with_custom_cache_control_headers
 class EtagRenderTest < ActionController::TestCase
   tests TestControllerWithExtraEtags
   include TemplateModificationHelper
+  include EtagHelper
 
   def test_strong_etag
     @request.if_none_match = strong_etag(["strong", "ab", :cde, [:f]])
@@ -738,15 +740,6 @@ def test_etag_reflects_implicit_template_digest
       assert_not_equal etag, @response.etag
     end
   end
-
-  private
-    def weak_etag(record)
-      "W/#{strong_etag record}"
-    end
-
-    def strong_etag(record)
-      %("#{ActiveSupport::Digest.hexdigest(ActiveSupport::Cache.expand_cache_key(record))}")
-    end
 end
 
 class NamespacedEtagRenderTest < ActionController::TestCase

actionpack/test/support/etag_helper.rb

@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+module EtagHelper
+  def weak_etag(record)
+    "W/#{strong_etag record}"
+  end
+
+  def strong_etag(record)
+    %("#{ActiveSupport::Digest.hexdigest(ActiveSupport::Cache.expand_cache_key(record))}")
+  end
+end

guides/source/caching_with_rails.md

@@ -656,6 +656,10 @@ class ProductsController < ApplicationController
 end
 ```
 
+When both `last_modified` and `etag` are set, behavior varies depending on the value of `config.action_dispatch.strict_freshness`.
+If set to `true`, only the `etag` is considered as specified by RFC 7232 section 6.
+If set to `false`, both are considered and the cache is considered fresh if both conditions are satisfied, as was the historical Rails behavior.
+
 Sometimes we want to cache response, for example a static page, that never gets
 expired. To achieve this, we can use `http_cache_forever` helper and by doing
 so browser and proxies will cache it indefinitely.

guides/source/configuring.md

@@ -60,6 +60,7 @@ Below are the default values associated with each target version. In cases of co
 
 #### Default Values for Target Version 8.0
 
+- [`config.action_dispatch.strict_freshness`](#config-action-dispatch-strict-freshness): `true`
 - [`config.active_support.to_time_preserves_timezone`](#config-active-support-to-time-preserves-timezone): `:zone`
 
 #### Default Values for Target Version 7.2
@@ -2116,6 +2117,20 @@ Setting the value to `:none` configures Action Pack raise all exceptions.
 | (original)            | `true`                |
 | 7.1                   | `:all`                |
 
+### `config.action_dispatch.strict_freshness`
+
+Configures whether the `ActionDispatch::ETag` middleware should prefer the `ETag` header over the `Last-Modified` header when both are present in the response.
+
+If set to `true`, when both headers are present only the `ETag` is considered as specificed by RFC 7232 section 6.
+
+If set to `false`, when both headers are present, both headers are checked and both need to match for the response to be considered fresh.
+
+| Starting with version | The default value is  |
+| --------------------- | --------------------- |
+| (original)            | `false`               |
+| 8.0                   | `true`                |
+
+
 #### `ActionDispatch::Callbacks.before`
 
 Takes a block of code to run before the request.

railties/lib/rails/application/configuration.rb

@@ -343,6 +343,10 @@ def load_defaults(target_version)
           if respond_to?(:active_support)
             active_support.to_time_preserves_timezone = :zone
           end
+
+          if respond_to?(:action_dispatch)
+            action_dispatch.strict_freshness = true
+          end
         else
           raise "Unknown version #{target_version.to_s.inspect}"
         end

railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_8_0.rb.tt

@@ -16,3 +16,10 @@
 # If `false`, `to_time` methods will convert to the local system UTC offset instead.
 #++
 # Rails.application.config.active_support.to_time_preserves_timezone = :zone
+
+###
+# When both `If-Modified-Since` and `If-None-Match` are provided by the client
+# only consider `If-None-Match` as specified by RFC 7232 Section 6.
+# If set to `false` both conditions need to be satisfied.
+#++
+# Rails.application.config.action_dispatch.strict_freshness = true

railties/test/application/configuration_test.rb

@@ -3358,6 +3358,25 @@ def index
       assert_equal 308, Rails.application.config.action_dispatch.ssl_default_redirect_status
     end
 
+    test "Rails.application.config.action_dispatch.strict_freshness is false by default for older applications" do
+      remove_from_config '.*config\.load_defaults.*\n'
+      app "development"
+
+      assert_equal false, Rails.application.config.action_dispatch.strict_freshness
+    end
+
+    test "Rails.application.config.action_dispatch.strict_freshness can be configured in an initializer" do
+      remove_from_config '.*config\.load_defaults.*\n'
+      add_to_config <<-RUBY
+        config.action_dispatch.strict_freshness = true
+      RUBY
+
+      app "development"
+
+      assert_equal true, ActionDispatch::Http::Cache::Request.strict_freshness
+    end
+
+
     test "Rails.application.config.action_mailer.smtp_settings have open_timeout and read_timeout defined as 5 in 7.0 defaults" do
       remove_from_config '.*config\.load_defaults.*\n'
       add_to_config <<-RUBY