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

GitHub Issue #180: config.scrub_fields is applied to config.person #216

Merged
merged 2 commits into from
Jul 3, 2017

Conversation

ArturMoczulski
Copy link
Contributor

This adds scrub_whitelist config which allows to create exceptions to scrub fields based on an associative array dot notation.

@ArturMoczulski
Copy link
Contributor Author

This is ready for review.

src/Scrubber.php Outdated
@@ -24,20 +26,35 @@ protected function setScrubFields($config)
}
$this->scrubFields = self::$defaults->scrubFields($fromConfig);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Get rid of this whitespace

src/Scrubber.php Outdated
$current = !$path ? $key : $path . '.' . $key;

if (in_array($current, $scrubber->getWhitelist())) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

If person is in scrub_fields and data.person.username is in the whitelist, then I am pretty sure we would end up with data.person === '********' which I think is the correct behaviour that we want, but is not necessarily obvious. This only works at whitelisting things that would otherwise be scrubbed based on the exact key and not a earlier path component. i.e elements of the whitelist data.a.b.c.d will only be whitelisted if a, b, and c are not scrub_fields.

@rokob
Copy link
Contributor

rokob commented Jul 3, 2017

LGTM

@rokob rokob merged commit 9b1b11f into rollbar:master Jul 3, 2017
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.

2 participants