push order arg checks down to allow for binds

3 files changed, 47 insertions, 28 deletions

diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb
index 1dcd786498..cef0847651 100644
--- a/activerecord/lib/active_record/relation/query_methods.rb
+++ b/activerecord/lib/active_record/relation/query_methods.rb
@@ -297,11 +297,6 @@ module ActiveRecord
 
     # Same as #order but operates on relation in-place instead of copying.
     def order!(*args) # :nodoc:
-      @klass.enforce_raw_sql_whitelist(
-        column_names_from_order_arguments(args),
-        whitelist: AttributeMethods::ClassMethods::COLUMN_NAME_ORDER_WHITELIST
-      )
-
       preprocess_order_args(args)
 
       self.order_values += args
@@ -324,11 +319,6 @@ module ActiveRecord
 
     # Same as #reorder but operates on relation in-place instead of copying.
     def reorder!(*args) # :nodoc:
-      @klass.enforce_raw_sql_whitelist(
-        column_names_from_order_arguments(args),
-        whitelist: AttributeMethods::ClassMethods::COLUMN_NAME_ORDER_WHITELIST
-      )
-
       preprocess_order_args(args)
 
       self.reordering_value = true
@@ -928,23 +918,6 @@ module ActiveRecord
 
     private
 
-      # Extract column names from arguments passed to #order or #reorder.
-      def column_names_from_order_arguments(args)
-        args.flat_map do |arg|
-          case arg
-          when Hash
-            # Tag.order(id: :desc)
-            arg.keys
-          when Array
-            # Tag.order([Arel.sql("field(id, ?)"), [1, 3, 2]])
-            arg.flatten
-          else
-            # Tag.order(:id)
-            arg
-          end
-        end
-      end
-
       def assert_mutability!
         raise ImmutableRelation if @loaded
         raise ImmutableRelation if defined?(@arel) && @arel
@@ -1146,6 +1119,12 @@ module ActiveRecord
           klass.send(:sanitize_sql_for_order, arg)
         end
         order_args.flatten!
+
+        @klass.enforce_raw_sql_whitelist(
+          order_args.flat_map { |a| a.is_a?(Hash) ? a.keys : a },
+          whitelist: AttributeMethods::ClassMethods::COLUMN_NAME_ORDER_WHITELIST
+        )
+
         validate_order_args(order_args)
 
         references = order_args.grep(String)
diff --git a/activerecord/lib/active_record/sanitization.rb b/activerecord/lib/active_record/sanitization.rb
index 4caf1145f0..232743d3b6 100644
--- a/activerecord/lib/active_record/sanitization.rb
+++ b/activerecord/lib/active_record/sanitization.rb
@@ -63,13 +63,17 @@ module ActiveRecord
         #   # => "id ASC"
         def sanitize_sql_for_order(condition) # :doc:
           if condition.is_a?(Array) && condition.first.to_s.include?("?")
+            enforce_raw_sql_whitelist([condition.first],
+              whitelist: AttributeMethods::ClassMethods::COLUMN_NAME_ORDER_WHITELIST
+            )
+
             # Ensure we aren't dealing with a subclass of String that might
             # override methods we use (eg. Arel::Nodes::SqlLiteral).
             if condition.first.kind_of?(String) && !condition.first.instance_of?(String)
               condition = [String.new(condition.first), *condition[1..-1]]
             end
 
-            sanitize_sql_array(condition)
+            Arel.sql(sanitize_sql_array(condition))
           else
             condition
           end
diff --git a/activerecord/test/cases/unsafe_raw_sql_test.rb b/activerecord/test/cases/unsafe_raw_sql_test.rb
index 18c6f4bae3..861df8f1da 100644
--- a/activerecord/test/cases/unsafe_raw_sql_test.rb
+++ b/activerecord/test/cases/unsafe_raw_sql_test.rb
@@ -138,6 +138,42 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
     assert_equal ids_depr, ids_disabled
   end
 
+  test "order: allows Arel.sql with binds" do
+    ids_expected = Post.order(Arel.sql('INSTR(title, "comments"), id')).pluck(:id)
+
+    ids_depr     = with_unsafe_raw_sql_deprecated { Post.order([Arel.sql("INSTR(title, ?), id"), "comments"]).pluck(:id) }
+    ids_disabled =  with_unsafe_raw_sql_disabled  { Post.order([Arel.sql("INSTR(title, ?), id"), "comments"]).pluck(:id) }
+
+    assert_equal ids_expected, ids_depr
+    assert_equal ids_expected, ids_disabled
+  end
+
+  test "order: disallows invalid bind statement" do
+    with_unsafe_raw_sql_disabled   do
+      assert_raises(ActiveRecord::UnknownAttributeReference) do
+        Post.order(["INSTR(title, ?), id", "comments"]).pluck(:id)
+      end
+    end
+  end
+
+  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)
+      end
+    end
+  end
+
+  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) }
+
+    assert_equal ids_expected, ids_depr
+    assert_equal ids_expected, ids_disabled
+  end
+
   test "order: logs deprecation warning for unrecognized column" do
     with_unsafe_raw_sql_deprecated do
       ActiveSupport::Deprecation.expects(:warn).with do |msg|