[zahin-mohammad]

Ticket: WCN-3735 (follow-up to support ticket CUS-2174)

Problem

Wallets.bulkAcceptShare always fetched and decrypted the user's ECDH sharing keychain, even when every selected share is a multi-key rotation share (userMultiKeyRotationRequired: true, e.g. Go Account / OFC). Those shares carry no keychain and generate a fresh keypair encrypted with the login password, so they never use the ECDH key.

For a user whose settings have ecdhKeychain: null (confirmed on prod for the CUS-2174 user), getECDHKeychain() returned an Error object instead of throwing. bulkAcceptShare read .encryptedXprv off it, got undefined, and surfaced encryptedXprv was not found on sharing keychain in the Accept Keys modal.

Changes

• sdk-core: bulkAcceptShare fetches and decrypts the ECDH keychain only when at least one selected share has a keychain (same idea as hasSharesRequiringDecryption in bulkUpdateWalletShare), and asserts the decrypted key exists before deriving the shared secret.
• sdk-api: getECDHKeychain now throws ecdh keychain not found for user instead of returning the Error. All in-repo callers read encryptedXprv off the result, so none relied on the returned value.

Tests

• UMKR-only batch succeeds and never calls getECDHKeychain (fails without the fix).
• Mixed batch with a keychain share still rejects with encryptedXprv was not found on sharing keychain.
• getECDHKeychain rejects when the user has no ecdhKeychain (fails without the fix).
• Ran walletsEncryptionVersion.ts (13 passing) and modules/sdk-api/test/unit/bitgoAPI.ts (75 passing); tsc --noEmit on sdk-core clean. The larger modules/bitgo wallets suite was not run.

Not covered

• Why this user has no ECDH keychain (terms accepted 2026-10-20) is a separate data question; other Go Account invitees may be affected.
• bitgo-retail needs an @bitgo-beta/sdk-core bump after release before users see the fix.

🤖 Generated with Claude Code

[linear-code]

WCN-3735

[github-actions]

⚠️ Unit tests are failing on Node 26.x (Current release line, non-blocking). This is not an LTS version yet, so it does not block merge, but it signals an incompatibility to fix before Node 26.x becomes LTS.

View run

[zahin-mohammad]

@claude

[github-actions]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[zahin-mohammad]

@claude review

[github-actions]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[zahin-mohammad]

@claude review

[github-actions]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[claude]

The latest commit fixes the wrong-password issue I raised earlier, and I found no new bugs. A code owner should still look, because this changes how keys are handled when accepting wallet shares.

What was reviewed:

• bulkAcceptShare now always fetches the ECDH keychain. When the user has one, it still decrypts it with userLoginPassword, so a mistyped password is rejected for batches that contain only Go Account/OFC shares.
• When the keychain is missing, the per-share assert stops a share that has a keychain before any ECDH derivation runs.
• The other callers (wallets.ts:1283 and :1560, v1 wallets.ts:250, getECDHKeypairPrivate) now get an error thrown earlier with a clearer message. Before, they hit encryptedXprv on a returned Error, so their behavior is otherwise unchanged.

Extended reasoning...

The change touches getECDHKeychain in sdk-api, the BitGoBase interface, and Wallets.bulkAcceptShare in sdk-core, plus unit tests. It handles the decryption of the ECDH sharing key and the check on the login password used to encrypt new wallet keys, which is crypto-sensitive. CODEOWNERS assigns sdk-core and sdk-api to the wallet-core teams. One known limit remains: if a user has no ECDH keychain, a batch of only multi-key rotation shares has nothing to check the password against. Before this change, those users could not accept such shares at all.

[zahin-mohammad]

AI-generated review — generated by the wallet-platform:review-pr workflow. Please use engineering judgment before acting on any finding.

PR Review: #10024 — skip the ECDH keychain for bulk multi-key share acceptance

Author: Zahin Mohammad · Changes: +125/−12 across 5 files · Ticket: WCN-3735

TL;DR

• The null-keychain failure is fixed, and the focused tests pass.
• BLOCKER: the exact missing-keychain path can accept a mistyped login password and encrypt the newly generated wallet private key under that typo.
• The implementation still calls getECDHKeychain for every batch, so the PR description and ticket statement that rotation-only batches do not fetch it are stale.
• No database query or unsafe cast was added. The new any | undefined return type is nevertheless ineffective TypeScript modeling.

Finding

[P1] Validate the password when the ECDH keychain is missing

VERIFIED. When allowMissing returns undefined—the exact WCN-3735 account state—the guard skips the only password check (wallets.ts:1364). With no explicit newWalletPassphrase, the same unchecked userLoginPassword becomes the encryption password; the method then generates a new wallet key and submits it (wallets.ts:1378). The request contains the encrypted private key and public key, but no password that the acceptance endpoint could validate (iWallets.ts:368, wallets.ts:1048).

Trigger: a user with no Elliptic Curve Diffie-Hellman (ECDH) sharing key accepts only user multi-key rotation (UMKR) shares, omits newWalletPassphrase, and mistypes userLoginPassword. The share can be accepted with a newly generated private key encrypted under the typo, so the real login password cannot decrypt it later.

Use a server-backed password check before creating the encryption session or key when no ECDH key is available. BitGoAPI.verifyPassword already calls /user/verifypassword (bitgoAPI.ts:1307); expose a narrowly typed equivalent through BitGoBase, or provide another explicit verification path. Add a regression test where getECDHKeychain returns undefined, the password verifier rejects, and createEncryptionSession, key creation, and PUT /walletshares/accept are never called.

ASSUMED / human check: another application may validate the password before calling this public Software Development Kit (SDK) method. That is not visible in this repository and would not protect direct SDK callers, so it should not be the method's safety boundary.

Summary and data flow

This change makes getECDHKeychain optionally return undefined, uses that mode for UMKR-only batches, and preserves ECDH decryption for conventional shares. Rotation-only shares generate a fresh per-user key; conventional shares derive a shared secret from the ECDH key before re-encryption.

The implemented flow is: list incoming shares; detect whether any conventional share needs ECDH; call getECDHKeychain in both branches; decrypt it when present; create an encryption session; generate a new key for UMKR shares or decrypt/re-encrypt a conventional shared key; then submit the bulk acceptance request. A diagram is omitted because the available Mermaid renderer could not be validated locally.

Ticket alignment

• Intent match: ⚠️ PARTIAL. The production case with settings.ecdhKeychain: null now proceeds, but WCN-3735 says an UMKR-only batch should succeed “without fetching the ECDH keychain.” The current code always calls getECDHKeychain; allowMissing only changes the null result (wallets.ts:1361).
• Scope: ✅ IN SCOPE. All five files implement or test the wallet-share/keychain behavior.
• Ticket quality: ✅ SUFFICIENT. The ticket includes the production state, failure mechanism, requested behavior, and exclusions.
• Documentation caveat: the PR body says the UMKR-only test “never calls getECDHKeychain,” while the current test asserts that it is called with { allowMissing: true } (walletsEncryptionVersion.ts:148). Update the body before merge.

Database index analysis

No new database query is introduced; this change only performs existing HTTP client operations.

Type safety

[P3] Promise<any | undefined> does not make callers handle the new missing result because any absorbs undefined (bitgoBase.ts:51, bitgoAPI.ts:1881). Define an ECDH-keychain result type and overload the method so only { allowMissing: true } returns Result | undefined; the default overload should return Result because it throws when absent.

No added as any, double cast, non-null assertion, or TypeScript suppression was found.

Verification

Focused tests were run in a temporary detached worktree at commit 12675b80e45dc52dada05ad9c9e6489e10d4c430 with Node.js 22.23.3:

Commands and real output

$ mocha --config modules/sdk-core/.mocharc.js modules/sdk-core/test/unit/bitgo/wallet/walletsEncryptionVersion.ts
14 passing (16ms)

$ mocha --config modules/sdk-api/.mocharc.js modules/sdk-api/test/unit/bitgoAPI.ts
76 passing (1s)

$ git diff --unified=0 origin/master...origin/zahinmohammad/wcn-3735-bulk-accept-share-skip-ecdh -- '*.ts' | rg '^\\+[^+].*(as any|as unknown as|@ts-ignore|@ts-expect-error)'
(no output)

$ git diff --unified=0 origin/master...origin/zahinmohammad/wcn-3735-bulk-accept-share-skip-ecdh -- '*.ts' | rg '^\\+[^+].*\\.(find|findOne|aggregate|updateOne|updateMany|deleteOne|countDocuments)\\s*\\('
(no output)

The current GitHub checks are also green on Node.js 22, 24, and 26: BitGo SDK run.

Alternatives considered

Option	Password safety for missing ECDH	Keeps UMKR independent of ECDH	Recommendation
Current allowMissing lookup	No	No	Do not merge as-is
Server-backed password verification	Yes	Yes	Recommended
Accept without verification	No	Yes	Reject; a typo can strand the new key

Glossary

• ECDH — Elliptic Curve Diffie-Hellman: the account sharing key used here to derive the secret for decrypting a conventional wallet share.
• UMKR — user multi-key rotation: a share type that creates a fresh user wallet key instead of decrypting a shared keychain.
• SDK — Software Development Kit: the public client library containing Wallets.bulkAcceptShare.

---

Generated by the wallet-platform:review-pr workflow.

[zahin-mohammad]

Correction to my earlier AI-generated review: I retract the P1 password-verification finding.

Creating a login password also creates the Elliptic Curve Diffie-Hellman (ECDH) sharing key. The affected account authenticated with a passkey and, per CUS-2179, had skipped both one-time password setup and ECDH creation. In the missing-ECDH branch there is therefore no login password to verify; adding verifyPassword would block the exact account #10024 fixed.

Follow-up #10069 keeps that behavior and addresses the valid type-safety/documentation finding by giving getECDHKeychain overloads whose return type reflects allowMissing.