diff options
Diffstat (limited to 'actionpack')
13 files changed, 196 insertions, 69 deletions
diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index 9a6bd4bb45..8bb2c73e52 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,3 +1,27 @@ +* 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. + + *Edouard Chin* + +* System tests require Capybara 3.26 or newer. + + *George Claghorn* + * Reduced log noise handling ActionController::RoutingErrors. *Alberto Fernández-Capel* 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_controller/metal/strong_parameters.rb b/actionpack/lib/action_controller/metal/strong_parameters.rb index 6fbd52dd51..920ae52f2b 100644 --- a/actionpack/lib/action_controller/metal/strong_parameters.rb +++ b/actionpack/lib/action_controller/metal/strong_parameters.rb @@ -749,6 +749,18 @@ module ActionController end alias_method :delete_if, :reject! + # Returns a new instance of <tt>ActionController::Parameters</tt> without the blank values. + # Uses Object#blank? for determining if a value is blank. + def compact_blank + reject { |_k, v| v.blank? } + end + + # Removes all blank values in place and returns self. + # Uses Object#blank? for determining if a value is blank. + def compact_blank! + reject! { |_k, v| v.blank? } + end + # Returns values that were assigned to the given +keys+. Note that all the # +Hash+ objects will be converted to <tt>ActionController::Parameters</tt>. def values_at(*keys) 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/lib/action_dispatch/middleware/cookies.rb b/actionpack/lib/action_dispatch/middleware/cookies.rb index 642f155085..9b5a5cf2b0 100644 --- a/actionpack/lib/action_dispatch/middleware/cookies.rb +++ b/actionpack/lib/action_dispatch/middleware/cookies.rb @@ -346,28 +346,6 @@ module ActionDispatch @cookies.map { |k, v| "#{escape(k)}=#{escape(v)}" }.join "; " end - def handle_options(options) # :nodoc: - if options[:expires].respond_to?(:from_now) - options[:expires] = options[:expires].from_now - end - - options[:path] ||= "/" - - if options[:domain] == :all || options[:domain] == "all" - # If there is a provided tld length then we use it otherwise default domain regexp. - domain_regexp = options[:tld_length] ? /([^.]+\.?){#{options[:tld_length]}}$/ : DOMAIN_REGEXP - - # If host is not ip and matches domain regexp. - # (ip confirms to domain regexp so we explicitly check for ip) - options[:domain] = if (request.host !~ /^[\d.]+$/) && (request.host =~ domain_regexp) - ".#{$&}" - end - elsif options[:domain].is_a? Array - # If host matches one of the supplied domains without a dot in front of it. - options[:domain] = options[:domain].find { |domain| request.host.include? domain.sub(/^\./, "") } - end - end - # Sets the cookie named +name+. The second argument may be the cookie's # value or a hash of options as documented above. def []=(name, options) @@ -447,6 +425,28 @@ module ActionDispatch def write_cookie?(cookie) request.ssl? || !cookie[:secure] || always_write_cookie end + + def handle_options(options) + if options[:expires].respond_to?(:from_now) + options[:expires] = options[:expires].from_now + end + + options[:path] ||= "/" + + if options[:domain] == :all || options[:domain] == "all" + # If there is a provided tld length then we use it otherwise default domain regexp. + domain_regexp = options[:tld_length] ? /([^.]+\.?){#{options[:tld_length]}}$/ : DOMAIN_REGEXP + + # If host is not ip and matches domain regexp. + # (ip confirms to domain regexp so we explicitly check for ip) + options[:domain] = if (request.host !~ /^[\d.]+$/) && (request.host =~ domain_regexp) + ".#{$&}" + end + elsif options[:domain].is_a? Array + # If host matches one of the supplied domains without a dot in front of it. + options[:domain] = options[:domain].find { |domain| request.host.include? domain.sub(/^\./, "") } + end + end end class AbstractCookieJar # :nodoc: diff --git a/actionpack/lib/action_dispatch/system_test_case.rb b/actionpack/lib/action_dispatch/system_test_case.rb index 4fda2cf44f..aae96975c7 100644 --- a/actionpack/lib/action_dispatch/system_test_case.rb +++ b/actionpack/lib/action_dispatch/system_test_case.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -gem "capybara", ">= 2.15" +gem "capybara", ">= 3.26" require "capybara/dsl" require "capybara/minitest" @@ -119,17 +119,6 @@ module ActionDispatch def initialize(*) # :nodoc: super self.class.driver.use - @proxy_route = if ActionDispatch.test_app - Class.new do - include ActionDispatch.test_app.routes.url_helpers - - def url_options - default_url_options.merge(host: Capybara.app_host) - end - end.new - else - nil - end end def self.start_application # :nodoc: @@ -170,16 +159,33 @@ module ActionDispatch driven_by :selenium - def method_missing(method, *args, &block) - if @proxy_route.respond_to?(method) - @proxy_route.send(method, *args, &block) - else - super + private + def url_helpers + @url_helpers ||= + if ActionDispatch.test_app + Class.new do + include ActionDispatch.test_app.routes.url_helpers + + def url_options + default_url_options.reverse_merge(host: Capybara.app_host || Capybara.current_session.server_url) + end + end.new + end end - end - ActiveSupport.run_load_hooks(:action_dispatch_system_test_case, self) - end + def method_missing(name, *args, &block) + if url_helpers.respond_to?(name) + url_helpers.public_send(name, *args, &block) + else + super + end + end - SystemTestCase.start_application + def respond_to_missing?(name, include_private = false) + url_helpers.respond_to?(name) + end + end end + +ActiveSupport.run_load_hooks :action_dispatch_system_test_case, ActionDispatch::SystemTestCase +ActionDispatch::SystemTestCase.start_application diff --git a/actionpack/lib/action_dispatch/system_testing/test_helpers/setup_and_teardown.rb b/actionpack/lib/action_dispatch/system_testing/test_helpers/setup_and_teardown.rb index 20f6a7634f..30dc21ebb9 100644 --- a/actionpack/lib/action_dispatch/system_testing/test_helpers/setup_and_teardown.rb +++ b/actionpack/lib/action_dispatch/system_testing/test_helpers/setup_and_teardown.rb @@ -4,15 +4,12 @@ module ActionDispatch module SystemTesting module TestHelpers module SetupAndTeardown # :nodoc: - DEFAULT_HOST = "http://127.0.0.1" - def host!(host) - Capybara.app_host = host - end + ActiveSupport::Deprecation.warn \ + "ActionDispatch::SystemTestCase#host! is deprecated with no replacement. " \ + "Set Capybara.app_host directly or rely on Capybara's default host." - def before_setup - host! DEFAULT_HOST - super + Capybara.app_host = host end def before_teardown diff --git a/actionpack/lib/action_dispatch/testing/integration.rb b/actionpack/lib/action_dispatch/testing/integration.rb index c5f8b816a4..9e7b4301a8 100644 --- a/actionpack/lib/action_dispatch/testing/integration.rb +++ b/actionpack/lib/action_dispatch/testing/integration.rb @@ -49,11 +49,16 @@ module ActionDispatch # Follow a single redirect response. If the last response was not a # redirect, an exception will be raised. Otherwise, the redirect is - # performed on the location header. Any arguments are passed to the - # underlying call to `get`. + # performed on the location header. If the redirection is a 307 redirect, + # the same HTTP verb will be used when redirecting, otherwise a GET request + # will be performed. Any arguments are passed to the + # underlying request. def follow_redirect!(**args) raise "not a redirect! #{status} #{status_message}" unless redirect? - get(response.location, **args) + + method = response.status == 307 ? request.method.downcase : :get + public_send(method, response.location, **args) + status end end diff --git a/actionpack/test/controller/integration_test.rb b/actionpack/test/controller/integration_test.rb index cce229b30d..cb7c2467ac 100644 --- a/actionpack/test/controller/integration_test.rb +++ b/actionpack/test/controller/integration_test.rb @@ -213,6 +213,10 @@ class IntegrationProcessTest < ActionDispatch::IntegrationTest redirect_to action_url("get") end + def redirect_307 + redirect_to action_url("post"), status: 307 + end + def remove_header response.headers.delete params[:header] head :ok, "c" => "3" @@ -337,6 +341,15 @@ class IntegrationProcessTest < ActionDispatch::IntegrationTest end end + def test_307_redirect_uses_the_same_http_verb + with_test_route_set do + post "/redirect_307" + assert_equal 307, status + follow_redirect! + assert_equal "POST", request.method + end + end + def test_redirect_reset_html_document with_test_route_set do get "/redirect" @@ -530,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/actionpack/test/controller/parameters/mutators_test.rb b/actionpack/test/controller/parameters/mutators_test.rb index 31ee7964f5..ffffd2996f 100644 --- a/actionpack/test/controller/parameters/mutators_test.rb +++ b/actionpack/test/controller/parameters/mutators_test.rb @@ -127,4 +127,22 @@ class ParametersMutatorsTest < ActiveSupport::TestCase test "deep_transform_keys! retains unpermitted status" do assert_not_predicate @params.deep_transform_keys! { |k| k }, :permitted? end + + test "compact_blank retains permitted status" do + @params.permit! + assert_predicate @params.compact_blank, :permitted? + end + + test "compact_blank retains unpermitted status" do + assert_not_predicate @params.compact_blank, :permitted? + end + + test "compact_blank! retains permitted status" do + @params.permit! + assert_predicate @params.compact_blank!, :permitted? + end + + test "compact_blank! retains unpermitted status" do + assert_not_predicate @params.compact_blank!, :permitted? + end end diff --git a/actionpack/test/controller/render_test.rb b/actionpack/test/controller/render_test.rb index a2a6c69dd3..dd76824413 100644 --- a/actionpack/test/controller/render_test.rb +++ b/actionpack/test/controller/render_test.rb @@ -4,6 +4,11 @@ require "abstract_unit" require "controller/fake_models" class TestControllerWithExtraEtags < ActionController::Base + self.view_paths = [ActionView::FixtureResolver.new( + "test/with_implicit_template.erb" => "Hello explicitly!", + "test/hello_world.erb" => "Hello world!" + )] + def self.controller_name; "test"; end def self.controller_path; "test"; end @@ -37,6 +42,11 @@ class TestControllerWithExtraEtags < ActionController::Base end class ImplicitRenderTestController < ActionController::Base + self.view_paths = [ActionView::FixtureResolver.new( + "implicit_render_test/hello_world.erb" => "Hello world!", + "implicit_render_test/empty_action_with_template.html.erb" => "<h1>Empty action rendered this implicitly.</h1>\n" + )] + def empty_action end @@ -46,6 +56,10 @@ end module Namespaced class ImplicitRenderTestController < ActionController::Base + self.view_paths = [ActionView::FixtureResolver.new( + "namespaced/implicit_render_test/hello_world.erb" => "Hello world!" + )] + def hello_world fresh_when(etag: "abc") end @@ -293,13 +307,15 @@ end module TemplateModificationHelper private def modify_template(name) - path = File.expand_path("../fixtures/#{name}.erb", __dir__) - original = File.read(path) - File.write(path, "#{original} Modified!") + hash = @controller.view_paths.first.instance_variable_get(:@hash) + key = name + ".erb" + original = hash[key] + hash[key] = "#{original} Modified!" ActionView::LookupContext::DetailsKey.clear yield ensure - File.write(path, original) + hash[key] = original + ActionView::LookupContext::DetailsKey.clear end end diff --git a/actionpack/test/dispatch/system_testing/system_test_case_test.rb b/actionpack/test/dispatch/system_testing/system_test_case_test.rb index 3319db1665..d235f7ad89 100644 --- a/actionpack/test/dispatch/system_testing/system_test_case_test.rb +++ b/actionpack/test/dispatch/system_testing/system_test_case_test.rb @@ -36,12 +36,10 @@ class SetDriverToSeleniumHeadlessFirefoxTest < DrivenBySeleniumWithHeadlessFiref end class SetHostTest < DrivenByRackTest - test "sets default host" do - assert_equal "http://127.0.0.1", Capybara.app_host - end - test "overrides host" do - host! "http://example.com" + assert_deprecated do + host! "http://example.com" + end assert_equal "http://example.com", Capybara.app_host end |