[PM-30703] Introduce CXF payload parser and update to alpha04

[SaintPatrck]

🎟️ Tracking

PM-30703

📔 Objective

Update the Credential Exchange (CXF/CXP) implementation to align with the androidx.credentials.providerevents:1.0.0-alpha04 library. The previous implementation was based on alpha03 and is now obsolete.

The primary behavioral change is the removal of the two-layered JSON structure (CXP wrapping CXF). The alpha04 specification simplifies this to a single, direct JSON payload for credential exchange.

Specific changes include:

• Dependency Update:

  • Upgraded androidx.credentials.providerevents from 1.0.0-alpha03 to 1.0.0-alpha04.

• Payload Parsing:

  • Introduced a CredentialExchangePayloadParser to handle the new direct JSON payload structure. This parser validates the CXF version, checks for the presence of accounts, and handles various error conditions.
  • The CredentialExchangeImportManager is refactored to use this new parser, removing the now-unnecessary logic for decoding the two-layered, base64-encoded payload.

• Import/Export Flow:

  • CredentialExchangeImporterImpl: Updated to use the modern ImportCredentialsRequest builder, which no longer requires manual JSON construction.
  • CredentialExchangeCompletionManagerImpl: Modified to return the new, simplified direct JSON response, removing the CXP wrapper and base64 encoding.
  • CredentialExchangeRegistryImpl: Aligned with alpha04 by including a default export_matcher.bin (a WASM binary from Google) in the registration request.

• DI and Module Structure:

  • Added a new CxfModule to provide the CredentialExchangePayloadParser.
  • Updated build.gradle.kts for the cxf module to include Hilt dependencies for DI.

• Testing:

  • Added extensive unit tests for the new CredentialExchangePayloadParser to cover valid payloads, version checks, and various error states.
  • Updated existing tests for CredentialExchangeImporterTest and CredentialExchangeImportManagerTest to reflect the API and structural changes from the library update.

⏰ 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

[github-actions]

Checkmarx One – Scan Summary & Details – 8847580e-481f-4cda-b464-083632966d99

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

[claude]

Claude finished @SaintPatrck's task in 2m 29s —— View job

---

Code Review Summary

This PR introduces a well-structured CXF payload parser and updates to alpha04. The architectural refactoring successfully centralizes parsing logic and improves maintainability. However, 3 critical/important issues require resolution before merge.

---

Critical Issues (Must Fix)

❌ Silent Multi-Account Data Loss (CredentialExchangePayloadParserImpl.kt:104)

• Parser silently drops all accounts except the first with no logging or user notification
• Users importing multiple accounts will lose data without realizing it
• Required: Add logging, document limitation, consider exposing dropped count

---

Important Issues (Should Fix)

⚠️ Incorrect Base64 URL Decoding (CredentialExchangePayloadParserImpl.kt:137)

• Missing padding restoration will cause decoding failures for certain payload lengths
• Implementation doesn't follow RFC 4648 Section 5 (Base64Url standard)
• Required: Restore padding (= characters) before decoding

⚠️ Lost Error Context in Upload (CredentialExchangeImportManagerImpl.kt:72)

• Server validation errors are discarded when converting to generic exception
• Makes debugging and support significantly harder
• Recommended: Log validation errors or create custom exception type

---

Strengths

✅ Excellent separation of concerns - Parser extraction from import manager follows SRP
✅ Comprehensive test coverage - 326 lines of parser tests covering V1/V2 formats and edge cases
✅ Proper dependency injection - New CxfModule follows Hilt patterns correctly
✅ Clean sealed class design - CredentialExchangePayload provides type-safe result handling
✅ Backward compatibility - V1 (legacy CXP) and V2 (direct CXF) formats both supported
✅ Security: Properly handles version validation and JSON parsing errors

---

Architecture Compliance

• ✅ Follows MVVM + UDF patterns
• ✅ Hilt DI with interface/impl split per guidelines
• ✅ Result types for error handling (no exceptions in business logic)
• ✅ Proper module organization (:cxf specialized library)
• ✅ Test coverage with JUnit 5 and MockK

---

Additional Observations

Binary Addition: export_matcher.bin (144KB WASM) is required by alpha04 API for export matching logic. This is a Google-provided binary from the identity-samples repo.

Breaking Changes: The ImportCredentialsRequest constructor change from alpha03 to alpha04 is properly handled (adds knownExtensions parameter).

Refactoring Impact: The test refactoring in CredentialExchangeImportManagerTest shows significant cleanup (291 additions, 427 deletions) - code is now more focused on business logic rather than parsing details.

---

Verdict

Requires Changes - The architectural improvements are excellent, but the data loss and decoding issues must be fixed before merge. The Base64 decoding bug could cause production failures for certain payloads, and silent multi-account data loss is a user trust issue.

Once the critical and important issues are addressed, this will be a solid improvement to the codebase.

---

Generated by Claude Code - Bitwarden Code Review Agent

[claude]

❌ CRITICAL: Silent multi-account data loss

The comment on line 101-102 states "silently discard additional accounts," but this is a security and data integrity concern:

Issues:

1. No user notification: Users have no way to know their additional accounts were dropped
2. No logging: No Timber log to track when this occurs in production
3. Documentation gap: The public CredentialExchangePayloadParser interface doesn't document this single-account limitation

Why this matters:
A user exporting multiple accounts from another password manager would see a successful import but silently lose all but the first account. This could lead to:

• Lost credentials they believe were imported
• Security incidents if they delete source data assuming import succeeded
• Support burden when users discover missing data

Required fixes:

1. Add Timber.w() logging when accounts.size > 1 to track occurrence
2. Update CredentialExchangePayloadParser KDoc to explicitly document single-account limitation
3. Consider returning a new CredentialExchangePayload variant like PartialImport(accountsJson: String, droppedAccountCount: Int) to inform callers

[claude]

⚠️ IMPORTANT: Inconsistent error handling loses context

The uploadCiphers function converts specific ImportCiphersResponseJson.Invalid responses into a generic ImportCredentialsUnknownErrorException, losing valuable error context.

Problems:

1. Lost validation errors: Invalid contains a validationErrors map with specific field-level errors, but this is completely discarded
2. Debugging difficulty: Support and developers lose insight into why the import failed on the server side
3. User experience: Generic errors provide no actionable feedback

Recommended fix:

is ImportCiphersResponseJson.Invalid -> {
    // Log validation errors for debugging
    Timber.w("Import ciphers validation failed: ${response.validationErrors}")
    Result.failure(
        ImportCredentialsUnknownErrorException(
            "Server validation failed: ${response.validationErrors.keys.joinToString()}"
        )
    )
}

Alternatively, consider creating a custom exception type that preserves the validation errors for upstream handling.

[codecov]

Codecov Report

❌ Patch coverage is 97.67442% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.49%. Comparing base (d86959b) to head (9de5ff0).

Files with missing lines	Patch %	Lines
...den/cxf/importer/CredentialExchangeImporterImpl.kt	86.66%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6347      +/-   ##
==========================================
- Coverage   85.49%   85.49%   -0.01%     
==========================================
  Files         764      765       +1     
  Lines       54724    54714      -10     
  Branches     7881     7881              
==========================================
- Hits        46785    46775      -10     
  Misses       5201     5201              
  Partials     2738     2738              

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