Add `Vary: Accept` header when rendering

Problem description (quoted from @rafaelfranca's excellent explanation in https://github.com/rails/jquery-ujs/issues/318#issuecomment-88129005):

> Let say that we requested /tasks/1 using Ajax, and the previous page has the same url. When we click the back button the browser tries to get the response from its cache and it gets the javascript response. With vary we "fix" this behavior because we are telling the browser that the url is the same but it is not from the same type what will skip the cache.

And there's a Rails issue discussing about this problem as well https://github.com/rails/rails/issues/25842

Also, according to [RFC 7231 7.1.4](https://tools.ietf.org/html/rfc7231#section-7.1.4)

>  An origin server SHOULD send a Vary header field when its algorithm
>  for selecting a representation varies based on aspects of the request
>  message other than the method and request target

we should add `Vary: Accept` header when determining content based on the `Accept` header.

Although adding such header by default could cause unnecessary cache invalidation. But this PR only adds the header if:
- The format param is not provided
- The request is a `xhr` request
- The request has accept headers and the headers are valid

So if the user
- sends request with explicit format, like `/users/1.json`
- or sends a normal request (non xhr)
- or doesn't specify accept headers

then the header won't be added.

See the discussion in https://github.com/rails/rails/issues/25842 and
https://github.com/rails/rails/pull/36213 for more details.

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