Remove deprecated support for using unsafe raw SQL in ActiveRecord::Relation methods

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`.
 

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

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
   #

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(", ")

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
   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
   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
   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
   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
   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
+    e = assert_raises(ActiveRecord::UnknownAttributeReference) do
       Post.order("REPLACE(title, 'misc', 'zzzz')")
     end
-    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) }
+    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
   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
   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
   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
+    e = assert_raises(ActiveRecord::UnknownAttributeReference) do
       Post.includes(:comments).pluck(:title, "REPLACE(title, 'misc', 'zzzz')")
     end
-    end
-  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
+    assert_match(/Query method called with non-attribute argument\(s\):/, e.message)
   end
 end

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