ref(integrations): route HTTP header filtering through data collection config#6788
ref(integrations): route HTTP header filtering through data collection config#6788ericapisani wants to merge 26 commits into
Conversation
…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
Codecov Results 📊✅ 92209 passed | ⏭️ 6302 skipped | Total: 98511 | Pass Rate: 93.6% | Execution Time: 318m 28s 📊 Comparison with Base Branch
➖ Removed Tests (1)View removed tests
All tests are passing successfully. ✅ Patch coverage is 100.00%. Project has 2478 uncovered lines. Files with missing lines (1)
Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
+ Coverage 89.68% 89.70% +0.02%
==========================================
Files 193 193 —
Lines 24007 24048 +41
Branches 8342 8372 +30
==========================================
+ Hits 21529 21570 +41
- Misses 2478 2478 —
- Partials 1387 1388 +1Generated by Codecov Action |
… _experiments property in the client
…still being needed for the url attribute
…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`.
| headers = _get_headers(asgi_scope) | ||
|
|
||
| request_data["headers"] = _filter_headers( | ||
| headers, | ||
| use_annotated_value=False, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
As long as this doesn't affect current behavior should be ok 👍🏻
…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
… _experiments property in the client
…still being needed for the url attribute
…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`.
951c408 to
d717172
Compare
…ntry/sentry-python into py-2584-update-wsgi-filter-headers
The streaming path no longer emits a client span when there is no current span (#6810), so unpack only the server span.
sentrivana
left a comment
There was a problem hiding this comment.
Lgtm! Left some suggestions.
| for key in filtered: | ||
| if isinstance(key, str) and key.lower() in ("cookie", "set-cookie"): | ||
| filtered[key] = SENSITIVE_DATA_SUBSTITUTE |
| _experiments={ | ||
| "trace_lifecycle": "stream", | ||
| "data_collection": options["data_collection"], | ||
| }, |
There was a problem hiding this comment.
| _experiments={ | |
| "trace_lifecycle": "stream", | |
| "data_collection": options["data_collection"], | |
| }, | |
| trace_lifecycle="stream", | |
| _experiments={ | |
| "data_collection": options["data_collection"], | |
| }, |
| # 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"] |
There was a problem hiding this comment.
Why have this here in the first place? It doesn't look related to sensitive header behavior
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2e2f4f0. Configure here.

_filter_headersfollows the legacysend_default_pii/use_annotated_valuebehaviour when the data collection behaviour is not enabled in_experiments.When data collection is enabled, it now delegates to
_apply_key_value_collection_filteringfromsentry_sdk.data_collection, so header scrubbing respects the newdata_collection.http_headers.requestallowlist/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