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

Support deep keys #137

Closed
grosser opened this issue Dec 12, 2016 · 6 comments
Closed

Support deep keys #137

grosser opened this issue Dec 12, 2016 · 6 comments

Comments

@grosser
Copy link
Contributor

grosser commented Dec 12, 2016

rails filter_parameters allows foo.bar meaning foo[bar]
which is not compatible with airbrakes blacklist filter
so please either:

  • support it (might be tricky, but a useful filter)
  • change airbrake rails docs to point this out
@kyrylo
Copy link
Contributor

kyrylo commented Dec 12, 2016

Thanks for filing this! I think we already support that on notifier level (if you filter just foo, it'll be filtered everywhere, even in:

{
  not_foo: {
    not_foo_again: {
      foo: 'will be filtered'
    }
  }
}

But in this regard our API is probably indeed incompatible. I guess it's better to document this and ignore bar in foo[bar]. That said, it's possible to filter both, foo and bar (globally, like we do as of now), but it might be risky/unwanted.

@grosser
Copy link
Contributor Author

grosser commented Dec 12, 2016

doing this atm ... just a evil trap that the docs say 'put filter into blacklist' and then they do not work the same way ... which ends up leaking secrets into airbrake ... not fun :(

@kyrylo
Copy link
Contributor

kyrylo commented Dec 13, 2016

Sorry about that. I was simply unaware of the foo.bar syntax. It seems like it's a Rails 5 feature.

@grosser
Copy link
Contributor Author

grosser commented Dec 13, 2016

would you accept a PR to add this feature ? ... I think it's pretty useful :)

@kyrylo
Copy link
Contributor

kyrylo commented Dec 13, 2016

Depends on the implementation. If there's a neat way to implement this, then why not? However our current policy is to filter out every matching key, no matter how deep it is. We also support whitelists. This might make the implementation harder. Without special cases the code for truncation is nice, fast and tidy.

TL;DR Go for it, but it might be harder than you think :P
P.S. But probably not too hard

@grosser
Copy link
Contributor Author

grosser commented Dec 14, 2016

took a look ... would mean rewriting filtering logic ... and make it a bunch slower ... so not doing it ...

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

No branches or pull requests

2 participants