Require explict allowlisting of attributes and associations (activerecord-hackery#1400)

Co-authored-by: lukas-eu <62448426+lukas-eu@users.noreply.github.com>
Co-authored-by: Wes Oldenbeuving <wes@narnach.com>

Author: 3 people

bug_report_templates/test-ransacker-arel-present-predicate.rb

@@ -56,6 +56,10 @@ class Project < ActiveRecord::Base
   ransacker :number do |parent|
     parent.table[:number]
   end
+
+  def self.ransackable_attributes(_auth_object = nil)
+    ["name", "number"]
+  end
 end
 
 class BugTest < Minitest::Test

lib/ransack/adapters/active_record/base.rb

@@ -35,20 +35,15 @@ def ransack_alias(new_name, old_name)
         # For overriding with a whitelist array of strings.
         #
         def ransackable_attributes(auth_object = nil)
-          @ransackable_attributes ||= if Ransack::SUPPORTS_ATTRIBUTE_ALIAS
-            column_names + _ransackers.keys + _ransack_aliases.keys +
-            attribute_aliases.keys
-          else
-            column_names + _ransackers.keys + _ransack_aliases.keys
-          end
+          @ransackable_attributes ||= deprecated_ransackable_list(:ransackable_attributes)
         end
 
         # Ransackable_associations, by default, returns the names
         # of all associations as an array of strings.
         # For overriding with a whitelist array of strings.
         #
         def ransackable_associations(auth_object = nil)
-          @ransackable_associations ||= reflect_on_all_associations.map { |a| a.name.to_s }
+          @ransackable_associations ||= deprecated_ransackable_list(:ransackable_associations)
         end
 
         # Ransortable_attributes, by default, returns the names
@@ -75,6 +70,82 @@ def ransackable_scopes_skip_sanitize_args
           []
         end
 
+        # Bare list of all potentially searchable attributes. Searchable attributes
+        # need to be explicitly allowlisted through the `ransackable_attributes`
+        # method in each model, but if you're allowing almost everything to be
+        # searched, this list can be used as a base for exclusions.
+        #
+        def authorizable_ransackable_attributes
+          if Ransack::SUPPORTS_ATTRIBUTE_ALIAS
+            column_names + _ransackers.keys + _ransack_aliases.keys +
+            attribute_aliases.keys
+          else
+            column_names + _ransackers.keys + _ransack_aliases.keys
+          end.uniq
+        end
+
+        # Bare list of all potentially searchable associations. Searchable
+        # associations need to be explicitly allowlisted through the
+        # `ransackable_associations` method in each model, but if you're
+        # allowing almost everything to be searched, this list can be used as a
+        # base for exclusions.
+        #
+        def authorizable_ransackable_associations
+          reflect_on_all_associations.map { |a| a.name.to_s }
+        end
+
+        private
+
+        def deprecated_ransackable_list(method)
+          list_type = method.to_s.delete_prefix("ransackable_")
+
+          if explicitly_defined?(method)
+            warn_deprecated <<~ERROR
+              Ransack's builtin `#{method}` method is deprecated and will result
+              in an error in the future. If you want to authorize the full list
+              of searchable #{list_type} for this model, use
+              `authorizable_#{method}` instead of delegating to `super`.
+            ERROR
+
+            public_send("authorizable_#{method}")
+          else
+            raise <<~MESSAGE
+              Ransack needs #{name} #{list_type} explicitly allowlisted as
+              searchable. Define a `#{method}` class method in your `#{name}`
+              model, watching out for items you DON'T want searchable (for
+              example, `encrypted_password`, `password_reset_token`, `owner` or
+              other sensitive information). You can use the following as a base:
+
+              ```ruby
+              class #{name} < ApplicationRecord
+
+                # ...
+
+                def self.#{method}(auth_object = nil)
+                  #{public_send("authorizable_#{method}").sort.inspect}
+                end
+
+                # ...
+
+              end
+              ```
+            MESSAGE
+          end
+        end
+
+        def explicitly_defined?(method)
+          definer_ancestor = singleton_class.ancestors.find do |ancestor|
+            ancestor.instance_methods(false).include?(method)
+          end
+
+          definer_ancestor != Ransack::Adapters::ActiveRecord::Base
+        end
+
+        def warn_deprecated(message)
+          caller_location = caller_locations.find { |location| !location.path.start_with?(File.expand_path("../..", __dir__)) }
+
+          warn "DEPRECATION WARNING: #{message.squish} (called at #{caller_location.path}:#{caller_location.lineno})"
+        end
       end
     end
   end

spec/ransack/adapters/active_record/base_spec.rb

@@ -657,6 +657,37 @@ def self.simple_escaping?
             it { should_not include 'only_sort' }
             it { should include 'only_admin' }
           end
+
+          context 'when not defined in model, nor in ApplicationRecord' do
+            subject { Article.ransackable_attributes }
+
+            it "raises a helpful error" do
+              without_application_record_method(:ransackable_attributes) do
+                expect { subject }.to raise_error(RuntimeError, /Ransack needs Article attributes explicitly allowlisted/)
+              end
+            end
+          end
+
+          context 'when defined only in model by delegating to super' do
+            subject { Article.ransackable_attributes }
+
+            around do |example|
+              Article.singleton_class.define_method(:ransackable_attributes) do
+                super(nil) - super(nil)
+              end
+
+              example.run
+            ensure
+              Article.singleton_class.remove_method(:ransackable_attributes)
+            end
+
+            it "returns the allowlist in the model, and warns" do
+              without_application_record_method(:ransackable_attributes) do
+                expect { subject }.to output(/Ransack's builtin `ransackable_attributes` method is deprecated/).to_stderr
+                expect(subject).to be_empty
+              end
+            end
+          end
         end
 
         describe '#ransortable_attributes' do
@@ -689,6 +720,37 @@ def self.simple_escaping?
           it { should include 'parent' }
           it { should include 'children' }
           it { should include 'articles' }
+
+          context 'when not defined in model, nor in ApplicationRecord' do
+            subject { Article.ransackable_associations }
+
+            it "raises a helpful error" do
+              without_application_record_method(:ransackable_associations) do
+                expect { subject }.to raise_error(RuntimeError, /Ransack needs Article associations explicitly allowlisted/)
+              end
+            end
+          end
+
+          context 'when defined only in model by delegating to super' do
+            subject { Article.ransackable_associations }
+
+            around do |example|
+              Article.singleton_class.define_method(:ransackable_associations) do
+                super(nil) - super(nil)
+              end
+
+              example.run
+            ensure
+              Article.singleton_class.remove_method(:ransackable_associations)
+            end
+
+            it "returns the allowlist in the model, and warns" do
+              without_application_record_method(:ransackable_associations) do
+                expect { subject }.to output(/Ransack's builtin `ransackable_associations` method is deprecated/).to_stderr
+                expect(subject).to be_empty
+              end
+            end
+          end
         end
 
         describe '#ransackable_scopes' do
@@ -704,6 +766,17 @@ def self.simple_escaping?
         end
 
         private
+
+        def without_application_record_method(method)
+          ApplicationRecord.singleton_class.alias_method :"original_#{method}", :"#{method}"
+          ApplicationRecord.singleton_class.remove_method :"#{method}"
+
+          yield
+        ensure
+          ApplicationRecord.singleton_class.alias_method :"#{method}", :"original_#{method}"
+          ApplicationRecord.singleton_class.remove_method :"original_#{method}"
+        end
+
         def rails7_and_mysql
           ::ActiveRecord::VERSION::MAJOR >= 7 && ENV['DB'] == 'mysql'
         end

spec/support/schema.rb

@@ -28,7 +28,24 @@
   )
 end
 
-class Person < ActiveRecord::Base
+# This is just a test app with no sensitive data, so we explicitly allowlist all
+# attributes and associations for search. In general, end users should
+# explicitly authorize each model, but this shows a way to configure the
+# unrestricted default behavior of versions prior to Ransack 4.
+#
+class ApplicationRecord < ActiveRecord::Base
+  self.abstract_class = true
+
+  def self.ransackable_attributes(auth_object = nil)
+    authorizable_ransackable_attributes
+  end
+
+  def self.ransackable_associations(auth_object = nil)
+    authorizable_ransackable_associations
+  end
+end
+
+class Person < ApplicationRecord
   default_scope { order(id: :desc) }
   belongs_to :parent, class_name: 'Person', foreign_key: :parent_id
   has_many   :children, class_name: 'Person', foreign_key: :parent_id
@@ -111,9 +128,9 @@ class Person < ActiveRecord::Base
 
   def self.ransackable_attributes(auth_object = nil)
     if auth_object == :admin
-      super - ['only_sort']
+      authorizable_ransackable_attributes - ['only_sort']
     else
-      super - ['only_sort', 'only_admin']
+      authorizable_ransackable_attributes - ['only_sort', 'only_admin']
     end
   end
 
@@ -129,7 +146,7 @@ def self.ransortable_attributes(auth_object = nil)
 class Musician < Person
 end
 
-class Article < ActiveRecord::Base
+class Article < ApplicationRecord
   belongs_to :person
   has_many :comments
   has_and_belongs_to_many :tags
@@ -182,7 +199,7 @@ class Article < ActiveRecord::Base
 class StoryArticle < Article
 end
 
-class Recommendation < ActiveRecord::Base
+class Recommendation < ApplicationRecord
   belongs_to :person
   belongs_to :target_person, class_name: 'Person'
   belongs_to :article
@@ -200,22 +217,22 @@ class Article < ::Article
   end
 end
 
-class Comment < ActiveRecord::Base
+class Comment < ApplicationRecord
   belongs_to :article
   belongs_to :person
 
   default_scope { where(disabled: false) }
 end
 
-class Tag < ActiveRecord::Base
+class Tag < ApplicationRecord
   has_and_belongs_to_many :articles
 end
 
-class Note < ActiveRecord::Base
+class Note < ApplicationRecord
   belongs_to :notable, polymorphic: true
 end
 
-class Account < ActiveRecord::Base
+class Account < ApplicationRecord
   belongs_to :agent_account, class_name: "Account"
   belongs_to :trade_account, class_name: "Account"
 end
@@ -308,7 +325,7 @@ def self.create
 end
 
 module SubDB
-  class Base < ActiveRecord::Base
+  class Base < ApplicationRecord
     self.abstract_class = true
     establish_connection(
       adapter: 'sqlite3',