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(pii): Removes legacy special behaviour from safe fields parsing #2701

Merged
merged 1 commit into from
Nov 8, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
**Bug Fixes**:

- Disable scrubbing for the User-Agent header. ([#2641](https://github.com/getsentry/relay/pull/2641))
- Fixes certain safe fields disabling data scrubbing for all string fields. ([#2701](https://github.com/getsentry/relay/pull/2701))

**Internal**:

Expand Down
62 changes: 61 additions & 1 deletion relay-pii/src/convert.rs
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ pub fn to_pii_config(
continue;
}

let spec = match field.parse() {
let spec = match SelectorSpec::parse_non_legacy(field) {
Ok(spec) => spec,
Err(_) => {
// Ideally safe fields should be caught by sentry-side validation,
Expand Down Expand Up @@ -1559,4 +1559,64 @@ THd+9FBxiHLGXNKhG/FRSyREXEt+NyYIf/0cyByc9tNksat794ddUqnLOg0vwSkv
}
"###);
}

#[test]
fn test_safe_field_legacy_disables_all_string_parsing() {
let mut data = Event::from_value(
serde_json::json!({
"request": {
"data": {
"email": "foo@bar.de",
"password": "kkee",
"recaptcha": null,
}
},
})
.into(),
);

let pii_config = to_pii_config(&DataScrubbingConfig {
// this triggered legacy behaviour and disabled scrubbing for all fields
exclude_fields: vec!["email".to_owned()],
scrub_data: true,
scrub_ip_addresses: true,
sensitive_fields: vec!["email".to_owned(), "password".to_owned()],
scrub_defaults: true,
..Default::default()
})
.unwrap();

let mut pii_processor = PiiProcessor::new(pii_config.compiled());
process_value(&mut data, &mut pii_processor, ProcessingState::root()).unwrap();
assert_annotated_snapshot!(data, @r###"
{
"request": {
"data": {
"email": "foo@bar.de",
"password": "[Filtered]",
"recaptcha": null
}
},
"_meta": {
"request": {
"data": {
"password": {
"": {
"rem": [
[
"@password:filter",
"s",
0,
10
]
],
"len": 4
}
}
}
}
}
}
"###);
}
}
23 changes: 14 additions & 9 deletions relay-pii/src/selector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,19 @@ pub enum SelectorSpec {
}

impl SelectorSpec {
/// Parses a selector from a string without legacy special handling.
pub fn parse_non_legacy(s: &str) -> Result<SelectorSpec, InvalidSelectorError> {
handle_selector(
SelectorParser::parse(Rule::RootSelector, s)
.map_err(|e| InvalidSelectorError::ParseError(Box::new(e)))?
.next()
.unwrap()
.into_inner()
.next()
.unwrap(),
Comment on lines +174 to +177
Copy link
Member

Choose a reason for hiding this comment

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

In a next step we should try to get rid of these unwraps.

)
}

/// Checks if a path matches given selector.
///
/// This walks both the selector and the path starting at the end and towards the root
Expand Down Expand Up @@ -315,15 +328,7 @@ impl FromStr for SelectorSpec {
_ => {}
}

handle_selector(
SelectorParser::parse(Rule::RootSelector, s)
.map_err(|e| InvalidSelectorError::ParseError(Box::new(e)))?
.next()
.unwrap()
.into_inner()
.next()
.unwrap(),
)
Self::parse_non_legacy(s)
}
}

Expand Down