filters/keys_filter: add support for filtering using procs

[kyrylo]

No description provided.

[vmihailenco]

Was there a feature request?

I think this should not go in. Loading keys from database is not typical at all and if user really wants this he can write such filter himself.

[kyrylo]

To whom it may concern: as @vmihailenco points out, you could use the add_filter API instead. Bear in mind that with this approach query parameters are not supported:

class MyFilter
  def call(notice)
    notice[:params] = FilteredParameters.filter(notice[:params])
  end
end

Airbrake.add_filter(MyFilter.new)

We decided not to introduce this feature, but if anybody reading this wants to revive this discussion, feel free to ping me. You're more than welcome 👍

[vmihailenco]

Bear in mind that with this approach query parameters are not supported

You can achieve this with some refactoring and inheritance, e.g.

class MyFilter extends airbrake.KeysFilter
  def blacklist_keys:
    return [foo, bar]

That assumes that airbrake.KeysFilter uses blacklist_keys method to retrieve list of blacklisted keys.

[dquimper]

I'm little late to the party here. @kyrylo introduced this for me (customer). So here's my two cents on the subject.

Airbrake should integrate with Rails features. To me, that should mean Airbrake automatically filtering the same params as Rails does for it's params. Rails.application.config.filter_parameters. That would be the ideal integration.

Managing a separate blacklist and whitelist isn't as ideal as you always have to remember to update those list too. Ideally, Airbrake should cover the same functionality as Rails's filter_parameters and support lambda so I could just plug on into the other:

c.blacklist_keys = Rails.application.config.filter_parameters

In my case, my list is dynamic. That requires requires the application to be loaded before I can set these value. The add_filter worked for me, and that's why I'm so late to argue here, but it forces me to put that code outside of Airbrake's initializer. It isn't as clean. Another drawback as @vmihailenco points out, is that it only works for params and not for those same params in the query string.

If I compare Airbrake and New Relic. Both are external resources which handles potentials filtered params. I didn't have to do anything for New Relics to filter my params.

From one of their discussion board:

the New Relic Ruby agent uses the Rails filter_parameters config setting to filter out particular parameters

How about replacing the Airbrake's blanklist_keys and whitelist_keys with:

c.filter_parameters = Rails.application.config.filter_parameters

Thanks for your time on this. I know filter_parameters probably isn't a feature used frequently. But when it is used, in our case for HIPAA concerns, it's an essential feature.

[kyrylo]

Airbrake should integrate with Rails features. To me, that should mean Airbrake automatically filtering the same params as Rails does for it's params. Rails.application.config.filter_parameters. That would be the ideal integration.

I like this idea. We should keep in mind that this gem is Rails-free, though. So here we try to establish an API for https://github.com/airbrake/airbrake, which would configure this. We also don't want Rails to affect our naming choices here, since this gem must support any Ruby program, too.

In my case, my list is dynamic. That requires requires the application to be loaded before I can set these value.

Does the proposed solution work for you, at least (technically)? I restored the branch, could you please give it a try? Just try to add a proc filter (instead of your current solution) to test. If this works good, then we could continue discussing next steps.

If I compare Airbrake and New Relic. Both are external resources which handles potentials filtered params. I didn't have to do anything for New Relics to filter my params.

Thanks for the tip. I'll take a look at how it works. I definitely agree that we can improve this and integrate better with Rails. We just need a solid ground in this gem, first.

[dquimper]

Thanks! I'll test it today.

Since Rails is the most use framework, some gem automatically gets their configuration automatically out of Rails. But still offer a way to overwrite them in their own config. What way, people using Sinatra or others aren't not left out.

Or you could have this line commented in your default config file:

# c.filter_parameters = Rails.application.config.filter_parameters

So people using Rails can just uncomment it.

[dquimper]

Hi @kyrylo
It works!

My config only has:

c.blacklist_keys = [lambda { FilteredParameters.model_sensitive_attributes }]

And both the params and the query script was [Filtered].

The syntax is a little strange looking with a lambda in an array. But I prefer this than using the add_filter.

Thanks

[dquimper]

Can I keep using this or should I revert my code?
Is this going to be included in a future release?

[kyrylo]

I need more time to think this through. At the moment this API is not fully compatible with Rails.application.config.filter_parameters because Rails' API works differently (and I dislike the way it works). In Rails you are forced to filter manually with sub! or whatever code you want to use, but in our library we do this bit for you, you just need to provide us the keys (which I think it much cleaner).

[dquimper]

Rails doesn't force you to use sub!. You can just do:

config.filter_parameters += [:password, :password_confirmation, :credit_card]

I believe they use [FILTERED] instead of [Filtered]

[kyrylo]

I understand, and as of now this API should already be working (although I haven't tested), thus...

c.blacklist_parameter = Rails.application.config.filter_parameters

...should just work. But I was talking about the lambda API. Rails' API wants lambdas to yield a key and a value, while our API doesn't need this. There's a chance that Rails.application.config.filter_parameters already is compatible.

@dquimper could you please verify this by using your app and this branch (so basically, just try to configure your initializer like you described).

[dquimper]

I tried with:

c.blacklist_keys = Rails.application.config.filter_parameters

And it doesn't work. None of the params are filtered or is the query string.

[dquimper]

c.blacklist_parameter = Rails.application.config.filter_parameters

Crashes with:

NoMethodError: undefined method `blacklist_parameter=' for #<Airbrake::Config:0x007fe3631d5148>

[sgray]

I tried with:

c.blacklist_keys = Rails.application.config.filter_parameters
And it doesn't work. None of the params are filtered or is the query string.

@dquimper my guess is this didn't work because the Airbrake initializer was run before the filter_parameter_logging.rb initializer. When it hit the Airbrake initializer, Rails.application.config.filter_parameters was still just an empty array. Rails runs those in alphabetical order, so renaming these to have the Airbrake initializer come after filter_parameter_logging.rb should work fine.

[dquimper]

I retried:

c.blacklist_keys = Rails.application.config.filter_parameters

I added a require at the top to make sure filter_parameter_logging is loaded first.
When an exception is generated, I get a wrong number of arguments (0 for 2) error.
On Rails.application.config.filter_parameters << lambda do |k,v|

Rails.application.config.filter_parameters returns a lambda which takes 2 params.

I also tried:

c.blacklist_keys = FilteredParameters.model_sensitive_attributes

Without the lambda. It doesn't work as I have to wait until the full Rails is loaded before I can read FilteredParameters.model_sensitive_attributes. Simply changing the other of the initializers won't work.

[kyrylo]

@dquimper could you show me how you configure your filter_parameter filters (originally)?

I need to recreate your setup, so I can figure out how to debug this.

[dquimper]

I use this initializer:

class FilteredParameters
  def self.model_sensitive_attributes
    @@RAILS_FILTER_PARAMS ||= begin
      attr = ActiveRecord::Base.descendants.
        map { |m| m.encrypted_attributes.keys }.
        flatten.compact.uniq.map(&:to_s)
      attr << "password"
      attr << "password_confirmation"
      attr
    end
  end

  ....
end

Rails.application.config.filter_parameters << lambda do |k,v|
  Rails.application.eager_load!
  if FilteredParameters.model_sensitive_attributes.include?(k.to_s)
    v.sub!(/.*/,"[Filtered]")
  end
end

encrypted_attributes returns a list of attributes which all start with encrypted_* (ie: encrypted_first_name)

ActiveRecord::Base.descendants only returns the full list of models when the application is fully loaded.

[kyrylo]

Thanks for the config. It seems like there's a way to make it work. Try to replace lambda with proc and make sure your config returns an array of keys:

# config/initializers/1_filter_parameter_logging.rb
Rails.application.config.filter_parameters << proc do |k,v| # <------------ PROC
  Rails.application.eager_load!
  if FilteredParameters.model_sensitive_attributes.include?(k.to_s)
    v.sub!(/.*/,"[Filtered]")
  end
  FilteredParameters.model_sensitive_attributes # <----------- NEW
end

Could please confirm that the above code works for you?

[kyrylo]

The code above works for me, so I still think this PR adds enough support for the feature you wish to use. There will be a few restrictions, though. First of all, you must use proc instead of lambda. This eliminates the difference between our and Rails' API. We don't yield any arguments, while Rails does that. Next, the proc must return the keys to be filtered (Rails filters don't rely on return values). With these simple rules this should just work:

c.blacklist_keys = Rails.application.config.filter_parameters

[kyrylo]

I am going to release a new version of the gem today, but unfortunately I cannot include this patch into the release because I need your assistance here. So far nobody else has requested this feature, so if you could continue helping, we might be able to make this production ready. Cheers!

[kyrylo]

Please ping me if you are still interested in this. I'm sure we'll find a solution.

[dquimper]

It doesn't work, the values are not filtered.

The syntax here is strange here. We're doing two things in the same proc:
Modifying the params and returning a list.

I prefer the previous syntax where we set Rails.application.config.filter_parameters and c.blacklist_keys separately. It's not ideal, but it's less confusing.

[kyrylo]

The syntax here is strange here. We're doing two things in the same proc:
Modifying the params and returning a list.

I'm not quite sure what's strange here. It's definitely not:

c.blacklist_keys = FilteredParameters.model_sensitive_attributes

...but it's my best suggestion so far.

Unfortunately, it won't be possible to implement the "canonical" version due to the differences between our and Rails' procs, which I outlined in earlier posts. I am pretty sure that this example should work just fine. Just to be on the same page, I'm sharing this repository to prove my point: https://github.com/kyrylo/airbrake-ruby-issue108

It uses this branch and it is configured to filter out some Environment values (HTTP method and headers). Please clone it and configure your own Airbrake ID & Key, so you could see it being filtered.

I think the repository replicates your setup. If not, let me know what is different for you.

[dquimper]

The strange syntax is here:

Rails.application.config.filter_parameters << proc do |k,v| # <------------ PROC
  Rails.application.eager_load!
  if FilteredParameters.model_sensitive_attributes.include?(k.to_s)
    v.sub!(/.*/,"[Filtered]")
  end
  FilteredParameters.model_sensitive_attributes # <----------- NEW
end

Because we method has two purpose. Maybe I'm been too "by the book" over this, but it feels wrong because the if statement doesn't affect the output of the method.

This should work fine.

c.blacklist_keys = FilteredParameters.model_sensitive_attributes

I tried:

c.blacklist_keys = Rails.application.config.filter_parameters

I don't know why, but the proc doesn't seem to do anything. The params aren't filtered.

I currently have:

c.blacklist_keys = [lambda { FilteredParameters.model_sensitive_attributes }]

And it works fine. Let's leave it at this.

[dquimper]

Its going to take me a while to find the time to test your repo. I'll let you know when I do.

[kyrylo]

The strange syntax is here:

It's not really that strange if you take a closer look. It's exactly the same filter, with just 1 line added.

I currently have:
And it works fine. Let's leave it at this.

Sure, whichever way works for you. I haven't made any additional changes ever since I introduced this PR. It's all the same code and it's up to you if you want to use additional procs or return procs from Rails' param filters. Either way is supported in this PR (it's essentially the same thing).

Its going to take me a while to find the time to test your repo. I'll let you know when I do.

This is fine, you can ping me whenever you have the time. It's really important for me to know why the keys are not filtered with your setup. So, I hope this test repo will help us to get closer to your production setup.

[dquimper]

I tried your branch. It works on it. I removed the 1_ and 2_ from the file names and replaced them with a require at the top of airbrake.rb. That had the same effect (loading order is the same) and it works too.

I did the same changes to my code and it also works. I think you can merge this.

[kyrylo]

I slightly amended the docs, so they are less specific (not mention the contrived DB example). I am not particularly happy about the code and how it integrates, but it's not extremely terrible. This is why I think this should go in, though:

1. Solves HIPAA concerns
2. Adds full support for Rails.application.config.filter_parameters (but imposes certain rules with regard to how to write filters, described in README: explain how to integrate Rails' filter_parameters airbrake#606)