Add PiiEventProcessor to filter out personal identifiable information #527
Conversation
… from the events.
maciejwalkowiak
requested review from
bruno-garcia and
marandaneto
as code owners
maciejwalkowiak
changed the title
| @Override | ||
| public SentryEvent process(SentryEvent event, @Nullable Object hint) { | ||
| if (!options.isSendDefaultPii()) { | ||
| final User user = event.getUser(); |
There was a problem hiding this comment.
do we have any PII set (like IP) in event.request that we should strip off as well? maybe in event.request.headers if it has a fixed key.
There was a problem hiding this comment.
Good point. I also realised that we need to make sure that PiiEventProcessor is the last processor that runs, otherwise other processors added later can actually add PII.
I will add also removing:
X-FORWARDED-FORheaderAuthorizationheader- cookies
There was a problem hiding this comment.
good point, I noticed that too, that should be the last one, but I dont see how we can do that easily, any other processor added by the client manually will be added later, we can guarantee that its the last one in the SDK, but not the last one overall, I guess.
ideas are welcome of course.
There was a problem hiding this comment.
I extracted PiiEventProcessor to separate field. Another option would be to have special type of list with method addBefore(processor, Class<T>) but I think in this case it would be overengineering.
There was a problem hiding this comment.
yeah liked the idea, I'm fine with it, maybe @bruno-garcia has opinions.
There was a problem hiding this comment.
I'd prefer to avoid adding any pii to begin with.
sentry-core/src/main/java/io/sentry/core/PiiEventProcessor.java
Outdated
Show resolved
Hide resolved
sentry-core/src/main/java/io/sentry/core/PiiEventProcessor.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
looks good, left a few comments.
I'd uncheck No breaking changes on the PR template because users would need to swap the flag on App. init to send user data now and previously, this was sent by default, maybe the target branch should be 3.0.0 though.
A note for me would be to add an Android-friendly way of swapping this flag eg ManifestMetadataReader so people can configure that on the Manifest file like any other option.
Lets take Spring integration as an example - do you mean to not have a flag on sentry options and the Spring Security filter that adds user data? This filter is not configured anyway, so the idea with a flag and processor was just a convenient way to make sure that no PII data is sent to sentry no matter what is configured on the application side. |
| @NotNull | ||
| public EventProcessor getPiiEventProcessor() { | ||
| return piiEventProcessor; | ||
| } | ||
|
|
There was a problem hiding this comment.
I believe it's safer to keep this API out and simply have the options reference where needed to inspect for sendDefaultPii before adding any PII in the first place.
This will help with auditing the code for PII, avoid issues where we somehow don't run the processor accidentally (change of order or some other bug) and also forget to add a clean up logic for something we collect as PII without the guard
|
Superseded by #531. |
3 participants
📢 Type of change
📜 Description
Adds an option for users to decide if they want to send personal identifiable information (email, username, ip address) along with the events.
💡 Motivation and Context
Discussion initially started when working on Spring Boot integration - we want to give users option to decide if detailed user information is sent or not - similar to how it's done in Python integration: https://docs.sentry.io/error-reporting/configuration/?platform=python
📝 Checklist
Breaking change: sending PII is turn off by default.