-
Notifications
You must be signed in to change notification settings - Fork 103
feat(ourlog): Only enable less destructive PII on the log body #5272
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
Conversation
8eccec5 to
cd52de5
Compare
cd52de5 to
578574f
Compare
|
|
||
| /// Log body. | ||
| #[metastructure(required = true, pii = "true", trim = false)] | ||
| #[metastructure(required = true, pii = "maybe", trim = false)] |
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.
Shouldn't this still be true? E.g. $logentry.formatted is also in REPLACE_ONLY_SELECTOR but still has pii = true.
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.
$logentry.formatted is explicitly removed from PII instead:
static DATASCRUBBER_IGNORE: LazyLock<SelectorSpec> = LazyLock::new(|| {
"(debug_meta.** | $frame.filename | $frame.abs_path | $logentry.formatted | $error.value | $request.headers.user-agent)"
.parse()
.unwrap()
});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.
Is this some weirdness around legacy? I'd expect Pii::Maybe to be advanced rules only, is this because
relay/relay-pii/src/convert.rs
Lines 113 to 122 in bbe038d
| applications.insert( | |
| REPLACE_ONLY_SELECTOR.clone(), | |
| vec![ | |
| "@email:replace".to_owned(), | |
| "@creditcard:replace".to_owned(), | |
| "@iban:replace".to_owned(), | |
| "@usssn:replace".to_owned(), | |
| "@bearer:replace".to_owned(), | |
| ], | |
| ); |
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.
Legacy (default scrubbers) are converted to 'advanced rules' there is some magic where these default rules won't apply to maybe. But we opt out in the conversion and inject these extra rules to for specific fields now. There is a longterm plan now to unify this, but until we have that, this is the workaround.
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.
I think this is fine, however I don't know the ramifications of removing the password:filter. Although it is destructive, it catches auth / secrets / api keys and if those are important we really should try to replace that behaviour if it was previously working for users.
|
|
||
| /// Log body. | ||
| #[metastructure(required = true, pii = "true", trim = false)] | ||
| #[metastructure(required = true, pii = "maybe", trim = false)] |
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.
Is this some weirdness around legacy? I'd expect Pii::Maybe to be advanced rules only, is this because
relay/relay-pii/src/convert.rs
Lines 113 to 122 in bbe038d
| applications.insert( | |
| REPLACE_ONLY_SELECTOR.clone(), | |
| vec![ | |
| "@email:replace".to_owned(), | |
| "@creditcard:replace".to_owned(), | |
| "@iban:replace".to_owned(), | |
| "@usssn:replace".to_owned(), | |
| "@bearer:replace".to_owned(), | |
| ], | |
| ); |
Applies only non-destructive rules to
$log.body, also re-uses pii fixtures from existing tests.