Fixed invalid cipher reprompt values

[gbubemismith]

🎟️ Tracking

📔 Objective

📸 Screenshots

⏰ 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 @gbubemismith's task —— View job

---

Code Review Complete

Summary

The fix correctly validates the reprompt value when mapping to SDK format, ensuring only CipherRepromptType.Password or CipherRepromptType.None are sent. The code change itself is correct, but there are several issues with the PR metadata and testing.

---

Findings

Finding 1: PR lacks essential information in title, objective, and tracking section

Finding 2: Missing test coverage for the new validation logic

Finding 3: Inconsistent naming - the fix targets toSdkCipher, but fromSdkCipher may have the same issue

---

Details

💭 Finding 1: PR metadata is incomplete

The PR is missing critical information:

• Title: "Fixed invalid cipher reprompt values" - This is vague. What were the invalid values? What was the symptom?

  • Suggested: "Validate cipher reprompt values when converting to SDK format" or "Fix: Normalize reprompt values to valid SDK types in toSdkCipher"

• Objective section: Empty - Should explain:

  • What bug this fixes (e.g., "Invalid reprompt values were being passed to the SDK")
  • Why it's a problem (e.g., "SDK only accepts 0 or 1, but we were passing other values")
  • What the fix does (e.g., "Added validation to normalize to Password or None")

• Tracking section: Empty - Should reference the Jira ticket that tracks this bug

Suggested PR description rewrite

## 🎟️ Tracking

[Link to Jira ticket]

## 📔 Objective

This PR fixes a bug where invalid reprompt values were being passed to the Rust SDK in the `toSdkCipher` method. The SDK's `CipherRepromptType` only accepts two values: `None` (0) and `Password` (1). However, the TypeScript `Cipher` domain model was passing the raw `reprompt` value without validation, which could potentially include invalid values.

**Changes:**
- Added validation in `toSdkCipher` to ensure only `CipherRepromptType.Password` or `CipherRepromptType.None` are passed to the SDK
- Any value other than `Password` defaults to `None`

## ⏰ Reminders before review
[...]

---

⚠️ Finding 2: Missing test coverage for the new validation logic

The existing test in cipher.spec.ts (line 899-1059) only tests the case where reprompt: CipherRepromptType.None, which means the new validation logic is never exercised by tests.

Required tests:

1. Test with reprompt: CipherRepromptType.Password to verify it maps correctly
2. Test with an invalid/undefined value to verify it defaults to None

Suggested test additions

Add these test cases to the toSdkCipher describe block:

it("should map reprompt Password value correctly", () => {
  const cipher = new Cipher();
  cipher.reprompt = CipherRepromptType.Password;
  cipher.type = CipherType.Login;
  
  const sdkCipher = cipher.toSdkCipher();
  
  expect(sdkCipher.reprompt).toBe(SdkCipherRepromptType.Password);
});

it("should default invalid reprompt values to None", () => {
  const cipher = new Cipher();
  cipher.reprompt = 999 as any; // Invalid value
  cipher.type = CipherType.Login;
  
  const sdkCipher = cipher.toSdkCipher();
  
  expect(sdkCipher.reprompt).toBe(SdkCipherRepromptType.None);
});

---

💭 Finding 3: Consider validating fromSdkCipher for consistency

The fromSdkCipher method (line 467-513) directly assigns cipher.reprompt = sdkCipher.reprompt without validation (line 503). While the SDK should only ever send valid values, defensive programming suggests we should validate here as well.

Question: Could the SDK ever send an invalid value that we need to handle? If so, should we add similar validation in fromSdkCipher?

libs/common/src/vault/models/domain/cipher.ts:503

---

Recommendations

1. ❌ Update PR title and description with clear information about what bug this fixes
2. ⚠️ Add test coverage for the new validation logic (both valid Password case and invalid value case)
3. 💭 Consider whether fromSdkCipher needs similar validation for consistency

---

[github-actions]

Checkmarx One – Scan Summary & Details – 2fcbaa1d-2f44-4ac8-93c4-a6b52323809b

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

[claude]

👍 Good defensive validation here! This ensures only valid SDK enum values are passed.

However, this change needs test coverage. The existing test in cipher.spec.ts only tests CipherRepromptType.None. Consider adding tests for:

1. reprompt: CipherRepromptType.Password → should map to SdkCipherRepromptType.Password
2. Invalid values → should default to SdkCipherRepromptType.None

[codecov]

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 41.26%. Comparing base (e23b2d0) to head (2a88559).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
libs/common/src/vault/models/domain/cipher.ts	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #17513   +/-   ##
=======================================
  Coverage   41.25%   41.26%           
=======================================
  Files        3546     3546           
  Lines      102040   102041    +1     
  Branches    15308    15309    +1     
=======================================
+ Hits        42095    42103    +8     
+ Misses      58180    58173    -7     
  Partials     1765     1765           

☔ 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.