feat(integrations): apply data_collection cookie filtering to wsgi, starlette, litestar, starlite#6797
feat(integrations): apply data_collection cookie filtering to wsgi, starlette, litestar, starlite#6797ericapisani wants to merge 9 commits into
Conversation
…tarlette, litestar, starlite Extends the granular cookie collection controls (data_collection.cookies) to _wsgi_common, starlette, litestar, and starlite, matching the behavior already used elsewhere. Falls back to should_send_default_pii() when data_collection is not configured for cookies. HTTP "Cookie" and "set-cookie" headers will continue to be completely filtered with the "[Filtered]" value. Fixes PY-2581 Fixes #6741
| request_info = event.get("request", {}) | ||
| if info: | ||
| if "cookies" in info and should_send_default_pii(): | ||
| if "cookies" in info: |
There was a problem hiding this comment.
the should_send_default_pii check here is no longer needed since this check happens within StarletteRequestExtractor
There was a problem hiding this comment.
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=False after this change)
There was a problem hiding this comment.
Been some time since I last looked at the extractor code -- so the Starlette extractor runs after this and overwrites the cookies?
It runs before this, and then, yes, overwrites the cookies.
If should_send_default_pii is false or if the data collection configuration filters out cookies, then the cookies key/value doesn't get set on info within the Starlette extractor's extract_request_info (and line 121 doesn't run).
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Codecov Results 📊✅ 92565 passed | ⏭️ 6302 skipped | Total: 98867 | Pass Rate: 93.63% | Execution Time: 323m 16s 📊 Comparison with Base Branch
All tests are passing successfully. ✅ Patch coverage is 96.88%. Project has 2463 uncovered lines. Files with missing lines (3)
Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
+ Coverage 89.74% 89.76% +0.02%
==========================================
Files 193 193 —
Lines 23995 24059 +64
Branches 8350 8398 +48
==========================================
+ Hits 21533 21596 +63
- Misses 2462 2463 +1
- Partials 1381 1383 +2Generated by Codecov Action |
…de is off Previously the async request extractors attached an empty cookies dict when the data_collection cookies mode was off, while sync route handlers omitted it entirely. Make all integrations consistent by not attaching the cookies field at all when filtering yields no cookies.
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 620e222. Configure here.
951c408 to
d717172
Compare
ec6f520 to
0396813
Compare
…tarlette, litestar, starlite Extends the granular cookie collection controls (data_collection.cookies) to _wsgi_common, starlette, litestar, and starlite, matching the behavior already used elsewhere. Falls back to should_send_default_pii() when data_collection is not configured for cookies. HTTP "Cookie" and "set-cookie" headers will continue to be completely filtered with the "[Filtered]" value. Fixes PY-2581 Fixes #6741
…de is off Previously the async request extractors attached an empty cookies dict when the data_collection cookies mode was off, while sync route handlers omitted it entirely. Make all integrations consistent by not attaching the cookies field at all when filtering yields no cookies.
0396813 to
326d796
Compare
sentrivana
left a comment
sentrivana
left a comment
There was a problem hiding this comment.
Looks good! Had a small question regarding the event processor and the NO_COOKIES sentinel in the tests.
| pytest.param( | ||
| {"sessionid": "123", "csrftoken": "456", "foo": "bar"}, | ||
| {"cookies": {"mode": "off"}}, | ||
| NO_COOKIES, |
There was a problem hiding this comment.
I wanted to try it out to see if it read a bit better, but happy to change this to None to match what's happened on the previous branch if we'd rather not have an extra variable lying around in the test file:)
| request_info = event.get("request", {}) | ||
| if info: | ||
| if "cookies" in info and should_send_default_pii(): | ||
| if "cookies" in info: |
There was a problem hiding this comment.
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=False after this change)
2 participants
Extends the granular cookie collection controls (data_collection.cookies)
to _wsgi_common, starlette, litestar, and starlite, matching the behavior
already used elsewhere. Falls back to should_send_default_pii() when
data_collection is not configured for cookies.
HTTP "Cookie" and "set-cookie" headers will continue to be completely filtered
with the "[Filtered]" value.
Fixes PY-2581
Fixes #6741