Obey safe repr for exceptions #91
Conversation
otherwise they can leak sensitive information
scrub call args if name is in scrub fields
|
Seems reasonable to me. |
Obey safe repr for exceptions
|
Thanks @sibson ! |
| @@ -864,7 +864,7 @@ def _add_locals_data(data, exc_info): | |||
| # Fill in all of the named args | |||
| for named_arg in named_args: | |||
| if named_arg in local_vars: | |||
| args.append(local_vars[named_arg]) | |||
| args.append(_transform(local_vars[named_arg], key=(named_arg,))) | |||
There was a problem hiding this comment.
Shouldn't this happen as part of the _serialize_frame_data() call below?
There was a problem hiding this comment.
no, at that point you've lost the important context that the arg was password and only have the content left, which is insufficient for scrubbing. eg https://github.com/rollbar/pyrollbar/pull/91/files#diff-d7fa735556b7ff27de68295083f733d4R754.
I've been experimenting with an alternate solution that may be "cleaner" but has a larger impact on how rollbar is capturing and presenting data, https://github.com/rollbar/pyrollbar/pull/91/files#diff-d7fa735556b7ff27de68295083f733d4R754. I can PR against this repo if you'd like to have more discussion around it.
There was a problem hiding this comment.
That'd be great. Thanks!
There was a problem hiding this comment.
This reverts a pull request we accepted and merged recently (#91). It turns out that that request causes excpetion messages to not be passed along as expected, and that isn't desired behaviour. We're going to attack this problem again soon and come up with a solution that meets the needs of the reverted pull request without causing the undesired side effects.
Exceptions may contain sensitive information in args, thus _transform() should be used rather than str() to ensure safe_repr, and scrub_fields settings are honoured.