fix(otp): handle conflicting issuer in otpauth URI query param

Cargo.toml

@@ -44,6 +44,7 @@ tempfile = "3.27.0"
 thiserror = "2.0.18"
 totp-rs = { version = "5.7.1", features = ["otpauth"] }
 url = "2.5.8"
+urlencoding = "2.1.3"
 
 [dev-dependencies]
 assert_cmd = "2.2.2"

src/password_store/otp.rs

@@ -1,5 +1,5 @@
 use serde::Serialize;
-use totp_rs::TOTP;
+use totp_rs::{TOTP, TotpUrlError};
 use url::Url;
 
 use super::{DecryptedEntry, PasswordStoreError};
@@ -19,7 +19,7 @@ impl OtpCode {
             .ok_or(PasswordStoreError::OtpNotFound)?;
         let normalized_uri = normalize_otp_uri(otp_uri)?;
         let totp = TOTP::from_url_unchecked(&normalized_uri)
-            .map_err(|_| PasswordStoreError::InvalidOtpUri)?;
+            .map_err(|e| PasswordStoreError::InvalidOtpUri(totp_error_message(e)))?;
 
         Ok(Self {
             code: totp.generate(timestamp),
@@ -29,6 +29,25 @@ impl OtpCode {
     }
 }
 
+fn totp_error_message(err: TotpUrlError) -> String {
+    match err {
+        TotpUrlError::Url(e) => format!("invalid URL: {e}"),
+        TotpUrlError::Scheme(s) => format!("invalid scheme: {s}"),
+        TotpUrlError::Host(s) => format!("invalid host: {s}"),
+        TotpUrlError::Secret(_) => "invalid base32 secret".to_string(),
+        TotpUrlError::SecretSize(n) => format!("secret too short: {n} bits"),
+        TotpUrlError::Algorithm(s) => format!("unknown algorithm: {s}"),
+        TotpUrlError::Digits(s) => format!("invalid digits: {s}"),
+        TotpUrlError::DigitsNumber(n) => format!("invalid digits count: {n}"),
+        TotpUrlError::Step(s) => format!("invalid period: {s}"),
+        TotpUrlError::Issuer(s) => format!("issuer contains colon: {s}"),
+        TotpUrlError::IssuerDecoding(s) => format!("could not decode issuer: {s}"),
+        TotpUrlError::IssuerMistmatch(a, b) => format!("issuer mismatch: {a} != {b}"),
+        TotpUrlError::AccountName(s) => format!("invalid account name: {s}"),
+        TotpUrlError::AccountNameDecoding(s) => format!("could not decode account name: {s}"),
+    }
+}
+
 fn remaining_seconds(timestamp: u64, period: u64) -> u64 {
     let elapsed = timestamp % period;
 
@@ -40,9 +59,16 @@ fn remaining_seconds(timestamp: u64, period: u64) -> u64 {
 }
 
 fn normalize_otp_uri(otp_uri: &str) -> Result<String, PasswordStoreError> {
-    let mut url = Url::parse(otp_uri).map_err(|_| PasswordStoreError::InvalidOtpUri)?;
+    let mut url = Url::parse(otp_uri)
+        .map_err(|e| PasswordStoreError::InvalidOtpUri(format!("invalid URL: {e}")))?;
+
+    let path_has_issuer = urlencoding::decode(url.path().trim_start_matches('/'))
+        .ok()
+        .is_some_and(|decoded| decoded.contains(':'));
+
     let query_pairs = url
         .query_pairs()
+        .filter(|(key, _)| !(path_has_issuer && key.eq_ignore_ascii_case("issuer")))
         .map(|(key, value)| {
             let normalized_value = normalize_query_value(&key, &value);
 
@@ -65,7 +91,10 @@ fn normalize_query_value(key: &str, value: &str) -> String {
 
 #[cfg(test)]
 mod tests {
-    use super::{DecryptedEntry, OtpCode, normalize_otp_uri, remaining_seconds};
+    use super::{
+        DecryptedEntry, OtpCode, normalize_otp_uri, remaining_seconds, totp_error_message,
+    };
+    use totp_rs::TotpUrlError;
 
     #[test]
     fn generates_deterministic_totp_code() {
@@ -107,6 +136,21 @@ otpauth://totp/Stripe:test?secret=eq2gkb3bljy7hansqf2kmqb7
         assert_eq!(code.code.len(), 6);
     }
 
+    #[test]
+    fn generates_code_with_conflicting_issuer_in_query() {
+        let entry = DecryptedEntry::parse(
+            "\
+secret
+otpauth://totp/Politecnico+Grancolombiano:user@example.com?secret=KRSXG5CTMVRXEZLUKN2XAZLSKNSWG4TFOQ&issuer=Microsoft
+",
+        );
+
+        let code = OtpCode::generate_at(&entry, 1000).expect("otp");
+
+        assert_eq!(code.code.len(), 6);
+        assert_eq!(code.code, "804420");
+    }
+
     #[test]
     fn preserves_non_secret_query_parameters() {
         let normalized = normalize_otp_uri(
@@ -115,7 +159,118 @@ otpauth://totp/Stripe:test?secret=eq2gkb3bljy7hansqf2kmqb7
         .expect("normalized uri");
 
         assert!(normalized.contains("secret=EQ2GKB3BLJY7HANSQF2KMQB7"));
-        assert!(normalized.contains("issuer=Stripe"));
         assert!(normalized.contains("period=60"));
+        assert!(normalized.contains("/Stripe:test"));
+        // issuer query param dropped because path already has issuer
+        assert!(!normalized.contains("issuer=Stripe"));
+    }
+
+    #[test]
+    fn drops_issuer_param_when_path_has_issuer() {
+        let normalized = normalize_otp_uri(
+            "otpauth://totp/Politecnico+Grancolombiano:user@example.com?secret=KRSXG5CTMVRXEZLUKN2XAZLSKNSWG4TFOQ&issuer=Microsoft",
+        )
+        .expect("normalized uri");
+
+        assert!(normalized.contains("secret=KRSXG5CTMVRXEZLUKN2XAZLSKNSWG4TFOQ"));
+        assert!(normalized.contains("/Politecnico+Grancolombiano:user@example.com"));
+        assert!(!normalized.contains("issuer=Microsoft"));
+    }
+
+    #[test]
+    fn keeps_issuer_param_when_path_has_no_issuer() {
+        let normalized = normalize_otp_uri(
+            "otpauth://totp/user@example.com?secret=KRSXG5CTMVRXEZLUKN2XAZLSKNSWG4TFOQ&issuer=GitHub",
+        )
+        .expect("normalized uri");
+
+        assert!(normalized.contains("issuer=GitHub"));
+    }
+
+    #[test]
+    fn drops_issuer_param_when_path_has_percent_encoded_colon() {
+        let normalized = normalize_otp_uri(
+            "otpauth://totp/Politecnico+Grancolombiano%3auser@example.com?secret=KRSXG5CTMVRXEZLUKN2XAZLSKNSWG4TFOQ&issuer=Microsoft",
+        )
+        .expect("normalized uri");
+
+        assert!(normalized.contains("secret=KRSXG5CTMVRXEZLUKN2XAZLSKNSWG4TFOQ"));
+        assert!(!normalized.contains("issuer=Microsoft"));
+    }
+
+    #[test]
+    fn generates_code_with_percent_encoded_colon_in_path() {
+        let entry = DecryptedEntry::parse(
+            "\
+secret
+otpauth://totp/Politecnico+Grancolombiano%3auser@example.com?secret=KRSXG5CTMVRXEZLUKN2XAZLSKNSWG4TFOQ&issuer=Microsoft
+",
+        );
+
+        let code = OtpCode::generate_at(&entry, 1000).expect("otp");
+
+        assert_eq!(code.code.len(), 6);
+        assert_eq!(code.code, "804420");
+    }
+
+    #[test]
+    fn totp_error_message_does_not_leak_secret() {
+        let err = TotpUrlError::Secret("supersecret123".to_string());
+        let msg = totp_error_message(err);
+        assert_eq!(msg, "invalid base32 secret");
+    }
+
+    #[test]
+    fn totp_error_message_variants() {
+        let cases = vec![
+            (TotpUrlError::SecretSize(80), "secret too short: 80 bits"),
+            (
+                TotpUrlError::Algorithm("MD5".to_string()),
+                "unknown algorithm: MD5",
+            ),
+            (TotpUrlError::Host("hotp".to_string()), "invalid host: hotp"),
+            (
+                TotpUrlError::Scheme("https".to_string()),
+                "invalid scheme: https",
+            ),
+            (
+                TotpUrlError::Digits("abc".to_string()),
+                "invalid digits: abc",
+            ),
+            (TotpUrlError::DigitsNumber(5), "invalid digits count: 5"),
+            (TotpUrlError::Step("xyz".to_string()), "invalid period: xyz"),
+            (
+                TotpUrlError::Issuer("Iss:uer".to_string()),
+                "issuer contains colon: Iss:uer",
+            ),
+            (
+                TotpUrlError::IssuerDecoding("iss%uer".to_string()),
+                "could not decode issuer: iss%uer",
+            ),
+            (
+                TotpUrlError::IssuerMistmatch("Google".to_string(), "Github".to_string()),
+                "issuer mismatch: Google != Github",
+            ),
+            (
+                TotpUrlError::AccountName("Laziz:".to_string()),
+                "invalid account name: Laziz:",
+            ),
+            (
+                TotpUrlError::AccountNameDecoding("Laz%iz".to_string()),
+                "could not decode account name: Laz%iz",
+            ),
+            (
+                TotpUrlError::Url(url::ParseError::EmptyHost),
+                "invalid URL: empty host",
+            ),
+        ];
+
+        for (err, expected) in cases {
+            assert_eq!(
+                totp_error_message(err),
+                expected,
+                "mismatch for variant {expected}",
+            );
+        }
     }
 }

src/password_store/store_directory.rs

@@ -120,8 +120,8 @@ pub enum PasswordStoreError {
     #[error("entry does not contain an otpauth URI")]
     OtpNotFound,
 
-    #[error("entry contains an invalid otpauth URI")]
-    InvalidOtpUri,
+    #[error("entry contains an invalid otpauth URI: {0}")]
+    InvalidOtpUri(String),
 
     #[error("recipient not found: {0}")]
     RecipientNotFound(String),
@@ -153,7 +153,7 @@ impl PasswordStoreError {
             Self::GpgOutputNotUtf8(_) => "gpg_output_not_utf8",
             Self::GpgVersionFailed(_) => "gpg_version_failed",
             Self::OtpNotFound => "otp_not_found",
-            Self::InvalidOtpUri => "invalid_otp_uri",
+            Self::InvalidOtpUri(_) => "invalid_otp_uri",
             Self::RecipientNotFound(_) => "recipient_not_found",
             Self::Io(_) => "io_error",
         }

tests/otp_command.rs

@@ -178,7 +178,7 @@ otpauth://totp/example?secret=not-valid-secret
         .assert()
         .failure()
         .stderr(predicate::str::contains(
-            "entry contains an invalid otpauth URI",
+            "entry contains an invalid otpauth URI: invalid base32 secret",
         ))
         .stderr(predicate::str::contains(leaked_secret).not());
 }