add config to check arguments to unsafe AR methods

6 files changed, 293 insertions, 21 deletions

diff --git a/activerecord/lib/active_record/attribute_methods.rb b/activerecord/lib/active_record/attribute_methods.rb
index 23d2aef214..fa0d79ba5f 100644
--- a/activerecord/lib/active_record/attribute_methods.rb
+++ b/activerecord/lib/active_record/attribute_methods.rb
@@ -167,6 +167,22 @@ module ActiveRecord
         end
       end
 
+      # Can the given name be treated as a column name? Returns true if name
+      # is attribute or attribute alias.
+      #
+      #   class Person < ActiveRecord::Base
+      #   end
+      #
+      #   Person.respond_to_attribute?(:name)
+      #   # => true
+      #
+      #   Person.respond_to_attribute?("foo")
+      #   # => false
+      def respond_to_attribute?(name)
+        name = name.to_s
+        attribute_names.include?(name) || attribute_aliases.include?(name)
+      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..b1e3f71dfe 100644
--- a/activerecord/lib/active_record/core.rb
+++ b/activerecord/lib/active_record/core.rb
@@ -76,6 +76,15 @@ 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
+      #   enabled    - Unsafe raw SQL can be passed to query methods.
+      #   deprecated - Warnings are logged when unsafe raw SQL is passed to
+      #                query methods.
+      #   disabled   - Unsafe raw SQL passed to query methods results in
+      #                ArguementError.
+      mattr_accessor :allow_unsafe_raw_sql, instance_writer: false, default: :enabled
+
       ##
       # :singleton-method:
       # Specify whether or not to use timestamps for migration versions
diff --git a/activerecord/lib/active_record/querying.rb b/activerecord/lib/active_record/querying.rb
index 3996d5661f..0233835bcf 100644
--- a/activerecord/lib/active_record/querying.rb
+++ b/activerecord/lib/active_record/querying.rb
@@ -2,18 +2,25 @@
 
 module ActiveRecord
   module Querying
-    delegate :find, :take, :take!, :first, :first!, :last, :last!, :exists?, :any?, :many?, :none?, :one?, to: :all
-    delegate :second, :second!, :third, :third!, :fourth, :fourth!, :fifth, :fifth!, :forty_two, :forty_two!, :third_to_last, :third_to_last!, :second_to_last, :second_to_last!, to: :all
+    delegate :find, :take, :take!, :first, :first!, :last, :last!, :exists?,
+             :any?, :many?, :none?, :one?, to: :all
+    delegate :second, :second!, :third, :third!, :fourth, :fourth!, :fifth,
+             :fifth!, :forty_two, :forty_two!, :third_to_last, :third_to_last!,
+             :second_to_last, :second_to_last!, to: :all
     delegate :first_or_create, :first_or_create!, :first_or_initialize, to: :all
-    delegate :find_or_create_by, :find_or_create_by!, :find_or_initialize_by, to: :all
+    delegate :find_or_create_by, :find_or_create_by!, :find_or_initialize_by,
+             to: :all
     delegate :find_by, :find_by!, to: :all
     delegate :destroy_all, :delete_all, :update_all, to: :all
     delegate :find_each, :find_in_batches, :in_batches, to: :all
-    delegate :select, :group, :order, :except, :reorder, :limit, :offset, :joins, :left_joins, :left_outer_joins, :or,
-             :where, :rewhere, :preload, :eager_load, :includes, :from, :lock, :readonly, :extending,
-             :having, :create_with, :distinct, :references, :none, :unscope, :merge, to: :all
+    delegate :select, :group, :order, :unsafe_raw_order, :except, :reorder,
+             :unsafe_raw_reorder, :limit, :offset, :joins, :left_joins,
+             :left_outer_joins, :or, :where, :rewhere, :preload, :eager_load,
+             :includes, :from, :lock, :readonly, :extending, :having,
+             :create_with, :distinct, :references, :none, :unscope, :merge,
+             to: :all
     delegate :count, :average, :minimum, :maximum, :sum, :calculate, to: :all
-    delegate :pluck, :ids, to: :all
+    delegate :pluck, :unsafe_raw_pluck, :ids, to: :all
 
     # Executes a custom SQL query against your database and returns all the results. The results will
     # be returned as an array with columns requested encapsulated as attributes of the model you call
diff --git a/activerecord/lib/active_record/relation/calculations.rb b/activerecord/lib/active_record/relation/calculations.rb
index 11256ab3d9..dea7542f03 100644
--- a/activerecord/lib/active_record/relation/calculations.rb
+++ b/activerecord/lib/active_record/relation/calculations.rb
@@ -175,21 +175,13 @@ module ActiveRecord
     # See also #ids.
     #
     def pluck(*column_names)
-      if loaded? && (column_names.map(&:to_s) - @klass.attribute_names - @klass.attribute_aliases.keys).empty?
-        return records.pluck(*column_names)
-      end
+      _pluck(column_names, @klass.allow_unsafe_raw_sql == :enabled)
+    end
 
-      if has_include?(column_names.first)
-        relation = apply_join_dependency
-        relation.pluck(*column_names)
-      else
-        relation = spawn
-        relation.select_values = column_names.map { |cn|
-          @klass.has_attribute?(cn) || @klass.attribute_alias?(cn) ? arel_attribute(cn) : cn
-        }
-        result = skip_query_cache_if_necessary { klass.connection.select_all(relation.arel, nil) }
-        result.cast_values(klass.attribute_types)
-      end
+    # Same as #pluck but allows raw SQL regardless of `allow_unsafe_raw_sql`
+    # config setting.
+    def unsafe_raw_pluck(*column_names)
+      _pluck(column_names, true)
     end
 
     # Pluck all the ID's for the relation using the table's primary key
@@ -202,6 +194,28 @@ module ActiveRecord
 
     private
 
+      def _pluck(column_names, unsafe_raw)
+        unrecognized = column_names.reject do |cn|
+          @klass.respond_to_attribute?(cn)
+        end
+
+        if loaded? && unrecognized.none?
+          records.pluck(*column_names)
+        elsif has_include?(column_names.first)
+          relation = apply_join_dependency
+          relation.pluck(*column_names)
+        elsif unsafe_raw || unrecognized.none?
+          relation = spawn
+          relation.select_values = column_names.map { |cn|
+            @klass.respond_to_attribute?(cn) ? arel_attribute(cn) : cn
+          }
+          result = skip_query_cache_if_necessary { klass.connection.select_all(relation.arel, nil) }
+          result.cast_values(klass.attribute_types)
+        else
+          raise ArgumentError, "Invalid column name: #{unrecognized}"
+        end
+      end
+
       def has_include?(column_name)
         eager_loading? || (includes_values.present? && column_name && column_name != :all)
       end
diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb
index 897ff5c8af..63b1d8e154 100644
--- a/activerecord/lib/active_record/relation/query_methods.rb
+++ b/activerecord/lib/active_record/relation/query_methods.rb
@@ -295,7 +295,22 @@ module ActiveRecord
       spawn.order!(*args)
     end
 
+    # Same as #order but allows raw SQL regardless of `allow_unsafe_raw_sql`
+    # config setting.
+    def unsafe_raw_order(*args) # :nodoc:
+      check_if_method_has_arguments!(:order, args)
+      spawn.unsafe_raw_order!(*args)
+    end
+
+    # Same as #order but operates on relation in-place instead of copying.
     def order!(*args) # :nodoc:
+      restrict_order_args(args) unless klass.allow_unsafe_raw_sql == :enabled
+      unsafe_raw_order!(*args)
+    end
+
+    # Same as #order! but allows raw SQL regardless of `allow_unsafe_raw_sql`
+    # config setting.
+    def unsafe_raw_order!(*args) # :nodoc:
       preprocess_order_args(args)
 
       self.order_values += args
@@ -316,7 +331,22 @@ module ActiveRecord
       spawn.reorder!(*args)
     end
 
+    # Same as #reorder but allows raw SQL regardless of `allow_unsafe_raw_sql`
+    # config setting.
+    def unsafe_raw_reorder(*args) # :nodoc:
+      check_if_method_has_arguments!(:reorder, args)
+      spawn.unsafe_raw_reorder!(*args)
+    end
+
+    # Same as #reorder but operates on relation in-place instead of copying.
     def reorder!(*args) # :nodoc:
+      restrict_order_args(args) unless klass.allow_unsafe_raw_sql == :enabled
+      unsafe_raw_reorder!
+    end
+
+    # Same as #reorder! but allows raw SQL regardless of `allow_unsafe_raw_sql`
+    # config setting.
+    def unsafe_raw_reorder!(*args) # :nodoc:
       preprocess_order_args(args)
 
       self.reordering_value = true
@@ -1139,6 +1169,25 @@ module ActiveRecord
         end.flatten!
       end
 
+      # Only allow column names and directions as arguments to #order and
+      # #reorder. Other arguments will cause an ArugmentError to be raised.
+      def restrict_order_args(args)
+        args = args.dup
+        orderings = args.extract_options!
+        columns = args | orderings.keys
+
+        unrecognized = columns.reject { |c| klass.respond_to_attribute?(c) }
+        if unrecognized.any?
+          raise ArgumentError, "Invalid order column: #{unrecognized}"
+        end
+
+        # TODO: find a better list of modifiers.
+        unrecognized = orderings.values.reject { |d| VALID_DIRECTIONS.include?(d.to_s) }
+        if unrecognized.any?
+          raise ArgumentError, "Invalid order direction: #{unrecognized}"
+        end
+      end
+
       # Checks to make sure that the arguments are not blank. Note that if some
       # blank-like object were initially passed into the query method, then this
       # method will not raise an error.
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..d05f0f12d9
--- /dev/null
+++ b/activerecord/test/cases/unsafe_raw_sql_test.rb
@@ -0,0 +1,177 @@
+# 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
+    enabled, disabled = with_configs(:enabled, :disabled) do
+      Post.order("title").pluck(:id)
+    end
+
+    assert_equal enabled, disabled
+    assert_equal disabled, Post.unsafe_raw_order("title").pluck(:id)
+  end
+
+  test "order: allows symbol column name" do
+    enabled, disabled = with_configs(:enabled, :disabled) do
+      Post.order(:title).pluck(:id)
+    end
+
+    assert_equal enabled, disabled
+    assert_equal disabled, Post.unsafe_raw_order(:title).pluck(:id)
+  end
+
+  test "order: allows downcase symbol direction" do
+    enabled, disabled = with_configs(:enabled, :disabled) do
+      Post.order(title: :asc).pluck(:id)
+    end
+
+    assert_equal enabled, disabled
+    assert_equal disabled, Post.unsafe_raw_order(title: :asc).pluck(:id)
+  end
+
+  test "order: allows upcase symbol direction" do
+    enabled, disabled = with_configs(:enabled, :disabled) do
+      Post.order(title: :ASC).pluck(:id)
+    end
+
+    assert_equal enabled, disabled
+    assert_equal disabled, Post.unsafe_raw_order(title: :ASC).pluck(:id)
+  end
+
+  test "order: allows string direction" do
+    enabled, disabled = with_configs(:enabled, :disabled) do
+      Post.order(title: "asc").pluck(:id)
+    end
+
+    assert_equal enabled, disabled
+    assert_equal disabled, Post.unsafe_raw_order(title: "asc").pluck(:id)
+  end
+
+  test "order: allows multiple columns" do
+    enabled, disabled = with_configs(:enabled, :disabled) do
+      Post.order(:author_id, :title).pluck(:id)
+    end
+
+    assert_equal enabled, disabled
+    assert_equal disabled, Post.unsafe_raw_order(:author_id, :title).pluck(:id)
+  end
+
+  test "order: allows mixed" do
+    enabled, disabled = with_configs(:enabled, :disabled) do
+      Post.order(:author_id, title: :asc).pluck(:id)
+    end
+
+    assert_equal enabled, disabled
+    assert_equal disabled, Post.unsafe_raw_order(:author_id, title: :asc).pluck(:id)
+  end
+
+  test "order: disallows invalid column name" do
+    with_config(:disabled) do
+      assert_raises(ArgumentError) do
+        Post.order("title asc").pluck(:id)
+      end
+    end
+  end
+
+  test "order: disallows invalid direction" do
+    with_config(:disabled) do
+      assert_raises(ArgumentError) do
+        Post.order(title: :foo).pluck(:id)
+      end
+    end
+  end
+
+  test "order: disallows invalid column with direction" do
+    with_config(:disabled) do
+      assert_raises(ArgumentError) do
+        Post.order(foo: :asc).pluck(:id)
+      end
+    end
+  end
+
+  test "pluck: allows string column name" do
+    enabled, disabled = with_configs(:enabled, :disabled) do
+      Post.pluck("title")
+    end
+
+    assert_equal enabled, disabled
+    assert_equal disabled, Post.unsafe_raw_pluck("title")
+  end
+
+  test "pluck: allows symbol column name" do
+    enabled, disabled = with_configs(:enabled, :disabled) do
+      Post.pluck(:title)
+    end
+
+    assert_equal enabled, disabled
+    assert_equal disabled, Post.unsafe_raw_pluck(:title)
+  end
+
+  test "pluck: allows multiple column names" do
+    enabled, disabled = with_configs(:enabled, :disabled) do
+      Post.pluck(:title, :id)
+    end
+
+    assert_equal enabled, disabled
+    assert_equal disabled, Post.unsafe_raw_pluck(:title, :id)
+  end
+
+  test "pluck: allows column names with includes" do
+    enabled, disabled = with_configs(:enabled, :disabled) do
+      Post.includes(:comments).pluck(:title, :id)
+    end
+
+    assert_equal enabled, disabled
+    assert_equal disabled, Post.includes(:comments).unsafe_raw_pluck(:title, :id)
+  end
+
+  test "pluck: allows auto-generated attributes" do
+    enabled, disabled = with_configs(:enabled, :disabled) do
+      Post.pluck(:tags_count)
+    end
+
+    assert_equal enabled, disabled
+    assert_equal disabled, Post.unsafe_raw_pluck(:tags_count)
+  end
+
+  test "pluck: disallows invalid column name" do
+    with_config(:disabled) do
+      assert_raises(ArgumentError) do
+        Post.pluck("length(title)")
+      end
+    end
+  end
+
+  test "pluck: disallows invalid column name amongst valid names" do
+    with_config(:disabled) do
+      assert_raises(ArgumentError) do
+        Post.pluck(:title, "length(title)")
+      end
+    end
+  end
+
+  test "pluck: disallows invalid column names with includes" do
+    with_config(:disabled) do
+      assert_raises(ArgumentError) do
+        Post.includes(:comments).pluck(:title, "length(title)")
+      end
+    end
+  end
+
+  def with_configs(*new_values, &blk)
+    new_values.map { |nv| with_config(nv, &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