feat: Introduce 'sanitizeKeys' config option #1264
Conversation
|
On a roll Kamil! haven't reviewed the implementation in depth yet, but I think it's worth discussing the API/behavior/guarantees/terminology of this feature, because it's really important and this client could become the gold standard for our other clients going forward. This is not to say that we need to get this perfect in this PR, just that it would be good to identify our goals for the best possible API here so this can work towards that in a backwards-compatible way. Afaik (and I could be very wrong, @bretthoerner @HazAT ) the only clients which support this right now are python and ruby. Pythondocs:
Ruby:docs:
|
|
I'd prefer something simple like sanitizeKeys over the general processors pattern we implemented elsewhere. I think also supporting regexp is good. |
|
The main issue I have with RegExp is that it can help people easily shoot themselves in the foot, as this sanitizer goes through all keys of the payload sent to Sentry. Including exception, message, frames, etc. etc. @MaxBittker I added defaults at first, but then thought that there may be scenarios where people actually want to let them through, and changing this behavior later, would mean a breaking API change. On the other hand, we already have some default values in the Sentry, so even if a user sends them through, they'll get sanitized. This means that adding them as a default value and merging passed config keys in the SDK as well shouldn't do any harm. |
I can understand why this can be an example of "shooting yourself in the foot ", but many companies/organization want to scrub PII everywhere (frames, tags, extra information, error title/msg, etc.). Maybe a config that specifies to look through everything / etc.?? |
src/utils.js
Outdated
| } | ||
|
|
||
| function sanitize(input, sanitizeKeys) { | ||
| sanitizeKeys = isArray(sanitizeKeys) ? sanitizeKeys : []; |
There was a problem hiding this comment.
if it's not an array, maybe just bail out here and return array? to reduce the risk involved in shipping this code?
|
|
||
| function sanitize(input, sanitizeKeys) { | ||
| sanitizeKeys = isArray(sanitizeKeys) ? sanitizeKeys : []; | ||
| var sanitizeMask = '********'; |
There was a problem hiding this comment.
i know we use asterisks elsewhere, but i think something explicit like [value sanitized by raven.js]could be better.
probably not worth breaking with the precedent though
| return ''; | ||
| } | ||
|
|
||
| function sanitize(input, sanitizeKeys) { |
There was a problem hiding this comment.
how does this deal with cycles? or will the data be guaranteed by this point not to have any? how strong is that guarantee? (afraid of freezing a browser in some weird scenario)
There was a problem hiding this comment.
It didn't. Thanks for catching that!
docs/config.rst
Outdated
| .. describe:: sanitizeKeys | ||
|
|
||
| An array of strings representing keys that should be scrubbed from the payload sent to Sentry. |
There was a problem hiding this comment.
(worth being explicit here that this doesn't scrub or match on string values or query encoded data)
There was a problem hiding this comment.
also worth mentioning that sentry itself can do do server side sanitizing and this is different
There was a problem hiding this comment.
basically i think people are really gonna want to know what guarantees they get from this feature so it's worth writing a longer explanation
|
@dcramer added support for RegExp. |
4 participants
Resolves #296
It was quite requested feature by some of the clients recently, so we may get it in, in one form or another. I'm open for the feedback.