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 9 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

[Hinton]

I still have reservations around mobile handling this in the conversion layer and web handles this by loosely parsing serialized json it's inconsistent. As we shift the response parsing into the SDK this will stop working since we won't manually parse the json and will need to handle this in the impl From<SecureNoteResponse> for SecureNote.

Once we have data migrations one of the first migrations should be to get rid of notes with values other than 1 since this will cause headache should we ever want another note type.

Also please ensure Android is updated to handle this gracefully as I understand iOS parses unknown to 1, but Android throws errors.