From 0c8dd2cab6793973384f7320c2cb2b832ec38aff Mon Sep 17 00:00:00 2001 From: Michael Colavita Date: Mon, 13 Jul 2015 13:48:10 -0400 Subject: :only and :except are now chained for routing resource(s) Allow chaining the :only and :except options for routing resource(s). Previously, the following yielded routes for both show and destroy: resource :account, :only => [:show, :destroy], :except => :destroy This now yields only the show action. This chaining can be useful for passing optional :except options to code that makes use of the :only option (e.g. for a gem with its own routing methods). --- actionpack/CHANGELOG.md | 9 +++++++++ actionpack/lib/action_dispatch/routing/mapper.rb | 12 +++++++++--- actionpack/test/controller/resources_test.rb | 22 ++++++++++++++++++++++ 3 files changed, 40 insertions(+), 3 deletions(-) (limited to 'actionpack') diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index 46856ffc5d..1c0c3c4e10 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,3 +1,12 @@ +* Allow chaining the :only and :except options for routing resource(s). + + resource :account, :only => [:show, :destroy], :except => :destroy + + This now yields only the show action. This chaining can be useful for passing optional :except + options to code that makes use of the :only option. + + *Michael Colavita* + * Add ability to filter parameters based on parent keys. # matches {credit_card: {code: "xxxx"}} diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb index 7cfe4693c1..40e00b55c2 100644 --- a/actionpack/lib/action_dispatch/routing/mapper.rb +++ b/actionpack/lib/action_dispatch/routing/mapper.rb @@ -1061,16 +1061,22 @@ module ActionDispatch end end - def actions + def available_actions if only = @options[:only] Array(only).map(&:to_sym) - elsif except = @options[:except] - default_actions - Array(except).map(&:to_sym) else default_actions end end + def actions + if except = @options[:except] + available_actions - Array(except).map(&:to_sym) + else + available_actions + end + end + def name @as || @name end diff --git a/actionpack/test/controller/resources_test.rb b/actionpack/test/controller/resources_test.rb index 5a279639cc..603d7848f9 100644 --- a/actionpack/test/controller/resources_test.rb +++ b/actionpack/test/controller/resources_test.rb @@ -838,6 +838,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 -- cgit v1.2.3 From 6fe45f378ab7bc88fecb99576fa367f83b3255f3 Mon Sep 17 00:00:00 2001 From: Bogdan Date: Mon, 15 Oct 2018 15:28:15 +0300 Subject: Fix `ActionController::Parameters#each_value` and add changelog entry to this method (#34210) * Fix `ActionController::Parameters#each_value` `each_value` should yield with "value" of the params instead of "value" as an array. Related to #33979 * Add changelog entry about `ActionController::Parameters#each_value`. Follow up #33979 --- actionpack/CHANGELOG.md | 4 ++++ actionpack/lib/action_controller/metal/strong_parameters.rb | 2 +- actionpack/test/controller/parameters/accessors_test.rb | 8 ++++++-- 3 files changed, 11 insertions(+), 3 deletions(-) (limited to 'actionpack') diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index 8205d79dd2..3858c211ea 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,3 +1,7 @@ +* Add `ActionController::Parameters#each_value`. + + *Lukáš Zapletal* + * Deprecate `ActionDispatch::Http::ParameterFilter` in favor of `ActiveSupport::ParameterFilter`. *Yoshiyuki Kinjo* diff --git a/actionpack/lib/action_controller/metal/strong_parameters.rb b/actionpack/lib/action_controller/metal/strong_parameters.rb index c1272ce667..04922b0715 100644 --- a/actionpack/lib/action_controller/metal/strong_parameters.rb +++ b/actionpack/lib/action_controller/metal/strong_parameters.rb @@ -352,7 +352,7 @@ module ActionController # the same way as Hash#each_value. def each_value(&block) @parameters.each_pair do |key, value| - yield [convert_hashes_to_parameters(key, value)] + yield convert_hashes_to_parameters(key, value) end end diff --git a/actionpack/test/controller/parameters/accessors_test.rb b/actionpack/test/controller/parameters/accessors_test.rb index 9f1fb3d042..7789e654d5 100644 --- a/actionpack/test/controller/parameters/accessors_test.rb +++ b/actionpack/test/controller/parameters/accessors_test.rb @@ -77,11 +77,15 @@ class ParametersAccessorsTest < ActiveSupport::TestCase test "each_value carries permitted status" do @params.permit! - @params["person"].each_value { |value| assert(value.permitted?) if value == 32 } + @params.each_value do |value| + assert_predicate(value, :permitted?) + end end test "each_value carries unpermitted status" do - @params["person"].each_value { |value| assert_not(value.permitted?) if value == 32 } + @params.each_value do |value| + assert_not_predicate(value, :permitted?) + end end test "each_key converts to hash for permitted" do -- cgit v1.2.3 From ed91b75c937805cb52b3930f2549b7a179cdc421 Mon Sep 17 00:00:00 2001 From: Andrew White Date: Mon, 22 Oct 2018 17:10:01 +0100 Subject: Apply mapping to symbols returned from dynamic CSP sources Previously if a dynamic source returned a symbol such as :self it would be converted to a string implicity, e.g: policy.default_src -> { :self } would generate the header: Content-Security-Policy: default-src self and now it generates: Content-Security-Policy: default-src 'self' --- actionpack/CHANGELOG.md | 17 +++++++++++++++++ .../lib/action_dispatch/http/content_security_policy.rb | 3 ++- .../test/dispatch/content_security_policy_test.rb | 4 ++-- 3 files changed, 21 insertions(+), 3 deletions(-) (limited to 'actionpack') diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index 3858c211ea..8d0477ead3 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,3 +1,20 @@ +* Apply mapping to symbols returned from dynamic CSP sources + + Previously if a dynamic source returned a symbol such as :self it + would be converted to a string implicity, e.g: + + policy.default_src -> { :self } + + would generate the header: + + Content-Security-Policy: default-src self + + and now it generates: + + Content-Security-Policy: default-src 'self' + + *Andrew White* + * Add `ActionController::Parameters#each_value`. *Lukáš Zapletal* diff --git a/actionpack/lib/action_dispatch/http/content_security_policy.rb b/actionpack/lib/action_dispatch/http/content_security_policy.rb index 50953e32b5..15b7bd1233 100644 --- a/actionpack/lib/action_dispatch/http/content_security_policy.rb +++ b/actionpack/lib/action_dispatch/http/content_security_policy.rb @@ -257,7 +257,8 @@ module ActionDispatch #:nodoc: if context.nil? raise RuntimeError, "Missing context for the dynamic content security policy source: #{source.inspect}" else - context.instance_exec(&source) + resolved = context.instance_exec(&source) + resolved.is_a?(Symbol) ? apply_mapping(resolved) : resolved end else raise RuntimeError, "Unexpected content security policy source: #{source.inspect}" diff --git a/actionpack/test/dispatch/content_security_policy_test.rb b/actionpack/test/dispatch/content_security_policy_test.rb index 13ad22b5c5..8dd4b8edb1 100644 --- a/actionpack/test/dispatch/content_security_policy_test.rb +++ b/actionpack/test/dispatch/content_security_policy_test.rb @@ -264,8 +264,8 @@ class DefaultContentSecurityPolicyIntegrationTest < ActionDispatch::IntegrationT end POLICY = ActionDispatch::ContentSecurityPolicy.new do |p| - p.default_src :self - p.script_src :https + p.default_src -> { :self } + p.script_src -> { :https } end class PolicyConfigMiddleware -- cgit v1.2.3 From a150a026591b7b9dcaba5a2ef5fce02f7d990aba Mon Sep 17 00:00:00 2001 From: Andrew White Date: Mon, 22 Oct 2018 17:15:33 +0100 Subject: 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. --- actionpack/CHANGELOG.md | 10 ++++++++++ actionpack/lib/action_dispatch/http/content_security_policy.rb | 3 ++- actionpack/test/dispatch/content_security_policy_test.rb | 10 ++++++++-- 3 files changed, 20 insertions(+), 3 deletions(-) (limited to 'actionpack') diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index 8d0477ead3..5554d4e6b8 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,3 +1,13 @@ +* 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 Previously if a dynamic source returned a symbol such as :self it diff --git a/actionpack/lib/action_dispatch/http/content_security_policy.rb b/actionpack/lib/action_dispatch/http/content_security_policy.rb index 15b7bd1233..b1e5a28be5 100644 --- a/actionpack/lib/action_dispatch/http/content_security_policy.rb +++ b/actionpack/lib/action_dispatch/http/content_security_policy.rb @@ -22,7 +22,8 @@ module ActionDispatch #:nodoc: if policy = request.content_security_policy nonce = request.content_security_policy_nonce - headers[header_name(request)] = policy.build(request.controller_instance, nonce) + context = request.controller_instance || request + headers[header_name(request)] = policy.build(context, nonce) end response diff --git a/actionpack/test/dispatch/content_security_policy_test.rb b/actionpack/test/dispatch/content_security_policy_test.rb index 8dd4b8edb1..c8c885f35c 100644 --- a/actionpack/test/dispatch/content_security_policy_test.rb +++ b/actionpack/test/dispatch/content_security_policy_test.rb @@ -260,6 +260,7 @@ class DefaultContentSecurityPolicyIntegrationTest < ActionDispatch::IntegrationT ROUTES.draw do scope module: "default_content_security_policy_integration_test" do get "/", to: "policy#index" + get "/redirect", to: redirect("/") end end @@ -295,14 +296,19 @@ class DefaultContentSecurityPolicyIntegrationTest < ActionDispatch::IntegrationT def test_adds_nonce_to_script_src_content_security_policy_only_once get "/" get "/" + assert_response :success + assert_policy "default-src 'self'; script-src https: 'nonce-iyhD0Yc0W+c='" + end + + def test_redirect_works_with_dynamic_sources + get "/redirect" + assert_response :redirect assert_policy "default-src 'self'; script-src https: 'nonce-iyhD0Yc0W+c='" end private def assert_policy(expected, report_only: false) - assert_response :success - if report_only expected_header = "Content-Security-Policy-Report-Only" unexpected_header = "Content-Security-Policy" -- cgit v1.2.3 From 170cb2ee9ba825c6f9ea6544bb118f7e31194251 Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Thu, 25 Oct 2018 10:36:39 -0500 Subject: ActionController::API *does* support cookies, sessions ActionController::Metal provides session support by delegating `session to the request (`"@_request"`) https://github.com/rails/rails/blob/a3dcba42e2422eb9c2e77011a39ce72dc934b420/actionpack/lib/action_controller/metal.rb#L149 Though the ActionController::Cookies modules isn't included, it's really a convenience for providing a first class `cookies` object. *all* ActionController::Metal subclasses support setting cookies via the `session` object. --- actionpack/lib/action_controller/api.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_controller/api.rb b/actionpack/lib/action_controller/api.rb index 93ffff1bd6..c276ee57c0 100644 --- a/actionpack/lib/action_controller/api.rb +++ b/actionpack/lib/action_controller/api.rb @@ -12,7 +12,7 @@ module ActionController # # An API Controller is different from a normal controller in the sense that # by default it doesn't include a number of features that are usually required - # by browser access only: layouts and templates rendering, cookies, sessions, + # by browser access only: layouts and templates rendering, # flash, assets, and so on. This makes the entire controller stack thinner, # suitable for API applications. It doesn't mean you won't have such # features if you need them: they're all available for you to include in -- cgit v1.2.3 From ad3669ee110ef3a667be966e40777ab7911b1cc3 Mon Sep 17 00:00:00 2001 From: Akira Matsuda Date: Tue, 30 Oct 2018 10:19:53 +0900 Subject: We don't want these internal methods as public methods in our controllers or they would be listed in `action_methods` --- actionpack/lib/action_controller/metal/live.rb | 48 ++++++++++++++------------ 1 file changed, 25 insertions(+), 23 deletions(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_controller/metal/live.rb b/actionpack/lib/action_controller/metal/live.rb index 1482b2999a..083b762f5a 100644 --- a/actionpack/lib/action_controller/metal/live.rb +++ b/actionpack/lib/action_controller/metal/live.rb @@ -280,33 +280,35 @@ module ActionController raise error if error end - # Spawn a new thread to serve up the controller in. This is to get - # around the fact that Rack isn't based around IOs and we need to use - # a thread to stream data from the response bodies. Nobody should call - # this method except in Rails internals. Seriously! - def new_controller_thread # :nodoc: - Thread.new { - t2 = Thread.current - t2.abort_on_exception = true - yield - } + def response_body=(body) + super + response.close if response end - def log_error(exception) - logger = ActionController::Base.logger - return unless logger + private - logger.fatal do - message = +"\n#{exception.class} (#{exception.message}):\n" - message << exception.annoted_source_code.to_s if exception.respond_to?(:annoted_source_code) - message << " " << exception.backtrace.join("\n ") - "#{message}\n\n" + # Spawn a new thread to serve up the controller in. This is to get + # around the fact that Rack isn't based around IOs and we need to use + # a thread to stream data from the response bodies. Nobody should call + # this method except in Rails internals. Seriously! + def new_controller_thread # :nodoc: + Thread.new { + t2 = Thread.current + t2.abort_on_exception = true + yield + } end - end - def response_body=(body) - super - response.close if response - end + def log_error(exception) + logger = ActionController::Base.logger + return unless logger + + logger.fatal do + message = +"\n#{exception.class} (#{exception.message}):\n" + message << exception.annoted_source_code.to_s if exception.respond_to?(:annoted_source_code) + message << " " << exception.backtrace.join("\n ") + "#{message}\n\n" + end + end end end -- cgit v1.2.3 From 1c11688b5624394c3792d1bb37599fd1e3452c9c Mon Sep 17 00:00:00 2001 From: Gannon McGibbon Date: Tue, 6 Nov 2018 14:17:23 -0500 Subject: Add CVE note to security guide and gemspecs [ci skip] --- actionpack/actionpack.gemspec | 3 +++ 1 file changed, 3 insertions(+) (limited to 'actionpack') diff --git a/actionpack/actionpack.gemspec b/actionpack/actionpack.gemspec index 1dc8abf746..4b9c729955 100644 --- a/actionpack/actionpack.gemspec +++ b/actionpack/actionpack.gemspec @@ -2,6 +2,9 @@ version = File.read(File.expand_path("../RAILS_VERSION", __dir__)).strip +# NOTE: There's no need to update dependencies for CVEs in minor +# releases when users can simply run `bundle update vulnerable_gem`. + Gem::Specification.new do |s| s.platform = Gem::Platform::RUBY s.name = "actionpack" -- cgit v1.2.3 From e74fdbe00cd0f403d34f2bc83eb09e7a5bc56109 Mon Sep 17 00:00:00 2001 From: Gannon McGibbon Date: Tue, 6 Nov 2018 18:05:40 -0500 Subject: Amend CVE note and security guide section wordings Reword first sentence of dep management and CVE section of security guide. Also, reword and move gemspec notes above deps. [ci skip] --- actionpack/actionpack.gemspec | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'actionpack') diff --git a/actionpack/actionpack.gemspec b/actionpack/actionpack.gemspec index 4b9c729955..ec56de18f1 100644 --- a/actionpack/actionpack.gemspec +++ b/actionpack/actionpack.gemspec @@ -2,9 +2,6 @@ version = File.read(File.expand_path("../RAILS_VERSION", __dir__)).strip -# NOTE: There's no need to update dependencies for CVEs in minor -# releases when users can simply run `bundle update vulnerable_gem`. - Gem::Specification.new do |s| s.platform = Gem::Platform::RUBY s.name = "actionpack" @@ -29,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" -- cgit v1.2.3 From 5df4efd2fd08dfdf8583bcb6a4322aa5997c18e1 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Thu, 1 Nov 2018 12:42:06 +0900 Subject: Fix broken CHANGELOG markup [ci skip] And remove trailing spaces. --- actionpack/CHANGELOG.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'actionpack') diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index 5554d4e6b8..81201795d4 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -3,9 +3,9 @@ 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 +14,7 @@ would be converted to a string implicity, e.g: policy.default_src -> { :self } - + would generate the header: Content-Security-Policy: default-src self -- cgit v1.2.3 From 59895db44b73b9a333387eee2078fda8feec9ce5 Mon Sep 17 00:00:00 2001 From: Maxim Perepelitsa Date: Fri, 9 Nov 2018 01:03:04 +0700 Subject: Reset sessions on failed system test screenshot Reset Capybara sessions if `take_failed_screenshot` raise exception in system test `after_teardown`. --- actionpack/CHANGELOG.md | 7 +++++++ .../system_testing/test_helpers/setup_and_teardown.rb | 7 +++++-- 2 files changed, 12 insertions(+), 2 deletions(-) (limited to 'actionpack') diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index 81201795d4..97d7049cba 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,3 +1,10 @@ +* 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 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 -- cgit v1.2.3 From 6b3faf8e502dbd238fe4b4c409e96308638297a1 Mon Sep 17 00:00:00 2001 From: Gannon McGibbon Date: Mon, 29 Oct 2018 13:45:26 -0400 Subject: Allow rescue from parameter parse errors [Gannon McGibbon + Josh Cheek] --- actionpack/CHANGELOG.md | 10 +++++++ .../lib/action_controller/metal/params_wrapper.rb | 5 +++- .../lib/action_dispatch/http/filter_parameters.rb | 2 ++ .../lib/action_dispatch/http/mime_negotiation.rb | 7 ++++- actionpack/lib/action_dispatch/http/parameters.rb | 16 ++++++++-- actionpack/lib/action_dispatch/http/request.rb | 3 -- actionpack/test/controller/params_parse_test.rb | 34 ++++++++++++++++++++++ actionpack/test/controller/rescue_test.rb | 26 +++++++++++++++++ 8 files changed, 95 insertions(+), 8 deletions(-) create mode 100644 actionpack/test/controller/params_parse_test.rb (limited to 'actionpack') diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index 97d7049cba..90cf989100 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,3 +1,13 @@ +* 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 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/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 -- cgit v1.2.3 From 028bd403efa2cba02f3871651cb3d14420ab9a9b Mon Sep 17 00:00:00 2001 From: "yuuji.yaginuma" Date: Sun, 18 Nov 2018 09:00:50 +0900 Subject: Remove unused `Journey::Router::RoutingError` `Journey::Router::RoutingError` is no longer used since db06d128262b49c8b02e153cf95eb46f4eff364b. --- actionpack/lib/action_dispatch/journey/router.rb | 3 --- 1 file changed, 3 deletions(-) (limited to 'actionpack') 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) -- cgit v1.2.3 From dde9c488398293fb1cbdc02595b8c4e9860b03cc Mon Sep 17 00:00:00 2001 From: Gannon McGibbon Date: Tue, 20 Nov 2018 13:16:39 -0500 Subject: 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. --- actionpack/CHANGELOG.md | 7 +++++++ actionpack/lib/action_dispatch/routing/mapper.rb | 6 ++---- actionpack/test/dispatch/routing_test.rb | 16 +++++++++++++--- 3 files changed, 22 insertions(+), 7 deletions(-) (limited to 'actionpack') diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index 90cf989100..740c6db06f 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,3 +1,10 @@ +* 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: ``` diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb index 06ce165f76..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 @@ -1952,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/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" -- cgit v1.2.3 From 38b676181b7cce5191b1877ad6781c490d38436d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Mon, 26 Nov 2018 15:21:11 -0500 Subject: Use env instead of headers on those tests We are dealing with the rack env so it is better to specify it in the tests. --- actionpack/test/dispatch/show_exceptions_test.rb | 28 ++++++++++++------------ 1 file changed, 14 insertions(+), 14 deletions(-) (limited to 'actionpack') diff --git a/actionpack/test/dispatch/show_exceptions_test.rb b/actionpack/test/dispatch/show_exceptions_test.rb index b69071b44b..f802abc653 100644 --- a/actionpack/test/dispatch/show_exceptions_test.rb +++ b/actionpack/test/dispatch/show_exceptions_test.rb @@ -36,30 +36,30 @@ class ShowExceptionsTest < ActionDispatch::IntegrationTest test "skip exceptions app if not showing exceptions" do @app = ProductionApp assert_raise RuntimeError do - get "/", headers: { "action_dispatch.show_exceptions" => false } + get "/", env: { "action_dispatch.show_exceptions" => false } end end test "rescue with error page" do @app = ProductionApp - get "/", headers: { "action_dispatch.show_exceptions" => true } + get "/", env: { "action_dispatch.show_exceptions" => true } assert_response 500 assert_equal "500 error fixture\n", body - get "/bad_params", headers: { "action_dispatch.show_exceptions" => true } + get "/bad_params", env: { "action_dispatch.show_exceptions" => true } assert_response 400 assert_equal "400 error fixture\n", body - get "/not_found", headers: { "action_dispatch.show_exceptions" => true } + get "/not_found", env: { "action_dispatch.show_exceptions" => true } assert_response 404 assert_equal "404 error fixture\n", body - get "/method_not_allowed", headers: { "action_dispatch.show_exceptions" => true } + get "/method_not_allowed", env: { "action_dispatch.show_exceptions" => true } assert_response 405 assert_equal "", body - get "/unknown_http_method", headers: { "action_dispatch.show_exceptions" => true } + get "/unknown_http_method", env: { "action_dispatch.show_exceptions" => true } assert_response 405 assert_equal "", body end @@ -70,11 +70,11 @@ class ShowExceptionsTest < ActionDispatch::IntegrationTest begin @app = ProductionApp - get "/", headers: { "action_dispatch.show_exceptions" => true } + get "/", env: { "action_dispatch.show_exceptions" => true } assert_response 500 assert_equal "500 localized error fixture\n", body - get "/not_found", headers: { "action_dispatch.show_exceptions" => true } + get "/not_found", env: { "action_dispatch.show_exceptions" => true } assert_response 404 assert_equal "404 error fixture\n", body ensure @@ -85,14 +85,14 @@ class ShowExceptionsTest < ActionDispatch::IntegrationTest test "sets the HTTP charset parameter" do @app = ProductionApp - get "/", headers: { "action_dispatch.show_exceptions" => true } + get "/", env: { "action_dispatch.show_exceptions" => true } assert_equal "text/html; charset=utf-8", response.headers["Content-Type"] end test "show registered original exception for wrapped exceptions" do @app = ProductionApp - get "/not_found_original_exception", headers: { "action_dispatch.show_exceptions" => true } + get "/not_found_original_exception", env: { "action_dispatch.show_exceptions" => true } assert_response 404 assert_match(/404 error/, body) end @@ -106,7 +106,7 @@ class ShowExceptionsTest < ActionDispatch::IntegrationTest end @app = ActionDispatch::ShowExceptions.new(Boomer.new, exceptions_app) - get "/not_found_original_exception", headers: { "action_dispatch.show_exceptions" => true } + get "/not_found_original_exception", env: { "action_dispatch.show_exceptions" => true } assert_response 404 assert_equal "YOU FAILED", body end @@ -117,7 +117,7 @@ class ShowExceptionsTest < ActionDispatch::IntegrationTest end @app = ActionDispatch::ShowExceptions.new(Boomer.new, exceptions_app) - get "/method_not_allowed", headers: { "action_dispatch.show_exceptions" => true } + get "/method_not_allowed", env: { "action_dispatch.show_exceptions" => true } assert_response 405 assert_equal "", body end @@ -125,12 +125,12 @@ class ShowExceptionsTest < ActionDispatch::IntegrationTest test "bad params exception is returned in the correct format" do @app = ProductionApp - get "/bad_params", headers: { "action_dispatch.show_exceptions" => true } + get "/bad_params", env: { "action_dispatch.show_exceptions" => true } assert_equal "text/html; charset=utf-8", response.headers["Content-Type"] assert_response 400 assert_match(/400 error/, body) - get "/bad_params.json", headers: { "action_dispatch.show_exceptions" => true } + get "/bad_params.json", env: { "action_dispatch.show_exceptions" => true } assert_equal "application/json; charset=utf-8", response.headers["Content-Type"] assert_response 400 assert_equal("{\"status\":400,\"error\":\"Bad Request\"}", body) -- cgit v1.2.3 From 4b5c4ca377a9b5c75d2c4b4d4f63f53866553b40 Mon Sep 17 00:00:00 2001 From: "yuuji.yaginuma" Date: Wed, 28 Nov 2018 16:10:14 +0900 Subject: Use `Testing::Parallelization` in Action Packs's test Parallel execution of `ForkingExecutor` is the same approach as `Testing::Parallelization`. So do not need to have own code inside Action Pack. Let's use an already existing feature. --- actionpack/test/abstract_unit.rb | 78 +--------------------------------------- 1 file changed, 1 insertion(+), 77 deletions(-) (limited to 'actionpack') diff --git a/actionpack/test/abstract_unit.rb b/actionpack/test/abstract_unit.rb index 65dd28b3d7..f23151e518 100644 --- a/actionpack/test/abstract_unit.rb +++ b/actionpack/test/abstract_unit.rb @@ -13,13 +13,6 @@ silence_warnings do Encoding.default_external = Encoding::UTF_8 end -require "drb" -begin - require "drb/unix" -rescue LoadError - puts "'drb/unix' is not available" -end - if ENV["TRAVIS"] PROCESS_COUNT = 0 else @@ -80,7 +73,7 @@ end module ActiveSupport class TestCase if RUBY_ENGINE == "ruby" && PROCESS_COUNT > 0 - parallelize_me! + parallelize(workers: PROCESS_COUNT) end end end @@ -359,75 +352,6 @@ class ImagesController < ResourcesController; end require "active_support/testing/method_call_assertions" -class ForkingExecutor - class Server - include DRb::DRbUndumped - - def initialize - @queue = Queue.new - end - - def record(reporter, result) - reporter.record result - end - - def <<(o) - o[2] = DRbObject.new(o[2]) if o - @queue << o - end - def pop; @queue.pop; end - end - - def initialize(size) - @size = size - @queue = Server.new - @pool = nil - @url = DRb.start_service("drbunix:", @queue).uri - end - - def <<(work); @queue << work; end - - def shutdown - pool = @size.times.map { - fork { - DRb.stop_service - queue = DRbObject.new_with_uri @url - while job = queue.pop - klass = job[0] - method = job[1] - reporter = job[2] - result = Minitest.run_one_method klass, method - if result.error? - translate_exceptions result - end - queue.record reporter, result - end - } - } - @size.times { @queue << nil } - pool.each { |pid| Process.waitpid pid } - end - - private - def translate_exceptions(result) - result.failures.map! { |e| - begin - Marshal.dump e - e - rescue TypeError - ex = Exception.new e.message - ex.set_backtrace e.backtrace - Minitest::UnexpectedError.new ex - end - } - end -end - -if RUBY_ENGINE == "ruby" && PROCESS_COUNT > 0 - # Use N processes (N defaults to 4) - Minitest.parallel_executor = ForkingExecutor.new(PROCESS_COUNT) -end - class ActiveSupport::TestCase include ActiveSupport::Testing::MethodCallAssertions -- cgit v1.2.3 From 0b2c49aa4dad73d2af098bbbd545c05ed2ace3cd Mon Sep 17 00:00:00 2001 From: Samuel Cochran Date: Wed, 28 Nov 2018 16:45:31 +1100 Subject: Log exceptions atomically When distributed over multiple logger calls the lines can become intermixed with other log statements. Combining them into a single logger call makes sure they always get logged together. --- .../lib/action_dispatch/middleware/debug_exceptions.rb | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb b/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb index 5f5fdbc66a..61e7e73a68 100644 --- a/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb +++ b/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb @@ -180,11 +180,14 @@ module ActionDispatch trace = wrapper.framework_trace if trace.empty? ActiveSupport::Deprecation.silence do - logger.fatal " " - logger.fatal "#{exception.class} (#{exception.message}):" - log_array logger, exception.annoted_source_code if exception.respond_to?(:annoted_source_code) - logger.fatal " " - log_array logger, trace + message = [] + message << " " + message << "#{exception.class} (#{exception.message}):" + message += exception.annoted_source_code if exception.respond_to?(:annoted_source_code) + message << " " + message += trace + + log_array(logger, message) end end -- cgit v1.2.3 From 00638f31d1f5c914bac32c5f00cb0e0693274b99 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Wed, 28 Nov 2018 14:21:19 -0500 Subject: Add autoload hook for AbstractController::ActionNotFound The error can be reproduced with require "bundler/setup" require "action_controller" AbstractController::ActionNotFound --- actionpack/lib/abstract_controller.rb | 1 + 1 file changed, 1 insertion(+) (limited to 'actionpack') diff --git a/actionpack/lib/abstract_controller.rb b/actionpack/lib/abstract_controller.rb index 0477e7f1c9..3a98931167 100644 --- a/actionpack/lib/abstract_controller.rb +++ b/actionpack/lib/abstract_controller.rb @@ -7,6 +7,7 @@ require "active_support/i18n" module AbstractController extend ActiveSupport::Autoload + autoload :ActionNotFound, "abstract_controller/base" autoload :Base autoload :Caching autoload :Callbacks -- cgit v1.2.3 From acbc0e0467e7d4b7075c7b2df6404d807ba7d06c Mon Sep 17 00:00:00 2001 From: Samuel Cochran Date: Thu, 29 Nov 2018 09:14:24 +1100 Subject: Avoid extra array allocations --- actionpack/lib/action_dispatch/middleware/debug_exceptions.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb b/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb index 61e7e73a68..7669767ae3 100644 --- a/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb +++ b/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb @@ -183,9 +183,9 @@ module ActionDispatch message = [] message << " " message << "#{exception.class} (#{exception.message}):" - message += exception.annoted_source_code if exception.respond_to?(:annoted_source_code) + message.concat(exception.annoted_source_code) if exception.respond_to?(:annoted_source_code) message << " " - message += trace + message.concat(trace) log_array(logger, message) end -- cgit v1.2.3 From c93bad207087ab656b27c559012b5e8eb31ed20d Mon Sep 17 00:00:00 2001 From: Alberto Almagro Date: Sun, 2 Dec 2018 18:10:03 +0100 Subject: Remove unnecessary variable route The variable `route` was only allocated to hold an object that was immediately returned. This patch removes that variable. --- actionpack/lib/action_dispatch/routing/mapper.rb | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb index 421e2023c2..d67044b4ac 100644 --- a/actionpack/lib/action_dispatch/routing/mapper.rb +++ b/actionpack/lib/action_dispatch/routing/mapper.rb @@ -160,17 +160,8 @@ module ActionDispatch end def make_route(name, precedence) - route = Journey::Route.new(name, - application, - path, - conditions, - required_defaults, - defaults, - request_method, - precedence, - @internal) - - route + Journey::Route.new(name, application, path, conditions, required_defaults, + defaults, request_method, precedence, @internal) end def application -- cgit v1.2.3 From 8e25e9e3153d3564b6b60f840a71bd2ad9a6387c Mon Sep 17 00:00:00 2001 From: blahed Date: Mon, 3 Dec 2018 19:40:46 -0500 Subject: colorize the unpermitted params log message --- actionpack/lib/action_controller/log_subscriber.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_controller/log_subscriber.rb b/actionpack/lib/action_controller/log_subscriber.rb index afbd38e7fe..d8b04d8ddb 100644 --- a/actionpack/lib/action_controller/log_subscriber.rb +++ b/actionpack/lib/action_controller/log_subscriber.rb @@ -56,7 +56,7 @@ module ActionController def unpermitted_parameters(event) debug do unpermitted_keys = event.payload[:keys] - "Unpermitted parameter#{'s' if unpermitted_keys.size > 1}: #{unpermitted_keys.map { |e| ":#{e}" }.join(", ")}" + color("Unpermitted parameter#{'s' if unpermitted_keys.size > 1}: #{unpermitted_keys.map { |e| ":#{e}" }.join(", ")}", RED) end end -- cgit v1.2.3 From 0086400dd75e73152429f9acf2fce984d8f46e02 Mon Sep 17 00:00:00 2001 From: Alberto Almagro Date: Fri, 7 Dec 2018 00:02:38 +0100 Subject: Expand metaprogramming for Symbol, Slash and Dot. This first started with moving type method inside `ActionDispatch::Journey::Nodes::Symbol`. `AD::Journey::Nodes::Symbol#type` was generated dynamically with an `each` block. While this is OK for classes like `AD::Journey::Nodes::Slash` or `AD::Journey::Nodes::Dot` which don't have further implementation, all other classes containing more logic have this method defined in their class body. This patch does the same in this case. On code review process @kamipo suggested to fully expand over metaprogramming for Slash and Dot classes, a topic on which I agree with him. --- actionpack/lib/action_dispatch/journey/nodes/node.rb | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_dispatch/journey/nodes/node.rb b/actionpack/lib/action_dispatch/journey/nodes/node.rb index 32f632800c..086d6a3e07 100644 --- a/actionpack/lib/action_dispatch/journey/nodes/node.rb +++ b/actionpack/lib/action_dispatch/journey/nodes/node.rb @@ -65,12 +65,12 @@ module ActionDispatch def literal?; false; end end - %w{ Symbol Slash Dot }.each do |t| - class_eval <<-eoruby, __FILE__, __LINE__ + 1 - class #{t} < Terminal; - def type; :#{t.upcase}; end - end - eoruby + class Slash < Terminal # :nodoc: + def type; :SLASH; end + end + + class Dot < Terminal # :nodoc: + def type; :DOT; end end class Symbol < Terminal # :nodoc: @@ -89,6 +89,7 @@ module ActionDispatch regexp == DEFAULT_EXP end + def type; :SYMBOL; end def symbol?; true; end end -- cgit v1.2.3