-
Notifications
You must be signed in to change notification settings - Fork 642
feat(integrations): apply data_collection cookie filtering to wsgi, starlette, litestar, starlite #6797
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
feat(integrations): apply data_collection cookie filtering to wsgi, starlette, litestar, starlite #6797
Changes from all commits
a19be39
620e222
ec6f520
293a291
530e83c
326d796
48f7c7b
621d79b
fad2292
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 |
|---|---|---|
|
|
@@ -12,6 +12,9 @@ | |
| from django.core.urlresolvers import reverse | ||
|
|
||
|
|
||
| NO_COOKIES = object() | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def client(): | ||
| return Client(application) | ||
|
|
@@ -99,3 +102,118 @@ def test_scrub_django_custom_session_cookies_filtered( | |
| "csrf_secret": "[Filtered]", | ||
| "foo": "bar", | ||
| } | ||
|
|
||
|
|
||
| @pytest.mark.forked | ||
| @pytest_mark_django_db_decorator() | ||
| @pytest.mark.parametrize( | ||
| "cookies_to_set, data_collection, expected_cookies", | ||
| [ | ||
| pytest.param( | ||
| {"sessionid": "123", "csrftoken": "456", "foo": "bar"}, | ||
| {"cookies": {"mode": "off"}}, | ||
| NO_COOKIES, | ||
|
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 not just
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. I wanted to try it out to see if it read a bit better, but happy to change this to |
||
| id="off", | ||
| ), | ||
| pytest.param( | ||
| {"sessionid": "123", "csrftoken": "456", "foo": "bar"}, | ||
| {"cookies": {"mode": "denylist"}}, | ||
| { | ||
| "sessionid": "[Filtered]", | ||
| "csrftoken": "[Filtered]", | ||
| "foo": "bar", | ||
| }, | ||
| id="denylist-default", | ||
| ), | ||
| pytest.param( | ||
| {"sessionid": "123", "csrftoken": "456", "foo": "bar"}, | ||
| {"cookies": {"mode": "denylist", "terms": ["foo"]}}, | ||
| { | ||
| "sessionid": "[Filtered]", | ||
| "csrftoken": "[Filtered]", | ||
| "foo": "[Filtered]", | ||
| }, | ||
| id="denylist-extra-terms", | ||
| ), | ||
| pytest.param( | ||
| {"sessionid": "123", "csrftoken": "456", "foo": "bar", "bar": "baz"}, | ||
| {"cookies": {"mode": "allowlist", "terms": ["foo"]}}, | ||
| { | ||
| "sessionid": "[Filtered]", | ||
| "csrftoken": "[Filtered]", | ||
| "foo": "bar", | ||
| "bar": "[Filtered]", | ||
| }, | ||
| id="allowlist", | ||
| ), | ||
| pytest.param( | ||
| {"sessionid": "123", "csrftoken": "456", "foo": "bar", "bar": "baz"}, | ||
| {"cookies": {"mode": "allowlist", "terms": ["sessionid", "foo"]}}, | ||
| { | ||
| "sessionid": "[Filtered]", | ||
| "csrftoken": "[Filtered]", | ||
| "foo": "bar", | ||
| "bar": "[Filtered]", | ||
| }, | ||
| id="allowlist-cannot-override-sensitive", | ||
| ), | ||
| pytest.param( | ||
| {"sessionid": "123", "csrftoken": "456", "foo": "bar"}, | ||
| {}, | ||
| { | ||
| "sessionid": "[Filtered]", | ||
| "csrftoken": "[Filtered]", | ||
| "foo": "bar", | ||
| }, | ||
| id="cookies-omitted-defaults-to-denylist", | ||
| ), | ||
| ], | ||
| ) | ||
| def test_data_collection_cookies( | ||
| sentry_init, | ||
| client, | ||
| capture_items, | ||
| cookies_to_set, | ||
| data_collection, | ||
| expected_cookies, | ||
| ): | ||
| sentry_init( | ||
| integrations=[DjangoIntegration()], | ||
| _experiments={"data_collection": data_collection}, | ||
| ) | ||
| items = capture_items("event") | ||
| for name, value in cookies_to_set.items(): | ||
| werkzeug_set_cookie(client, "localhost", name, value) | ||
| client.get(reverse("view_exc")) | ||
|
|
||
| (event,) = (item.payload for item in items if item.type == "event") | ||
| if expected_cookies is NO_COOKIES: | ||
| assert "cookies" not in event["request"] | ||
| else: | ||
| assert event["request"]["cookies"] == expected_cookies | ||
|
|
||
|
|
||
| @pytest.mark.forked | ||
| @pytest_mark_django_db_decorator() | ||
| def test_data_collection_cookies_precedence_over_send_default_pii( | ||
| sentry_init, client, capture_items | ||
| ): | ||
| # ``data_collection`` is the single source of truth: even with | ||
| # ``send_default_pii=False``, the configured cookie behaviour still applies. | ||
| sentry_init( | ||
| integrations=[DjangoIntegration()], | ||
| send_default_pii=False, | ||
| _experiments={"data_collection": {"cookies": {"mode": "denylist"}}}, | ||
| ) | ||
| items = capture_items("event") | ||
| werkzeug_set_cookie(client, "localhost", "sessionid", "123") | ||
| werkzeug_set_cookie(client, "localhost", "csrftoken", "456") | ||
| werkzeug_set_cookie(client, "localhost", "foo", "bar") | ||
| client.get(reverse("view_exc")) | ||
|
|
||
| (event,) = (item.payload for item in items if item.type == "event") | ||
| assert event["request"]["cookies"] == { | ||
| "sessionid": "[Filtered]", | ||
| "csrftoken": "[Filtered]", | ||
| "foo": "bar", | ||
| } | ||
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.
the
should_send_default_piicheck here is no longer needed since this check happens withinStarletteRequestExtractorThere 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.
Been some time since I last looked at the extractor code -- so the Starlette extractor runs after this and overwrites the cookies?
Just wanted to double-check that we have the precedence right (and that the cookies are really missing if
should_send_default_pii=Falseafter this change)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.
It runs before this, and then, yes, overwrites the cookies.
If
should_send_default_piiis false or if the data collection configuration filters out cookies, then thecookieskey/value doesn't get set oninfowithin the Starlette extractor'sextract_request_info(and line 121 doesn't run).