[PM-45908] t:bug Fix sync account Off-by-one error in authenticator bridge (#7547)

2026-11-09 10:34:18 -06:00 · 2026-11-02 18:07:21 +00:00
parent 85258b792b
commit 4ecfcd31cf
2 changed files with 143 additions and 5 deletions
@@ -4,6 +4,7 @@ import android.content.Context
 import android.os.Build
 import android.os.IInterface
 import android.os.RemoteCallbackList
+import android.os.RemoteException
 import androidx.core.net.toUri
 import com.bitwarden.authenticatorbridge.IAuthenticatorBridgeService
 import com.bitwarden.authenticatorbridge.IAuthenticatorBridgeServiceCallback
@@ -144,17 +145,25 @@ class AuthenticatorBridgeProcessorImpl(
 }

 /**
- * This function mirrors the hidden "RemoteCallbackList.broadcast" function.
+ * Invokes [action] on each registered callback, similar to the hidden
+ * "RemoteCallbackList.broadcast" function. A failure in one callback is logged and does not
+ * prevent the remaining callbacks from being invoked.
 */
@Suppress("TooGenericExceptionCaught")
 private fun <T : IInterface> RemoteCallbackList<T>.forEach(action: (T) -> Unit) {
    val count = this.beginBroadcast()
    try {
-        for (index in 0..count) {
-            action(this.getBroadcastItem(index))
+        for (index in 0 until count) {
+            try {
+                action(this.getBroadcastItem(index))
+            } catch (e: RemoteException) {
+                // Callback process died; RemoteCallbackList unregisters it via binderDied.
+                Timber.w(e, "Bridge callback failed")
+            } catch (e: RuntimeException) {
+                // Catch broadly so one misbehaving callback cannot block the rest.
+                Timber.e(e, "Bridge callback threw unexpectedly")
+            }
        }
-    } catch (e: Exception) {
-        // Broadcast failed
    } finally {
        this.finishBroadcast()
    }
@@ -5,6 +5,7 @@ import android.content.Intent
 import android.net.Uri
 import android.os.Build
 import android.os.RemoteCallbackList
+import android.os.RemoteException
 import com.bitwarden.authenticatorbridge.IAuthenticatorBridgeService
 import com.bitwarden.authenticatorbridge.IAuthenticatorBridgeServiceCallback
 import com.bitwarden.authenticatorbridge.model.AddTotpLoginItemData
@@ -25,6 +26,7 @@ import com.bitwarden.ui.platform.util.getTotpDataOrNull
 import com.x8bit.bitwarden.data.auth.manager.AddTotpItemFromAuthenticatorManagerImpl
 import com.x8bit.bitwarden.data.platform.repository.AuthenticatorBridgeRepository
 import com.x8bit.bitwarden.data.platform.util.createAddTotpItemFromAuthenticatorIntent
+import io.mockk.clearConstructorMockk
 import io.mockk.coEvery
 import io.mockk.coVerify
 import io.mockk.every
@@ -259,6 +261,133 @@ class AuthenticatorBridgeProcessorTest {
            lastAccountsSync = null
        }

+        @Test
+        fun `syncAccounts should only fetch callbacks within the broadcast count`() {
+            clearConstructorMockk(RemoteCallbackList::class, answers = false)
+            val sharedAccountData = mockk<SharedAccountData>()
+            val expected = mockk<EncryptedSharedAccountData>()
+
+            every {
+                authenticatorBridgeRepository.authenticatorSyncSymmetricKey
+            } returns SYMMETRIC_KEY.symmetricEncryptionKey.byteArray
+
+            coEvery {
+                authenticatorBridgeRepository.getSharedAccounts()
+            } returns sharedAccountData
+
+            every {
+                sharedAccountData.encrypt(SYMMETRIC_KEY)
+            } returns expected.asSuccess()
+
+            getDefaultBinder().syncAccounts()
+
+            assertEquals(expected, lastAccountsSync)
+            verify(exactly = 1) {
+                anyConstructed<RemoteCallbackList<IAuthenticatorBridgeServiceCallback>>()
+                    .getBroadcastItem(0)
+            }
+
+            verify(exactly = 0) {
+                anyConstructed<RemoteCallbackList<IAuthenticatorBridgeServiceCallback>>()
+                    .getBroadcastItem(1)
+            }
+
+            verify(exactly = 1) {
+                anyConstructed<RemoteCallbackList<IAuthenticatorBridgeServiceCallback>>()
+                    .finishBroadcast()
+            }
+        }
+
+        @Test
+        fun `syncAccounts should still notify later callbacks when an earlier one fails`() {
+            val deadCallback = object : IAuthenticatorBridgeServiceCallback.Stub() {
+                override fun onAccountsSync(data: EncryptedSharedAccountData?) {
+                    throw RemoteException("process died")
+                }
+            }
+
+            every {
+                anyConstructed<RemoteCallbackList<IAuthenticatorBridgeServiceCallback>>()
+                    .beginBroadcast()
+            } returns 2
+
+            every {
+                anyConstructed<RemoteCallbackList<IAuthenticatorBridgeServiceCallback>>()
+                    .getBroadcastItem(0)
+            } returns deadCallback
+
+            every {
+                anyConstructed<RemoteCallbackList<IAuthenticatorBridgeServiceCallback>>()
+                    .getBroadcastItem(1)
+            } returns serviceCallback
+
+            val sharedAccountData = mockk<SharedAccountData>()
+            val expected = mockk<EncryptedSharedAccountData>()
+
+            every {
+                authenticatorBridgeRepository.authenticatorSyncSymmetricKey
+            } returns SYMMETRIC_KEY.symmetricEncryptionKey.byteArray
+
+            coEvery {
+                authenticatorBridgeRepository.getSharedAccounts()
+            } returns sharedAccountData
+
+            every {
+                sharedAccountData.encrypt(SYMMETRIC_KEY)
+            } returns expected.asSuccess()
+
+            getDefaultBinder().syncAccounts()
+
+            assertEquals(expected, lastAccountsSync)
+        }
+
+        @Test
+        fun `syncAccounts should still notify later callbacks when one throws unexpectedly`() {
+            val faultyCallback = object : IAuthenticatorBridgeServiceCallback.Stub() {
+                override fun onAccountsSync(data: EncryptedSharedAccountData?) {
+                    throw IllegalStateException("unexpected failure")
+                }
+            }
+
+            every {
+                anyConstructed<RemoteCallbackList<IAuthenticatorBridgeServiceCallback>>()
+                    .beginBroadcast()
+            } returns 2
+
+            every {
+                anyConstructed<RemoteCallbackList<IAuthenticatorBridgeServiceCallback>>()
+                    .getBroadcastItem(0)
+            } returns faultyCallback
+
+            every {
+                anyConstructed<RemoteCallbackList<IAuthenticatorBridgeServiceCallback>>()
+                    .getBroadcastItem(1)
+            } returns serviceCallback
+
+            val sharedAccountData = mockk<SharedAccountData>()
+            val expected = mockk<EncryptedSharedAccountData>()
+
+            every {
+                authenticatorBridgeRepository.authenticatorSyncSymmetricKey
+            } returns SYMMETRIC_KEY.symmetricEncryptionKey.byteArray
+
+            coEvery {
+                authenticatorBridgeRepository.getSharedAccounts()
+            } returns sharedAccountData
+
+            every {
+                sharedAccountData.encrypt(SYMMETRIC_KEY)
+            } returns expected.asSuccess()
+
+            getDefaultBinder().syncAccounts()
+
+            assertEquals(expected, lastAccountsSync)
+            verify(exactly = 1) {
+                anyConstructed<RemoteCallbackList<IAuthenticatorBridgeServiceCallback>>()
+                    .finishBroadcast()
+            }
+        }
+
        @Test
        fun `syncAccounts when symmetricEncryptionKeyData is null should do nothing`() {
            every { authenticatorBridgeRepository.authenticatorSyncSymmetricKey } returns null