Accept SecureNote with Type 1 and set to 0

[Jingo88]

🎟️ Tracking

PM-27214

📔 Objective

Some users have Notes with a Type:1 in their data. This was breaking some vaults as failures were returning. Changes allow for Cipher Notes to have Type:1 and setting them to 0

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

---

Code Review Complete

• Read repository guidelines (CLAUDE.md)
• Review changed files and implementation
• Analyze backward compatibility implications
• Address reviewer concerns about mobile apps and future types
• Evaluate test coverage
• Provide comprehensive feedback

---

Summary of Changes

This PR modifies SecureNote deserialization to accept Type:1 values and silently convert them to Type:0 (Generic). The change replaces the derived Deserialize_repr with a custom deserializer that treats any non-zero value as Generic.

Commits since last review: 2 commits

• da0c62c0 - Main implementation
• 6e6c310d - Removed excess comment

---

Critical Issues

⚠️ 1. Silently Swallowing Deserialization Errors (Lines 38-52)

Err(_) => Ok(SecureNoteType::Generic),

The custom deserializer returns Ok(SecureNoteType::Generic) for all deserialization errors, not just unknown type values. This masks legitimate deserialization failures.

Problems:

• If the type field contains invalid data (e.g., string, null, negative number, out-of-range integer), it silently succeeds
• Violates fail-fast principle - corrupt data propagates through the system
• Makes debugging production issues extremely difficult
• No logging/telemetry when data migration occurs

Recommendation:

impl<'de> Deserialize<'de> for SecureNoteType {
    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
    where
        D: Deserializer<'de>,
    {
        let value = u8::deserialize(deserializer)?;  // Propagate actual errors
        match value {
            0 => Ok(SecureNoteType::Generic),
            1 => {
                // Known legacy type - explicitly handle Type 1
                log::debug!("Migrating SecureNote Type 1 to Generic");
                Ok(SecureNoteType::Generic)
            }
            unknown => {
                log::warn!("Unknown SecureNoteType: {}", unknown);
                Err(serde::de::Error::custom(format!(
                    "Unknown SecureNoteType: {}. Expected 0 or 1.", 
                    unknown
                )))
            }
        }
    }
}

⚠️ 2. Violates CLAUDE.md Serialization Guidelines

From CLAUDE.md line 52-54:

Breaking serialization

• Backward compatibility required. Users must decrypt vaults from older versions.

Current issue: The custom deserializer accepts Type 1 on read, but the serializer (via Serialize_repr) will write Type 0 back. This creates a one-way data transformation without user awareness.

Scenarios of concern:

1. User with Type 1 notes opens vault in this SDK → silently converted to Type 0
2. User expects to preserve original Type value but it's lost permanently
3. If Type 1 had semantic meaning historically, that information is destroyed

Missing from PR:

• No explanation of what Type 1 originally meant
• No confirmation this is safe to discard
• No audit of whether any client code depends on Type 1

❓ 3. Hinton's Valid Concern About Future Type Handling (Review Comment)

Hinton correctly points out:

"a future type can not generally be mapped to an existing type without potential data loss"

Your comment states:

// This allows the SDK to handle future secure note types gracefully

This is misleading - the implementation doesn't handle future types gracefully, it silently discards them. If a Type 2 is introduced in the future with different behavior/schema, this deserializer would destroy that information.

Recommendation: Remove the misleading comment. Replace with accurate description:

/// Legacy data migration for Type 1 secure notes
/// Historical context: Type 1 was [REASON], but is now deprecated.
/// All Type 1 notes are converted to Generic (Type 0) on load.
/// 
/// Note: This migration is permanent - the original type value is not preserved.

🤔 4. Missing Root Cause Analysis

Unanswered questions:

1. How did Type 1 notes end up in user vaults?
2. What did Type 1 represent historically?
3. Why did mobile apps not encounter this issue? (per Hinton's question)
4. Are there Type 2+ values in the wild?
5. Is this a server-side data issue that should be fixed at the source?

The PR description says "Some users have Notes with a Type:1" but doesn't explain why or how many users are affected.

---

Suggested Improvements

🎨 1. Zero Test Coverage for New Code Path

Codecov correctly reports 0% coverage for the new deserializer (10 lines untested). The existing tests in lines 164-197 only test CipherKind behavior, not deserialization.

Missing test cases:

#[test]
fn test_deserialize_type_0() {
    let json = r#"{"type": 0}"#;
    let note: SecureNote = serde_json::from_str(json).unwrap();
    assert!(matches!(note.r#type, SecureNoteType::Generic));
}

#[test]
fn test_deserialize_type_1_migrates_to_generic() {
    let json = r#"{"type": 1}"#;
    let note: SecureNote = serde_json::from_str(json).unwrap();
    assert!(matches!(note.r#type, SecureNoteType::Generic));
}

#[test]
fn test_deserialize_invalid_type_rejects() {
    let json = r#"{"type": 255}"#;
    let result: Result<SecureNote, _> = serde_json::from_str(json);
    assert!(result.is_err());
}

#[test]
fn test_deserialize_non_numeric_type_rejects() {
    let json = r#"{"type": "invalid"}"#;
    let result: Result<SecureNote, _> = serde_json::from_str(json);
    assert!(result.is_err());
}

🎨 2. Inconsistent Error Handling Pattern

The codebase uses VaultParseError (line 15, 88) for vault data issues, but your deserializer returns Ok() for all cases. Consider:

// In TryFrom<CipherSecureNoteModel>:
fn try_from(model: CipherSecureNoteModel) -> Result<Self, Self::Error> {
    let r#type = require!(model.r#type);
    if let bitwarden_api_api::models::SecureNoteType::Generic = r#type {
        Ok(Self { r#type: SecureNoteType::Generic })
    } else {
        // Handle Type 1 here with proper error type
        Ok(Self { r#type: SecureNoteType::Generic })
    }
}

Actually, the current flow is:

1. CipherSecureNoteModel deserializes from API using bitwarden_api_api::models::SecureNoteType
2. That gets converted to vault::SecureNoteType via TryFrom (line 97-102)

The problem is in step 1 - the API model's SecureNoteType enum only has Generic = 0, so it can't deserialize Type 1.

🎨 3. Architectural Layering Violation

Looking at the API model flow:

API JSON → bitwarden_api_api::models::SecureNoteType (auto-generated, line 20 only has Generic)
         → CipherSecureNoteModel (line 18 uses API SecureNoteType)
         → vault::SecureNoteType (your custom deserializer)

The issue: You're adding the custom deserializer to the vault type, but the API type will fail deserialization first if Type 1 exists in JSON.

This PR may not actually fix the issue - the API layer's auto-generated enum will reject Type 1 before your custom deserializer runs.

🎨 4. Need for Integration Test

Add a test that deserializes a complete cipher JSON with Type 1:

#[test]
fn test_cipher_with_legacy_type1_secure_note() {
    let json = r#"{
        "id": "test-id",
        "type": 2,
        "secureNote": {
            "type": 1
        },
        ...
    }"#;
    let result = Cipher::deserialize(json);
    // Verify it doesn't error and converts Type 1 to Generic
}

---

Good Practices Observed

✅ Removed Deserialize_repr derive to implement custom logic
✅ Preserved backward compatibility for Type 0
✅ Removed excess comment in follow-up commit (demonstrates responsiveness)

---

Action Items for Author

1. CRITICAL: Fix error swallowing - only accept Type 0 and Type 1, reject unknown values
2. CRITICAL: Add comprehensive deserialization tests (at minimum: Type 0, Type 1, invalid values)
3. HIGH: Verify the API layer (bitwarden_api_api::models::SecureNoteType) can actually deserialize Type 1 - your change may be in the wrong layer
4. HIGH: Add logging/telemetry when Type 1 is encountered for production monitoring
5. MEDIUM: Update comment to accurately describe legacy migration (not "future types")
6. MEDIUM: Document what Type 1 historically represented and why it's safe to discard
7. MEDIUM: Answer Hinton's question about mobile app behavior
8. LOW: Consider adding metrics to track how many users have Type 1 notes

---

Response to Reviewer Concerns

@Hinton's questions:

Do you know how the mobile apps currently handles this?

This needs investigation. Possible explanations:

• Mobile apps might use the same "accept and convert" pattern
• Mobile apps might have a different deserialization path
• Mobile apps might not have encountered this data in their user base
• The issue might be specific to SDK usage patterns

I don't quite agree on this [handling future types gracefully]

Fully agree with Hinton. The current implementation doesn't handle future types gracefully - it silently destroys them. The comment is misleading and should be corrected per my suggestions above.

---

📋 Detailed Code References

Key Files Analyzed

• crates/bitwarden-vault/src/cipher/secure_note.rs - Modified file (lines 38-52 new deserializer)
• crates/bitwarden-api-api/src/models/secure_note_type.rs - Auto-generated API model (only defines Generic = 0)
• crates/bitwarden-api-api/src/models/cipher_secure_note_model.rs - API model using API SecureNoteType

Relevant CLAUDE.md Guidelines

• Line 52-54: Breaking serialization requires backward compatibility
• Line 22-25: Auto-generated code changes are generally discouraged

---

[github-actions]

Checkmarx One – Scan Summary & Details – f13d7ed5-7151-4b23-91ce-896b2be572a3

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

[codecov]

Codecov Report

❌ Patch coverage is 0% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.34%. Comparing base (2a00b72) to head (6e6c310).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/bitwarden-vault/src/cipher/secure_note.rs	0.00%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             main     #521     +/-   ##
=========================================
  Coverage   78.34%   78.34%             
=========================================
  Files         287      291      +4     
  Lines       28095    29400   +1305     
=========================================
+ Hits        22010    23034   +1024     
- Misses       6085     6366    +281     

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

[Hinton]

Do you know how the mobile apps currently handles this? We haven't had any issues there and they've used this logic since those apps were built. I suspect they handle the failures on their mapping side.

// This allows the SDK to handle future secure note types gracefully

I don't quite agree on this, a future type can not generally be mapped to an existing type without potential data loss.

[Jingo88]

That's fair about the comment, it can be a little misleading. I'll remove it.

Currently iOS will default to 0. Android throws an exception.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

[Hinton]

Are we concerned about the different behaviour between mobile and clients? Mobile never goes through the json layer and the 0/1 types are handled in the mapping layer.

[Jingo88]

Are we concerned about the different behaviour between mobile and clients? Mobile never goes through the json layer and the 0/1 types are handled in the mapping layer.

@Hinton No real concerns, the behavior is already different between Mobile and Clients and this is bringing the behavior more in line with the iOS mapping that already automatically maps 1>0