From c4c21a9f8d7c9c8ca6570bdb82d64e2dc860e62c Mon Sep 17 00:00:00 2001
From: Gannon McGibbon <gannon.mcgibbon@shopify.com>
Date: Thu, 18 Feb 2021 13:17:08 -0500
Subject: [PATCH] Prevent string polymorphic route arguments

url_for supports building polymorphic URLs via an array
of arguments (usually symbols and records). If an array is passed,
strings can result in unwanted route helper calls.

CVE-2021-22885
---
 .../routing/polymorphic_routes.rb             | 12 +++--
 actionpack/test/controller/redirect_test.rb   | 45 +++++++++++++++++++
 .../activerecord/polymorphic_routes_test.rb   | 22 ++++++---
 3 files changed, 69 insertions(+), 10 deletions(-)

diff --git a/actionpack/lib/action_dispatch/routing/polymorphic_routes.rb b/actionpack/lib/action_dispatch/routing/polymorphic_routes.rb
index e98e4aaa66..77499bf157 100644
--- a/actionpack/lib/action_dispatch/routing/polymorphic_routes.rb
+++ b/actionpack/lib/action_dispatch/routing/polymorphic_routes.rb
@@ -287,10 +287,12 @@ module ActionDispatch
 
             args = []
 
-            route = record_list.map { |parent|
+            route = record_list.map do |parent|
               case parent
-              when Symbol, String
+              when Symbol
                 parent.to_s
+              when String
+                raise(ArgumentError, "Please use symbols for polymorphic route arguments.")
               when Class
                 args << parent
                 parent.model_name.singular_route_key
@@ -298,12 +300,14 @@ module ActionDispatch
                 args << parent.to_model
                 parent.to_model.model_name.singular_route_key
               end
-            }
+            end
 
             route <<
             case record
-            when Symbol, String
+            when Symbol
               record.to_s
+            when String
+              raise(ArgumentError, "Please use symbols for polymorphic route arguments.")
             when Class
               @key_strategy.call record.model_name
             else
diff --git a/actionpack/test/controller/redirect_test.rb b/actionpack/test/controller/redirect_test.rb
index f7495703c7..e1b31667a1 100644
--- a/actionpack/test/controller/redirect_test.rb
+++ b/actionpack/test/controller/redirect_test.rb
@@ -130,6 +130,14 @@ class RedirectController < ActionController::Base
     redirect_to nil
   end
 
+  def redirect_to_polymorphic
+    redirect_to [:internal, Workshop.new(5)]
+  end
+
+  def redirect_to_polymorphic_string_args
+    redirect_to ["internal", Workshop.new(5)]
+  end
+
   def redirect_to_params
     redirect_to ActionController::Parameters.new(status: 200, protocol: "javascript", f: "%0Aeval(name)")
   end
@@ -376,6 +384,43 @@ class RedirectTest < ActionController::TestCase
     end
   end
 
+  def test_polymorphic_redirect
+    with_routing do |set|
+      set.draw do
+        namespace :internal do
+          resources :workshops
+        end
+
+        ActiveSupport::Deprecation.silence do
+          get ":controller/:action"
+        end
+      end
+
+      get :redirect_to_polymorphic
+      assert_equal "http://test.host/internal/workshops/5", redirect_to_url
+      assert_redirected_to [:internal, Workshop.new(5)]
+    end
+  end
+
+  def test_polymorphic_redirect_with_string_args
+    with_routing do |set|
+      set.draw do
+        namespace :internal do
+          resources :workshops
+        end
+
+        ActiveSupport::Deprecation.silence do
+          get ":controller/:action"
+        end
+      end
+
+      error = assert_raises(ArgumentError) do
+        get :redirect_to_polymorphic_string_args
+      end
+      assert_equal("Please use symbols for polymorphic route arguments.", error.message)
+    end
+  end
+
   def test_redirect_to_nil
     error = assert_raise(ActionController::ActionControllerError) do
       get :redirect_to_nil
diff --git a/actionview/test/activerecord/polymorphic_routes_test.rb b/actionview/test/activerecord/polymorphic_routes_test.rb
index 45f7173d24..bde1911417 100644
--- a/actionview/test/activerecord/polymorphic_routes_test.rb
+++ b/actionview/test/activerecord/polymorphic_routes_test.rb
@@ -464,12 +464,6 @@ class PolymorphicRoutesTest < ActionController::TestCase
     end
   end
 
-  def test_with_array_containing_single_string_name
-    with_test_routes do
-      assert_url "http://example.com/projects", ["projects"]
-    end
-  end
-
   def test_with_array_containing_symbols
     with_test_routes do
       assert_url "http://example.com/series/new", [:new, :series]
@@ -624,6 +618,22 @@ class PolymorphicRoutesTest < ActionController::TestCase
     end
   end
 
+  def test_string_route_arguments
+    with_admin_test_routes do
+      error = assert_raises(ArgumentError) do
+        polymorphic_url(["admin", @project])
+      end
+
+      assert_equal("Please use symbols for polymorphic route arguments.", error.message)
+
+      error = assert_raises(ArgumentError) do
+        polymorphic_url([@project, "bid"])
+      end
+
+      assert_equal("Please use symbols for polymorphic route arguments.", error.message)
+    end
+  end
+
   def with_namespaced_routes(name)
     with_routing do |set|
       set.draw do