fix(pii): prevent ip derivation if already scrubbed before (#4491)

This PR adds remarks if the user IP was PII scrubbed to prevent assuming
that empty IP means `{{auto}}` for certain platforms.

fix #4480

CHANGELOG.md

@@ -16,6 +16,7 @@
 
 - Fix a bug where parsing large unsigned integers would fail incorrectly. ([#4472](https://github.com/getsentry/relay/pull/4472))
 - Set size limit for UserReport attachments so it gets properly reported as too large. ([#4482](https://github.com/getsentry/relay/pull/4482))
+- Fix a bug where scrubbed IP addresses were derived again on certain platforms. ([#4491](https://github.com/getsentry/relay/pull/4491))
 - Improve stripping of SQL comments during Insights query normalization. ([#4493](https://github.com/getsentry/relay/pull/4493))
 
 **Internal**:

relay-event-normalization/src/event.rs

@@ -460,9 +460,17 @@ pub fn normalize_ip_addresses(
         let user = user.value_mut().get_or_insert_with(User::default);
         // auto is already handled above
         if user.ip_address.value().is_none() {
-            // In an ideal world all SDKs would set {{auto}} explicitly.
-            if let Some("javascript") | Some("cocoa") | Some("objc") = platform {
-                user.ip_address = Annotated::new(client_ip.to_owned());
+            // Only assume that empty means {{auto}} if there is no remark that the IP address has been removed.
+            let scrubbed_before = user
+                .ip_address
+                .meta()
+                .iter_remarks()
+                .any(|r| r.ty == RemarkType::Removed);
+            if !scrubbed_before {
+                // In an ideal world all SDKs would set {{auto}} explicitly.
+                if let Some("javascript") | Some("cocoa") | Some("objc") = platform {
+                    user.ip_address = Annotated::new(client_ip.to_owned());
+                }
             }
         }
     }

relay-pii/src/processor.rs

@@ -227,6 +227,10 @@ impl Processor for PiiProcessor<'_> {
         // wiped out in renormalization anyway.
         if ip_was_valid && !has_other_fields && !ip_is_still_valid {
             user.id = mem::take(&mut user.ip_address).map_value(|ip| ip.into_inner().into());
+            user.ip_address.meta_mut().add_remark(Remark::new(
+                RemarkType::Removed,
+                "pii:ip_address".to_string(),
+            ));
         }
 
         Ok(())
@@ -529,7 +533,6 @@ fn apply_regex_to_chunks<'a>(
             insert_replacement_chunks(rule, &search_string, &mut rv);
         }
     }
-
     rv
 }
 

tests/integration/test_normalization.py

@@ -375,3 +375,19 @@ def test_relay_chain_normalizes_minidump_events(
 
     assert event["exception"]["values"] is not None
     assert event["type"] == "error"
+
+
+@pytest.mark.parametrize("relay_chain", ["relay->relay->sentry"], indirect=True)
+def test_ip_normalization_with_remove_remark(mini_sentry, relay_chain):
+    project_id = 42
+    relay = relay_chain(min_relay_version="25.01.0")
+
+    config = mini_sentry.add_basic_project_config(project_id)
+    config["config"]["piiConfig"]["applications"]["$user.ip_address"] = ["@ip:hash"]
+
+    relay.send_event(project_id, {"platform": "javascript"})
+
+    envelope = mini_sentry.captured_events.get(timeout=1)
+    event = envelope.get_event()
+    assert event["user"]["ip_address"] is None
+    assert event["user"]["id"] == "AE12FE3B5F129B5CC4CDD2B136B7B7947C4D2741"