Allow column name with function (e.g. `length(title)`) as safe SQL string

Currently, almost all "Dangerous query method" warnings are false alarm.
As long as almost all the warnings are false alarm, developers think
"Let's ignore the warnings by using `Arel.sql()`, it actually is false
alarm in practice.", so I think we should effort to reduce false alarm
in order to make the warnings valuable.

This allows column name with function (e.g. `length(title)`) as safe SQL
string, which is very common false alarm pattern, even in the our
codebase.

Related 6c82b6c99, 6607ecb2a, #36420.

Fixes #32995.

1 files changed, 28 insertions, 27 deletions

diff --git a/activerecord/test/cases/unsafe_raw_sql_test.rb b/activerecord/test/cases/unsafe_raw_sql_test.rb
index d4cb51afba..87edb163f2 100644
--- a/activerecord/test/cases/unsafe_raw_sql_test.rb
+++ b/activerecord/test/cases/unsafe_raw_sql_test.rb
@@ -141,7 +141,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
   test "order: disallows invalid column name" do
     with_unsafe_raw_sql_disabled do
       assert_raises(ActiveRecord::UnknownAttributeReference) do
-        Post.order("len(title) asc").pluck(:id)
+        Post.order("REPLACE(title, 'misc', 'zzzz') asc").pluck(:id)
       end
     end
   end
@@ -157,7 +157,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
   test "order: disallows invalid column with direction" do
     with_unsafe_raw_sql_disabled do
       assert_raises(ActiveRecord::UnknownAttributeReference) do
-        Post.order("len(title)" => :asc).pluck(:id)
+        Post.order("REPLACE(title, 'misc', 'zzzz')" => :asc).pluck(:id)
       end
     end
   end
@@ -190,7 +190,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
   test "order: disallows invalid Array arguments" do
     with_unsafe_raw_sql_disabled do
       assert_raises(ActiveRecord::UnknownAttributeReference) do
-        Post.order(["author_id", "length(title)"]).pluck(:id)
+        Post.order(["author_id", "REPLACE(title, 'misc', 'zzzz')"]).pluck(:id)
       end
     end
   end
@@ -198,8 +198,8 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
   test "order: allows valid Array arguments" do
     ids_expected = Post.order(Arel.sql("author_id, length(title)")).pluck(:id)
 
-    ids_depr     = with_unsafe_raw_sql_deprecated { Post.order(["author_id", Arel.sql("length(title)")]).pluck(:id) }
-    ids_disabled = with_unsafe_raw_sql_disabled   { Post.order(["author_id", Arel.sql("length(title)")]).pluck(:id) }
+    ids_depr     = with_unsafe_raw_sql_deprecated { Post.order(["author_id", "length(title)"]).pluck(:id) }
+    ids_disabled = with_unsafe_raw_sql_disabled   { Post.order(["author_id", "length(title)"]).pluck(:id) }
 
     assert_equal ids_expected, ids_depr
     assert_equal ids_expected, ids_disabled
@@ -208,7 +208,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
   test "order: logs deprecation warning for unrecognized column" do
     with_unsafe_raw_sql_deprecated do
       assert_deprecated(/Dangerous query method/) do
-        Post.order("length(title)")
+        Post.order("REPLACE(title, 'misc', 'zzzz')")
       end
     end
   end
@@ -223,11 +223,11 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
     assert_equal titles_expected, titles_disabled
   end
 
-  test "pluck: allows string column name with alias" do
-    titles_expected = Post.pluck(Arel.sql("title"))
+  test "pluck: allows string column name with function and alias" do
+    titles_expected = Post.pluck(Arel.sql("UPPER(title)"))
 
-    titles_depr     = with_unsafe_raw_sql_deprecated { Post.pluck("title AS posts_title") }
-    titles_disabled = with_unsafe_raw_sql_disabled   { Post.pluck("title AS posts_title") }
+    titles_depr     = with_unsafe_raw_sql_deprecated { Post.pluck("UPPER(title) AS title") }
+    titles_disabled = with_unsafe_raw_sql_disabled   { Post.pluck("UPPER(title) AS title") }
 
     assert_equal titles_expected, titles_depr
     assert_equal titles_expected, titles_disabled
@@ -297,7 +297,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
   test "pluck: disallows invalid column name" do
     with_unsafe_raw_sql_disabled do
       assert_raises(ActiveRecord::UnknownAttributeReference) do
-        Post.pluck("length(title)")
+        Post.pluck("REPLACE(title, 'misc', 'zzzz')")
       end
     end
   end
@@ -305,7 +305,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
   test "pluck: disallows invalid column name amongst valid names" do
     with_unsafe_raw_sql_disabled do
       assert_raises(ActiveRecord::UnknownAttributeReference) do
-        Post.pluck(:title, "length(title)")
+        Post.pluck(:title, "REPLACE(title, 'misc', 'zzzz')")
       end
     end
   end
@@ -313,7 +313,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
   test "pluck: disallows invalid column names with includes" do
     with_unsafe_raw_sql_disabled do
       assert_raises(ActiveRecord::UnknownAttributeReference) do
-        Post.includes(:comments).pluck(:title, "length(title)")
+        Post.includes(:comments).pluck(:title, "REPLACE(title, 'misc', 'zzzz')")
       end
     end
   end
@@ -328,24 +328,25 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
   test "pluck: logs deprecation warning" do
     with_unsafe_raw_sql_deprecated do
       assert_deprecated(/Dangerous query method/) do
-        Post.includes(:comments).pluck(:title, "length(title)")
+        Post.includes(:comments).pluck(:title, "REPLACE(title, 'misc', 'zzzz')")
       end
     end
   end
 
-  def with_unsafe_raw_sql_disabled(&blk)
-    with_config(:disabled, &blk)
-  end
+  private
+    def with_unsafe_raw_sql_disabled(&block)
+      with_config(:disabled, &block)
+    end
 
-  def with_unsafe_raw_sql_deprecated(&blk)
-    with_config(:deprecated, &blk)
-  end
+    def with_unsafe_raw_sql_deprecated(&block)
+      with_config(:deprecated, &block)
+    end
 
-  def with_config(new_value, &blk)
-    old_value = ActiveRecord::Base.allow_unsafe_raw_sql
-    ActiveRecord::Base.allow_unsafe_raw_sql = new_value
-    blk.call
-  ensure
-    ActiveRecord::Base.allow_unsafe_raw_sql = old_value
-  end
+    def with_config(new_value, &block)
+      old_value = ActiveRecord::Base.allow_unsafe_raw_sql
+      ActiveRecord::Base.allow_unsafe_raw_sql = new_value
+      yield
+    ensure
+      ActiveRecord::Base.allow_unsafe_raw_sql = old_value
+    end
 end