[PM-24467] Introduce cipher risk service

[shane-melton]

🎟️ Tracking

PM-24467

📔 Objective

Introduce the CipherRiskService and have it call the SDK client using the CipherRiskClient introduced in bitwarden/sdk-internal#499.

⏰ 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

[claude]

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

---

Code Review Complete ✓

Summary of Changes Since Last Review

This PR introduces a new CipherRiskService that provides password risk analysis capabilities by calling the SDK's CipherRiskClient. The implementation includes:

• New abstraction layer: cipher-risk.service.ts
• Implementation: default-cipher-risk.service.ts
• Comprehensive test suite: default-cipher-risk.service.spec.ts
• SDK version updates from 0.2.0-main.357 to 0.2.0-main.365

9 commits were added since the initial implementation, including improvements based on earlier feedback (using switchMap, test cleanup, SDK model updates).

---

Critical Issues Found

1. ⚠️ Potential Index Out of Bounds Error

Location: libs/common/src/vault/services/default-cipher-risk.service.ts:72

async computeCipherRiskForUser(...): Promise<CipherRiskResult> {
  // ...
  const results = await this.computeRiskForCiphers([targetCipher], userId, {
    passwordMap,
    checkExposed,
  });

  return results[0];  // ⚠️ Unsafe array access
}

Issue: If computeRiskForCiphers returns an empty array (which could happen if the cipher is filtered out in mapToLoginDetails), accessing results[0] will return undefined, violating the return type contract.

Scenario: Although the targetCipher is found, if it doesn't have a password or isn't a Login type, mapToLoginDetails filters it out, causing computeRiskForCiphers to return [].

Recommendation:

if (results.length === 0) {
  throw new Error(`Unable to compute risk for cipher ${cipherId}`);
}
return results[0];

2. ⚠️ Inconsistent Default Values for checkExposed

Locations:

• cipher-risk.service.ts:39 - defaults to true (in JSDoc comment)
• default-cipher-risk.service.ts:42 - defaults to false in implementation
• default-cipher-risk.service.ts:52 - defaults to true in method signature

Issue: The abstraction documentation states checkExposed behavior, but the base computeRiskForCiphers method defaults to false when options are undefined, while computeCipherRiskForUser defaults to true. This creates inconsistent behavior.

Recommendation: Make the defaults consistent across all methods. If breach checking should be opt-in for performance/API quota reasons, default to false everywhere. If it should be the standard behavior, default to true everywhere. Document the rationale in comments.

3. 🤔 Null Safety: Optional Chaining Missing Error Handling

Location: libs/common/src/vault/services/default-cipher-risk.service.ts:55

const allCiphers = await firstValueFrom(this.cipherService.cipherViews$(userId));
const targetCipher = allCiphers?.find((c) => asUuid<CipherId>(c.id) === cipherId);

Issue: The optional chaining on allCiphers?.find() suggests allCiphers could be null or undefined. If it is, targetCipher will be undefined, and the next check will throw an error. However, the error message says "Cipher with id X not found" which is misleading if the issue is that the entire ciphers list is unavailable.

Recommendation: Handle the null case explicitly:

const allCiphers = await firstValueFrom(this.cipherService.cipherViews$(userId));
if (!allCiphers) {
  throw new Error(`Unable to retrieve ciphers for user ${userId}`);
}
const targetCipher = allCiphers.find((c) => asUuid<CipherId>(c.id) === cipherId);

---

Suggested Improvements

4. 🎨 Consider Caching Password Reuse Map for Bulk Operations

Location: libs/common/src/vault/services/default-cipher-risk.service.ts:64

Issue: computeCipherRiskForUser always builds a fresh password reuse map by scanning all ciphers. This is inefficient if calling code wants to check multiple ciphers sequentially.

Recommendation: Document that callers who need to check multiple ciphers should use computeRiskForCiphers with a pre-built passwordMap instead of calling computeCipherRiskForUser in a loop. Consider adding this guidance to the JSDoc comments.

Alternatively, add a convenience method:

async computeRisksForUserCiphers(
  cipherIds: CipherId[],
  userId: UserId,
  checkExposed?: boolean,
): Promise<Map<CipherId, CipherRiskResult>>

5. 🎨 Type Safety: Consider Stricter Return Type

Location: libs/common/src/vault/abstractions/cipher-risk.service.ts:22-26

Issue: The method computeRiskForCiphers accepts an array of CipherView[] but returns CipherRiskResult[] without guaranteeing the ordering or count matches the input. This could lead to caller confusion about which result corresponds to which cipher.

Current behavior: SDK returns results only for valid Login ciphers with passwords, which may be fewer than input ciphers.

Recommendation:

• Document the filtering behavior clearly in the JSDoc (✓ already partially done)
• Consider returning Map<CipherId, CipherRiskResult> instead of an array to make the association explicit
• Or document that results maintain the same order as filtered input ciphers

6. 📝 Missing JSDoc for hibpBaseUrl Option

Location: libs/common/src/vault/abstractions/cipher-risk.service.ts:19

Issue: The JSDoc mentions checkExposed but not hibpBaseUrl, which is part of CipherRiskOptions (visible in tests at line 62).

Recommendation: Update JSDoc to explain all available options:

/**
 * @param options - Optional configuration for risk computation
 *   - passwordMap: Pre-computed password reuse map for efficiency
 *   - checkExposed: Whether to check HIBP for password breaches
 *   - hibpBaseUrl: Override HIBP API endpoint (primarily for testing)
 */

7. 🎨 Resource Management: Consider Error Handling for SDK Client

Location: libs/common/src/vault/services/default-cipher-risk.service.ts:38

Issue: The code uses using statement for automatic resource disposal, which is excellent. However, there's no explicit error handling around SDK operations.

Recommendation: Consider whether SDK errors should be caught and re-thrown with more context, or if letting them bubble up is acceptable. Document the expected exceptions in JSDoc:

/**
 * @throws {UserNotLoggedInError} If the user is not logged in
 * @throws {Error} If SDK client initialization fails
 * @throws {Error} If HIBP API request fails (when checkExposed is true)
 */

8. 🤔 Performance: firstValueFrom on Observable per Call

Location: Multiple locations using firstValueFrom(this.sdkService.userClient$(userId))

Issue: Each method call converts an Observable to a Promise. If userClient$ is a hot observable that emits frequently, this is fine. If it's cold and expensive, this could be inefficient.

Question: Is this the standard pattern for SDK service usage in this codebase? (Checking other services suggests yes, so this is likely fine, just noting for awareness)

9. ⛏ Naming: Consider Renaming mapToLoginDetails

Location: libs/common/src/vault/services/default-cipher-risk.service.ts:97

Current name: mapToLoginDetails
Suggestion: filterAndMapToLoginDetails or extractLoginDetailsForRiskAnalysis

Rationale: The method both filters (removes non-Login ciphers and those without passwords) AND maps to a different type. The current name only conveys the mapping aspect.

---

Good Practices Observed

• ✅ Excellent test coverage with 15+ test cases covering edge cases
• ✅ Proper use of TypeScript using statement for resource management (disposable pattern)
• ✅ Follows Observable patterns with firstValueFrom and switchMap
• ✅ Type-safe UUID handling with asUuid<CipherId>
• ✅ Proper separation of concerns (abstraction + implementation)
• ✅ Follows ADR-0025 (uses const object for CipherType, not enum)
• ✅ No logging of sensitive data (passwords, vault content)
• ✅ Proper filtering to only process Login ciphers with passwords

---

Security Considerations

✅ No Security Issues Found

The implementation properly:

• Does not log passwords or decrypted vault data
• Handles sensitive data appropriately by passing it to SDK without intermediate storage
• Uses type-safe operations to prevent type confusion
• Follows the project's security guidelines

📝 Note on HIBP Integration

The checkExposed feature calls HIBP (Have I Been Pwned) to check if passwords have been exposed in data breaches. This is done via the SDK's CipherRiskClient, which should implement k-anonymity protocol (only sending partial password hashes). Verify this is handled correctly in the SDK implementation.

---

Architecture & Pattern Adherence

✅ Follows Project Guidelines

• ADR-0003 (Observable Data Services): Uses RxJS Observables correctly with firstValueFrom and switchMap
• ADR-0025 (No TypeScript Enums): Uses CipherType const object instead of enums
• Dependency Injection: Proper constructor injection of services
• Abstraction Pattern: Clean separation between interface and implementation

🤔 Observable vs Async Pattern

Observation: The service methods return Promise instead of Observable. This is a valid choice for one-shot operations, but consider whether any methods might benefit from Observable streams (e.g., reactive risk updates when vault changes).

Current pattern matches other services like buildPasswordReuseMap that are one-shot computations, so this is consistent with the codebase.

---

Action Items for Author

High Priority

1. Fix: Add bounds checking in computeCipherRiskForUser before accessing results[0]
2. Fix: Resolve inconsistent checkExposed defaults across methods
3. Fix: Improve error message when allCiphers is null/undefined

Medium Priority

4. Improve: Add JSDoc documentation for all CipherRiskOptions fields
5. Improve: Add @throws documentation for expected exceptions
6. Consider: Document performance characteristics for bulk operations

Low Priority (Nice to Have)

7. Consider: Add convenience method for bulk cipher risk computation
8. Consider: Return type improvement (Map vs Array) for better type safety
9. Polish: Rename mapToLoginDetails to better convey filtering + mapping behavior

---

Conclusion

This is a well-implemented addition to the codebase with excellent test coverage and proper security handling. The main concerns are around edge case error handling (array bounds, null handling) and API consistency (default values). The architectural patterns are sound and follow project guidelines.

Once the critical issues are addressed, this will be a solid foundation for password risk analysis features.

Recommended next steps: Address the 3 critical issues, then this should be ready for merge.

---

Review Context Information

• Reviewer: Claude Code (AI Assistant)
• PR Branch: vault/pm-24467/cipher-risk-service
• Base Branch: main
• Files Changed: 5 (3 new, 2 modified)
• Lines Added: +718 / Lines Removed: -10

[github-actions]

Checkmarx One – Scan Summary & Details – 0897e7b1-07e1-4f98-96d6-c067f13e84ca

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

[nikwithak]

LGTM - minor naming question.

[nikwithak]

suggestion (non-blocking): Is forUser the appropriate qualifier here? This is more about a specific cipher than a specific user.

[shane-melton]

I'm not strongly attached to that qualifier, this is just a convenience method that builds the password use map using the all of the specified user's ciphers/Vault (instead of the caller passing in their own list of ciphers). Perhaps, *ForUserVault be more clear?

[nikwithak]

I was thinking mo,re along the lines of *ForCipher since it's calculating a specific/single cipher, but I understand now that you intended it to mean "using the User's vault" - I don't have a better suggestion to convey that (naming is hard).

[codecov]

Codecov Report

❌ Patch coverage is 86.84211% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 40.44%. Comparing base (2b00977) to head (34b2cac).
⚠️ Report is 16 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
.../src/vault/services/default-cipher-risk.service.ts	89.18%	2 Missing and 2 partials ⚠️
...mmon/src/vault/abstractions/cipher-risk.service.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #17009      +/-   ##
==========================================
+ Coverage   40.42%   40.44%   +0.01%     
==========================================
  Files        3503     3505       +2     
  Lines      100080   100118      +38     
  Branches    15011    15018       +7     
==========================================
+ Hits        40458    40493      +35     
- Misses      57904    57905       +1     
- Partials     1718     1720       +2     

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

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.