allow Arel.sql() for pluck

5 files changed, 90 insertions, 68 deletions

diff --git a/activerecord/lib/active_record/attribute_methods.rb b/activerecord/lib/active_record/attribute_methods.rb
index fa0d79ba5f..b3d3c0559f 100644
--- a/activerecord/lib/active_record/attribute_methods.rb
+++ b/activerecord/lib/active_record/attribute_methods.rb
@@ -167,6 +167,30 @@ module ActiveRecord
         end
       end
 
+      def enforce_raw_sql_whitelist(args, whitelist: attribute_names_and_aliases) # :nodoc:
+        return if allow_unsafe_raw_sql == :enabled
+
+        unexpected = args.reject do |arg|
+          whitelist.include?(arg.to_s) ||
+            arg.kind_of?(Arel::Node) || arg.is_a?(Arel::Nodes::SqlLiteral)
+        end
+
+        return if unexpected.none?
+
+        if allow_unsafe_raw_sql == :deprecated
+          ActiveSupport::Deprecation.warn(
+            "Dangerous query method used with non-attribute argument(s): " +
+            "#{unexpected.map(&:inspect).join(", ")}. Non-argument " +
+            "arguments will be disallowed in Rails 5.3."
+          )
+        else
+          raise(ActiveRecord::UnknownAttributeReference,
+            "Query method called with non-attribute argument(s): " +
+            unexpected.map(&:inspect).join(", ")
+          )
+        end
+      end
+
       # Can the given name be treated as a column name? Returns true if name
       # is attribute or attribute alias.
       #
@@ -178,7 +202,7 @@ module ActiveRecord
       #
       #   Person.respond_to_attribute?("foo")
       #   # => false
-      def respond_to_attribute?(name)
+      def respond_to_attribute?(name) # :nodoc:
         name = name.to_s
         attribute_names.include?(name) || attribute_aliases.include?(name)
       end
@@ -214,6 +238,18 @@ module ActiveRecord
           ConnectionAdapters::NullColumn.new(name)
         end
       end
+
+      # An Array of String attribute names and aliases for accessing those
+      # attributes.
+      #
+      #   class Person < ActiveRecord::Base
+      #   end
+      #
+      #   Person.attribute_names_and_aliases
+      #   # => ["id", "created_at", "updated_at", "name", "age"]
+      def attribute_names_and_aliases # :nodoc:
+        attribute_names + attribute_aliases.keys
+      end
     end
 
     # A Person object with a name attribute can ask <tt>person.respond_to?(:name)</tt>,
diff --git a/activerecord/lib/active_record/errors.rb b/activerecord/lib/active_record/errors.rb
index 9ef3316393..3c039eef82 100644
--- a/activerecord/lib/active_record/errors.rb
+++ b/activerecord/lib/active_record/errors.rb
@@ -339,4 +339,29 @@ module ActiveRecord
   # Wait time value is set by innodb_lock_wait_timeout.
   class TransactionTimeout < 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/querying.rb b/activerecord/lib/active_record/querying.rb
index 0233835bcf..3996d5661f 100644
--- a/activerecord/lib/active_record/querying.rb
+++ b/activerecord/lib/active_record/querying.rb
@@ -2,25 +2,18 @@
 
 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, :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 :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 :count, :average, :minimum, :maximum, :sum, :calculate, to: :all
-    delegate :pluck, :unsafe_raw_pluck, :ids, to: :all
+    delegate :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 dea7542f03..236d36e15f 100644
--- a/activerecord/lib/active_record/relation/calculations.rb
+++ b/activerecord/lib/active_record/relation/calculations.rb
@@ -175,13 +175,21 @@ module ActiveRecord
     # See also #ids.
     #
     def pluck(*column_names)
-      _pluck(column_names, @klass.allow_unsafe_raw_sql == :enabled)
-    end
+      if loaded? && (column_names.map(&:to_s) - @klass.attribute_names_and_aliases).empty?
+        return records.pluck(*column_names)
+      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)
+      if has_include?(column_names.first)
+        construct_relation_for_association_calculations.pluck(*column_names)
+      else
+        enforce_raw_sql_whitelist(column_names)
+        relation = spawn
+        relation.select_values = column_names.map { |cn|
+          @klass.respond_to_attribute?(cn) ? arel_attribute(cn) : cn
+        }
+        result = klass.connection.select_all(relation.arel, nil, bound_attributes)
+        result.cast_values(klass.attribute_types)
+      end
     end
 
     # Pluck all the ID's for the relation using the table's primary key
diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb
index 63b1d8e154..4c63d0450a 100644
--- a/activerecord/lib/active_record/relation/query_methods.rb
+++ b/activerecord/lib/active_record/relation/query_methods.rb
@@ -295,22 +295,9 @@ 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:
+      enforce_raw_sql_whitelist(column_names_from_order_arguments(args))
       preprocess_order_args(args)
 
       self.order_values += args
@@ -331,22 +318,9 @@ 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:
+      enforce_raw_sql_whitelist(column_names_from_order_arguments(args))
       preprocess_order_args(args)
 
       self.reordering_value = true
@@ -946,6 +920,11 @@ module ActiveRecord
 
     private
 
+      # Extract column names from arguments passed to #order or #reorder.
+      def column_names_from_order_arguments(args)
+        args.flat_map { |arg| arg.is_a?(Hash) ? arg.keys : arg }
+      end
+
       def assert_mutability!
         raise ImmutableRelation if @loaded
         raise ImmutableRelation if defined?(@arel) && @arel
@@ -1169,25 +1148,6 @@ 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.