aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorRafael França <rafael@franca.dev>2019-07-26 13:43:10 -0400
committerGitHub <noreply@github.com>2019-07-26 13:43:10 -0400
commitf45e23ead96507c2bb582c223f2615ca71244312 (patch)
tree78e56f72c4477cd829f7e1f190e602649bd6faef
parenta40da823d729296514f310d13a09ce8e0b37b699 (diff)
parent5745a3c0928ee5604ce80af19348efb42189f1d6 (diff)
downloadrails-f45e23ead96507c2bb582c223f2615ca71244312.tar.gz
rails-f45e23ead96507c2bb582c223f2615ca71244312.tar.bz2
rails-f45e23ead96507c2bb582c223f2615ca71244312.zip
Merge pull request #36213 from st0012/fix-25842
Add `Vary: Accept` header when using `Accept` header for response
-rw-r--r--actionpack/CHANGELOG.md15
-rw-r--r--actionpack/lib/abstract_controller/rendering.rb4
-rw-r--r--actionpack/lib/action_controller/metal/rendering.rb4
-rw-r--r--actionpack/lib/action_dispatch/http/mime_negotiation.rb18
-rw-r--r--actionpack/test/controller/integration_test.rb26
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