From a10fef625b04e582d5177e60528aa95e981864a4 Mon Sep 17 00:00:00 2001 From: Shane Date: Wed, 8 Oct 2025 19:41:42 -0500 Subject: [PATCH 01/12] [PM-24468] Introduce CipherRiskClient --- Cargo.lock | 4 +- crates/bitwarden-vault/Cargo.toml | 2 + crates/bitwarden-vault/src/cipher/cipher.rs | 17 + .../bitwarden-vault/src/cipher/cipher_risk.rs | 73 ++ .../src/cipher/cipher_risk_client.rs | 645 ++++++++++++++++++ crates/bitwarden-vault/src/cipher/mod.rs | 4 + crates/bitwarden-vault/src/error.rs | 11 + crates/bitwarden-vault/src/lib.rs | 2 +- crates/bitwarden-vault/src/vault_client.rs | 11 +- 9 files changed, 765 insertions(+), 4 deletions(-) create mode 100644 crates/bitwarden-vault/src/cipher/cipher_risk.rs create mode 100644 crates/bitwarden-vault/src/cipher/cipher_risk_client.rs diff --git a/Cargo.lock b/Cargo.lock index 972fe3366..a99371332 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1,6 +1,6 @@ # This file is automatically @generated by Cargo. # It is not intended for manual editing. -version = 3 +version = 4 [[package]] name = "addr2line" @@ -937,6 +937,8 @@ dependencies = [ "uuid", "wasm-bindgen", "wasm-bindgen-futures", + "wiremock", + "zxcvbn", ] [[package]] diff --git a/crates/bitwarden-vault/Cargo.toml b/crates/bitwarden-vault/Cargo.toml index 9ae609694..bc03992a0 100644 --- a/crates/bitwarden-vault/Cargo.toml +++ b/crates/bitwarden-vault/Cargo.toml @@ -55,11 +55,13 @@ uniffi = { workspace = true, optional = true } uuid = { workspace = true } wasm-bindgen = { workspace = true, optional = true } wasm-bindgen-futures = { workspace = true, optional = true } +zxcvbn = ">=3.0.1, <4.0" [dev-dependencies] bitwarden-api-api = { workspace = true, features = ["mockall"] } bitwarden-test = { workspace = true } tokio = { workspace = true, features = ["rt"] } +wiremock = { workspace = true } [lints] workspace = true diff --git a/crates/bitwarden-vault/src/cipher/cipher.rs b/crates/bitwarden-vault/src/cipher/cipher.rs index 9c6b36173..16f4a55f3 100644 --- a/crates/bitwarden-vault/src/cipher/cipher.rs +++ b/crates/bitwarden-vault/src/cipher/cipher.rs @@ -486,6 +486,23 @@ impl CipherView { } } + /// Extract login details for risk evaluation (login ciphers only). + /// + /// Returns `Some(CipherLoginDetails)` if this is a login cipher with a password, + /// otherwise returns `None`. + pub fn to_login_details(&self) -> Option { + if let Some(login) = &self.login { + if let Some(password) = &login.password { + return Some(crate::cipher::cipher_risk::CipherLoginDetails { + id: self.id, + password: password.clone(), + username: login.username.clone(), + }); + } + } + None + } + fn reencrypt_attachment_keys( &mut self, ctx: &mut KeyStoreContext, diff --git a/crates/bitwarden-vault/src/cipher/cipher_risk.rs b/crates/bitwarden-vault/src/cipher/cipher_risk.rs new file mode 100644 index 000000000..73ab01ee6 --- /dev/null +++ b/crates/bitwarden-vault/src/cipher/cipher_risk.rs @@ -0,0 +1,73 @@ +use std::collections::HashMap; + +use serde::{Deserialize, Serialize}; +#[cfg(feature = "wasm")] +use {tsify::Tsify, wasm_bindgen::prelude::*}; + +use crate::CipherId; + +/// Minimal login cipher data needed for risk evaluation +#[derive(Serialize, Deserialize, Debug, Clone)] +#[cfg_attr(feature = "uniffi", derive(uniffi::Record))] +#[cfg_attr(feature = "wasm", derive(Tsify), tsify(into_wasm_abi, from_wasm_abi))] +pub struct CipherLoginDetails { + /// Cipher ID to identify which cipher in results + pub id: Option, + /// The decrypted password to evaluate + pub password: String, + /// Username or email (login ciphers only have one field) + pub username: Option, +} + +/// Password reuse map wrapper for WASM compatibility +#[derive(Serialize, Deserialize, Debug, Clone)] +#[cfg_attr(feature = "uniffi", derive(uniffi::Record))] +#[cfg_attr(feature = "wasm", derive(Tsify), tsify(into_wasm_abi, from_wasm_abi))] +#[serde(transparent)] +pub struct PasswordReuseMap { + /// Map of passwords to their occurrence count + #[cfg_attr(feature = "wasm", tsify(type = "Record"))] + pub map: HashMap, +} + +/// Options for configuring risk computation +#[derive(Serialize, Deserialize, Debug, Clone)] +#[cfg_attr(feature = "uniffi", derive(uniffi::Record))] +#[cfg_attr(feature = "wasm", derive(Tsify), tsify(into_wasm_abi, from_wasm_abi))] +#[serde(rename_all = "camelCase")] +#[derive(Default)] +pub struct CipherRiskOptions { + /// Pre-computed password reuse map (password → count) + /// If provided, enables reuse detection across ciphers + pub password_map: Option, + /// Whether to check passwords against Have I Been Pwned API + /// When true, makes network requests to check for exposed passwords + pub check_exposed: bool, +} + +/// Risk evaluation result for a single cipher +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)] +#[cfg_attr(feature = "uniffi", derive(uniffi::Record))] +#[cfg_attr(feature = "wasm", derive(Tsify), tsify(into_wasm_abi, from_wasm_abi))] +pub struct CipherRisk { + /// Cipher ID matching the input CipherLoginDetails + pub id: Option, + /// Password strength score from 0 (weakest) to 4 (strongest) + /// Calculated using zxcvbn with cipher-specific context + pub password_strength: u8, + /// Number of times password appears in HIBP database + /// None if check_exposed was false in options + pub exposed_count: Option, + /// Number of times this password appears in the provided cipher list + /// Minimum value is 1 (the cipher itself) + pub reuse_count: u32, +} + +#[cfg(feature = "wasm")] +impl wasm_bindgen::__rt::VectorIntoJsValue for CipherRisk { + fn vector_into_jsvalue( + vector: wasm_bindgen::__rt::std::boxed::Box<[Self]>, + ) -> wasm_bindgen::JsValue { + wasm_bindgen::__rt::js_value_vector_into_jsvalue(vector) + } +} diff --git a/crates/bitwarden-vault/src/cipher/cipher_risk_client.rs b/crates/bitwarden-vault/src/cipher/cipher_risk_client.rs new file mode 100644 index 000000000..d81d6ca4f --- /dev/null +++ b/crates/bitwarden-vault/src/cipher/cipher_risk_client.rs @@ -0,0 +1,645 @@ +use std::collections::HashMap; + +use bitwarden_core::Client; +#[cfg(feature = "wasm")] +use wasm_bindgen::prelude::wasm_bindgen; + +use super::cipher_risk::{CipherLoginDetails, CipherRisk, CipherRiskOptions, PasswordReuseMap}; +use crate::CipherRiskError; + +/// Client for evaluating credential risk for login ciphers. +#[cfg_attr(feature = "wasm", wasm_bindgen)] +pub struct CipherRiskClient { + pub(crate) client: Client, +} + +#[cfg_attr(feature = "wasm", wasm_bindgen)] +impl CipherRiskClient { + /// Build password reuse map for a list of login ciphers. + /// + /// Returns a map where keys are passwords and values are the number of times + /// each password appears in the provided list. This map can be passed to `compute_risk()` + /// to enable password reuse detection. + /// + /// # Returns + /// PasswordReuseMap containing password occurrence counts + pub fn password_reuse_map( + &self, + login_details: Vec, + ) -> Result { + let mut map = HashMap::new(); + for details in login_details { + if !details.password.is_empty() { + *map.entry(details.password).or_insert(0) += 1; + } + } + Ok(PasswordReuseMap { map }) + } + + /// Evaluate security risks for multiple login ciphers. + /// + /// For each cipher, this method: + /// 1. Calculates password strength (0-4) using zxcvbn with cipher-specific context + /// 2. Optionally checks if the password has been exposed via Have I Been Pwned API + /// 3. Counts how many times the password is reused across the provided ciphers + /// + /// # Returns + /// Vector of `CipherRisk` results, one for each input cipher + /// + /// # Errors + /// Returns `CipherRiskError::Network` if HIBP API requests fail when `check_exposed` is enabled + pub async fn compute_risk( + &self, + login_details: Vec, + options: CipherRiskOptions, + ) -> Result, CipherRiskError> { + let mut results = Vec::with_capacity(login_details.len()); + + for details in login_details { + // Calculate password strength using cipher-specific inputs + let password_strength = + self.calculate_password_strength(&details.password, details.username.as_deref()); + + // Check exposure via HIBP API if enabled + let exposed_count = if options.check_exposed { + Some(self.check_password_exposed(&details.password).await?) + } else { + None + }; + + // Check reuse from provided map (default to 1 if not in map) + let reuse_count = options + .password_map + .as_ref() + .and_then(|reuse_map| reuse_map.map.get(&details.password)) + .copied() + .unwrap_or(1); + + results.push(CipherRisk { + id: details.id, + password_strength, + exposed_count, + reuse_count, + }); + } + + Ok(results) + } + + /// Calculate password strength with cipher-specific context. + /// + /// Uses zxcvbn to score password strength (0-4) and penalizes passwords + /// that contain parts of the username/email. + /// + /// # Returns + /// Score from 0 (weakest) to 4 (strongest) + fn calculate_password_strength(&self, password: &str, username: Option<&str>) -> u8 { + let mut user_inputs = Vec::new(); + + // Extract meaningful parts from username field + if let Some(username) = username { + user_inputs.extend(Self::extract_user_inputs(username)); + } + + // Call zxcvbn with cipher-specific inputs only (no "bitwarden" globals) + let inputs_refs: Vec<&str> = user_inputs.iter().map(|s| s.as_str()).collect(); + zxcvbn::zxcvbn(password, &inputs_refs).score().into() + } + + /// Extract meaningful tokens from username/email for password penalization. + /// + /// Handles both email addresses and plain usernames by: + /// - For emails: extracts and tokenizes the local part (before @) + /// - For usernames: tokenizes the entire string + /// - Splits on non-alphanumeric characters + /// - Converts to lowercase for case-insensitive matching + /// + /// # Returns + /// Vector of lowercase tokens extracted from the input + fn extract_user_inputs(username: &str) -> Vec { + // Check if it's email-like (contains @) + if let Some((local_part, _domain)) = username.split_once('@') { + // Email: extract local part tokens + local_part + .trim() + .to_lowercase() + .split(|c: char| !c.is_alphanumeric()) + .filter(|s| !s.is_empty()) + .map(str::to_owned) + .collect() + } else { + // Username: split on non-alphanumeric + username + .trim() + .to_lowercase() + .split(|c: char| !c.is_alphanumeric()) + .filter(|s| !s.is_empty()) + .map(str::to_owned) + .collect() + } + } + + /// Check if a password has been exposed using the Have I Been Pwned API. + /// + /// Implements k-anonymity model: + /// 1. Hash password with SHA-1 + /// 2. Send only first 5 characters of hash to HIBP API + /// 3. API returns all hash suffixes matching that prefix + /// 4. Check locally if full hash exists in results + /// + /// This ensures the actual password never leaves the client. + /// + /// # Returns + /// Number of times the password appears in HIBP database (0 if not found) + /// + /// # Errors + /// Returns `CipherRiskError::Network` if API request fails + async fn check_password_exposed(&self, password: &str) -> Result { + const HIBP_BASE_URL: &str = "https://api.pwnedpasswords.com"; + + self.check_password_exposed_hibp(password, HIBP_BASE_URL) + .await + } + + /// Hash password with SHA-1 and split into prefix/suffix for k-anonymity. + /// + /// # Returns + /// Tuple of (prefix: first 5 chars, suffix: remaining chars) + fn hash_password_for_hibp(password: &str) -> (String, String) { + use sha1::{Digest, Sha1}; + + let hash = Sha1::digest(password.as_bytes()); + let hash_hex = format!("{:X}", hash); + let (prefix, suffix) = hash_hex.split_at(5); + (prefix.to_string(), suffix.to_string()) + } + + /// Parse HIBP API response to find password hash and return breach count. + /// + /// Response format: "SUFFIX:COUNT\r\n..." (e.g., + /// "0018A45C4D1DEF81644B54AB7F969B88D65:3\r\n...") + /// + /// # Returns + /// Number of times the password appears in breaches (0 if not found) + fn parse_hibp_response(response: &str, target_suffix: &str) -> u32 { + for line in response.lines() { + if let Some((hash_suffix, count_str)) = line.split_once(':') { + if hash_suffix.eq_ignore_ascii_case(target_suffix) { + return count_str.trim().parse().unwrap_or(0); + } + } + } + 0 + } + + /// Check if a password has been exposed using the Have I Been Pwned API. + /// + /// Implements k-anonymity model to ensure privacy: + /// 1. Hash password with SHA-1 + /// 2. Send only first 5 characters of hash to HIBP API + /// 3. API returns all hash suffixes matching that prefix + /// 4. Check locally if full hash exists in results + /// + /// This ensures the actual password never leaves the client. + /// + /// # Arguments + /// * `password` - Password to check for exposure + /// * `base_url` - HIBP API base URL (for testing, can inject mock server URL) + /// + /// # Returns + /// Number of times the password appears in HIBP database (0 if not found) + /// + /// # Errors + /// Returns `CipherRiskError::Network` if API request fails + async fn check_password_exposed_hibp( + &self, + password: &str, + base_url: &str, + ) -> Result { + let (prefix, suffix) = Self::hash_password_for_hibp(password); + + // Query HIBP API with prefix only (k-anonymity) + let url = format!("{}/range/{}", base_url, prefix); + let response = self + .client + .internal + .get_http_client() + .get(&url) + .send() + .await + .map_err(|e| CipherRiskError::Network(e.to_string()))? + .error_for_status() + .map_err(|e| CipherRiskError::Network(e.to_string()))? + .text() + .await + .map_err(|e| CipherRiskError::Network(e.to_string()))?; + + Ok(Self::parse_hibp_response(&response, &suffix)) + } +} + +#[cfg(test)] +mod tests { + use bitwarden_core::client::test_accounts::test_bitwarden_com_account; + + use super::*; + + #[test] + fn test_extract_user_inputs_from_email() { + let inputs = CipherRiskClient::extract_user_inputs("john.doe@example.com"); + assert_eq!(inputs, vec!["john", "doe"]); + } + + #[test] + fn test_extract_user_inputs_from_username() { + let inputs = CipherRiskClient::extract_user_inputs("john_doe123"); + assert_eq!(inputs, vec!["john", "doe123"]); + } + + #[test] + fn test_extract_user_inputs_lowercase() { + let inputs = CipherRiskClient::extract_user_inputs("JohnDoe@Example.COM"); + assert_eq!(inputs, vec!["johndoe"]); + } + + #[test] + fn test_extract_user_inputs_empty() { + let inputs = CipherRiskClient::extract_user_inputs(""); + assert!(inputs.is_empty()); + } + + #[tokio::test] + async fn test_password_reuse_map() { + let client = Client::init_test_account(test_bitwarden_com_account()).await; + let risk_client = CipherRiskClient { + client: client.clone(), + }; + + let login_details = vec![ + CipherLoginDetails { + id: None, + password: "password123".to_string(), + username: Some("user1".to_string()), + }, + CipherLoginDetails { + id: None, + password: "password123".to_string(), + username: Some("user2".to_string()), + }, + CipherLoginDetails { + id: None, + password: "unique_password".to_string(), + username: Some("user3".to_string()), + }, + ]; + + let password_map = risk_client.password_reuse_map(login_details).unwrap(); + + assert_eq!(password_map.map.get("password123"), Some(&2)); + assert_eq!(password_map.map.get("unique_password"), Some(&1)); + } + + #[tokio::test] + async fn test_calculate_password_strength_weak() { + let client = Client::init_test_account(test_bitwarden_com_account()).await; + let risk_client = CipherRiskClient { + client: client.clone(), + }; + + let strength = risk_client.calculate_password_strength("password", None); + assert!(strength <= 1, "Expected weak password, got {}", strength); + } + + #[tokio::test] + async fn test_calculate_password_strength_strong() { + let client = Client::init_test_account(test_bitwarden_com_account()).await; + let risk_client = CipherRiskClient { + client: client.clone(), + }; + + let strength = risk_client.calculate_password_strength("xK9#mP$2qL@7vN&4wR", None); + assert!(strength >= 3, "Expected strong password, got {}", strength); + } + + #[tokio::test] + async fn test_calculate_password_strength_penalizes_username() { + let client = Client::init_test_account(test_bitwarden_com_account()).await; + let risk_client = CipherRiskClient { + client: client.clone(), + }; + + // Password containing username should be weaker + let strength_with_username = + risk_client.calculate_password_strength("johndoe123!", Some("johndoe")); + let strength_without_username = + risk_client.calculate_password_strength("johndoe123!", None); + + assert!( + strength_with_username <= strength_without_username, + "Password should be weaker when it contains username" + ); + } + + #[tokio::test] + async fn test_compute_risk_without_hibp() { + let client = Client::init_test_account(test_bitwarden_com_account()).await; + let risk_client = CipherRiskClient { + client: client.clone(), + }; + + let login_details = vec![ + CipherLoginDetails { + id: None, + password: "password123".to_string(), + username: Some("user1".to_string()), + }, + CipherLoginDetails { + id: None, + password: "password123".to_string(), + username: Some("user2".to_string()), + }, + ]; + + let password_map = risk_client + .password_reuse_map(login_details.clone()) + .unwrap(); + + let options = CipherRiskOptions { + password_map: Some(password_map), + check_exposed: false, + }; + + let risks = risk_client + .compute_risk(login_details, options) + .await + .unwrap(); + + assert_eq!(risks.len(), 2); + assert_eq!(risks[0].reuse_count, 2); + assert_eq!(risks[1].reuse_count, 2); + assert!(risks[0].exposed_count.is_none()); + assert!(risks[1].exposed_count.is_none()); + } + + #[tokio::test] + async fn test_password_reuse_map_empty_passwords() { + let client = Client::init_test_account(test_bitwarden_com_account()).await; + let risk_client = CipherRiskClient { + client: client.clone(), + }; + + let login_details = vec![ + CipherLoginDetails { + id: None, + password: "".to_string(), + username: Some("user1".to_string()), + }, + CipherLoginDetails { + id: None, + password: "valid_password".to_string(), + username: Some("user2".to_string()), + }, + ]; + + let password_map = risk_client.password_reuse_map(login_details).unwrap(); + + // Empty passwords should not be in the map + assert!(password_map.map.get("").is_none()); + assert_eq!(password_map.map.get("valid_password"), Some(&1)); + } + + #[test] + fn test_hash_password_for_hibp() { + // Test with a known password: "password" + // SHA-1 hash of "password" is: 5BAA61E4C9B93F3F0682250B6CF8331B7EE68FD8 + let (prefix, suffix) = CipherRiskClient::hash_password_for_hibp("password"); + + assert_eq!(prefix, "5BAA6"); + assert_eq!(suffix, "1E4C9B93F3F0682250B6CF8331B7EE68FD8"); + + // Validate expected lengths (5 for prefix, 35 for suffix = 40 total SHA-1 hex) + assert_eq!(prefix.len(), 5); + assert_eq!(suffix.len(), 35); + } + + #[test] + fn test_parse_hibp_response_found() { + // Simulate real HIBP API response format with the target password + let mock_response = "1E4C9B93F3F0682250B6CF8331B7EE68FD8:6\r\n\ + 0018A45C4D1DEF81644B54AB7F969B88D65:3\r\n\ + 00D4F6E8FA6EECAD2A3AA415EEC418D38EC:2\r\n"; + + let target_suffix = "1E4C9B93F3F0682250B6CF8331B7EE68FD8"; + + let count = CipherRiskClient::parse_hibp_response(mock_response, target_suffix); + + assert_eq!(count, 6); + } + + #[test] + fn test_parse_hibp_response_not_found() { + // Simulate HIBP API response without target hash + let mock_response = "0018A45C4D1DEF81644B54AB7F969B88D65:3\r\n\ + 00D4F6E8FA6EECAD2A3AA415EEC418D38EC:2\r\n\ + 011053FD0102E94D6AE2F8B83D76FAF94F6:1\r\n"; + + let target_suffix = "NOTFOUNDNOTFOUNDNOTFOUNDNOTFOUND"; + + let count = CipherRiskClient::parse_hibp_response(mock_response, target_suffix); + + assert_eq!(count, 0); + } + + #[test] + fn test_parse_hibp_response_case_insensitive() { + // HIBP API returns uppercase hashes, but we should match case-insensitively + let mock_response = "1E4C9B93F3F0682250B6CF8331B7EE68FD8:12345\r\n"; + + // Test with lowercase suffix + let target_suffix_lower = "1e4c9b93f3f0682250b6cf8331b7ee68fd8"; + + let count = CipherRiskClient::parse_hibp_response(mock_response, target_suffix_lower); + + assert_eq!(count, 12345); + } + + #[test] + fn test_parse_hibp_response_multiple_matches() { + // Response with multiple hashes, target is in the middle + let mock_response = "AAA111:100\r\n\ + BBB222:200\r\n\ + CCC333:300\r\n\ + DDD444:400\r\n"; + + let count = CipherRiskClient::parse_hibp_response(mock_response, "CCC333"); + assert_eq!(count, 300); + } + + #[test] + fn test_parse_hibp_response_empty() { + // Empty response + let mock_response = ""; + + let count = CipherRiskClient::parse_hibp_response(mock_response, "ANYTHING"); + assert_eq!(count, 0); + } + + #[test] + fn test_parse_hibp_response_malformed_count() { + // Response with invalid count (should return 0 on parse failure) + let mock_response = "AAA111:not_a_number\r\n"; + + let count = CipherRiskClient::parse_hibp_response(mock_response, "AAA111"); + assert_eq!(count, 0); + } + + // Wiremock tests for actual HIBP API integration + #[tokio::test] + async fn test_hibp_api_password_found() { + use wiremock::{ + Mock, MockServer, ResponseTemplate, + matchers::{method, path}, + }; + + let server = MockServer::start().await; + + // Mock HIBP API response for "password" (hash: 5BAA61E4C9B93F3F0682250B6CF8331B7EE68FD8) + Mock::given(method("GET")) + .and(path("/range/5BAA6")) + .respond_with(ResponseTemplate::new(200).set_body_string( + "1E4C9B93F3F0682250B6CF8331B7EE68FD8:3861493\r\n\ + 0018A45C4D1DEF81644B54AB7F969B88D65:3\r\n\ + 00D4F6E8FA6EECAD2A3AA415EEC418D38EC:2\r\n", + )) + .mount(&server) + .await; + + let client = Client::init_test_account(test_bitwarden_com_account()).await; + let risk_client = CipherRiskClient { client }; + let result = risk_client + .check_password_exposed_hibp("password", &server.uri()) + .await + .unwrap(); + + assert_eq!(result, 3861493); + } + + #[tokio::test] + async fn test_hibp_api_password_not_found() { + use wiremock::{ + Mock, MockServer, ResponseTemplate, + matchers::{method, path}, + }; + + let server = MockServer::start().await; + + // Mock HIBP API response that doesn't contain our password + Mock::given(method("GET")) + .and(path("/range/A94A8")) + .respond_with(ResponseTemplate::new(200).set_body_string( + "0018A45C4D1DEF81644B54AB7F969B88D65:3\r\n\ + 00D4F6E8FA6EECAD2A3AA415EEC418D38EC:2\r\n\ + 011053FD0102E94D6AE2F8B83D76FAF94F6:1\r\n", + )) + .mount(&server) + .await; + + let client = Client::init_test_account(test_bitwarden_com_account()).await; + let risk_client = CipherRiskClient { client }; + // "test" hashes to A94A8FE5CCB19BA61C4C0873D391E987982FBBD3 + let result = risk_client + .check_password_exposed_hibp("test", &server.uri()) + .await + .unwrap(); + + assert_eq!(result, 0); + } + + #[tokio::test] + async fn test_hibp_api_network_error() { + use wiremock::{ + Mock, MockServer, ResponseTemplate, + matchers::{method, path}, + }; + + let server = MockServer::start().await; + + // Mock network error (500 status) + Mock::given(method("GET")) + .and(path("/range/5BAA6")) + .respond_with(ResponseTemplate::new(500)) + .mount(&server) + .await; + + let client = Client::init_test_account(test_bitwarden_com_account()).await; + let risk_client = CipherRiskClient { client }; + let result = risk_client + .check_password_exposed_hibp("password", &server.uri()) + .await; + + assert!(result.is_err()); + assert!(matches!(result.unwrap_err(), CipherRiskError::Network(_))); + } + + #[tokio::test] + async fn test_compute_risk_integration() { + // Integration test verifying the full compute_risk flow + // This tests compute_risk without HIBP (check_exposed=false) to avoid + // network calls and test stability issues + let client = Client::init_test_account(test_bitwarden_com_account()).await; + let risk_client = CipherRiskClient { + client: client.clone(), + }; + + let login_details = vec![ + CipherLoginDetails { + id: None, + password: "weak".to_string(), + username: Some("user1".to_string()), + }, + CipherLoginDetails { + id: None, + password: "xK9#mP$2qL@7vN&4wR".to_string(), + username: Some("user2".to_string()), + }, + ]; + + let password_map = risk_client + .password_reuse_map(login_details.clone()) + .unwrap(); + + let options = CipherRiskOptions { + password_map: Some(password_map), + check_exposed: false, + }; + + let results = risk_client + .compute_risk(login_details, options) + .await + .unwrap(); + + assert_eq!(results.len(), 2); + + // Weak password should have low strength + assert!( + results[0].password_strength <= 1, + "Expected weak password strength, got {}", + results[0].password_strength + ); + + // Strong password should have high strength + assert!( + results[1].password_strength >= 3, + "Expected strong password strength, got {}", + results[1].password_strength + ); + + // Both passwords used once + assert_eq!(results[0].reuse_count, 1); + assert_eq!(results[1].reuse_count, 1); + + // HIBP not checked + assert!(results[0].exposed_count.is_none()); + assert!(results[1].exposed_count.is_none()); + } +} diff --git a/crates/bitwarden-vault/src/cipher/mod.rs b/crates/bitwarden-vault/src/cipher/mod.rs index 39fe85361..1db307059 100644 --- a/crates/bitwarden-vault/src/cipher/mod.rs +++ b/crates/bitwarden-vault/src/cipher/mod.rs @@ -5,6 +5,8 @@ pub(crate) mod card; pub(crate) mod cipher; pub(crate) mod cipher_client; pub(crate) mod cipher_permissions; +pub(crate) mod cipher_risk; +pub(crate) mod cipher_risk_client; pub(crate) mod field; pub(crate) mod identity; pub(crate) mod linked_id; @@ -23,6 +25,8 @@ pub use cipher::{ CipherType, CipherView, DecryptCipherListResult, EncryptionContext, }; pub use cipher_client::CiphersClient; +pub use cipher_risk::{CipherLoginDetails, CipherRisk, CipherRiskOptions, PasswordReuseMap}; +pub use cipher_risk_client::CipherRiskClient; pub use field::{FieldType, FieldView}; pub use identity::IdentityView; pub use login::{ diff --git a/crates/bitwarden-vault/src/error.rs b/crates/bitwarden-vault/src/error.rs index b713296ef..a51e27e7d 100644 --- a/crates/bitwarden-vault/src/error.rs +++ b/crates/bitwarden-vault/src/error.rs @@ -31,3 +31,14 @@ pub enum VaultParseError { #[error(transparent)] MissingField(#[from] bitwarden_core::MissingFieldError), } + +/// Error type for cipher risk evaluation operations +#[allow(missing_docs)] +#[bitwarden_error(flat)] +#[derive(Debug, Error)] +pub enum CipherRiskError { + #[error("Network error while checking password exposure: {0}")] + Network(String), + #[error("Invalid password format")] + InvalidPassword, +} diff --git a/crates/bitwarden-vault/src/lib.rs b/crates/bitwarden-vault/src/lib.rs index 61d5b099f..f42747e89 100644 --- a/crates/bitwarden-vault/src/lib.rs +++ b/crates/bitwarden-vault/src/lib.rs @@ -20,7 +20,7 @@ pub use totp::{ Totp, TotpAlgorithm, TotpError, TotpResponse, generate_totp, generate_totp_cipher_view, }; mod error; -pub use error::{DecryptError, EncryptError, VaultParseError}; +pub use error::{CipherRiskError, DecryptError, EncryptError, VaultParseError}; mod vault_client; pub use vault_client::{VaultClient, VaultClientExt}; diff --git a/crates/bitwarden-vault/src/vault_client.rs b/crates/bitwarden-vault/src/vault_client.rs index 2fd6a513e..e671a7ba2 100644 --- a/crates/bitwarden-vault/src/vault_client.rs +++ b/crates/bitwarden-vault/src/vault_client.rs @@ -3,8 +3,8 @@ use bitwarden_core::Client; use wasm_bindgen::prelude::*; use crate::{ - AttachmentsClient, CiphersClient, FoldersClient, PasswordHistoryClient, SyncRequest, - SyncResponse, TotpClient, + AttachmentsClient, CipherRiskClient, CiphersClient, FoldersClient, PasswordHistoryClient, + SyncRequest, SyncResponse, TotpClient, collection_client::CollectionsClient, sync::{SyncError, sync}, }; @@ -70,6 +70,13 @@ impl VaultClient { client: self.client.clone(), } } + + /// Cipher risk evaluation operations. + pub fn cipher_risk(&self) -> CipherRiskClient { + CipherRiskClient { + client: self.client.clone(), + } + } } #[allow(missing_docs)] From 12f17c0775e67bbf6bd4ed9ae8ca04a0a6f21230 Mon Sep 17 00:00:00 2001 From: Shane Date: Mon, 13 Oct 2025 12:22:45 -0700 Subject: [PATCH 02/12] [PM-24468] Add concurrency support for compute_risk in CipherRiskClient Cleanup docs --- Cargo.lock | 1 + crates/bitwarden-vault/Cargo.toml | 1 + .../bitwarden-vault/src/cipher/cipher_risk.rs | 44 ++- .../src/cipher/cipher_risk_client.rs | 366 +++++++++++------- crates/bitwarden-vault/src/error.rs | 6 +- 5 files changed, 256 insertions(+), 162 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a99371332..564915aae 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -922,6 +922,7 @@ dependencies = [ "bitwarden-uuid", "chrono", "data-encoding", + "futures", "hmac", "percent-encoding", "reqwest", diff --git a/crates/bitwarden-vault/Cargo.toml b/crates/bitwarden-vault/Cargo.toml index bc03992a0..fe4ce9457 100644 --- a/crates/bitwarden-vault/Cargo.toml +++ b/crates/bitwarden-vault/Cargo.toml @@ -56,6 +56,7 @@ uuid = { workspace = true } wasm-bindgen = { workspace = true, optional = true } wasm-bindgen-futures = { workspace = true, optional = true } zxcvbn = ">=3.0.1, <4.0" +futures = "0.3" [dev-dependencies] bitwarden-api-api = { workspace = true, features = ["mockall"] } diff --git a/crates/bitwarden-vault/src/cipher/cipher_risk.rs b/crates/bitwarden-vault/src/cipher/cipher_risk.rs index 73ab01ee6..0b2fe2370 100644 --- a/crates/bitwarden-vault/src/cipher/cipher_risk.rs +++ b/crates/bitwarden-vault/src/cipher/cipher_risk.rs @@ -6,60 +6,62 @@ use {tsify::Tsify, wasm_bindgen::prelude::*}; use crate::CipherId; -/// Minimal login cipher data needed for risk evaluation +/// Login cipher data needed for risk evaluation. #[derive(Serialize, Deserialize, Debug, Clone)] #[cfg_attr(feature = "uniffi", derive(uniffi::Record))] #[cfg_attr(feature = "wasm", derive(Tsify), tsify(into_wasm_abi, from_wasm_abi))] pub struct CipherLoginDetails { - /// Cipher ID to identify which cipher in results + /// Cipher ID to identify which cipher in results. pub id: Option, - /// The decrypted password to evaluate + /// The decrypted password to evaluate. pub password: String, - /// Username or email (login ciphers only have one field) + /// Username or email (login ciphers only have one field). pub username: Option, } -/// Password reuse map wrapper for WASM compatibility +/// Password reuse map wrapper for WASM compatibility. #[derive(Serialize, Deserialize, Debug, Clone)] #[cfg_attr(feature = "uniffi", derive(uniffi::Record))] #[cfg_attr(feature = "wasm", derive(Tsify), tsify(into_wasm_abi, from_wasm_abi))] #[serde(transparent)] pub struct PasswordReuseMap { - /// Map of passwords to their occurrence count + /// Map of passwords to their occurrence count. #[cfg_attr(feature = "wasm", tsify(type = "Record"))] pub map: HashMap, } -/// Options for configuring risk computation -#[derive(Serialize, Deserialize, Debug, Clone)] +/// Options for configuring risk computation. +#[derive(Serialize, Deserialize, Debug, Clone, Default)] #[cfg_attr(feature = "uniffi", derive(uniffi::Record))] #[cfg_attr(feature = "wasm", derive(Tsify), tsify(into_wasm_abi, from_wasm_abi))] #[serde(rename_all = "camelCase")] -#[derive(Default)] pub struct CipherRiskOptions { - /// Pre-computed password reuse map (password → count) - /// If provided, enables reuse detection across ciphers + /// Pre-computed password reuse map (password → count). + /// If provided, enables reuse detection across ciphers. pub password_map: Option, - /// Whether to check passwords against Have I Been Pwned API - /// When true, makes network requests to check for exposed passwords + /// Whether to check passwords against Have I Been Pwned API. + /// When true, makes network requests to check for exposed passwords. pub check_exposed: bool, + /// Optional HIBP API base URL override. When None, uses the production HIBP URL. + /// Can be used for testing or alternative password breach checking services. + pub hibp_base_url: Option, } -/// Risk evaluation result for a single cipher +/// Risk evaluation result for a single cipher. #[derive(Serialize, Deserialize, Debug, Clone, PartialEq)] #[cfg_attr(feature = "uniffi", derive(uniffi::Record))] #[cfg_attr(feature = "wasm", derive(Tsify), tsify(into_wasm_abi, from_wasm_abi))] pub struct CipherRisk { - /// Cipher ID matching the input CipherLoginDetails + /// Cipher ID matching the input CipherLoginDetails. pub id: Option, - /// Password strength score from 0 (weakest) to 4 (strongest) - /// Calculated using zxcvbn with cipher-specific context + /// Password strength score from 0 (weakest) to 4 (strongest). + /// Calculated using zxcvbn with cipher-specific context. pub password_strength: u8, - /// Number of times password appears in HIBP database - /// None if check_exposed was false in options + /// Number of times password appears in HIBP database. + /// None if check_exposed was false in options. pub exposed_count: Option, - /// Number of times this password appears in the provided cipher list - /// Minimum value is 1 (the cipher itself) + /// Number of times this password appears in the provided cipher list. + /// Minimum value is 1 (the cipher itself). pub reuse_count: u32, } diff --git a/crates/bitwarden-vault/src/cipher/cipher_risk_client.rs b/crates/bitwarden-vault/src/cipher/cipher_risk_client.rs index d81d6ca4f..c6231e6b7 100644 --- a/crates/bitwarden-vault/src/cipher/cipher_risk_client.rs +++ b/crates/bitwarden-vault/src/cipher/cipher_risk_client.rs @@ -1,6 +1,7 @@ use std::collections::HashMap; use bitwarden_core::Client; +use futures::{StreamExt, TryStreamExt, stream}; #[cfg(feature = "wasm")] use wasm_bindgen::prelude::wasm_bindgen; @@ -20,9 +21,6 @@ impl CipherRiskClient { /// Returns a map where keys are passwords and values are the number of times /// each password appears in the provided list. This map can be passed to `compute_risk()` /// to enable password reuse detection. - /// - /// # Returns - /// PasswordReuseMap containing password occurrence counts pub fn password_reuse_map( &self, login_details: Vec, @@ -36,64 +34,81 @@ impl CipherRiskClient { Ok(PasswordReuseMap { map }) } - /// Evaluate security risks for multiple login ciphers. + /// Evaluate security risks for multiple login ciphers concurrently. /// - /// For each cipher, this method: + /// For each cipher: /// 1. Calculates password strength (0-4) using zxcvbn with cipher-specific context /// 2. Optionally checks if the password has been exposed via Have I Been Pwned API /// 3. Counts how many times the password is reused across the provided ciphers /// - /// # Returns - /// Vector of `CipherRisk` results, one for each input cipher + /// Returns a vector of `CipherRisk` results, one for each input cipher. /// /// # Errors - /// Returns `CipherRiskError::Network` if HIBP API requests fail when `check_exposed` is enabled + /// + /// Returns `CipherRiskError::Reqwest` if HIBP API requests fail when `check_exposed` is + /// enabled. Network errors include timeouts, connection failures, HTTP errors, or rate + /// limiting. On error, the entire operation fails - no partial results are returned. pub async fn compute_risk( &self, login_details: Vec, options: CipherRiskOptions, ) -> Result, CipherRiskError> { - let mut results = Vec::with_capacity(login_details.len()); + // Create futures that can run concurrently + let futures = login_details.into_iter().map(|details| { + let http_client = self.client.internal.get_http_client().clone(); + let password_map = options.password_map.clone(); + let base_url = options + .hibp_base_url + .clone() + .unwrap_or_else(|| "https://api.pwnedpasswords.com".to_string()); + + async move { + let password_strength = Self::calculate_password_strength( + &details.password, + details.username.as_deref(), + ); + + // Check exposure via HIBP API if enabled + // Network errors now propagate up instead of being silently ignored + let exposed_count = if options.check_exposed { + Some( + Self::check_password_exposed(&http_client, &details.password, &base_url) + .await?, + ) + } else { + None + }; + + // Check reuse from provided map (default to 1 if not in map) + let reuse_count = password_map + .as_ref() + .and_then(|reuse_map| reuse_map.map.get(&details.password)) + .copied() + .unwrap_or(1); + + Ok::(CipherRisk { + id: details.id, + password_strength, + exposed_count, + reuse_count, + }) + } + }); - for details in login_details { - // Calculate password strength using cipher-specific inputs - let password_strength = - self.calculate_password_strength(&details.password, details.username.as_deref()); - - // Check exposure via HIBP API if enabled - let exposed_count = if options.check_exposed { - Some(self.check_password_exposed(&details.password).await?) - } else { - None - }; - - // Check reuse from provided map (default to 1 if not in map) - let reuse_count = options - .password_map - .as_ref() - .and_then(|reuse_map| reuse_map.map.get(&details.password)) - .copied() - .unwrap_or(1); - - results.push(CipherRisk { - id: details.id, - password_strength, - exposed_count, - reuse_count, - }); - } + // Process up to 100 futures concurrently, fail fast on first error + let results: Vec = stream::iter(futures) + .buffer_unordered(100) + .try_collect() + .await?; Ok(results) } /// Calculate password strength with cipher-specific context. /// - /// Uses zxcvbn to score password strength (0-4) and penalizes passwords - /// that contain parts of the username/email. - /// - /// # Returns - /// Score from 0 (weakest) to 4 (strongest) - fn calculate_password_strength(&self, password: &str, username: Option<&str>) -> u8 { + /// Uses zxcvbn to score password strength from 0 (weakest) to 4 (strongest). + /// Penalizes passwords that contain parts of the username/email. + fn calculate_password_strength(password: &str, username: Option<&str>) -> u8 { let mut user_inputs = Vec::new(); // Extract meaningful parts from username field @@ -108,14 +123,10 @@ impl CipherRiskClient { /// Extract meaningful tokens from username/email for password penalization. /// - /// Handles both email addresses and plain usernames by: + /// Handles both email addresses and plain usernames: /// - For emails: extracts and tokenizes the local part (before @) /// - For usernames: tokenizes the entire string - /// - Splits on non-alphanumeric characters - /// - Converts to lowercase for case-insensitive matching - /// - /// # Returns - /// Vector of lowercase tokens extracted from the input + /// - Splits on non-alphanumeric characters and converts to lowercase fn extract_user_inputs(username: &str) -> Vec { // Check if it's email-like (contains @) if let Some((local_part, _domain)) = username.split_once('@') { @@ -139,32 +150,9 @@ impl CipherRiskClient { } } - /// Check if a password has been exposed using the Have I Been Pwned API. - /// - /// Implements k-anonymity model: - /// 1. Hash password with SHA-1 - /// 2. Send only first 5 characters of hash to HIBP API - /// 3. API returns all hash suffixes matching that prefix - /// 4. Check locally if full hash exists in results - /// - /// This ensures the actual password never leaves the client. - /// - /// # Returns - /// Number of times the password appears in HIBP database (0 if not found) - /// - /// # Errors - /// Returns `CipherRiskError::Network` if API request fails - async fn check_password_exposed(&self, password: &str) -> Result { - const HIBP_BASE_URL: &str = "https://api.pwnedpasswords.com"; - - self.check_password_exposed_hibp(password, HIBP_BASE_URL) - .await - } - /// Hash password with SHA-1 and split into prefix/suffix for k-anonymity. /// - /// # Returns - /// Tuple of (prefix: first 5 chars, suffix: remaining chars) + /// Returns a tuple of (prefix: first 5 chars, suffix: remaining chars). fn hash_password_for_hibp(password: &str) -> (String, String) { use sha1::{Digest, Sha1}; @@ -177,10 +165,8 @@ impl CipherRiskClient { /// Parse HIBP API response to find password hash and return breach count. /// /// Response format: "SUFFIX:COUNT\r\n..." (e.g., - /// "0018A45C4D1DEF81644B54AB7F969B88D65:3\r\n...") - /// - /// # Returns - /// Number of times the password appears in breaches (0 if not found) + /// "0018A45C4D1DEF81644B54AB7F969B88D65:3\r\n..."). + /// Returns the number of times the password appears in breaches (0 if not found). fn parse_hibp_response(response: &str, target_suffix: &str) -> u32 { for line in response.lines() { if let Some((hash_suffix, count_str)) = line.split_once(':') { @@ -192,47 +178,32 @@ impl CipherRiskClient { 0 } - /// Check if a password has been exposed using the Have I Been Pwned API. + /// Check password exposure via HIBP API using k-anonymity model. /// - /// Implements k-anonymity model to ensure privacy: + /// Implements k-anonymity to ensure privacy: /// 1. Hash password with SHA-1 /// 2. Send only first 5 characters of hash to HIBP API /// 3. API returns all hash suffixes matching that prefix /// 4. Check locally if full hash exists in results /// /// This ensures the actual password never leaves the client. - /// - /// # Arguments - /// * `password` - Password to check for exposure - /// * `base_url` - HIBP API base URL (for testing, can inject mock server URL) - /// - /// # Returns - /// Number of times the password appears in HIBP database (0 if not found) - /// - /// # Errors - /// Returns `CipherRiskError::Network` if API request fails - async fn check_password_exposed_hibp( - &self, + /// Returns the number of times the password appears in HIBP database (0 if not found). + async fn check_password_exposed( + http_client: &reqwest::Client, password: &str, - base_url: &str, + hibp_base_url: &str, ) -> Result { let (prefix, suffix) = Self::hash_password_for_hibp(password); // Query HIBP API with prefix only (k-anonymity) - let url = format!("{}/range/{}", base_url, prefix); - let response = self - .client - .internal - .get_http_client() + let url = format!("{}/range/{}", hibp_base_url, prefix); + let response = http_client .get(&url) .send() - .await - .map_err(|e| CipherRiskError::Network(e.to_string()))? - .error_for_status() - .map_err(|e| CipherRiskError::Network(e.to_string()))? + .await? + .error_for_status()? .text() - .await - .map_err(|e| CipherRiskError::Network(e.to_string()))?; + .await?; Ok(Self::parse_hibp_response(&response, &suffix)) } @@ -301,38 +272,23 @@ mod tests { #[tokio::test] async fn test_calculate_password_strength_weak() { - let client = Client::init_test_account(test_bitwarden_com_account()).await; - let risk_client = CipherRiskClient { - client: client.clone(), - }; - - let strength = risk_client.calculate_password_strength("password", None); + let strength = CipherRiskClient::calculate_password_strength("password", None); assert!(strength <= 1, "Expected weak password, got {}", strength); } #[tokio::test] async fn test_calculate_password_strength_strong() { - let client = Client::init_test_account(test_bitwarden_com_account()).await; - let risk_client = CipherRiskClient { - client: client.clone(), - }; - - let strength = risk_client.calculate_password_strength("xK9#mP$2qL@7vN&4wR", None); + let strength = CipherRiskClient::calculate_password_strength("xK9#mP$2qL@7vN&4wR", None); assert!(strength >= 3, "Expected strong password, got {}", strength); } #[tokio::test] async fn test_calculate_password_strength_penalizes_username() { - let client = Client::init_test_account(test_bitwarden_com_account()).await; - let risk_client = CipherRiskClient { - client: client.clone(), - }; - // Password containing username should be weaker let strength_with_username = - risk_client.calculate_password_strength("johndoe123!", Some("johndoe")); + CipherRiskClient::calculate_password_strength("johndoe123!", Some("johndoe")); let strength_without_username = - risk_client.calculate_password_strength("johndoe123!", None); + CipherRiskClient::calculate_password_strength("johndoe123!", None); assert!( strength_with_username <= strength_without_username, @@ -367,6 +323,7 @@ mod tests { let options = CipherRiskOptions { password_map: Some(password_map), check_exposed: false, + hibp_base_url: None, }; let risks = risk_client @@ -514,12 +471,13 @@ mod tests { .mount(&server) .await; - let client = Client::init_test_account(test_bitwarden_com_account()).await; - let risk_client = CipherRiskClient { client }; - let result = risk_client - .check_password_exposed_hibp("password", &server.uri()) - .await - .unwrap(); + let result = CipherRiskClient::check_password_exposed( + &reqwest::Client::new(), + "password", + &server.uri(), + ) + .await + .unwrap(); assert_eq!(result, 3861493); } @@ -544,13 +502,14 @@ mod tests { .mount(&server) .await; - let client = Client::init_test_account(test_bitwarden_com_account()).await; - let risk_client = CipherRiskClient { client }; // "test" hashes to A94A8FE5CCB19BA61C4C0873D391E987982FBBD3 - let result = risk_client - .check_password_exposed_hibp("test", &server.uri()) - .await - .unwrap(); + let result = CipherRiskClient::check_password_exposed( + &reqwest::Client::new(), + "test", + &server.uri(), + ) + .await + .unwrap(); assert_eq!(result, 0); } @@ -571,14 +530,60 @@ mod tests { .mount(&server) .await; + let result = CipherRiskClient::check_password_exposed( + &reqwest::Client::new(), + "password", + &server.uri(), + ) + .await; + + assert!(result.is_err()); + assert!(matches!(result.unwrap_err(), CipherRiskError::Reqwest(_))); + } + + #[tokio::test] + async fn test_compute_risk_propagates_network_errors() { + // Test that network errors from HIBP API are properly propagated + // instead of being silently swallowed + use wiremock::{ + Mock, MockServer, ResponseTemplate, + matchers::{method, path_regex}, + }; + + let server = MockServer::start().await; + + // Mock network error (500 status) for all HIBP range requests + Mock::given(method("GET")) + .and(path_regex(r"^/range/[A-F0-9]{5}$")) + .respond_with(ResponseTemplate::new(500)) + .mount(&server) + .await; + let client = Client::init_test_account(test_bitwarden_com_account()).await; let risk_client = CipherRiskClient { client }; - let result = risk_client - .check_password_exposed_hibp("password", &server.uri()) - .await; + let login_details = vec![CipherLoginDetails { + id: None, + password: "password123".to_string(), + username: Some("user1".to_string()), + }]; + + let options = CipherRiskOptions { + password_map: None, + check_exposed: true, // Enable HIBP checking + hibp_base_url: Some(server.uri()), + }; + + let result = risk_client.compute_risk(login_details, options).await; + + // Verify error is propagated, not swallowed assert!(result.is_err()); - assert!(matches!(result.unwrap_err(), CipherRiskError::Network(_))); + let err = result.unwrap_err(); + assert!( + matches!(err, CipherRiskError::Reqwest(_)), + "Expected CipherRiskError::Reqwest, got {:?}", + err + ); } #[tokio::test] @@ -611,6 +616,7 @@ mod tests { let options = CipherRiskOptions { password_map: Some(password_map), check_exposed: false, + hibp_base_url: None, }; let results = risk_client @@ -642,4 +648,90 @@ mod tests { assert!(results[0].exposed_count.is_none()); assert!(results[1].exposed_count.is_none()); } + + #[tokio::test] + async fn test_compute_risk_concurrent_requests() { + // This test verifies that compute_risk truly executes requests concurrently + // by tracking request timestamps. If concurrent, multiple requests arrive + // within a short time window. If sequential, requests are spaced out. + use std::{ + sync::{Arc, Mutex}, + time::{Duration, Instant}, + }; + + use wiremock::{ + Mock, MockServer, ResponseTemplate, + matchers::{method, path_regex}, + }; + + let server = MockServer::start().await; + + // Track when each request arrives + let request_times = Arc::new(Mutex::new(Vec::new())); + + // Mock HIBP API that records request times + Mock::given(method("GET")) + .and(path_regex(r"^/range/[A-F0-9]{5}$")) + .respond_with({ + let request_times = request_times.clone(); + move |_req: &wiremock::Request| { + // Record the time this request arrived + request_times.lock().unwrap().push(Instant::now()); + + ResponseTemplate::new(200) + .set_body_string("AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA:1\r\n") + .set_delay(Duration::from_millis(10)) + } + }) + .mount(&server) + .await; + + let client = Client::init_test_account(test_bitwarden_com_account()).await; + let risk_client = CipherRiskClient { client }; + + // Create 5 different passwords to ensure different hash prefixes + // This forces 5 separate API calls + let login_details: Vec = (0..5) + .map(|i| CipherLoginDetails { + id: None, + password: format!("password{}", i), + username: Some(format!("user{}", i)), + }) + .collect(); + + let options = CipherRiskOptions { + password_map: None, + check_exposed: true, // Enable HIBP checking to test concurrency + hibp_base_url: Some(server.uri()), // Use mock server URL + }; + + let results = risk_client + .compute_risk(login_details, options) + .await + .unwrap(); + + // Verify all results were returned + assert_eq!(results.len(), 5); + + // Verify all passwords were checked + for result in &results { + assert!(result.exposed_count.is_some()); + } + + // Prove concurrency by analyzing request arrival times + // If truly concurrent, all 5 requests should arrive within a very short window (< 5ms + // window) If sequential with 10ms delays, they'd be spread over 40-50ms + let times = request_times.lock().unwrap(); + let first = times[0]; + let last = times[times.len() - 1]; + let time_span = last.duration_since(first); + + assert!( + time_span < Duration::from_millis(5), + "Expected concurrent execution (all requests within 5ms), \ + but requests were spread over {}ms. This suggests requests \ + are being made sequentially instead of concurrently.", + time_span.as_millis() + ); + } } diff --git a/crates/bitwarden-vault/src/error.rs b/crates/bitwarden-vault/src/error.rs index a51e27e7d..acf123150 100644 --- a/crates/bitwarden-vault/src/error.rs +++ b/crates/bitwarden-vault/src/error.rs @@ -37,8 +37,6 @@ pub enum VaultParseError { #[bitwarden_error(flat)] #[derive(Debug, Error)] pub enum CipherRiskError { - #[error("Network error while checking password exposure: {0}")] - Network(String), - #[error("Invalid password format")] - InvalidPassword, + #[error(transparent)] + Reqwest(#[from] reqwest::Error), } From cffd1df66cd011560752521a141cd203d6256daf Mon Sep 17 00:00:00 2001 From: Shane Date: Mon, 13 Oct 2025 15:55:41 -0700 Subject: [PATCH 03/12] [PM-24468] Use Arc for password_map to avoid expensive clones Remove performance test --- crates/bitwarden-vault/src/cipher/cipher_risk_client.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/crates/bitwarden-vault/src/cipher/cipher_risk_client.rs b/crates/bitwarden-vault/src/cipher/cipher_risk_client.rs index c6231e6b7..d8186f7ec 100644 --- a/crates/bitwarden-vault/src/cipher/cipher_risk_client.rs +++ b/crates/bitwarden-vault/src/cipher/cipher_risk_client.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use std::{collections::HashMap, sync::Arc}; use bitwarden_core::Client; use futures::{StreamExt, TryStreamExt, stream}; @@ -53,10 +53,13 @@ impl CipherRiskClient { login_details: Vec, options: CipherRiskOptions, ) -> Result, CipherRiskError> { + // Wrap password_map in Arc to avoid cloning the HashMap for each future + let password_map = options.password_map.map(Arc::new); + // Create futures that can run concurrently let futures = login_details.into_iter().map(|details| { let http_client = self.client.internal.get_http_client().clone(); - let password_map = options.password_map.clone(); + let password_map = password_map.clone(); let base_url = options .hibp_base_url .clone() @@ -361,7 +364,7 @@ mod tests { let password_map = risk_client.password_reuse_map(login_details).unwrap(); // Empty passwords should not be in the map - assert!(password_map.map.get("").is_none()); + assert!(!password_map.map.contains_key("")); assert_eq!(password_map.map.get("valid_password"), Some(&1)); } From d803671353624da5d66a207b18158b88350f22ca Mon Sep 17 00:00:00 2001 From: Shane Date: Mon, 13 Oct 2025 16:43:35 -0700 Subject: [PATCH 04/12] [PM-24468] Fix magic values --- .../bitwarden-vault/src/cipher/cipher_risk_client.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/crates/bitwarden-vault/src/cipher/cipher_risk_client.rs b/crates/bitwarden-vault/src/cipher/cipher_risk_client.rs index d8186f7ec..21a843c04 100644 --- a/crates/bitwarden-vault/src/cipher/cipher_risk_client.rs +++ b/crates/bitwarden-vault/src/cipher/cipher_risk_client.rs @@ -8,6 +8,12 @@ use wasm_bindgen::prelude::wasm_bindgen; use super::cipher_risk::{CipherLoginDetails, CipherRisk, CipherRiskOptions, PasswordReuseMap}; use crate::CipherRiskError; +/// Default base URL for the Have I Been Pwned (HIBP) Pwned Passwords API. +const HIBP_DEFAULT_BASE_URL: &str = "https://api.pwnedpasswords.com"; + +/// Maximum number of concurrent requests when checking passwords. +const MAX_CONCURRENT_REQUESTS: usize = 100; + /// Client for evaluating credential risk for login ciphers. #[cfg_attr(feature = "wasm", wasm_bindgen)] pub struct CipherRiskClient { @@ -63,7 +69,7 @@ impl CipherRiskClient { let base_url = options .hibp_base_url .clone() - .unwrap_or_else(|| "https://api.pwnedpasswords.com".to_string()); + .unwrap_or_else(|| HIBP_DEFAULT_BASE_URL.to_string()); async move { let password_strength = Self::calculate_password_strength( @@ -98,9 +104,9 @@ impl CipherRiskClient { } }); - // Process up to 100 futures concurrently, fail fast on first error + // Process up to MAX_CONCURRENT_REQUESTS futures concurrently, fail fast on first error let results: Vec = stream::iter(futures) - .buffer_unordered(100) + .buffer_unordered(MAX_CONCURRENT_REQUESTS) .try_collect() .await?; From b327c2437233b3b4f18bad767bccf6d29ff51052 Mon Sep 17 00:00:00 2001 From: Shane Date: Mon, 13 Oct 2025 17:09:20 -0700 Subject: [PATCH 05/12] [PM-24468] Remove redundant tests --- .../src/cipher/cipher_risk_client.rs | 129 ------------------ 1 file changed, 129 deletions(-) diff --git a/crates/bitwarden-vault/src/cipher/cipher_risk_client.rs b/crates/bitwarden-vault/src/cipher/cipher_risk_client.rs index 21a843c04..0124cdd94 100644 --- a/crates/bitwarden-vault/src/cipher/cipher_risk_client.rs +++ b/crates/bitwarden-vault/src/cipher/cipher_risk_client.rs @@ -279,18 +279,6 @@ mod tests { assert_eq!(password_map.map.get("unique_password"), Some(&1)); } - #[tokio::test] - async fn test_calculate_password_strength_weak() { - let strength = CipherRiskClient::calculate_password_strength("password", None); - assert!(strength <= 1, "Expected weak password, got {}", strength); - } - - #[tokio::test] - async fn test_calculate_password_strength_strong() { - let strength = CipherRiskClient::calculate_password_strength("xK9#mP$2qL@7vN&4wR", None); - assert!(strength >= 3, "Expected strong password, got {}", strength); - } - #[tokio::test] async fn test_calculate_password_strength_penalizes_username() { // Password containing username should be weaker @@ -305,48 +293,6 @@ mod tests { ); } - #[tokio::test] - async fn test_compute_risk_without_hibp() { - let client = Client::init_test_account(test_bitwarden_com_account()).await; - let risk_client = CipherRiskClient { - client: client.clone(), - }; - - let login_details = vec![ - CipherLoginDetails { - id: None, - password: "password123".to_string(), - username: Some("user1".to_string()), - }, - CipherLoginDetails { - id: None, - password: "password123".to_string(), - username: Some("user2".to_string()), - }, - ]; - - let password_map = risk_client - .password_reuse_map(login_details.clone()) - .unwrap(); - - let options = CipherRiskOptions { - password_map: Some(password_map), - check_exposed: false, - hibp_base_url: None, - }; - - let risks = risk_client - .compute_risk(login_details, options) - .await - .unwrap(); - - assert_eq!(risks.len(), 2); - assert_eq!(risks[0].reuse_count, 2); - assert_eq!(risks[1].reuse_count, 2); - assert!(risks[0].exposed_count.is_none()); - assert!(risks[1].exposed_count.is_none()); - } - #[tokio::test] async fn test_password_reuse_map_empty_passwords() { let client = Client::init_test_account(test_bitwarden_com_account()).await; @@ -429,18 +375,6 @@ mod tests { assert_eq!(count, 12345); } - #[test] - fn test_parse_hibp_response_multiple_matches() { - // Response with multiple hashes, target is in the middle - let mock_response = "AAA111:100\r\n\ - BBB222:200\r\n\ - CCC333:300\r\n\ - DDD444:400\r\n"; - - let count = CipherRiskClient::parse_hibp_response(mock_response, "CCC333"); - assert_eq!(count, 300); - } - #[test] fn test_parse_hibp_response_empty() { // Empty response @@ -460,69 +394,6 @@ mod tests { } // Wiremock tests for actual HIBP API integration - #[tokio::test] - async fn test_hibp_api_password_found() { - use wiremock::{ - Mock, MockServer, ResponseTemplate, - matchers::{method, path}, - }; - - let server = MockServer::start().await; - - // Mock HIBP API response for "password" (hash: 5BAA61E4C9B93F3F0682250B6CF8331B7EE68FD8) - Mock::given(method("GET")) - .and(path("/range/5BAA6")) - .respond_with(ResponseTemplate::new(200).set_body_string( - "1E4C9B93F3F0682250B6CF8331B7EE68FD8:3861493\r\n\ - 0018A45C4D1DEF81644B54AB7F969B88D65:3\r\n\ - 00D4F6E8FA6EECAD2A3AA415EEC418D38EC:2\r\n", - )) - .mount(&server) - .await; - - let result = CipherRiskClient::check_password_exposed( - &reqwest::Client::new(), - "password", - &server.uri(), - ) - .await - .unwrap(); - - assert_eq!(result, 3861493); - } - - #[tokio::test] - async fn test_hibp_api_password_not_found() { - use wiremock::{ - Mock, MockServer, ResponseTemplate, - matchers::{method, path}, - }; - - let server = MockServer::start().await; - - // Mock HIBP API response that doesn't contain our password - Mock::given(method("GET")) - .and(path("/range/A94A8")) - .respond_with(ResponseTemplate::new(200).set_body_string( - "0018A45C4D1DEF81644B54AB7F969B88D65:3\r\n\ - 00D4F6E8FA6EECAD2A3AA415EEC418D38EC:2\r\n\ - 011053FD0102E94D6AE2F8B83D76FAF94F6:1\r\n", - )) - .mount(&server) - .await; - - // "test" hashes to A94A8FE5CCB19BA61C4C0873D391E987982FBBD3 - let result = CipherRiskClient::check_password_exposed( - &reqwest::Client::new(), - "test", - &server.uri(), - ) - .await - .unwrap(); - - assert_eq!(result, 0); - } - #[tokio::test] async fn test_hibp_api_network_error() { use wiremock::{ From cc28aabb8a7a19b5514be8a206c18286e6300a89 Mon Sep 17 00:00:00 2001 From: Shane Date: Mon, 13 Oct 2025 17:31:04 -0700 Subject: [PATCH 06/12] [PM-24468] Sort Vault Cargo.toml --- crates/bitwarden-vault/Cargo.toml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/bitwarden-vault/Cargo.toml b/crates/bitwarden-vault/Cargo.toml index fe4ce9457..f95cb5d3e 100644 --- a/crates/bitwarden-vault/Cargo.toml +++ b/crates/bitwarden-vault/Cargo.toml @@ -19,7 +19,7 @@ uniffi = [ "bitwarden-collections/uniffi", "bitwarden-core/uniffi", "bitwarden-crypto/uniffi", - "dep:uniffi" + "dep:uniffi", ] # Uniffi bindings wasm = [ "bitwarden-collections/wasm", @@ -27,7 +27,7 @@ wasm = [ "bitwarden-encoding/wasm", "dep:tsify", "dep:wasm-bindgen", - "dep:wasm-bindgen-futures" + "dep:wasm-bindgen-futures", ] # WASM support [dependencies] @@ -41,6 +41,7 @@ bitwarden-state = { workspace = true } bitwarden-uuid = { workspace = true } chrono = { workspace = true } data-encoding = { workspace = true } +futures = "0.3" hmac = ">=0.12.1, <0.13" percent-encoding = ">=2.1, <3.0" reqwest = { workspace = true } @@ -56,7 +57,6 @@ uuid = { workspace = true } wasm-bindgen = { workspace = true, optional = true } wasm-bindgen-futures = { workspace = true, optional = true } zxcvbn = ">=3.0.1, <4.0" -futures = "0.3" [dev-dependencies] bitwarden-api-api = { workspace = true, features = ["mockall"] } From 25ecee3de9fea6619895a91a64f3bbadeac9289e Mon Sep 17 00:00:00 2001 From: Shane Date: Tue, 14 Oct 2025 08:03:32 -0700 Subject: [PATCH 07/12] [PM-24468] Fix cargo.toml formatting --- crates/bitwarden-vault/Cargo.toml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bitwarden-vault/Cargo.toml b/crates/bitwarden-vault/Cargo.toml index f95cb5d3e..513cf62be 100644 --- a/crates/bitwarden-vault/Cargo.toml +++ b/crates/bitwarden-vault/Cargo.toml @@ -19,7 +19,7 @@ uniffi = [ "bitwarden-collections/uniffi", "bitwarden-core/uniffi", "bitwarden-crypto/uniffi", - "dep:uniffi", + "dep:uniffi" ] # Uniffi bindings wasm = [ "bitwarden-collections/wasm", @@ -27,7 +27,7 @@ wasm = [ "bitwarden-encoding/wasm", "dep:tsify", "dep:wasm-bindgen", - "dep:wasm-bindgen-futures", + "dep:wasm-bindgen-futures" ] # WASM support [dependencies] From 5daddda94c1a2624b61bc6fb01236b1e4ec63fc3 Mon Sep 17 00:00:00 2001 From: Shane Date: Tue, 14 Oct 2025 09:01:22 -0700 Subject: [PATCH 08/12] [PM-24468] Better empty password handling --- .../bitwarden-vault/src/cipher/cipher_risk.rs | 2 +- .../src/cipher/cipher_risk_client.rs | 40 ++++++++++++++++++- 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/crates/bitwarden-vault/src/cipher/cipher_risk.rs b/crates/bitwarden-vault/src/cipher/cipher_risk.rs index 0b2fe2370..184f0d9ce 100644 --- a/crates/bitwarden-vault/src/cipher/cipher_risk.rs +++ b/crates/bitwarden-vault/src/cipher/cipher_risk.rs @@ -60,7 +60,7 @@ pub struct CipherRisk { /// Number of times password appears in HIBP database. /// None if check_exposed was false in options. pub exposed_count: Option, - /// Number of times this password appears in the provided cipher list. + /// Number of times this password appears in the provided password_map. /// Minimum value is 1 (the cipher itself). pub reuse_count: u32, } diff --git a/crates/bitwarden-vault/src/cipher/cipher_risk_client.rs b/crates/bitwarden-vault/src/cipher/cipher_risk_client.rs index 0124cdd94..fe1e29ae6 100644 --- a/crates/bitwarden-vault/src/cipher/cipher_risk_client.rs +++ b/crates/bitwarden-vault/src/cipher/cipher_risk_client.rs @@ -45,7 +45,7 @@ impl CipherRiskClient { /// For each cipher: /// 1. Calculates password strength (0-4) using zxcvbn with cipher-specific context /// 2. Optionally checks if the password has been exposed via Have I Been Pwned API - /// 3. Counts how many times the password is reused across the provided ciphers + /// 3. Counts how many times the password is reused in the provided `password_map`. /// /// Returns a vector of `CipherRisk` results, one for each input cipher. /// @@ -72,6 +72,16 @@ impl CipherRiskClient { .unwrap_or_else(|| HIBP_DEFAULT_BASE_URL.to_string()); async move { + if details.password.is_empty() { + // Skip empty passwords, return default risk values + return Ok(CipherRisk { + id: details.id, + password_strength: 0, + exposed_count: None, + reuse_count: 0, + }); + } + let password_strength = Self::calculate_password_strength( &details.password, details.username.as_deref(), @@ -421,6 +431,34 @@ mod tests { assert!(matches!(result.unwrap_err(), CipherRiskError::Reqwest(_))); } + #[tokio::test] + async fn test_compute_risk_skips_empty_passwords() { + let client = Client::init_test_account(test_bitwarden_com_account()).await; + let risk_client = CipherRiskClient { client }; + + let login_details = vec![CipherLoginDetails { + id: None, + password: "".to_string(), + username: Some("user1".to_string()), + }]; + + let options = CipherRiskOptions { + password_map: None, + check_exposed: true, // Enable HIBP checking + hibp_base_url: None, + }; + + let result = risk_client.compute_risk(login_details, options).await; + + // Verify that empty passwords are skipped + assert!(result.is_ok()); + let results = result.unwrap(); + assert_eq!(results.len(), 1); + assert_eq!(results[0].password_strength, 0); + assert_eq!(results[0].exposed_count, None); + assert_eq!(results[0].reuse_count, 0); + } + #[tokio::test] async fn test_compute_risk_propagates_network_errors() { // Test that network errors from HIBP API are properly propagated From 56fe32576b987b2b394a08a013e140d1ce76cb4e Mon Sep 17 00:00:00 2001 From: Shane Date: Tue, 14 Oct 2025 09:21:59 -0700 Subject: [PATCH 09/12] [PM-24468] Strip URL from HIBP errors --- crates/bitwarden-vault/src/cipher/cipher_risk_client.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/crates/bitwarden-vault/src/cipher/cipher_risk_client.rs b/crates/bitwarden-vault/src/cipher/cipher_risk_client.rs index fe1e29ae6..59c4b5944 100644 --- a/crates/bitwarden-vault/src/cipher/cipher_risk_client.rs +++ b/crates/bitwarden-vault/src/cipher/cipher_risk_client.rs @@ -219,10 +219,13 @@ impl CipherRiskClient { let response = http_client .get(&url) .send() - .await? - .error_for_status()? + .await + .map_err(|e| e.without_url())? + .error_for_status() + .map_err(|e| e.without_url())? .text() - .await?; + .await + .map_err(|e| e.without_url())?; Ok(Self::parse_hibp_response(&response, &suffix)) } From c5b7e4005391c2a8ebbb3aac6bba2cceb3a5b5c0 Mon Sep 17 00:00:00 2001 From: Shane Date: Tue, 14 Oct 2025 09:44:05 -0700 Subject: [PATCH 10/12] [PM-24468] Use Option for reuse_count result --- .../bitwarden-vault/src/cipher/cipher_risk.rs | 4 ++-- .../src/cipher/cipher_risk_client.rs | 20 +++++++++---------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/crates/bitwarden-vault/src/cipher/cipher_risk.rs b/crates/bitwarden-vault/src/cipher/cipher_risk.rs index 184f0d9ce..296472a2c 100644 --- a/crates/bitwarden-vault/src/cipher/cipher_risk.rs +++ b/crates/bitwarden-vault/src/cipher/cipher_risk.rs @@ -61,8 +61,8 @@ pub struct CipherRisk { /// None if check_exposed was false in options. pub exposed_count: Option, /// Number of times this password appears in the provided password_map. - /// Minimum value is 1 (the cipher itself). - pub reuse_count: u32, + /// None if not found or if no password_map was provided. + pub reuse_count: Option, } #[cfg(feature = "wasm")] diff --git a/crates/bitwarden-vault/src/cipher/cipher_risk_client.rs b/crates/bitwarden-vault/src/cipher/cipher_risk_client.rs index 59c4b5944..5bdd0eec9 100644 --- a/crates/bitwarden-vault/src/cipher/cipher_risk_client.rs +++ b/crates/bitwarden-vault/src/cipher/cipher_risk_client.rs @@ -78,7 +78,7 @@ impl CipherRiskClient { id: details.id, password_strength: 0, exposed_count: None, - reuse_count: 0, + reuse_count: None, }); } @@ -98,12 +98,12 @@ impl CipherRiskClient { None }; - // Check reuse from provided map (default to 1 if not in map) - let reuse_count = password_map - .as_ref() - .and_then(|reuse_map| reuse_map.map.get(&details.password)) - .copied() - .unwrap_or(1); + // Check reuse from provided map + let reuse_count = if let Some(map) = &password_map { + map.map.get(&details.password).copied() + } else { + None + }; Ok::(CipherRisk { id: details.id, @@ -459,7 +459,7 @@ mod tests { assert_eq!(results.len(), 1); assert_eq!(results[0].password_strength, 0); assert_eq!(results[0].exposed_count, None); - assert_eq!(results[0].reuse_count, 0); + assert_eq!(results[0].reuse_count, None); } #[tokio::test] @@ -562,8 +562,8 @@ mod tests { ); // Both passwords used once - assert_eq!(results[0].reuse_count, 1); - assert_eq!(results[1].reuse_count, 1); + assert_eq!(results[0].reuse_count, Some(1)); + assert_eq!(results[1].reuse_count, Some(1)); // HIBP not checked assert!(results[0].exposed_count.is_none()); From 1956e61aac955e79c5ae12c9e1e1d5134aabc8c7 Mon Sep 17 00:00:00 2001 From: Shane Date: Tue, 14 Oct 2025 09:58:50 -0700 Subject: [PATCH 11/12] [PM-24468] Refactor HIBP exposure handling to capture errors per-cipher in exposed_count --- .../bitwarden-vault/src/cipher/cipher_risk.rs | 6 +- .../src/cipher/cipher_risk_client.rs | 163 +++++++++++++++--- 2 files changed, 142 insertions(+), 27 deletions(-) diff --git a/crates/bitwarden-vault/src/cipher/cipher_risk.rs b/crates/bitwarden-vault/src/cipher/cipher_risk.rs index 296472a2c..d76ebdca6 100644 --- a/crates/bitwarden-vault/src/cipher/cipher_risk.rs +++ b/crates/bitwarden-vault/src/cipher/cipher_risk.rs @@ -58,8 +58,10 @@ pub struct CipherRisk { /// Calculated using zxcvbn with cipher-specific context. pub password_strength: u8, /// Number of times password appears in HIBP database. - /// None if check_exposed was false in options. - pub exposed_count: Option, + /// - `None`: check_exposed was false, or password was empty + /// - `Some(Ok(n))`: Successfully checked, found n breaches + /// - `Some(Err(msg))`: HIBP API request failed for this cipher with the given error message + pub exposed_count: Option>, /// Number of times this password appears in the provided password_map. /// None if not found or if no password_map was provided. pub reuse_count: Option, diff --git a/crates/bitwarden-vault/src/cipher/cipher_risk_client.rs b/crates/bitwarden-vault/src/cipher/cipher_risk_client.rs index 5bdd0eec9..1bc36e6f2 100644 --- a/crates/bitwarden-vault/src/cipher/cipher_risk_client.rs +++ b/crates/bitwarden-vault/src/cipher/cipher_risk_client.rs @@ -1,7 +1,7 @@ use std::{collections::HashMap, sync::Arc}; use bitwarden_core::Client; -use futures::{StreamExt, TryStreamExt, stream}; +use futures::{StreamExt, stream}; #[cfg(feature = "wasm")] use wasm_bindgen::prelude::wasm_bindgen; @@ -49,11 +49,15 @@ impl CipherRiskClient { /// /// Returns a vector of `CipherRisk` results, one for each input cipher. /// + /// Individual HIBP API errors are captured per-cipher in `exposed_count` as + /// `Some(Err(msg))` where `msg` is the error message string. This allows the operation + /// to continue even if some API requests fail, ensuring that one bad request doesn't + /// cancel processing of other ciphers. + /// /// # Errors /// - /// Returns `CipherRiskError::Reqwest` if HIBP API requests fail when `check_exposed` is - /// enabled. Network errors include timeouts, connection failures, HTTP errors, or rate - /// limiting. On error, the entire operation fails - no partial results are returned. + /// This method only returns an error for internal logic failures. HIBP API errors are + /// captured per-cipher in the `exposed_count` field as `Some(Err(String))`. pub async fn compute_risk( &self, login_details: Vec, @@ -74,12 +78,12 @@ impl CipherRiskClient { async move { if details.password.is_empty() { // Skip empty passwords, return default risk values - return Ok(CipherRisk { + return CipherRisk { id: details.id, password_strength: 0, exposed_count: None, reuse_count: None, - }); + }; } let password_strength = Self::calculate_password_strength( @@ -88,11 +92,12 @@ impl CipherRiskClient { ); // Check exposure via HIBP API if enabled - // Network errors now propagate up instead of being silently ignored + // Capture errors per-cipher instead of propagating them let exposed_count = if options.check_exposed { Some( Self::check_password_exposed(&http_client, &details.password, &base_url) - .await?, + .await + .map_err(|e| e.to_string()), ) } else { None @@ -105,20 +110,22 @@ impl CipherRiskClient { None }; - Ok::(CipherRisk { + CipherRisk { id: details.id, password_strength, exposed_count, reuse_count, - }) + } } }); - // Process up to MAX_CONCURRENT_REQUESTS futures concurrently, fail fast on first error + // Process up to MAX_CONCURRENT_REQUESTS futures concurrently + // Individual HIBP errors are captured per-cipher, so we use collect() instead of + // try_collect() let results: Vec = stream::iter(futures) .buffer_unordered(MAX_CONCURRENT_REQUESTS) - .try_collect() - .await?; + .collect() + .await; Ok(results) } @@ -453,7 +460,7 @@ mod tests { let result = risk_client.compute_risk(login_details, options).await; - // Verify that empty passwords are skipped + // Verify that empty passwords are skipped (no HIBP check performed) assert!(result.is_ok()); let results = result.unwrap(); assert_eq!(results.len(), 1); @@ -463,9 +470,9 @@ mod tests { } #[tokio::test] - async fn test_compute_risk_propagates_network_errors() { - // Test that network errors from HIBP API are properly propagated - // instead of being silently swallowed + async fn test_compute_risk_captures_network_errors_per_cipher() { + // Test that network errors from HIBP API are captured per-cipher + // instead of canceling the entire batch use wiremock::{ Mock, MockServer, ResponseTemplate, matchers::{method, path_regex}, @@ -497,13 +504,110 @@ mod tests { let result = risk_client.compute_risk(login_details, options).await; - // Verify error is propagated, not swallowed - assert!(result.is_err()); - let err = result.unwrap_err(); + // Verify operation succeeds but error is captured per-cipher + assert!(result.is_ok()); + let results = result.unwrap(); + assert_eq!(results.len(), 1); + + // The exposed_count should be Some(Err(...)) + assert!(results[0].exposed_count.is_some()); + let exposed_result = results[0].exposed_count.as_ref().unwrap(); assert!( - matches!(err, CipherRiskError::Reqwest(_)), - "Expected CipherRiskError::Reqwest, got {:?}", - err + exposed_result.is_err(), + "Expected Err, but got Ok for exposed_count" + ); + + // Verify the error message is present + if let Err(err_msg) = exposed_result { + assert!(!err_msg.is_empty(), "Error message should not be empty"); + } + } + + #[tokio::test] + async fn test_compute_risk_partial_failures() { + // Test that when some HIBP checks succeed and others fail, + // all results are returned with appropriate success/error states + use wiremock::{ + Mock, MockServer, ResponseTemplate, + matchers::{method, path}, + }; + + let server = MockServer::start().await; + + // Hash prefix for "password1": E38AD (SHA1: E38AD214943DAAD1D64C102FAEC29DE4AFE9DA3D) + // Hash prefix for "password2": 2AA60 (SHA1: 2AA60A8FF7FCD473D321E0146AFD9E26DF395147) + + // Mock success for password1's hash prefix - return the suffix for password1 + Mock::given(method("GET")) + .and(path("/range/E38AD")) + .respond_with( + ResponseTemplate::new(200) + .set_body_string("214943DAAD1D64C102FAEC29DE4AFE9DA3D:5\r\n"), + ) + .mount(&server) + .await; + + // Mock failure for password2's hash prefix + Mock::given(method("GET")) + .and(path("/range/2AA60")) + .respond_with(ResponseTemplate::new(500)) + .mount(&server) + .await; + + let client = Client::init_test_account(test_bitwarden_com_account()).await; + let risk_client = CipherRiskClient { client }; + + let login_details = vec![ + CipherLoginDetails { + id: None, + password: "password1".to_string(), + username: Some("user1".to_string()), + }, + CipherLoginDetails { + id: None, + password: "password2".to_string(), + username: Some("user2".to_string()), + }, + ]; + + let options = CipherRiskOptions { + password_map: None, + check_exposed: true, + hibp_base_url: Some(server.uri()), + }; + + let result = risk_client.compute_risk(login_details, options).await; + + // Operation should succeed + assert!(result.is_ok()); + let results = result.unwrap(); + assert_eq!(results.len(), 2); + + // Both should have exposed_count populated (one success, one error) + assert!(results[0].exposed_count.is_some()); + assert!(results[1].exposed_count.is_some()); + + // Count successes and failures + let mut success_count = 0; + let mut error_count = 0; + + for result in &results { + match result.exposed_count.as_ref().unwrap() { + Ok(_) => success_count += 1, + Err(_) => error_count += 1, + } + } + + // We should have exactly one success and one failure + assert_eq!( + success_count, 1, + "Expected 1 successful HIBP check, got {}", + success_count + ); + assert_eq!( + error_count, 1, + "Expected 1 failed HIBP check, got {}", + error_count ); } @@ -634,9 +738,18 @@ mod tests { // Verify all results were returned assert_eq!(results.len(), 5); - // Verify all passwords were checked + // Verify all passwords were checked successfully for result in &results { - assert!(result.exposed_count.is_some()); + assert!( + result.exposed_count.is_some(), + "exposed_count should be Some for checked passwords" + ); + let exposed_result = result.exposed_count.as_ref().unwrap(); + assert!( + exposed_result.is_ok(), + "HIBP check should succeed, got error: {:?}", + exposed_result + ); } // Prove concurrency by analyzing request arrival times From 678856f83bd36db00bac4da911c8f93422a32fca Mon Sep 17 00:00:00 2001 From: Shane Date: Tue, 14 Oct 2025 11:37:23 -0700 Subject: [PATCH 12/12] [PM-24468] Update exposed password result type --- .../bitwarden-vault/src/cipher/cipher_risk.rs | 24 +++- .../src/cipher/cipher_risk_client.rs | 103 ++++++++++-------- 2 files changed, 74 insertions(+), 53 deletions(-) diff --git a/crates/bitwarden-vault/src/cipher/cipher_risk.rs b/crates/bitwarden-vault/src/cipher/cipher_risk.rs index d76ebdca6..ba042bcdd 100644 --- a/crates/bitwarden-vault/src/cipher/cipher_risk.rs +++ b/crates/bitwarden-vault/src/cipher/cipher_risk.rs @@ -6,6 +6,20 @@ use {tsify::Tsify, wasm_bindgen::prelude::*}; use crate::CipherId; +/// Result of checking password exposure via HIBP API. +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)] +#[cfg_attr(feature = "uniffi", derive(uniffi::Enum))] +#[cfg_attr(feature = "wasm", derive(Tsify), tsify(into_wasm_abi, from_wasm_abi))] +#[serde(tag = "type", content = "value")] +pub enum ExposedPasswordResult { + /// Password exposure check was not performed (check_exposed was false or password was empty) + NotChecked, + /// Successfully checked, found in this many breaches + Found(u32), + /// HIBP API request failed with error message + Error(String), +} + /// Login cipher data needed for risk evaluation. #[derive(Serialize, Deserialize, Debug, Clone)] #[cfg_attr(feature = "uniffi", derive(uniffi::Record))] @@ -57,11 +71,11 @@ pub struct CipherRisk { /// Password strength score from 0 (weakest) to 4 (strongest). /// Calculated using zxcvbn with cipher-specific context. pub password_strength: u8, - /// Number of times password appears in HIBP database. - /// - `None`: check_exposed was false, or password was empty - /// - `Some(Ok(n))`: Successfully checked, found n breaches - /// - `Some(Err(msg))`: HIBP API request failed for this cipher with the given error message - pub exposed_count: Option>, + /// Result of checking password exposure via HIBP API. + /// - `NotChecked`: check_exposed was false, or password was empty + /// - `Found(n)`: Successfully checked, found in n breaches + /// - `Error(msg)`: HIBP API request failed for this cipher with the given error message + pub exposed_result: ExposedPasswordResult, /// Number of times this password appears in the provided password_map. /// None if not found or if no password_map was provided. pub reuse_count: Option, diff --git a/crates/bitwarden-vault/src/cipher/cipher_risk_client.rs b/crates/bitwarden-vault/src/cipher/cipher_risk_client.rs index 1bc36e6f2..74622c216 100644 --- a/crates/bitwarden-vault/src/cipher/cipher_risk_client.rs +++ b/crates/bitwarden-vault/src/cipher/cipher_risk_client.rs @@ -5,7 +5,9 @@ use futures::{StreamExt, stream}; #[cfg(feature = "wasm")] use wasm_bindgen::prelude::wasm_bindgen; -use super::cipher_risk::{CipherLoginDetails, CipherRisk, CipherRiskOptions, PasswordReuseMap}; +use super::cipher_risk::{ + CipherLoginDetails, CipherRisk, CipherRiskOptions, ExposedPasswordResult, PasswordReuseMap, +}; use crate::CipherRiskError; /// Default base URL for the Have I Been Pwned (HIBP) Pwned Passwords API. @@ -45,19 +47,23 @@ impl CipherRiskClient { /// For each cipher: /// 1. Calculates password strength (0-4) using zxcvbn with cipher-specific context /// 2. Optionally checks if the password has been exposed via Have I Been Pwned API - /// 3. Counts how many times the password is reused in the provided `password_map`. + /// 3. Counts how many times the password is reused in the provided `password_map` /// /// Returns a vector of `CipherRisk` results, one for each input cipher. /// - /// Individual HIBP API errors are captured per-cipher in `exposed_count` as - /// `Some(Err(msg))` where `msg` is the error message string. This allows the operation - /// to continue even if some API requests fail, ensuring that one bad request doesn't - /// cancel processing of other ciphers. + /// ## HIBP Check Results (`exposed_result` field) + /// + /// The `exposed_result` field uses the `ExposedPasswordResult` enum with three possible states: + /// - `NotChecked`: Password exposure check was not performed because: + /// - `check_exposed` option was `false`, or + /// - Password was empty + /// - `Found(n)`: Successfully checked via HIBP API, password appears in `n` data breaches + /// - `Error(msg)`: HIBP API request failed with error message `msg` /// /// # Errors /// - /// This method only returns an error for internal logic failures. HIBP API errors are - /// captured per-cipher in the `exposed_count` field as `Some(Err(String))`. + /// This method only returns `Err` for internal logic failures. HIBP API errors are + /// captured per-cipher in the `exposed_result` field as `ExposedPasswordResult::Error(msg)`. pub async fn compute_risk( &self, login_details: Vec, @@ -81,7 +87,7 @@ impl CipherRiskClient { return CipherRisk { id: details.id, password_strength: 0, - exposed_count: None, + exposed_result: ExposedPasswordResult::NotChecked, reuse_count: None, }; } @@ -93,14 +99,15 @@ impl CipherRiskClient { // Check exposure via HIBP API if enabled // Capture errors per-cipher instead of propagating them - let exposed_count = if options.check_exposed { - Some( - Self::check_password_exposed(&http_client, &details.password, &base_url) - .await - .map_err(|e| e.to_string()), - ) + let exposed_result = if options.check_exposed { + match Self::check_password_exposed(&http_client, &details.password, &base_url) + .await + { + Ok(count) => ExposedPasswordResult::Found(count), + Err(e) => ExposedPasswordResult::Error(e.to_string()), + } } else { - None + ExposedPasswordResult::NotChecked }; // Check reuse from provided map @@ -113,7 +120,7 @@ impl CipherRiskClient { CipherRisk { id: details.id, password_strength, - exposed_count, + exposed_result, reuse_count, } } @@ -465,7 +472,7 @@ mod tests { let results = result.unwrap(); assert_eq!(results.len(), 1); assert_eq!(results[0].password_strength, 0); - assert_eq!(results[0].exposed_count, None); + assert_eq!(results[0].exposed_result, ExposedPasswordResult::NotChecked); assert_eq!(results[0].reuse_count, None); } @@ -509,17 +516,17 @@ mod tests { let results = result.unwrap(); assert_eq!(results.len(), 1); - // The exposed_count should be Some(Err(...)) - assert!(results[0].exposed_count.is_some()); - let exposed_result = results[0].exposed_count.as_ref().unwrap(); - assert!( - exposed_result.is_err(), - "Expected Err, but got Ok for exposed_count" - ); - - // Verify the error message is present - if let Err(err_msg) = exposed_result { - assert!(!err_msg.is_empty(), "Error message should not be empty"); + // The exposed_result should be Error(...) + match &results[0].exposed_result { + ExposedPasswordResult::Error(msg) => { + assert!(!msg.is_empty(), "Error message should not be empty"); + } + ExposedPasswordResult::Found(_) => { + panic!("Expected Error variant, but got Found"); + } + ExposedPasswordResult::NotChecked => { + panic!("Expected Error variant, but got NotChecked"); + } } } @@ -583,18 +590,17 @@ mod tests { let results = result.unwrap(); assert_eq!(results.len(), 2); - // Both should have exposed_count populated (one success, one error) - assert!(results[0].exposed_count.is_some()); - assert!(results[1].exposed_count.is_some()); - // Count successes and failures let mut success_count = 0; let mut error_count = 0; for result in &results { - match result.exposed_count.as_ref().unwrap() { - Ok(_) => success_count += 1, - Err(_) => error_count += 1, + match &result.exposed_result { + ExposedPasswordResult::Found(_) => success_count += 1, + ExposedPasswordResult::Error(_) => error_count += 1, + ExposedPasswordResult::NotChecked => { + panic!("Expected Found or Error, but got NotChecked") + } } } @@ -670,8 +676,8 @@ mod tests { assert_eq!(results[1].reuse_count, Some(1)); // HIBP not checked - assert!(results[0].exposed_count.is_none()); - assert!(results[1].exposed_count.is_none()); + assert_eq!(results[0].exposed_result, ExposedPasswordResult::NotChecked); + assert_eq!(results[1].exposed_result, ExposedPasswordResult::NotChecked); } #[tokio::test] @@ -740,16 +746,17 @@ mod tests { // Verify all passwords were checked successfully for result in &results { - assert!( - result.exposed_count.is_some(), - "exposed_count should be Some for checked passwords" - ); - let exposed_result = result.exposed_count.as_ref().unwrap(); - assert!( - exposed_result.is_ok(), - "HIBP check should succeed, got error: {:?}", - exposed_result - ); + match &result.exposed_result { + ExposedPasswordResult::Found(_) => { + // Success - password was checked + } + ExposedPasswordResult::Error(err) => { + panic!("HIBP check should succeed, got error: {}", err); + } + ExposedPasswordResult::NotChecked => { + panic!("All passwords should be checked when check_exposed=true"); + } + } } // Prove concurrency by analyzing request arrival times