fix(pii): Do not panic on large regex [INC-202]

CHANGELOG.md

@@ -13,6 +13,9 @@
 - Process required stacktraces to fix filtering events originating from browser extensions. ([#1423](https://github.com/getsentry/relay/pull/1423))
 - Fix error message filtering when formatting the message of logentry. ([#1442](https://github.com/getsentry/relay/pull/1442))
 - Loosen type requirements for the `user.id` field in Replays. ([#1443](https://github.com/getsentry/relay/pull/1443))
+- Use the different configuration for billing outcomes when specified. ([#1461](https://github.com/getsentry/relay/pull/1461))
+- Fix panic in datascrubbing when number of sensitive fields was too large. ([#1474](https://github.com/getsentry/relay/pull/1474))
+
 
 **Internal**:
 

py/CHANGELOG.md

@@ -4,6 +4,7 @@
 
 - Add `transaction_info` to event payloads, including the transaction's source and internal original transaction name. ([#1330](https://github.com/getsentry/relay/pull/1330))
 - Add user-agent parsing to replays processor. ([#1420](https://github.com/getsentry/relay/pull/1420))
+- convert_datascrubbing_config will now return an error string when conversion fails. ([#1474](https://github.com/getsentry/relay/pull/1474))
 
 ## 0.8.13
 

relay-cabi/src/processing.rs

@@ -143,8 +143,10 @@ pub unsafe extern "C" fn relay_validate_pii_config(value: *const RelayStr) -> Re
 pub unsafe extern "C" fn relay_convert_datascrubbing_config(config: *const RelayStr) -> RelayStr {
     let config: DataScrubbingConfig = serde_json::from_str((*config).as_str())?;
     match config.pii_config() {
-        Some(config) => RelayStr::from_string(config.to_json()?),
-        None => RelayStr::new("{}"),
+        Ok(Some(config)) => RelayStr::from_string(config.to_json()?),
+        Ok(None) => RelayStr::new("{}"),
+        // NOTE: Callers of this function must be able to handle this error.
+        Err(e) => RelayStr::from_string(e.to_string()),
     }
 }
 

relay-general/benches/benchmarks.rs

@@ -140,7 +140,7 @@ fn bench_pii_stripping(c: &mut Criterion) {
         |b, datascrubbing_config| b.iter(|| datascrubbing_config.pii_config_uncached()),
     );
 
-    let pii_config = datascrubbing_config.pii_config_uncached().unwrap();
+    let pii_config = datascrubbing_config.pii_config_uncached().unwrap().unwrap();
 
     group.bench_with_input(
         BenchmarkId::new("compile_pii_config", config_name),

relay-general/src/pii/config.rs

@@ -8,10 +8,33 @@ use serde::{de::Error, Deserialize, Deserializer, Serialize, Serializer};
 
 use crate::pii::{CompiledPiiConfig, Redaction};
 use crate::processor::SelectorSpec;
+use failure::Fail;
+
+#[derive(Clone, Debug, Fail)]
+pub enum PiiConfigError {
+    #[fail(display = "could not parse regex")]
+    RegexError(#[cause] regex::Error),
+}
 
 /// A regex pattern for text replacement.
 #[derive(Clone)]
-pub struct Pattern(pub Regex);
+pub struct Pattern(Regex);
+
+impl Pattern {
+    pub fn new(s: &str, case_insensitive: bool) -> Result<Self, PiiConfigError> {
+        let regex = RegexBuilder::new(s)
+            .size_limit(262_144)
+            .case_insensitive(case_insensitive)
+            .build()
+            .map_err(PiiConfigError::RegexError)?;
+
+        Ok(Self(regex))
+    }
+
+    pub fn regex(&self) -> &Regex {
+        &self.0
+    }
+}
 
 impl Deref for Pattern {
     type Target = Regex;
@@ -42,11 +65,8 @@ impl Serialize for Pattern {
 impl<'de> Deserialize<'de> for Pattern {
     fn deserialize<D: Deserializer<'de>>(deserializer: D) -> Result<Self, D::Error> {
         let raw = String::deserialize(deserializer)?;
-        let pattern = RegexBuilder::new(&raw)
-            .size_limit(262_144)
-            .build()
-            .map_err(Error::custom)?;
-        Ok(Pattern(pattern))
+        let pattern = Pattern::new(&raw, false).map_err(Error::custom)?;
+        Ok(pattern)
     }
 }
 

relay-general/src/pii/convert.rs

@@ -1,13 +1,14 @@
 use std::collections::BTreeMap;
 
 use once_cell::sync::Lazy;
-use regex::RegexBuilder;
 
 use crate::pii::{
     DataScrubbingConfig, Pattern, PiiConfig, RedactPairRule, Redaction, RuleSpec, RuleType, Vars,
 };
 use crate::processor::{SelectorPathItem, SelectorSpec, ValueType};
 
+use crate::pii::config::PiiConfigError;
+
 // XXX: Move to @ip rule for better IP address scrubbing. Right now we just try to keep
 // compatibility with Python.
 static KNOWN_IP_FIELDS: Lazy<SelectorSpec> = Lazy::new(|| {
@@ -26,7 +27,9 @@ static DATASCRUBBER_IGNORE: Lazy<SelectorSpec> = Lazy::new(|| {
         .unwrap()
 });
 
-pub fn to_pii_config(datascrubbing_config: &DataScrubbingConfig) -> Option<PiiConfig> {
+pub fn to_pii_config(
+    datascrubbing_config: &DataScrubbingConfig,
+) -> Result<Option<PiiConfig>, PiiConfigError> {
     let mut custom_rules = BTreeMap::new();
     let mut applied_rules = Vec::new();
     let mut applications = BTreeMap::new();
@@ -70,12 +73,7 @@ pub fn to_pii_config(datascrubbing_config: &DataScrubbingConfig) -> Option<PiiCo
                 "strip-fields".to_owned(),
                 RuleSpec {
                     ty: RuleType::RedactPair(RedactPairRule {
-                        key_pattern: Pattern(
-                            RegexBuilder::new(&key_pattern)
-                                .case_insensitive(true)
-                                .build()
-                                .unwrap(),
-                        ),
+                        key_pattern: Pattern::new(&key_pattern, true)?,
                     }),
                     redaction: Redaction::Replace("[Filtered]".to_owned().into()),
                 },
@@ -86,7 +84,7 @@ pub fn to_pii_config(datascrubbing_config: &DataScrubbingConfig) -> Option<PiiCo
     }
 
     if applied_rules.is_empty() && applications.is_empty() {
-        return None;
+        return Ok(None);
     }
 
     let mut conjunctions = vec![
@@ -116,12 +114,12 @@ pub fn to_pii_config(datascrubbing_config: &DataScrubbingConfig) -> Option<PiiCo
         applications.insert(applied_selector, applied_rules);
     }
 
-    Some(PiiConfig {
+    Ok(Some(PiiConfig {
         rules: custom_rules,
         vars: Vars::default(),
         applications,
         ..Default::default()
-    })
+    }))
 }
 
 #[cfg(test)]
@@ -140,7 +138,7 @@ mod tests {
     use super::to_pii_config as to_pii_config_impl;
 
     fn to_pii_config(datascrubbing_config: &DataScrubbingConfig) -> Option<PiiConfig> {
-        let rv = to_pii_config_impl(datascrubbing_config);
+        let rv = to_pii_config_impl(datascrubbing_config).unwrap();
         if let Some(ref config) = rv {
             let roundtrip: PiiConfig =
                 serde_json::from_value(serde_json::to_value(config).unwrap()).unwrap();
@@ -284,6 +282,19 @@ THd+9FBxiHLGXNKhG/FRSyREXEt+NyYIf/0cyByc9tNksat794ddUqnLOg0vwSkv
         "###);
     }
 
+    #[test]
+    fn test_convert_sensitive_fields_too_large() {
+        let result = to_pii_config_impl(&DataScrubbingConfig {
+            sensitive_fields: vec!["1"]
+                .repeat(4085) // lowest number that will fail
+                .into_iter()
+                .map(|x| x.to_string())
+                .collect(),
+            ..simple_enabled_config()
+        });
+        assert!(result.is_err());
+    }
+
     #[test]
     fn test_convert_exclude_field() {
         let pii_config = to_pii_config(&DataScrubbingConfig {

relay-general/src/pii/legacy.rs

@@ -8,6 +8,8 @@ use serde::{Deserialize, Serialize};
 
 use crate::pii::{convert, PiiConfig};
 
+use super::config::PiiConfigError;
+
 /// Helper method to check whether a flag is false.
 #[allow(clippy::trivially_copy_pass_by_ref)]
 fn is_flag_default(flag: &bool) -> bool {
@@ -36,7 +38,7 @@ pub struct DataScrubbingConfig {
     ///
     /// Cached because the conversion process is expensive.
     #[serde(skip)]
-    pub(super) pii_config: OnceCell<Option<PiiConfig>>,
+    pub(super) pii_config: OnceCell<Result<Option<PiiConfig>, PiiConfigError>>,
 }
 
 impl DataScrubbingConfig {
@@ -48,7 +50,7 @@ impl DataScrubbingConfig {
             scrub_ip_addresses: false,
             sensitive_fields: vec![],
             scrub_defaults: false,
-            pii_config: OnceCell::with_value(None),
+            pii_config: OnceCell::with_value(Ok(None)),
         }
     }
 
@@ -61,15 +63,15 @@ impl DataScrubbingConfig {
     ///
     /// This can be computationally expensive when called for the first time. The result is cached
     /// internally and reused on the second call.
-    pub fn pii_config(&self) -> Option<&'_ PiiConfig> {
+    pub fn pii_config(&self) -> Result<&'_ Option<PiiConfig>, &PiiConfigError> {
         self.pii_config
             .get_or_init(|| self.pii_config_uncached())
             .as_ref()
     }
 
     /// Like [`pii_config`](Self::pii_config) but without internal caching.
     #[inline]
-    pub fn pii_config_uncached(&self) -> Option<PiiConfig> {
+    pub fn pii_config_uncached(&self) -> Result<Option<PiiConfig>, PiiConfigError> {
         convert::to_pii_config(self)
     }
 }

relay-general/src/pii/mod.rs

@@ -16,8 +16,8 @@ mod utils;
 pub use self::attachments::{PiiAttachmentsProcessor, ScrubEncodings};
 pub use self::compiledconfig::CompiledPiiConfig;
 pub use self::config::{
-    AliasRule, MultipleRule, Pattern, PatternRule, PiiConfig, RedactPairRule, RuleSpec, RuleType,
-    Vars,
+    AliasRule, MultipleRule, Pattern, PatternRule, PiiConfig, PiiConfigError, RedactPairRule,
+    RuleSpec, RuleType, Vars,
 };
 pub use self::generate_selectors::selector_suggestions_from_value;
 pub use self::legacy::DataScrubbingConfig;

relay-general/src/pii/regexes.rs

@@ -54,7 +54,7 @@ pub fn get_regex_for_rule_type(
     match ty {
         RuleType::RedactPair(ref redact_pair) => smallvec![(
             kv,
-            &redact_pair.key_pattern.0,
+            redact_pair.key_pattern.regex(),
             ReplaceBehavior::replace_value()
         )],
         RuleType::Password => {
@@ -69,7 +69,7 @@ pub fn get_regex_for_rule_type(
                 None => ReplaceBehavior::replace_match(),
             };
 
-            smallvec![(v, &r.pattern.0, replace_behavior)]
+            smallvec![(v, r.pattern.regex(), replace_behavior)]
         }
 
         RuleType::Imei => smallvec![(v, &*IMEI_REGEX, ReplaceBehavior::replace_match())],

relay-server/src/actors/processor.rs

@@ -13,12 +13,11 @@ use failure::Fail;
 use flate2::write::{GzEncoder, ZlibEncoder};
 use flate2::Compression;
 use once_cell::sync::OnceCell;
-use serde_json::Value as SerdeValue;
-
 use relay_auth::RelayVersion;
 use relay_common::{clone, ProjectId, ProjectKey, UnixTimestamp};
 use relay_config::{Config, HttpEncoding};
 use relay_filter::FilterStatKey;
+use relay_general::pii::PiiConfigError;
 use relay_general::pii::{PiiAttachmentsProcessor, PiiProcessor};
 use relay_general::processor::{process_value, ProcessingState};
 use relay_general::protocol::{
@@ -34,6 +33,7 @@ use relay_quotas::{DataCategory, RateLimits, ReasonCode};
 use relay_redis::RedisPool;
 use relay_sampling::{DynamicSamplingContext, RuleId};
 use relay_statsd::metric;
+use serde_json::Value as SerdeValue;
 
 use crate::actors::envelopes::{EnvelopeManager, SendEnvelope, SendEnvelopeError, SubmitEnvelope};
 use crate::actors::outcome::{DiscardReason, Outcome, TrackOutcome};
@@ -113,6 +113,9 @@ pub enum ProcessingError {
 
     #[fail(display = "event dropped by sampling rule {}", _0)]
     Sampled(RuleId),
+
+    #[fail(display = "invalid pii config")]
+    PiiConfigError(PiiConfigError),
 }
 
 impl ProcessingError {
@@ -139,7 +142,7 @@ impl ProcessingError {
             Self::InvalidUnrealReport(_) => Some(Outcome::Invalid(DiscardReason::ProcessUnreal)),
 
             // Internal errors
-            Self::SerializeFailed(_) | Self::ProcessingFailed(_) => {
+            Self::SerializeFailed(_) | Self::ProcessingFailed(_) | Self::PiiConfigError(_) => {
                 Some(Outcome::Invalid(DiscardReason::Internal))
             }
             #[cfg(feature = "processing")]
@@ -1738,7 +1741,11 @@ impl EnvelopeProcessor {
                 process_value(event, &mut processor, ProcessingState::root())
                     .map_err(ProcessingError::ProcessingFailed)?;
             }
-            if let Some(config) = config.datascrubbing_settings.pii_config() {
+            let pii_config = config
+                .datascrubbing_settings
+                .pii_config()
+                .map_err(|e| ProcessingError::PiiConfigError(e.clone()))?;
+            if let Some(config) = pii_config {
                 let mut processor = PiiProcessor::new(config.compiled());
                 process_value(event, &mut processor, ProcessingState::root())
                     .map_err(ProcessingError::ProcessingFailed)?;