Skip to content
Draft
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 @@ -7,6 +7,7 @@
- Reject spans, logs, trace metrics, replays when they are too old instead of shifting their timestamp. ([#6272](https://github.com/getsentry/relay/pull/6272))
- Extract nvgpu dumps and create GPU events. ([#6242](https://github.com/getsentry/relay/pull/6242))
- Preserve transaction `contexts`, `extra`, and `breadcrumbs` on the segment span as serialized attributes. ([#6286](https://github.com/getsentry/relay/pull/6286))
- Report `custom-filter` as the outcome reason for customer defined inbound filters, instead of the filter identifier. ([#6301](https://github.com/getsentry/relay/pull/6301))

**Bug Fixes**:

Expand Down
19 changes: 18 additions & 1 deletion relay-filter/src/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@ pub enum FilterStatKey {

/// Filtered due to a generic filter.
GenericFilter(String),

/// Filtered due to a customer defined inbound filter.
CustomFilter,
}

// An event grouped to a removed group.
Expand All @@ -63,7 +66,19 @@ pub enum FilterStatKey {
// that is why it was commented here and moved to OutcomeInvalidReason enum
// Cors,

/// Prefix that Sentry gives to the identifiers of customer defined inbound filters.
const CUSTOM_FILTER_PREFIX: &str = "cif-";

impl FilterStatKey {
/// Returns the stat key for a generic filter with the given identifier.
pub fn from_generic_filter_id(id: &str) -> Self {
if id.starts_with(CUSTOM_FILTER_PREFIX) {
FilterStatKey::CustomFilter
} else {
FilterStatKey::GenericFilter(id.to_owned())
}
}

/// Returns the string identifier of the filter stat key.
pub fn name(self) -> Cow<'static, str> {
Cow::Borrowed(match self {
Expand All @@ -79,6 +94,7 @@ impl FilterStatKey {
FilterStatKey::DeniedName => "denied-name",
FilterStatKey::DisabledNamespace => "disabled-namespace",
FilterStatKey::Discarded => "discarded",
FilterStatKey::CustomFilter => "custom-filter",
FilterStatKey::GenericFilter(filter_identifier) => {
return Cow::Owned(filter_identifier);
}
Expand Down Expand Up @@ -106,7 +122,8 @@ impl<'a> TryFrom<&'a str> for FilterStatKey {
"web-crawlers" => FilterStatKey::WebCrawlers,
"invalid-csp" => FilterStatKey::InvalidCsp,
"filtered-transaction" => FilterStatKey::FilteredTransactions,
other => FilterStatKey::GenericFilter(other.to_owned()),
"custom-filter" => FilterStatKey::CustomFilter,
other => FilterStatKey::from_generic_filter_id(other),
})
}
}
24 changes: 23 additions & 1 deletion relay-filter/src/generic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@

for filter_config in filters {
if filter_config.is_enabled && matches(item, filter_config.condition) {
return Err(FilterStatKey::GenericFilter(filter_config.id.to_owned()));
return Err(FilterStatKey::from_generic_filter_id(filter_config.id));
}
}

Expand Down Expand Up @@ -224,6 +224,28 @@
);
}

#[test]
fn test_should_filter_custom_filter() {
let config = GenericFiltersConfig {
version: 1,
filters: vec![GenericFilterConfig {
id: "cif-42".to_owned(),
is_enabled: true,
condition: Some(RuleCondition::eq("event.release", "1.0")),
}]
.into(),
};

let event = Event {
release: Annotated::new(LenientString("1.0".to_owned())),
..Default::default()
};

Check warning on line 242 in relay-filter/src/generic.rs

View check run for this annotation

@sentry/warden / warden: wrdn-dos-review

RuleCondition evaluation in generic/custom filters lacks recursion depth bound

RuleCondition::matches recurses through nested And, Or, Not, Any, and All variants without a depth limit. When a project config contains a deeply nested custom or generic filter condition, evaluating it against an event via should_filter can trigger a stack-overflow abort.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

RuleCondition evaluation in generic/custom filters lacks recursion depth bound

RuleCondition::matches recurses through nested And, Or, Not, Any, and All variants without a depth limit. When a project config contains a deeply nested custom or generic filter condition, evaluating it against an event via should_filter can trigger a stack-overflow abort.

Evidence
  • relay-filter/src/generic.rs line ~55: should_filter iterates over generic/custom filter configs and calls matches(item, filter_config.condition).
  • relay-filter/src/generic.rs line ~31: matches delegates to RuleCondition::matches, entering the recursive sink.
  • relay-protocol/src/condition.rs lines ~790-802: RuleCondition::matches dispatches to AndCondition::matches, OrCondition::matches, NotCondition::matches, AnyCondition::matches, and AllCondition::matches with no depth guard or visited-set.
  • The RuleCondition trees are deserialized from upstream project-configuration JSON (including new custom filters prefixed with cif-); Relay enforces a 20 MB total response size cap but no nesting-depth validation before evaluation.
  • No sibling depth-bounded equivalent exists for this evaluation path.

Identified by Warden · wrdn-dos-review · 48Z-6E5

assert_eq!(
should_filter(&event, &config, None),
Err(FilterStatKey::CustomFilter)
);
}

#[test]
fn test_should_filter_fifo_match_rules() {
let config = GenericFiltersConfig {
Expand Down
Loading