Skip to content

ref(integrations): route HTTP header filtering through data collection config#6788

Open
ericapisani wants to merge 22 commits into
masterfrom
py-2584-update-wsgi-filter-headers
Open

ref(integrations): route HTTP header filtering through data collection config#6788
ericapisani wants to merge 22 commits into
masterfrom
py-2584-update-wsgi-filter-headers

Conversation

@ericapisani

@ericapisani ericapisani commented Jul 8, 2026

Copy link
Copy Markdown
Member

_filter_headers follows the legacy send_default_pii/use_annotated_value behaviour when the data collection behaviour is not enabled in _experiments.

When data collection is enabled, it now delegates to _apply_key_value_collection_filtering from sentry_sdk.data_collection, so header scrubbing respects the new data_collection.http_headers.request allowlist/denylist/off configuration when data collection.

Work to scrub cookies in a more granular way will be tackled as part of PY-2581/#6741.

Fixes PY-2584
Fixes #6744

…n config

`_filter_headers` previously used a hardcoded sensitive-header tuple and a
`send_default_pii`/`use_annotated_value` toggle. It now delegates to
`_apply_key_value_collection_filtering` from `sentry_sdk.data_collection`,
so header scrubbing respects the new `data_collection.http_headers.request`
allowlist/denylist/off configuration.

Cookie and set-cookie headers are
always redacted regardless of mode. Drops the now-unused
`use_annotated_value` parameter from all call sites.

Work to scrub cookies in a more granular way will be tackled as part of
PY-2581/#6741.

Fixes PY-2584
Fixes #6744
@linear-code

linear-code Bot commented Jul 8, 2026

Copy link
Copy Markdown

PY-2584

PY-2581

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Codecov Results 📊

92209 passed | ⏭️ 6302 skipped | Total: 98511 | Pass Rate: 93.6% | Execution Time: 316m 4s

📊 Comparison with Base Branch

Metric Change
Total Tests 📈 +295
Passed Tests 📈 +295
Failed Tests
Skipped Tests

All tests are passing successfully.

✅ Patch coverage is 100.00%. Project has 2478 uncovered lines.
✅ Project coverage is 89.69%. Comparing base (base) to head (head).

Files with missing lines (1)
File Patch % Lines
sentry_sdk/data_collection.py 100.00% ⚠️ 1 partials
Coverage diff
@@            Coverage Diff             @@
##          main       #PR       +/-##
==========================================
+ Coverage    89.67%    89.69%    +0.02%
==========================================
  Files          193       193         —
  Lines        24004     24045       +41
  Branches      8340      8370       +30
==========================================
+ Hits         21526     21567       +41
- Misses        2478      2478         —
- Partials      1387      1387         —

Generated by Codecov Action

Comment thread sentry_sdk/data_collection.py
Comment thread sentry_sdk/integrations/_wsgi_common.py
…ures

The new lambda_functions_with_embedded_sdk fixture directories were
missing the .gitignore that the other fixtures use to keep everything
except index.py untracked. As a result, certifi and urllib3 packages
installed by the test setup got committed, and ruff failed CI linting
against them since they're unmodified third-party code.

Add the missing .gitignore to each new fixture directory and remove
the committed vendored packages; they are regenerated automatically at
test time via `uv pip install --target`.
Comment on lines +118 to +122
headers = _get_headers(asgi_scope)

request_data["headers"] = _filter_headers(
headers,
use_annotated_value=False,

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This change was done because when if the headers are filtered with "allowlist" and no terms are provided, and the "host" header is present, then the constructed URL in _get_url below would contain [Filtered] within the URL.

Doing this ensures that it gets filtered from the headers to respect data collection, but still correctly appears in the url

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As long as this doesn't affect current behavior should be ok 👍🏻

@ericapisani
ericapisani marked this pull request as ready for review July 9, 2026 22:48
@ericapisani
ericapisani requested a review from a team as a code owner July 9, 2026 22:48
…n config

`_filter_headers` previously used a hardcoded sensitive-header tuple and a
`send_default_pii`/`use_annotated_value` toggle. It now delegates to
`_apply_key_value_collection_filtering` from `sentry_sdk.data_collection`,
so header scrubbing respects the new `data_collection.http_headers.request`
allowlist/denylist/off configuration.

Cookie and set-cookie headers are
always redacted regardless of mode. Drops the now-unused
`use_annotated_value` parameter from all call sites.

Work to scrub cookies in a more granular way will be tackled as part of
PY-2581/#6741.

Fixes PY-2584
Fixes #6744
…ures

The new lambda_functions_with_embedded_sdk fixture directories were
missing the .gitignore that the other fixtures use to keep everything
except index.py untracked. As a result, certifi and urllib3 packages
installed by the test setup got committed, and ruff failed CI linting
against them since they're unmodified third-party code.

Add the missing .gitignore to each new fixture directory and remove
the committed vendored packages; they are regenerated automatically at
test time via `uv pip install --target`.
@ericapisani
ericapisani force-pushed the py-2584-update-wsgi-filter-headers branch from 951c408 to d717172 Compare July 14, 2026 17:43
…ntry/sentry-python into py-2584-update-wsgi-filter-headers
Comment thread sentry_sdk/data_collection.py
@ericapisani
ericapisani marked this pull request as draft July 15, 2026 19:23
The streaming path no longer emits a client span when there is no current
span (#6810), so unpack only the server span.
@ericapisani
ericapisani marked this pull request as ready for review July 15, 2026 19:47
Comment thread sentry_sdk/data_collection.py
Comment thread sentry_sdk/integrations/_wsgi_common.py

@sentrivana sentrivana left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Lgtm! Left some suggestions.

Comment on lines +220 to +222
for key in filtered:
if isinstance(key, str) and key.lower() in ("cookie", "set-cookie"):
filtered[key] = SENSITIVE_DATA_SUBSTITUTE

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Work to scrub cookies in a more granular way will be tackled as part of PY-2581/#6741.

This means this cookies logic will be updated later right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Correct, it's part of #6797 😄

Comment on lines +1378 to +1381
_experiments={
"trace_lifecycle": "stream",
"data_collection": options["data_collection"],
},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
_experiments={
"trace_lifecycle": "stream",
"data_collection": options["data_collection"],
},
trace_lifecycle="stream",
_experiments={
"data_collection": options["data_collection"],
},


(server_span,) = [item.payload for item in items]

if request.node.callspec.id.endswith("data_collection_off_does_not_add_headers"):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we, instead of matching on the id here, just match on expected is None?

Suggested change
if request.node.callspec.id.endswith("data_collection_off_does_not_add_headers"):
if expected is None:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah, that works here 👍🏻

Comment on lines +1423 to +1431
# client.address and user.ip_address is captured under send_default_pii=True.
# TODO: This block will eventually need to be removed from this test into a separate
# test once data collection gating is introduced on these values
if options["send_default_pii"]:
assert server_span["attributes"]["client.address"] == "127.0.0.1"
assert server_span["attributes"]["user.ip_address"] == "127.0.0.1"
else:
assert "user.ip_address" not in server_span["attributes"]
assert "client.address" not in server_span["attributes"]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why have this here in the first place? It doesn't look related to sensitive header behavior

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It isn't related to sensitive header behaviour, but I had decided to add this in to make it clear that it isn't expected that user data would be present when data collection is enabled just yet.

Since assertions on these values appear in other tests within the file, I thought that not having assertions like these in a data collection-specific context would raise flags.

}


def test_get_request_data_url_with_filtered_host(sentry_init):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we change these new test cases to set up a proper ASGI app, like the tests below? Just to make them a little less unit-testy.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I missed this earlier on, but I addressed this in https://github.com/getsentry/sentry-python/pull/6841/changes#diff-a4ca49dac9c5fcce27a4fb5fb53e5f8e764b807773ba7c5476d70d74a9789353 as

I can look into pulling that change into this branch, but if we wanted to merge this stack 1 big feature branch, it won't make a difference as it'll eventually be applied.

assert len(spans) == 1

segment = spans[0]
if request.node.callspec.id == "data_collection_off_does_not_add_headers":

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same suggestion as above, to make this a bit more future-proof if someone decides to change the ID.

Suggested change
if request.node.callspec.id == "data_collection_off_does_not_add_headers":
if expected is None:

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.

Update _filter_headers to check data collection settings when available

2 participants