[SVCS-334] Sentry captures dataverse API tokens on certain error types #313
Conversation
waterbutler/server/sanitize.py
Outdated
| """Subclass the sanitize function of the `SanitizePasswordsProcessor'.""" | ||
|
|
||
| # As of raven version 6.4 this attribute name has been changed from FIELDS to KEYS. | ||
| # Will need to be updated when we upgrade. |
waterbutler/server/sanitize.py
Outdated
|
|
||
| # As of raven version 6.4 this attribute name has been changed from FIELDS to KEYS. | ||
| # Will need to be updated when we upgrade. | ||
| self.FIELDS = self.FIELDS.union(['key', 'token', 'refresh_token']) |
There was a problem hiding this comment.
+1 for using .union on frozenset. I had thought this doesn't work but then I learned that .union on frozenset updates the set and returns a frozenset.
Is there a specific reason why we don't do this in __init__()? The side-effect of doing it insanitize() is that the .union is called every time the function is called. (Note that we have recursion here).
There was a problem hiding this comment.
Good catch, no, no good reason. I will move when ticket next goes to additional development
There was a problem hiding this comment.
Sounds good. Back to Add'l Dev and you.
waterbutler/server/app.py
Outdated
| @@ -44,7 +44,13 @@ def make_app(debug): | |||
| [(r'/status', handlers.StatusHandler)], | |||
| debug=debug, | |||
| ) | |||
| app.sentry_client = AsyncSentryClient(settings.SENTRY_DSN, release=__version__) | |||
|
|
|||
| app.sentry_client = AsyncSentryClient( | |||
There was a problem hiding this comment.
FYI, I fixed merge conflicts when updating the PR directly on GitHub.
SVCS-334 Refactor subclass of SanitizePasswordsProcessor to take better advantage of inheritance.
| @@ -44,7 +44,8 @@ def make_app(debug): | |||
| [(r'/status', handlers.StatusHandler)], | |||
| debug=debug, | |||
| ) | |||
| app.sentry_client = AsyncSentryClient(settings.SENTRY_DSN, release=__version__) | |||
| app.sentry_client = AsyncSentryClient(settings.SENTRY_DSN, release=__version__, | |||
There was a problem hiding this comment.
@felliott If you prefer, you can update the style during merge. Thanks.
This PR includes merge of #297 plus refactor of sub-classed raven processor.
TODO
Ticket
SVCS-334
Purpose
Sanitize and censor tokens we get back from Dataverse that show up in local variables
Changes
Added a sanitizer based that inherits from the default raven one. It has increased functionality to parse through dictionaries, look for dataverse keys, and a larger scope of variable names to sanitize (key, token)
NOTE: sanitize.py and test_sanitize.py's location in waterbutler are pretty variable. I put them in server just because that was the easiest place at the time. if they should live somewhere else, that is an easy change, and something that might need to be considered.
Side effects
Some sentry things that we don't want to sanitize may end up being sanitized.
QA Notes
To test, you need to trigger an error that will log a Dataverse API token in a local variable or request some how.
An easy way to do this is if locally testing, upgrade your furl to 1.0.1 in Waterbutler with Dataverse attached to your project. You will also need to add a personal sentry account to your Waterbutler (through your raven.config file, or by manually putting it in your settings).
One thing to note: If an API key shows up in the actual error message, this is currently not censored (not sure best way to go about this, but the only time I ever encountered this was with the error :Bad API token.. so the token is not valid anymore anyway.
To trigger the above "Bad API token" functionality, attach a dataverse account. Then on dataverse, go refresh your token and refresh the page. This error will trigger until the OSF has to revalidate (Or something like that. This only happened to me once or twice).
There are other manual ways to test locally, such as raising errors in odd places with variables named things like 'key' or having a variable value that looks like a dataverse token etc.