From 2922e334ad2ff8dbcdcdbb02e97c6548e487ff43 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Rodr=C3=ADguez?= Date: Tue, 7 Feb 2023 00:03:21 +0100 Subject: [PATCH] Require explict allowlisting of attributes and associations (#1400) Co-authored-by: lukas-eu <62448426+lukas-eu@users.noreply.github.com> Co-authored-by: Wes Oldenbeuving --- .../test-ransacker-arel-present-predicate.rb | 4 + lib/ransack/adapters/active_record/base.rb | 85 +++++++++++++++++-- .../adapters/active_record/base_spec.rb | 73 ++++++++++++++++ spec/support/schema.rb | 37 +++++--- 4 files changed, 182 insertions(+), 17 deletions(-) diff --git a/bug_report_templates/test-ransacker-arel-present-predicate.rb b/bug_report_templates/test-ransacker-arel-present-predicate.rb index 6939c204d..f3c82fbdd 100644 --- a/bug_report_templates/test-ransacker-arel-present-predicate.rb +++ b/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 diff --git a/lib/ransack/adapters/active_record/base.rb b/lib/ransack/adapters/active_record/base.rb index a6883b965..15a4cd666 100644 --- a/lib/ransack/adapters/active_record/base.rb +++ b/lib/ransack/adapters/active_record/base.rb @@ -35,12 +35,7 @@ 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 @@ -48,7 +43,7 @@ def ransackable_attributes(auth_object = nil) # 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 diff --git a/spec/ransack/adapters/active_record/base_spec.rb b/spec/ransack/adapters/active_record/base_spec.rb index 44ecc2708..10e3640de 100644 --- a/spec/ransack/adapters/active_record/base_spec.rb +++ b/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 diff --git a/spec/support/schema.rb b/spec/support/schema.rb index 726261387..53af0f38c 100644 --- a/spec/support/schema.rb +++ b/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',