Skip to content

Add option to drop scrubbed user IP addresses#6241

Open
juliosuas wants to merge 1 commit intogetsentry:masterfrom
juliosuas:fix/5701-drop-scrubbed-user-ip
Open

Add option to drop scrubbed user IP addresses#6241
juliosuas wants to merge 1 commit intogetsentry:masterfrom
juliosuas:fix/5701-drop-scrubbed-user-ip

Conversation

@juliosuas
Copy link
Copy Markdown

@juliosuas juliosuas commented May 8, 2026

Summary

Fixes #5701 by adding an EventScrubber option to remove user.ip_address instead of replacing it with [Filtered].

This avoids emitting [Filtered] in a field Relay expects to be a valid IP address, while preserving the existing default behavior for backward compatibility. By default, user.ip_address is still scrubbed through the normal scrubber path and keeps the _meta annotation.

Testing

  • python -m pytest tests/test_scrubber.py tests/new_scopes_compat/test_new_scopes_compat_event.py -q
  • ruff check sentry_sdk/scrubber.py tests/test_scrubber.py
  • ruff format --check sentry_sdk/scrubber.py tests/test_scrubber.py
  • git diff --check

@juliosuas juliosuas requested a review from a team as a code owner May 8, 2026 15:50
Comment thread sentry_sdk/scrubber.py Outdated
Comment on lines +141 to +143
if isinstance(user, dict) and "ip_address" in self.denylist:
user.pop("ip_address", None)
self.scrub_dict(user)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Removing user.ip_address via pop instead of letting the scrubber replace it breaks backward-compatibility tests that expect the field to be present and marked as "[Filtered]".
Severity: MEDIUM

Suggested Fix

Revert the logic to allow the scrub_dict function to handle the scrubbing of ip_address. Instead of preemptively removing the key with user.pop("ip_address", None), let the existing scrubbing mechanism process it. This will replace the value with "[Filtered]" and generate the necessary _meta annotations, maintaining backward compatibility.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: sentry_sdk/scrubber.py#L141-L143

Potential issue: When `send_default_pii=False`, the new logic in `EventScrubber`
preemptively removes `user.ip_address` by calling `user.pop("ip_address", None)`. This
occurs before the `scrub_dict` function is executed. As a result, `scrub_dict` never
processes the `ip_address` field, so it is not replaced with `"[Filtered]"` and the
corresponding `_meta` scrubbing annotation is not created. This breaks several
backward-compatibility tests in
`tests/new_scopes_compat/test_new_scopes_compat_event.py` that assert the presence of
the scrubbed value and its metadata.

Did we get this right? 👍 / 👎 to inform future reviews.

@juliosuas juliosuas force-pushed the fix/5701-drop-scrubbed-user-ip branch 2 times, most recently from a6083fb to 1f0e89b Compare May 8, 2026 17:54
@juliosuas juliosuas changed the title Drop scrubbed user IP addresses Add option to drop scrubbed user IP addresses May 8, 2026
Copy link
Copy Markdown
Member

@ericapisani ericapisani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for opening a PR! These changes are generally looking good, just one small adjustment before we're good to ship :shipit:

Comment thread sentry_sdk/scrubber.py Outdated
Comment on lines +141 to +142
if isinstance(user, dict) and "ip_address" in self.denylist:
user.pop("ip_address", None)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the right idea, but let's make one further optimization:

Suggested change
if isinstance(user, dict) and "ip_address" in self.denylist:
user.pop("ip_address", None)
if "ip_address" in self.denylist and isinstance(user, dict):
user.pop("ip_address", None)

By moving the "ip_address" in self.denylist earlier in the conditional and evaluating it first, it means that we can ignore the second condition if the first isn't satisfied.

@juliosuas juliosuas force-pushed the fix/5701-drop-scrubbed-user-ip branch from 1f0e89b to 26e6a22 Compare May 8, 2026 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Processing error due to invalid IP but an IP is shown in the event information

2 participants