-
Notifications
You must be signed in to change notification settings - Fork 189
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(core): incorporate secret-scrubber into core #393
Conversation
@@ -23,17 +23,6 @@ const REQUEST_OBJECT_SHORTHAND_OPTIONS = { isShorthand: true, replace: true }; | |||
const DEFAULT_LOGGING_HTTP_ENDPOINT = 'https://httplogger.zapier.com/input'; | |||
const DEFAULT_LOGGING_HTTP_API_KEY = 'R24hzu86v3jntwtX2DtYECeWAB'; // It's ok, this isn't PROD | |||
|
|||
const SENSITIVE_KEYS = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this logic was moved to the package
const authData = Object.values(bundle.authData || {}); | ||
// for the most part, we should censor all the values from authData | ||
// the exception is safe urls, which should be filtered out - we want those to be logged | ||
const sensitiveAuthData = authData.filter( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic keeps "safe urls" around. there's code in scrubber to "intelligently" pull sensitive data out of urls which we could use here too, but we're also fine to censor all non-safe urls.
If we did that, it would pull my-secret
out of https://www.example.com/whatever?token=my-secret&foo=bar
. Then instead of censoring the whole url, it would be https://www.example.com/whatever?token=:censored:3:asdf:&foo=bar
. But, it's also fine as is.
@@ -72,9 +72,9 @@ describe('logger', () => { | |||
token: options.token, | |||
message: 'test', | |||
data: { | |||
password: ':censored:6:a5023f748d:', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests are all the same, but I changed the default salt for the censor
function, so the hashes had to be updated.
The CLI tests are still broken, but should auto-resolve once secret-scrubber-js!2 is released |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool work! I don't see any issues and I'll take another look once you fix the failing test.
I'd love to see we make the secret-scrubber repo public. Is that something you plan to do?
ok, tests pass! I'm glad they caught that node versioning issue. And yes, I do want them open sourced! The JS package is already publicly available, so hopefully it's not too much work. We went with Gitlab because we're not sure to what extent we're moving away from Github; that's a thing to circle back to. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Incorporates my hackathon project, the secret scrubber, into the platform!
It was mostly a project of moving and improving existing scrubbing code. Now, others can take advantage of it too!
PDE-2555