ref(event): Add correct limit and validation to distribution name [INGEST-1615]

CHANGELOG.md

@@ -6,6 +6,10 @@
 
 - Support decaying rules. Decaying rules are regular sampling rules, but they are only applicable in a specific time range. ([#1544](https://github.com/getsentry/relay/pull/1544))
 
+**Bug Fixes**:
+
+- Validate the distrubution name in the event. ([#1556](https://github.com/getsentry/relay/pull/1556))
+
 **Internal**:
 
 - Implement response context schema. ([#1529](https://github.com/getsentry/relay/pull/1529))

relay-general/derive/src/lib.rs

@@ -529,6 +529,7 @@ fn parse_max_chars(name: &str) -> TokenStream {
         "tag_key" => quote!(crate::processor::MaxChars::TagKey),
         "tag_value" => quote!(crate::processor::MaxChars::TagValue),
         "environment" => quote!(crate::processor::MaxChars::Environment),
+        "distribution" => quote!(crate::processor::MaxChars::Distribution),
         _ => panic!("invalid max_chars variant '{}'", name),
     }
 }

relay-general/src/processor/attrs.rs

@@ -101,6 +101,7 @@ pub enum MaxChars {
     TagKey,
     TagValue,
     Environment,
+    Distribution,
     Hard(usize),
     Soft(usize),
 }
@@ -123,6 +124,7 @@ impl MaxChars {
             MaxChars::TagKey => 32,
             MaxChars::TagValue => 200,
             MaxChars::Environment => 64,
+            MaxChars::Distribution => 64,
             MaxChars::Soft(len) | MaxChars::Hard(len) => len,
         }
     }
@@ -143,6 +145,7 @@ impl MaxChars {
             MaxChars::TagKey => 0,
             MaxChars::TagValue => 0,
             MaxChars::Environment => 0,
+            MaxChars::Distribution => 0,
             MaxChars::Soft(_) => 10,
             MaxChars::Hard(_) => 0,
         }

relay-general/src/protocol/event.rs

@@ -365,7 +365,7 @@ pub struct Event {
     /// an application. For example, the dist can be the build number of an XCode build or the
     /// version code of an Android build.
     #[metastructure(
-        max_chars = "tag_value",  // dist ends in tag
+        max_chars = "distribution",
         allow_chars = "a-zA-Z0-9_.-",
         trim_whitespace = "true",
         required = "false",

relay-general/src/store/normalize.rs

@@ -90,20 +90,19 @@ pub fn is_valid_platform(platform: &str) -> bool {
     VALID_PLATFORMS.contains(&platform)
 }
 
-pub fn normalize_dist(dist: &mut Option<String>) {
-    let mut erase = false;
-    if let Some(val) = dist {
-        if val.is_empty() {
-            erase = true;
-        }
-        let trimmed = val.trim();
-        if trimmed != val {
-            *val = trimmed.to_string()
+pub fn normalize_dist(distribution: &mut Annotated<String>) -> ProcessingResult {
+    distribution.apply(|dist, meta| {
+        let trimmed = dist.trim();
+        if trimmed.is_empty() {
+            return Err(ProcessingAction::DeleteValueHard);
+        } else if bytecount::num_chars(trimmed.as_bytes()) > MaxChars::Distribution.limit() {
+            meta.add_error(Error::new(ErrorKind::ValueTooLong));
+            return Err(ProcessingAction::DeleteValueSoft);
+        } else if trimmed != dist {
+            *dist = trimmed.to_string();
         }
-    }
-    if erase {
-        *dist = None;
-    }
+        Ok(())
+    })
 }
 
 /// Compute additional measurements derived from existing ones.
@@ -545,8 +544,8 @@ fn normalize_timestamps(
 }
 
 /// Ensures that the `release` and `dist` fields match up.
-fn normalize_release_dist(event: &mut Event) {
-    normalize_dist(event.dist.value_mut());
+fn normalize_release_dist(event: &mut Event) -> ProcessingResult {
+    normalize_dist(&mut event.dist)
 }
 
 fn is_security_report(event: &Event) -> bool {
@@ -715,7 +714,7 @@ pub fn light_normalize_event(
 
         // Default required attributes, even if they have errors
         normalize_logentry(&mut event.logentry, meta)?;
-        normalize_release_dist(event); // dist is a tag extracted along with other metrics from transactions
+        normalize_release_dist(event)?; // dist is a tag extracted along with other metrics from transactions
         normalize_timestamps(
             event,
             meta,
@@ -1713,6 +1712,32 @@ mod tests {
         );
     }
 
+    #[test]
+    fn test_too_long_distribution() {
+        let json = r#"{
+  "event_id": "52df9022835246eeb317dbd739ccd059",
+  "fingerprint": [
+    "{{ default }}"
+  ],
+  "platform": "other",
+  "dist": "52df9022835246eeb317dbd739ccd059-52df9022835246eeb317dbd739ccd059-52df9022835246eeb317dbd739ccd059"
+}"#;
+
+        let mut event = Annotated::<Event>::from_json(json).unwrap();
+
+        let mut processor = NormalizeProcessor::default();
+        let config = LightNormalizationConfig::default();
+        light_normalize_event(&mut event, &config).unwrap();
+        process_value(&mut event, &mut processor, ProcessingState::root()).unwrap();
+
+        let dist = &event.value().unwrap().dist;
+        let result = &Annotated::<String>::from_error(
+            Error::new(ErrorKind::ValueTooLong),
+            Some(Value::String("52df9022835246eeb317dbd739ccd059-52df9022835246eeb317dbd739ccd059-52df9022835246eeb317dbd739ccd059".to_string()))
+        );
+        assert_eq!(dist, result);
+    }
+
     #[test]
     fn test_regression_backfills_abs_path_even_when_moving_stacktrace() {
         let mut event = Annotated::new(Event {
@@ -1964,30 +1989,30 @@ mod tests {
 
     #[test]
     fn test_normalize_dist_none() {
-        let mut dist = None;
-        normalize_dist(&mut dist);
-        assert_eq!(dist, None);
+        let mut dist = Annotated::default();
+        normalize_dist(&mut dist).unwrap();
+        assert_eq!(dist.value(), None);
     }
 
     #[test]
     fn test_normalize_dist_empty() {
-        let mut dist = Some("".to_owned());
-        normalize_dist(&mut dist);
-        assert_eq!(dist, None);
+        let mut dist = Annotated::new("".to_string());
+        normalize_dist(&mut dist).unwrap();
+        assert_eq!(dist.value(), None);
     }
 
     #[test]
     fn test_normalize_dist_trim() {
-        let mut dist = Some(" foo  ".to_owned());
-        normalize_dist(&mut dist);
-        assert_eq!(dist.unwrap(), "foo");
+        let mut dist = Annotated::new(" foo  ".to_string());
+        normalize_dist(&mut dist).unwrap();
+        assert_eq!(dist.value(), Some(&"foo".to_string()));
     }
 
     #[test]
     fn test_normalize_dist_whitespace() {
-        let mut dist = Some(" ".to_owned());
-        normalize_dist(&mut dist);
-        assert_eq!(dist.unwrap(), ""); // Not sure if this is what we want
+        let mut dist = Annotated::new(" ".to_owned());
+        normalize_dist(&mut dist).unwrap();
+        assert_eq!(dist.value(), None);
     }
 
     #[test]