[PM-25012] Add data field to Cipher type, and add functions for client-side parsing of the field.

[nikwithak]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-25012

📔 Objective

This PR adds the data field to the Cipher struct, and adds a function for parsing that field into the appropriate CipherType (Identity, SshKey, Login, SecureNote, Card) on the client-side, rather than consuming the server-provided values.

The function sits unused - after #455 is merged, we should consider adding this to the CiphersClient::get and CipherClient::list functions behind a new feature flag.

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

---

Code Review Complete

Tasks

• Read and analyze changed files
• Review against CLAUDE.md guidelines
• Analyze error handling and security implications
• Check test coverage
• Provide comprehensive feedback

---

Summary of Changes

This PR adds client-side parsing of cipher data by introducing a data field to the Cipher struct and implementing a populate_cipher_types() function. The function deserializes the JSON in the data field into the appropriate cipher type (Login, SecureNote, Card, Identity, SshKey), allowing clients to independently populate cipher types rather than relying solely on server-provided values. The function is currently marked #[allow(unused)] and will be integrated behind a feature flag after PR #455 merges.

Key additions:

• data: Option<String> field added to Cipher struct (cipher.rs:148)
• populate_cipher_types() method (cipher.rs:462-476)
• Comprehensive test coverage for all cipher types
• Error handling via VaultParseError::SerdeJson

---

Critical Issues

1. ⚠️ Potential Information Disclosure in Error Messages

Location: cipher.rs:462-476, error.rs:28-37

The populate_cipher_types() function uses serde_json::from_str() which may leak encrypted data in error messages. From the author's testing:

• SerdeJson(Error("EncString(InvalidTypeSymm { enc_type: \"0\", parts: 1 })", line: 1, column: 39))
• SerdeJson(Error("EncString(NotB64EncodedError)", line: 1, column: 126))

While these examples don't show plaintext leakage, the error includes EncString parsing details and column positions. Per CLAUDE.md: "Do not log keys, passwords, or vault data in logs or error paths. Redact sensitive data."

Recommendation: Wrap deserialization errors with a sanitized message:

pub(crate) fn populate_cipher_types(&mut self) -> Result<(), VaultParseError> {
    let data = self.data.as_ref()
        .ok_or(VaultParseError::MissingField(MissingFieldError("data")))?;

    let result = match &self.r#type {
        crate::CipherType::Login => 
            serde_json::from_str(data).map(|v| { self.login = v; }),
        // ... other types
    };
    
    result.map_err(|_| VaultParseError::InvalidCipherData)?;
    Ok(())
}

Add to error.rs:

#[error("Failed to parse cipher data field")]
InvalidCipherData,

Alternative approach: Custom deserializer with explicit error handling

Implement a custom deserializer that catches and redacts errors:

fn parse_cipher_data<T: DeserializeOwned>(data: &str) -> Result<T, VaultParseError> {
    serde_json::from_str(data).map_err(|_| VaultParseError::InvalidCipherData)
}

---

2. ⚠️ Incomplete Data Field Population on Encryption

Location: cipher.rs:349

When encrypting a CipherView back to a Cipher, the data field is set to None:

data: None, // TODO: Do we need to repopulate this on this on the cipher?

Issues:

• If populate_cipher_types() is called, then the cipher is modified and re-encrypted, the data field will be lost
• This creates an inconsistency where ciphers parsed from data cannot round-trip through encryption
• The TODO comment indicates uncertainty about the intended behavior

Questions:

• Should data be repopulated from the cipher type fields?
• Should data be preserved as-is from the original cipher?
• What happens when a client using populate_cipher_types() sends an update to the server without the data field?

Recommendation: Clarify the expected behavior and either:

1. Preserve the original data field: data: self.data.clone() (if data is added to CipherView)
2. Regenerate data from the populated fields
3. Document explicitly why data should be None after client modifications

---

Suggested Improvements

3. 🎨 Add Documentation for populate_cipher_types() (Addresses reviewer question)

Location: cipher.rs:458-476

Per reviewer comment, the function needs documentation explaining when it should be used and why it's currently unused. Current state only has a single-line doc comment.

Recommendation:

/// Populates cipher type fields (`login`, `secure_note`, `card`, `identity`, `ssh_key`) 
/// by parsing the `data` field instead of using server-provided values.
///
/// # Purpose
/// This enables client-side validation and parsing of cipher data, providing an
/// independent source of truth separate from server responses. This is part of
/// cipher versioning support (PM-25012).
///
/// # Usage
/// This function is currently unused but will be integrated behind a feature flag
/// after #455 merges. It will be called in `CiphersClient::get()` and 
/// `CipherClient::list()` to provide client-side parsing as an alternative to
/// server-provided cipher type data.
///
/// # Errors
/// Returns `VaultParseError::MissingField` if the `data` field is `None`.
/// Returns `VaultParseError::SerdeJson` if deserialization fails.
///
/// # Security Note
/// This function deserializes encrypted data. Deserialization errors may contain
/// fragments of the encrypted strings but should not contain decrypted plaintext.
#[allow(unused)] // Will be used by future changes to support cipher versioning.
pub(crate) fn populate_cipher_types(&mut self) -> Result<(), VaultParseError> {

---

4. 🎨 Missing deny_unknown_fields on Cipher Type Structs

Location:

• card.rs:14-25 (has rename_all but not deny_unknown_fields)
• login.rs:282-296 (has rename_all but not deny_unknown_fields)
• ssh_key.rs:17-28 (has rename_all but not deny_unknown_fields)
• identity.rs:14-37 (has rename_all but not deny_unknown_fields)

The Cipher and CipherView structs use #[serde(rename_all = "camelCase", deny_unknown_fields)] but the individual cipher type structs only have rename_all. This inconsistency could mask data format changes.

Recommendation: Add deny_unknown_fields to all cipher type structs for consistency:

#[derive(Serialize, Deserialize, Debug, Clone)]
#[serde(rename_all = "camelCase", deny_unknown_fields)]  // Add deny_unknown_fields

---

5. 🤔 Test Coverage Gaps

Location: cipher.rs:1446-1812

Test coverage is excellent (97.91%) but missing coverage for:

• Error.rs: 7 lines uncovered (likely the From impl at lines 39-48)
• Cipher.rs: 1 line uncovered (likely line 349 - the TODO about data field)

Missing test scenarios:

• Mixed invalid/valid scenarios (e.g., valid JSON but wrong cipher type structure)
• Behavior when data field contains partial/corrupted encrypted strings
• Round-trip test: parse with populate_cipher_types(), encrypt, check data field state

Recommendation:

#[test]
fn test_populate_cipher_types_wrong_structure() {
    let mut cipher = /* ... setup cipher with type=Login ... */;
    cipher.data = Some(r#"{"cardholderName": "..."}"#.to_string()); // Card data for Login type
    
    let result = cipher.populate_cipher_types();
    assert!(matches!(result, Err(VaultParseError::SerdeJson(_))));
}

#[test]
fn test_populate_then_encrypt_data_field() {
    // Test what happens to data field after populate + encrypt
    let mut cipher = /* ... with valid data ... */;
    cipher.populate_cipher_types().unwrap();
    
    let cipher_view = key_store.decrypt(&cipher).unwrap();
    let cipher_enc = key_store.encrypt(cipher_view).unwrap();
    
    // Document expected behavior: data is None, Some(original), or Some(regenerated)?
    assert!(cipher_enc.data.is_none()); // or assert_eq!?
}

---

6. 🌱 Future: Consider Using #[serde(flatten)] for Type Fields

Location: cipher.rs:125-130

Currently, the Cipher struct has separate Optional fields for each cipher type. This works but creates some redundancy with the r#type discriminant.

Future consideration: After this PR stabilizes, consider refactoring to an enum:

#[serde(tag = "type", content = "data", rename_all = "camelCase")]
pub enum CipherData {
    Login(Login),
    SecureNote(SecureNote),
    Card(Card),
    Identity(Identity),
    SshKey(SshKey),
}

This would make the type system enforce the invariant that exactly one cipher type is present. However, this is a significant refactor and shouldn't block this PR.

---

Good Practices Observed

• ✅ Comprehensive test coverage for all cipher types with realistic encrypted test data
• ✅ Proper error propagation using ? operator and custom error types
• ✅ Consistent use of require! macro for required fields
• ✅ Test data uses actual EncString format rather than mock values
• ✅ Tests cover null/missing data scenarios
• ✅ Proper separation of concerns (parsing logic in domain model, not in API client)

---

Action Items for Author

Must address before merge:

1. ⚠️ Sanitize error messages from populate_cipher_types() to prevent information disclosure (Dependency Dashboard #1)
2. ⚠️ Resolve TODO at line 349 - clarify and implement expected behavior for data field during encryption ([PM-13376] Phase 2 - SDK split #2)
3. 📝 Add comprehensive documentation to populate_cipher_types() (Rename android artifact to -temp #3)

Should address:
4. Add deny_unknown_fields to cipher type structs (#4)
5. Add test coverage for edge cases (#5)

Future consideration:
6. Evaluate enum-based approach for cipher types (#6)

---

Status of Previously Identified Issues

From gbubemismith's review:

• ❓ Error message format: Addressed above in Dependency Dashboard #1 - needs attention
• ❓ Documentation on unused function: Addressed above in Rename android artifact to -temp #3 - needs addition
• 👍 Test helpers noted as helpful

---

[github-actions]

Checkmarx One – Scan Summary & Details – 5fde5709-ace0-43d3-8892-a06a2a1652a1

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

[codecov]

Codecov Report

❌ Patch coverage is 97.92208% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.62%. Comparing base (d553d37) to head (02cdfa1).

Files with missing lines	Patch %	Lines
crates/bitwarden-vault/src/error.rs	0.00%	7 Missing ⚠️
crates/bitwarden-vault/src/cipher/cipher.rs	99.73%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #517      +/-   ##
==========================================
+ Coverage   78.37%   78.62%   +0.25%     
==========================================
  Files         291      291              
  Lines       29391    29775     +384     
==========================================
+ Hits        23034    23410     +376     
- Misses       6357     6365       +8     

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

[gbubemismith]

This looks really good. I just have a few questions

[gbubemismith]

@nikwithak I think we can target main directly

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
1.2% Duplication on New Code

See analysis details on SonarQube Cloud

[coroiu]

I don't think platform owns any of this code anymore, but I'll approve anyways :)