Merge pull request #30367 from ptoomey3/consistent-cache-control-headers

Normalize/process Cache-Control headers consistently

3 files changed, 34 insertions, 8 deletions

diff --git a/actionpack/lib/action_dispatch/http/cache.rb b/actionpack/lib/action_dispatch/http/cache.rb
index 8073685b78..3328ce17a0 100644
--- a/actionpack/lib/action_dispatch/http/cache.rb
+++ b/actionpack/lib/action_dispatch/http/cache.rb
@@ -166,19 +166,23 @@ module ActionDispatch
           @cache_control = cache_control_headers
         end
 
-        def handle_conditional_get!
-          if etag? || last_modified? || !@cache_control.empty?
-            set_conditional_cache_control!(@cache_control)
-          end
-        end
-
         DEFAULT_CACHE_CONTROL = "max-age=0, private, must-revalidate".freeze
         NO_CACHE              = "no-cache".freeze
         PUBLIC                = "public".freeze
         PRIVATE               = "private".freeze
         MUST_REVALIDATE       = "must-revalidate".freeze
 
-        def set_conditional_cache_control!(cache_control)
+        def handle_conditional_get!
+          # Normally default cache control setting is handled by ETag
+          # middleware. But, if an etag is already set, the middleware
+          # defaults to `no-cache` unless a default `Cache-Control` value is
+          # previously set. So, set a default one here.
+          if (etag? || last_modified?) && !self._cache_control
+            self._cache_control = DEFAULT_CACHE_CONTROL
+          end
+        end
+
+        def merge_and_normalize_cache_control!(cache_control)
           control = {}
           cc_headers = cache_control_headers
           if extras = cc_headers.delete(:extras)
@@ -191,7 +195,7 @@ module ActionDispatch
           control.merge! cache_control
 
           if control.empty?
-            self._cache_control = DEFAULT_CACHE_CONTROL
+            # Let middleware handle default behavior
           elsif control[:no_cache]
             self._cache_control = NO_CACHE
             if control[:extras]
diff --git a/actionpack/lib/action_dispatch/http/response.rb b/actionpack/lib/action_dispatch/http/response.rb
index b314dbecfe..0c7b153420 100644
--- a/actionpack/lib/action_dispatch/http/response.rb
+++ b/actionpack/lib/action_dispatch/http/response.rb
@@ -433,6 +433,7 @@ module ActionDispatch # :nodoc:
     def before_committed
       return if committed?
       assign_default_content_type_and_charset!
+      merge_and_normalize_cache_control!(@cache_control)
       handle_conditional_get!
       handle_no_content!
     end
diff --git a/actionpack/test/controller/render_test.rb b/actionpack/test/controller/render_test.rb
index 1ca668b9b1..37a62edc15 100644
--- a/actionpack/test/controller/render_test.rb
+++ b/actionpack/test/controller/render_test.rb
@@ -162,6 +162,17 @@ class TestController < ActionController::Base
     render action: "hello_world"
   end
 
+  def conditional_hello_with_expires_and_confliciting_cache_control_headers
+    response.headers["Cache-Control"] = "no-cache, must-revalidate"
+    expires_now
+    render action: "hello_world"
+  end
+
+  def conditional_hello_without_expires_and_confliciting_cache_control_headers
+    response.headers["Cache-Control"] = "no-cache, must-revalidate"
+    render action: "hello_world"
+  end
+
   def conditional_hello_with_bangs
     render action: "hello_world"
   end
@@ -368,6 +379,16 @@ class ExpiresInRenderTest < ActionController::TestCase
     assert_match(/no-transform/, @response.headers["Cache-Control"])
   end
 
+  def test_expires_now_with_conflicting_cache_control_headers
+    get :conditional_hello_with_expires_and_confliciting_cache_control_headers
+    assert_equal "no-cache", @response.headers["Cache-Control"]
+  end
+
+  def test_no_expires_now_with_conflicting_cache_control_headers
+    get :conditional_hello_without_expires_and_confliciting_cache_control_headers
+    assert_equal "no-cache", @response.headers["Cache-Control"]
+  end
+
   def test_date_header_when_expires_in
     time = Time.mktime(2011, 10, 30)
     Time.stub :now, time do