From 32b03b46150b0161eba2321ccac7678511e3d58e Mon Sep 17 00:00:00 2001 From: Yoshiyuki Kinjo Date: Tue, 9 Oct 2018 16:14:51 +0900 Subject: [PATCH] Implement AR#inspect using ParamterFilter. AR instance support `filter_parameters` since #33756. Though Regex or Proc is valid as `filter_parameters`, they are not supported as AR#inspect. I also add :mask option and #filter_params to `ActiveSupport::ParameterFilter#new` to implement this. --- activerecord/CHANGELOG.md | 6 +- .../lib/active_record/attribute_methods.rb | 19 +++--- activerecord/lib/active_record/core.rb | 41 ++++++------ .../test/cases/filter_attributes_test.rb | 56 ++++++++++++++++- .../lib/active_support/parameter_filter.rb | 62 ++++++++++++------- activesupport/test/parameter_filter_test.rb | 54 ++++++++++++++++ .../test/application/configuration_test.rb | 2 +- 7 files changed, 185 insertions(+), 55 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 00f4ee1aaa5..0c3359658d6 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -138,13 +138,13 @@ specify sensitive attributes to specific model. ``` - Rails.application.config.filter_parameters += [:credit_card_number] - Account.last.inspect # => # + Rails.application.config.filter_parameters += [:credit_card_number, /phone/] + Account.last.inspect # => # SecureAccount.filter_attributes += [:name] SecureAccount.last.inspect # => # ``` - *Zhang Kang* + *Zhang Kang*, *Yoshiyuki Kinjo* * Deprecate `column_name_length`, `table_name_length`, `columns_per_table`, `indexes_per_table`, `columns_per_multicolumn_index`, `sql_query_length`, diff --git a/activerecord/lib/active_record/attribute_methods.rb b/activerecord/lib/active_record/attribute_methods.rb index 3c785180e16..1581490f161 100644 --- a/activerecord/lib/active_record/attribute_methods.rb +++ b/activerecord/lib/active_record/attribute_methods.rb @@ -336,14 +336,7 @@ module ActiveRecord # # => "[1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11]" def attribute_for_inspect(attr_name) value = read_attribute(attr_name) - - if value.is_a?(String) && value.length > 50 - "#{value[0, 50]}...".inspect - elsif value.is_a?(Date) || value.is_a?(Time) - %("#{value.to_s(:db)}") - else - value.inspect - end + format_for_inspect(value) end # Returns +true+ if the specified +attribute+ has been set by the user or by a @@ -463,6 +456,16 @@ module ActiveRecord end end + def format_for_inspect(value) + if value.is_a?(String) && value.length > 50 + "#{value[0, 50]}...".inspect + elsif value.is_a?(Date) || value.is_a?(Time) + %("#{value.to_s(:db)}") + else + value.inspect + end + end + def readonly_attribute?(name) self.class.readonly_attributes.include?(name) end diff --git a/activerecord/lib/active_record/core.rb b/activerecord/lib/active_record/core.rb index b53681ee203..97802583027 100644 --- a/activerecord/lib/active_record/core.rb +++ b/activerecord/lib/active_record/core.rb @@ -2,15 +2,13 @@ require "active_support/core_ext/hash/indifferent_access" require "active_support/core_ext/string/filters" +require "active_support/parameter_filter" require "concurrent/map" -require "set" module ActiveRecord module Core extend ActiveSupport::Concern - FILTERED = "[FILTERED]" # :nodoc: - included do ## # :singleton-method: @@ -239,9 +237,7 @@ module ActiveRecord end # Specifies columns which shouldn't be exposed while calling +#inspect+. - def filter_attributes=(attributes_names) - @filter_attributes = attributes_names.map(&:to_s).to_set - end + attr_writer :filter_attributes # Returns a string like 'Post(id:integer, title:string, body:text)' def inspect # :nodoc: @@ -514,11 +510,14 @@ module ActiveRecord inspection = if defined?(@attributes) && @attributes self.class.attribute_names.collect do |name| if has_attribute?(name) - if filter_attribute?(name) - "#{name}: #{ActiveRecord::Core::FILTERED}" + attr = read_attribute(name) + value = if attr.nil? + attr.inspect else - "#{name}: #{attribute_for_inspect(name)}" + attr = format_for_inspect(attr) + inspection_filter.filter_param(name, attr) end + "#{name}: #{value}" end end.compact.join(", ") else @@ -534,18 +533,16 @@ module ActiveRecord return super if custom_inspect_method_defined? pp.object_address_group(self) do if defined?(@attributes) && @attributes - column_names = self.class.column_names.select { |name| has_attribute?(name) || new_record? } - pp.seplist(column_names, proc { pp.text "," }) do |column_name| + attr_names = self.class.attribute_names.select { |name| has_attribute?(name) } + pp.seplist(attr_names, proc { pp.text "," }) do |attr_name| pp.breakable " " pp.group(1) do - pp.text column_name + pp.text attr_name pp.text ":" pp.breakable - if filter_attribute?(column_name) - pp.text ActiveRecord::Core::FILTERED - else - pp.pp read_attribute(column_name) - end + value = read_attribute(attr_name) + value = inspection_filter.filter_param(attr_name, value) unless value.nil? + pp.pp value end end else @@ -597,8 +594,14 @@ module ActiveRecord self.class.instance_method(:inspect).owner != ActiveRecord::Base.instance_method(:inspect).owner end - def filter_attribute?(attribute_name) - self.class.filter_attributes.include?(attribute_name) && !read_attribute(attribute_name).nil? + def inspection_filter + @inspection_filter ||= begin + mask = DelegateClass(::String).new(ActiveSupport::ParameterFilter::FILTERED) + def mask.pretty_print(pp) + pp.text __getobj__ + end + ActiveSupport::ParameterFilter.new(self.class.filter_attributes, mask: mask) + end end end end diff --git a/activerecord/test/cases/filter_attributes_test.rb b/activerecord/test/cases/filter_attributes_test.rb index af5badd87dd..47161a547a9 100644 --- a/activerecord/test/cases/filter_attributes_test.rb +++ b/activerecord/test/cases/filter_attributes_test.rb @@ -4,6 +4,7 @@ require "cases/helper" require "models/admin" require "models/admin/user" require "models/admin/account" +require "models/user" require "pp" class FilterAttributesTest < ActiveRecord::TestCase @@ -30,6 +31,32 @@ class FilterAttributesTest < ActiveRecord::TestCase end end + test "string filter_attributes perform pertial match" do + ActiveRecord::Base.filter_attributes = ["n"] + Admin::Account.all.each do |account| + assert_includes account.inspect, "name: [FILTERED]" + assert_equal 1, account.inspect.scan("[FILTERED]").length + end + end + + test "regex filter_attributes are accepted" do + ActiveRecord::Base.filter_attributes = [/\An\z/] + account = Admin::Account.find_by(name: "37signals") + assert_includes account.inspect, 'name: "37signals"' + assert_equal 0, account.inspect.scan("[FILTERED]").length + + ActiveRecord::Base.filter_attributes = [/\An/] + account = Admin::Account.find_by(name: "37signals") + assert_includes account.reload.inspect, "name: [FILTERED]" + assert_equal 1, account.inspect.scan("[FILTERED]").length + end + + test "proc filter_attributes are accepted" do + ActiveRecord::Base.filter_attributes = [ lambda { |key, value| value.reverse! if key == "name" } ] + account = Admin::Account.find_by(name: "37signals") + assert_includes account.inspect, 'name: "slangis73"' + end + test "filter_attributes could be overwritten by models" do Admin::Account.all.each do |account| assert_includes account.inspect, "name: [FILTERED]" @@ -37,7 +64,6 @@ class FilterAttributesTest < ActiveRecord::TestCase end begin - previous_account_filter_attributes = Admin::Account.filter_attributes Admin::Account.filter_attributes = [] # Above changes should not impact other models @@ -51,7 +77,7 @@ class FilterAttributesTest < ActiveRecord::TestCase assert_equal 0, account.inspect.scan("[FILTERED]").length end ensure - Admin::Account.filter_attributes = previous_account_filter_attributes + Admin::Account.remove_instance_variable(:@filter_attributes) end end @@ -63,6 +89,18 @@ class FilterAttributesTest < ActiveRecord::TestCase assert_equal 0, account.inspect.scan("[FILTERED]").length end + test "filter_attributes should handle [FILTERED] value properly" do + begin + User.filter_attributes = ["auth"] + user = User.new(token: "[FILTERED]", auth_token: "[FILTERED]") + + assert_includes user.inspect, "auth_token: [FILTERED]" + assert_includes user.inspect, 'token: "[FILTERED]"' + ensure + User.remove_instance_variable(:@filter_attributes) + end + end + test "filter_attributes on pretty_print" do user = admin_users(:david) actual = "".dup @@ -81,4 +119,18 @@ class FilterAttributesTest < ActiveRecord::TestCase assert_not_includes actual, "name: [FILTERED]" assert_equal 0, actual.scan("[FILTERED]").length end + + test "filter_attributes on pretty_print should handle [FILTERED] value properly" do + begin + User.filter_attributes = ["auth"] + user = User.new(token: "[FILTERED]", auth_token: "[FILTERED]") + actual = "".dup + PP.pp(user, StringIO.new(actual)) + + assert_includes actual, "auth_token: [FILTERED]" + assert_includes actual, 'token: "[FILTERED]"' + ensure + User.remove_instance_variable(:@filter_attributes) + end + end end diff --git a/activesupport/lib/active_support/parameter_filter.rb b/activesupport/lib/active_support/parameter_filter.rb index 59945e9daaa..1389d825233 100644 --- a/activesupport/lib/active_support/parameter_filter.rb +++ b/activesupport/lib/active_support/parameter_filter.rb @@ -28,22 +28,36 @@ module ActiveSupport class ParameterFilter FILTERED = "[FILTERED]" # :nodoc: - def initialize(filters = []) + # Create instance with given filters. Supported type of filters are +String+, +Regexp+, and +Proc+. + # Other types of filters are treated as +String+ using +to_s+. + # For +Proc+ filters, key, value, and optional original hash is passed to block arguments. + # + # ==== Options + # + # * :mask - A replaced object when filtered. Defaults to +"[FILTERED]"+ + def initialize(filters = [], mask: FILTERED) @filters = filters + @mask = mask end + # Mask value of +params+ if key matches one of filters. def filter(params) compiled_filter.call(params) end + # Returns filtered value for given key. For +Proc+ filters, third block argument is not populated. + def filter_param(key, value) + @filters.empty? ? value : compiled_filter.value_for_key(key, value) + end + private def compiled_filter - @compiled_filter ||= CompiledFilter.compile(@filters) + @compiled_filter ||= CompiledFilter.compile(@filters, mask: @mask) end class CompiledFilter # :nodoc: - def self.compile(filters) + def self.compile(filters, mask:) return lambda { |params| params.dup } if filters.empty? strings, regexps, blocks = [], [], [] @@ -65,42 +79,46 @@ module ActiveSupport regexps << Regexp.new(strings.join("|"), true) unless strings.empty? deep_regexps << Regexp.new(deep_strings.join("|"), true) unless deep_strings.empty? - new regexps, deep_regexps, blocks + new regexps, deep_regexps, blocks, mask: mask end attr_reader :regexps, :deep_regexps, :blocks - def initialize(regexps, deep_regexps, blocks) + def initialize(regexps, deep_regexps, blocks, mask:) @regexps = regexps @deep_regexps = deep_regexps.any? ? deep_regexps : nil @blocks = blocks + @mask = mask end def call(params, parents = [], original_params = params) filtered_params = params.class.new params.each do |key, value| - parents.push(key) if deep_regexps - if regexps.any? { |r| key =~ r } - value = FILTERED - elsif deep_regexps && (joined = parents.join(".")) && deep_regexps.any? { |r| joined =~ r } - value = FILTERED - elsif value.is_a?(Hash) - value = call(value, parents, original_params) - elsif value.is_a?(Array) - value = value.map { |v| v.is_a?(Hash) ? call(v, parents, original_params) : v } - elsif blocks.any? - key = key.dup if key.duplicable? - value = value.dup if value.duplicable? - blocks.each { |b| b.arity == 2 ? b.call(key, value) : b.call(key, value, original_params) } - end - parents.pop if deep_regexps - - filtered_params[key] = value + filtered_params[key] = value_for_key(key, value, parents, original_params) end filtered_params end + + def value_for_key(key, value, parents = [], original_params = nil) + parents.push(key) if deep_regexps + if regexps.any? { |r| r.match?(key) } + value = @mask + elsif deep_regexps && (joined = parents.join(".")) && deep_regexps.any? { |r| r.match?(joined) } + value = @mask + elsif value.is_a?(Hash) + value = call(value, parents, original_params) + elsif value.is_a?(Array) + value = value.map { |v| v.is_a?(Hash) ? call(v, parents, original_params) : v } + elsif blocks.any? + key = key.dup if key.duplicable? + value = value.dup if value.duplicable? + blocks.each { |b| b.arity == 2 ? b.call(key, value) : b.call(key, value, original_params) } + end + parents.pop if deep_regexps + value + end end end end diff --git a/activesupport/test/parameter_filter_test.rb b/activesupport/test/parameter_filter_test.rb index 3403a3188b4..d2dc71061d9 100644 --- a/activesupport/test/parameter_filter_test.rb +++ b/activesupport/test/parameter_filter_test.rb @@ -36,6 +36,51 @@ class ParameterFilterTest < ActiveSupport::TestCase end end + test "filter should return mask option when value is filtered" do + mask = Object.new.freeze + test_hashes = [ + [{ "foo" => "bar" }, { "foo" => "bar" }, %w'food'], + [{ "foo" => "bar" }, { "foo" => mask }, %w'foo'], + [{ "foo" => "bar", "bar" => "foo" }, { "foo" => mask, "bar" => "foo" }, %w'foo baz'], + [{ "foo" => "bar", "baz" => "foo" }, { "foo" => mask, "baz" => mask }, %w'foo baz'], + [{ "bar" => { "foo" => "bar", "bar" => "foo" } }, { "bar" => { "foo" => mask, "bar" => "foo" } }, %w'fo'], + [{ "foo" => { "foo" => "bar", "bar" => "foo" } }, { "foo" => mask }, %w'f banana'], + [{ "deep" => { "cc" => { "code" => "bar", "bar" => "foo" }, "ss" => { "code" => "bar" } } }, { "deep" => { "cc" => { "code" => mask, "bar" => "foo" }, "ss" => { "code" => "bar" } } }, %w'deep.cc.code'], + [{ "baz" => [{ "foo" => "baz" }, "1"] }, { "baz" => [{ "foo" => mask }, "1"] }, [/foo/]]] + + test_hashes.each do |before_filter, after_filter, filter_words| + parameter_filter = ActiveSupport::ParameterFilter.new(filter_words, mask: mask) + assert_equal after_filter, parameter_filter.filter(before_filter) + + filter_words << "blah" + filter_words << lambda { |key, value| + value.reverse! if key =~ /bargain/ + } + filter_words << lambda { |key, value, original_params| + value.replace("world!") if original_params["barg"]["blah"] == "bar" && key == "hello" + } + + parameter_filter = ActiveSupport::ParameterFilter.new(filter_words, mask: mask) + before_filter["barg"] = { :bargain => "gain", "blah" => "bar", "bar" => { "bargain" => { "blah" => "foo", "hello" => "world" } } } + after_filter["barg"] = { :bargain => "niag", "blah" => mask, "bar" => { "bargain" => { "blah" => mask, "hello" => "world!" } } } + + assert_equal after_filter, parameter_filter.filter(before_filter) + end + end + + test "filter_param" do + parameter_filter = ActiveSupport::ParameterFilter.new(["foo", /bar/]) + assert_equal "[FILTERED]", parameter_filter.filter_param("food", "secret vlaue") + assert_equal "[FILTERED]", parameter_filter.filter_param("baz.foo", "secret vlaue") + assert_equal "[FILTERED]", parameter_filter.filter_param("barbar", "secret vlaue") + assert_equal "non secret value", parameter_filter.filter_param("baz", "non secret value") + end + + test "filter_param can work with empty filters" do + parameter_filter = ActiveSupport::ParameterFilter.new + assert_equal "bar", parameter_filter.filter_param("foo", "bar") + end + test "parameter filter should maintain hash with indifferent access" do test_hashes = [ [{ "foo" => "bar" }.with_indifferent_access, ["blah"]], @@ -48,4 +93,13 @@ class ParameterFilterTest < ActiveSupport::TestCase parameter_filter.filter(before_filter) end end + + test "filter_param should return mask option when value is filtered" do + mask = Object.new.freeze + parameter_filter = ActiveSupport::ParameterFilter.new(["foo", /bar/], mask: mask) + assert_equal mask, parameter_filter.filter_param("food", "secret vlaue") + assert_equal mask, parameter_filter.filter_param("baz.foo", "secret vlaue") + assert_equal mask, parameter_filter.filter_param("barbar", "secret vlaue") + assert_equal "non secret value", parameter_filter.filter_param("baz", "non secret value") + end end diff --git a/railties/test/application/configuration_test.rb b/railties/test/application/configuration_test.rb index 9b01d42b1e6..fa418f564b7 100644 --- a/railties/test/application/configuration_test.rb +++ b/railties/test/application/configuration_test.rb @@ -2109,7 +2109,7 @@ module ApplicationTests RUBY app "development" assert_equal [ :password, :credit_card_number ], Rails.application.config.filter_parameters - assert_equal [ "password", "credit_card_number" ].to_set, ActiveRecord::Base.filter_attributes + assert_equal [ :password, :credit_card_number ], ActiveRecord::Base.filter_attributes end test "ActiveStorage.routes_prefix can be configured via config.active_storage.routes_prefix" do