Skip to content

Conversation

shane-melton
Copy link
Member

@shane-melton shane-melton commented Oct 9, 2025

🎟️ Tracking

PM-24468

📔 Objective

Implement the cipher risk evaluation logic in the Vault SDK as a separate CipherRiskClient so that it can be re-used in mobile and gain potential performance improvements.

The CipherRiskClient contains logic to check if a multiple login ciphers' passwords are exposed (via HIBP), weak (via zxcvbn), or reused.

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation
    team

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed
    issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

Copy link

claude bot commented Oct 9, 2025

Claude finished @shane-melton's task —— View job


Code Review Complete ✅

Summary of Changes

This PR introduces CipherRiskClient to evaluate security risks for login ciphers. The implementation checks for:

  • Password strength: Using zxcvbn with cipher-specific context (username/email)
  • Password exposure: Via HIBP API using k-anonymity
  • Password reuse: Tracking duplicate passwords across ciphers

Key additions:

  • cipher_risk_client.rs (617 lines): Core client implementation with async concurrent processing
  • cipher_risk.rs (75 lines): Data structures for risk evaluation
  • Helper method to_login_details() in cipher.rs for extracting login data
  • New CipherRiskError type for error handling

Critical Issues

🔴 Security: Potential Password Leakage in Error Messages

Location: cipher_risk_client.rs:81-86

Network errors from HIBP API could potentially expose sensitive information if passwords are included in error messages. While reqwest::Error shouldn't include request body content by default, this needs verification.

Recommendation: Add explicit error wrapping to ensure passwords are never logged:

let exposed_count = if options.check_exposed {
    Some(
        Self::check_password_exposed(&http_client, &details.password, &base_url)
            .await
            .map_err(|e| {
                // Strip any potential sensitive data from error message
                CipherRiskError::Reqwest(e)
            })?,
    )
} else {
    None
};

Additionally, verify that reqwest::Error display/debug implementations don't leak request bodies.

Reference CLAUDE.md line 29: "Do not log keys, passwords, or vault data in logs or error paths. Redact sensitive data."


⚠️ Performance: Inefficient Password Reuse Detection

Location: cipher_risk_client.rs:30-41 and cipher_risk_client.rs:92-96

The current implementation requires users to call password_reuse_map() separately, then pass the result to compute_risk(). This creates several issues:

  1. Two passes over data: Users must iterate through all ciphers once to build the map, then again to compute risks
  2. Memory overhead: The entire password map is cloned into Arc for each concurrent task
  3. API ergonomics: Users must manually handle the two-step process

Recommendation:

pub async fn compute_risk(
    &self,
    login_details: Vec<CipherLoginDetails>,
    options: CipherRiskOptions,
) -> Result<Vec<CipherRisk>, CipherRiskError> {
    // Build password map internally if not provided
    let password_map = options.password_map
        .unwrap_or_else(|| {
            let mut map = HashMap::new();
            for details in &login_details {
                if !details.password.is_empty() {
                    *map.entry(details.password.clone()).or_insert(0) += 1;
                }
            }
            PasswordReuseMap { map }
        });
    
    let password_map = Arc::new(password_map);
    // ... rest of implementation
}

This provides better defaults while still allowing advanced users to pre-compute the map for multiple calls.


⚠️ Error Handling: Silent Failures in Hash Parsing

Location: cipher_risk_client.rs:179-188

The parse_hibp_response() function silently returns 0 when count parsing fails:

return count_str.trim().parse().unwrap_or(0);

This masks data corruption or API changes. A malformed response indicating millions of breaches could be silently treated as zero breaches.

Recommendation:

fn parse_hibp_response(response: &str, target_suffix: &str) -> Result<u32, CipherRiskError> {
    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()
                    .map_err(|_| CipherRiskError::InvalidHibpResponse(line.to_string()));
            }
        }
    }
    Ok(0)
}

Add corresponding error variant to CipherRiskError.


Suggested Improvements

🎨 API Design: Make check_exposed Default to False

Location: cipher_risk.rs:34-48

The CipherRiskOptions::default() implementation sets check_exposed: false, but this field is not marked with #[serde(default)]. This could cause confusion for API consumers.

Recommendation: Make the default explicit and consistent:

#[derive(Serialize, Deserialize, Debug, Clone)]
#[serde(rename_all = "camelCase")]
pub struct CipherRiskOptions {
    pub password_map: Option<PasswordReuseMap>,
    #[serde(default)]
    pub check_exposed: bool,
    pub hibp_base_url: Option<String>,
}

🎨 Concurrency: Magic Number Without Justification

Location: cipher_risk_client.rs:15

const MAX_CONCURRENT_REQUESTS: usize = 100;

This value seems quite high for HIBP API calls. HIBP has rate limiting (though generous), and 100 concurrent requests could:

  1. Trigger rate limiting under certain conditions
  2. Overwhelm client network resources
  3. Create unnecessary connection overhead

Recommendation:

  • Document the rationale for this value
  • Consider lowering to 10-20 based on typical use cases
  • Make it configurable via CipherRiskOptions for advanced users

🎨 Code Quality: Username Extraction Logic Could Be Simplified

Location: cipher_risk_client.rs:139-160

The username parsing logic handles email vs username cases, but the implementation is identical for both branches after splitting.

Recommendation:

fn extract_user_inputs(username: &str) -> Vec<String> {
    // For emails, only use local part; for usernames, use entire string
    let relevant_part = username.split_once('@')
        .map(|(local, _)| local)
        .unwrap_or(username);
    
    relevant_part
        .trim()
        .to_lowercase()
        .split(|c: char| !c.is_alphanumeric())
        .filter(|s| !s.is_empty())
        .map(str::to_owned)
        .collect()
}

This is more concise and equally clear.


🎨 Testing: Missing Test Coverage

Test coverage: 93.4% patch coverage with 32 missing lines (per codecov)

Missing coverage areas:

  1. cipher.rs:489-504 - to_login_details() method (lines 493-504 not covered)
  2. cipher_risk.rs - All data structures (6 lines missing)
  3. vault_client.rs:75-79 - cipher_risk() method integration (5 lines missing)

Recommendation: Add unit tests for:

#[test]
fn test_to_login_details_with_password() {
    let cipher = CipherView {
        login: Some(LoginView {
            password: Some("test".to_string()),
            username: Some("user".to_string()),
            // ... other fields
        }),
        // ... other fields
    };
    
    let details = cipher.to_login_details();
    assert!(details.is_some());
    assert_eq!(details.unwrap().password, "test");
}

#[test]
fn test_to_login_details_without_password() {
    let cipher = CipherView {
        login: Some(LoginView {
            password: None,
            // ... other fields
        }),
        // ... other fields
    };
    
    assert!(cipher.to_login_details().is_none());
}

🎨 Documentation: Missing Examples

Location: cipher_risk_client.rs:25-41, 43-114

The public API methods have good documentation, but lack usage examples. This is especially important for a new feature.

Recommendation: Add examples to docstrings:

/// Build password reuse map for a list of login ciphers.
///
/// # Example
///
/// ```rust
/// let login_details = vec![
///     CipherLoginDetails {
///         id: Some(cipher_id),
///         password: "password123".to_string(),
///         username: Some("[email protected]".to_string()),
///     },
/// ];
///
/// let password_map = client.password_reuse_map(login_details)?;
/// ```

📝 Minor Issues & Nitpicks

  1. Inconsistent error propagation (cipher_risk_client.rs:81-86): The error comment mentions "Network errors now propagate up instead of being silently ignored" but this comment should be removed or moved to PR description as it references old behavior not visible in current code.

  2. Redundant clone (cipher_risk_client.rs:493): login_details.clone() in test could be avoided by adjusting the test flow.

  3. Test naming (cipher_risk_client.rs:425-467): test_compute_risk_propagates_network_errors could be simplified to test_compute_risk_network_errors since propagation is the expected behavior.

  4. Magic strings (cipher_risk_client.rs:327-330): Test uses hardcoded SHA-1 hash without explaining what password it corresponds to. The comment "Test with a known password: 'password'" helps, but the test would be clearer with a constant.

  5. Empty password handling (cipher_risk_client.rs:36): The code skips empty passwords in reuse detection but doesn't document why. Empty passwords are arguably a security issue worth flagging.

  6. HIBP URL validation (cipher_risk_client.rs:69-72): No validation that hibp_base_url is a valid URL. Malformed URLs will fail at request time, but early validation would provide better error messages.


Good Practices Observed

👍 Comprehensive test suite including unit, integration, and network mocking tests
👍 K-anonymity implementation for HIBP API calls protects user privacy
👍 Proper use of Arc to avoid expensive HashMap clones in concurrent context
👍 Clear separation of concerns (client, data structures, errors)
👍 WASM and uniffi feature flags properly configured
👍 Case-insensitive hash comparison for HIBP responses
👍 Concurrent request processing with configurable buffer
👍 Proper error type hierarchy


Action Items

For Author (Priority Order)

  1. HIGH: Add explicit password redaction verification in error paths
  2. HIGH: Fix parse_hibp_response to propagate parsing errors instead of silently returning 0
  3. MEDIUM: Consider automatic password map construction in compute_risk() when not provided
  4. MEDIUM: Add test coverage for to_login_details() and vault client integration
  5. MEDIUM: Document rationale for MAX_CONCURRENT_REQUESTS = 100 or make it configurable
  6. LOW: Add usage examples to public API documentation
  7. LOW: Simplify extract_user_inputs() implementation

For Reviewers

  • Verify that reqwest::Error cannot leak password data in any scenario
  • Consider if password reuse map API could be more ergonomic
  • Validate if 100 concurrent requests is appropriate for HIBP API

| View PR diff

Copy link
Contributor

github-actions bot commented Oct 9, 2025

Logo
Checkmarx One – Scan Summary & Details65dfd6f5-95b6-43db-9b7c-bcb17341fa21

Great job! No new security vulnerabilities introduced in this pull request

Copy link

codecov bot commented Oct 9, 2025

Codecov Report

❌ Patch coverage is 93.90244% with 30 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.26%. Comparing base (80fbec9) to head (cc28aab).
⚠️ Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
crates/bitwarden-vault/src/cipher/cipher.rs 0.00% 12 Missing ⚠️
crates/bitwarden-vault/src/cipher/cipher_risk.rs 0.00% 6 Missing ⚠️
...s/bitwarden-vault/src/cipher/cipher_risk_client.rs 98.71% 6 Missing ⚠️
crates/bitwarden-vault/src/vault_client.rs 0.00% 5 Missing ⚠️
crates/bitwarden-vault/src/error.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #499      +/-   ##
==========================================
+ Coverage   77.98%   78.26%   +0.28%     
==========================================
  Files         287      289       +2     
  Lines       27672    28165     +493     
==========================================
+ Hits        21579    22044     +465     
- Misses       6093     6121      +28     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@shane-melton shane-melton force-pushed the vault/pm-24468/cipher-risk-client branch from 915fe76 to a10fef6 Compare October 13, 2025 17:54
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant