From ddf742e1c8fb84a85ad6976c40b82a75e8f72455 Mon Sep 17 00:00:00 2001 From: Prince Mathew Date: Thu, 23 Jul 2026 14:03:54 +0530 Subject: [PATCH] fix : Added guard against the crash happening due to double invocation of continuation resume --- .../storage/BaseCredentialsManager.kt | 32 ++++++++-- .../storage/CredentialsManager.kt | 18 +++--- .../storage/SecureCredentialsManager.kt | 18 +++--- .../storage/CredentialsManagerTest.kt | 64 +++++++++++++++++++ .../storage/SecureCredentialsManagerTest.kt | 60 +++++++++++++++++ 5 files changed, 170 insertions(+), 22 deletions(-) diff --git a/auth0/src/main/java/com/auth0/android/authentication/storage/BaseCredentialsManager.kt b/auth0/src/main/java/com/auth0/android/authentication/storage/BaseCredentialsManager.kt index c6e3c5404..8e75b6a18 100644 --- a/auth0/src/main/java/com/auth0/android/authentication/storage/BaseCredentialsManager.kt +++ b/auth0/src/main/java/com/auth0/android/authentication/storage/BaseCredentialsManager.kt @@ -11,7 +11,7 @@ import com.auth0.android.result.Credentials import com.auth0.android.result.SSOCredentials import com.auth0.android.result.UserProfile import com.auth0.android.util.Clock -import java.util.* +import java.util.concurrent.atomic.AtomicBoolean /** * Base class meant to abstract common logic across Credentials Manager implementations. @@ -428,18 +428,42 @@ public abstract class BaseCredentialsManager internal constructor( internal inline fun runCatchingOnExecutor( callback: Callback, - block: () -> Unit + block: (Callback) -> Unit ) { + val singleShotCallback = SingleShotCallback(callback) try { - block() + block(singleShotCallback) } catch (t: Throwable) { if (t is VirtualMachineError || t is ThreadDeath || t is LinkageError) { throw t } Log.e("BaseCredentialsManager", "Unexpected error in executor block", t) - callback.onFailure( + singleShotCallback.onFailure( CredentialsManagerException(CredentialsManagerException.Code.UNKNOWN_ERROR, t) ) } } + + /** + * Delegates to [delegate] at most once. Any [onSuccess]/[onFailure] call after the first is + * silently dropped, upholding the fire-once contract of [Callback] even when the executor block + * both invokes the callback and later throws. + */ + internal class SingleShotCallback( + private val delegate: Callback + ) : Callback { + private val handled = AtomicBoolean(false) + + override fun onSuccess(result: T) { + if (handled.compareAndSet(false, true)) { + delegate.onSuccess(result) + } + } + + override fun onFailure(error: CredentialsManagerException) { + if (handled.compareAndSet(false, true)) { + delegate.onFailure(error) + } + } + } } diff --git a/auth0/src/main/java/com/auth0/android/authentication/storage/CredentialsManager.kt b/auth0/src/main/java/com/auth0/android/authentication/storage/CredentialsManager.kt index 889a7052e..beb82f318 100644 --- a/auth0/src/main/java/com/auth0/android/authentication/storage/CredentialsManager.kt +++ b/auth0/src/main/java/com/auth0/android/authentication/storage/CredentialsManager.kt @@ -138,7 +138,7 @@ public class CredentialsManager @VisibleForTesting(otherwise = VisibleForTesting callback: Callback ) { serialExecutor.execute { - runCatchingOnExecutor(callback) { + runCatchingOnExecutor(callback) { callback -> // IPSIE session_expiry: enforce the upstream-IdP session ceiling before exchanging the // refresh token, so the SSO exchange is never used to outlive the session. if (isSessionExpired(storage.retrieveString(KEY_ID_TOKEN))) { @@ -220,11 +220,11 @@ public class CredentialsManager @VisibleForTesting(otherwise = VisibleForTesting parameters, object : Callback { override fun onSuccess(result: SSOCredentials) { - continuation.resume(result) + if (continuation.isActive) continuation.resume(result) } override fun onFailure(error: CredentialsManagerException) { - continuation.resumeWithException(error) + if (continuation.isActive) continuation.resumeWithException(error) } }) } @@ -329,11 +329,11 @@ public class CredentialsManager @VisibleForTesting(otherwise = VisibleForTesting forceRefresh, object : Callback { override fun onSuccess(result: Credentials) { - continuation.resume(result) + if (continuation.isActive) continuation.resume(result) } override fun onFailure(error: CredentialsManagerException) { - continuation.resumeWithException(error) + if (continuation.isActive) continuation.resumeWithException(error) } }) } @@ -366,11 +366,11 @@ public class CredentialsManager @VisibleForTesting(otherwise = VisibleForTesting audience, scope, minTtl, parameters, headers, object : Callback { override fun onSuccess(result: APICredentials) { - continuation.resume(result) + if (continuation.isActive) continuation.resume(result) } override fun onFailure(error: CredentialsManagerException) { - continuation.resumeWithException(error) + if (continuation.isActive) continuation.resumeWithException(error) } } ) @@ -466,7 +466,7 @@ public class CredentialsManager @VisibleForTesting(otherwise = VisibleForTesting callback: Callback ) { serialExecutor.execute { - runCatchingOnExecutor(callback) { + runCatchingOnExecutor(callback) { callback -> val accessToken = storage.retrieveString(KEY_ACCESS_TOKEN) val refreshToken = storage.retrieveString(KEY_REFRESH_TOKEN) val idToken = storage.retrieveString(KEY_ID_TOKEN) @@ -607,7 +607,7 @@ public class CredentialsManager @VisibleForTesting(otherwise = VisibleForTesting ) { serialExecutor.execute { - runCatchingOnExecutor(callback) { + runCatchingOnExecutor(callback) { callback -> // IPSIE session_expiry: enforce the upstream-IdP session ceiling before serving cached // API credentials or exchanging the refresh token, so the session is never extended past it. if (isSessionExpired(storage.retrieveString(KEY_ID_TOKEN))) { diff --git a/auth0/src/main/java/com/auth0/android/authentication/storage/SecureCredentialsManager.kt b/auth0/src/main/java/com/auth0/android/authentication/storage/SecureCredentialsManager.kt index f3782c2a8..e3ff8b0b8 100644 --- a/auth0/src/main/java/com/auth0/android/authentication/storage/SecureCredentialsManager.kt +++ b/auth0/src/main/java/com/auth0/android/authentication/storage/SecureCredentialsManager.kt @@ -231,7 +231,7 @@ public class SecureCredentialsManager @VisibleForTesting(otherwise = VisibleForT callback: Callback ) { serialExecutor.execute { - runCatchingOnExecutor(callback) { + runCatchingOnExecutor(callback) { callback -> val existingCredentials: Credentials = try { getExistingCredentials() } catch (exception: CredentialsManagerException) { @@ -330,11 +330,11 @@ public class SecureCredentialsManager @VisibleForTesting(otherwise = VisibleForT parameters, object : Callback { override fun onSuccess(result: SSOCredentials) { - continuation.resume(result) + if (continuation.isActive) continuation.resume(result) } override fun onFailure(error: CredentialsManagerException) { - continuation.resumeWithException(error) + if (continuation.isActive) continuation.resumeWithException(error) } }) } @@ -459,11 +459,11 @@ public class SecureCredentialsManager @VisibleForTesting(otherwise = VisibleForT forceRefresh, object : Callback { override fun onSuccess(result: Credentials) { - continuation.resume(result) + if (continuation.isActive) continuation.resume(result) } override fun onFailure(error: CredentialsManagerException) { - continuation.resumeWithException(error) + if (continuation.isActive) continuation.resumeWithException(error) } }) } @@ -500,11 +500,11 @@ public class SecureCredentialsManager @VisibleForTesting(otherwise = VisibleForT headers, object : Callback { override fun onSuccess(result: APICredentials) { - continuation.resume(result) + if (continuation.isActive) continuation.resume(result) } override fun onFailure(error: CredentialsManagerException) { - continuation.resumeWithException(error) + if (continuation.isActive) continuation.resumeWithException(error) } }) } @@ -793,7 +793,7 @@ public class SecureCredentialsManager @VisibleForTesting(otherwise = VisibleForT callback: Callback ) { serialExecutor.execute { - runCatchingOnExecutor(callback) { + runCatchingOnExecutor(callback) { callback -> val encryptedEncoded = storage.retrieveString(KEY_CREDENTIALS) if (encryptedEncoded.isNullOrBlank()) { callback.onFailure(CredentialsManagerException.NO_CREDENTIALS) @@ -960,7 +960,7 @@ public class SecureCredentialsManager @VisibleForTesting(otherwise = VisibleForT callback: Callback ) { serialExecutor.execute { - runCatchingOnExecutor(callback) { + runCatchingOnExecutor(callback) { callback -> // IPSIE session_expiry: enforce the upstream-IdP session ceiling before serving // cached API credentials or exchanging the refresh token. The ceiling is read from // the value persisted at login (KEY_SESSION_EXPIRY) so it holds even though the diff --git a/auth0/src/test/java/com/auth0/android/authentication/storage/CredentialsManagerTest.kt b/auth0/src/test/java/com/auth0/android/authentication/storage/CredentialsManagerTest.kt index 57b7dee0c..5ff4ebace 100644 --- a/auth0/src/test/java/com/auth0/android/authentication/storage/CredentialsManagerTest.kt +++ b/auth0/src/test/java/com/auth0/android/authentication/storage/CredentialsManagerTest.kt @@ -42,6 +42,7 @@ import org.mockito.Mockito import org.mockito.MockitoAnnotations import org.mockito.kotlin.KArgumentCaptor import org.mockito.kotlin.any +import org.mockito.kotlin.anyOrNull import org.mockito.kotlin.argumentCaptor import org.mockito.kotlin.eq import org.mockito.kotlin.mock @@ -2644,6 +2645,69 @@ public class CredentialsManagerTest { MatcherAssert.assertThat(exception.cause, Is.`is`(error)) } + // a Throwable escaping the executor block AFTER onSuccess already fired must not + // trigger a second terminal callback. runCatchingOnExecutor wraps the callback in a single-shot + // guard, so onFailure is dropped. Without it, the coroutine bridge would resume twice + // ("Already resumed") and crash the process. + @Test + public fun shouldNotInvokeCallbackTwiceWhenConsumerThrowsAfterSuccessOnGetCredentials() { + Mockito.`when`(storage.retrieveString("com.auth0.id_token")).thenReturn("idToken") + Mockito.`when`(storage.retrieveString("com.auth0.access_token")).thenReturn("accessToken") + Mockito.`when`(storage.retrieveString("com.auth0.refresh_token")).thenReturn("refreshToken") + Mockito.`when`(storage.retrieveString("com.auth0.token_type")).thenReturn("type") + val expirationTime = CredentialsMock.ONE_HOUR_AHEAD_MS + Mockito.`when`(storage.retrieveLong("com.auth0.expires_at")).thenReturn(expirationTime) + Mockito.`when`(storage.retrieveLong("com.auth0.cache_expires_at")).thenReturn(expirationTime) + Mockito.`when`(storage.retrieveString("com.auth0.scope")).thenReturn("scope") + + var successCount = 0 + var failureCount = 0 + manager.getCredentials(object : Callback { + override fun onSuccess(result: Credentials) { + successCount++ + // Simulate a consumer/bridge that throws right after receiving the result. + throw IllegalStateException("Simulated post-resume failure in executor block") + } + + override fun onFailure(error: CredentialsManagerException) { + failureCount++ + } + }) + + MatcherAssert.assertThat(successCount, Is.`is`(1)) + MatcherAssert.assertThat(failureCount, Is.`is`(0)) + } + + // Layer B (coroutine bridge) regression: if the underlying callback getCredentials fires a + // terminal callback twice (onSuccess then onFailure), the suspendCancellableCoroutine bridge + // must resume the continuation only once. Without the `if (continuation.isActive)` guard the + // second resume throws IllegalStateException("Already resumed") on the executor thread and + // crashes the process. Here the first resume wins, so awaitCredentials returns normally. + @Test + @ExperimentalCoroutinesApi + public fun shouldNotResumeContinuationTwiceWhenCallbackFiresTwiceOnAwaitCredentials(): Unit = + runTest { + val credentials = CredentialsMock.create( + "idToken", "accessToken", "type", "refreshToken", + Date(CredentialsMock.ONE_HOUR_AHEAD_MS), "scope" + ) + Mockito.doAnswer { invocation -> + @Suppress("UNCHECKED_CAST") + val cb = + invocation.arguments.last() as Callback + // Simulate a double terminal callback reaching the bridge. + cb.onSuccess(credentials) + cb.onFailure(CredentialsManagerException(CredentialsManagerException.Code.UNKNOWN_ERROR)) + null + }.`when`(manager).getCredentials( + anyOrNull(), any(), any(), any(), any(), any() + ) + + val result = manager.awaitCredentials() + MatcherAssert.assertThat(result, Is.`is`(Matchers.notNullValue())) + MatcherAssert.assertThat(result.accessToken, Is.`is`("accessToken")) + } + @After public fun tearDown() { DPoPUtil.keyStore = DPoPKeyStore() diff --git a/auth0/src/test/java/com/auth0/android/authentication/storage/SecureCredentialsManagerTest.kt b/auth0/src/test/java/com/auth0/android/authentication/storage/SecureCredentialsManagerTest.kt index 1897f6cc5..f67f87297 100644 --- a/auth0/src/test/java/com/auth0/android/authentication/storage/SecureCredentialsManagerTest.kt +++ b/auth0/src/test/java/com/auth0/android/authentication/storage/SecureCredentialsManagerTest.kt @@ -4404,6 +4404,66 @@ public class SecureCredentialsManagerTest { MatcherAssert.assertThat(exception.cause, Is.`is`(error)) } + // a Throwable escaping the executor block AFTER onSuccess already fired must not + // trigger a second terminal callback. runCatchingOnExecutor wraps the callback in a single-shot + // guard, so onFailure is dropped. Without it, the awaitCredentials coroutine bridge would resume + // twice ("Already resumed") on the serial executor thread and crash the process. + @Test + public fun shouldNotInvokeCallbackTwiceWhenConsumerThrowsAfterSuccessOnGetCredentials() { + Mockito.`when`(localAuthenticationManager.authenticate()).then { + localAuthenticationManager.resultCallback.onSuccess(true) + } + val expiresAt = Date(CredentialsMock.CURRENT_TIME_MS + ONE_HOUR_SECONDS * 1000) + insertTestCredentials(true, true, true, expiresAt, "scope") + + var successCount = 0 + var failureCount = 0 + manager.getCredentials(object : Callback { + override fun onSuccess(result: Credentials) { + successCount++ + // Simulate a consumer/bridge that throws right after receiving the result. + throw IllegalStateException("Simulated post-resume failure in executor block") + } + + override fun onFailure(error: CredentialsManagerException) { + failureCount++ + } + }) + + MatcherAssert.assertThat(successCount, Is.`is`(1)) + MatcherAssert.assertThat(failureCount, Is.`is`(0)) + } + + // Layer B (coroutine bridge) regression: if the underlying callback getCredentials fires a + // terminal callback twice (onSuccess then onFailure), the suspendCancellableCoroutine bridge + // must resume the continuation only once. Without the `if (continuation.isActive)` guard the + // second resume throws IllegalStateException("Already resumed") on the serial executor thread + // and crashes the process. Here the first resume wins, so awaitCredentials returns normally. + @Test + @ExperimentalCoroutinesApi + public fun shouldNotResumeContinuationTwiceWhenCallbackFiresTwiceOnAwaitCredentials(): Unit = + runTest { + val credentials = CredentialsMock.create( + "idToken", "accessToken", "type", "refreshToken", + Date(CredentialsMock.ONE_HOUR_AHEAD_MS), "scope" + ) + Mockito.doAnswer { invocation -> + @Suppress("UNCHECKED_CAST") + val cb = + invocation.arguments.last() as Callback + // Simulate a double terminal callback reaching the bridge. + cb.onSuccess(credentials) + cb.onFailure(CredentialsManagerException(CredentialsManagerException.Code.UNKNOWN_ERROR)) + null + }.`when`(manager).getCredentials( + anyOrNull(), any(), any(), any(), any(), any() + ) + + val result = manager.awaitCredentials() + MatcherAssert.assertThat(result, Is.`is`(Matchers.notNullValue())) + MatcherAssert.assertThat(result.accessToken, Is.`is`("accessToken")) + } + // Verifies the outer runCatchingOnExecutor safety net in getSsoCredentials. // The exception is thrown from getExistingCredentials() → storage.retrieveString() // before any inner try/catch is reached.