fix(format/common): validate TOTP secret ahead of time

Fixes #2949

2 files changed, 34 insertions, 13 deletions

diff --git a/format/common/src/main/kotlin/app/passwordstore/data/passfile/PasswordEntry.kt b/format/common/src/main/kotlin/app/passwordstore/data/passfile/PasswordEntry.kt
index 1618374f..f4dcc2b7 100644
--- a/format/common/src/main/kotlin/app/passwordstore/data/passfile/PasswordEntry.kt
+++ b/format/common/src/main/kotlin/app/passwordstore/data/passfile/PasswordEntry.kt
@@ -9,6 +9,9 @@ import androidx.annotation.VisibleForTesting
 import app.passwordstore.util.time.UserClock
 import app.passwordstore.util.totp.Otp
 import app.passwordstore.util.totp.TotpFinder
+import com.github.michaelbull.result.Err
+import com.github.michaelbull.result.Ok
+import com.github.michaelbull.result.Result
 import com.github.michaelbull.result.mapBoth
 import dagger.assisted.Assisted
 import dagger.assisted.AssistedFactory
@@ -60,14 +63,22 @@ constructor(
     require(totpSecret != null) { "Cannot collect this flow without a TOTP secret" }
     do {
       val otp = calculateTotp()
-      emit(otp)
-      delay(THOUSAND_MILLIS.milliseconds)
+      if (otp.isOk) {
+        emit(otp.value)
+        delay(THOUSAND_MILLIS.milliseconds)
+      } else {
+        throw otp.error
+      }
     } while (coroutineContext.isActive)
   }
 
   /** Obtain the [Totp.value] for this [PasswordEntry] at the current time. */
   public val currentOtp: String
-    get() = calculateTotp().value
+    get() {
+      val otp = calculateTotp()
+      check(otp.isOk)
+      return otp.value.value
+    }
 
   /**
    * String representation of [extraContent] but with authentication related data such as TOTP URIs
@@ -83,7 +94,14 @@ constructor(
     extraContentWithoutAuthData = generateExtraContentWithoutAuthData()
     extraContent = generateExtraContentPairs()
     username = findUsername()
-    totpSecret = totpFinder.findSecret(content)
+    // Verify the TOTP secret is valid and disable TOTP if not.
+    val secret = totpFinder.findSecret(content)
+    totpSecret =
+      if (secret != null && calculateTotp(secret).isOk) {
+        secret
+      } else {
+        null
+      }
   }
 
   public fun hasTotp(): Boolean {
@@ -175,26 +193,21 @@ constructor(
     return null
   }
 
-  private fun calculateTotp(): Totp {
+  private fun calculateTotp(secret: String = totpSecret!!): Result<Totp, Throwable> {
     val digits = totpFinder.findDigits(content)
     val totpPeriod = totpFinder.findPeriod(content)
     val totpAlgorithm = totpFinder.findAlgorithm(content)
     val issuer = totpFinder.findIssuer(content)
     val millis = clock.millis()
     val remainingTime = (totpPeriod - ((millis / THOUSAND_MILLIS) % totpPeriod)).seconds
-    Otp.calculateCode(
-        totpSecret!!,
+    return Otp.calculateCode(
+        secret,
         millis / (THOUSAND_MILLIS * totpPeriod),
         totpAlgorithm,
         digits,
         issuer
       )
-      .mapBoth(
-        { code ->
-          return Totp(code, remainingTime)
-        },
-        { throwable -> throw throwable }
-      )
+      .mapBoth({ code -> Ok(Totp(code, remainingTime)) }, ::Err)
   }
 
   @AssistedFactory
diff --git a/format/common/src/test/kotlin/app/passwordstore/data/passfile/PasswordEntryTest.kt b/format/common/src/test/kotlin/app/passwordstore/data/passfile/PasswordEntryTest.kt
index 865a13c1..95fb3e62 100644
--- a/format/common/src/test/kotlin/app/passwordstore/data/passfile/PasswordEntryTest.kt
+++ b/format/common/src/test/kotlin/app/passwordstore/data/passfile/PasswordEntryTest.kt
@@ -13,6 +13,7 @@ import app.passwordstore.util.totp.UriTotpFinder
 import java.util.Locale
 import kotlin.test.Test
 import kotlin.test.assertEquals
+import kotlin.test.assertFalse
 import kotlin.test.assertNotNull
 import kotlin.test.assertNull
 import kotlin.test.assertTrue
@@ -181,6 +182,13 @@ class PasswordEntryTest {
     }
   }
 
+  // https://github.com/android-password-store/Android-Password-Store/issues/2949
+  @Test
+  fun disablesTotpForInvalidUri() = runTest {
+    val entry = makeEntry("password\notpauth://totp/otp-secret?secret=")
+    assertFalse(entry.hasTotp())
+  }
+
   @Test
   fun onlyLooksForUriInFirstLine() {
     val entry = makeEntry("id:\n$TOTP_URI")