fix(metrics): Remove/reject nul-bytes from metric strings [ingest-1204]

CHANGELOG.md

@@ -7,6 +7,10 @@
 - Add platform, op, http.method and status tag to all extracted transaction metrics. ([#1227](https://github.com/getsentry/relay/pull/1227))
 - Add units in built-in measurements. ([#1229](https://github.com/getsentry/relay/pull/1229))
 
+**Internal**:
+
+- Remove/reject nul-bytes from metric strings. ([#1235](https://github.com/getsentry/relay/pull/1235))
+
 ## 22.4.0
 
 **Features**:

relay-general/src/protocol/event.rs

@@ -343,7 +343,7 @@ pub struct Event {
     /// can be the git SHA for the given project, or a product identifier with a semantic version.
     #[metastructure(
         max_chars = "tag_value",  // release ends in tag
-        deny_chars = "\r\n\x0c\t/\\",
+        // release allowed chars are validated in the sentry-release-parser crate!
         required = "false",
         trim_whitespace = "true",
         nonempty = "true",
@@ -374,7 +374,7 @@ pub struct Event {
     /// ```
     #[metastructure(
         max_chars = "environment",
-        deny_chars = "\r\n\x0C/",
+        // environment allowed chars are validated in the sentry-release-parser crate!
         nonempty = "true",
         required = "false",
         trim_whitespace = "true"

relay-general/src/store/schema.rs

@@ -105,7 +105,7 @@ fn verify_value_characters(
 mod tests {
     use super::SchemaProcessor;
     use crate::processor::{process_value, ProcessingState};
-    use crate::types::{Annotated, Array, Error, Object, Value};
+    use crate::types::{Annotated, Array, Error, Object};
 
     fn assert_nonempty_base<T>()
     where
@@ -148,29 +148,6 @@ mod tests {
         assert_nonempty_base::<Object<u64>>();
     }
 
-    #[test]
-    fn test_release_invalid_newlines() {
-        use crate::protocol::Event;
-
-        let mut event = Annotated::new(Event {
-            release: Annotated::new("a\nb".to_string().into()),
-            ..Default::default()
-        });
-
-        process_value(&mut event, &mut SchemaProcessor, ProcessingState::root()).unwrap();
-
-        assert_eq_dbg!(
-            event,
-            Annotated::new(Event {
-                release: Annotated::from_error(
-                    Error::invalid("invalid character \'\\n\'"),
-                    Some(Value::String("a\nb".into())),
-                ),
-                ..Default::default()
-            })
-        );
-    }
-
     #[test]
     fn test_invalid_email() {
         use crate::protocol::User;

relay-metrics/src/aggregation.rs

@@ -745,6 +745,31 @@ impl BucketKey {
         std::hash::Hash::hash(self, &mut hasher);
         hasher.finalize() as i64
     }
+
+    /// Remove invalid characters from tags and metric names.
+    ///
+    /// Returns `Err` if the metric should be dropped.
+    fn validate_strings(mut self) -> Result<Self, ()> {
+        if self.metric_name.contains('\0') {
+            relay_log::error!("invalid metric name {:?}", self.metric_name);
+            return Err(());
+        }
+
+        self.metric_name.retain(|c| c != '\0');
+        self.tags.retain(|tag_key, _| {
+            if !tag_key.contains('\0') {
+                true
+            } else {
+                relay_log::error!("invalid metric tag key {:?}", tag_key);
+                false
+            }
+        });
+        for (_, tag_value) in self.tags.iter_mut() {
+            tag_value.retain(|c| c != '\0');
+        }
+
+        Ok(self)
+    }
 }
 
 /// Parameters used by the [`Aggregator`].
@@ -1055,6 +1080,8 @@ impl Aggregator {
         let timestamp = key.timestamp;
         let project_key = key.project_key;
 
+        let key = key.validate_strings().map_err(|()| AggregateMetricsError)?;
+
         match self.buckets.entry(key) {
             Entry::Occupied(mut entry) => {
                 relay_statsd::metric!(
@@ -2078,4 +2105,50 @@ mod tests {
             rounded_now + 10
         );
     }
+
+    #[test]
+    fn test_validate_strings() {
+        let project_key = ProjectKey::parse("a94ae32be2584e0bbd7a4cbb95971fee").unwrap();
+
+        let bucket_key = BucketKey {
+            project_key,
+            timestamp: UnixTimestamp::now(),
+            metric_name: "hergus.bergus".to_owned(),
+            metric_type: MetricType::Counter,
+            metric_unit: MetricUnit::None,
+            tags: {
+                let mut tags = BTreeMap::new();
+                // There are some SDKs which mess up content encodings, and interpret the raw bytes
+                // of an UTF-16 string as UTF-8. Leading to ASCII
+                // strings getting null-bytes interleaved.
+                //
+                // Somehow those values end up as release tag in sessions, while in error events we
+                // haven't observed this malformed encoding. We believe it's slightly better to
+                // strip out NUL-bytes instead of dropping the tag such that those values line up
+                // again across sessions and events. Should that cause too high cardinality we'll
+                // have to drop tags.
+                //
+                // Note that releases are validated separately against much stricter character set,
+                // but the above idea should still apply to other tags.
+                tags.insert(
+                    "is_it_garbage".to_owned(),
+                    "a\0b\0s\0o\0l\0u\0t\0e\0l\0y".to_owned(),
+                );
+                tags.insert("another\0garbage".to_owned(), "bye".to_owned());
+                tags
+            },
+        };
+
+        let mut bucket_key = bucket_key.validate_strings().unwrap();
+
+        assert_eq!(bucket_key.tags.len(), 1);
+        assert_eq!(
+            bucket_key.tags.get("is_it_garbage"),
+            Some(&"absolutely".to_owned())
+        );
+        assert_eq!(bucket_key.tags.get("another\0garbage"), None);
+
+        bucket_key.metric_name = "hergus\0bergus".to_owned();
+        bucket_key.validate_strings().unwrap_err();
+    }
 }