From 3eb9e74c287750a9fe11f700fc96d3be1e83aa35 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
---
 actionpack/CHANGELOG.md                       | 10 +++++
 .../routing/polymorphic_routes.rb             | 12 +++--
 actionpack/test/controller/redirect_test.rb   | 45 +++++++++++++++++++
 .../activerecord/polymorphic_routes_test.rb   | 22 ++++++---
 4 files changed, 79 insertions(+), 10 deletions(-)

diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
index ad806ecab9..088da2d556 100644
--- a/actionpack/CHANGELOG.md
+++ b/actionpack/CHANGELOG.md
@@ -1,3 +1,13 @@
+*   Prevent string polymorphic route arguments.
+
+    `url_for` supports building polymorphic URLs via an array
+    of arguments (usually symbols and records). If a developer passes a
+    user input array, strings can result in unwanted route helper calls.
+
+    CVE-2021-22885
+
+    *Gannon McGibbon*
+
 ## Rails 5.2.4.5 (February 10, 2021) ##
 
 *   No changes.
diff --git a/actionpack/lib/action_dispatch/routing/polymorphic_routes.rb b/actionpack/lib/action_dispatch/routing/polymorphic_routes.rb
index 6da869c0c2..84b78e1cb2 100644
--- a/actionpack/lib/action_dispatch/routing/polymorphic_routes.rb
+++ b/actionpack/lib/action_dispatch/routing/polymorphic_routes.rb
@@ -288,10 +288,12 @@ def handle_list(list)
 
             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
@@ -299,12 +301,14 @@ def handle_list(list)
                 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 461e627154..173dba6e0a 100644
--- a/actionpack/test/controller/redirect_test.rb
+++ b/actionpack/test/controller/redirect_test.rb
@@ -108,6 +108,14 @@ def redirect_to_nil
     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
@@ -310,6 +318,43 @@ def test_redirect_to_record
     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 4b931f793f..f41581d788 100644
--- a/actionview/test/activerecord/polymorphic_routes_test.rb
+++ b/actionview/test/activerecord/polymorphic_routes_test.rb
@@ -460,12 +460,6 @@ def test_with_array_containing_single_name
     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]
@@ -620,6 +614,22 @@ def test_nested_routing_to_a_model_delegate
     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
-- 
2.30.0