feat(data-collection): create DataCollection option in client#6702
feat(data-collection): create DataCollection option in client#6702ericapisani wants to merge 20 commits into
12 issues
Medium
`_kvcb_from_value` raises `AttributeError` instead of a descriptive error for non-dict inputs - `sentry_sdk/data_collection.py:168-169`
If a user passes a non-dict value for cookies or query_params (e.g. data_collection={"cookies": "off"}), val.get("mode", "deny_list") raises AttributeError: 'str' object has no attribute 'get' with no guidance on what went wrong. Consider adding an isinstance(val, dict) guard with a clear TypeError or ValueError.
Also found at:
sentry_sdk/consts.py:1282sentry_sdk/consts.py:1451-1454sentry_sdk/data_collection.py:254
`typing.Literal` imported at runtime, causes `ImportError` on Python 3.6 and 3.7 - `sentry_sdk/data_collection.py:26`
Move Literal into the if TYPE_CHECKING: block (or import from typing_extensions) — it's only used in a # type: comment and its runtime import raises ImportError on Python < 3.8, which the SDK explicitly supports (python_requires=">=3.6").
Low
`graphql.document` docstring contradicts implementation in `_map_from_send_default_pii` - `sentry_sdk/client.py:27`
The docstring for _map_from_send_default_pii states that graphql.document "stays True" regardless of send_default_pii, while only graphql.variables and database.query_params follow it. The implementation instead sets graphql.document to send_default_pii, so it becomes False when PII is disabled. This is either a documentation error or a behavioral bug: if the spec intends the GraphQL query structure (document, not the PII-bearing variables) to always be collected, then gating it on send_default_pii is wrong. At minimum the contradiction will mislead maintainers about intended PII behavior. Worth confirming against the data-collection spec whether graphql.document should be gated on PII.
DeprecationWarning for `send_default_pii` points to internal SDK code instead of user callsite - `sentry_sdk/consts.py:1282`
The stacklevel=2 in resolve_data_collection (data_collection.py:253) points the warning to _get_options in client.py rather than the user's sentry_sdk.init() call. The actual call depth is: user → _init → Client.__init__ → _get_options → resolve_data_collection → warnings.warn, so a larger stacklevel is needed to surface the correct file/line in user code.
Also found at:
sentry_sdk/consts.py:1438-1440sentry_sdk/data_collection.py:252
`_http_headers_from_value` uses substring `in` on str values, masking malformed input and risking AttributeError - `sentry_sdk/data_collection.py:186-196`
_http_headers_from_value is typed to receive a dict but is fed d.get("http_headers", {}) with no type check. It uses "request" in val / "response" in val, which for a string performs a substring search rather than a key lookup. The test at test_http_headers_collection_defaults documents passing the str "off" as an invalid option and relies on this accident (neither "request" nor "response" is a substring of "off", so both fields silently fall back to deny_list). If a user instead passes a string that contains "request" or "response" (e.g. "incoming request"), the condition becomes True and val["request"] returns a single character, which is passed to _kvcb_from_value(...) and raises an unhelpful AttributeError (str has no .get). Adding an isinstance(val, dict) guard would make malformed input fail cleanly (or fall back to defaults) instead of silently masking it. Impact is limited to unsupported/malformed configuration input.
Also found at:
tests/test_data_collection.py:31-36
No test for `TypeError` when `data_collection` receives a non-dict value - `tests/test_data_collection.py:12-13`
The resolve_data_collection function explicitly raises TypeError when data_collection is not a dict, but there is no test exercising this path — unlike test_kvcb_invalid_mode which covers the ValueError for invalid modes.
`graphql.document` docstring contradicts implementation in `_map_from_send_default_pii` - `sentry_sdk/_types.py:173-174`
The docstring of _map_from_send_default_pii (data_collection.py:93) states that in the send_default_pii legacy path, graphql.document stays True, while graphql.variables and database.query_params follow send_default_pii. However, the implementation at data_collection.py:110 sets "graphql": {"document": send_default_pii, "variables": send_default_pii}, so graphql.document becomes False when send_default_pii=False. The explicit-config path (_graphql_from_value) defaults document to True, treating the GraphQL document as non-PII. The two resolution paths therefore disagree, and either the docstring or the legacy-path implementation is wrong. This is a code-quality/correctness inconsistency, not a security issue.
Also found at:
sentry_sdk/client.py:26sentry_sdk/data_collection.py:110tests/test_data_collection.py:183-190
DeprecationWarning for `send_default_pii` points to internal SDK code instead of user call site - `sentry_sdk/client.py:77`
The warnings.warn(..., stacklevel=2) in resolve_data_collection (sentry_sdk/data_collection.py) reports the warning as originating from _get_options in sentry_sdk/client.py (the call site of resolve_data_collection) rather than the user's sentry_sdk.init() call, making it hard for users to identify which line of their own code to fix. Because resolve_data_collection is a nested helper, stacklevel=2 only unwinds to _get_options itself, one frame short of even the existing enable_tracing warning which reaches _Client.__init__.
EventScrubber not updated when spotlight overrides data_collection - `sentry_sdk/client.py:356-358`
In _get_options() the EventScrubber is created with send_default_pii=rv["data_collection"]["user_info"], which is False by default, so DEFAULT_PII_DENYLIST fields (ip_address, x_forwarded_for, x_real_ip, remote_addr) are added to the scrubber denylist. In DSN-less spotlight mode, Client.__init__ later re-derives data_collection with user_info=True but does not recreate event_scrubber. As a result, spotlight events continue to have PII fields (e.g. user IP address) scrubbed even though PII collection is intentionally enabled for local debugging. Impact is limited to spotlight local-dev mode and the effect is over-scrubbing (data removal, not disclosure), so this is a functional inconsistency rather than a security issue.
Also found at:
sentry_sdk/client.py:638-645sentry_sdk/consts.py:1282
`data_collection` property raises `KeyError` for clients pickled by a pre-PR SDK version - `sentry_sdk/client.py:761`
The data_collection property (sentry_sdk/client.py:761) uses a hard subscript self.options["data_collection"] instead of self.options.get("data_collection"). Within any single SDK version the key is always populated by _get_options() (line 353, via resolve_data_collection), so normal usage is safe. However, _Client.__setstate__ restores the raw pickled options dict and re-runs only _init_impl(), never _get_options(). A client pickled by a pre-PR SDK version (whose options dict has no data_collection key) will therefore lack the key after unpickling, and any access to this property will raise KeyError. Note the DSN-less/spotlight branch in _init_impl() (line 637) has the same latent issue. Using .get(...) with a sensible default, as should_send_default_pii() does, would avoid this.
...and 2 more
4 skills analyzed
| Skill | Findings | Duration | Cost |
|---|---|---|---|
| security-review | 0 | 44.2s | $0.11 |
| code-review | 5 | 10m 35s | $5.32 |
| find-bugs | 7 | 29m 23s | $9.21 |
| skill-scanner | 0 | 11m 41s | $0.04 |
⏱ 52m 23s · 12.0M in / 387.4k out · $14.68