[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

Completed comprehensive review of PR #517: Adding data field to Cipher type with client-side parsing

---

Summary of Changes

This PR introduces a new data field to the Cipher struct and implements client-side parsing via the populate_cipher_types() method. The function currently sits unused (marked with #[allow(unused)]) and is intended for future cipher versioning support. The implementation adds 434 lines across 10 files with excellent test coverage (97.92%).

---

Critical Issues

1. Security: No EncString Validation in Error Messages ⚠️

Location: crates/bitwarden-vault/src/cipher/cipher.rs:461-475

The populate_cipher_types() function could potentially leak encrypted data through error messages. While testing shows serde_json errors include the EncString representation, we should verify this doesn't expose sensitive patterns.

From PR comments, error messages look like:

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

Recommendation: While the current error handling appears safe (EncString parse errors don't expose raw values), consider adding explicit error wrapping to guarantee no sensitive data leakage:

Suggested improvement

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),
        crate::CipherType::SecureNote => serde_json::from_str(data).map(|v| self.secure_note = v),
        crate::CipherType::Card => serde_json::from_str(data).map(|v| self.card = v),
        crate::CipherType::Identity => serde_json::from_str(data).map(|v| self.identity = v),
        crate::CipherType::SshKey => serde_json::from_str(data).map(|v| self.ssh_key = v),
    };
    
    result.map_err(|e| {
        // Sanitize error to avoid leaking data content
        VaultParseError::SerdeJson(serde_json::Error::custom("Failed to parse cipher data"))
    })
}

However, this may reduce debuggability. Consider if the current approach is acceptable given that EncString errors are already sanitized.

2. Incomplete Symmetry: Missing data Population on Encryption 🤔

Location: crates/bitwarden-vault/src/cipher/cipher.rs:349

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

This creates an asymmetry: we can parse data → cipher types, but we don't serialize cipher types → data. This could lead to data loss if a cipher is decrypted, modified, and re-encrypted.

Impact: If the data field is the source of truth (as intended for versioning), failing to repopulate it means:

• Modified ciphers lose their data field
• Potential inconsistency between data and individual cipher type fields
• Could break future version migration logic

Recommendation: Document the intended behavior clearly:

• Is data read-only (never repopulated)?
• Is data single-use (only for initial parsing)?
• Should data be maintained across encrypt/decrypt cycles?

If data should be maintained, implement serialization in the encryption path.

3. Type Safety: Match Statement Doesn't Use self.r#type

Location: crates/bitwarden-vault/src/cipher/cipher.rs:467-472

The match uses unqualified crate::CipherType::Login instead of matching on the enum value directly:

match &self.r#type {
    crate::CipherType::Login => self.login = serde_json::from_str(data)?,
    // ...
}

Issue: This is correct but could be more idiomatic:

match self.r#type {
    CipherType::Login => self.login = serde_json::from_str(data)?,
    CipherType::SecureNote => self.secure_note = serde_json::from_str(data)?,
    CipherType::Card => self.card = serde_json::from_str(data)?,
    CipherType::Identity => self.identity = serde_json::from_str(data)?,
    CipherType::SshKey => self.ssh_key = serde_json::from_str(data)?,
}

Minor point, but improves readability and is consistent with other code in the file (see lines 798-807).

---

Suggested Improvements

1. Test Coverage Gaps

Location: crates/bitwarden-vault/src/error.rs:39-48

The VaultParseError conversion implementation has 0% coverage according to codecov. While these conversions are straightforward, consider adding a test to ensure proper error propagation:

Suggested test

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn test_vault_parse_error_to_cipher_error() {
        let crypto_err = bitwarden_crypto::CryptoError::MissingField("test");
        let parse_err = VaultParseError::Crypto(crypto_err);
        let cipher_err: CipherError = parse_err.into();
        assert!(matches!(cipher_err, CipherError::Crypto(_)));
    }
}

2. Documentation: Function Purpose Needs Expansion

Location: crates/bitwarden-vault/src/cipher/cipher.rs:457-460

Current comment:

/// This replaces the values provided by the API in the `login`, `secure_note`, `card`,
/// `identity`, and `ssh_key` fields, relying instead on client-side parsing of the
/// `data` field.
#[allow(unused)] // Will be used by future changes to support cipher versioning.

Recommendation: Expand to include:

• When should this be called? (After deserialization? Only for certain cipher versions?)
• Safety: What happens if called twice?
• Side effects: Does this mutate existing type fields or only populate if None?
• Versioning strategy: How will this integrate with the versioning system?

3. Defensive Programming: Add Guard Against Double-Population

The current implementation unconditionally overwrites the cipher type fields. Consider whether this is intended:

pub(crate) fn populate_cipher_types(&mut self) -> Result<(), VaultParseError> {
    // Should we skip if already populated?
    if self.cipher_type_is_populated() {
        return Ok(());
    }
    
    let data = self.data.as_ref()...
}

This prevents accidental data loss if the function is called multiple times.

4. Consider Adding Validation

The function deserializes JSON but doesn't validate the deserialized data matches the cipher type:

match &self.r#type {
    crate::CipherType::Login => {
        let login: Option<Login> = serde_json::from_str(data)?;
        // Validate that login actually contains valid login data?
        self.login = login;
    }
    // ...
}

For future-proofing, consider adding validation that the deserialized type contains expected fields.

5. Consistency: deny_unknown_fields Attribute

Observation: Most structs in the codebase use #[serde(rename_all = "camelCase", deny_unknown_fields)], but the Cipher struct uses #[serde(rename_all = "camelCase", deny_unknown_fields)] while Card uses only #[serde(rename_all = "camelCase")] (line 15 in card.rs).

Since the data field will contain serialized cipher type data, ensure that all cipher types consistently handle unknown fields. This is important for forward compatibility during versioning.

---

Good Practices Observed

• Comprehensive testing: Excellent test coverage with tests for all cipher types
• Error handling: Proper error propagation via VaultParseError
• Feature flagging mindset: Function marked as unused with clear intent for future use
• Type safety: Using Rust's type system effectively with enum matching
• Documentation: PR clearly explains the feature is unused and waiting on [PM-25818] Migrate Basic Cipher Create, Edit, and Get Operations to SDK #455

---

Backward Compatibility Assessment ✅

Per CLAUDE.md requirement: This change maintains backward compatibility:

1. Serialization compatibility: The data field is Option<String>, so existing ciphers without it continue to work
2. Decryption compatibility: Existing decryption logic unchanged (lines 354-403)
3. API compatibility: The populate_cipher_types() function is internal (pub(crate)) and unused
4. No breaking changes: All changes are additive

Concern: The TODO at line 349 (data: None) means re-encrypted ciphers lose their data field. This is acceptable only if:

• The data field is intended as read-only/single-use
• Server re-sends data on every sync
• Or, this is a temporary state until full versioning support

---

Security Assessment

Areas Reviewed:

1. ✅ No secrets in logs: Error handling doesn't log raw data field content
2. ✅ Encrypted data handling: The data field contains JSON with EncStrings, properly typed
3. ⚠️ Error message safety: See Critical Issue Dependency Dashboard #1 above
4. ✅ No crypto changes: No modifications to encryption/decryption algorithms
5. ✅ No unsafe blocks: All code is safe Rust

Minor Concern:

The data field is stored as String rather than a more structured type. While this provides flexibility for versioning, it means:

• No compile-time validation of content structure
• Potential for malformed JSON to cause runtime errors
• Risk of forgetting to handle new cipher types in future versions

Consider adding a CipherData enum wrapper in the future to provide type safety.

---

Action Items for Author

1. Critical: Decide on and document the data field lifecycle:

   • Should it be repopulated on encryption? (Line 349)
   • Is it read-only after initial parse?
   • How does it interact with versioning?

2. Important: Consider adding sanitization wrapper for parse errors to guarantee no data leakage

3. Nice to have: Add guard against double-population of cipher types

4. Nice to have: Add tests for VaultParseError → CipherError conversion

5. Documentation: Expand the function doc comment to explain when/how to use

---

Overall Assessment

This is a well-implemented, well-tested addition that properly sets up infrastructure for future cipher versioning. The code quality is high, test coverage is excellent, and the implementation follows Rust best practices.

The main concerns are around the intended lifecycle of the data field and ensuring it doesn't introduce data loss or inconsistency when ciphers are modified. Once the TODO at line 349 is addressed with a clear decision, this PR will be in excellent shape.

Recommendation: Approve pending clarification on the data field lifecycle and encryption symmetry.

---

[github-actions]

Checkmarx One – Scan Summary & Details – 6c4c2732-0058-4588-9381-d99a7ea60b95

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 :)