diff options
9 files changed, 89 insertions, 10 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 diff --git a/actionview/app/assets/javascripts/rails-ujs/utils/form.coffee b/actionview/app/assets/javascripts/rails-ujs/utils/form.coffee index 736cab08db..9e11bfa7ed 100644 --- a/actionview/app/assets/javascripts/rails-ujs/utils/form.coffee +++ b/actionview/app/assets/javascripts/rails-ujs/utils/form.coffee @@ -11,6 +11,7 @@ Rails.serializeElement = (element, additionalParam) -> inputs.forEach (input) -> return if !input.name || input.disabled + return if input.closest('fieldset[disabled]') if matches(input, 'select') toArray(input.options).forEach (option) -> params.push(name: input.name, value: option.value) if option.selected diff --git a/actionview/test/ujs/public/test/data-remote.js b/actionview/test/ujs/public/test/data-remote.js index 16ea114f3b..dbbb383995 100644 --- a/actionview/test/ujs/public/test/data-remote.js +++ b/actionview/test/ujs/public/test/data-remote.js @@ -490,4 +490,22 @@ asyncTest('changing a select option without "data-url" attribute still fires aja setTimeout(function() { start() }, 20) }) +asyncTest('inputs inside disabled fieldset are not submited on remote forms', 3, function() { + $('form') + .append('<fieldset>\ + <input name="description" value="A wise man" />\ + </fieldset>') + .append('<fieldset disabled="disabled">\ + <input name="age" />\ + </fieldset>') + .bindNative('ajax:success', function(e, data, status, xhr) { + equal(data.params.user_name, 'john') + equal(data.params.description, 'A wise man') + equal(data.params.age, undefined) + + start() + }) + .triggerNative('submit') +}) + })() diff --git a/activerecord/lib/active_record/associations/builder/has_and_belongs_to_many.rb b/activerecord/lib/active_record/associations/builder/has_and_belongs_to_many.rb index 6ad4c75fb5..2072f93194 100644 --- a/activerecord/lib/active_record/associations/builder/has_and_belongs_to_many.rb +++ b/activerecord/lib/active_record/associations/builder/has_and_belongs_to_many.rb @@ -62,7 +62,7 @@ module ActiveRecord::Associations::Builder # :nodoc: def middle_reflection(join_model) middle_name = [lhs_model.name.downcase.pluralize, - association_name].join("_").gsub("::", "_").to_sym + association_name.to_s].sort.join("_").gsub("::", "_").to_sym middle_options = middle_options join_model HasMany.create_reflection(lhs_model, diff --git a/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb b/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb index 25cfa0a723..de9742b250 100644 --- a/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb @@ -700,10 +700,17 @@ class HasAndBelongsToManyAssociationsTest < ActiveRecord::TestCase assert_equal ["id"], developers(:david).projects.select(:id).first.attributes.keys end + def test_join_middle_table_alias + assert_equal( + 2, + Project.includes(:developers_projects).where.not("developers_projects.joined_on": nil).to_a.size + ) + end + def test_join_table_alias assert_equal( 3, - Developer.includes(projects: :developers).where.not("projects_developers_projects_join.joined_on": nil).to_a.size + Developer.includes(projects: :developers).where.not("developers_projects_projects_join.joined_on": nil).to_a.size ) end @@ -716,7 +723,7 @@ class HasAndBelongsToManyAssociationsTest < ActiveRecord::TestCase assert_equal( 3, - Developer.includes(projects: :developers).where.not("projects_developers_projects_join.joined_on": nil).group(group.join(",")).to_a.size + Developer.includes(projects: :developers).where.not("developers_projects_projects_join.joined_on": nil).group(group.join(",")).to_a.size ) end |