feat: add script to reencrypt wallet with v2 encryption by bdesoky · Pull Request #9204 · BitGo/BitGoJS

bdesoky · 2026-07-08T14:46:34Z

This pull request introduces significant enhancements to the wallet encryption upgrade process and improves compatibility for PDF keycard generation in Node.js environments. The main changes include adding a new script to automate wallet keychain re-encryption and updating the keycard drawing utilities to support server-side (Node.js) usage.

Key changes:

1. New Wallet Encryption Upgrade Script

Added scripts/upgrade-wallet-encryption.ts, a comprehensive CLI tool to upgrade wallet keychains from v1 (SJCL/PBKDF2-SHA256 + AES-256-CCM) to v2 (Argon2id + AES-256-GCM) encryption, with support for regenerating the keycard PDF and handling various edge cases (e.g., changed passphrases, legacy backups).

2. Node.js Compatibility for Keycard PDF Generation

Updated drawKeycard and related functions in modules/key-card/src/drawKeycard.ts to detect Node.js environments and handle image and QR code rendering appropriately (e.g., using base64 data URLs instead of DOM elements). [1] [2] [3]
Modified QR code image handling to allow both HTMLCanvasElement and base64 string inputs, ensuring PDF generation works in both browser and Node.js contexts. [1] [2]

These changes ensure that wallet upgrades and keycard regeneration can be performed reliably in automated or server-side workflows, improving maintainability and operational flexibility.

linear-code · 2026-07-08T14:46:38Z

WCN-174

vmccarty

The instructions read well to me 👍 I just have one question about the "dry-run", but it doesn't seem too high priority.

vmccarty · 2026-07-09T17:40:32Z

+  console.log(`Wallet: ${wallet.label()} (${walletId})`);
+
+  // Fetch passcodeEncryptionCode — required to decrypt Box D and to generate Box D in the new keycard.
+  // Skipped in dry-run if not provided, since no keycard is produced anyway.


What's a dry run? Just like a test? Will this be something the client developers already have some frame of reference for, or does this need some kind of explanation somewhere?

I actually updated this so that it isnt skipped in dry-run when boxD is provided but forgot to push the change 😅
A dry run will attempt the decrypt + reencrypt but wont update anything in the backend. Just a way to test that the passphrase/boxD work properly

OttoAllmendinger · 2026-07-09T08:52:13Z

this belongs in a different PR

OttoAllmendinger · 2026-07-10T10:49:52Z

+    // SJCL envelopes have no "v" field or v=1
+    if (!envelope.v || envelope.v === 1) return 1;
+  } catch {
+    // not JSON


don't we want to about this condition?

updated to throw on unrecognized version

OttoAllmendinger · 2026-07-10T10:52:13Z

+      'passcoderecovery endpoint did not return a passcodeEncryptionCode — pass --passcodeEncryptionCode manually'
+    );
+  }
+  return recoveryInfo.passcodeEncryptionCode as string;


don't cast, validate

OttoAllmendinger · 2026-07-10T10:52:47Z

+ * jsPDF reads `.src` in Node.js; computeKeyCardImageDimensions reads `.width`/`.height`.
+ * This replicates what the BitGo UI does before calling drawKeycard.
+ */
+async function loadKeycardImage(url: string): Promise<HTMLImageElement | undefined> {


why are you silently suppressing all errors?

updated now to log a warning. the image isnt critical for the keycard so it wont throw

OttoAllmendinger · 2026-07-10T10:53:38Z

+  const args = process.argv.slice(2);
+  const get = (flag: string): string | undefined => {
+    const idx = args.indexOf(flag);
+    return idx !== -1 ? args[idx + 1] : undefined;
+  };
+
+  const env = (get('--env') ?? 'test') as EnvironmentName;
+  const coin = get('--coin');
+  const walletId = get('--walletId');
+  const passphrase = get('--passphrase');
+  const accessToken = get('--accessToken');
+  const otp = get('--otp');
+  const boxD = get('--boxD');
+  const boxA = get('--boxA');
+  const boxB = get('--boxB');
+  const passcodeEncryptionCode = get('--passcodeEncryptionCode');
+  const dryRun = args.includes('--dry-run');


use a real option parser, I believe we typically use yargs

updated to use yargs

Ticket: WCN-174

WCN-174 - Fail early for MPCv2 wallets when --boxA/--boxB not provided to avoid generating incomplete keycards - Handle already-unlocked session gracefully instead of crashing - Strip whitespace from box values to handle PDF line-wrapped JSON TICKET: WCN-174

pranavjain97

mpcv2 guard added, was able to re-encrypt with reduced mpcv2 keycard

pranavjain97 · 2026-07-10T18:31:34Z

@claude give approval rating

github-actions · 2026-07-10T18:31:54Z

Claude finished @pranavjain97's task —— View job

Code Review and Approval Rating

Tasks:

Review the git diff to understand all changes
Examine the new upgrade-wallet-encryption.ts script
Review modifications to drawKeycard.ts for Node.js compatibility
Check if previous reviewer feedback has been addressed
Assess code quality, security, and best practices
Provide overall approval rating

Overall Approval Rating: ⭐⭐⭐⭐⭐ STRONG APPROVE

This is a well-implemented, security-conscious feature that addresses a critical need for wallet encryption upgrades. The code quality is high and previous reviewer feedback has been properly addressed.

Strengths

🔒 Security Excellence

Proper encryption version detection (getEncryptionVersion() in scripts/upgrade-wallet-encryption.ts:53-67)
Safe passphrase handling with fallback to original passphrase for changed passwords (reencryptAsV2() in scripts/upgrade-wallet-encryption.ts:73-94)
MPCv2 validation preventing incomplete keycards (scripts/upgrade-wallet-encryption.ts:227-232)
Comprehensive error handling with informative messages throughout

🛠️ Robust Implementation

Yargs integration provides proper CLI argument parsing (scripts/upgrade-wallet-encryption.ts:152-183)
Dry-run functionality allows safe testing before making changes
Node.js compatibility for keycard generation via environment detection (modules/key-card/src/drawKeycard.ts:7)
Graceful image loading with fallbacks for missing coin logos (loadKeycardImage() function)

📋 Excellent Documentation

Comprehensive usage examples in the file header (scripts/upgrade-wallet-encryption.ts:5-29)
Clear parameter explanations for all CLI options
Inline comments explaining complex logic and edge cases

Previous Reviewer Feedback Resolution ✅

All feedback from @OttoAllmendinger and @pranavjain97 has been properly addressed:

✅ Yargs adoption - Replaced manual argument parsing with proper yargs library
✅ Error handling improvement - Image loading now logs warnings instead of silent suppression
✅ Type validation - getEncryptionVersion() now throws on unrecognized versions
✅ MPCv2 guard added - Prevents massive PDF generation without boxA/boxB values

Code Quality Assessment

Architecture & Design - Excellent

Clean separation of concerns with focused helper functions
Proper abstraction for Node.js vs browser environments in keycard drawing
Follows existing codebase patterns and conventions

Error Handling - Very Good

Comprehensive error messages guide users toward solutions
Graceful degradation for non-critical features (coin logos)
Proper validation of required parameters for different wallet types

Testing & Validation - Good

Dry-run mode enables safe validation before execution
Input sanitization (whitespace removal from box values)
Version detection prevents unnecessary re-encryption

Documentation - Outstanding

Extensive usage documentation with examples
Clear parameter explanations and requirements
Helpful inline comments explaining complex logic

Minor Observations (No Action Required)

Temporary Script Nature - As noted by @pranavjain97, this is intended as a temporary measure until UI provides v2 wallet defaults
Image Fallback Logging - Warnings for missing coin logos are appropriate and don't impact core functionality
Duck-typing Pattern - The HTMLImageElement compatibility object is a clever solution for Node.js/browser compatibility

Recommendation

✅ APPROVE - This PR is ready for merge. It addresses a critical operational need with a well-designed, secure implementation that properly handles edge cases and provides excellent user experience through comprehensive documentation and dry-run capabilities.

The code demonstrates BitGo's high security standards and will enable safe migration of wallet encryption for users requiring enhanced security provided by v2 encryption (Argon2id + AES-256-GCM).

Run Details:

Branch: WCN-174-reencrypt
Job Run Link

OttoAllmendinger

not going to ACK without module and tests, flushing

flushing review

OttoAllmendinger

see previous comment

pranavjain97 · 2026-07-13T16:20:46Z

not going to ACK without module and tests, flushing

I've moved the business logic to the key card module with tests. still have the script to invoke/e2e sample for clients. @OttoAllmendinger can you review?

zahin-mohammad

In addition to moving this out, have we done an e2e test with the keycard with

bitgo ui
wallet recovery wizard

Adds a wallet encryption upgrade path from v1 (SJCL / PBKDF2-SHA256 + AES-256-CCM) to v2 (Argon2id + AES-256-GCM), invokable via the SDK and driven by a CLI script. sdk-core: - Keychains.getEncryptionVersion: now public, throws on unknown versions - Keychains.reencryptAsV2: new instance method for v1 to v2 upgrade - Wallet.upgradeEncryption: orchestrates the full flow with an injectable PDF generator for testability key-card: - Exports loadKeycardImage and createKeycardPdfGenerator factory (returns an UpgradeEncryptionPdfGenerator compatible with the wallet method) scripts/upgrade-wallet-encryption.ts: - Thin CLI wrapper around Wallet.upgradeEncryption - Handles MPCv2 (boxA/boxB), boxD-derived passphrase, dry-run, already-unlocked session, and PDF regeneration TICKET: WCN-174

zahin-mohammad

Logs can be moved in a follow up, along with OFC support. (OFC support can be done via key rotation instead of re-encryption)

zahin-mohammad · 2026-07-13T21:27:25Z

+        throw new Error(
+          'Failed to decrypt with the provided passphrase. ' +
+            'If the wallet password was changed after creation, provide the original passphrase so the backup key can be decrypted.'
+        );


Nothing about this method is specific to the backup key? I am confused by this error message...

zahin-mohammad · 2026-07-13T21:30:16Z

+    const [userKeychain, backupKeychain, bitgoKeychain] = await Promise.all([
+      keychainsApi.get({ id: keyIds[0] }),
+      keychainsApi.get({ id: keyIds[1] }),
+      keychainsApi.get({ id: keyIds[2] }),
+    ]);


What about wallets that do not have 3 keys? (i.e. OFC)

OttoAllmendinger

looks much better, agree with zahin

bdesoky force-pushed the WCN-174-reencrypt branch 3 times, most recently from 00ed82a to 567bcdf Compare July 8, 2026 20:12

bdesoky changed the title ~~feat: add sdk method to reencrypt wallet with v2 encryption~~ feat: add script to reencrypt wallet with v2 encryption Jul 8, 2026

bdesoky force-pushed the WCN-174-reencrypt branch from 567bcdf to a30b584 Compare July 8, 2026 20:18

vmccarty previously approved these changes Jul 9, 2026

View reviewed changes

vmccarty reviewed Jul 9, 2026

View reviewed changes

bdesoky dismissed vmccarty’s stale review via db796c3 July 9, 2026 17:45

bdesoky force-pushed the WCN-174-reencrypt branch from a30b584 to db796c3 Compare July 9, 2026 17:45

bdesoky marked this pull request as ready for review July 9, 2026 18:01

bdesoky requested review from a team as code owners July 9, 2026 18:01

bdesoky requested review from Marzooqa and s84krish July 9, 2026 18:01

bdesoky force-pushed the WCN-174-reencrypt branch 2 times, most recently from 4d55f08 to ab75384 Compare July 9, 2026 21:59

OttoAllmendinger previously requested changes Jul 10, 2026

View reviewed changes

feat: add script to reencrypt wallet with v2 encryption

6f43e3f

Ticket: WCN-174

bdesoky force-pushed the WCN-174-reencrypt branch from ab75384 to 6f43e3f Compare July 10, 2026 14:46

pranavjain97 requested changes Jul 10, 2026

View reviewed changes

Comment thread scripts/upgrade-wallet-encryption.ts Outdated

pranavjain97 force-pushed the WCN-174-reencrypt branch from 06084fd to ff43ff7 Compare July 10, 2026 18:20

pranavjain97 force-pushed the WCN-174-reencrypt branch from ff43ff7 to 455f6c1 Compare July 10, 2026 18:28

pranavjain97 previously approved these changes Jul 10, 2026

View reviewed changes

pranavjain97 requested a review from OttoAllmendinger July 10, 2026 18:31

OttoAllmendinger reviewed Jul 10, 2026

View reviewed changes

OttoAllmendinger requested review from OttoAllmendinger and vmccarty July 10, 2026 19:46

OttoAllmendinger reviewed Jul 10, 2026

View reviewed changes

pranavjain97 dismissed their stale review via 5051e01 July 10, 2026 20:55

pranavjain97 requested review from OttoAllmendinger and zahin-mohammad July 13, 2026 16:20

zahin-mohammad requested changes Jul 13, 2026

View reviewed changes

Comment thread modules/key-card/src/upgradeWalletEncryption.ts

pranavjain97 force-pushed the WCN-174-reencrypt branch from f87bf33 to 364e027 Compare July 13, 2026 20:58

pranavjain97 requested a review from a team as a code owner July 13, 2026 20:58

pranavjain97 requested a review from zahin-mohammad July 13, 2026 21:17

zahin-mohammad approved these changes Jul 13, 2026

View reviewed changes

OttoAllmendinger reviewed Jul 14, 2026

View reviewed changes

Comment thread scripts/upgrade-wallet-encryption.ts

OttoAllmendinger approved these changes Jul 14, 2026

View reviewed changes

Uh oh!

Conversation

bdesoky commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1. New Wallet Encryption Upgrade Script

2. Node.js Compatibility for Keycard PDF Generation

Uh oh!

linear-code Bot commented Jul 8, 2026

Uh oh!

vmccarty left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bdesoky Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pranavjain97 left a comment

Choose a reason for hiding this comment

Uh oh!

pranavjain97 commented Jul 10, 2026

Uh oh!

github-actions Bot commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review and Approval Rating

Tasks:

Overall Approval Rating: ⭐⭐⭐⭐⭐ STRONG APPROVE

Strengths

🔒 Security Excellence

🛠️ Robust Implementation

📋 Excellent Documentation

Previous Reviewer Feedback Resolution ✅

Code Quality Assessment

Architecture & Design - Excellent

Error Handling - Very Good

Testing & Validation - Good

Documentation - Outstanding

Minor Observations (No Action Required)

Recommendation

Uh oh!

OttoAllmendinger left a comment

Choose a reason for hiding this comment

Uh oh!

OttoAllmendinger left a comment

Choose a reason for hiding this comment

Uh oh!

pranavjain97 commented Jul 13, 2026

Uh oh!

zahin-mohammad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zahin-mohammad left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bdesoky commented Jul 8, 2026 •

edited

Loading

vmccarty left a comment •

edited

Loading

bdesoky Jul 9, 2026 •

edited

Loading

github-actions Bot commented Jul 10, 2026 •

edited

Loading