Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parameter filter logic for Honeybadger is different from Rails #178

Closed
jasonkim opened this issue Feb 29, 2016 · 4 comments
Closed

Parameter filter logic for Honeybadger is different from Rails #178

jasonkim opened this issue Feb 29, 2016 · 4 comments
Assignees
Milestone

Comments

@jasonkim
Copy link

For string filters, Honeybadger will use string comparison, whereas Rails will convert it into regexp and compare using regex.

In Honeybadger, see line
https://github.com/honeybadger-io/honeybadger-ruby/blob/master/lib/honeybadger/util/sanitizer.rb#L128

In Rails, see lines 24-44 in https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/http/parameter_filter.rb#L24

More specifically, this action dispatch code converts it into regexp

regexps << Regexp.new(strings.join('|'.freeze), true) unless strings.empty?

Let me know if you need more information or if I misunderstood something.

@joshuap
Copy link
Member

joshuap commented Mar 1, 2016

@jasonkim yep, looks like you're right. It would be ideal if our filters behaved the same way as Rails'; I'll have to think about the implications of making a change.

@joshuap
Copy link
Member

joshuap commented Mar 1, 2016

One solution to this could be to separate the Rails filters from ours; if Rails filters are available then we would use an instance of ActionDispatch::Http:: ParameterFilter to apply them in addition to performing our own sanitization.

@jasonkim
Copy link
Author

jasonkim commented Mar 1, 2016

Yeah, I guess I don't really understand the implications of converting everything into regex. Whether that'll cause something unexpected, at least for me, is unknown. If that was possible, it's pretty simple to change it in the initializer.
The solution you proposed is a good compromise. Although the default behaviors for hb and rails are different, it also isn't a breaking change to the existing hb applications.

I would, at some point, deprecate string matching and go with regex only as it should be more robust (like any field called [something]_password should never be exposed). You can also emulate string match with regex by matching on first/last chars.
It's just a suggestion. For now, I've added a separate regex in the yml file to filter out the same keys in my hb config.

@joshuap
Copy link
Member

joshuap commented Mar 1, 2016

Sounds good; agreed on your suggestion to eventually do regexp conversion (I'll think about that some more). I'll plan to move to ActionDispatch::Http:: ParameterFilter for Rails in the next minor release.

@joshuap joshuap added this to the 3.0 milestone Nov 17, 2016
@joshuap joshuap self-assigned this Nov 30, 2016
joshuap pushed a commit that referenced this issue Nov 30, 2016
joshuap pushed a commit that referenced this issue Dec 1, 2016
joshuap pushed a commit that referenced this issue Dec 1, 2016
joshuap pushed a commit that referenced this issue Dec 13, 2016
@joshuap joshuap closed this as completed in 54b1f7d Feb 6, 2017
@stympy stympy removed the in progress label Feb 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants