Add separate pii_denylist to EventScrubber and run it always

Author: sl0thentr0py

sentry_sdk/client.py

@@ -125,7 +125,7 @@ def _get_options(*args, **kwargs):
         rv["traces_sample_rate"] = 1.0
 
     if rv["event_scrubber"] is None:
-        rv["event_scrubber"] = EventScrubber()
+        rv["event_scrubber"] = EventScrubber(send_default_pii=rv["send_default_pii"])
 
     if rv["socket_options"] and not isinstance(rv["socket_options"], list):
         logger.warning(
@@ -526,7 +526,7 @@ def _prepare_event(
 
         if event is not None:
             event_scrubber = self.options["event_scrubber"]
-            if event_scrubber and not self.options["send_default_pii"]:
+            if event_scrubber:
                 event_scrubber.scrub_event(event)
 
         # Postprocess the event here so that annotated types do

sentry_sdk/scrubber.py

@@ -24,21 +24,17 @@
     "privatekey",
     "private_key",
     "token",
-    "ip_address",
     "session",
     # django
     "csrftoken",
     "sessionid",
     # wsgi
-    "remote_addr",
     "x_csrftoken",
     "x_forwarded_for",
     "set_cookie",
     "cookie",
     "authorization",
     "x_api_key",
-    "x_forwarded_for",
-    "x_real_ip",
     # other common names used in the wild
     "aiohttp_session",  # aiohttp
     "connect.sid",  # Express
@@ -54,11 +50,35 @@
     "XSRF-TOKEN",  # Angular, Laravel
 ]
 
+DEFAULT_PII_DENYLIST = [
+    "x_forwarded_for",
+    "x_real_ip",
+    "ip_address",
+    "remote_addr",
+]
+
 
 class EventScrubber(object):
-    def __init__(self, denylist=None, recursive=False):
-        # type: (Optional[List[str]], bool) -> None
-        self.denylist = DEFAULT_DENYLIST if denylist is None else denylist
+    def __init__(
+        self, denylist=None, recursive=False, send_default_pii=False, pii_denylist=None
+    ):
+        # type: (Optional[List[str]], bool, bool, Optional[List[str]]) -> None
+        """
+        A scrubber that goes through the event payload and removes sensitive data configured through denylists.
+
+        :param denylist: A security denylist that is always scrubbed, defaults to DEFAULT_DENYLIST.
+        :param recursive: Whether to scrub the event payload recursively, default False.
+        :param send_default_pii: Whether pii is sending is on, pii fields are not scrubbed.
+        :param pii_denylist: The denylist to use for scrubbing when pii is not sent, defaults to DEFAULT_PII_DENYLIST.
+        """
+        self.denylist = DEFAULT_DENYLIST.copy() if denylist is None else denylist
+
+        if not send_default_pii:
+            pii_denylist = (
+                DEFAULT_PII_DENYLIST.copy() if pii_denylist is None else pii_denylist
+            )
+            self.denylist += pii_denylist
+
         self.denylist = [x.lower() for x in self.denylist]
         self.recursive = recursive
 

tests/integrations/django/asgi/test_asgi.py

@@ -434,7 +434,7 @@ async def test_trace_from_headers_if_performance_disabled(sentry_init, capture_e
             [(b"content-type", b"application/json")],
             "post_echo_async",
             b'{"username":"xyz","password":"xyz"}',
-            {"username": "xyz", "password": "xyz"},
+            {"username": "xyz", "password": "[Filtered]"},
         ),
         (
             True,
@@ -453,7 +453,7 @@ async def test_trace_from_headers_if_performance_disabled(sentry_init, capture_e
             ],
             "post_echo_async",
             BODY_FORM,
-            {"password": "hello123", "photo": "", "username": "Jane"},
+            {"password": "[Filtered]", "photo": "", "username": "Jane"},
         ),
         (
             False,

tests/test_scrubber.py

@@ -25,6 +25,7 @@ def test_request_scrubbing(sentry_init, capture_events):
                 "COOKIE": "secret",
                 "authorization": "Bearer bla",
                 "ORIGIN": "google.com",
+                "ip_address": "127.0.0.1",
             },
             "cookies": {
                 "sessionid": "secret",
@@ -45,6 +46,7 @@ def test_request_scrubbing(sentry_init, capture_events):
             "COOKIE": "[Filtered]",
             "authorization": "[Filtered]",
             "ORIGIN": "google.com",
+            "ip_address": "[Filtered]",
         },
         "cookies": {"sessionid": "[Filtered]", "foo": "bar"},
         "data": {"token": "[Filtered]", "foo": "bar"},
@@ -54,12 +56,39 @@ def test_request_scrubbing(sentry_init, capture_events):
         "headers": {
             "COOKIE": {"": {"rem": [["!config", "s"]]}},
             "authorization": {"": {"rem": [["!config", "s"]]}},
+            "ip_address": {"": {"rem": [["!config", "s"]]}},
         },
         "cookies": {"sessionid": {"": {"rem": [["!config", "s"]]}}},
         "data": {"token": {"": {"rem": [["!config", "s"]]}}},
     }
 
 
+def test_ip_address_not_scrubbed_when_pii_enabled(sentry_init, capture_events):
+    sentry_init(send_default_pii=True)
+    events = capture_events()
+
+    try:
+        1 / 0
+    except ZeroDivisionError:
+        ev, _hint = event_from_exception(sys.exc_info())
+
+        ev["request"] = {"headers": {"COOKIE": "secret", "ip_address": "127.0.0.1"}}
+
+        capture_event(ev)
+
+    (event,) = events
+
+    assert event["request"] == {
+        "headers": {"COOKIE": "[Filtered]", "ip_address": "127.0.0.1"}
+    }
+
+    assert event["_meta"]["request"] == {
+        "headers": {
+            "COOKIE": {"": {"rem": [["!config", "s"]]}},
+        }
+    }
+
+
 def test_stack_var_scrubbing(sentry_init, capture_events):
     sentry_init()
     events = capture_events()
@@ -131,11 +160,16 @@ def test_span_data_scrubbing(sentry_init, capture_events):
 
 
 def test_custom_denylist(sentry_init, capture_events):
-    sentry_init(event_scrubber=EventScrubber(denylist=["my_sensitive_var"]))
+    sentry_init(
+        event_scrubber=EventScrubber(
+            denylist=["my_sensitive_var"], pii_denylist=["my_pii_var"]
+        )
+    )
     events = capture_events()
 
     try:
         my_sensitive_var = "secret"  # noqa
+        my_pii_var = "jane.doe"  # noqa
         safe = "keepthis"  # noqa
         1 / 0
     except ZeroDivisionError:
@@ -146,13 +180,15 @@ def test_custom_denylist(sentry_init, capture_events):
     frames = event["exception"]["values"][0]["stacktrace"]["frames"]
     (frame,) = frames
     assert frame["vars"]["my_sensitive_var"] == "[Filtered]"
+    assert frame["vars"]["my_pii_var"] == "[Filtered]"
     assert frame["vars"]["safe"] == "'keepthis'"
 
     meta = event["_meta"]["exception"]["values"]["0"]["stacktrace"]["frames"]["0"][
         "vars"
     ]
     assert meta == {
         "my_sensitive_var": {"": {"rem": [["!config", "s"]]}},
+        "my_pii_var": {"": {"rem": [["!config", "s"]]}},
     }
 
 

tests/tracing/test_decorator.py

@@ -39,7 +39,7 @@ def test_trace_decorator_no_trx():
             assert result == "return_of_sync_function"
 
             result2 = start_child_span_decorator(my_example_function)()
-            fake_debug.assert_called_once_with(
+            fake_debug.assert_called_with(
                 "Cannot create a child span for %s. "
                 "Please start a Sentry transaction before calling this function.",
                 "test_decorator.my_example_function",