fix(pii): Replace only user in the userpath #2724
Conversation
relay-pii/src/regexes.rs
Outdated
| @@ -231,7 +231,7 @@ static PATH_REGEX: Lazy<Regex> = Lazy::new(|| { | |||
| ) | |||
| ) | |||
| ( | |||
| [^/\\]+ | |||
| [A–Za-z0–9'\._!\#^~-]+ | |||
There was a problem hiding this comment.
I think this would stop at first space? and I believe spaces are allowed in windows usernames. Should it also be in the list of allowed characters?
I checked on regex101 for this
There was a problem hiding this comment.
Maybe should match anything until \ or newline.
Edit: did this ever work on Windows with backwards slashes for path separators?
There was a problem hiding this comment.
@TBS1996 Overall it's not advisable to have spaces in the usernames and I'm not sure we want to support it either.
@Dav1dde Also if we remove everything , or just will new line we again end up with a somewhat wrong scrubbing.
See for example changes in relay-server/tests/snapshots/test_fixtures__dotnet__pii_stripping.snap below - it was scrubbed kind of a little bit too much. I do not mind that either, but we have to agree on the final rule 😄
There was a problem hiding this comment.
included also space in the regex now
#2724 silently broke user path scrubbing in minidumps. The regression was not automatically flagged because minidump scrubbing [ignores regexes that cannot be compiled with unicode disabled](https://github.com/getsentry/relay/blob/63ca425f5eba144cd5b8976b4201cd61084dafb9/relay-pii/src/attachments.rs#L37-L41), regardless of whether they are user-defined regexes or builtin static ones. This PR both fixes the regex and adds an automatic test to each PII regex, so we can catch similar problems in the future.
Explicitly use only allowed characters for the username.
Currently we replace everything till between the slashes, and if the slash after username is missing we will replace everything till the end of the input string.
fix: #2722
#skip-changelog