Merge pull request #36213 from st0012/fix-25842

Add `Vary: Accept` header when using `Accept` header for response

5 files changed, 60 insertions, 7 deletions

diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
index ca7ae6621b..8bb2c73e52 100644
--- a/actionpack/CHANGELOG.md
+++ b/actionpack/CHANGELOG.md
@@ -1,3 +1,18 @@
+*   Add `Vary: Accept` header when using `Accept` header for response
+
+    For some requests like `/users/1`, Rails uses requests' `Accept`
+    header to determine what to return. And if we don't add `Vary`
+    in the response header, browsers might accidentally cache different
+    types of content, which would cause issues: e.g. javascript got displayed
+    instead of html content. This PR fixes these issues by adding `Vary: Accept`
+    in these types of requests. For more detailed problem description, please read:
+
+    https://github.com/rails/rails/pull/36213
+
+    Fixes #25842
+
+    *Stan Lo*
+
 *   Fix IntegrationTest `follow_redirect!` to follow redirection using the same HTTP verb when following
     a 307 redirection.
 
diff --git a/actionpack/lib/abstract_controller/rendering.rb b/actionpack/lib/abstract_controller/rendering.rb
index 8ba2b25552..280af50a44 100644
--- a/actionpack/lib/abstract_controller/rendering.rb
+++ b/actionpack/lib/abstract_controller/rendering.rb
@@ -28,6 +28,7 @@ module AbstractController
       else
         _set_rendered_content_type rendered_format
       end
+      _set_vary_header
       self.response_body = rendered_body
     end
 
@@ -109,6 +110,9 @@ module AbstractController
     def _set_html_content_type # :nodoc:
     end
 
+    def _set_vary_header # :nodoc:
+    end
+
     def _set_rendered_content_type(format) # :nodoc:
     end
 
diff --git a/actionpack/lib/action_controller/metal/rendering.rb b/actionpack/lib/action_controller/metal/rendering.rb
index efa5de313c..fd22c4fa64 100644
--- a/actionpack/lib/action_controller/metal/rendering.rb
+++ b/actionpack/lib/action_controller/metal/rendering.rb
@@ -77,6 +77,10 @@ module ActionController
         end
       end
 
+      def _set_vary_header
+        self.headers["Vary"] = "Accept" if request.should_apply_vary_header?
+      end
+
       # Normalize arguments by catching blocks and setting them on :update.
       def _normalize_args(action = nil, options = {}, &blk)
         options = super
diff --git a/actionpack/lib/action_dispatch/http/mime_negotiation.rb b/actionpack/lib/action_dispatch/http/mime_negotiation.rb
index a2cac49082..ac0ff133eb 100644
--- a/actionpack/lib/action_dispatch/http/mime_negotiation.rb
+++ b/actionpack/lib/action_dispatch/http/mime_negotiation.rb
@@ -62,13 +62,7 @@ module ActionDispatch
 
       def formats
         fetch_header("action_dispatch.request.formats") do |k|
-          params_readable = begin
-                              parameters[:format]
-                            rescue *RESCUABLE_MIME_FORMAT_ERRORS
-                              false
-                            end
-
-          v = if params_readable
+          v = if params_readable?
             Array(Mime[parameters[:format]])
           elsif use_accept_header && valid_accept_header
             accepts
@@ -153,9 +147,19 @@ module ActionDispatch
         order.include?(Mime::ALL) ? format : nil
       end
 
+      def should_apply_vary_header?
+        !params_readable? && use_accept_header && valid_accept_header
+      end
+
       private
         BROWSER_LIKE_ACCEPTS = /,\s*\*\/\*|\*\/\*\s*,/
 
+        def params_readable? # :doc:
+          parameters[:format]
+        rescue *RESCUABLE_MIME_FORMAT_ERRORS
+          false
+        end
+
         def valid_accept_header # :doc:
           (xhr? && (accept.present? || content_mime_type)) ||
             (accept.present? && accept !~ BROWSER_LIKE_ACCEPTS)
diff --git a/actionpack/test/controller/integration_test.rb b/actionpack/test/controller/integration_test.rb
index caacd66bd6..cb7c2467ac 100644
--- a/actionpack/test/controller/integration_test.rb
+++ b/actionpack/test/controller/integration_test.rb
@@ -543,6 +543,32 @@ class IntegrationProcessTest < ActionDispatch::IntegrationTest
     end
   end
 
+  def test_setting_vary_header_when_request_is_xhr_with_accept_header
+    with_test_route_set do
+      get "/get", headers: { "Accept" => "application/json" }, xhr: true
+      assert_equal "Accept", response.headers["Vary"]
+    end
+  end
+
+  def test_not_setting_vary_header_when_format_is_provided
+    with_test_route_set do
+      get "/get", params: { format: "json" }
+      assert_nil response.headers["Vary"]
+    end
+  end
+
+  def test_not_setting_vary_header_when_ignore_accept_header_is_set
+    original_ignore_accept_header = ActionDispatch::Request.ignore_accept_header
+    ActionDispatch::Request.ignore_accept_header = true
+
+    with_test_route_set do
+      get "/get", headers: { "Accept" => "application/json" }, xhr: true
+      assert_nil response.headers["Vary"]
+    end
+  ensure
+    ActionDispatch::Request.ignore_accept_header = original_ignore_accept_header
+  end
+
   private
     def with_default_headers(headers)
       original = ActionDispatch::Response.default_headers