Merge pull request rails#44010 from siegfault/dangerous_query_method_…

…allow_nested_functions Accept nested functions in Dangerous Query Methods
bloomfire · Jul 19, 2022 · 71c59c6 · 71c59c6
2 parents fa7f977 + 8fe1bd5
commit 71c59c6
Show file tree

Hide file tree

Showing 6 changed files with 28 additions and 8 deletions.
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
@@ -1,3 +1,7 @@
+*   Allow nested functions as safe SQL string
+
+    *Michael Siegfried*
+
 *   Allow `destroy_association_async_job=` to be configured with a class string instead of a constant.
 
     Defers an autoloading dependency between `ActiveRecord::Base` and `ActiveJob::Base`

diff --git a/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb b/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb
@@ -167,7 +167,7 @@ def column_name_with_order_matcher # :nodoc:
         (
           (?:
             # table_name.column_name | function(one or no argument)
-            ((?:\w+\.)?\w+) | \w+\((?:|\g<2>)\)
+            ((?:\w+\.)?\w+ | \w+\((?:|\g<2>)\))
           )
           (?:(?:\s+AS)?\s+\w+)?
         )
@@ -191,7 +191,7 @@ def column_name_with_order_matcher # :nodoc:
         (
           (?:
             # table_name.column_name | function(one or no argument)
-            ((?:\w+\.)?\w+) | \w+\((?:|\g<2>)\)
+            ((?:\w+\.)?\w+ | \w+\((?:|\g<2>)\))
           )
           (?:\s+ASC|\s+DESC)?
           (?:\s+NULLS\s+(?:FIRST|LAST))?

diff --git a/activerecord/lib/active_record/connection_adapters/mysql/quoting.rb b/activerecord/lib/active_record/connection_adapters/mysql/quoting.rb
@@ -84,7 +84,7 @@ def column_name_with_order_matcher
           (
             (?:
               # `table_name`.`column_name` | function(one or no argument)
-              ((?:\w+\.|`\w+`\.)?(?:\w+|`\w+`)) | \w+\((?:|\g<2>)\)
+              ((?:\w+\.|`\w+`\.)?(?:\w+|`\w+`) | \w+\((?:|\g<2>)\))
             )
             (?:(?:\s+AS)?\s+(?:\w+|`\w+`))?
           )
@@ -97,7 +97,7 @@ def column_name_with_order_matcher
           (
             (?:
               # `table_name`.`column_name` | function(one or no argument)
-              ((?:\w+\.|`\w+`\.)?(?:\w+|`\w+`)) | \w+\((?:|\g<2>)\)
+              ((?:\w+\.|`\w+`\.)?(?:\w+|`\w+`) | \w+\((?:|\g<2>)\))
             )
             (?:\s+COLLATE\s+(?:\w+|"\w+"))?
             (?:\s+ASC|\s+DESC)?

diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb b/activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb
@@ -134,7 +134,7 @@ def column_name_with_order_matcher
           (
             (?:
               # "schema_name"."table_name"."column_name"::type_name | function(one or no argument)::type_name
-              ((?:\w+\.|"\w+"\.){,2}(?:\w+|"\w+")(?:::\w+)?) | \w+\((?:|\g<2>)\)(?:::\w+)?
+              ((?:\w+\.|"\w+"\.){,2}(?:\w+|"\w+")(?:::\w+)? | \w+\((?:|\g<2>)\)(?:::\w+)?)
             )
             (?:(?:\s+AS)?\s+(?:\w+|"\w+"))?
           )
@@ -147,7 +147,7 @@ def column_name_with_order_matcher
           (
             (?:
               # "schema_name"."table_name"."column_name"::type_name | function(one or no argument)::type_name
-              ((?:\w+\.|"\w+"\.){,2}(?:\w+|"\w+")(?:::\w+)?) | \w+\((?:|\g<2>)\)(?:::\w+)?
+              ((?:\w+\.|"\w+"\.){,2}(?:\w+|"\w+")(?:::\w+)? | \w+\((?:|\g<2>)\)(?:::\w+)?)
             )
             (?:\s+COLLATE\s+"\w+")?
             (?:\s+ASC|\s+DESC)?

diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3/quoting.rb b/activerecord/lib/active_record/connection_adapters/sqlite3/quoting.rb
@@ -86,7 +86,7 @@ def column_name_with_order_matcher
           (
             (?:
               # "table_name"."column_name" | function(one or no argument)
-              ((?:\w+\.|"\w+"\.)?(?:\w+|"\w+")) | \w+\((?:|\g<2>)\)
+              ((?:\w+\.|"\w+"\.)?(?:\w+|"\w+") | \w+\((?:|\g<2>)\))
             )
             (?:(?:\s+AS)?\s+(?:\w+|"\w+"))?
           )
@@ -99,7 +99,7 @@ def column_name_with_order_matcher
           (
             (?:
               # "table_name"."column_name" | function(one or no argument)
-              ((?:\w+\.|"\w+"\.)?(?:\w+|"\w+")) | \w+\((?:|\g<2>)\)
+              ((?:\w+\.|"\w+"\.)?(?:\w+|"\w+") | \w+\((?:|\g<2>)\))
             )
             (?:\s+COLLATE\s+(?:\w+|"\w+"))?
             (?:\s+ASC|\s+DESC)?

diff --git a/activerecord/test/cases/unsafe_raw_sql_test.rb b/activerecord/test/cases/unsafe_raw_sql_test.rb
@@ -180,6 +180,14 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
     assert_equal ids_expected, ids
   end
 
+  test "order: allows nested functions" do
+    ids_expected = Post.order(Arel.sql("author_id, length(trim(title))")).pluck(:id)
+
+    ids = Post.order("author_id, length(trim(title))").pluck(:id)
+
+    assert_equal ids_expected, ids
+  end
+
   test "order: logs deprecation warning for unrecognized column" do
     e = assert_raises(ActiveRecord::UnknownAttributeReference) do
       Post.order("REPLACE(title, 'misc', 'zzzz')")
@@ -253,6 +261,14 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
     assert_equal titles_expected, titles
   end
 
+  test "pluck: allows nested functions" do
+    title_lengths_expected = Post.pluck(Arel.sql("length(trim(title))"))
+
+    title_lengths = Post.pluck("length(trim(title))")
+
+    assert_equal title_lengths_expected, title_lengths
+  end
+
   test "pluck: disallows invalid column name" do
     assert_raises(ActiveRecord::UnknownAttributeReference) do
       Post.pluck("REPLACE(title, 'misc', 'zzzz')")