diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 1749d554df..3b111d1538 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,11 @@ +* Deprecate `ActiveRecord::Base.allow_unsafe_raw_sql`. + + *Rafael Mendonça França* + +* Remove deprecated support for using unsafe raw SQL in `ActiveRecord::Relation` methods. + + *Rafael Mendonça França* + * Allow users to silence the "Rails couldn't infer whether you are using multiple databases..." message using `config.active_record.suppress_multiple_database_warning`. diff --git a/activerecord/lib/active_record/core.rb b/activerecord/lib/active_record/core.rb index 69b89afe51..f0922b5924 100644 --- a/activerecord/lib/active_record/core.rb +++ b/activerecord/lib/active_record/core.rb @@ -93,14 +93,6 @@ 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 @@ -236,6 +228,14 @@ module ActiveRecord klass end + def self.allow_unsafe_raw_sql # :nodoc: + ActiveSupport::Deprecation.warn("ActiveRecord::Base.allow_unsafe_raw_sql is deprecated and will be removed in Rails 6.2") + end + + def self.allow_unsafe_raw_sql=(value) # :nodoc: + ActiveSupport::Deprecation.warn("ActiveRecord::Base.allow_unsafe_raw_sql= is deprecated and will be removed in Rails 6.2") + end + self.default_connection_handler = ConnectionAdapters::ConnectionHandler.new self.default_role = writing_role self.default_shard = :default diff --git a/activerecord/lib/active_record/errors.rb b/activerecord/lib/active_record/errors.rb index f5a7440891..ffb6978daf 100644 --- a/activerecord/lib/active_record/errors.rb +++ b/activerecord/lib/active_record/errors.rb @@ -399,17 +399,15 @@ module ActiveRecord 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. + # value is passed to a query method. 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: + # For example, the following code would raise this exception: # # Post.order("length(title)").first # diff --git a/activerecord/lib/active_record/sanitization.rb b/activerecord/lib/active_record/sanitization.rb index 54f5f24040..6e3ffe5fc6 100644 --- a/activerecord/lib/active_record/sanitization.rb +++ b/activerecord/lib/active_record/sanitization.rb @@ -141,19 +141,7 @@ module ActiveRecord (unexpected ||= []) << arg end - return unless unexpected - - 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.1. 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 + if unexpected raise(ActiveRecord::UnknownAttributeReference, "Query method called with non-attribute argument(s): " + unexpected.map(&:inspect).join(", ") diff --git a/activerecord/test/cases/unsafe_raw_sql_test.rb b/activerecord/test/cases/unsafe_raw_sql_test.rb index 87edb163f2..af957e73ca 100644 --- a/activerecord/test/cases/unsafe_raw_sql_test.rb +++ b/activerecord/test/cases/unsafe_raw_sql_test.rb @@ -10,112 +10,90 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase 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) } + ids = Post.order("title").pluck(:id) - assert_equal ids_expected, ids_depr - assert_equal ids_expected, ids_disabled + assert_equal ids_expected, ids 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) } + ids = Post.order(:title).pluck(:id) - assert_equal ids_expected, ids_depr - assert_equal ids_expected, ids_disabled + assert_equal ids_expected, ids 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) } + ids = Post.order(title: :asc).pluck(:id) - assert_equal ids_expected, ids_depr - assert_equal ids_expected, ids_disabled + assert_equal ids_expected, ids 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) } + ids = Post.order(title: :ASC).pluck(:id) - assert_equal ids_expected, ids_depr - assert_equal ids_expected, ids_disabled + assert_equal ids_expected, ids 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) } + ids = Post.order(title: "asc").pluck(:id) - assert_equal ids_expected, ids_depr - assert_equal ids_expected, ids_disabled + assert_equal ids_expected, ids 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) } + ids = Post.order(:author_id, :title).pluck(:id) - assert_equal ids_expected, ids_depr - assert_equal ids_expected, ids_disabled + assert_equal ids_expected, ids 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) } + ids = Post.order(:author_id, title: :asc).pluck(:id) - assert_equal ids_expected, ids_depr - assert_equal ids_expected, ids_disabled + assert_equal ids_expected, ids end test "order: allows table and column names" 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) } + ids = Post.order("posts.title").pluck(:id) - assert_equal ids_expected, ids_depr - assert_equal ids_expected, ids_disabled + assert_equal ids_expected, ids end test "order: allows quoted table and column names" do ids_expected = Post.order(Arel.sql("title")).pluck(:id) quoted_title = Post.connection.quote_table_name("posts.title") - ids_depr = with_unsafe_raw_sql_deprecated { Post.order(quoted_title).pluck(:id) } - ids_disabled = with_unsafe_raw_sql_disabled { Post.order(quoted_title).pluck(:id) } + ids = Post.order(quoted_title).pluck(:id) - assert_equal ids_expected, ids_depr - assert_equal ids_expected, ids_disabled + assert_equal ids_expected, ids 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) } + ids = Post.order("title desc").pluck(:id) - assert_equal ids_expected, ids_depr - assert_equal ids_expected, ids_disabled + assert_equal ids_expected, ids 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) } + ids = Post.order("posts.title desc").pluck(:id) - assert_equal ids_expected, ids_depr - assert_equal ids_expected, ids_disabled + assert_equal ids_expected, ids end test "order: allows NULLS FIRST and NULLS LAST too" do @@ -129,224 +107,168 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase %w(first last).each do |position| ids_expected = Post.order(Arel.sql("type::text #{direction} nulls #{position}")).pluck(:id) - ids_depr = with_unsafe_raw_sql_deprecated { Post.order("type::text #{direction} nulls #{position}").pluck(:id) } - ids_disabled = with_unsafe_raw_sql_disabled { Post.order("type::text #{direction} nulls #{position}").pluck(:id) } + ids = Post.order("type::text #{direction} nulls #{position}").pluck(:id) - assert_equal ids_expected, ids_depr - assert_equal ids_expected, ids_disabled + assert_equal ids_expected, ids end end end if current_adapter?(:PostgreSQLAdapter) test "order: disallows invalid column name" do - with_unsafe_raw_sql_disabled do - assert_raises(ActiveRecord::UnknownAttributeReference) do - Post.order("REPLACE(title, 'misc', 'zzzz') asc").pluck(:id) - end + assert_raises(ActiveRecord::UnknownAttributeReference) do + Post.order("REPLACE(title, 'misc', 'zzzz') asc").pluck(:id) 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 + assert_raises(ArgumentError) do + Post.order(title: :foo).pluck(:id) end end test "order: disallows invalid column with direction" do - with_unsafe_raw_sql_disabled do - assert_raises(ActiveRecord::UnknownAttributeReference) do - Post.order("REPLACE(title, 'misc', 'zzzz')" => :asc).pluck(:id) - end + assert_raises(ActiveRecord::UnknownAttributeReference) do + Post.order("REPLACE(title, 'misc', 'zzzz')" => :asc).pluck(:id) 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) } + titles = Post.order(Arel.sql("length(title)")).pluck(:title) - assert_equal ids_depr, ids_disabled + assert_not_empty titles 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) } + ids = Post.order([Arel.sql("REPLACE(title, ?, ?), id"), "misc", "zzzz"]).pluck(:id) - assert_equal ids_expected, ids_depr - assert_equal ids_expected, ids_disabled + assert_equal ids_expected, ids 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 + assert_raises(ActiveRecord::UnknownAttributeReference) do + Post.order(["REPLACE(title, ?, ?), id", "misc", "zzzz"]).pluck(:id) end end test "order: disallows invalid Array arguments" do - with_unsafe_raw_sql_disabled do - assert_raises(ActiveRecord::UnknownAttributeReference) do - Post.order(["author_id", "REPLACE(title, 'misc', 'zzzz')"]).pluck(:id) - end + assert_raises(ActiveRecord::UnknownAttributeReference) do + Post.order(["author_id", "REPLACE(title, 'misc', 'zzzz')"]).pluck(:id) 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", "length(title)"]).pluck(:id) } - ids_disabled = with_unsafe_raw_sql_disabled { Post.order(["author_id", "length(title)"]).pluck(:id) } + ids = Post.order(["author_id", "length(title)"]).pluck(:id) - assert_equal ids_expected, ids_depr - assert_equal ids_expected, ids_disabled + assert_equal ids_expected, ids end test "order: logs deprecation warning for unrecognized column" do - with_unsafe_raw_sql_deprecated do - assert_deprecated(/Dangerous query method/) do - Post.order("REPLACE(title, 'misc', 'zzzz')") - end + e = assert_raises(ActiveRecord::UnknownAttributeReference) do + Post.order("REPLACE(title, 'misc', 'zzzz')") end + + assert_match(/Query method called with non-attribute argument\(s\):/, e.message) 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") } + titles = Post.pluck("title") - assert_equal titles_expected, titles_depr - assert_equal titles_expected, titles_disabled + assert_equal titles_expected, titles end test "pluck: allows string column name with function and alias" do titles_expected = Post.pluck(Arel.sql("UPPER(title)")) - titles_depr = with_unsafe_raw_sql_deprecated { Post.pluck("UPPER(title) AS title") } - titles_disabled = with_unsafe_raw_sql_disabled { Post.pluck("UPPER(title) AS title") } + titles = Post.pluck("UPPER(title) AS title") - assert_equal titles_expected, titles_depr - assert_equal titles_expected, titles_disabled + assert_equal titles_expected, titles 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) } + titles = Post.pluck(:title) - assert_equal titles_expected, titles_depr - assert_equal titles_expected, titles_disabled + assert_equal titles_expected, titles 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) } + values = Post.pluck(:title, :id) - assert_equal values_expected, values_depr - assert_equal values_expected, values_disabled + assert_equal values_expected, values 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) } + values = Post.includes(:comments).pluck(:title, :id) - assert_equal values_expected, values_depr - assert_equal values_expected, values_disabled + assert_equal values_expected, values 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) } + values = Post.pluck(:tags_count) - assert_equal values_expected, values_depr - assert_equal values_expected, values_disabled + assert_equal values_expected, values 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") } + titles = Post.pluck("posts.title") - assert_equal titles_expected, titles_depr - assert_equal titles_expected, titles_disabled + assert_equal titles_expected, titles end test "pluck: allows quoted table and column names" do titles_expected = Post.pluck(Arel.sql("title")) - quoted_title = Post.connection.quote_table_name("posts.title") - titles_depr = with_unsafe_raw_sql_deprecated { Post.pluck(quoted_title) } - titles_disabled = with_unsafe_raw_sql_disabled { Post.pluck(quoted_title) } + quoted_title = Post.connection.quote_table_name("posts.title") + titles = Post.pluck(quoted_title) - assert_equal titles_expected, titles_depr - assert_equal titles_expected, titles_disabled + assert_equal titles_expected, titles end test "pluck: disallows invalid column name" do - with_unsafe_raw_sql_disabled do - assert_raises(ActiveRecord::UnknownAttributeReference) do - Post.pluck("REPLACE(title, 'misc', 'zzzz')") - end + assert_raises(ActiveRecord::UnknownAttributeReference) do + Post.pluck("REPLACE(title, 'misc', 'zzzz')") 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, "REPLACE(title, 'misc', 'zzzz')") - end + assert_raises(ActiveRecord::UnknownAttributeReference) do + Post.pluck(:title, "REPLACE(title, 'misc', 'zzzz')") 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, "REPLACE(title, 'misc', 'zzzz')") - end + assert_raises(ActiveRecord::UnknownAttributeReference) do + Post.includes(:comments).pluck(:title, "REPLACE(title, 'misc', 'zzzz')") 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)")) } + excepted_values = Post.includes(:comments).pluck(:title).map { |title| [title, title.size] } + values = Post.includes(:comments).pluck(:title, Arel.sql("length(title)")) - assert_equal values_depr, values_disabled + assert_equal excepted_values, values end test "pluck: logs deprecation warning" do - with_unsafe_raw_sql_deprecated do - assert_deprecated(/Dangerous query method/) do - Post.includes(:comments).pluck(:title, "REPLACE(title, 'misc', 'zzzz')") - end + e = assert_raises(ActiveRecord::UnknownAttributeReference) do + Post.includes(:comments).pluck(:title, "REPLACE(title, 'misc', 'zzzz')") end + + assert_match(/Query method called with non-attribute argument\(s\):/, e.message) end - - private - def with_unsafe_raw_sql_disabled(&block) - with_config(:disabled, &block) - end - - def with_unsafe_raw_sql_deprecated(&block) - with_config(:deprecated, &block) - end - - def with_config(new_value, &block) - old_value = ActiveRecord::Base.allow_unsafe_raw_sql - ActiveRecord::Base.allow_unsafe_raw_sql = new_value - yield - ensure - ActiveRecord::Base.allow_unsafe_raw_sql = old_value - end end diff --git a/guides/source/6_1_release_notes.md b/guides/source/6_1_release_notes.md index 9536fb6c05..bd1629c4c1 100644 --- a/guides/source/6_1_release_notes.md +++ b/guides/source/6_1_release_notes.md @@ -149,8 +149,12 @@ Please refer to the [Changelog][active-record] for detailed changes. ### Removals +* Remove deprecated support for using unsafe raw SQL in `ActiveRecord::Relation` methods. + ### Deprecations +* Deprecate `ActiveRecord::Base.allow_unsafe_raw_sql`. + ### Notable changes Active Storage