Scrub args alternate implementation #95
Conversation
|
|
||
| # Add any varargs | ||
| if arginfo.varargs is not None: | ||
| args.extend(local_vars[arginfo.varargs]) | ||
| if SETTINGS['locals']['scrub_varargs']: |
There was a problem hiding this comment.
*args could contain anything, including passwords so the safest thing is to scrub by default
|
Thanks for starting up this convo. I'll summarize to see if I understand the intent:
I think this is a great idea. There are a few implementation details here that we need to fix before we can merge this:
I like that you're using |
|
|
||
| # Add any varargs | ||
| if arginfo.varargs is not None: | ||
| args.extend(local_vars[arginfo.varargs]) | ||
| if SETTINGS['locals']['scrub_varargs']: | ||
| args[arginfo.varargs] = ['*' * randint(3, 20) for i in range(len(local_vars[arginfo.varargs]))] |
There was a problem hiding this comment.
'*' * randint() here feels janky but it's not obvious to me what the better solution is
There was a problem hiding this comment.
You should see if it's possible to use ScrubTransform here.
There was a problem hiding this comment.
Alright, this may be a bit more complicated than I anticipated. I'm going to make a PR to this branch containing the Scrub code.
|
I've fixed the py2.6 issue and merged master but I'm not sure I understand the safe_repr comment. I think that this |
|
Hey @sibson, I'll be working on this with you in the coming week. Looking forwards to merging it! |
|
@sibson can you pull in changes from master again? |
|
@sibson, we had a discussion after reviewing your PR and I think we have a way to both properly scrub args and simplify the notifier code. Basically, args would become a list of argument names sourced from ArgSpec. We would substitute argument names for argument values for in Rollbar's UI by sourcing the values from locals (which come from ArgInfo). The Rollbar UI won't actually change but the data format will (since we are going to deprecate “args” and add “argspec”). The plan as it stands would take a bump of pyrollbar's minor version and a completely new branch without any of your commits. Will this address your original PR? |
|
@ezarowny let me rephrase to see if I understand.
If that is correct it seems reasonable. A possible gotcha is what happens if I've set locals=False in my config? I suppose just displaying the argname is acceptable. |
|
Yep, that's correct! I think what will happen in this case is that we will not include locals which means the UI will not have any args or locals in it. I'd have to double check that though. |
|
I think what would happen is that we wouldn't inspect the frame at all if locals.enabled was False. Only the trace would be reported. |
|
After clarification, would the proposed changes still address your original PR? |
|
yes I think it addresses this PR. The grandfather of this was #91, where the issue around str(exc) is still outstanding, https://github.com/rollbar/pyrollbar/pull/91/files#diff-1fcbac3a897889dbaf8984fdfe14c99cR633. A proposed solution involved #67 but I don't think it would be secure by default as it stands. |
|
Closing in favor of #113 |
Based on discussion on https://github.com/rollbar/pyrollbar/pull/91/files#r54032504 I'm submitting this as a discussion piece. I expect it will change how args are displayed in rollbar.