Skip to content
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

fix(pii): Mark sentry_user field as PII #3948

Merged
merged 3 commits into from
Aug 27, 2024
Merged

Conversation

loewenheim
Copy link
Contributor

@loewenheim loewenheim commented Aug 26, 2024

This marks User::sentry_user as PII so it gets scrubbed correctly.

Fixes #3917.

#skip-changelog

@loewenheim loewenheim requested a review from a team as a code owner August 26, 2024 14:37
@loewenheim loewenheim self-assigned this Aug 26, 2024
Copy link
Member

@Dav1dde Dav1dde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is extracting after PII scrubbing feasible?

This works now for IP addresses but not custom rules where the Sentry user specifically targets the ip field.

@@ -81,7 +81,7 @@ pub struct User {
/// This field is computed by concatenating the name of specific fields of the `User`
/// struct with their value. For example, if `id` is set, `sentry_user` will be equal to
/// `"id:id-of-the-user".
#[metastructure(skip_serialization = "empty")]
#[metastructure(pii = "true", skip_serialization = "empty")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mini nit: I would put this field declaration right after name, i.e. group it with the fields it is constructed from.

@loewenheim
Copy link
Contributor Author

Is extracting after PII scrubbing feasible?

This works now for IP addresses but not custom rules where the Sentry user specifically targets the ip field.

I tried switching around PII stripping and normalization (where sentry_user is set) and it broke a test by changing the reported browser name. I don't think I can predict the consequences of making that change.

@jjbayer
Copy link
Member

jjbayer commented Aug 27, 2024

I tried switching around PII stripping and normalization (where sentry_user is set) and it broke a test by changing the reported browser name. I don't think I can predict the consequences of making that change.

Yes I'm afraid we cannot move all of event normalization after PII scrubbing. But we could introduce a new "post-pii normalization" step that for now only contains the creation of the sentry_user field. Tricky part is making sure the trimming processor still runs after that.

Copy link
Member

@Dav1dde Dav1dde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes the issue at hand for now, thinking about re-ordering processing is probably a not so easy/big task, think we should move forward with this. @jjbayer wdyt?

Copy link
Member

@jjbayer jjbayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes the issue at hand for now, thinking about re-ordering processing is probably a not so easy/big task, think we should move forward with.

Yep

@loewenheim loewenheim enabled auto-merge (squash) August 27, 2024 09:49
@loewenheim loewenheim merged commit dc843b0 into master Aug 27, 2024
24 checks passed
@loewenheim loewenheim deleted the fix/sentry-user-pii branch August 27, 2024 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sentry_user not PII scrubbed
3 participants