Filter params on Rails 3.1 when calling notify_airbrake #59

Merged
merged 1 commit into from Feb 27, 2012

Conversation

Projects
None yet
4 participants
@benjaminoakes
Contributor

benjaminoakes commented Feb 23, 2012

We recently noticed that Airbrake doesn't always filter our parameters.

I looked into the cases, and eventually narrowed it down to this: an unhandled exception is filtered properly, but a notification made via notify_airbrake does not.

I traced the problem to Airbrake::Rails::ControllerMethods#airbrake_filter_if_filtering:

def airbrake_filter_if_filtering(hash)
  return hash if ! hash.is_a?(Hash)

  if respond_to?(:filter_parameters)
    filter_parameters(hash) rescue hash
  else
    hash
  end
end

After doing some research, I found that filter_parameters is a Rails 2 method that has since been replaced by ActionDispatch::Http::ParameterFilter. There are other methods used in Airbrake are deprecated by Rails 3. For example filter_parameter_logging (found in test/catcher_test.rb) disappeared after Rails 2.3.8.

I attempted to add a test for my changes, but couldn't reproduce the problem using the available helpers in test/catcher_test.rb, although the problem clearly exists in our production use. As near as I can tell process_action_with_manual_notification may not actually be testing in a realistic way; there seem to be at least two ways that Airbrake filters parameters. If I'm missing something, please point me in the right direction.

At any rate, the changes in this pull request fixes the problem when I tested on a staging environment. Our app is using Ruby 1.9.2p180 and Rails 3.1.2 at the time of this writing.

@benarent

This comment has been minimized.

Show comment Hide comment
@benarent

benarent Feb 24, 2012

Contributor

Thanks Benjamin. This seems reasonable. I'll get a dev to test the merge and will update accordingly.

Contributor

benarent commented Feb 24, 2012

Thanks Benjamin. This seems reasonable. I'll get a dev to test the merge and will update accordingly.

shime added a commit that referenced this pull request Feb 27, 2012

Merge pull request #59 from benjaminoakes/master
Filter params on Rails 3.1 when calling notify_airbrake

@shime shime merged commit ad9ffed into airbrake:master Feb 27, 2012

@shime

This comment has been minimized.

Show comment Hide comment
@shime

shime Feb 27, 2012

Contributor

Yes, this method has been deprecated. Thanks for fixing this, Benjamin! 👍

Contributor

shime commented Feb 27, 2012

Yes, this method has been deprecated. Thanks for fixing this, Benjamin! 👍

@benjaminoakes

This comment has been minimized.

Show comment Hide comment
@benjaminoakes

benjaminoakes Feb 27, 2012

Contributor

Thanks for merging and maintaining Airbrake!

Contributor

benjaminoakes commented Feb 27, 2012

Thanks for merging and maintaining Airbrake!

@nevernormal1

This comment has been minimized.

Show comment Hide comment
@nevernormal1

nevernormal1 May 2, 2012

This bug was happening for us, too, and the parameters that were not getting filtered out were highly sensitive. In fact, the reason apps would filter parameters in the first place is because they are sensitive. We noticed that no gem update has been made since this issue was fixed. We highly recommend pushing a new version of the gem to rubygems before this causes some big problems for someone. In other words, this seems like a critical issue to push out an update for immediately.

This bug was happening for us, too, and the parameters that were not getting filtered out were highly sensitive. In fact, the reason apps would filter parameters in the first place is because they are sensitive. We noticed that no gem update has been made since this issue was fixed. We highly recommend pushing a new version of the gem to rubygems before this causes some big problems for someone. In other words, this seems like a critical issue to push out an update for immediately.

@benjaminoakes

This comment has been minimized.

Show comment Hide comment
@benjaminoakes

benjaminoakes May 2, 2012

Contributor

@nevernormal1 Agreed. I'm surprised there has not been a release.

Contributor

benjaminoakes commented May 2, 2012

@nevernormal1 Agreed. I'm surprised there has not been a release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment