From 4752e7d83794ecf23c6d0367f0bcad8eee33da59 Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Tue, 7 Jul 2015 13:47:16 -0400 Subject: [PATCH] Prevent ActionController::Parameters from being passed to url_for directly --- actionpack/lib/action_controller/metal/redirecting.rb | 1 - actionpack/lib/action_dispatch/routing/url_for.rb | 5 ++++- actionpack/test/controller/redirect_test.rb | 4 ++-- actionpack/test/controller/test_case_test.rb | 2 +- actionpack/test/controller/url_for_test.rb | 7 +++++++ actionview/test/template/url_helper_test.rb | 9 --------- 6 files changed, 14 insertions(+), 14 deletions(-) diff --git a/actionpack/lib/action_controller/metal/redirecting.rb b/actionpack/lib/action_controller/metal/redirecting.rb index 0febc905f1..4d5a709a7e 100644 --- a/actionpack/lib/action_controller/metal/redirecting.rb +++ b/actionpack/lib/action_controller/metal/redirecting.rb @@ -67,7 +67,6 @@ module ActionController # ActionController::RedirectBackError. def redirect_to(options = {}, response_status = {}) #:doc: raise ActionControllerError.new("Cannot redirect to nil!") unless options - raise ActionControllerError.new("Cannot redirect to a parameter hash!") if options.is_a?(ActionController::Parameters) raise AbstractController::DoubleRenderError if response_body self.status = _extract_redirect_to_status(options, response_status) diff --git a/actionpack/lib/action_dispatch/routing/url_for.rb b/actionpack/lib/action_dispatch/routing/url_for.rb index b6c031dcf4..f91679593e 100644 --- a/actionpack/lib/action_dispatch/routing/url_for.rb +++ b/actionpack/lib/action_dispatch/routing/url_for.rb @@ -172,8 +172,11 @@ module ActionDispatch _routes.url_for(options.symbolize_keys.reverse_merge!(url_options), route_name) when ActionController::Parameters + unless options.permitted? + raise ArgumentError.new("Generating an URL from non sanitized request parameters is insecure!") + end route_name = options.delete :use_route - _routes.url_for(options.to_unsafe_h.symbolize_keys. + _routes.url_for(options.to_h.symbolize_keys. reverse_merge!(url_options), route_name) when String options diff --git a/actionpack/test/controller/redirect_test.rb b/actionpack/test/controller/redirect_test.rb index 631ff7d02a..7f806f2b1d 100644 --- a/actionpack/test/controller/redirect_test.rb +++ b/actionpack/test/controller/redirect_test.rb @@ -273,10 +273,10 @@ class RedirectTest < ActionController::TestCase end def test_redirect_to_params - error = assert_raise(ActionController::ActionControllerError) do + error = assert_raise(ArgumentError) do get :redirect_to_params end - assert_equal "Cannot redirect to a parameter hash!", error.message + assert_equal "Generating an URL from non sanitized request parameters is insecure!", error.message end def test_redirect_to_with_block diff --git a/actionpack/test/controller/test_case_test.rb b/actionpack/test/controller/test_case_test.rb index e50373a0cc..b9caddcdb7 100644 --- a/actionpack/test/controller/test_case_test.rb +++ b/actionpack/test/controller/test_case_test.rb @@ -172,7 +172,7 @@ XML before_action { @dynamic_opt = 'opt' } def test_url_options_reset - render plain: url_for(params) + render plain: url_for end def default_url_options diff --git a/actionpack/test/controller/url_for_test.rb b/actionpack/test/controller/url_for_test.rb index 78e883f134..67212fea38 100644 --- a/actionpack/test/controller/url_for_test.rb +++ b/actionpack/test/controller/url_for_test.rb @@ -375,6 +375,13 @@ module AbstractController assert_equal({'query[person][position][]' => 'prof' }.to_query, params[3]) end + def test_url_action_controller_parameters + add_host! + assert_raise(ArgumentError) do + W.new.url_for(ActionController::Parameters.new(:controller => 'c', :action => 'a', protocol: 'javascript', f: '%0Aeval(name)')) + end + end + def test_path_generation_for_symbol_parameter_keys assert_generates("/image", :controller=> :image) end diff --git a/actionview/test/template/url_helper_test.rb b/actionview/test/template/url_helper_test.rb index 62fa75bc63..784a48ed8d 100644 --- a/actionview/test/template/url_helper_test.rb +++ b/actionview/test/template/url_helper_test.rb @@ -636,10 +636,6 @@ class UrlHelperControllerTest < ActionController::TestCase render inline: "<%= url_for controller: 'url_helper_controller_test/url_helper', action: 'show_url_for' %>" end - def show_overridden_url_for - render inline: "<%= url_for params.merge(controller: 'url_helper_controller_test/url_helper', action: 'show_url_for') %>" - end - def show_named_route render inline: "<%= show_named_route_#{params[:kind]} %>" end @@ -673,11 +669,6 @@ class UrlHelperControllerTest < ActionController::TestCase assert_equal '/url_helper_controller_test/url_helper/show_url_for', @response.body end - def test_overridden_url_for_shows_only_path - get :show_overridden_url_for - assert_equal '/url_helper_controller_test/url_helper/show_url_for', @response.body - end - def test_named_route_url_shows_host_and_path get :show_named_route, params: { kind: 'url' } assert_equal 'http://test.host/url_helper_controller_test/url_helper/show_named_route',