diff options
| -rw-r--r-- | activerecord/lib/active_record/attribute_methods.rb | 16 | ||||
| -rw-r--r-- | activerecord/lib/active_record/core.rb | 9 | ||||
| -rw-r--r-- | activerecord/lib/active_record/querying.rb | 21 | ||||
| -rw-r--r-- | activerecord/lib/active_record/relation/calculations.rb | 42 | ||||
| -rw-r--r-- | activerecord/lib/active_record/relation/query_methods.rb | 49 | ||||
| -rw-r--r-- | activerecord/test/cases/unsafe_raw_sql_test.rb | 177 |
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 |