Skip to content

Commit

Permalink
fix(pii): Ignore replacement_chunks when they aren't used (#1180)
Browse files Browse the repository at this point in the history
With ReplaceBehavior::Value, we only want to replace a string value.
This means we don't need the replacement chunks since
`insert_replacement_chunks` correctly adds the replacement and the
`process_text` function is useless because we don't need it (besides,
processing an empty string doesn't perform any action).

Now that the `replacement_chunks` are only needed in
ReplaceBehavior::Groups, we can move the processing and assertion we do
on them to that branch of the `match`. This solves the issue of
asserting on them when we don't use them.
  • Loading branch information
iker-barriocanal authored Feb 15, 2022
1 parent ea87ce6 commit 760c683
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 6 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
**Bug Fixes**:

- Fix regression in CSP report parsing. ([#1174](https://github.com/getsentry/relay/pull/1174))
- Ignore replacement_chunks when they aren't used. ([#1180](https://github.com/getsentry/relay/pull/1180))

## 22.1.0

Expand Down
36 changes: 30 additions & 6 deletions relay-general/src/pii/processor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -304,17 +304,17 @@ fn apply_regex_to_chunks<'a>(
}
}
}
process_text(&search_string[pos..], &mut rv, &mut replacement_chunks);
debug_assert!(replacement_chunks.is_empty());
}
ReplaceBehavior::Value => {
process_text("", &mut rv, &mut replacement_chunks);
// We only want to replace a string value, and the replacement chunk for that is
// inserted by insert_replacement_chunks. Adding chunks from replacement_chunks
// results in the incorrect behavior of a total of more chunks than the input.
insert_replacement_chunks(rule, &search_string, &mut rv);
pos = search_string.len();
}
}

process_text(&search_string[pos..], &mut rv, &mut replacement_chunks);
debug_assert!(replacement_chunks.is_empty());

rv
}

Expand Down Expand Up @@ -355,7 +355,7 @@ fn insert_replacement_chunks(rule: &RuleRef, text: &str, output: &mut Vec<Chunk<

#[cfg(test)]
use {
crate::pii::PiiConfig,
crate::pii::{PiiConfig, ReplaceRedaction},
crate::processor::process_value,
crate::protocol::{
Addr, DebugImage, DebugMeta, Event, ExtraValue, Headers, LogEntry, NativeDebugImage,
Expand Down Expand Up @@ -944,3 +944,27 @@ fn test_ip_address_hashing_does_not_overwrite_id() {

assert_eq!(user.id.value().unwrap().as_str(), "123");
}

#[test]
fn test_replace_replaced_text() {
let chunks = vec![Chunk::Redaction {
text: "[ip]".into(),
rule_id: "@ip".into(),
ty: RemarkType::Substituted,
}];
let rule = RuleRef {
id: "@ip:replace".into(),
origin: "@ip".into(),
ty: RuleType::Ip,
redaction: Redaction::Replace(ReplaceRedaction {
text: "[ip]".into(),
}),
};
let res = apply_regex_to_chunks(
chunks.clone(),
&rule,
&Regex::new(r#".*"#).unwrap(),
ReplaceBehavior::Value,
);
assert_eq!(chunks, res);
}

0 comments on commit 760c683

Please sign in to comment.