From 3f1a6e97fd48bd6e9c7e23e83d3af974fd637dc2 Mon Sep 17 00:00:00 2001 From: Phil Cappelli <150719757+phil-livefront@users.noreply.github.com> Date: Thu, 27 Feb 2025 16:42:38 -0500 Subject: [PATCH] PM-17568 - Authenticator Sync: Sometimes synced verification codes only display the TOTP Key, not Issuer/username (#4783) --- .../AuthenticatorBridgeRepositoryImpl.kt | 14 +- .../repository/util/TotpUriSanitizer.kt | 75 +++++++++ .../AuthenticatorBridgeRepositoryTest.kt | 24 ++- .../repository/util/TotpUriSanitizerTest.kt | 153 ++++++++++++++++++ 4 files changed, 257 insertions(+), 9 deletions(-) create mode 100644 app/src/main/java/com/x8bit/bitwarden/data/platform/repository/util/TotpUriSanitizer.kt create mode 100644 app/src/test/java/com/x8bit/bitwarden/data/platform/repository/util/TotpUriSanitizerTest.kt diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/AuthenticatorBridgeRepositoryImpl.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/AuthenticatorBridgeRepositoryImpl.kt index 132b949454..26a43eed7c 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/AuthenticatorBridgeRepositoryImpl.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/AuthenticatorBridgeRepositoryImpl.kt @@ -3,6 +3,7 @@ package com.x8bit.bitwarden.data.platform.repository import com.bitwarden.authenticatorbridge.model.SharedAccountData import com.x8bit.bitwarden.data.auth.datasource.disk.AuthDiskSource import com.x8bit.bitwarden.data.auth.repository.AuthRepository +import com.x8bit.bitwarden.data.platform.repository.util.sanitizeTotpUri import com.x8bit.bitwarden.data.vault.datasource.disk.VaultDiskSource import com.x8bit.bitwarden.data.vault.datasource.sdk.VaultSdkSource import com.x8bit.bitwarden.data.vault.repository.VaultRepository @@ -96,18 +97,21 @@ class AuthenticatorBridgeRepositoryImpl( val totpUris = vaultDiskSource .getCiphers(userId) .first() - // Filter out any ciphers without a totp item and also deleted ciphers: + // Filter out any ciphers without a totp item and also deleted ciphers .filter { it.login?.totp != null && it.deletedDate == null } .mapNotNull { - // Decrypt each cipher and take just totp codes: - vaultSdkSource + val decryptedCipher = vaultSdkSource .decryptCipher( userId = userId, cipher = it.toEncryptedSdkCipher(), ) .getOrNull() - ?.login - ?.totp + + val rawTotp = decryptedCipher?.login?.totp + val cipherName = decryptedCipher?.name + val username = decryptedCipher?.login?.username + + rawTotp.sanitizeTotpUri(cipherName, username) } // Lock the user's vault if we unlocked it for this operation: diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/util/TotpUriSanitizer.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/util/TotpUriSanitizer.kt new file mode 100644 index 0000000000..9cd60510a4 --- /dev/null +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/util/TotpUriSanitizer.kt @@ -0,0 +1,75 @@ +package com.x8bit.bitwarden.data.platform.repository.util + +import java.net.URLEncoder + +private const val OTPAUTH_PREFIX = "otpauth://totp/" +private const val STEAM_PREFIX = "steam://" + +/** + * Utility for ensuring that a given TOTP string is a properly formatted otpauth:// or steam:// URI. + * If the input TOTP is already a valid URI, it is returned as-is. + * If the TOTP is manually entered and does not follow the URI format, + * this function reconstructs it using the provided issuer and username. + * + * Uses this as a guide for format + * https://github.com/google/google-authenticator/wiki/Key-Uri-Format + * + * Replace spaces (+) with %20, and encode the label and issuer (per the above link) + * https://datatracker.ietf.org/doc/html/rfc5234 + * */ +fun String?.sanitizeTotpUri( + issuer: String?, + username: String?, +): String? { + if (this.isNullOrBlank()) return null + + return if (this.startsWith(OTPAUTH_PREFIX) || this.startsWith(STEAM_PREFIX)) { + // ✅ Already a valid TOTP or Steam URI, return as-is. + this + } else { + // ❌ Manually entered secret, reconstruct as otpauth://totp/ URI. + + // Trim spaces from issuer and username + val trimmedIssuer = issuer + ?.trim() + ?.takeIf { it.isNotEmpty() } + + val trimmedUsername = username + ?.trim() + ?.takeIf { it.isNotEmpty() } + + // Determine raw label correctly (avoid empty `:` issue) + val rawLabel = if (trimmedIssuer != null && trimmedUsername != null) { + "$trimmedIssuer:$trimmedUsername" + } else { + trimmedUsername + } + + // Encode label only if it's not empty + val encodedLabel = rawLabel + ?.let { + URLEncoder + .encode(it, "UTF-8") + .replace("+", "%20") + } + .orEmpty() + + // Encode issuer separately for the query parameter + val encodedIssuer = trimmedIssuer?.let { + URLEncoder + .encode(it, "UTF-8") + .replace("+", "%20") + } + + // Construct the issuer query parameter. + val issuerParameter = encodedIssuer + ?.let { "&issuer=$it" } + .orEmpty() + + // Remove spaces from the manually entered secret + val sanitizedSecret = this.filterNot { it.isWhitespace() } + + // Construct final TOTP URI + "$OTPAUTH_PREFIX$encodedLabel?secret=$sanitizedSecret$issuerParameter" + } +} diff --git a/app/src/test/java/com/x8bit/bitwarden/data/platform/repository/AuthenticatorBridgeRepositoryTest.kt b/app/src/test/java/com/x8bit/bitwarden/data/platform/repository/AuthenticatorBridgeRepositoryTest.kt index 5bb97ec79d..4674145c45 100644 --- a/app/src/test/java/com/x8bit/bitwarden/data/platform/repository/AuthenticatorBridgeRepositoryTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/data/platform/repository/AuthenticatorBridgeRepositoryTest.kt @@ -8,6 +8,7 @@ import com.bitwarden.vault.CipherView import com.x8bit.bitwarden.data.auth.datasource.disk.util.FakeAuthDiskSource import com.x8bit.bitwarden.data.auth.repository.AuthRepository import com.x8bit.bitwarden.data.auth.repository.model.UserState +import com.x8bit.bitwarden.data.platform.repository.util.sanitizeTotpUri import com.x8bit.bitwarden.data.platform.util.asSuccess import com.x8bit.bitwarden.data.vault.datasource.disk.VaultDiskSource import com.x8bit.bitwarden.data.vault.datasource.network.model.SyncResponseJson @@ -112,12 +113,17 @@ class AuthenticatorBridgeRepositoryTest { coEvery { vaultSdkSource.decryptCipher(USER_2_ID, USER_2_ENCRYPTED_SDK_TOTP_CIPHER) } returns USER_2_DECRYPTED_TOTP_CIPHER.asSuccess() + mockkStatic(String::sanitizeTotpUri) + every { + any().sanitizeTotpUri(any(), any()) + } returns "totp" } @AfterEach fun teardown() { confirmVerified(authRepository, vaultSdkSource, vaultRepository, vaultDiskSource) unmockkStatic(SyncResponseJson.Cipher::toEncryptedSdkCipher) + unmockkStatic(String::sanitizeTotpUri) } @Test @@ -385,31 +391,41 @@ private val USER_STATE = UserState( private val USER_1_TOTP_CIPHER = mockk { every { login?.totp } returns "encryptedTotp1" + every { login?.username } returns "username" every { deletedDate } returns null + every { name } returns "cipher1" } private val USER_1_DELETED_TOTP_CIPHER = mockk { every { login?.totp } returns "encryptedTotp1Deleted" + every { login?.username } returns "username" every { deletedDate } returns ZonedDateTime.now() + every { name } returns "cipher1" } private val USER_2_TOTP_CIPHER = mockk { every { login?.totp } returns "encryptedTotp2" + every { login?.username } returns "username" every { deletedDate } returns null + every { name } returns "cipher2" } private val USER_1_ENCRYPTED_SDK_TOTP_CIPHER = mockk() private val USER_2_ENCRYPTED_SDK_TOTP_CIPHER = mockk() private val USER_1_DECRYPTED_TOTP_CIPHER = mockk { - every { login?.totp } returns "totp1" + every { login?.totp } returns "totp" + every { login?.username } returns "username" + every { name } returns "cipher1" } private val USER_2_DECRYPTED_TOTP_CIPHER = mockk { - every { login?.totp } returns "totp2" + every { login?.totp } returns "totp" + every { login?.username } returns "username" + every { name } returns "cipher1" } -private val USER_1_EXPECTED_TOTP_LIST = listOf("totp1") -private val USER_2_EXPECTED_TOTP_LIST = listOf("totp2") +private val USER_1_EXPECTED_TOTP_LIST = listOf("totp") +private val USER_2_EXPECTED_TOTP_LIST = listOf("totp") private val USER_1_SHARED_ACCOUNT = SharedAccountData.Account( userId = ACCOUNT_1.userId, diff --git a/app/src/test/java/com/x8bit/bitwarden/data/platform/repository/util/TotpUriSanitizerTest.kt b/app/src/test/java/com/x8bit/bitwarden/data/platform/repository/util/TotpUriSanitizerTest.kt new file mode 100644 index 0000000000..63a5a8bce0 --- /dev/null +++ b/app/src/test/java/com/x8bit/bitwarden/data/platform/repository/util/TotpUriSanitizerTest.kt @@ -0,0 +1,153 @@ +package com.x8bit.bitwarden.data.platform.repository.util + +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Assertions.assertNull +import org.junit.jupiter.api.Test + +class TotpUriSanitizerTest { + + @Test + fun `valid TOTP URI should remain unchanged`() { + val validUri = "otpauth://totp/Company:test@test.com?secret=abcdef12345&issuer=Company" + val result = validUri.sanitizeTotpUri("Company", "test@test.com") + + assertEquals(validUri, result) + } + + @Test + fun `manually entered TOTP with both issuer and account should be converted correctly`() { + val rawTotp = "zdd3 jqnn" + val expectedUri = "otpauth://totp/Company%3Atest%40test.com?secret=zdd3jqnn&issuer=Company" + val result = rawTotp.sanitizeTotpUri("Company", "test@test.com") + + assertEquals(expectedUri, result) + } + + @Test + fun `only issuer provided should not be included in label but should be in query`() { + val rawTotp = "secret123" + val expectedUri = "otpauth://totp/?secret=secret123&issuer=Company" + val result = rawTotp.sanitizeTotpUri("Company", null) + + assertEquals(expectedUri, result) + } + + @Test + fun `only account name provided should be included in label`() { + val rawTotp = "secret456" + val expectedUri = "otpauth://totp/test%40email.com?secret=secret456" + val result = rawTotp.sanitizeTotpUri(null, "test@email.com") + + assertEquals(expectedUri, result) + } + + @Test + fun `both issuer and account missing should generate minimal valid URI`() { + val rawTotp = "secret789" + val expectedUri = "otpauth://totp/?secret=secret789" + val result = rawTotp.sanitizeTotpUri(null, null) + + assertEquals(expectedUri, result) + } + + @Test + fun `extra spaces in TOTP secret should be removed`() { + val rawTotp = "a b c d e f 1 2 3" + val expectedUri = "otpauth://totp/Issuer%3Auser%40domain.com?secret=abcdef123&issuer=Issuer" + val result = rawTotp.sanitizeTotpUri("Issuer", "user@domain.com") + + assertEquals(expectedUri, result) + } + + @Test + fun `null TOTP input should return null`() { + val result = null.sanitizeTotpUri("Company", "test@test.com") + assertNull(result) + } + + @Test + fun `empty TOTP should return null`() { + val result = "".sanitizeTotpUri("Company", "test@test.com") + assertNull(result) + } + + @Test + fun `invalid characters in issuer and account should be properly encoded`() { + val rawTotp = "secure" + val expected = "otpauth://totp/My%20App%3Auser%40example.com?secret=secure&issuer=My%20App" + val result = rawTotp.sanitizeTotpUri("My App", "user@example.com") + + assertEquals(expected, result) + } + + @Test + fun `issuer with special characters should be encoded correctly`() { + val rawTotp = "tokenvalue" + val expected = "otpauth://totp/?secret=tokenvalue&issuer=Super%26Secure" + val result = rawTotp.sanitizeTotpUri("Super&Secure", null) + + assertEquals(expected, result) + } + + @Test + fun `account name with special characters should be encoded correctly`() { + val rawTotp = "secret999" + val expected = "otpauth://totp/user%2Bname%40email.com?secret=secret999" + val result = rawTotp.sanitizeTotpUri(null, "user+name@email.com") + + assertEquals(expected, result) + } + + @Test + fun `both issuer and account name empty should generate minimal valid URI`() { + val rawTotp = "secretminimal" + val expected = "otpauth://totp/?secret=secretminimal" + val result = rawTotp.sanitizeTotpUri("", "") + + assertEquals(expected, result) + } + + @Test + fun `both issuer and account name are null should generate minimal valid URI`() { + val rawTotp = "secretminimal" + val expected = "otpauth://totp/?secret=secretminimal" + val result = rawTotp.sanitizeTotpUri(null, null) + + assertEquals(expected, result) + } + + @Test + fun `account name with spaces should be properly encoded`() { + val rawTotp = "secret1234" + val expectedUri = "otpauth://totp/John%20Doe%40email.com?secret=secret1234" + val result = rawTotp.sanitizeTotpUri(null, "John Doe@email.com") + + assertEquals(expectedUri, result) + } + + @Test + fun `TOTP secret with leading and trailing spaces should be sanitized`() { + val rawTotp = " a1b2c3d4 " + val expected = "otpauth://totp/Company%3Atest%40test.com?secret=a1b2c3d4&issuer=Company" + val result = rawTotp.sanitizeTotpUri("Company", "test@test.com") + + assertEquals(expected, result) + } + + @Test + fun `issuer and account name with trailing spaces should be trimmed before encoding`() { + val rawTotp = "secure" + val expected = "otpauth://totp/Company%3Auser%40secure.com?secret=secure&issuer=Company" + val result = rawTotp.sanitizeTotpUri(" Company ", " user@secure.com ") + + assertEquals(expected, result) + } + + @Test + fun `valid Steam URI should remain unchanged`() { + val validSteamUri = "steam://abcdef12345" + val result = validSteamUri.sanitizeTotpUri("Steam", "test@test.com") + + assertEquals(validSteamUri, result) + } +}