Support parameter filtering based on allow lists

[p8]

Summary

ActiveSupport::ParameterFilter currently works as a deny list: you specify the parameters that should be filtered.

Rails.application.config.filter_parameters += [:passw]

If you want use an allow list and specify the parameters that should be allowed you could do it with a lambda. For example:

Rails.application.config.filter_parameters += [
  lambda { |key, value|
    # filter all parameters that aren't :id
    value.replace('[FILTERED]') unless key.in?(:id)
  }
]

But this can be error prone. For example this doesn't work if a value is nil.

This change makes it possible to list the parameters that are allowed. All other parameters will be masked.

For example, to only allow primary keys and foreign keys you could configure the following:

Rails.application.config.allow_parameters = [:id, /_id\z/]

allow_parameters takes precedence over filter_parameters. So if the allow_parameters are defined filter_parameters will be ignored.

The majority of the tests are based on the deny list equivalent:

• The RequestParameterAllowFilter test is based on RequestParameterFilter test.
• activerecord/test/cases/allow_attributes_test.rb is based on activerecord/test/cases/filter_attributes_test.rb

The current implementation of ParameterAllowFilter supports all features from ParameterFilter, including lambdas. But maybe it should be simplified instead by removing lambda support.

[p8]

Thanks for reviewing this!

Enhancing lambdas would be a less drastic change, and I don't think we need to support defining allowed parameters with nested keys like: "credit_card.code" or lambdas to modify the values.

Another option for lambdas could be adding the case for arity of 1, which would allow us to rely on the return value. As we only need to look at the key, the API for lambdas becomes more similar to passing string and regexs [/foo/, "bar"]: filter the params if it matches or returns true.

Rails.application.config.filter_parameters += [
  lambda { |key| !(key == :id || key.match?(/_id\z/) }
]

blocks.each do |b|
   case b.arity
   when 1 
     value = @mask if b.call(key)
   when 2
     b.call(key, value)
   when 3
     b.call(key, value, original_params)
   end
end

I won't have time to work on this PR for the coming weeks, but I'm happy to continue in August.

[jonathanhefner]

That could work too. Though branching on arity == 1 effectively creates two "classes" of callables — one where the return value matters and one where it doesn't. Also, branching on arity can be tricky to get right:

proc { |key = "default_key"| }.arity
# => 0

lambda { |key = "default_key"| }.arity
# => -1

proc { |key, value = "ignore me"| }.arity
# => 1

lambda { |key, value = "ignore me"| }.arity
# => -2

proc { |key, *| }.arity
# => -2

lambda { |key, *| }.arity
# => -2

proc { |*args| key = args.first }.arity
# => -1

lambda { |*args| key = args.first }.arity
# => -1

[p8]

Hey @jonathanhefner, Found some time to continue with this...

The current implementation already does some branching on arity, although this doesn't change behaviour in the branches: https://github.com/rails/rails/blob/f95c0b7e96eb36bc3efc0c5beffbb9e84ea664e4/activesupport/lib/active_support/parameter_filter.rb#L131

Also ActiveSupport::Notifications.subscribe will return an event object for a single argument proc or methods on that event for multi argument procs: https://guides.rubyonrails.org/active_support_instrumentation.html#subscribing-to-an-event But we do some Ruby black magic there it seems, and I'm not sure we want to go there. https://github.com/rails/rails/blob/main/activesupport/lib/active_support/notifications/fanout.rb#L323-L325

My main concern with the following is that I think it's a bit difficult to reason about with the negation of the match. And I'm not sure many developers are familiar with the second argument to throw.

Rails.application.config.filter_parameters += [
  lambda { |key, value| throw :filter, !key.match?(/(\Aid|_id)\z/) }
]

Maybe inverting it makes it simpler?

Rails.application.config.filter_parameters += [
  lambda { |key, value| throw :allow, key.match?(/(\Aid|_id)\z/) }
]

Another option could be a new config setting to enable looking at the return value of the proc. This would not be a new default, but would allow apps to use it if they want to do allow lists.

[jonathanhefner]

Maybe inverting it makes it simpler?

Rails.application.config.filter_parameters += [
  lambda { |key, value| throw :allow, key.match?(/(\Aid|_id)\z/) }
]

Syntactically, I like it. But it implies that the key will be allowed / not filtered regardless of any subsequent filters, and I think that might be difficult to support, because procs are grouped together and processed separately from the other filters.

Thinking about this more, I notice that the example with id and _id could also be implemented with a negative lookahead or lookbehind regexp:

Rails.application.config.filter_parameters = [/\A(?!id\z|.*_id\z)/]
# OR
Rails.application.config.filter_parameters = [/(?<!\Aid|_id)\z/]

Though I'm not sure whether other examples would be as simple. Do you have additional examples of "allow only" filters?