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..3426d3d 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,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}", + ); + } } } 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()); }