diff options
| -rw-r--r-- | activerecord/lib/active_record/attribute_methods.rb | 38 | ||||
| -rw-r--r-- | activerecord/lib/active_record/errors.rb | 25 | ||||
| -rw-r--r-- | activerecord/lib/active_record/querying.rb | 21 | ||||
| -rw-r--r-- | activerecord/lib/active_record/relation/calculations.rb | 20 | ||||
| -rw-r--r-- | activerecord/lib/active_record/relation/query_methods.rb | 54 | ||||
| -rw-r--r-- | activerecord/test/cases/calculations_test.rb | 24 | ||||
| -rw-r--r-- | activerecord/test/cases/relation/merging_test.rb | 8 | ||||
| -rw-r--r-- | activerecord/test/cases/unsafe_raw_sql_test.rb | 70 |
8 files changed, 159 insertions, 101 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. diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb index 66bc14b5ab..20dcb0080b 100644 --- a/activerecord/test/cases/calculations_test.rb +++ b/activerecord/test/cases/calculations_test.rb @@ -144,7 +144,7 @@ class CalculationsTest < ActiveRecord::TestCase end def test_should_order_by_calculation - c = Account.group(:firm_id).order("sum_credit_limit desc, firm_id").sum(:credit_limit) + c = Account.group(:firm_id).order(Arel.sql("sum_credit_limit desc, firm_id")).sum(:credit_limit) assert_equal [105, 60, 53, 50, 50], c.keys.collect { |k| c[k] } assert_equal [6, 2, 9, 1], c.keys.compact end @@ -644,7 +644,7 @@ class CalculationsTest < ActiveRecord::TestCase end def test_pluck_with_qualified_column_name - assert_equal [1, 2, 3, 4, 5], Topic.order(:id).pluck("topics.id") + assert_equal [1, 2, 3, 4, 5], Topic.order(:id).pluck(Arel.sql("topics.id")) end def test_pluck_auto_table_name_prefix @@ -659,18 +659,18 @@ class CalculationsTest < ActiveRecord::TestCase def test_pluck_not_auto_table_name_prefix_if_column_joined Company.create!(name: "test", contracts: [Contract.new(developer_id: 7)]) - assert_equal [7], Company.joins(:contracts).pluck(:developer_id) + assert_equal [7], Company.joins(:contracts).pluck(Arel.sql("developer_id")) 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 @@ -684,7 +684,7 @@ class CalculationsTest < ActiveRecord::TestCase def test_pluck_not_auto_table_name_prefix_if_column_included Company.create!(name: "test", contracts: [Contract.new(developer_id: 7)]) - ids = Company.includes(:contracts).pluck(:developer_id) + ids = Company.includes(:contracts).pluck(Arel.sql("developer_id")) assert_equal Company.count, ids.length assert_equal [7], ids.compact end @@ -704,12 +704,12 @@ class CalculationsTest < ActiveRecord::TestCase def test_pluck_with_multiple_columns_and_selection_clause assert_equal [[1, 50], [2, 50], [3, 50], [4, 60], [5, 55], [6, 53]], - Account.pluck("id, credit_limit") + Account.pluck(Arel.sql("id, credit_limit")) end def test_pluck_with_multiple_columns_and_includes Company.create!(name: "test", contracts: [Contract.new(developer_id: 7)]) - companies_and_developers = Company.order("companies.id").includes(:contracts).pluck(:name, :developer_id) + companies_and_developers = Company.order(Arel.sql("companies.id")).includes(:contracts).pluck(:name, Arel.sql("developer_id")) assert_equal Company.count, companies_and_developers.length assert_equal ["37signals", nil], companies_and_developers.first @@ -731,7 +731,7 @@ class CalculationsTest < ActiveRecord::TestCase def test_pluck_columns_with_same_name expected = [["The First Topic", "The Second Topic of the day"], ["The Third Topic of the day", "The Fourth Topic of the day"]] actual = Topic.joins(:replies) - .pluck("topics.title", "replies_topics.title") + .pluck(Arel.sql("topics.title"), Arel.sql("replies_topics.title")) assert_equal expected, actual end @@ -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/relation/merging_test.rb b/activerecord/test/cases/relation/merging_test.rb index b68b3723f6..953e0fee76 100644 --- a/activerecord/test/cases/relation/merging_test.rb +++ b/activerecord/test/cases/relation/merging_test.rb @@ -118,7 +118,7 @@ class MergingDifferentRelationsTest < ActiveRecord::TestCase test "merging where relations" do hello_by_bob = Post.where(body: "hello").joins(:author). - merge(Author.where(name: "Bob")).order("posts.id").pluck("posts.id") + merge(Author.where(name: "Bob")).order("posts.id").pluck(Arel.sql("posts.id")) assert_equal [posts(:misc_by_bob).id, posts(:other_by_bob).id], hello_by_bob @@ -126,19 +126,19 @@ class MergingDifferentRelationsTest < ActiveRecord::TestCase test "merging order relations" do posts_by_author_name = Post.limit(3).joins(:author). - merge(Author.order(:name)).pluck("authors.name") + merge(Author.order(:name)).pluck(Arel.sql("authors.name")) assert_equal ["Bob", "Bob", "David"], posts_by_author_name posts_by_author_name = Post.limit(3).joins(:author). - merge(Author.order("name")).pluck("authors.name") + merge(Author.order("name")).pluck(Arel.sql("authors.name")) assert_equal ["Bob", "Bob", "David"], posts_by_author_name end test "merging order relations (using a hash argument)" do posts_by_author_name = Post.limit(4).joins(:author). - merge(Author.order(name: :desc)).pluck("authors.name") + merge(Author.order(name: :desc)).pluck(Arel.sql("authors.name")) assert_equal ["Mary", "Mary", "Mary", "David"], posts_by_author_name end diff --git a/activerecord/test/cases/unsafe_raw_sql_test.rb b/activerecord/test/cases/unsafe_raw_sql_test.rb index d05f0f12d9..89eb02594a 100644 --- a/activerecord/test/cases/unsafe_raw_sql_test.rb +++ b/activerecord/test/cases/unsafe_raw_sql_test.rb @@ -13,7 +13,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase end assert_equal enabled, disabled - assert_equal disabled, Post.unsafe_raw_order("title").pluck(:id) + assert_equal disabled, Post.order(Arel.sql("title")).pluck(:id) end test "order: allows symbol column name" do @@ -22,7 +22,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase end assert_equal enabled, disabled - assert_equal disabled, Post.unsafe_raw_order(:title).pluck(:id) + assert_equal disabled, Post.order(Arel.sql("title")).pluck(:id) end test "order: allows downcase symbol direction" do @@ -31,7 +31,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase end assert_equal enabled, disabled - assert_equal disabled, Post.unsafe_raw_order(title: :asc).pluck(:id) + assert_equal disabled, Post.order(Arel.sql("title") => Arel.sql("asc")).pluck(:id) end test "order: allows upcase symbol direction" do @@ -40,7 +40,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase end assert_equal enabled, disabled - assert_equal disabled, Post.unsafe_raw_order(title: :ASC).pluck(:id) + assert_equal disabled, Post.order(Arel.sql("title") => Arel.sql("ASC")).pluck(:id) end test "order: allows string direction" do @@ -49,7 +49,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase end assert_equal enabled, disabled - assert_equal disabled, Post.unsafe_raw_order(title: "asc").pluck(:id) + assert_equal disabled, Post.order(Arel.sql("title") => Arel.sql("asc")).pluck(:id) end test "order: allows multiple columns" do @@ -58,7 +58,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase end assert_equal enabled, disabled - assert_equal disabled, Post.unsafe_raw_order(:author_id, :title).pluck(:id) + assert_equal disabled, Post.order(Arel.sql("author_id"), Arel.sql("title")).pluck(:id) end test "order: allows mixed" do @@ -67,12 +67,12 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase end assert_equal enabled, disabled - assert_equal disabled, Post.unsafe_raw_order(:author_id, title: :asc).pluck(:id) + assert_equal disabled, Post.order(Arel.sql("author_id"), Arel.sql("title") => Arel.sql("asc")).pluck(:id) end test "order: disallows invalid column name" do with_config(:disabled) do - assert_raises(ArgumentError) do + assert_raises(ActiveRecord::UnknownAttributeReference) do Post.order("title asc").pluck(:id) end end @@ -88,19 +88,37 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase test "order: disallows invalid column with direction" do with_config(:disabled) do - assert_raises(ArgumentError) do + assert_raises(ActiveRecord::UnknownAttributeReference) do Post.order(foo: :asc).pluck(:id) end end end + test "order: always allows Arel" do + enabled, disabled = with_configs(:enabled, :disabled) do + Post.order(Arel.sql("length(title)")).pluck(:title) + end + + assert_equal enabled, disabled + end + + test "order: logs deprecation warning for unrecognized column" do + with_config(:deprecated) do + ActiveSupport::Deprecation.expects(:warn).with do |msg| + msg =~ /\ADangerous query method used with .*length\(title\)/ + end + + Post.order("length(title)") + 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") + assert_equal disabled, Post.pluck(Arel.sql("title")) end test "pluck: allows symbol column name" do @@ -109,7 +127,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase end assert_equal enabled, disabled - assert_equal disabled, Post.unsafe_raw_pluck(:title) + assert_equal disabled, Post.pluck(Arel.sql("title")) end test "pluck: allows multiple column names" do @@ -118,7 +136,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase end assert_equal enabled, disabled - assert_equal disabled, Post.unsafe_raw_pluck(:title, :id) + assert_equal disabled, Post.pluck(Arel.sql("title"), Arel.sql("id")) end test "pluck: allows column names with includes" do @@ -127,7 +145,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase end assert_equal enabled, disabled - assert_equal disabled, Post.includes(:comments).unsafe_raw_pluck(:title, :id) + assert_equal disabled, Post.includes(:comments).pluck(Arel.sql("title"), Arel.sql("id")) end test "pluck: allows auto-generated attributes" do @@ -136,12 +154,12 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase end assert_equal enabled, disabled - assert_equal disabled, Post.unsafe_raw_pluck(:tags_count) + assert_equal disabled, Post.pluck(Arel.sql("tags_count")) end test "pluck: disallows invalid column name" do with_config(:disabled) do - assert_raises(ArgumentError) do + assert_raises(ActiveRecord::UnknownAttributeReference) do Post.pluck("length(title)") end end @@ -149,7 +167,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase test "pluck: disallows invalid column name amongst valid names" do with_config(:disabled) do - assert_raises(ArgumentError) do + assert_raises(ActiveRecord::UnknownAttributeReference) do Post.pluck(:title, "length(title)") end end @@ -157,12 +175,30 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase test "pluck: disallows invalid column names with includes" do with_config(:disabled) do - assert_raises(ArgumentError) do + assert_raises(ActiveRecord::UnknownAttributeReference) do Post.includes(:comments).pluck(:title, "length(title)") end end end + test "pluck: always allows Arel" do + enabled, disabled = with_configs(:enabled, :disabled) do + Post.includes(:comments).pluck(:title, Arel.sql("length(title)")) + end + + assert_equal enabled, disabled + end + + test "pluck: logs deprecation warning" do + with_config(:deprecated) do + ActiveSupport::Deprecation.expects(:warn).with do |msg| + msg =~ /\ADangerous query method used with .*length\(title\)/ + end + + Post.includes(:comments).pluck(:title, "length(title)") + end + end + def with_configs(*new_values, &blk) new_values.map { |nv| with_config(nv, &blk) } end |