feat(general): Scrub all fields with IP address #1725
Conversation
TBS1996
changed the title
relay-general/src/pii/convert.rs
Outdated
| @@ -39,7 +31,8 @@ pub fn to_pii_config( | |||
| } | |||
|
|
|||
| if datascrubbing_config.scrub_ip_addresses { | |||
| applications.insert(KNOWN_IP_FIELDS.clone(), vec!["@anything:remove".to_owned()]); | |||
| let wildcard = SelectorSpec::Path(vec![SelectorPathItem::DeepWildcard]); | |||
| applications.insert(wildcard, vec!["@ip:replace".to_owned()]); | |||
There was a problem hiding this comment.
The question is do we want to keep this consistent with old behaviour and just remove them or actually switch to replacing all the IPs as you did here.
relay-general/src/pii/convert.rs
Outdated
| #[test] | ||
| fn test_user_ip_stripped() { | ||
| let mut data = Event::from_value( | ||
| serde_json::json!({ | ||
| "user": { | ||
| "username": "secret", | ||
| "username": "73.133.27.120", |
There was a problem hiding this comment.
it would be great to test if the scrubbing works also in , e.g. breadcrumbs - if you add the message with ip inside , as it's done in test
relay/relay-general/src/pii/convert.rs
Line 1197 in 3b42784
the protocol for
breadcrumbs is in
where you can see which fields are PII
Co-authored-by: Oleksandr <[email protected]>
relay-general/src/pii/snapshots/relay_general__pii__convert__tests__user_ip_stripped.snap
Show resolved
Hide resolved
relay-general/src/pii/convert.rs
Outdated
| applications.insert(KNOWN_IP_FIELDS.clone(), vec!["@anything:remove".to_owned()]); | ||
|
|
||
| // checks actual contents of all fields and scrubs where there is an IP address | ||
| let wildcard = SelectorSpec::Path(vec![SelectorPathItem::DeepWildcard]); |
There was a problem hiding this comment.
Instead of pushing to applications with a custom selector spec for IP scrubbing, I would simply push @ip:replace to applied_rules (see code further below). It will then use the same selector as the other default rules, which I think is what we want.
| { | ||
| "message": "73.133.27.120", | ||
| "data": { | ||
| "test_data": "73.133.27.120" // test deep wildcard stripping |
There was a problem hiding this comment.
As we talked in the meeting, can we also make sure to test that the IPs are scrubbed also from inside the strings, e.g.
"test_data": "contains ip's like 73.133.27.120 but also the text"which should result in something like
"test_data": "contains ip's like [ip] but also the text"
or make sure this actually can work?
There was a problem hiding this comment.
I would create a separate issue for this, as it would also the touch IP scrubbing for users who configured it manually through advanced data scrubbing.
Co-authored-by: Joris Bayer <[email protected]>
* master: (35 commits) ref(actix): Migrate ProjectUpstream to `relay_system::Service` (#1727) feat(general): Add unknown SessionStatus variant (#1736) ref: Convert integration tests about dropping transactions to unit tests (#1720) release: 0.8.16 ci: Skip redundant self-hosted E2E on library release (#1755) doc(changelog): Add relevant changes to python changelog (#1753) feat(profiling): Add profile context (#1748) release: 23.1.0 profiling(fix): use an unpadded base64 encoding (#1749) Revert "feat(replays): Enable PII scrubbing for all organizations" (#1747) feat: Switch from base64 to data-encoding (#1743) instr(replays): Add timer metric to recording processing (#1742) feat(replays): Use Annotated struct definition for replay-event parsing (#1582) feat(sessions): Retire session duration metric (#1739) feat(general): Scrub all fields with IP address (#1725) feat(replays): Enable PII scrubbing for all organizations (#1678) chore(project): Add backoff mechanism for fetching projects (#1726) feat(profiling): Add new measurement units for profiling (#1732) chore(toolchain): update rust to 1.66.1 (#1735) ref(actix): Migrate server actor to the "service" arch (#1723) ...
This change will scrub all fields containing an IP address, rather than scrubbing certain fields known to have them
for the unit tests, testing ip scrubbing in sdk and user have been merged as it should scrub all fields.
referencing issue #1693