feat(scrubbing): Scrub breadcrumb.data.http.query & data.other with default scrubbers
#1901
Conversation
| @@ -1123,7 +1123,7 @@ pub struct DataElement { | |||
| #[metastructure(pii = "maybe")] | |||
| pub http: Annotated<HttpElement>, | |||
|
|
|||
| #[metastructure(additional_properties, retain = "true", pii = "maybe")] | |||
| #[metastructure(additional_properties, retain = "true", pii = "true")] | |||
There was a problem hiding this comment.
This change doesn't seem to affect the changes made to Span, and is motivated by this test (which fails otherwise):
relay/relay-general/src/pii/convert.rs
Line 438 in 00b83a3
There was a problem hiding this comment.
The data property in Breadcrumb is also marked as pii == true.
There was a problem hiding this comment.
If breadcrumb.data was already pii = true, why do we even need to change its type? Does pii = true not descend into the Value?
There was a problem hiding this comment.
I haven't dove into the code, but apparently, it does not descend -- the test linked above would have passed on the first attempt. Maybe it acts the other way around, where pii = false means no pii checks on child elements? I'd have to check the code; but I don't think that's an issue.
There was a problem hiding this comment.
I modified the "span" test, I think we now scrub too much in span.data:
relay/relay-general/src/pii/processor.rs
Line 1037 in 1125801
An easy way around this would be to have separate SpanData and BreadcrumbData structs, instead of a common DataElement.
There was a problem hiding this comment.
With this change
breadcrumb.data.http.queryis scrubbed by defaultbreadcrumb.data.whateveris scrubbed by defaultbreadcrumb.data.http.whateveris not scrubbed by default
Is this really the behavior we want? Because if we also want the 3rd case to be scrubbed by default (ie. scrub everything in data), we might as well change the type of breadcrumb.data back to Value. test_breadcrumb_data_object_is_scrubbed and test_breadcrumb_data_string_is_scrubbed seem to pass just fine with that simplified schema.
iker-barriocanal
changed the title
breadcrumb.data.http.query with default scrubbersbreadcrumb.data.http.query & data.other with default scrubbers
| @@ -1123,7 +1123,7 @@ pub struct DataElement { | |||
| #[metastructure(pii = "maybe")] | |||
| pub http: Annotated<HttpElement>, | |||
|
|
|||
| #[metastructure(additional_properties, retain = "true", pii = "maybe")] | |||
| #[metastructure(additional_properties, retain = "true", pii = "true")] | |||
There was a problem hiding this comment.
With this change
breadcrumb.data.http.queryis scrubbed by defaultbreadcrumb.data.whateveris scrubbed by defaultbreadcrumb.data.http.whateveris not scrubbed by default
Is this really the behavior we want? Because if we also want the 3rd case to be scrubbed by default (ie. scrub everything in data), we might as well change the type of breadcrumb.data back to Value. test_breadcrumb_data_object_is_scrubbed and test_breadcrumb_data_string_is_scrubbed seem to pass just fine with that simplified schema.
|
Closing, see #1915 (comment). |
Ref: #1855
breadcrumb.data.http.querymay contain sensitive data, which is currently not scrubbed. This PR makes relay to scrub that field with default data scrubbers.data.otheris also scrubbed to be consistent with the current IP address scrubbing in breadcrumb's data. This should not cause any problems with #1593, where the biggest issue was scrubbing span descriptions (which they are still not scrubbed).