diff options
Diffstat (limited to 'actionpack')
| -rw-r--r-- | actionpack/CHANGELOG.md | 30 | ||||
| -rw-r--r-- | actionpack/actionpack.gemspec | 3 | ||||
| -rw-r--r-- | actionpack/lib/action_controller/metal/params_wrapper.rb | 5 | ||||
| -rw-r--r-- | actionpack/lib/action_dispatch/http/filter_parameters.rb | 2 | ||||
| -rw-r--r-- | actionpack/lib/action_dispatch/http/mime_negotiation.rb | 7 | ||||
| -rw-r--r-- | actionpack/lib/action_dispatch/http/parameters.rb | 16 | ||||
| -rw-r--r-- | actionpack/lib/action_dispatch/http/request.rb | 3 | ||||
| -rw-r--r-- | actionpack/lib/action_dispatch/journey/router.rb | 3 | ||||
| -rw-r--r-- | actionpack/lib/action_dispatch/routing/mapper.rb | 16 | ||||
| -rw-r--r-- | actionpack/lib/action_dispatch/system_testing/test_helpers/setup_and_teardown.rb | 7 | ||||
| -rw-r--r-- | actionpack/test/controller/params_parse_test.rb | 34 | ||||
| -rw-r--r-- | actionpack/test/controller/rescue_test.rb | 26 | ||||
| -rw-r--r-- | actionpack/test/controller/resources_test.rb | 22 | ||||
| -rw-r--r-- | actionpack/test/dispatch/routing_test.rb | 16 |
14 files changed, 165 insertions, 25 deletions
diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index 5554d4e6b8..740c6db06f 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,11 +1,35 @@ +* Raise an error on root route naming conflicts. + + Raises an ArgumentError when multiple root routes are defined in the + same context instead of assigning nil names to subsequent roots. + + *Gannon McGibbon* + +* Allow rescue from parameter parse errors: + + ``` + rescue_from ActionDispatch::Http::Parameters::ParseError do + head :unauthorized + end + ``` + + *Gannon McGibbon*, *Josh Cheek* + +* Reset Capybara sessions if failed system test screenshot raising an exception. + + Reset Capybara sessions if `take_failed_screenshot` raise exception + in system test `after_teardown`. + + *Maxim Perepelitsa* + * Use request object for context if there's no controller There is no controller instance when using a redirect route or a mounted rack application so pass the request object as the context when resolving dynamic CSP sources in this scenario. - + Fixes #34200. - + *Andrew White* * Apply mapping to symbols returned from dynamic CSP sources @@ -14,7 +38,7 @@ would be converted to a string implicity, e.g: policy.default_src -> { :self } - + would generate the header: Content-Security-Policy: default-src self diff --git a/actionpack/actionpack.gemspec b/actionpack/actionpack.gemspec index 1dc8abf746..ec56de18f1 100644 --- a/actionpack/actionpack.gemspec +++ b/actionpack/actionpack.gemspec @@ -26,6 +26,9 @@ Gem::Specification.new do |s| "changelog_uri" => "https://github.com/rails/rails/blob/v#{version}/actionpack/CHANGELOG.md" } + # NOTE: Please read our dependency guidelines before updating versions: + # https://edgeguides.rubyonrails.org/security.html#dependency-management-and-cves + s.add_dependency "activesupport", version s.add_dependency "rack", "~> 2.0" diff --git a/actionpack/lib/action_controller/metal/params_wrapper.rb b/actionpack/lib/action_controller/metal/params_wrapper.rb index a678377d4f..7361946de5 100644 --- a/actionpack/lib/action_controller/metal/params_wrapper.rb +++ b/actionpack/lib/action_controller/metal/params_wrapper.rb @@ -253,7 +253,10 @@ module ActionController # This will display the wrapped hash in the log file. request.filtered_parameters.merge! wrapped_filtered_hash end - super + ensure + # NOTE: Rescues all exceptions so they + # may be caught in ActionController::Rescue. + return super end private diff --git a/actionpack/lib/action_dispatch/http/filter_parameters.rb b/actionpack/lib/action_dispatch/http/filter_parameters.rb index 9f8f178402..cbb772175c 100644 --- a/actionpack/lib/action_dispatch/http/filter_parameters.rb +++ b/actionpack/lib/action_dispatch/http/filter_parameters.rb @@ -41,6 +41,8 @@ module ActionDispatch # Returns a hash of parameters with all sensitive data replaced. def filtered_parameters @filtered_parameters ||= parameter_filter.filter(parameters) + rescue ActionDispatch::Http::Parameters::ParseError + @filtered_parameters = {} end # Returns a hash of request.env with all sensitive data replaced. diff --git a/actionpack/lib/action_dispatch/http/mime_negotiation.rb b/actionpack/lib/action_dispatch/http/mime_negotiation.rb index be129965d1..498b1e6695 100644 --- a/actionpack/lib/action_dispatch/http/mime_negotiation.rb +++ b/actionpack/lib/action_dispatch/http/mime_negotiation.rb @@ -7,6 +7,11 @@ module ActionDispatch module MimeNegotiation extend ActiveSupport::Concern + RESCUABLE_MIME_FORMAT_ERRORS = [ + ActionController::BadRequest, + ActionDispatch::Http::Parameters::ParseError, + ] + included do mattr_accessor :ignore_accept_header, default: false end @@ -59,7 +64,7 @@ module ActionDispatch fetch_header("action_dispatch.request.formats") do |k| params_readable = begin parameters[:format] - rescue ActionController::BadRequest + rescue *RESCUABLE_MIME_FORMAT_ERRORS false end diff --git a/actionpack/lib/action_dispatch/http/parameters.rb b/actionpack/lib/action_dispatch/http/parameters.rb index 8d7431fd6b..13d0963a33 100644 --- a/actionpack/lib/action_dispatch/http/parameters.rb +++ b/actionpack/lib/action_dispatch/http/parameters.rb @@ -111,13 +111,23 @@ module ActionDispatch begin strategy.call(raw_post) rescue # JSON or Ruby code block errors. - my_logger = logger || ActiveSupport::Logger.new($stderr) - my_logger.debug "Error occurred while parsing request parameters.\nContents:\n\n#{raw_post}" - + log_parse_error_once raise ParseError end end + def log_parse_error_once + @parse_error_logged ||= begin + parse_logger = logger || ActiveSupport::Logger.new($stderr) + parse_logger.debug <<~MSG.chomp + Error occurred while parsing request parameters. + Contents: + + #{raw_post} + MSG + end + end + def params_parsers ActionDispatch::Request.parameter_parsers end diff --git a/actionpack/lib/action_dispatch/http/request.rb b/actionpack/lib/action_dispatch/http/request.rb index 7bc364d370..44f23940d3 100644 --- a/actionpack/lib/action_dispatch/http/request.rb +++ b/actionpack/lib/action_dispatch/http/request.rb @@ -383,9 +383,6 @@ module ActionDispatch end self.request_parameters = Request::Utils.normalize_encode_params(pr) end - rescue Http::Parameters::ParseError # one of the parse strategies blew up - self.request_parameters = Request::Utils.normalize_encode_params(super || {}) - raise rescue Rack::Utils::ParameterTypeError, Rack::Utils::InvalidParameterError => e raise ActionController::BadRequest.new("Invalid request parameters: #{e.message}") end diff --git a/actionpack/lib/action_dispatch/journey/router.rb b/actionpack/lib/action_dispatch/journey/router.rb index 30af3ff930..89a164f968 100644 --- a/actionpack/lib/action_dispatch/journey/router.rb +++ b/actionpack/lib/action_dispatch/journey/router.rb @@ -15,9 +15,6 @@ require "action_dispatch/journey/path/pattern" module ActionDispatch module Journey # :nodoc: class Router # :nodoc: - class RoutingError < ::StandardError # :nodoc: - end - attr_accessor :routes def initialize(routes) diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb index 99f3b4c2cd..421e2023c2 100644 --- a/actionpack/lib/action_dispatch/routing/mapper.rb +++ b/actionpack/lib/action_dispatch/routing/mapper.rb @@ -656,7 +656,7 @@ module ActionDispatch # Query if the following named route was already defined. def has_named_route?(name) - @set.named_routes.key? name + @set.named_routes.key?(name) end private @@ -1171,10 +1171,16 @@ module ActionDispatch end def actions + if @except + available_actions - Array(@except).map(&:to_sym) + else + available_actions + end + end + + def available_actions if @only Array(@only).map(&:to_sym) - elsif @except - default_actions - Array(@except).map(&:to_sym) else default_actions end @@ -1946,9 +1952,7 @@ module ActionDispatch end def match_root_route(options) - name = has_named_route?(name_for_action(:root, nil)) ? nil : :root - args = ["/", { as: name, via: :get }.merge!(options)] - + args = ["/", { as: :root, via: :get }.merge(options)] match(*args) end end 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 e47d5020f4..600e9c733b 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 @@ -17,8 +17,11 @@ module ActionDispatch end def after_teardown - take_failed_screenshot - Capybara.reset_sessions! + begin + take_failed_screenshot + ensure + Capybara.reset_sessions! + end ensure super end diff --git a/actionpack/test/controller/params_parse_test.rb b/actionpack/test/controller/params_parse_test.rb new file mode 100644 index 0000000000..440ab06fd7 --- /dev/null +++ b/actionpack/test/controller/params_parse_test.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +require "abstract_unit" + +class ParamsParseTest < ActionController::TestCase + class UsersController < ActionController::Base + def create + head :ok + end + end + + tests UsersController + + def test_parse_error_logged_once + log_output = capture_log_output do + post :create, body: "{", as: :json + end + assert_equal <<~LOG, log_output + Error occurred while parsing request parameters. + Contents: + + { + LOG + end + + private + + def capture_log_output + output = StringIO.new + request.set_header "action_dispatch.logger", ActiveSupport::Logger.new(output) + yield + output.string + end +end diff --git a/actionpack/test/controller/rescue_test.rb b/actionpack/test/controller/rescue_test.rb index 4ed79073e5..3c39373e55 100644 --- a/actionpack/test/controller/rescue_test.rb +++ b/actionpack/test/controller/rescue_test.rb @@ -70,6 +70,10 @@ class RescueController < ActionController::Base render plain: "io error" end + rescue_from ActionDispatch::Http::Parameters::ParseError do + render plain: "parse error", status: :bad_request + end + before_action(only: :before_action_raises) { raise "umm nice" } def before_action_raises @@ -130,6 +134,11 @@ class RescueController < ActionController::Base raise ResourceUnavailableToRescueAsString end + def arbitrary_action + params + render plain: "arbitrary action" + end + def missing_template end @@ -306,6 +315,23 @@ class RescueControllerTest < ActionController::TestCase get :exception_with_no_handler_for_wrapper assert_response :unprocessable_entity end + + test "can rescue a ParseError" do + capture_log_output do + post :arbitrary_action, body: "{", as: :json + end + assert_response :bad_request + assert_equal "parse error", response.body + end + + private + + def capture_log_output + output = StringIO.new + request.set_header "action_dispatch.logger", ActiveSupport::Logger.new(output) + yield + output.string + end end class RescueTest < ActionDispatch::IntegrationTest diff --git a/actionpack/test/controller/resources_test.rb b/actionpack/test/controller/resources_test.rb index d336b96eff..d2146f12a5 100644 --- a/actionpack/test/controller/resources_test.rb +++ b/actionpack/test/controller/resources_test.rb @@ -853,6 +853,28 @@ class ResourcesTest < ActionController::TestCase end end + def test_resource_has_show_action_but_does_not_have_destroy_action + with_routing do |set| + set.draw do + resources :products, only: [:show, :destroy], except: :destroy + end + + assert_resource_allowed_routes("products", {}, { id: "1" }, :show, [:index, :new, :create, :edit, :update, :destroy]) + assert_resource_allowed_routes("products", { format: "xml" }, { id: "1" }, :show, [:index, :new, :create, :edit, :update, :destroy]) + end + end + + def test_singleton_resource_has_show_action_but_does_not_have_destroy_action + with_routing do |set| + set.draw do + resource :account, only: [:show, :destroy], except: :destroy + end + + assert_singleton_resource_allowed_routes("accounts", {}, :show, [:new, :create, :edit, :update, :destroy]) + assert_singleton_resource_allowed_routes("accounts", { format: "xml" }, :show, [:new, :create, :edit, :update, :destroy]) + end + end + def test_resource_has_only_create_action_and_named_route with_routing do |set| set.draw do diff --git a/actionpack/test/dispatch/routing_test.rb b/actionpack/test/dispatch/routing_test.rb index affc2d8497..4dffbd0db1 100644 --- a/actionpack/test/dispatch/routing_test.rb +++ b/actionpack/test/dispatch/routing_test.rb @@ -3698,15 +3698,25 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest end end - def test_multiple_roots + def test_multiple_roots_raises_error + ex = assert_raises(ArgumentError) { + draw do + root "pages#index", constraints: { host: "www.example.com" } + root "admin/pages#index", constraints: { host: "admin.example.com" } + end + } + assert_match(/Invalid route name, already in use: 'root'/, ex.message) + end + + def test_multiple_named_roots draw do namespace :foo do root "pages#index", constraints: { host: "www.example.com" } - root "admin/pages#index", constraints: { host: "admin.example.com" } + root "admin/pages#index", constraints: { host: "admin.example.com" }, as: :admin_root end root "pages#index", constraints: { host: "www.example.com" } - root "admin/pages#index", constraints: { host: "admin.example.com" } + root "admin/pages#index", constraints: { host: "admin.example.com" }, as: :admin_root end get "http://www.example.com/foo" |