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

Send user.ip_address = {{auto}} when sendDefaultPii is true #1144

Merged
merged 6 commits into from
Jan 4, 2021
Merged
Show file tree
Hide file tree
Changes from 2 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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
* Fix: Initialize Logback after context refreshes (#1129)
* Ref: Return NoOpTransaction instead of null (#1126)
* Fix: Do not crash when passing null values to @Nullable methods, eg User and Scope
* Enhancement: Send user.ip_address = {{ auto }} when sendDefaultPii is true
* Fix: Resolving dashed properties from external configuration
* Feat: Read `uncaught.handler.enabled` property from the external configuration

Expand Down
16 changes: 16 additions & 0 deletions sentry/src/main/java/io/sentry/MainEventProcessor.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package io.sentry;

import io.sentry.protocol.SentryException;
import io.sentry.protocol.User;
import io.sentry.util.ApplyScopeUtils;
import io.sentry.util.Objects;
import java.util.ArrayList;
Expand All @@ -19,6 +20,12 @@ public final class MainEventProcessor implements EventProcessor {
*/
private static final String DEFAULT_ENVIRONMENT = "production";

/**
* Default value for {@link User#getIpAddress()} set when event does not have user and ip address
* set and when {@link SentryOptions#isSendDefaultPii()} is set to true.
*/
private static final String DEFAULT_IP_ADDRESS = "{{auto}}";

private final @NotNull SentryOptions options;
private final @NotNull SentryThreadFactory sentryThreadFactory;
private final @NotNull SentryExceptionFactory sentryExceptionFactory;
Expand Down Expand Up @@ -123,5 +130,14 @@ private void processNonCachedEvent(final @NotNull SentryEvent event) {
event.setThreads(sentryThreadFactory.getCurrentThread());
}
}
if (options.isSendDefaultPii()) {
if (event.getUser() == null) {
final User user = new User();
user.setIpAddress(DEFAULT_IP_ADDRESS);
event.setUser(user);
} else if (event.getUser().getIpAddress() == null) {
event.getUser().setIpAddress(DEFAULT_IP_ADDRESS);
Copy link
Member

Choose a reason for hiding this comment

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

You probably want to undo this in the spring integration, since {{auto}} there will give the IP of the server.

Like check for default and set null instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In such case wouldn't it be better if we make it an opt in which is pre-configured in Android? Undoing it in Spring looks for me like a workaround.

Copy link
Member

Choose a reason for hiding this comment

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

@maciejwalkowiak That means desktop apps would have to opt-in too.
I think it's fair to say: It's enabled by default but you can opt-out. And we know better in some integrations to opt out already.

We need to document this behavior. Similar to how the default of another option is documented here:
getsentry/sentry-docs#2815

Copy link
Contributor

Choose a reason for hiding this comment

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

if we check for {{auto}} and set it to null if isSendDefaultPii is disabled would be a breaking change btw which is fine as 4.0 is a major bump, but yeah we'd need to document.

Also, it could be written {{ auto }} with spaces, so we'd need to trim spaces off if we want to compare that properly as well, otherwise, it'd not be a match and IPs will be set by the server.

Copy link
Contributor

@marandaneto marandaneto Jan 4, 2021

Choose a reason for hiding this comment

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

this is breaking the Android user, see:

https://github.com/getsentry/sentry-java/blob/main/sentry-android-core/src/main/java/io/sentry/android/core/DefaultAndroidEventProcessor.java#L151-L154

now the user will never be null (if isSendDefaultPii enabled), we need to fix the if condition in the DefaultAndroidEventProcessor class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Undoing in Spring integration ✅
Updating Android code ✅

Copy link
Contributor

Choose a reason for hiding this comment

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

this has been merged but there's a bug here.

{{ auto }} is valid
{{auto}} is also valid

previously {{auto}} was not valid and its been fixed on the backend, so people were setting {{ auto }} manually, those people, it won't work as the comparison is not trimming off the spaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Will be fixed in next PR.

}
}
}
}
36 changes: 35 additions & 1 deletion sentry/src/test/java/io/sentry/MainEventProcessorTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import com.nhaarman.mockitokotlin2.mock
import io.sentry.hints.ApplyScopeData
import io.sentry.hints.Cached
import io.sentry.protocol.SdkVersion
import io.sentry.protocol.User
import java.lang.RuntimeException
import kotlin.test.Test
import kotlin.test.assertEquals
Expand All @@ -25,10 +26,13 @@ class MainEventProcessorTest {
version = "1.2.3"
}
}
fun getSut(attachThreads: Boolean = true, attachStackTrace: Boolean = true, environment: String? = "environment", tags: Map<String, String> = emptyMap()): MainEventProcessor {
fun getSut(attachThreads: Boolean = true, attachStackTrace: Boolean = true, environment: String? = "environment", tags: Map<String, String> = emptyMap(), sendDefaultPii: Boolean? = null): MainEventProcessor {
sentryOptions.isAttachThreads = attachThreads
sentryOptions.isAttachStacktrace = attachStackTrace
sentryOptions.environment = environment
if (sendDefaultPii != null) {
sentryOptions.isSendDefaultPii = sendDefaultPii
}
tags.forEach { sentryOptions.setTag(it.key, it.value) }
return MainEventProcessor(sentryOptions)
}
Expand Down Expand Up @@ -191,6 +195,36 @@ class MainEventProcessorTest {
assertEquals("production", event.environment)
}

@Test
fun `when event does not have ip address set and sendDefaultPii is set to true, sets "{{auto}}" as the ip address`() {
val sut = fixture.getSut(sendDefaultPii = true)
val event = SentryEvent()
sut.process(event, null)
assertNotNull(event.user)
assertEquals("{{auto}}", event.user.ipAddress)
}

@Test
fun `when event has ip address set and sendDefaultPii is set to true, keeps original ip address`() {
val sut = fixture.getSut(sendDefaultPii = true)
val event = SentryEvent()
event.user = User()
event.user.ipAddress = "192.168.0.1"
sut.process(event, null)
assertNotNull(event.user)
assertEquals("192.168.0.1", event.user.ipAddress)
}

@Test
fun `when event does not have ip address set and sendDefaultPii is set to false, does not set ip address`() {
val sut = fixture.getSut(sendDefaultPii = false)
val event = SentryEvent()
event.user = User()
sut.process(event, null)
assertNotNull(event.user)
assertNull(event.user.ipAddress)
}

@Test
fun `when event has environment set, does not overwrite environment`() {
val sut = fixture.getSut(environment = null)
Expand Down