feat(relay): do not assume empty means {{auto}} for some sdks

CHANGELOG.md

@@ -11,6 +11,7 @@
 
 - Track an utilization metric for internal services. ([#4501](https://github.com/getsentry/relay/pull/4501))
 - Add new `relay-threading` crate with asynchronous thread pool. ([#4500](https://github.com/getsentry/relay/pull/4500))
+- Do not convert empty ip address into `{{auto}}` for newer SDKs. ([#4519](https://github.com/getsentry/relay/pull/4519))
 
 ## 25.2.0
 

relay-event-normalization/src/event.rs

@@ -27,6 +27,7 @@ use smallvec::SmallVec;
 use uuid::Uuid;
 
 use crate::normalize::request;
+use crate::sdk_version::SdkVersion;
 use crate::span::ai::normalize_ai_measurements;
 use crate::span::tag_extraction::extract_span_tags_from_event;
 use crate::utils::{self, get_event_user_tag, MAX_DURATION_MOBILE_MS};
@@ -37,6 +38,9 @@ use crate::{
     RawUserAgentInfo, SpanDescriptionRule, TransactionNameConfig,
 };
 
+const CUTOFF_JAVASCRIPT_VERSION: SdkVersion = SdkVersion::new(9, 0, 0);
+const CUTOFF_COCOA_VERSION: SdkVersion = SdkVersion::new(6, 2, 0);
+
 /// Configuration for [`normalize_event`].
 #[derive(Clone, Debug)]
 pub struct NormalizationConfig<'a> {
@@ -260,6 +264,7 @@ fn normalize(event: &mut Event, meta: &mut Meta, config: &NormalizationConfig) {
         &mut event.user,
         event.platform.as_str(),
         config.client_ip,
+        &event.client_sdk,
     );
 
     if let Some(geoip_lookup) = config.geoip_lookup {
@@ -412,6 +417,7 @@ pub fn normalize_ip_addresses(
     user: &mut Annotated<User>,
     platform: Option<&str>,
     client_ip: Option<&IpAddr>,
+    client_sdk: &Annotated<ClientSdkInfo>,
 ) {
     // NOTE: This is highly order dependent, in the sense that both the statements within this
     // function need to be executed in a certain order, and that other normalization code
@@ -467,9 +473,35 @@ pub fn normalize_ip_addresses(
                 .iter_remarks()
                 .any(|r| r.ty == RemarkType::Removed);
             if !scrubbed_before {
-                // In an ideal world all SDKs would set {{auto}} explicitly.
+                // We stop assuming that empty means {{auto}} for the affected platforms
+                // starting with the versions listed below.
                 if let Some("javascript") | Some("cocoa") | Some("objc") = platform {
-                    user.ip_address = Annotated::new(client_ip.to_owned());
+                    if let Some(Ok(sdk_version)) = client_sdk
+                        .0
+                        .as_ref()
+                        .and_then(|sdk| sdk.version.0.as_ref())
+                        .map(|s| SdkVersion::try_parse(s.as_str()))
+                    {
+                        match platform {
+                            Some("javascript") => {
+                                if sdk_version < CUTOFF_JAVASCRIPT_VERSION {
+                                    user.ip_address = Annotated::new(client_ip.to_owned());
+                                }
+                            }
+                            Some("cocoa") => {
+                                if sdk_version < CUTOFF_COCOA_VERSION {
+                                    user.ip_address = Annotated::new(client_ip.to_owned());
+                                }
+                            }
+                            // The obj-c SDK is deprecated in favour of cocoa so we keep the
+                            // old behavior of empty == {{auto}}.
+                            // With the `scrubbed_before` check we made sure that we only
+                            // derive empty as {{auto}} if the empty string comes from the SDK
+                            // directly and is not the result of scrubbing.
+                            Some("objc") => user.ip_address = Annotated::new(client_ip.to_owned()),
+                            _ => {}
+                        }
+                    }
                 }
             }
         }
@@ -1462,16 +1494,15 @@ mod tests {
 
     use std::collections::BTreeMap;
 
+    use super::*;
+    use crate::{ClientHints, MeasurementsConfig, ModelCost};
     use insta::assert_debug_snapshot;
     use itertools::Itertools;
     use relay_common::glob2::LazyGlob;
     use relay_event_schema::protocol::{Breadcrumb, Csp, DebugMeta, DeviceContext, Values};
     use relay_protocol::{get_value, SerializableAnnotated};
     use serde_json::json;
 
-    use super::*;
-    use crate::{ClientHints, MeasurementsConfig, ModelCost};
-
     const IOS_MOBILE_EVENT: &str = r#"
         {
             "sdk": {"name": "sentry.cocoa"},

relay-event-normalization/src/lib.rs

@@ -17,6 +17,7 @@ mod normalize;
 mod regexes;
 mod remove_other;
 mod schema;
+mod sdk_version;
 mod stacktrace;
 mod statsd;
 mod timestamp;
@@ -26,6 +27,7 @@ mod validation;
 
 pub use validation::{validate_event, validate_span, EventValidationConfig};
 pub mod replay;
+
 pub use event::{
     normalize_event, normalize_measurements, normalize_performance_score, NormalizationConfig,
 };

relay-event-normalization/src/normalize/mod.rs

@@ -454,8 +454,10 @@ mod tests {
             },
         );
 
-        let ip_addr = get_value!(event.user.ip_address!);
-        assert_eq!(ip_addr, &IpAddr("2.125.160.216".to_string()));
+        assert_eq!(
+            Annotated::empty(),
+            event.0.unwrap().user.0.unwrap().ip_address
+        );
     }
 
     #[test]

relay-event-normalization/src/replay.rs

@@ -128,6 +128,7 @@ fn normalize_ip_address(replay: &mut Replay, ip_address: Option<StdIpAddr>) {
         &mut replay.user,
         replay.platform.as_str(),
         ip_address.map(|ip| IpAddr(ip.to_string())).as_ref(),
+        &replay.sdk,
     );
 }
 
@@ -310,8 +311,10 @@ mod tests {
             None,
         );
 
-        let ipaddr = get_value!(replay.user!).ip_address.as_str();
-        assert_eq!(Some("127.0.0.1"), ipaddr);
+        assert_eq!(
+            Annotated::empty(),
+            replay.0.unwrap().user.0.unwrap().ip_address
+        );
     }
 
     #[test]

relay-event-normalization/src/sdk_version.rs

@@ -0,0 +1,118 @@
+use std::num::ParseIntError;
+
+/// Represents an SDK Version using the semvar versioning, meaning MAJOR.MINOR.PATCH.
+/// An optional release type can be specified, then it becomes
+/// MAJOR.MINOR.PATCH-(alpha|beta).RELEASE_VERSION
+#[derive(Debug, PartialOrd, Eq, PartialEq, Ord)]
+pub struct SdkVersion {
+    major: usize,
+    minor: usize,
+    patch: usize,
+    release_type: ReleaseType,
+}
+
+/// Represents the release type which might be present in a version string,
+/// for example: 9.0.0-alpha.0.
+/// Release types are also comparable to each other, using the rules:
+/// alpha < beta < release.
+/// **NOTE**: The discriminants are explicitly set to keep the comparison rules
+///           even if the order is changed.
+#[derive(Debug, Eq, PartialEq, Ord, PartialOrd)]
+enum ReleaseType {
+    Alpha = 1,
+    Beta = 2,
+    Release = 3,
+}
+
+impl SdkVersion {
+    pub const fn new(major: usize, minor: usize, patch: usize) -> Self {
+        Self {
+            major,
+            minor,
+            patch,
+            release_type: ReleaseType::Release,
+        }
+    }
+
+    /// Attempts to parse a version string and returning a [`ParseIntError`] if it contains any
+    /// non-numerical characters apart from dots.
+    /// If a version part is not provided, 0 will be assumed.
+    /// For example:
+    /// 1.2 -> 1.2.0
+    /// 1   -> 1.0.0
+    pub fn try_parse(input: &str) -> Result<Self, ParseIntError> {
+        let mut split = input.split(".");
+        let major = split.next().map(str::parse).transpose()?.unwrap_or(0);
+        let minor = split.next().map(str::parse).transpose()?.unwrap_or(0);
+        let (patch, release_type) = if let Some(next) = split.next() {
+            match next.split_once("-") {
+                Some((patch, release_type)) => (
+                    patch.parse()?,
+                    if release_type.starts_with("alpha") {
+                        ReleaseType::Alpha
+                    } else {
+                        ReleaseType::Beta
+                    },
+                ),
+                None => (next.parse()?, ReleaseType::Release),
+            }
+        } else {
+            (0, ReleaseType::Release)
+        };
+        Ok(Self {
+            major,
+            minor,
+            patch,
+            release_type,
+        })
+    }
+}
+
+#[cfg(test)]
+mod test {
+    use crate::sdk_version::SdkVersion;
+
+    #[test]
+    fn test_version_compare() {
+        let main_version = SdkVersion::new(1, 2, 3);
+        let less = SdkVersion::new(1, 2, 1);
+        let greater = SdkVersion::new(2, 1, 1);
+        let equal = SdkVersion::new(1, 2, 3);
+        assert!(main_version > less);
+        assert!(main_version < greater);
+        assert_eq!(main_version, equal);
+    }
+
+    #[test]
+    fn test_version_string_compare() {
+        let main_version = SdkVersion::try_parse("1.2.3").unwrap();
+        let less = SdkVersion::try_parse("1.2.1").unwrap();
+        let greater = SdkVersion::try_parse("2.1.1").unwrap();
+        let equal = SdkVersion::try_parse("1.2.3").unwrap();
+        assert!(main_version > less);
+        assert!(main_version < greater);
+        assert_eq!(main_version, equal);
+    }
+
+    #[test]
+    fn test_release_type() {
+        let alpha = SdkVersion::try_parse("9.0.0-alpha.2").unwrap();
+        let beta = SdkVersion::try_parse("9.0.0-beta.2").unwrap();
+        let release = SdkVersion::try_parse("9.0.0").unwrap();
+
+        assert!(alpha < beta);
+        assert!(beta < release);
+        assert!(alpha < release)
+    }
+
+    #[test]
+    fn test_version_defaults_to_zero() {
+        let only_major = SdkVersion::try_parse("9").unwrap();
+        assert_eq!(only_major, SdkVersion::new(9, 0, 0))
+    }
+
+    #[test]
+    fn test_version_string_parse_failed() {
+        assert!(SdkVersion::try_parse("amd64").is_err());
+    }
+}

tests/fixtures/replay_missing_user_ip_address.json

@@ -30,7 +30,8 @@
     "user": {
         "id": "123",
         "username": "user",
-        "email": "[email protected]"
+        "email": "[email protected]",
+        "ip_address": "{{auto}}"
     },
     "request": {
         "url": null,

tests/integration/test_normalization.py

@@ -391,3 +391,95 @@ def test_ip_normalization_with_remove_remark(mini_sentry, relay_chain):
     event = envelope.get_event()
     assert event["user"]["ip_address"] is None
     assert event["user"]["id"] == "AE12FE3B5F129B5CC4CDD2B136B7B7947C4D2741"
+
+
+@pytest.mark.parametrize(
+    "platform, version, expected",
+    [
+        ("javascript", "8.55", "127.0.0.1"),
+        ("javascript", "9", None),
+        ("cocoa", "6.1.4", "127.0.0.1"),
+        ("cocoa", "6.2", None),
+        ("objc", "6.1.4", "127.0.0.1"),
+        ("objc", "6.2", "127.0.0.1"),
+        ("random_sdk", None, None),
+    ],
+    ids=[
+        "Javascript SDK version before cut-off",
+        "Javascript SDK version same to cut-off",
+        "Cocoa SDK version before cut-off",
+        "Cocoa SDK version same to cut-off",
+        "Obj-C SDK any version",
+        "Obj-C SDK with other version",
+        "random SDK string without any version",
+    ],
+)
+def test_empty_ip_not_auto(mini_sentry, relay, platform, version, expected):
+    project_id = 42
+    relay = relay(mini_sentry)
+
+    mini_sentry.add_basic_project_config(project_id)
+
+    relay.send_event(
+        project_id,
+        {
+            "platform": platform,
+            "sdk": {"version": version},
+            "user": {"ip_address": ""},
+        },
+    )
+
+    envelope = mini_sentry.captured_events.get(timeout=1)
+    event = envelope.get_event()
+    assert event["user"]["ip_address"] == expected
+
+
+@pytest.mark.parametrize(
+    "platform, version, expected",
+    [
+        ("javascript", "8.55", "89.128.74.91"),
+        ("javascript", "9", "89.128.74.91"),
+        ("cocoa", "6.1.4", "89.128.74.91"),
+        ("cocoa", "6.2", "89.128.74.91"),
+        ("objc", "6.1.4", "89.128.74.91"),
+        ("objc", "6.2", "89.128.74.91"),
+        ("random_sdk", None, "89.128.74.91"),
+    ],
+)
+def test_explicit_ip_version_cutoff(mini_sentry, relay, platform, version, expected):
+    project_id = 42
+    relay = relay(mini_sentry)
+
+    mini_sentry.add_basic_project_config(project_id)
+
+    relay.send_event(
+        project_id,
+        {
+            "platform": platform,
+            "sdk": {"version": version},
+            "user": {"ip_address": "89.128.74.91"},
+        },
+    )
+
+    envelope = mini_sentry.captured_events.get(timeout=1)
+    event = envelope.get_event()
+    assert event["user"]["ip_address"] == expected
+
+
+@pytest.mark.parametrize(
+    "scrub_ip_addresses, expected", [(True, None), (False, "127.0.0.1")]
+)
+def test_ip_address_scrubbed(mini_sentry, relay, scrub_ip_addresses, expected):
+    project_id = 42
+    relay = relay(mini_sentry)
+
+    config = mini_sentry.add_basic_project_config(project_id)
+    config["config"].setdefault("datascrubbingSettings", {})[
+        "scrubIpAddresses"
+    ] = scrub_ip_addresses
+
+    relay.send_event(project_id, {"user": {"ip_address": "{{auto}}"}})
+
+    envelope = mini_sentry.captured_events.get(timeout=1)
+    event = envelope.get_event()
+    assert event["user"]["ip_address"] == expected