From 70740b51a2ecedff9b6e1a5788ce79bdd5b987a7 Mon Sep 17 00:00:00 2001 From: Cristhian Melo Date: Fri, 19 Jun 2026 18:28:28 -0500 Subject: [PATCH 1/2] fix(otp): handle conflicting issuer in otpauth URI query param When the OTP URI has an issuer in the path label (e.g., 'issuer:account') and a different issuer in the query parameter, totp-rs rejects it with an issuer mismatch error. Drop the issuer query parameter when the path already contains an issuer, since the path issuer takes precedence per the otpauth spec. Also improve error reporting by preserving the specific TotpUrlError cause instead of mapping to a generic message, and by showing the URL parse error when normalization fails. --- Cargo.lock | 1 + Cargo.toml | 1 + src/password_store/otp.rs | 169 +++++++++++++++++++++++++- src/password_store/store_directory.rs | 6 +- tests/otp_command.rs | 2 +- 5 files changed, 170 insertions(+), 9 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index bc5ae63..8b23483 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -803,6 +803,7 @@ dependencies = [ "thiserror", "totp-rs", "url", + "urlencoding", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index e2f9059..32d201a 100644 --- a/Cargo.toml +++ b/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" diff --git a/src/password_store/otp.rs b/src/password_store/otp.rs index d2176fa..ebe27a3 100644 --- a/src/password_store/otp.rs +++ b/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 { - 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,8 @@ 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 +134,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 +157,124 @@ 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}", + ); + } } } diff --git a/src/password_store/store_directory.rs b/src/password_store/store_directory.rs index 1363e64..b55888d 100644 --- a/src/password_store/store_directory.rs +++ b/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", } diff --git a/tests/otp_command.rs b/tests/otp_command.rs index d9f2ca7..b777497 100644 --- a/tests/otp_command.rs +++ b/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()); } From 7984f26e16cfe546443d6058c4238119356373c5 Mon Sep 17 00:00:00 2001 From: Cristhian Melo Date: Fri, 19 Jun 2026 18:35:57 -0500 Subject: [PATCH 2/2] style: apply cargo fmt --- src/password_store/otp.rs | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/password_store/otp.rs b/src/password_store/otp.rs index ebe27a3..3426d3d 100644 --- a/src/password_store/otp.rs +++ b/src/password_store/otp.rs @@ -91,7 +91,9 @@ fn normalize_query_value(key: &str, value: &str) -> String { #[cfg(test)] mod tests { - use super::{DecryptedEntry, OtpCode, normalize_otp_uri, remaining_seconds, totp_error_message}; + use super::{ + DecryptedEntry, OtpCode, normalize_otp_uri, remaining_seconds, totp_error_message, + }; use totp_rs::TotpUrlError; #[test] @@ -226,10 +228,7 @@ otpauth://totp/Politecnico+Grancolombiano%3auser@example.com?secret=KRSXG5CTMVRX TotpUrlError::Algorithm("MD5".to_string()), "unknown algorithm: MD5", ), - ( - TotpUrlError::Host("hotp".to_string()), - "invalid host: hotp", - ), + (TotpUrlError::Host("hotp".to_string()), "invalid host: hotp"), ( TotpUrlError::Scheme("https".to_string()), "invalid scheme: https", @@ -239,10 +238,7 @@ otpauth://totp/Politecnico+Grancolombiano%3auser@example.com?secret=KRSXG5CTMVRX "invalid digits: abc", ), (TotpUrlError::DigitsNumber(5), "invalid digits count: 5"), - ( - TotpUrlError::Step("xyz".to_string()), - "invalid period: xyz", - ), + (TotpUrlError::Step("xyz".to_string()), "invalid period: xyz"), ( TotpUrlError::Issuer("Iss:uer".to_string()), "issuer contains colon: Iss:uer",