Skip to content
This repository was archived by the owner on Aug 30, 2023. It is now read-only.
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions sentry-core/src/main/java/io/sentry/core/PiiEventProcessor.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
package io.sentry.core;

import io.sentry.core.protocol.Request;
import io.sentry.core.protocol.User;
import io.sentry.core.util.CollectionUtils;
import io.sentry.core.util.Objects;
import java.util.Arrays;
import java.util.List;
import java.util.Map;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;

/**
* Removes personal identifiable information from {@link SentryEvent} if {@link
* SentryOptions#isSendDefaultPii()} is set to false.
*/
final class PiiEventProcessor implements EventProcessor {
private static final List<String> SENSITIVE_HEADERS =
Arrays.asList("X-FORWARDED-FOR", "Authorization", "Cookies");

private final @NotNull SentryOptions options;

PiiEventProcessor(final @NotNull SentryOptions options) {
this.options = Objects.requireNonNull(options, "The options object is required");
}

@Override
public SentryEvent process(SentryEvent event, @Nullable Object hint) {
if (!options.isSendDefaultPii()) {
final User user = event.getUser();
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@maciejwalkowiak maciejwalkowiak Aug 28, 2020

Choose a reason for hiding this comment

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

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-FOR header
  • Authorization header
  • cookies

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah liked the idea, I'm fine with it, maybe @bruno-garcia has opinions.

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to avoid adding any pii to begin with.

if (user != null) {
user.setUsername(null);
user.setIpAddress(null);
user.setEmail(null);
event.setUser(user);
}
final Request request = event.getRequest();
if (request != null) {
final Map<String, String> headers = CollectionUtils.shallowCopy(request.getHeaders());
if (headers != null) {
for (String sensitiveHeader : SENSITIVE_HEADERS) {
headers.remove(sensitiveHeader);
}
request.setHeaders(headers);
}
if (request.getCookies() != null) {
request.setCookies(null);
}
}
}
return event;
}
}
4 changes: 4 additions & 0 deletions sentry-core/src/main/java/io/sentry/core/SentryClient.java
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,10 @@ public SentryClient(final @NotNull SentryOptions options, @Nullable Connection c
}
}

if (event != null) {
event = options.getPiiEventProcessor().process(event, hint);
}

if (event == null) {
return SentryId.EMPTY_ID;
}
Expand Down
20 changes: 20 additions & 0 deletions sentry-core/src/main/java/io/sentry/core/SentryOptions.java
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ public class SentryOptions {
*/
private final @NotNull List<EventProcessor> eventProcessors = new CopyOnWriteArrayList<>();

private final @NotNull EventProcessor piiEventProcessor;

/**
* Code that provides middlewares, bindings or hooks into certain frameworks or environments,
* along with code that inserts those bindings and activates them.
Expand Down Expand Up @@ -210,6 +212,9 @@ public class SentryOptions {
/** SdkVersion object that contains the Sentry Client Name and its version */
private @Nullable SdkVersion sdkVersion;

/** whether to send personal identifiable information along with events */
private boolean sendDefaultPii = false;

/**
* Adds an event processor
*
Expand Down Expand Up @@ -996,6 +1001,19 @@ public void setSdkVersion(final @Nullable SdkVersion sdkVersion) {
this.sdkVersion = sdkVersion;
}

public boolean isSendDefaultPii() {
return sendDefaultPii;
}

public void setSendDefaultPii(boolean sendDefaultPii) {
this.sendDefaultPii = sendDefaultPii;
}

@NotNull
public EventProcessor getPiiEventProcessor() {
return piiEventProcessor;
}

Comment on lines +1012 to +1016
Copy link
Member

Choose a reason for hiding this comment

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

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

/** The BeforeSend callback */
public interface BeforeSendCallback {

Expand Down Expand Up @@ -1036,6 +1054,8 @@ public SentryOptions() {
integrations.add(new ShutdownHookIntegration());

eventProcessors.add(new MainEventProcessor(this));
// piiEventProcessor is set outside of eventProcessors to make sure that it runs last
piiEventProcessor = new PiiEventProcessor(this);

setSentryClientName(BuildConfig.SENTRY_JAVA_SDK_NAME + "/" + BuildConfig.VERSION_NAME);
setSdkVersion(createSdkVersion());
Expand Down
98 changes: 98 additions & 0 deletions sentry-core/src/test/java/io/sentry/core/PiiEventProcessorTest.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
package io.sentry.core

import io.sentry.core.protocol.Request
import io.sentry.core.protocol.User
import kotlin.test.Test
import kotlin.test.assertNotNull
import kotlin.test.assertNull

class PiiEventProcessorTest {

private class Fixture {
val user = with(User()) {
id = "some-id"
username = "john.doe"
email = "[email protected]"
ipAddress = "66.249.73.223"
this
}

val request = with(Request()) {
headers = mutableMapOf(
"X-FORWARDED-FOR" to "66.249.73.223",
"Authorization" to "token",
"Cookies" to "some-cookies",
"Safe-Header" to "some-value"
)
cookies = "some-cookies"
this
}

fun getSut(sendPii: Boolean) =
PiiEventProcessor(with(SentryOptions()) {
isSendDefaultPii = sendPii
this
})
}

private val fixture = Fixture()

@Test
fun `when sendDefaultPii is set to false, removes user data from events`() {
val eventProcessor = fixture.getSut(sendPii = false)
val event = SentryEvent()
event.user = fixture.user

eventProcessor.process(event, null)

assertNotNull(event.user.id)
assertNull(event.user.email)
assertNull(event.user.username)
assertNull(event.user.ipAddress)
}

@Test
fun `when sendDefaultPii is set to true, does not remove user data from events`() {
val eventProcessor = fixture.getSut(sendPii = true)
val event = SentryEvent()
event.user = fixture.user

eventProcessor.process(event, null)

assertNotNull(event.user)
assertNotNull(event.user.id)
assertNotNull(event.user.email)
assertNotNull(event.user.username)
assertNotNull(event.user.ipAddress)
}

@Test
fun `when sendDefaultPii is set to false, removes user identifiable request headers data from events`() {
val eventProcessor = fixture.getSut(sendPii = false)
val event = SentryEvent()
event.request = fixture.request

eventProcessor.process(event, null)

assertNotNull(event.request.headers)
assertNull(event.request.headers["Authorization"])
assertNull(event.request.headers["X-FORWARDED-FOR"])
assertNull(event.request.headers["Cookies"])
assertNotNull(event.request.headers["Safe-Header"])
}

@Test
fun `when sendDefaultPii is set to true, does not remove user identifiable request headers data from events`() {
val eventProcessor = fixture.getSut(sendPii = true)
val event = SentryEvent()
event.request = fixture.request

eventProcessor.process(event, null)

assertNotNull(event.request.headers)
assertNotNull(event.request.headers["Authorization"])
assertNotNull(event.request.headers["X-FORWARDED-FOR"])
assertNotNull(event.request.headers["Cookies"])
assertNotNull(event.request.headers["Safe-Header"])
}
}
15 changes: 15 additions & 0 deletions sentry-core/src/test/java/io/sentry/core/SentryClientTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -440,6 +440,21 @@ class SentryClientTest {
verify(processor).process(eq(event), anyOrNull())
}

@Test
fun `pii event processor is the last event processor applied`() {
val processor = EventProcessor { event, hint ->
event.user = User()
event.user.email = "[email protected]"
event
}
fixture.sentryOptions.addEventProcessor(processor)

val event = SentryEvent()

fixture.getSut().captureEvent(event)
assertNull(event.user.email)
}

@Test
fun `when captureSession and no release is set, do nothing`() {
fixture.getSut().captureSession(createSession(""))
Expand Down