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

Ensure params filtering take precedence over hash/array objects #567

Merged

Conversation

carlosantoniodasilva
Copy link
Contributor

This matches the current Rails implementation of parameter filtering, where it is possible to set a filter that will hide an entire array or nested hash, instead of just the inner keys.

Consider the following params payload:

    user: { username: "foo", password: "bar" }

Currently we can set a filter for both "username" and "password", but setting for the entire "user" params hash does not work, it's simply ignored, because we parse hashes separately looking at each individual key first - before checking the configured filter.

With this change, setting a filter for "user" would effectively hide it from the payload, scrubbing the field as expected, since we first check the filter before proceeding with other processing.


Unrelated: I'm getting this failure locally on both master and this branch:

  1) Rollbar project_gems should handle regex gem patterns
     Failure/Error: gem_paths.length.should > 1

       expected: > 1
            got:   1
     # ./spec/rollbar_spec.rb:1173:in `block (3 levels) in <top (required)>'

I haven't stopped to check it properly, but doesn't seem to be related to the changes here since it's also happening on master.

Let me know if I can help with anything!

@jondeandres
Copy link
Contributor

Hi @carlosantoniodasilva, thanks for the PR. It's quite good at all!

Tests are failing in Travis because a problem between our gemfiles and changes in rubygems. We'll fix master and merge a new PR with your commits, I think that will be the easiest.

@carlosantoniodasilva
Copy link
Contributor Author

@jondeandres sounds good, thanks! If you want I can also merge master back here when it's resolved there, just let me know.

@rokob
Copy link
Contributor

rokob commented Feb 24, 2017

I just pushed a fix for the tests to master. Could you merge master back into your branch?

This matches the current Rails implementation of parameter filtering,
where it is possible to set a filter that will hide an entire array or
nested hash, instead of just the inner keys.

Consider the following params payload:

    user: { username: "foo", password: "bar" }

Currently we can set a filter for both "username" and "password", but
setting for the entire "user" params hash does not work, it's simply
ignored, because we parse hashes separately looking at each individual
key first.

With this change, setting a filter for "user" would effectively hide it
from the payload, scrubbing the field as expected.
We do not rely on the result of the matching, or any part of it, just
whether it matches or not, so === is preferable as it's slightly faster
by not creating extra matching objects.

Also remove the condition for nil as it responds to === and always
returns false.

    >> nil === 'zomg'
    => false
@carlosantoniodasilva
Copy link
Contributor Author

@rokob thanks, I've rebased from master and CI is running again.

@rokob
Copy link
Contributor

rokob commented Feb 24, 2017

This looks good to me, thanks!

@rokob rokob merged commit 4f615b0 into rollbar:master Feb 24, 2017
@carlosantoniodasilva carlosantoniodasilva deleted the fix-param-filter-array-hash branch February 25, 2017 13:24
@tt
Copy link

tt commented Mar 14, 2017

@rokob, can you release this?

@rokob
Copy link
Contributor

rokob commented Mar 14, 2017

Yeah I will try to get a release out today.

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

Successfully merging this pull request may close these issues.

4 participants