-
Notifications
You must be signed in to change notification settings - Fork 643
feat(aiohttp): Apply data_collection filtering to URL query strings #6833
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
a19be39
620e222
ec6f520
4dd3ba6
d0bae90
a6c4688
d7b378b
52c2da1
aa645d7
8b6e3ad
d717172
c7f590f
293a291
530e83c
326d796
e9f0e20
48f7c7b
621d79b
3d7bddb
fad2292
40d9f38
e6e7148
1f9bce7
fad0c9a
21cf40b
3f0bc2a
eee4ea2
407ad17
b6b6e1e
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 |
|---|---|---|
|
|
@@ -4,7 +4,10 @@ | |
|
|
||
| import sentry_sdk | ||
| from sentry_sdk.api import continue_trace | ||
| from sentry_sdk.consts import OP, SPANDATA, SPANSTATUS | ||
|
Check warning on line 7 in sentry_sdk/integrations/aiohttp.py
|
||
| from sentry_sdk.data_collection import ( | ||
| _apply_data_collection_filtering_to_query_string, | ||
| ) | ||
| from sentry_sdk.integrations import ( | ||
| _DEFAULT_FAILED_REQUEST_STATUS_CODES, | ||
| DidNotEnable, | ||
|
|
@@ -46,6 +49,7 @@ | |
| capture_internal_exceptions, | ||
| ensure_integration_enabled, | ||
| event_from_exception, | ||
| has_data_collection_enabled, | ||
| logger, | ||
| parse_url, | ||
| parse_version, | ||
|
|
@@ -161,7 +165,26 @@ | |
| ) | ||
|
|
||
| url_attributes = {} | ||
| if should_send_default_pii(): | ||
| if has_data_collection_enabled(client.options): | ||
| url_attributes["url.full"] = "%s://%s%s" % ( | ||
|
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. I know this is just copied from the Probably makes more sense to fix this afterwards, when this stack has been merged. I'll make an issue for it.
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.
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. Thanks! 🙏🏻 |
||
| request.scheme, | ||
| request.host, | ||
| request.path, | ||
| ) | ||
| url_attributes["url.path"] = request.path | ||
|
|
||
| if request.query_string: | ||
| filtered_query_string = ( | ||
| _apply_data_collection_filtering_to_query_string( | ||
| query_string=request.query_string, | ||
| behaviour=client.options["data_collection"][ | ||
| "url_query_params" | ||
| ], | ||
| ) | ||
| ) | ||
| if filtered_query_string: | ||
| url_attributes["url.query"] = filtered_query_string | ||
| elif should_send_default_pii(): | ||
| url_attributes["url.full"] = "%s://%s%s" % ( | ||
| request.scheme, | ||
| request.host, | ||
|
|
@@ -362,14 +385,33 @@ | |
| "sentry.origin": AioHttpIntegration.origin, | ||
| "http.request.method": method, | ||
| } | ||
| if parsed_url is not None and should_send_default_pii(): | ||
| attributes["url.full"] = parsed_url.url | ||
| attributes["url.path"] = params.url.path | ||
|
|
||
| if parsed_url.query: | ||
| attributes["url.query"] = parsed_url.query | ||
| if parsed_url.fragment: | ||
| attributes["url.fragment"] = parsed_url.fragment | ||
| if parsed_url is not None: | ||
| if has_data_collection_enabled(client.options): | ||
| attributes["url.full"] = parsed_url.url | ||
| attributes["url.path"] = params.url.path | ||
|
|
||
| if parsed_url.fragment: | ||
| attributes["url.fragment"] = parsed_url.fragment | ||
|
|
||
| if parsed_url.query: | ||
| filtered_query = ( | ||
| _apply_data_collection_filtering_to_query_string( | ||
| query_string=parsed_url.query, | ||
| behaviour=client.options["data_collection"][ | ||
| "url_query_params" | ||
| ], | ||
| ) | ||
| ) | ||
| if filtered_query: | ||
| attributes["url.query"] = filtered_query | ||
| elif should_send_default_pii(): | ||
| attributes["url.full"] = parsed_url.url | ||
| attributes["url.path"] = params.url.path | ||
|
|
||
| if parsed_url.query: | ||
| attributes["url.query"] = parsed_url.query | ||
| if parsed_url.fragment: | ||
| attributes["url.fragment"] = parsed_url.fragment | ||
|
|
||
| span = sentry_sdk.traces.start_span( | ||
| name=span_name, attributes=attributes | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Legacy client span leaks unfiltered query strings when data_collection is configured
The legacy (non-streaming) client span path stores the raw query string without applying
_apply_data_collection_filtering_to_query_string, so sensitive parameters bypass the user-configureddata_collectionfilters.Evidence
_apply_data_collection_filtering_to_query_stringis imported at line 7 for use in the streaming span path.on_request_start, ~line 400),parsed_url.queryis filtered before being stored inattributes["url.query"].elsebranch at line 428,legacy_span.set_data(SPANDATA.HTTP_QUERY, parsed_url.query)stores the raw, unfiltered query string regardless ofdata_collectionsettings.test_aiohttp.py(test_client_url_query_data_collection_span_streaming) only exercise the streaming path withtrace_lifecycle="stream", leaving the legacy path uncovered.Identified by Warden find-bugs · EJW-8M7