Repository: getsentry/sentry-python

Issue:
Run EventScrubber with send_default_pii=True
### Problem Statement

In your data scrubber docs it says,

> You can use the event_scrubber configuration parameter to simplify removing sensitive data from your event payload.

> If send_default_pii is set to False, the default scrubber implementation will run automatically and filter anything in the [denylist](https://github.com/getsentry/sentry-python/blob/1.18.0/sentry_sdk/scrubber.py#L17-L58) from [potentially sensitive interfaces](https://docs.sentry.io/platforms/python/data-collected/) in the event payload.

We're in Django. What we actually want is send default PII (request.user data, etc) but scrub security information (passwords, etc)

Using your `EventScrubber`, extending the denylist a bit, would be enough for us. The issue is that to use it as it is designed, `send_default_pii` should be False. So instead, I choose to run the event scrubber inside `before_send()` with `send_default_pii=True`. 

However, I've found that as the event scrubber updates the event dict with AnnotatedValue objects, it can't be serialized into JSON.

> TypeError: Object of type AnnotatedValue is not JSON serializable

In order to use the `EventScrubber`, I guess we could use the `sentry_sdk.serializer.serialize()` method inside before_send(), and that worked as it takes care of AnnotatedValue objects, but that results in serializing the object twice (once in your SDK, and once inside before_send)
https://github.com/getsentry/sentry-python/blob/1.19.1/sentry_sdk/client.py#L315-L329

### Solution Brainstorm

One option would be to serialize() the event after before_send()

Another option would be to [remove](https://github.com/getsentry/sentry-python/blob/1.19.1/sentry_sdk/client.py#L317) the `send_default_pii=False` condition? Why wouldn't it be possible to run the event scrubber if `send_default_pii=True`? is it because `self.options["event_scrubber"]` has always a EventScrubber instance?

Discussion:
codecov[bot]: ### :x: 4 Tests Failed:
| Tests completed | Failed | Passed | Skipped |
|---|---|---|---|
| 843 | 4 | 839 | 7 |
<details><summary>View the top 3 failed tests by shortest run time</summary>

> 
> ```
> tests.integrations.django.asgi.test_asgi test_asgi_request_body[True-POST-headers2-post_echo_async-{&quot;username&quot;:&quot;xyz&quot;,&quot;password&quot;:&quot;xyz&quot;}-expected_data2-application0]
> ```
> 
> <details><summary>Stack Traces | 0.422s run time</summary>
> 
> > 
> > ```
> > .../django/asgi/test_asgi.py:543: in test_asgi_request_body
> >     assert event["request"]["data"] == expected_data
> > E   AssertionError: assert {'username': 'xyz', 'password': '[Filtered]'} == {'username': 'xyz', 'password': 'xyz'}
> > E     
> > E     Common items:
> > E     {'username': 'xyz'}
> > E     Differing items:
> > E     {'password': '[Filtered]'} != {'password': 'xyz'}
> > E     
> > E     Full diff:
> > E       {
> > E     -     'password': 'xyz',
> > E     +     'password': '[Filtered]',
> > E           'username': 'xyz',
> > E       }
> > ```
> 
> </details>


> 
> ```
> tests.integrations.django.asgi.test_asgi test_asgi_request_body[True-POST-headers4-post_echo_async---fd721ef49ea403a6\r\nContent-Disposition: form-data; name=&quot;username&quot;\r\n\r\nJane\r\n--fd721ef49ea403a6\r\nContent-Disposition: form-data; name=&quot;password&quot;\r\n\r\nhello123\r\n--fd721ef49ea403a6\r\nContent-Disposition: form-data; name=&quot;photo&quot;; filename=&quot;image.png&quot;\r\nContent-Type: image/png\r\nContent-Transfer-Encoding: base64\r\n\r\niVBORw0KGgoAAAANSUhEUgAAAAoAAAAJCAYAAAALpr0TAAAA+0lEQVQYV2P8DwQMSKCvcxGYV1QehyzMwIiscMvGgwzPn74GK5CUFmXw8beHK0ZR6O+exzB9Xi0DLy8XQ2ZSM4TNxw1WDFf4DGjS82evGfqBVte1ZDLcuvkAbHpqVghCIUgRyAQjU02GyBgvBpA7uycWMZTm9wE1ZTBIAZ0BNhEkAbIO5K5bNx4yqGnIg00zMtViWLFkO1BTMQPjzesP/ktKiTA01c5gSM0MAZsGMqWpZgbDjPm1DI0108GeYowOLv8PcvTnz98YZk9fw2AMNAUEDu47w1DXnAFmg5zFmJ7Q9J+RERIKoBBNzQpmaAaaDgK1QIWgIPsCNAQAgad47ThM9dkAAAAASUVORK5CYII=\r\n--fd721ef49ea403a6--\r\n-expected_data4-application0]
> ```
> 
> <details><summary>Stack Traces | 0.422s run time</summary>
> 
> > 
> > ```
> > .../django/asgi/test_asgi.py:543: in test_asgi_request_body
> >     assert event["request"]["data"] == expected_data
> > E   AssertionError: assert {'username': 'Jane', 'password': '[Filtered]', 'photo': ''} == {'password': 'hello123', 'photo': '', 'username': 'Jane'}
> > E     
> > E     Common items:
> > E     {'photo': '', 'username': 'Jane'}
> > E     Differing items:
> > E     {'password': '[Filtered]'} != {'password': 'hello123'}
> > E     
> > E     Full diff:
> > E       {
> > E     -     'password': 'hello123',
> > E     +     'password': '[Filtered]',
> > E           'photo': '',
> > E           'username': 'Jane',
> > E       }
> > ```
> 
> </details>


> 
> ```
> tests.integrations.django.asgi.test_asgi test_asgi_request_body[True-POST-headers4-post_echo_async---fd721ef49ea403a6\r\nContent-Disposition: form-data; name=&quot;username&quot;\r\n\r\nJane\r\n--fd721ef49ea403a6\r\nContent-Disposition: form-data; name=&quot;password&quot;\r\n\r\nhello123\r\n--fd721ef49ea403a6\r\nContent-Disposition: form-data; name=&quot;photo&quot;; filename=&quot;image.png&quot;\r\nContent-Type: image/png\r\nContent-Transfer-Encoding: base64\r\n\r\niVBORw0KGgoAAAANSUhEUgAAAAoAAAAJCAYAAAALpr0TAAAA+0lEQVQYV2P8DwQMSKCvcxGYV1QehyzMwIiscMvGgwzPn74GK5CUFmXw8beHK0ZR6O+exzB9Xi0DLy8XQ2ZSM4TNxw1WDFf4DGjS82evGfqBVte1ZDLcuvkAbHpqVghCIUgRyAQjU02GyBgvBpA7uycWMZTm9wE1ZTBIAZ0BNhEkAbIO5K5bNx4yqGnIg00zMtViWLFkO1BTMQPjzesP/ktKiTA01c5gSM0MAZsGMqWpZgbDjPm1DI0108GeYowOLv8PcvTnz98YZk9fw2AMNAUEDu47w1DXnAFmg5zFmJ7Q9J+RERIKoBBNzQpmaAaaDgK1QIWgIPsCNAQAgad47ThM9dkAAAAASUVORK5CYII=\r\n--fd721ef49ea403a6--\r\n-expected_data4-application1]
> ```
> 
> <details><summary>Stack Traces | 0.423s run time</summary>
> 
> > 
> > ```
> > .../django/asgi/test_asgi.py:543: in test_asgi_request_body
> >     assert event["request"]["data"] == expected_data
> > E   AssertionError: assert {'username': 'Jane', 'password': '[Filtered]', 'photo': ''} == {'password': 'hello123', 'photo': '', 'username': 'Jane'}
> > E     
> > E     Common items:
> > E     {'photo': '', 'username': 'Jane'}
> > E     Differing items:
> > E     {'password': '[Filtered]'} != {'password': 'hello123'}
> > E     
> > E     Full diff:
> > E       {
> > E     -     'password': 'hello123',
> > E     +     'password': '[Filtered]',
> > E           'photo': '',
> > E           'username': 'Jane',
> > E       }
> > ```
> 
> </details>

</details>

To view individual test run time comparison to the main branch, go to the [Test Analytics Dashboard](https://app.codecov.io/gh/getsentry/sentry-python/tests/neel/scrubber-pii-separate-lis)
sl0thentr0py: docs here
https://github.com/getsentry/sentry-docs/pull/11154

Patch:
diff --git a/sentry_sdk/client.py b/sentry_sdk/client.py
index b224cd1f..f8bc7677 100644
--- a/sentry_sdk/client.py
+++ b/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 @@ class _Client(BaseClient):
 
         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
diff --git a/sentry_sdk/scrubber.py b/sentry_sdk/scrubber.py
index 8eb01944..2bd0c8e4 100644
--- a/sentry_sdk/scrubber.py
+++ b/sentry_sdk/scrubber.py
@@ -25,21 +25,17 @@ DEFAULT_DENYLIST = [
     "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
@@ -55,11 +51,35 @@ DEFAULT_DENYLIST = [
     "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
 


Test patch:
diff --git a/tests/integrations/django/asgi/test_asgi.py b/tests/integrations/django/asgi/test_asgi.py
index abc27ccf..57a6faea 100644
--- a/tests/integrations/django/asgi/test_asgi.py
+++ b/tests/integrations/django/asgi/test_asgi.py
@@ -434,7 +434,7 @@ BODY_FORM_CONTENT_LENGTH = str(len(BODY_FORM)).encode("utf-8")
             [(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 @@ BODY_FORM_CONTENT_LENGTH = str(len(BODY_FORM)).encode("utf-8")
             ],
             "post_echo_async",
             BODY_FORM,
-            {"password": "hello123", "photo": "", "username": "Jane"},
+            {"password": "[Filtered]", "photo": "", "username": "Jane"},
         ),
         (
             False,
diff --git a/tests/test_scrubber.py b/tests/test_scrubber.py
index 5034121b..a544c31c 100644
--- a/tests/test_scrubber.py
+++ b/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,6 +180,7 @@ 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"][
@@ -153,6 +188,7 @@ def test_custom_denylist(sentry_init, capture_events):
     ]
     assert meta == {
         "my_sensitive_var": {"": {"rem": [["!config", "s"]]}},
+        "my_pii_var": {"": {"rem": [["!config", "s"]]}},
     }