[PM-27315] Add account cryptographic state service

[quexten]

🎟️ Tracking

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

📔 Objective

Adds a new service that holds the account cryptographic state and saves it to disk. It is intended to move over the SDK as a subscriber in a later step, and to replace the observables and state of key-service with a subscription on top of this one atomic state.

📸 Screenshots

⏰ 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[bot]]

Checkmarx One – Scan Summary & Details – 5d77c03c-e6f0-4e7e-871d-7c362dc94689

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

[claude[bot]]

Claude finished @quexten's task in 3m 59s —— View job

---

Code Review Complete

• [x] Code review completed using bitwarden-code-reviewer agent
• [x] Posted detailed review findings

The bitwarden-code-reviewer agent has completed a thorough review of this PR and posted inline comments and a summary directly to the PR. The review findings focus on security, correctness, and adherence to Bitwarden engineering standards.

[codecov[bot]]

Codecov Report

:x: Patch coverage is 84.44444% with 7 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 41.92%. Comparing base (3d06668) to head (04a8c59). :white_check_mark: All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
libs/angular/src/services/jslib-services.module.ts	0.00%	2 Missing :warning:
...-management/keys/response/private-keys.response.ts	66.66%	1 Missing and 1 partial :warning:
apps/browser/src/background/main.background.ts	50.00%	1 Missing :warning:
apps/desktop/src/app/services/services.module.ts	0.00%	1 Missing :warning:
...ryptography/account-cryptographic-state.service.ts	0.00%	1 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #17589      +/-   ##
==========================================
+ Coverage   41.90%   41.92%   +0.01%     
==========================================
  Files        3591     3593       +2     
  Lines      104147   104191      +44     
  Branches    15707    15716       +9     
==========================================
+ Hits        43640    43678      +38     
- Misses      58647    58653       +6     
  Partials     1860     1860              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[claude[bot]]

Overall Assessment: REQUEST CHANGES

Critical Issues:

• libs/common/src/key-management/account-cryptography/default-account-cryptographic-state.service.ts:13 - Deserializer lacks validation for cryptographic state
• libs/common/src/key-management/keys/response/private-keys.response.ts:77 - Redundant error throwing in unreachable code path

Important Issues:

• Multiple login strategies - Missing error handling when setting account cryptographic state
• libs/common/src/platform/sync/default-sync.service.ts:244 - Inconsistent null handling for accountKeys

---

Finding 1: Deserializer accepts any object without validation

Severity: ❌ CRITICAL
Location: libs/common/src/key-management/account-cryptography/default-account-cryptographic-state.service.ts:13

Details and fix

The deserializer casts any object to WrappedAccountCryptographicState without validation:

deserializer: (obj) => obj as WrappedAccountCryptographicState,

This means corrupted or malicious data from disk storage could be loaded as valid cryptographic state, potentially causing:

• Runtime errors when SDK expects valid structure
• Security vulnerabilities if malformed keys are used
• Silent failures during decryption operations

Fix: Add validation in the deserializer:

deserializer: (obj) => {
  if (obj == null || typeof obj !== "object") {
    return null;
  }
  // Validate V1 or V2 structure exists
  if (!("V1" in obj) && !("V2" in obj)) {
    return null;
  }
  return obj as WrappedAccountCryptographicState;
},

This is especially critical for cryptographic state stored on disk with CRYPTO_DISK storage.

---

Finding 2: Redundant error in unreachable else branch

Severity: ❌ CRITICAL
Location: libs/common/src/key-management/keys/response/private-keys.response.ts:76-78

Details and fix

Lines 76-78 contain an else branch that throws an error, but this is logically unreachable due to the validation in the constructor (lines 48-55):

} else {
  throw new Error("Both signatureKeyPair and securityState must be present or absent together");
}

The constructor already validates this invariant at lines 48-55, making this else branch unreachable. However, this creates a maintainability risk: if someone modifies the constructor validation without realizing this method depends on it, the error could become reachable.

Fix: Remove the else branch entirely and add a TypeScript assertion:

toWrappedAccountCryptographicState(): WrappedAccountCryptographicState {
  if (this.signatureKeyPair === null && this.securityState === null) {
    // V1 user
    return {
      V1: {
        private_key: this.publicKeyEncryptionKeyPair.wrappedPrivateKey,
      },
    };
  }
  
  // V2 user - constructor guarantees both are non-null here
  return {
    V2: {
      private_key: this.publicKeyEncryptionKeyPair.wrappedPrivateKey,
      signing_key: this.signatureKeyPair!.wrappedSigningKey,
      signed_public_key: this.publicKeyEncryptionKeyPair.signedPublicKey as SignedPublicKey,
      security_state: this.securityState!.securityState as string,
    },
  };
}

The ! non-null assertions document that the constructor guarantees these values are non-null when we reach this branch.

---

Finding 3: Missing error handling when setting cryptographic state

Severity: ⚠️ IMPORTANT
Location: Multiple login strategies (password-login.strategy.ts:160-164, sso-login.strategy.ts:342-348, etc.)

Details and fix

All login strategies call setAccountCryptographicState without error handling:

if (response.accountKeysResponseModel) {
  await this.accountCryptographicStateService.setAccountCryptographicState(
    response.accountKeysResponseModel.toWrappedAccountCryptographicState(),
    userId,
  );
}

If this fails (disk write error, storage quota exceeded, corrupted data), the login will appear successful but the user's cryptographic state won't be saved. This could prevent:

• Future SDK initialization
• Vault decryption on next login
• Organization key management

Fix: Add error handling and consider whether login should fail or log a warning:

if (response.accountKeysResponseModel) {
  try {
    await this.accountCryptographicStateService.setAccountCryptographicState(
      response.accountKeysResponseModel.toWrappedAccountCryptographicState(),
      userId,
    );
  } catch (error) {
    this.logService.error("Failed to set account cryptographic state during login", error);
    // Consider: Should this fail the entire login?
    // throw error;
  }
}

---

Finding 4: Inconsistent null handling for accountKeys

Severity: ⚠️ IMPORTANT
Location: libs/common/src/platform/sync/default-sync.service.ts:243-271

Details and fix

The sync service has a comment about cleaning up once the server always returns accountKeys (line 242), but the fallback logic (lines 269-271) doesn't set cryptographic state:

// Cleanup: Only the first branch should be kept after the server always returns accountKeys
if (response.accountKeys != null) {
  await this.accountCryptographicStateService.setAccountCryptographicState(...);
  // ... set keys
} else {
  await this.keyService.setPrivateKey(response.privateKey, response.id);
  // ⚠️ No cryptographic state set here
}

This creates an inconsistent state where:

• Login strategies always set cryptographic state (if available)
• Sync only sets it when accountKeys is present
• Old API responses won't have cryptographic state after sync

Implications:

1. Users on older server versions won't get cryptographic state from sync
2. SDK subscribers expecting cryptographic state will get null
3. Inconsistent state between login and sync paths

Decision needed:

• Is it acceptable for older API responses to skip cryptographic state?
• Should the else branch construct cryptographic state from privateKey?
• When will the server migration complete (PM-21768)?

If old responses must be supported long-term, consider constructing V1 cryptographic state in the else branch.

---

Finding 5: Test coverage for error paths is missing

Severity: 🎨 SUGGESTED
Location: libs/common/src/key-management/account-cryptography/default-account-cryptographic-state.service.spec.ts

Improvement suggestion

The test suite has good coverage for happy paths but is missing:

1. Deserialization of invalid data (if validation is added per Finding 1):

   • Null/undefined input
   • Non-object input
   • Object without V1/V2 keys
   • Malformed V1/V2 structures

2. Storage errors:

   • What happens when setUserState fails?
   • Quota exceeded scenarios

3. Concurrent updates:

   • Multiple calls to setAccountCryptographicState for the same user
   • Observable behavior during concurrent updates

Adding these tests would ensure robustness, especially for cryptographic state that can't be easily recovered if lost.

Example test to add:

it("handles storage errors gracefully", async () => {
  stateProvider.setUserState = jest.fn().mockRejectedValue(new Error("Storage full"));
  
  await expect(
    service.setAccountCryptographicState(mockState, mockUserId)
  ).rejects.toThrow("Storage full");
  
  // Verify state wasn't partially written
  const result = await firstValueFrom(service.accountCryptographicState$(mockUserId));
  expect(result).toBeNull();
});

---

Finding 6: PrivateKeysResponseModel validation is thorough

Severity: ❓ QUESTION
Location: libs/common/src/key-management/keys/response/private-keys.response.ts:17-56

Clarification needed

The constructor has excellent validation for the response structure, including type checking and the invariant that signatureKeyPair and securityState must be present or absent together (lines 48-55).

Question: Should this validation also check that wrappedPrivateKey and wrappedSigningKey (when present) are non-empty strings? Currently, it only validates they are objects, but empty/malformed encrypted keys could cause issues when the SDK tries to use them.

If the SDK handles validation, this may not be necessary. Just confirming the intended validation boundary.

---

Positive Observations

While I don't create praise-only comments, I want to acknowledge several good patterns in this PR:

1. Strong type safety: Using SDK types (WrappedAccountCryptographicState) prevents type mismatches
2. Clear separation: V1/V2 encryption handling is explicit and well-documented
3. Observable pattern: Follows ADR-0003 with accountCryptographicState$ observable
4. Consistent integration: All login strategies properly integrate the new service
5. Test coverage: Good baseline test coverage for the service (though error paths need work per Finding 5)

The architecture is sound; the findings above are about hardening edge cases and error handling for production reliability.

[bw-ghapp[bot]]

⚠️ Changes in this PR impact the Autofill experience of the browser client ⚠️

BIT has tested the core experience with these changes and the feature flag configuration used by vault.bitwarden.com

[!CAUTION] Unfortunately, one or more of these tests failed. 😞

Please resolve the failure before merging; reach out to @bitwarden/team-autofill-dev if you'd like help.

You can view the detailed results of the tests here.

[bw-ghapp[bot]]

Changes in this PR impact the Autofill experience of the browser client

BIT has tested the core experience with these changes and all feature flags disabled.

✅ Fortunately, these BIT tests have passed! 🎉

[quexten]

Looks like main being broken is failing the CI.