Merge pull request #20797 from byroot/prevent-url-for-ac-parameters

Prevent ActionController::Parameters in url_for

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 513f0bc7e1..b13ba06962 100644
--- a/actionpack/lib/action_controller/metal/redirecting.rb
+++ b/actionpack/lib/action_controller/metal/redirecting.rb
@@ -60,7 +60,6 @@ module ActionController
     #
     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 21dfd9cd03..0b184eace9 100644
--- a/actionpack/test/controller/redirect_test.rb
+++ b/actionpack/test/controller/redirect_test.rb
@@ -307,10 +307,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',