-
Notifications
You must be signed in to change notification settings - Fork 642
ref(integrations): route HTTP header filtering through data collection config #6788
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
7541460
5e9f610
a590c8e
16ed0c8
486f0bc
7d14a81
bb98334
61d1618
951c408
4dd3ba6
d0bae90
a6c4688
d7b378b
52c2da1
aa645d7
8b6e3ad
d717172
c7f590f
e9f0e20
3d7bddb
ec90ac6
5773973
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -115,9 +115,13 @@ def _get_request_data( | |
| if ty in ("http", "websocket"): | ||
| request_data["method"] = asgi_scope.get("method") | ||
|
|
||
| request_data["headers"] = headers = _filter_headers( | ||
| _get_headers(asgi_scope), | ||
| headers = _get_headers(asgi_scope) | ||
|
|
||
| request_data["headers"] = _filter_headers( | ||
| headers, | ||
| use_annotated_value=False, | ||
|
Comment on lines
+118
to
+122
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Doing this ensures that it gets filtered from the headers to respect data collection, but still correctly appears in the url
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As long as this doesn't affect current behavior should be ok 👍🏻 |
||
| ) | ||
|
|
||
| request_data["query_string"] = _get_query(asgi_scope) | ||
|
|
||
| request_data["url"] = _get_url( | ||
|
|
@@ -148,8 +152,10 @@ def _get_request_attributes( | |
| if asgi_scope.get("method"): | ||
| attributes["http.request.method"] = asgi_scope["method"].upper() | ||
|
|
||
| headers = _filter_headers(_get_headers(asgi_scope), use_annotated_value=False) | ||
| for header, value in headers.items(): | ||
| headers = _get_headers(asgi_scope) | ||
|
|
||
| filtered_headers = _filter_headers(headers, use_annotated_value=False) | ||
| for header, value in filtered_headers.items(): | ||
| attributes[f"http.request.header.{header.lower()}"] = value | ||
|
|
||
| if should_send_default_pii(): | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,8 +3,9 @@ | |
|
|
||
| import sentry_sdk | ||
| from sentry_sdk._types import SENSITIVE_DATA_SUBSTITUTE | ||
| from sentry_sdk.data_collection import _apply_key_value_collection_filtering | ||
|
ericapisani marked this conversation as resolved.
|
||
| from sentry_sdk.scope import should_send_default_pii | ||
| from sentry_sdk.utils import AnnotatedValue, logger | ||
| from sentry_sdk.utils import AnnotatedValue, has_data_collection_enabled, logger | ||
|
|
||
| try: | ||
| from django.http.request import RawPostDataException | ||
|
|
@@ -206,19 +207,39 @@ def _filter_headers( | |
| headers: "Mapping[str, str]", | ||
| use_annotated_value: bool = True, | ||
| ) -> "Mapping[str, Union[AnnotatedValue, str]]": | ||
| if should_send_default_pii(): | ||
| return headers | ||
| client_options = sentry_sdk.get_client().options | ||
|
|
||
| substitute: "Union[AnnotatedValue, str]" = ( | ||
| SENSITIVE_DATA_SUBSTITUTE | ||
| if not use_annotated_value | ||
| else AnnotatedValue.removed_because_over_size_limit() | ||
| ) | ||
| if has_data_collection_enabled(client_options): | ||
| data_collection_configuration = client_options["data_collection"] | ||
|
|
||
| filtered = _apply_key_value_collection_filtering( | ||
| items=headers, | ||
| behaviour=data_collection_configuration["http_headers"]["request"], | ||
| ) | ||
|
|
||
| for key in filtered: | ||
| if isinstance(key, str) and key.lower() in ("cookie", "set-cookie"): | ||
| filtered[key] = SENSITIVE_DATA_SUBSTITUTE | ||
|
Comment on lines
+220
to
+222
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This means this cookies logic will be updated later right?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct, it's part of #6797 😄 |
||
|
|
||
| return { | ||
| k: (v if k.upper().replace("-", "_") not in SENSITIVE_HEADERS else substitute) | ||
| for k, v in headers.items() | ||
| } | ||
| return filtered | ||
|
ericapisani marked this conversation as resolved.
|
||
| else: | ||
| if should_send_default_pii(): | ||
| return headers | ||
|
|
||
| substitute: "Union[AnnotatedValue, str]" = ( | ||
| SENSITIVE_DATA_SUBSTITUTE | ||
| if not use_annotated_value | ||
| else AnnotatedValue.removed_because_over_size_limit() | ||
| ) | ||
|
|
||
| return { | ||
| k: ( | ||
| v | ||
| if k.upper().replace("-", "_") not in SENSITIVE_HEADERS | ||
| else substitute | ||
| ) | ||
| for k, v in headers.items() | ||
| } | ||
|
|
||
|
|
||
| def _in_http_status_code_range( | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1254,8 +1254,185 @@ async def hello(request): | |||||||||||||||||
| ) | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| @pytest.mark.parametrize( | ||||||||||||||||||
| "options,expected", | ||||||||||||||||||
| [ | ||||||||||||||||||
| pytest.param( | ||||||||||||||||||
| { | ||||||||||||||||||
| "send_default_pii": True, | ||||||||||||||||||
| "data_collection": None, | ||||||||||||||||||
| }, | ||||||||||||||||||
| { | ||||||||||||||||||
| "authorization": "[Filtered]", | ||||||||||||||||||
| "custom": "foobar", | ||||||||||||||||||
| "cookie": "[Filtered]", | ||||||||||||||||||
| }, | ||||||||||||||||||
| id="enabled_send_default_pii_redacts_auth_header_due_to_data_collection_default_settings", | ||||||||||||||||||
| ), | ||||||||||||||||||
| pytest.param( | ||||||||||||||||||
| { | ||||||||||||||||||
| "send_default_pii": False, | ||||||||||||||||||
| "data_collection": None, | ||||||||||||||||||
| }, | ||||||||||||||||||
| { | ||||||||||||||||||
| "authorization": "[Filtered]", | ||||||||||||||||||
| "custom": "foobar", | ||||||||||||||||||
| "cookie": "[Filtered]", | ||||||||||||||||||
| }, | ||||||||||||||||||
| id="disabled_send_default_pii_redacts_auth_header_due_to_data_collection_default_settings", | ||||||||||||||||||
| ), | ||||||||||||||||||
| pytest.param( | ||||||||||||||||||
| { | ||||||||||||||||||
| "send_default_pii": False, | ||||||||||||||||||
| "data_collection": {"http_headers": {"request": {"mode": "off"}}}, | ||||||||||||||||||
| }, | ||||||||||||||||||
| None, | ||||||||||||||||||
| id="data_collection_off_does_not_add_headers", | ||||||||||||||||||
| ), | ||||||||||||||||||
| pytest.param( | ||||||||||||||||||
| { | ||||||||||||||||||
| "send_default_pii": False, | ||||||||||||||||||
| "data_collection": {"http_headers": {"request": {"mode": "allowlist"}}}, | ||||||||||||||||||
| }, | ||||||||||||||||||
| { | ||||||||||||||||||
| "authorization": "[Filtered]", | ||||||||||||||||||
| "custom": "[Filtered]", | ||||||||||||||||||
| "cookie": "[Filtered]", | ||||||||||||||||||
| }, | ||||||||||||||||||
| id="data_collection_allow_list_redacts_terms_that_do_not_appear", | ||||||||||||||||||
| ), | ||||||||||||||||||
| pytest.param( | ||||||||||||||||||
| { | ||||||||||||||||||
| "send_default_pii": False, | ||||||||||||||||||
| "data_collection": { | ||||||||||||||||||
| "http_headers": { | ||||||||||||||||||
| "request": {"mode": "allowlist", "terms": ["Authorization"]} | ||||||||||||||||||
| } | ||||||||||||||||||
| }, | ||||||||||||||||||
| }, | ||||||||||||||||||
| { | ||||||||||||||||||
| "authorization": "[Filtered]", | ||||||||||||||||||
| "custom": "[Filtered]", | ||||||||||||||||||
| "cookie": "[Filtered]", | ||||||||||||||||||
| }, | ||||||||||||||||||
| id="data_collection_allow_list_redacts_sensitive_terms_even_when_provided_by_user", | ||||||||||||||||||
| ), | ||||||||||||||||||
| pytest.param( | ||||||||||||||||||
| { | ||||||||||||||||||
| "send_default_pii": False, | ||||||||||||||||||
| "data_collection": { | ||||||||||||||||||
| "http_headers": { | ||||||||||||||||||
| "request": {"mode": "allowlist", "terms": ["custom"]} | ||||||||||||||||||
| } | ||||||||||||||||||
| }, | ||||||||||||||||||
| }, | ||||||||||||||||||
| { | ||||||||||||||||||
| "authorization": "[Filtered]", | ||||||||||||||||||
| "custom": "foobar", | ||||||||||||||||||
| "cookie": "[Filtered]", | ||||||||||||||||||
| }, | ||||||||||||||||||
| id="data_collection_allow_list_does_not_redact_provided_term", | ||||||||||||||||||
| ), | ||||||||||||||||||
| pytest.param( | ||||||||||||||||||
| { | ||||||||||||||||||
| "send_default_pii": False, | ||||||||||||||||||
| "data_collection": { | ||||||||||||||||||
| "http_headers": { | ||||||||||||||||||
| "request": {"mode": "denylist", "terms": ["custom"]} | ||||||||||||||||||
| } | ||||||||||||||||||
| }, | ||||||||||||||||||
| }, | ||||||||||||||||||
| { | ||||||||||||||||||
| "authorization": "[Filtered]", | ||||||||||||||||||
| "custom": "[Filtered]", | ||||||||||||||||||
| "cookie": "[Filtered]", | ||||||||||||||||||
| }, | ||||||||||||||||||
| id="data_collection_deny_list_redacts_sensitive_terms_when_provided_by_user", | ||||||||||||||||||
| ), | ||||||||||||||||||
| pytest.param( | ||||||||||||||||||
| { | ||||||||||||||||||
| "send_default_pii": False, | ||||||||||||||||||
| "data_collection": { | ||||||||||||||||||
| "http_headers": { | ||||||||||||||||||
| "request": {"mode": "allowlist", "terms": ["cookie"]} | ||||||||||||||||||
| } | ||||||||||||||||||
| }, | ||||||||||||||||||
| }, | ||||||||||||||||||
| { | ||||||||||||||||||
| "authorization": "[Filtered]", | ||||||||||||||||||
| "custom": "[Filtered]", | ||||||||||||||||||
| "cookie": "[Filtered]", | ||||||||||||||||||
| }, | ||||||||||||||||||
| id="data_collection_cookie_is_always_redacted_even_when_allow_listed", | ||||||||||||||||||
| ), | ||||||||||||||||||
| ], | ||||||||||||||||||
| ) | ||||||||||||||||||
| @pytest.mark.asyncio | ||||||||||||||||||
| async def test_sensitive_header_passthrough_with_pii_span_streaming( | ||||||||||||||||||
| sentry_init, aiohttp_client, capture_items, options, expected, request | ||||||||||||||||||
| ): | ||||||||||||||||||
| sentry_init( | ||||||||||||||||||
| integrations=[AioHttpIntegration()], | ||||||||||||||||||
| traces_sample_rate=1.0, | ||||||||||||||||||
| send_default_pii=options["send_default_pii"], | ||||||||||||||||||
| _experiments={ | ||||||||||||||||||
| "trace_lifecycle": "stream", | ||||||||||||||||||
| "data_collection": options["data_collection"], | ||||||||||||||||||
| }, | ||||||||||||||||||
|
Comment on lines
+1378
to
+1381
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||
| ) | ||||||||||||||||||
|
|
||||||||||||||||||
| async def hello(request): | ||||||||||||||||||
| return web.Response(text="hello") | ||||||||||||||||||
|
|
||||||||||||||||||
| app = web.Application() | ||||||||||||||||||
| app.router.add_get("/", hello) | ||||||||||||||||||
|
|
||||||||||||||||||
| items = capture_items("span") | ||||||||||||||||||
|
|
||||||||||||||||||
| client = await aiohttp_client(app) | ||||||||||||||||||
| await client.get( | ||||||||||||||||||
| "/", | ||||||||||||||||||
| headers={ | ||||||||||||||||||
| "Authorization": "Bearer secret-token", | ||||||||||||||||||
| "x-custom-header": "foobar", | ||||||||||||||||||
| "Cookie": "sessionid=secret", | ||||||||||||||||||
| }, | ||||||||||||||||||
| ) | ||||||||||||||||||
|
|
||||||||||||||||||
| sentry_sdk.flush() | ||||||||||||||||||
|
|
||||||||||||||||||
| (server_span,) = [item.payload for item in items] | ||||||||||||||||||
|
|
||||||||||||||||||
| if request.node.callspec.id.endswith("data_collection_off_does_not_add_headers"): | ||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we, instead of matching on the id here, just match on
Suggested change
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that works here 👍🏻 |
||||||||||||||||||
| assert "http.request.header.authorization" not in server_span["attributes"] | ||||||||||||||||||
| assert "http.request.header.cookie" not in server_span["attributes"] | ||||||||||||||||||
| else: | ||||||||||||||||||
| assert ( | ||||||||||||||||||
| server_span["attributes"]["http.request.header.authorization"] | ||||||||||||||||||
| == expected["authorization"] | ||||||||||||||||||
| ) | ||||||||||||||||||
| assert ( | ||||||||||||||||||
| server_span["attributes"]["http.request.header.x-custom-header"] | ||||||||||||||||||
| == expected["custom"] | ||||||||||||||||||
| ) | ||||||||||||||||||
| assert ( | ||||||||||||||||||
| server_span["attributes"]["http.request.header.cookie"] | ||||||||||||||||||
| == expected["cookie"] | ||||||||||||||||||
| ) | ||||||||||||||||||
|
|
||||||||||||||||||
| # 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"] | ||||||||||||||||||
|
Comment on lines
+1423
to
+1431
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| @pytest.mark.asyncio | ||||||||||||||||||
| async def test_sensitive_header_passthrough_with_pii_span_streaming_without_data_collection( | ||||||||||||||||||
| sentry_init, aiohttp_client, capture_items | ||||||||||||||||||
| ): | ||||||||||||||||||
| sentry_init( | ||||||||||||||||||
|
|
||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.