Skip to content
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

fix(core): handle nullish values in auth data #427

Merged
merged 3 commits into from
Sep 22, 2021
Merged

fix(core): handle nullish values in auth data #427

merged 3 commits into from
Sep 22, 2021

Conversation

xavdid
Copy link
Contributor

@xavdid xavdid commented Sep 17, 2021

fixes PDE-2771

Secret scrubber only takes a (string | number)[], so we have to filter out nullish values before they go in. Secret scrubber's recruseExtract does this, but only if we use it!

Tests won't pass until zapier/team-developer-platform/secret-scrubber-js!3 is merged.

Copy link
Contributor Author

@xavdid xavdid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok! Deployed secret scrubber and this is looking good. Now we don't explode when someone tries to log null.

(item) => !isUrl(item) || isUrlWithSecrets(item)
);
const sensitiveAuthData = recurseExtract(authData, (key, value) => {
if (isUrl(value) && !isUrlWithSecrets(value)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic is the same, but this felt more readable.

@@ -153,7 +153,7 @@ const flattenPaths = (data, { preserve = {} } = {}) => {

// A simpler, and memory-friendlier version of _.truncate()
const simpleTruncate = (string, length, suffix) => {
if (string === undefined) {
if (string == null) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this now returns for null and undefined; before we were converting null to '', which was weird.

@@ -288,7 +288,7 @@ describe('logger', () => {

// this test fails because the function that creates the sensitive bank doesn't
// recurse to find all sensitive values
it.skip('should replace sensitive data that nested', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@xavdid xavdid marked this pull request as ready for review September 20, 2021 23:57
@xavdid xavdid merged commit c35f094 into master Sep 22, 2021
@xavdid xavdid deleted the PDE-2771 branch September 22, 2021 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants