Merge pull request #36420 from kamipo/quoted_identifier_regex

Allow quoted identifier string as safe SQL string

10 files changed, 166 insertions, 68 deletions

diff --git a/activerecord/lib/active_record/attribute_methods.rb b/activerecord/lib/active_record/attribute_methods.rb
index fd32eaaf3a..21f72bb6c7 100644
--- a/activerecord/lib/active_record/attribute_methods.rb
+++ b/activerecord/lib/active_record/attribute_methods.rb
@@ -159,59 +159,6 @@ module ActiveRecord
         end
       end
 
-      # Regexp for column names (with or without a table name prefix). Matches
-      # the following:
-      #   "#{table_name}.#{column_name}"
-      #   "#{column_name}"
-      COLUMN_NAME = /\A(?:\w+\.)?\w+\z/i
-
-      # Regexp for column names with order (with or without a table name
-      # prefix, with or without various order modifiers). Matches the following:
-      #   "#{table_name}.#{column_name}"
-      #   "#{table_name}.#{column_name} #{direction}"
-      #   "#{table_name}.#{column_name} #{direction} NULLS FIRST"
-      #   "#{table_name}.#{column_name} NULLS LAST"
-      #   "#{column_name}"
-      #   "#{column_name} #{direction}"
-      #   "#{column_name} #{direction} NULLS FIRST"
-      #   "#{column_name} NULLS LAST"
-      COLUMN_NAME_WITH_ORDER = /
-        \A
-        (?:\w+\.)?
-        \w+
-        (?:\s+asc|\s+desc)?
-        (?:\s+nulls\s+(?:first|last))?
-        \z
-      /ix
-
-      def disallow_raw_sql!(args, permit: COLUMN_NAME) # :nodoc:
-        unexpected = nil
-        args.each do |arg|
-          next if arg.is_a?(Symbol) || Arel.arel_node?(arg) ||
-            arg.to_s.split(/\s*,\s*/).all? { |part| permit.match?(part) }
-          (unexpected ||= []) << arg
-        end
-
-        return unless unexpected
-
-        if allow_unsafe_raw_sql == :deprecated
-          ActiveSupport::Deprecation.warn(
-            "Dangerous query method (method whose arguments are used as raw " \
-            "SQL) called with non-attribute argument(s): " \
-            "#{unexpected.map(&:inspect).join(", ")}. Non-attribute " \
-            "arguments will be disallowed in Rails 6.1. This method should " \
-            "not be called with user-provided values, such as request " \
-            "parameters or model attributes. Known-safe values can be passed " \
-            "by wrapping them in Arel.sql()."
-          )
-        else
-          raise(ActiveRecord::UnknownAttributeReference,
-            "Query method called with non-attribute argument(s): " +
-            unexpected.map(&:inspect).join(", ")
-          )
-        end
-      end
-
       # Returns true if the given attribute exists, otherwise false.
       #
       #   class Person < ActiveRecord::Base
diff --git a/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb b/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb
index 2877530917..99e1a11f30 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb
@@ -142,6 +142,43 @@ module ActiveRecord
         value.to_s.gsub(%r{ (/ (?: | \g<1>) \*) \+? \s* | \s* (\* (?: | \g<2>) /) }x, "")
       end
 
+      def column_name_matcher # :nodoc:
+        COLUMN_NAME
+      end
+
+      def column_name_with_order_matcher # :nodoc:
+        COLUMN_NAME_WITH_ORDER
+      end
+
+      # Regexp for column names (with or without a table name prefix).
+      # Matches the following:
+      #
+      #   "#{table_name}.#{column_name}"
+      #   "#{column_name}"
+      COLUMN_NAME = /\A(?:\w+\.)?\w+\z/i
+
+      # Regexp for column names with order (with or without a table name prefix,
+      # with or without various order modifiers). Matches the following:
+      #
+      #   "#{table_name}.#{column_name}"
+      #   "#{table_name}.#{column_name} #{direction}"
+      #   "#{table_name}.#{column_name} #{direction} NULLS FIRST"
+      #   "#{table_name}.#{column_name} NULLS LAST"
+      #   "#{column_name}"
+      #   "#{column_name} #{direction}"
+      #   "#{column_name} #{direction} NULLS FIRST"
+      #   "#{column_name} NULLS LAST"
+      COLUMN_NAME_WITH_ORDER = /
+        \A
+        (?:\w+\.)?
+        \w+
+        (?:\s+ASC|\s+DESC)?
+        (?:\s+NULLS\s+(?:FIRST|LAST))?
+        \z
+      /ix
+
+      private_constant :COLUMN_NAME, :COLUMN_NAME_WITH_ORDER
+
       private
         def type_casted_binds(binds)
           if binds.first.is_a?(Array)
diff --git a/activerecord/lib/active_record/connection_adapters/mysql/quoting.rb b/activerecord/lib/active_record/connection_adapters/mysql/quoting.rb
index 75564a61d6..84354c0187 100644
--- a/activerecord/lib/active_record/connection_adapters/mysql/quoting.rb
+++ b/activerecord/lib/active_record/connection_adapters/mysql/quoting.rb
@@ -32,12 +32,33 @@ module ActiveRecord
           "x'#{value.hex}'"
         end
 
-        def _type_cast(value)
-          case value
-          when Date, Time then value
-          else super
-          end
+        def column_name_matcher
+          COLUMN_NAME
+        end
+
+        def column_name_with_order_matcher
+          COLUMN_NAME_WITH_ORDER
         end
+
+        COLUMN_NAME = /\A(?:(`?)\w+\k<1>\.)?(`?)\w+\k<2>\z/i
+
+        COLUMN_NAME_WITH_ORDER = /
+          \A
+          (?:(`?)\w+\k<1>\.)?
+          (`?)\w+\k<2>
+          (?:\s+ASC|\s+DESC)?
+          \z
+        /ix
+
+        private_constant :COLUMN_NAME, :COLUMN_NAME_WITH_ORDER
+
+        private
+          def _type_cast(value)
+            case value
+            when Date, Time then value
+            else super
+            end
+          end
       end
     end
   end
diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb b/activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb
index d40e0ef1f0..0ebed21717 100644
--- a/activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb
+++ b/activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb
@@ -78,6 +78,28 @@ module ActiveRecord
           type_map.lookup(column.oid, column.fmod, column.sql_type)
         end
 
+        def column_name_matcher
+          COLUMN_NAME
+        end
+
+        def column_name_with_order_matcher
+          COLUMN_NAME_WITH_ORDER
+        end
+
+        COLUMN_NAME = /\A(?:("?)\w+\k<1>\.)?("?)\w+\k<2>(?:::\w+)?\z/i
+
+        COLUMN_NAME_WITH_ORDER = /
+          \A
+          (?:("?)\w+\k<1>\.)?
+          ("?)\w+\k<2>
+          (?:::\w+)?
+          (?:\s+ASC|\s+DESC)?
+          (?:\s+NULLS\s+(?:FIRST|LAST))?
+          \z
+        /ix
+
+        private_constant :COLUMN_NAME, :COLUMN_NAME_WITH_ORDER
+
         private
           def lookup_cast_type(sql_type)
             super(query_value("SELECT #{quote(sql_type)}::regtype::oid", "SCHEMA").to_i)
diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3/quoting.rb b/activerecord/lib/active_record/connection_adapters/sqlite3/quoting.rb
index cb9d32a577..79d477cdb2 100644
--- a/activerecord/lib/active_record/connection_adapters/sqlite3/quoting.rb
+++ b/activerecord/lib/active_record/connection_adapters/sqlite3/quoting.rb
@@ -45,6 +45,26 @@ module ActiveRecord
           0
         end
 
+        def column_name_matcher
+          COLUMN_NAME
+        end
+
+        def column_name_with_order_matcher
+          COLUMN_NAME_WITH_ORDER
+        end
+
+        COLUMN_NAME = /\A(?:("?)\w+\k<1>\.)?("?)\w+\k<2>\z/i
+
+        COLUMN_NAME_WITH_ORDER = /
+          \A
+          (?:("?)\w+\k<1>\.)?
+          ("?)\w+\k<2>
+          (?:\s+ASC|\s+DESC)?
+          \z
+        /ix
+
+        private_constant :COLUMN_NAME, :COLUMN_NAME_WITH_ORDER
+
         private
 
           def _type_cast(value)
diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb
index 50ff733dc7..588cb130f2 100644
--- a/activerecord/lib/active_record/relation/query_methods.rb
+++ b/activerecord/lib/active_record/relation/query_methods.rb
@@ -1254,7 +1254,7 @@ module ActiveRecord
 
         @klass.disallow_raw_sql!(
           order_args.flat_map { |a| a.is_a?(Hash) ? a.keys : a },
-          permit: AttributeMethods::ClassMethods::COLUMN_NAME_WITH_ORDER
+          permit: connection.column_name_with_order_matcher
         )
 
         validate_order_args(order_args)
diff --git a/activerecord/lib/active_record/sanitization.rb b/activerecord/lib/active_record/sanitization.rb
index 750766714d..5296499bad 100644
--- a/activerecord/lib/active_record/sanitization.rb
+++ b/activerecord/lib/active_record/sanitization.rb
@@ -61,8 +61,9 @@ module ActiveRecord
       #   # => "id ASC"
       def sanitize_sql_for_order(condition)
         if condition.is_a?(Array) && condition.first.to_s.include?("?")
-          disallow_raw_sql!([condition.first],
-            permit: AttributeMethods::ClassMethods::COLUMN_NAME_WITH_ORDER
+          disallow_raw_sql!(
+            [condition.first],
+            permit: connection.column_name_with_order_matcher
           )
 
           # Ensure we aren't dealing with a subclass of String that might
@@ -133,6 +134,34 @@ module ActiveRecord
         end
       end
 
+      def disallow_raw_sql!(args, permit: connection.column_name_matcher) # :nodoc:
+        unexpected = nil
+        args.each do |arg|
+          next if arg.is_a?(Symbol) || Arel.arel_node?(arg) ||
+            arg.to_s.split(/\s*,\s*/).all? { |part| permit.match?(part) }
+          (unexpected ||= []) << arg
+        end
+
+        return unless unexpected
+
+        if allow_unsafe_raw_sql == :deprecated
+          ActiveSupport::Deprecation.warn(
+            "Dangerous query method (method whose arguments are used as raw " \
+            "SQL) called with non-attribute argument(s): " \
+            "#{unexpected.map(&:inspect).join(", ")}. Non-attribute " \
+            "arguments will be disallowed in Rails 6.1. This method should " \
+            "not be called with user-provided values, such as request " \
+            "parameters or model attributes. Known-safe values can be passed " \
+            "by wrapping them in Arel.sql()."
+          )
+        else
+          raise(ActiveRecord::UnknownAttributeReference,
+            "Query method called with non-attribute argument(s): " +
+            unexpected.map(&:inspect).join(", ")
+          )
+        end
+      end
+
       private
         def replace_bind_variables(statement, values)
           raise_if_bind_arity_mismatch(statement, statement.count("?"), values.size)
diff --git a/activerecord/test/cases/associations/eager_test.rb b/activerecord/test/cases/associations/eager_test.rb
index f7aad9d775..c270d60b2f 100644
--- a/activerecord/test/cases/associations/eager_test.rb
+++ b/activerecord/test/cases/associations/eager_test.rb
@@ -523,7 +523,7 @@ class EagerAssociationTest < ActiveRecord::TestCase
   def test_eager_association_loading_with_belongs_to_and_order_string_with_quoted_table_name
     quoted_posts_id = Comment.connection.quote_table_name("posts") + "." + Comment.connection.quote_column_name("id")
     assert_nothing_raised do
-      Comment.includes(:post).references(:posts).order(Arel.sql(quoted_posts_id))
+      Comment.includes(:post).references(:posts).order(quoted_posts_id)
     end
   end
 
diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb
index 4ec695c4c6..91353e4f9e 100644
--- a/activerecord/test/cases/relations_test.rb
+++ b/activerecord/test/cases/relations_test.rb
@@ -1679,7 +1679,7 @@ class RelationTest < ActiveRecord::TestCase
     scope = Post.order("comments.body")
     assert_equal ["comments"], scope.references_values
 
-    scope = Post.order(Arel.sql("#{Comment.quoted_table_name}.#{Comment.quoted_primary_key}"))
+    scope = Post.order("#{Comment.quoted_table_name}.#{Comment.quoted_primary_key}")
     if current_adapter?(:OracleAdapter)
       assert_equal ["COMMENTS"], scope.references_values
     else
@@ -1704,7 +1704,7 @@ class RelationTest < ActiveRecord::TestCase
     scope = Post.reorder("comments.body")
     assert_equal %w(comments), scope.references_values
 
-    scope = Post.reorder(Arel.sql("#{Comment.quoted_table_name}.#{Comment.quoted_primary_key}"))
+    scope = Post.reorder("#{Comment.quoted_table_name}.#{Comment.quoted_primary_key}")
     if current_adapter?(:OracleAdapter)
       assert_equal ["COMMENTS"], scope.references_values
     else
diff --git a/activerecord/test/cases/unsafe_raw_sql_test.rb b/activerecord/test/cases/unsafe_raw_sql_test.rb
index d5d8f2a09a..fc92bf73c9 100644
--- a/activerecord/test/cases/unsafe_raw_sql_test.rb
+++ b/activerecord/test/cases/unsafe_raw_sql_test.rb
@@ -77,7 +77,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
     assert_equal ids_expected, ids_disabled
   end
 
-  test "order: allows table and column name" do
+  test "order: allows table and column names" do
     ids_expected = Post.order(Arel.sql("title")).pluck(:id)
 
     ids_depr     = with_unsafe_raw_sql_deprecated { Post.order("posts.title").pluck(:id) }
@@ -87,6 +87,17 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
     assert_equal ids_expected, ids_disabled
   end
 
+  test "order: allows quoted table and column names" do
+    ids_expected = Post.order(Arel.sql("title")).pluck(:id)
+
+    quoted_title = Post.connection.quote_table_name("posts.title")
+    ids_depr     = with_unsafe_raw_sql_deprecated { Post.order(quoted_title).pluck(:id) }
+    ids_disabled = with_unsafe_raw_sql_disabled   { Post.order(quoted_title).pluck(:id) }
+
+    assert_equal ids_expected, ids_depr
+    assert_equal ids_expected, ids_disabled
+  end
+
   test "order: allows column name and direction in string" do
     ids_expected = Post.order(Arel.sql("title desc")).pluck(:id)
 
@@ -116,10 +127,10 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
 
     ["asc", "desc", ""].each do |direction|
       %w(first last).each do |position|
-        ids_expected = Post.order(Arel.sql("type #{direction} nulls #{position}")).pluck(:id)
+        ids_expected = Post.order(Arel.sql("type::text #{direction} nulls #{position}")).pluck(:id)
 
-        ids_depr     = with_unsafe_raw_sql_deprecated { Post.order("type #{direction} nulls #{position}").pluck(:id) }
-        ids_disabled = with_unsafe_raw_sql_disabled   { Post.order("type #{direction} nulls #{position}").pluck(:id) }
+        ids_depr     = with_unsafe_raw_sql_deprecated { Post.order("type::text #{direction} nulls #{position}").pluck(:id) }
+        ids_disabled = with_unsafe_raw_sql_disabled   { Post.order("type::text #{direction} nulls #{position}").pluck(:id) }
 
         assert_equal ids_expected, ids_depr
         assert_equal ids_expected, ids_disabled
@@ -262,6 +273,17 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
     assert_equal titles_expected, titles_disabled
   end
 
+  test "pluck: allows quoted table and column names" do
+    titles_expected = Post.pluck(Arel.sql("title"))
+
+    quoted_title    = Post.connection.quote_table_name("posts.title")
+    titles_depr     = with_unsafe_raw_sql_deprecated { Post.pluck(quoted_title) }
+    titles_disabled = with_unsafe_raw_sql_disabled   { Post.pluck(quoted_title) }
+
+    assert_equal titles_expected, titles_depr
+    assert_equal titles_expected, titles_disabled
+  end
+
   test "pluck: disallows invalid column name" do
     with_unsafe_raw_sql_disabled do
       assert_raises(ActiveRecord::UnknownAttributeReference) do