Skip to content

fix(pii): Prevent cycles in rule collection #6344

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.

Sign up for GitHub

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

Open
jjbayer wants to merge 5 commits into
base: master
Choose a base branch
from
+191 −4
Open

fix(pii): Prevent cycles in rule collection #6344

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b65e15a
fix(pii): Prevent cycles in rule collection
jjbayer Sep 2, 2026
a42c217
doc: changelog
jjbayer Sep 2, 2026
f4e9643
regression test
jjbayer Sep 2, 2026
c38f893
Merge branch 'master' into fix/pii-recursion
jjbayer Sep 2, 2026
37556b8
simplify
jjbayer Sep 4, 2026
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
- Reject Nintendo Switch dying message attachments with an invalid magic number instead of panicking on a short payload. ([#6253](https://github.com/getsentry/relay/pull/6253))
- Downgrade Kafka to prevent producers from getting stuck. ([#6336](https://github.com/getsentry/relay/pull/6336))
- Prevent memory bomb in PII processor's `split_chunks`. ([#6343](https://github.com/getsentry/relay/pull/6343))
- Prevent stack overflow in PII rule compilation. ([#6344](https://github.com/getsentry/relay/pull/6344))

**Internal**:

Expand Down
194 changes: 190 additions & 4 deletions relay-pii/src/compiledconfig.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,9 @@ impl CompiledPiiConfig {
for (selector, rules) in &config.applications {
#[allow(clippy::mutable_key_type)]
let mut rule_set = BTreeSet::default();
let mut seen_ids = BTreeSet::default();
for rule_id in rules {
collect_rules(config, &mut rule_set, rule_id, None);
collect_rules(config, &mut rule_set, &mut seen_ids, rule_id, None);
}
applications.push((selector.clone(), rule_set));
}
Expand Down Expand Up @@ -78,6 +79,7 @@ fn get_rule(config: &PiiConfig, id: &str) -> Option<RuleRef> {
fn collect_rules(
config: &PiiConfig,
rules: &mut BTreeSet<RuleRef>,
seen_ids: &mut BTreeSet<Box<str>>,
rule_id: &str,
parent: Option<RuleRef>,
) {
Expand All @@ -86,7 +88,7 @@ fn collect_rules(
None => return,
};

if rules.contains(&rule) {
if !seen_ids.insert(rule_id.into()) {
return;
}

Expand All @@ -103,7 +105,7 @@ fn collect_rules(
None
};
for rule_id in &m.rules {
collect_rules(config, rules, rule_id, parent.clone());
collect_rules(config, rules, seen_ids, rule_id, parent.clone());
}
}
RuleType::Alias(ref a) => {
Expand All @@ -112,7 +114,7 @@ fn collect_rules(
} else {
None
};
collect_rules(config, rules, &a.rule, parent);
collect_rules(config, rules, seen_ids, &a.rule, parent);
}
RuleType::Unknown(_) => {}
_ => {
Expand Down Expand Up @@ -172,3 +174,187 @@ impl Ord for RuleRef {
self.id.cmp(&other.id)
}
}

#[cfg(test)]
mod tests {
use std::collections::BTreeMap;

use crate::AliasRule;
Comment thread
sentry[bot] marked this conversation as resolved.

use super::*;

#[test]
fn cycle_singleton() {
// a -> a
let config = PiiConfig {
rules: BTreeMap::from([(
"a".to_owned(),
RuleSpec {
ty: RuleType::Alias(AliasRule {
rule: "a".to_owned(),
hide_inner: false,
}),
redaction: Redaction::Default,
},
)]),
..Default::default()
};
#[allow(clippy::mutable_key_type)]
let mut collected_rules = Default::default();
let mut seen_ids = Default::default();
collect_rules(&config, &mut collected_rules, &mut seen_ids, "a", None);

// The cycle has been removed:
assert!(collected_rules.is_empty());
}

#[test]
fn cycle_pair() {
// a -> b -> a
let config = PiiConfig {
rules: BTreeMap::from([
(
"a".to_owned(),
RuleSpec {
ty: RuleType::Alias(AliasRule {
rule: "b".to_owned(),
hide_inner: false,
}),
redaction: Redaction::Default,
},
),
(
"b".to_owned(),
RuleSpec {
ty: RuleType::Alias(AliasRule {
rule: "a".to_owned(),
hide_inner: false,
}),
redaction: Redaction::Default,
},
),
]),
..Default::default()
};
#[allow(clippy::mutable_key_type)]
let mut collected_rules = Default::default();
let mut seen_ids = Default::default();
collect_rules(&config, &mut collected_rules, &mut seen_ids, "a", None);

// The cycle has been removed:
assert!(collected_rules.is_empty());
}

#[test]
fn only_one_shared_rule_survives() {
// When multiple aliases point to the same rule, only one of their names survives.
// a -> c
// b -> c
let config = PiiConfig {
rules: BTreeMap::from([
(
"a".to_owned(),
RuleSpec {
ty: RuleType::Alias(AliasRule {
rule: "c".to_owned(),
hide_inner: true,
}),
redaction: Redaction::Default,
},
),
(
"b".to_owned(),
RuleSpec {
ty: RuleType::Alias(AliasRule {
rule: "c".to_owned(),
hide_inner: true,
}),
redaction: Redaction::Default,
},
),
(
"c".to_owned(),
RuleSpec {
ty: RuleType::Anything,
redaction: Redaction::Default,
},
),
]),
..Default::default()
};
#[allow(clippy::mutable_key_type)]
let mut collected_rules = BTreeSet::new();
let mut seen_ids = BTreeSet::new();
collect_rules(&config, &mut collected_rules, &mut seen_ids, "a", None);
collect_rules(&config, &mut collected_rules, &mut seen_ids, "b", None);

let collected_rules: Vec<_> = collected_rules
.into_iter()
.map(|rr| (rr.origin, rr.id))
.collect();

insta::assert_debug_snapshot!(collected_rules, @r#"
[
(
"a",
"c",
),
]
"#);
}

#[test]
fn double_origin() {
// a -> b -> c
let config = PiiConfig {
rules: BTreeMap::from([
(
"a".to_owned(),
RuleSpec {
ty: RuleType::Alias(AliasRule {
rule: "b".to_owned(),
hide_inner: true,
}),
redaction: Redaction::Default,
},
),
(
"b".to_owned(),
RuleSpec {
ty: RuleType::Alias(AliasRule {
rule: "c".to_owned(),
hide_inner: true,
}),
redaction: Redaction::Default,
},
),
(
"c".to_owned(),
RuleSpec {
ty: RuleType::Anything,
redaction: Redaction::Default,
},
),
]),
..Default::default()
};
#[allow(clippy::mutable_key_type)]
let mut collected_rules = Default::default();
let mut seen_ids = Default::default();
collect_rules(&config, &mut collected_rules, &mut seen_ids, "a", None);

let collected_rules: Vec<_> = collected_rules
.into_iter()
.map(|rr| (rr.origin, rr.id))
.collect();

insta::assert_debug_snapshot!(collected_rules, @r#"
[
(
"a",
"c",
),
]
"#);
}
}