Stop serializing all environment variables#84
Conversation
it's considered a security issue, since sensitive data may end up serialized
|
Is it currently possible to attach it manually instead? |
|
Nope, this just stops the serialization. |
|
It would be nice to allow attaching it if users need it |
+1. After this change, is there a way to configure sending these vars again, for those that may want to? |
|
Made it customizable now. |
|
|
||
| ## Features | ||
|
|
||
| * Automatically add `process.env` as well as other information when sending notifications |
There was a problem hiding this comment.
"as well as other information" was a feature. Specifically what is the other information, is it useful to list this as a feature?
This feature is needs more thought. It seems that ENV var blacklisting was already possible. If we merged this PR as-is, it would be very unclear how we are supposed to control which ENV vars are being reported. Do we need both whitelisting and blacklisting? |
|
I think we only need whitelisting. It should be secure by default and allow users to specify which environment variables to send. |
|
That sounds good to me. Are you considering removing the existing blacklisting mechanism? It would probably make this a minor or even a major release. Can we make the API simple and documented clearly? Any thoughts @kyrylo? |
|
Removed the blacklisting mechanism and added documentation for whitelisting. |
|
Removing blacklisting doesn't make a lot of sense to me. What if I only want to filter out passwords and credit cards? Why would I use whitelisting for that? For example, the existing Ruby notifier supports both techniques. |
|
Added support for both whitelisting and blacklisting of environment variables, for maximum flexibility. |
|
I think this mechanism should be global to most of the payload. Filtering is useful here, but it's also useful in some other places. I am not sure if this should be merged in the present state because it looks like we still send all the env variables, but filter them, if any filters are present. So, by default, we still send sensitive info, don't we? |
|
By default we don't send anything. |
lib/airbrake.js
Outdated
| } else if (this.blackListKeys.length > 0){ | ||
| Object.keys(process.env).forEach(function(key) { | ||
| if (self.blackListKeys.indexOf(key) > -1){ | ||
| return; |
There was a problem hiding this comment.
What do you think about saying [Filtered] similar to what we already do in the Ruby notifier? It's more descriptive and lets the user know that it was filtered, not missed.
There was a problem hiding this comment.
Yeah, [FILTERED] is better.
Stop serializing all environment variables
3 participants
It's considered a security issue, since sensitive data may end up serialized.