Merge pull request #6240 from getsentry/fix/callback-error-handling-spring-user-provider

fix(spring): Handle user provider failures

Author: adinauer

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

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);
   }
 

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<SentryUserProvider>()
+    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<OutOfMemoryError> {
+          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)

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);
   }
 

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<SentryUserProvider>()
+    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<OutOfMemoryError> {
+          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)

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);
   }
 

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<SentryUserProvider>()
+    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<OutOfMemoryError> {
+          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)