diff --git a/CHANGELOG.md b/CHANGELOG.md index 95f947f594d..1589a2f6f02 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### Fixes +- Keep Spring requests running when a `SentryUserProvider` throws and discard the incomplete user identity ([#6240](https://github.com/getsentry/sentry-java/pull/6240)) - Drop Apollo 5 spans when `beforeSpan` throws without disrupting the GraphQL request ([#6238](https://github.com/getsentry/sentry-java/pull/6238)) ### Features diff --git a/sentry-spring-7/src/main/java/io/sentry/spring7/SentryUserFilter.java b/sentry-spring-7/src/main/java/io/sentry/spring7/SentryUserFilter.java index da9d2f0f77e..5b4afe069df 100644 --- a/sentry-spring-7/src/main/java/io/sentry/spring7/SentryUserFilter.java +++ b/sentry-spring-7/src/main/java/io/sentry/spring7/SentryUserFilter.java @@ -4,7 +4,9 @@ import io.sentry.IScope; import io.sentry.IScopes; import io.sentry.IpAddressUtils; +import io.sentry.SentryLevel; import io.sentry.protocol.User; +import io.sentry.util.ExceptionUtils; import io.sentry.util.Objects; import jakarta.servlet.FilterChain; import jakarta.servlet.ServletException; @@ -43,16 +45,30 @@ protected void doFilterInternal( final @NotNull FilterChain chain) throws ServletException, IOException { final User user = new User(); + boolean providerFailed = false; for (final SentryUserProvider provider : sentryUserProviders) { - apply(user, provider.provideUser()); + try { + apply(user, provider.provideUser()); + } catch (Throwable e) { + ExceptionUtils.rethrowIfFatal(e); + providerFailed = true; + scopes + .getOptions() + .getLogger() + .log( + SentryLevel.ERROR, + e, + "The SentryUserProvider callback %s threw an exception.", + provider.getClass().getName()); + } } - if (scopes.getOptions().getDataCollectionResolver().isUserInfo()) { + if (!providerFailed && scopes.getOptions().getDataCollectionResolver().isUserInfo()) { if (IpAddressUtils.isDefault(user.getIpAddress())) { // unset {{auto}} as it would set the server's ip address as a user ip address user.setIpAddress(null); } } - scopes.setUser(user); + scopes.setUser(providerFailed ? new User() : user); chain.doFilter(request, response); } diff --git a/sentry-spring-7/src/test/kotlin/io/sentry/spring7/SentryUserFilterTest.kt b/sentry-spring-7/src/test/kotlin/io/sentry/spring7/SentryUserFilterTest.kt index 92327456e13..8d6a2fbcf69 100644 --- a/sentry-spring-7/src/test/kotlin/io/sentry/spring7/SentryUserFilterTest.kt +++ b/sentry-spring-7/src/test/kotlin/io/sentry/spring7/SentryUserFilterTest.kt @@ -6,9 +6,12 @@ import io.sentry.protocol.User import jakarta.servlet.FilterChain import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertFailsWith import kotlin.test.assertNull +import org.assertj.core.api.Assertions.assertThat import org.mockito.kotlin.check import org.mockito.kotlin.mock +import org.mockito.kotlin.never import org.mockito.kotlin.verify import org.mockito.kotlin.whenever import org.springframework.mock.web.MockHttpServletRequest @@ -151,6 +154,42 @@ class SentryUserFilterTest { verify(fixture.scopes).setUser(check { assertNull(it.ipAddress) }) } + @Test + fun `provider failure discards user data while later providers and request continue`() { + val failure = RuntimeException("provider failed") + val laterProvider = mock() + whenever(laterProvider.provideUser()).thenReturn(sampleUser) + val filter = + fixture.getSut( + userProviders = + listOf( + SentryUserProvider { sampleUser }, + SentryUserProvider { throw failure }, + laterProvider, + ) + ) + + filter.doFilter(fixture.request, fixture.response, fixture.chain) + + verify(fixture.scopes).setUser(check { assertEquals(User(), it) }) + verify(laterProvider).provideUser() + verify(fixture.chain).doFilter(fixture.request, fixture.response) + } + + @Test + fun `fatal provider failure propagates`() { + val failure = OutOfMemoryError("fatal") + val filter = fixture.getSut(userProviders = listOf(SentryUserProvider { throw failure })) + + assertThat( + assertFailsWith { + filter.doFilter(fixture.request, fixture.response, fixture.chain) + } + ) + .isSameAs(failure) + verify(fixture.chain, never()).doFilter(fixture.request, fixture.response) + } + private fun assertEquals(user1: User, user2: User) { assertEquals(user1.username, user2.username) assertEquals(user1.id, user2.id) diff --git a/sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/SentryUserFilter.java b/sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/SentryUserFilter.java index 23a77f79f0d..796dee9da7e 100644 --- a/sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/SentryUserFilter.java +++ b/sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/SentryUserFilter.java @@ -4,7 +4,9 @@ import io.sentry.IScope; import io.sentry.IScopes; import io.sentry.IpAddressUtils; +import io.sentry.SentryLevel; import io.sentry.protocol.User; +import io.sentry.util.ExceptionUtils; import io.sentry.util.Objects; import jakarta.servlet.FilterChain; import jakarta.servlet.ServletException; @@ -43,16 +45,30 @@ protected void doFilterInternal( final @NotNull FilterChain chain) throws ServletException, IOException { final User user = new User(); + boolean providerFailed = false; for (final SentryUserProvider provider : sentryUserProviders) { - apply(user, provider.provideUser()); + try { + apply(user, provider.provideUser()); + } catch (Throwable e) { + ExceptionUtils.rethrowIfFatal(e); + providerFailed = true; + scopes + .getOptions() + .getLogger() + .log( + SentryLevel.ERROR, + e, + "The SentryUserProvider callback %s threw an exception.", + provider.getClass().getName()); + } } - if (scopes.getOptions().getDataCollectionResolver().isUserInfo()) { + if (!providerFailed && scopes.getOptions().getDataCollectionResolver().isUserInfo()) { if (IpAddressUtils.isDefault(user.getIpAddress())) { // unset {{auto}} as it would set the server's ip address as a user ip address user.setIpAddress(null); } } - scopes.setUser(user); + scopes.setUser(providerFailed ? new User() : user); chain.doFilter(request, response); } diff --git a/sentry-spring-jakarta/src/test/kotlin/io/sentry/spring/jakarta/SentryUserFilterTest.kt b/sentry-spring-jakarta/src/test/kotlin/io/sentry/spring/jakarta/SentryUserFilterTest.kt index 15a7bf377cd..1cb91b1445f 100644 --- a/sentry-spring-jakarta/src/test/kotlin/io/sentry/spring/jakarta/SentryUserFilterTest.kt +++ b/sentry-spring-jakarta/src/test/kotlin/io/sentry/spring/jakarta/SentryUserFilterTest.kt @@ -6,9 +6,12 @@ import io.sentry.protocol.User import jakarta.servlet.FilterChain import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertFailsWith import kotlin.test.assertNull +import org.assertj.core.api.Assertions.assertThat import org.mockito.kotlin.check import org.mockito.kotlin.mock +import org.mockito.kotlin.never import org.mockito.kotlin.verify import org.mockito.kotlin.whenever import org.springframework.mock.web.MockHttpServletRequest @@ -151,6 +154,42 @@ class SentryUserFilterTest { verify(fixture.scopes).setUser(check { assertNull(it.ipAddress) }) } + @Test + fun `provider failure discards user data while later providers and request continue`() { + val failure = RuntimeException("provider failed") + val laterProvider = mock() + whenever(laterProvider.provideUser()).thenReturn(sampleUser) + val filter = + fixture.getSut( + userProviders = + listOf( + SentryUserProvider { sampleUser }, + SentryUserProvider { throw failure }, + laterProvider, + ) + ) + + filter.doFilter(fixture.request, fixture.response, fixture.chain) + + verify(fixture.scopes).setUser(check { assertEquals(User(), it) }) + verify(laterProvider).provideUser() + verify(fixture.chain).doFilter(fixture.request, fixture.response) + } + + @Test + fun `fatal provider failure propagates`() { + val failure = OutOfMemoryError("fatal") + val filter = fixture.getSut(userProviders = listOf(SentryUserProvider { throw failure })) + + assertThat( + assertFailsWith { + filter.doFilter(fixture.request, fixture.response, fixture.chain) + } + ) + .isSameAs(failure) + verify(fixture.chain, never()).doFilter(fixture.request, fixture.response) + } + private fun assertEquals(user1: User, user2: User) { assertEquals(user1.username, user2.username) assertEquals(user1.id, user2.id) diff --git a/sentry-spring/src/main/java/io/sentry/spring/SentryUserFilter.java b/sentry-spring/src/main/java/io/sentry/spring/SentryUserFilter.java index 18e1c0d2875..00b2ed9b62f 100644 --- a/sentry-spring/src/main/java/io/sentry/spring/SentryUserFilter.java +++ b/sentry-spring/src/main/java/io/sentry/spring/SentryUserFilter.java @@ -4,7 +4,9 @@ import io.sentry.IScope; import io.sentry.IScopes; import io.sentry.IpAddressUtils; +import io.sentry.SentryLevel; import io.sentry.protocol.User; +import io.sentry.util.ExceptionUtils; import io.sentry.util.Objects; import java.io.IOException; import java.util.List; @@ -43,16 +45,30 @@ protected void doFilterInternal( final @NotNull FilterChain chain) throws ServletException, IOException { final User user = new User(); + boolean providerFailed = false; for (final SentryUserProvider provider : sentryUserProviders) { - apply(user, provider.provideUser()); + try { + apply(user, provider.provideUser()); + } catch (Throwable e) { + ExceptionUtils.rethrowIfFatal(e); + providerFailed = true; + scopes + .getOptions() + .getLogger() + .log( + SentryLevel.ERROR, + e, + "The SentryUserProvider callback %s threw an exception.", + provider.getClass().getName()); + } } - if (scopes.getOptions().getDataCollectionResolver().isUserInfo()) { + if (!providerFailed && scopes.getOptions().getDataCollectionResolver().isUserInfo()) { if (IpAddressUtils.isDefault(user.getIpAddress())) { // unset {{auto}} as it would set the server's ip address as a user ip address user.setIpAddress(null); } } - scopes.setUser(user); + scopes.setUser(providerFailed ? new User() : user); chain.doFilter(request, response); } diff --git a/sentry-spring/src/test/kotlin/io/sentry/spring/SentryUserFilterTest.kt b/sentry-spring/src/test/kotlin/io/sentry/spring/SentryUserFilterTest.kt index 07283bd5b95..8c0ed0f2f92 100644 --- a/sentry-spring/src/test/kotlin/io/sentry/spring/SentryUserFilterTest.kt +++ b/sentry-spring/src/test/kotlin/io/sentry/spring/SentryUserFilterTest.kt @@ -6,9 +6,12 @@ import io.sentry.protocol.User import javax.servlet.FilterChain import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertFailsWith import kotlin.test.assertNull +import org.assertj.core.api.Assertions.assertThat import org.mockito.kotlin.check import org.mockito.kotlin.mock +import org.mockito.kotlin.never import org.mockito.kotlin.verify import org.mockito.kotlin.whenever import org.springframework.mock.web.MockHttpServletRequest @@ -151,6 +154,42 @@ class SentryUserFilterTest { verify(fixture.scopes).setUser(check { assertNull(it.ipAddress) }) } + @Test + fun `provider failure discards user data while later providers and request continue`() { + val failure = RuntimeException("provider failed") + val laterProvider = mock() + whenever(laterProvider.provideUser()).thenReturn(sampleUser) + val filter = + fixture.getSut( + userProviders = + listOf( + SentryUserProvider { sampleUser }, + SentryUserProvider { throw failure }, + laterProvider, + ) + ) + + filter.doFilter(fixture.request, fixture.response, fixture.chain) + + verify(fixture.scopes).setUser(check { assertEquals(User(), it) }) + verify(laterProvider).provideUser() + verify(fixture.chain).doFilter(fixture.request, fixture.response) + } + + @Test + fun `fatal provider failure propagates`() { + val failure = OutOfMemoryError("fatal") + val filter = fixture.getSut(userProviders = listOf(SentryUserProvider { throw failure })) + + assertThat( + assertFailsWith { + filter.doFilter(fixture.request, fixture.response, fixture.chain) + } + ) + .isSameAs(failure) + verify(fixture.chain, never()).doFilter(fixture.request, fixture.response) + } + private fun assertEquals(user1: User, user2: User) { assertEquals(user1.username, user2.username) assertEquals(user1.id, user2.id)