fix(pii): scrub PII in span description [INGEST-1709]#1590
Conversation
|
|
||
| /// Human readable description of a span (e.g. method URL). | ||
| #[metastructure(pii = "maybe")] | ||
| #[metastructure(pii = "true")] |
There was a problem hiding this comment.
data field is also maybe and some SDKs also set the URL in there.
See this https://github.com/getsentry/sentry/blob/fef9c695a1a7d3384fb3ce7ec6c264632e77061d/static/app/components/events/interfaces/spans/spanTreeModel.spec.tsx#L47
and https://github.com/getsentry/sentry/blob/fef9c695a1a7d3384fb3ce7ec6c264632e77061d/src/sentry/interfaces/spans.py#L33-L38
jjbayer
left a comment
There was a problem hiding this comment.
LGTM. I would add a bit more explanation to the PR description (that this change was motivated by an incident, etc.).
CHANGELOG.md
Outdated
| **Internal**: | ||
|
|
||
| - Emit a `service.back_pressure` metric that measures internal back pressure by service. ([#1583](https://github.com/getsentry/relay/pull/1583)) | ||
| - Scrub PII in the span description. ([#1590](https://github.com/getsentry/relay/pull/1590)) |
There was a problem hiding this comment.
| - Scrub PII in the span description. ([#1590](https://github.com/getsentry/relay/pull/1590)) | |
| - Always scrub PII in the span description. So far, the user had to configure this by hand. ([#1590](https://github.com/getsentry/relay/pull/1590)) |
There was a problem hiding this comment.
I'm not very consistent with this myself, but let's also add this to py/CHANGELOG.md. See https://github.com/getsentry/relay#instructions-for-changelogs.
|
Can we add a test for breadcrumbs as well? SDKs sets the |
|
relay/relay-general/src/protocol/request.rs Line 440 in 87181c0 Should we change that too? Same for query_string.
|
this already is tested in relay/relay-general/src/pii/convert.rs Line 1197 in 87181c0 |
we can add, but it can be a lot of false positives, if anything is in the URL , domain name which will contain the key words we look for we will strip entire URL. Is that ok? |
|
|
||
| let mut processor = PiiProcessor::new(config.compiled()); | ||
| process_value(&mut event, &mut processor, ProcessingState::root()).unwrap(); | ||
| assert_annotated_snapshot!(event); |
There was a problem hiding this comment.
nit: we're only interested in ensuring the description is [Filtered], so it may be easier to read that assertion over a snapshot?
There was a problem hiding this comment.
The test is to ensure that the things are working as expected now, and it will show the problem if we have the regression. But I do not really care who this implemented.
mm I am unsure, this will definatelly degrade the feature (filtering the whole URL instead of the only key words), Mobile already scrub out the query parameters (by not sending them), if you think this is ok, we are good already. |
@marandaneto , checked this with @jjbayer - we already normalize the URL and push query string from |
Filter description if it contains PII