Merge pull request #27947 from mastahyeti/unsafe_raw_sql

Disallow raw SQL in dangerous AR methods

13 files changed, 462 insertions, 33 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 81ff2923ce..57ec37c75b 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,33 @@
+*   Require raw SQL fragments to be explicitly marked when used in
+    relation query methods.
+
+    Before:
+    ```
+    Article.order("LENGTH(title)")
+    ```
+
+    After:
+    ```
+    Article.order(Arel.sql("LENGTH(title)"))
+    ```
+
+    This prevents SQL injection if applications use the [strongly
+    discouraged] form `Article.order(params[:my_order])`, under the
+    mistaken belief that only column names will be accepted.
+
+    Raw SQL strings will now cause a deprecation warning, which will
+    become an UnknownAttributeReference error in Rails 6.0. Applications
+    can opt in to the future behavior by setting `allow_unsafe_raw_sql`
+    to `:disabled`.
+
+    Common and judged-safe string values (such as simple column
+    references) are unaffected:
+    ```
+    Article.order("title DESC")
+    ```
+
+    *Ben Toews*
+
 *   `update_all` will now pass its values to `Type#cast` before passing them to
     `Type#serialize`. This means that `update_all(foo: 'true')` will properly
     persist a boolean.
diff --git a/activerecord/lib/active_record/attribute_methods.rb b/activerecord/lib/active_record/attribute_methods.rb
index 23d2aef214..64f81ca582 100644
--- a/activerecord/lib/active_record/attribute_methods.rb
+++ b/activerecord/lib/active_record/attribute_methods.rb
@@ -167,6 +167,46 @@ module ActiveRecord
         end
       end
 
+      # Regexp whitelist. Matches the following:
+      #   "#{table_name}.#{column_name}"
+      #   "#{column_name}"
+      COLUMN_NAME_WHITELIST = /\A(?:\w+\.)?\w+\z/i
+
+      # Regexp whitelist. Matches the following:
+      #   "#{table_name}.#{column_name}"
+      #   "#{table_name}.#{column_name} #{direction}"
+      #   "#{column_name}"
+      #   "#{column_name} #{direction}"
+      COLUMN_NAME_ORDER_WHITELIST = /\A(?:\w+\.)?\w+(?:\s+asc|\s+desc)?\z/i
+
+      def enforce_raw_sql_whitelist(args, whitelist: COLUMN_NAME_WHITELIST) # :nodoc:
+        unexpected = args.reject do |arg|
+          arg.kind_of?(Arel::Node) ||
+            arg.is_a?(Arel::Nodes::SqlLiteral) ||
+            arg.is_a?(Arel::Attributes::Attribute) ||
+            arg.to_s.split(/\s*,\s*/).all? { |part| whitelist.match?(part) }
+        end
+
+        return if unexpected.none?
+
+        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.0. 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/core.rb b/activerecord/lib/active_record/core.rb
index 0f7a503c90..b97b14644e 100644
--- a/activerecord/lib/active_record/core.rb
+++ b/activerecord/lib/active_record/core.rb
@@ -76,6 +76,14 @@ module ActiveRecord
       # scope being ignored is error-worthy, rather than a warning.
       mattr_accessor :error_on_ignored_order, instance_writer: false, default: false
 
+      # :singleton-method:
+      # Specify the behavior for unsafe raw query methods. Values are as follows
+      #   deprecated - Warnings are logged when unsafe raw SQL is passed to
+      #                query methods.
+      #   disabled   - Unsafe raw SQL passed to query methods results in
+      #                UnknownAttributeReference exception.
+      mattr_accessor :allow_unsafe_raw_sql, instance_writer: false, default: :deprecated
+
       ##
       # :singleton-method:
       # Specify whether or not to use timestamps for migration versions
diff --git a/activerecord/lib/active_record/errors.rb b/activerecord/lib/active_record/errors.rb
index f77cd23e22..7382879fce 100644
--- a/activerecord/lib/active_record/errors.rb
+++ b/activerecord/lib/active_record/errors.rb
@@ -342,4 +342,29 @@ module ActiveRecord
   # StatementTimeout will be raised when statement timeout exceeded.
   class StatementTimeout < StatementInvalid
   end
+
+  # UnknownAttributeReference is raised when an unknown and potentially unsafe
+  # value is passed to a query method when allow_unsafe_raw_sql is set to
+  # :disabled. For example, passing a non column name value to a relation's
+  # #order method might cause this exception.
+  #
+  # When working around this exception, caution should be taken to avoid SQL
+  # injection vulnerabilities when passing user-provided values to query
+  # methods. Known-safe values can be passed to query methods by wrapping them
+  # in Arel.sql.
+  #
+  # For example, with allow_unsafe_raw_sql set to :disabled, the following
+  # code would raise this exception:
+  #
+  #   Post.order("length(title)").first
+  #
+  # The desired result can be accomplished by wrapping the known-safe string
+  # in Arel.sql:
+  #
+  #   Post.order(Arel.sql("length(title)")).first
+  #
+  # Again, such a workaround should *not* be used when passing user-provided
+  # values, such as request parameters or model attributes to query methods.
+  class UnknownAttributeReference < ActiveRecordError
+  end
 end
diff --git a/activerecord/lib/active_record/relation/calculations.rb b/activerecord/lib/active_record/relation/calculations.rb
index 11256ab3d9..d49472fc70 100644
--- a/activerecord/lib/active_record/relation/calculations.rb
+++ b/activerecord/lib/active_record/relation/calculations.rb
@@ -183,6 +183,7 @@ module ActiveRecord
         relation = apply_join_dependency
         relation.pluck(*column_names)
       else
+        enforce_raw_sql_whitelist(column_names)
         relation = spawn
         relation.select_values = column_names.map { |cn|
           @klass.has_attribute?(cn) || @klass.attribute_alias?(cn) ? arel_attribute(cn) : cn
diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb
index 1219737e78..749223422f 100644
--- a/activerecord/lib/active_record/relation/query_methods.rb
+++ b/activerecord/lib/active_record/relation/query_methods.rb
@@ -295,6 +295,7 @@ module ActiveRecord
       spawn.order!(*args)
     end
 
+    # Same as #order but operates on relation in-place instead of copying.
     def order!(*args) # :nodoc:
       preprocess_order_args(args)
 
@@ -316,6 +317,7 @@ module ActiveRecord
       spawn.reorder!(*args)
     end
 
+    # Same as #reorder but operates on relation in-place instead of copying.
     def reorder!(*args) # :nodoc:
       preprocess_order_args(args)
 
@@ -1076,7 +1078,7 @@ module ActiveRecord
             end
             o.split(",").map! do |s|
               s.strip!
-              s.gsub!(/\sasc\Z/i, " DESC") || s.gsub!(/\sdesc\Z/i, " ASC") || s.concat(" DESC")
+              s.gsub!(/\sasc\Z/i, " DESC") || s.gsub!(/\sdesc\Z/i, " ASC") || (s << " DESC")
             end
           else
             o
@@ -1085,6 +1087,10 @@ module ActiveRecord
       end
 
       def does_not_support_reverse?(order)
+        # Account for String subclasses like Arel::Nodes::SqlLiteral that
+        # override methods like #count.
+        order = String.new(order) unless order.instance_of?(String)
+
         # Uses SQL function with multiple arguments.
         (order.include?(",") && order.split(",").find { |section| section.count("(") != section.count(")") }) ||
           # Uses "nulls first" like construction.
@@ -1118,6 +1124,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 90cc3373fb..21f8bc7cb2 100644
--- a/activerecord/lib/active_record/sanitization.rb
+++ b/activerecord/lib/active_record/sanitization.rb
@@ -63,7 +63,17 @@ module ActiveRecord
         #   # => "id ASC"
         def sanitize_sql_for_order(condition) # :doc:
           if condition.is_a?(Array) && condition.first.to_s.include?("?")
-            sanitize_sql_array(condition)
+            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
+
+            Arel.sql(sanitize_sql_array(condition))
           else
             condition
           end
diff --git a/activerecord/test/cases/associations/eager_test.rb b/activerecord/test/cases/associations/eager_test.rb
index 9afe6a893c..9a042c74db 100644
--- a/activerecord/test/cases/associations/eager_test.rb
+++ b/activerecord/test/cases/associations/eager_test.rb
@@ -427,7 +427,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(quoted_posts_id)
+      Comment.includes(:post).references(:posts).order(Arel.sql(quoted_posts_id))
     end
   end
 
@@ -874,14 +874,14 @@ class EagerAssociationTest < ActiveRecord::TestCase
       posts(:thinking, :sti_comments),
       Post.all.merge!(
         includes: [:author, :comments], where: { "authors.name" => "David" },
-        order: "UPPER(posts.title)", limit: 2, offset: 1
+        order: Arel.sql("UPPER(posts.title)"), limit: 2, offset: 1
       ).to_a
     )
     assert_equal(
       posts(:sti_post_and_comments, :sti_comments),
       Post.all.merge!(
         includes: [:author, :comments], where: { "authors.name" => "David" },
-        order: "UPPER(posts.title) DESC", limit: 2, offset: 1
+        order: Arel.sql("UPPER(posts.title) DESC"), limit: 2, offset: 1
       ).to_a
     )
   end
@@ -891,14 +891,14 @@ class EagerAssociationTest < ActiveRecord::TestCase
       posts(:thinking, :sti_comments),
       Post.all.merge!(
         includes: [:author, :comments], where: { "authors.name" => "David" },
-        order: ["UPPER(posts.title)", "posts.id"], limit: 2, offset: 1
+        order: [Arel.sql("UPPER(posts.title)"), "posts.id"], limit: 2, offset: 1
       ).to_a
     )
     assert_equal(
       posts(:sti_post_and_comments, :sti_comments),
       Post.all.merge!(
         includes: [:author, :comments], where: { "authors.name" => "David" },
-        order: ["UPPER(posts.title) DESC", "posts.id"], limit: 2, offset: 1
+        order: [Arel.sql("UPPER(posts.title) DESC"), "posts.id"], limit: 2, offset: 1
       ).to_a
     )
   end
diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb
index 66bc14b5ab..55b50e4f84 100644
--- a/activerecord/test/cases/calculations_test.rb
+++ b/activerecord/test/cases/calculations_test.rb
@@ -663,14 +663,14 @@ class CalculationsTest < ActiveRecord::TestCase
   end
 
   def test_pluck_with_selection_clause
-    assert_equal [50, 53, 55, 60], Account.pluck("DISTINCT credit_limit").sort
-    assert_equal [50, 53, 55, 60], Account.pluck("DISTINCT accounts.credit_limit").sort
-    assert_equal [50, 53, 55, 60], Account.pluck("DISTINCT(credit_limit)").sort
+    assert_equal [50, 53, 55, 60], Account.pluck(Arel.sql("DISTINCT credit_limit")).sort
+    assert_equal [50, 53, 55, 60], Account.pluck(Arel.sql("DISTINCT accounts.credit_limit")).sort
+    assert_equal [50, 53, 55, 60], Account.pluck(Arel.sql("DISTINCT(credit_limit)")).sort
 
     # MySQL returns "SUM(DISTINCT(credit_limit))" as the column name unless
     # an alias is provided.  Without the alias, the column cannot be found
     # and properly typecast.
-    assert_equal [50 + 53 + 55 + 60], Account.pluck("SUM(DISTINCT(credit_limit)) as credit_limit")
+    assert_equal [50 + 53 + 55 + 60], Account.pluck(Arel.sql("SUM(DISTINCT(credit_limit)) as credit_limit"))
   end
 
   def test_plucks_with_ids
@@ -772,7 +772,7 @@ class CalculationsTest < ActiveRecord::TestCase
     companies = Company.order(:name).limit(3).load
 
     assert_queries 1 do
-      assert_equal ["37signals", "Apex", "Ex Nihilo"], companies.pluck("DISTINCT name")
+      assert_equal ["37signals", "Apex", "Ex Nihilo"], companies.pluck(Arel.sql("DISTINCT name"))
     end
   end
 
diff --git a/activerecord/test/cases/finder_test.rb b/activerecord/test/cases/finder_test.rb
index d8bc917e7f..1268949ba9 100644
--- a/activerecord/test/cases/finder_test.rb
+++ b/activerecord/test/cases/finder_test.rb
@@ -239,7 +239,7 @@ class FinderTest < ActiveRecord::TestCase
 
   # Ensure +exists?+ runs without an error by excluding order value.
   def test_exists_with_order
-    assert_equal true, Topic.order("invalid sql here").exists?
+    assert_equal true, Topic.order(Arel.sql("invalid sql here")).exists?
   end
 
   def test_exists_with_joins
@@ -652,7 +652,7 @@ class FinderTest < ActiveRecord::TestCase
 
   def test_last_with_irreversible_order
     assert_raises(ActiveRecord::IrreversibleOrderError) do
-      Topic.order("coalesce(author_name, title)").last
+      Topic.order(Arel.sql("coalesce(author_name, title)")).last
     end
   end
 
diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb
index eec43ef79e..50ad1d5b26 100644
--- a/activerecord/test/cases/relations_test.rb
+++ b/activerecord/test/cases/relations_test.rb
@@ -250,7 +250,7 @@ class RelationTest < ActiveRecord::TestCase
   end
 
   def test_reverse_order_with_function
-    topics = Topic.order("length(title)").reverse_order
+    topics = Topic.order(Arel.sql("length(title)")).reverse_order
     assert_equal topics(:second).title, topics.first.title
   end
 
@@ -260,24 +260,24 @@ class RelationTest < ActiveRecord::TestCase
   end
 
   def test_reverse_order_with_function_other_predicates
-    topics = Topic.order("author_name, length(title), id").reverse_order
+    topics = Topic.order(Arel.sql("author_name, length(title), id")).reverse_order
     assert_equal topics(:second).title, topics.first.title
-    topics = Topic.order("length(author_name), id, length(title)").reverse_order
+    topics = Topic.order(Arel.sql("length(author_name), id, length(title)")).reverse_order
     assert_equal topics(:fifth).title, topics.first.title
   end
 
   def test_reverse_order_with_multiargument_function
     assert_raises(ActiveRecord::IrreversibleOrderError) do
-      Topic.order("concat(author_name, title)").reverse_order
+      Topic.order(Arel.sql("concat(author_name, title)")).reverse_order
     end
     assert_raises(ActiveRecord::IrreversibleOrderError) do
-      Topic.order("concat(lower(author_name), title)").reverse_order
+      Topic.order(Arel.sql("concat(lower(author_name), title)")).reverse_order
     end
     assert_raises(ActiveRecord::IrreversibleOrderError) do
-      Topic.order("concat(author_name, lower(title))").reverse_order
+      Topic.order(Arel.sql("concat(author_name, lower(title))")).reverse_order
     end
     assert_raises(ActiveRecord::IrreversibleOrderError) do
-      Topic.order("concat(lower(author_name), title, length(title)").reverse_order
+      Topic.order(Arel.sql("concat(lower(author_name), title, length(title)")).reverse_order
     end
   end
 
@@ -289,10 +289,10 @@ class RelationTest < ActiveRecord::TestCase
 
   def test_reverse_order_with_nulls_first_or_last
     assert_raises(ActiveRecord::IrreversibleOrderError) do
-      Topic.order("title NULLS FIRST").reverse_order
+      Topic.order(Arel.sql("title NULLS FIRST")).reverse_order
     end
     assert_raises(ActiveRecord::IrreversibleOrderError) do
-      Topic.order("title nulls last").reverse_order
+      Topic.order(Arel.sql("title nulls last")).reverse_order
     end
   end
 
@@ -385,29 +385,29 @@ class RelationTest < ActiveRecord::TestCase
 
   def test_finding_with_cross_table_order_and_limit
     tags = Tag.includes(:taggings).
-              order("tags.name asc", "taggings.taggable_id asc", "REPLACE('abc', taggings.taggable_type, taggings.taggable_type)").
+              order("tags.name asc", "taggings.taggable_id asc", Arel.sql("REPLACE('abc', taggings.taggable_type, taggings.taggable_type)")).
               limit(1).to_a
     assert_equal 1, tags.length
   end
 
   def test_finding_with_complex_order_and_limit
-    tags = Tag.includes(:taggings).references(:taggings).order("REPLACE('abc', taggings.taggable_type, taggings.taggable_type)").limit(1).to_a
+    tags = Tag.includes(:taggings).references(:taggings).order(Arel.sql("REPLACE('abc', taggings.taggable_type, taggings.taggable_type)")).limit(1).to_a
     assert_equal 1, tags.length
   end
 
   def test_finding_with_complex_order
-    tags = Tag.includes(:taggings).references(:taggings).order("REPLACE('abc', taggings.taggable_type, taggings.taggable_type)").to_a
+    tags = Tag.includes(:taggings).references(:taggings).order(Arel.sql("REPLACE('abc', taggings.taggable_type, taggings.taggable_type)")).to_a
     assert_equal 3, tags.length
   end
 
   def test_finding_with_sanitized_order
-    query = Tag.order(["field(id, ?)", [1, 3, 2]]).to_sql
+    query = Tag.order([Arel.sql("field(id, ?)"), [1, 3, 2]]).to_sql
     assert_match(/field\(id, 1,3,2\)/, query)
 
-    query = Tag.order(["field(id, ?)", []]).to_sql
+    query = Tag.order([Arel.sql("field(id, ?)"), []]).to_sql
     assert_match(/field\(id, NULL\)/, query)
 
-    query = Tag.order(["field(id, ?)", nil]).to_sql
+    query = Tag.order([Arel.sql("field(id, ?)"), nil]).to_sql
     assert_match(/field\(id, NULL\)/, query)
   end
 
@@ -1579,7 +1579,7 @@ class RelationTest < ActiveRecord::TestCase
     scope = Post.order("comments.body")
     assert_equal ["comments"], scope.references_values
 
-    scope = Post.order("#{Comment.quoted_table_name}.#{Comment.quoted_primary_key}")
+    scope = Post.order(Arel.sql("#{Comment.quoted_table_name}.#{Comment.quoted_primary_key}"))
     if current_adapter?(:OracleAdapter)
       assert_equal ["COMMENTS"], scope.references_values
     else
@@ -1596,7 +1596,7 @@ class RelationTest < ActiveRecord::TestCase
     scope = Post.order("comments.body asc")
     assert_equal ["comments"], scope.references_values
 
-    scope = Post.order("foo(comments.body)")
+    scope = Post.order(Arel.sql("foo(comments.body)"))
     assert_equal [], scope.references_values
   end
 
@@ -1604,7 +1604,7 @@ class RelationTest < ActiveRecord::TestCase
     scope = Post.reorder("comments.body")
     assert_equal %w(comments), scope.references_values
 
-    scope = Post.reorder("#{Comment.quoted_table_name}.#{Comment.quoted_primary_key}")
+    scope = Post.reorder(Arel.sql("#{Comment.quoted_table_name}.#{Comment.quoted_primary_key}"))
     if current_adapter?(:OracleAdapter)
       assert_equal ["COMMENTS"], scope.references_values
     else
@@ -1621,7 +1621,7 @@ class RelationTest < ActiveRecord::TestCase
     scope = Post.reorder("comments.body asc")
     assert_equal %w(comments), scope.references_values
 
-    scope = Post.reorder("foo(comments.body)")
+    scope = Post.reorder(Arel.sql("foo(comments.body)"))
     assert_equal [], scope.references_values
   end
 
diff --git a/activerecord/test/cases/unsafe_raw_sql_test.rb b/activerecord/test/cases/unsafe_raw_sql_test.rb
new file mode 100644
index 0000000000..72d4997d0b
--- /dev/null
+++ b/activerecord/test/cases/unsafe_raw_sql_test.rb
@@ -0,0 +1,299 @@
+# frozen_string_literal: true
+
+require "cases/helper"
+require "models/post"
+require "models/comment"
+
+class UnsafeRawSqlTest < ActiveRecord::TestCase
+  fixtures :posts, :comments
+
+  test "order: allows string column name" do
+    ids_expected = Post.order(Arel.sql("title")).pluck(:id)
+
+    ids_depr     = with_unsafe_raw_sql_deprecated { Post.order("title").pluck(:id) }
+    ids_disabled = with_unsafe_raw_sql_disabled   { Post.order("title").pluck(:id) }
+
+    assert_equal ids_expected, ids_depr
+    assert_equal ids_expected, ids_disabled
+  end
+
+  test "order: allows symbol column name" do
+    ids_expected = Post.order(Arel.sql("title")).pluck(:id)
+
+    ids_depr     = with_unsafe_raw_sql_deprecated { Post.order(:title).pluck(:id) }
+    ids_disabled = with_unsafe_raw_sql_disabled   { Post.order(:title).pluck(:id) }
+
+    assert_equal ids_expected, ids_depr
+    assert_equal ids_expected, ids_disabled
+  end
+
+  test "order: allows downcase symbol direction" do
+    ids_expected = Post.order(Arel.sql("title") => Arel.sql("asc")).pluck(:id)
+
+    ids_depr     = with_unsafe_raw_sql_deprecated { Post.order(title: :asc).pluck(:id) }
+    ids_disabled = with_unsafe_raw_sql_disabled   { Post.order(title: :asc).pluck(:id) }
+
+    assert_equal ids_expected, ids_depr
+    assert_equal ids_expected, ids_disabled
+  end
+
+  test "order: allows upcase symbol direction" do
+    ids_expected = Post.order(Arel.sql("title") => Arel.sql("ASC")).pluck(:id)
+
+    ids_depr     = with_unsafe_raw_sql_deprecated { Post.order(title: :ASC).pluck(:id) }
+    ids_disabled = with_unsafe_raw_sql_disabled   { Post.order(title: :ASC).pluck(:id) }
+
+    assert_equal ids_expected, ids_depr
+    assert_equal ids_expected, ids_disabled
+  end
+
+  test "order: allows string direction" do
+    ids_expected = Post.order(Arel.sql("title") => Arel.sql("asc")).pluck(:id)
+
+    ids_depr     = with_unsafe_raw_sql_deprecated { Post.order(title: "asc").pluck(:id) }
+    ids_disabled = with_unsafe_raw_sql_disabled   { Post.order(title: "asc").pluck(:id) }
+
+    assert_equal ids_expected, ids_depr
+    assert_equal ids_expected, ids_disabled
+  end
+
+  test "order: allows multiple columns" do
+    ids_expected = Post.order(Arel.sql("author_id"), Arel.sql("title")).pluck(:id)
+
+    ids_depr     = with_unsafe_raw_sql_deprecated { Post.order(:author_id, :title).pluck(:id) }
+    ids_disabled = with_unsafe_raw_sql_disabled   { Post.order(:author_id, :title).pluck(:id) }
+
+    assert_equal ids_expected, ids_depr
+    assert_equal ids_expected, ids_disabled
+  end
+
+  test "order: allows mixed" do
+    ids_expected = Post.order(Arel.sql("author_id"), Arel.sql("title") => Arel.sql("asc")).pluck(:id)
+
+    ids_depr     = with_unsafe_raw_sql_deprecated { Post.order(:author_id, title: :asc).pluck(:id) }
+    ids_disabled = with_unsafe_raw_sql_disabled   { Post.order(:author_id, title: :asc).pluck(:id) }
+
+    assert_equal ids_expected, ids_depr
+    assert_equal ids_expected, ids_disabled
+  end
+
+  test "order: allows table and column name" do
+    ids_expected = Post.order(Arel.sql("title")).pluck(:id)
+
+    ids_depr     = with_unsafe_raw_sql_deprecated { Post.order("posts.title").pluck(:id) }
+    ids_disabled = with_unsafe_raw_sql_disabled   { Post.order("posts.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)
+
+    ids_depr     = with_unsafe_raw_sql_deprecated { Post.order("title desc").pluck(:id) }
+    ids_disabled = with_unsafe_raw_sql_disabled   { Post.order("title desc").pluck(:id) }
+
+    assert_equal ids_expected, ids_depr
+    assert_equal ids_expected, ids_disabled
+  end
+
+  test "order: allows table name, column name and direction in string" do
+    ids_expected = Post.order(Arel.sql("title desc")).pluck(:id)
+
+    ids_depr     = with_unsafe_raw_sql_deprecated { Post.order("posts.title desc").pluck(:id) }
+    ids_disabled = with_unsafe_raw_sql_disabled   { Post.order("posts.title desc").pluck(:id) }
+
+    assert_equal ids_expected, ids_depr
+    assert_equal ids_expected, ids_disabled
+  end
+
+  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)
+      end
+    end
+  end
+
+  test "order: disallows invalid direction" do
+    with_unsafe_raw_sql_disabled do
+      assert_raises(ArgumentError) do
+        Post.order(title: :foo).pluck(:id)
+      end
+    end
+  end
+
+  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)
+      end
+    end
+  end
+
+  test "order: always allows Arel" do
+    ids_depr     = with_unsafe_raw_sql_deprecated { Post.order(Arel.sql("length(title)")).pluck(:title) }
+    ids_disabled = with_unsafe_raw_sql_disabled   { Post.order(Arel.sql("length(title)")).pluck(:title) }
+
+    assert_equal ids_depr, ids_disabled
+  end
+
+  test "order: allows Arel.sql with binds" do
+    ids_expected = Post.order(Arel.sql("REPLACE(title, 'misc', 'zzzz'), id")).pluck(:id)
+
+    ids_depr     = with_unsafe_raw_sql_deprecated { Post.order([Arel.sql("REPLACE(title, ?, ?), id"), "misc", "zzzz"]).pluck(:id) }
+    ids_disabled = with_unsafe_raw_sql_disabled   { Post.order([Arel.sql("REPLACE(title, ?, ?), id"), "misc", "zzzz"]).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(["REPLACE(title, ?, ?), id", "misc", "zzzz"]).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
+      assert_deprecated(/Dangerous query method/) do
+        Post.order("length(title)")
+      end
+    end
+  end
+
+  test "pluck: allows string column name" do
+    titles_expected = Post.pluck(Arel.sql("title"))
+
+    titles_depr     = with_unsafe_raw_sql_deprecated { Post.pluck("title") }
+    titles_disabled = with_unsafe_raw_sql_disabled   { Post.pluck("title") }
+
+    assert_equal titles_expected, titles_depr
+    assert_equal titles_expected, titles_disabled
+  end
+
+  test "pluck: allows symbol column name" do
+    titles_expected = Post.pluck(Arel.sql("title"))
+
+    titles_depr     = with_unsafe_raw_sql_deprecated { Post.pluck(:title) }
+    titles_disabled = with_unsafe_raw_sql_disabled   { Post.pluck(:title) }
+
+    assert_equal titles_expected, titles_depr
+    assert_equal titles_expected, titles_disabled
+  end
+
+  test "pluck: allows multiple column names" do
+    values_expected = Post.pluck(Arel.sql("title"), Arel.sql("id"))
+
+    values_depr     = with_unsafe_raw_sql_deprecated { Post.pluck(:title, :id) }
+    values_disabled = with_unsafe_raw_sql_disabled   { Post.pluck(:title, :id) }
+
+    assert_equal values_expected, values_depr
+    assert_equal values_expected, values_disabled
+  end
+
+  test "pluck: allows column names with includes" do
+    values_expected = Post.includes(:comments).pluck(Arel.sql("title"), Arel.sql("id"))
+
+    values_depr     = with_unsafe_raw_sql_deprecated { Post.includes(:comments).pluck(:title, :id) }
+    values_disabled = with_unsafe_raw_sql_disabled   { Post.includes(:comments).pluck(:title, :id) }
+
+    assert_equal values_expected, values_depr
+    assert_equal values_expected, values_disabled
+  end
+
+  test "pluck: allows auto-generated attributes" do
+    values_expected = Post.pluck(Arel.sql("tags_count"))
+
+    values_depr     = with_unsafe_raw_sql_deprecated { Post.pluck(:tags_count) }
+    values_disabled = with_unsafe_raw_sql_disabled   { Post.pluck(:tags_count) }
+
+    assert_equal values_expected, values_depr
+    assert_equal values_expected, values_disabled
+  end
+
+  test "pluck: allows table and column names" do
+    titles_expected = Post.pluck(Arel.sql("title"))
+
+    titles_depr     = with_unsafe_raw_sql_deprecated { Post.pluck("posts.title") }
+    titles_disabled = with_unsafe_raw_sql_disabled   { Post.pluck("posts.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
+        Post.pluck("length(title)")
+      end
+    end
+  end
+
+  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)")
+      end
+    end
+  end
+
+  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)")
+      end
+    end
+  end
+
+  test "pluck: always allows Arel" do
+    values_depr     = with_unsafe_raw_sql_deprecated { Post.includes(:comments).pluck(:title, Arel.sql("length(title)")) }
+    values_disabled = with_unsafe_raw_sql_disabled   { Post.includes(:comments).pluck(:title, Arel.sql("length(title)")) }
+
+    assert_equal values_depr, values_disabled
+  end
+
+  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)")
+      end
+    end
+  end
+
+  def with_unsafe_raw_sql_disabled(&blk)
+    with_config(:disabled, &blk)
+  end
+
+  def with_unsafe_raw_sql_deprecated(&blk)
+    with_config(:deprecated, &blk)
+  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
+end
diff --git a/activerecord/test/models/post.rb b/activerecord/test/models/post.rb
index 7f064bf3dd..780a2c17f5 100644
--- a/activerecord/test/models/post.rb
+++ b/activerecord/test/models/post.rb
@@ -319,5 +319,9 @@ class FakeKlass
     def arel_attribute(name, table)
       table[name]
     end
+
+    def enforce_raw_sql_whitelist(*args)
+      # noop
+    end
   end
 end