[PM-12436] replace node keytar

[JaredScar]

🎟️ Tracking

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

📔 Objective

Rid of node keytar from Directory Connector

[claude]

Claude finished @JaredScar's task in 5m 0s —— View job

---

Code Review Summary - PR #1012: PM-12436 replace node keytar

Overview

This PR replaces the deprecated node-keytar package with a custom Rust-based native module (dc-native) that wraps desktop_core for OS keychain access. It includes a Windows-specific migration for converting credentials from keytar's UTF-8 format to desktop_core's UTF-16 format, updates the CI/CD pipeline to compile Rust instead of downloading prebuilt keytar binaries, and bumps Node from v20 to v22.

Findings

Severity	Finding	Location
⚠️ IMPORTANT	PASSWORD_NOT_FOUND error string is hardcoded in JS and may not match the actual error message from desktop_core, causing get() to throw instead of returning null	native/index.js:80
⚠️ IMPORTANT	Windows credential migration only covers 6 directory-service credential keys but omits other keys (syncConfig, directoryType, organizationId, directoryConfig_) that were also stored in secure storage by the v1-to-v2 migration	src/services/stateMigration.service.ts:194-201
🎨 SUGGESTED	Service name "Bitwarden Directory Connector" is hardcoded in 3 separate locations; a shared constant would prevent drift	electronRendererSecureStorage.service.ts:7

Additional Observations

• No tests for new code: NativeSecureStorageService, ElectronRendererSecureStorageService refactor, and both new migration methods (migrateStateFrom3To4, migrateStateFrom4To5) have no unit tests. Given this is a credential storage migration affecting existing users, regression test coverage would reduce risk of data loss.
• SonarQube security gate failed: The SonarQube analysis flagged a security rating issue on new code, which likely relates to the hardcoded error string comparison pattern used for credential access control flow.
• Node version bump: The .nvmrc and engines field change from Node 20 to Node 22 is a notable change that may affect CI environments and developer setups beyond this PR's scope.

What Looks Good

• The Rust migration logic in native/src/migration.rs correctly identifies keytar's UTF-8 format vs desktop_core's UTF-16 format using a null-byte heuristic, and the migration is idempotent.
• Proper use of asarUnpack in electron-builder.json to ensure the native .node binary is accessible at runtime.
• The scopeguard::defer! pattern in migration.rs ensures CredFree is called even on early returns.
• CI pipeline changes consistently replace keytar download steps with Rust compilation across all platforms.

[sonarqubecloud]

Quality Gate failed

Failed conditions
E Security Rating on New Code (required ≥ D)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

[github-actions]

Checkmarx One – Scan Summary & Details – 9bc412fe-240f-42bf-b796-0ef63e201940

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

[claude]

⚠️ IMPORTANT (Correctness): PASSWORD_NOT_FOUND is hardcoded as "Password not found." in this JavaScript wrapper, but the actual error message originates from desktop_core::password functions, propagated through napi::Error::from_reason(e.to_string()) in lib.rs. If desktop_core returns a different error string (e.g., "password not found", "No password found", or a platform-specific message), the comparison e.message === passwords.PASSWORD_NOT_FOUND in both NativeSecureStorageService and ElectronRendererSecureStorageService will never match.

The consequence is that get() will throw an error instead of returning null for missing keys, and remove() will throw instead of silently succeeding. This would break credential lookups for any key not yet stored and could cause startup failures.

Consider one of:

1. Export the constant from the Rust side so the string is defined in one place:

   #[napi]
   pub const PASSWORD_NOT_FOUND: &str = "Password not found.";

   Then reference it in index.js via nativeBinding.PASSWORD_NOT_FOUND.

2. Use a structured error approach (error codes instead of message string comparison) so the contract is not dependent on exact text matching.

[claude]

⚠️ IMPORTANT (Correctness): The credential migration only covers the 6 directory-service credential keys, but the migrateStateFrom1To2 method (lines 168-178) stores all SecureStorageKeys values in secure storage with a ${userId}_ prefix. This means the following keys may also exist in the Windows credential store in the old keytar UTF-8 format and would not be migrated:

• ${userId}_directoryConfig_ (from SecureStorageKeys.directoryConfigPrefix)
• ${userId}_syncConfig (from SecureStorageKeys.sync)
• ${userId}_directoryType (from SecureStorageKeys.directoryType)
• ${userId}_organizationId (from SecureStorageKeys.organizationId)

Reading these unmigrated credentials through desktop_core on Windows would produce garbled UTF-16 output, the same corruption this migration is designed to prevent.

Suggested change

	const credentialKeys = [
	SecureStorageKeys.ldap,
	SecureStorageKeys.gsuite,
	SecureStorageKeys.azure,
	SecureStorageKeys.entra,
	SecureStorageKeys.okta,
	SecureStorageKeys.oneLogin,
	];
	const credentialKeys = [
	SecureStorageKeys.ldap,
	SecureStorageKeys.gsuite,
	SecureStorageKeys.azure,
	SecureStorageKeys.entra,
	SecureStorageKeys.okta,
	SecureStorageKeys.oneLogin,
	SecureStorageKeys.directoryConfigPrefix,
	SecureStorageKeys.sync,
	SecureStorageKeys.directoryType,
	SecureStorageKeys.organizationId,
	];

[claude]

🎨 SUGGESTED: This service name is now hardcoded in three separate locations:

1. Here in ElectronRendererSecureStorageService
2. In stateMigration.service.ts line 190 (const serviceName = "Bitwarden Directory Connector")
3. In bwdc.ts line 63 (const applicationName = "Bitwarden Directory Connector")

If any of these drift out of sync, credentials written by one path would be invisible to another. Consider extracting this to a shared constant (e.g., in a constants file) to make the coupling explicit and prevent future inconsistencies.

[codecov]

Codecov Report

❌ Patch coverage is 0% with 38 lines in your changes missing coverage. Please review.
✅ Project coverage is 15.08%. Comparing base (984ae97) to head (216f57a).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/services/stateMigration.service.ts	0.00%	21 Missing ⚠️
src/services/nativeSecureStorage.service.ts	0.00%	16 Missing ⚠️
src/bwdc.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1012      +/-   ##
==========================================
- Coverage   15.11%   15.08%   -0.04%     
==========================================
  Files          67       67              
  Lines        2798     2804       +6     
  Branches      483      480       -3     
==========================================
  Hits          423      423              
- Misses       2271     2277       +6     
  Partials      104      104              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

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