From ec443530c98fc389ae920310144f40f560089fd0 Mon Sep 17 00:00:00 2001 From: gomesalexandre Date: Wed, 2 Sep 2026 05:04:01 +0200 Subject: [PATCH 1/3] fix(multichain-account-service): stop leaking removed accounts through BaseBip44AccountProvider getAccounts() cast the AccountsController's (InternalAccount | undefined)[] response straight through, so an account ID this provider still tracks but the AccountsController no longer knows about (e.g. it was removed) silently produced an 'account' that was actually undefined at runtime. init() was purely additive, so a re-init after a removal (e.g. every unlock in metamask-mobile's Authentication flow, which calls MultichainAccountService.init() -> provider.init() on each call) left the removed account's ID tracked forever. Fix both: filter undefined entries out of getAccounts(), and make init() replace the tracked account set instead of adding to it. --- .../BaseBip44AccountProvider.test.ts | 168 ++++++++++++++++++ .../src/providers/BaseBip44AccountProvider.ts | 22 ++- 2 files changed, 183 insertions(+), 7 deletions(-) create mode 100644 packages/multichain-account-service/src/providers/BaseBip44AccountProvider.test.ts diff --git a/packages/multichain-account-service/src/providers/BaseBip44AccountProvider.test.ts b/packages/multichain-account-service/src/providers/BaseBip44AccountProvider.test.ts new file mode 100644 index 00000000000..b23272626cc --- /dev/null +++ b/packages/multichain-account-service/src/providers/BaseBip44AccountProvider.test.ts @@ -0,0 +1,168 @@ +import type { + CreateAccountOptions, + KeyringAccount, + KeyringCapabilities, +} from '@metamask/keyring-api'; +import type { Bip44Account } from '@metamask/account-api'; +import type { InternalAccount } from '@metamask/keyring-internal-api'; + +import { + getMultichainAccountServiceMessenger, + getRootMessenger, + MOCK_HD_ACCOUNT_1, + MOCK_HD_ACCOUNT_2, +} from '../tests/index.js'; +import type { RootMessenger } from '../tests/index.js'; +import { BaseBip44AccountProvider } from './BaseBip44AccountProvider.js'; + +/** + * Minimal concrete subclass so we can exercise the abstract base class + * directly, matching the pattern used to test other abstract collaborators + * in this package. + */ +class TestAccountProvider extends BaseBip44AccountProvider< + Bip44Account +> { + get capabilities(): KeyringCapabilities { + return { supportsEnabling: true } as unknown as KeyringCapabilities; + } + + getName(): string { + return 'TestAccountProvider'; + } + + async resyncAccounts(): Promise { + // No-op for this test double. + } + + isAccountCompatible(): boolean { + return true; + } + + async createAccounts( + _options: CreateAccountOptions, + ): Promise[]> { + return []; + } + + async deleteAccount(): Promise { + // No-op for this test double. + } + + async discoverAccounts(): Promise[]> { + return []; + } +} + +/** + * Sets up a provider wired to a messenger whose + * `AccountsController:getAccounts` handler mirrors the real + * `AccountsController.getAccounts` contract: one slot per requested ID, + * `undefined` in place for any ID the controller no longer knows about + * (rather than omitting it, which would silently shrink the array). + * + * @param knownAccounts - The accounts the mocked AccountsController knows + * about. + * @returns The provider under test and its messenger. + */ +function setup(knownAccounts: InternalAccount[] = []): { + provider: TestAccountProvider; + messenger: RootMessenger; +} { + const rootMessenger = getRootMessenger(); + const messenger = getMultichainAccountServiceMessenger(rootMessenger); + + rootMessenger.registerActionHandler( + 'AccountsController:getAccounts', + (accountIds: string[]) => + accountIds.map((id) => + knownAccounts.find((account) => account.id === id), + ), + ); + + return { + provider: new TestAccountProvider(messenger), + messenger: rootMessenger, + }; +} + +describe('BaseBip44AccountProvider', () => { + describe('getAccounts', () => { + it('filters out accounts the AccountsController no longer knows about', () => { + // Only account 1 is "known" -- account 2 has been removed from the + // AccountsController's perspective, but is still tracked by this + // provider (e.g. before a reconciling init() call happens). + const { provider } = setup([MOCK_HD_ACCOUNT_1]); + provider.init([MOCK_HD_ACCOUNT_1.id, MOCK_HD_ACCOUNT_2.id]); + + const accounts = provider.getAccounts(); + + expect(accounts).toStrictEqual([MOCK_HD_ACCOUNT_1]); + expect(accounts).not.toContain(undefined); + }); + + it('returns an empty array when none of the tracked accounts exist anymore', () => { + const { provider } = setup([]); + provider.init([MOCK_HD_ACCOUNT_1.id, MOCK_HD_ACCOUNT_2.id]); + + expect(provider.getAccounts()).toStrictEqual([]); + }); + + it('returns all accounts unchanged when every tracked account still exists', () => { + const { provider } = setup([MOCK_HD_ACCOUNT_1, MOCK_HD_ACCOUNT_2]); + provider.init([MOCK_HD_ACCOUNT_1.id, MOCK_HD_ACCOUNT_2.id]); + + expect(provider.getAccounts()).toStrictEqual([ + MOCK_HD_ACCOUNT_1, + MOCK_HD_ACCOUNT_2, + ]); + }); + }); + + describe('init', () => { + it('does not leave stale account IDs behind after a re-init with a smaller set', () => { + const { provider } = setup([MOCK_HD_ACCOUNT_1, MOCK_HD_ACCOUNT_2]); + + // First init: both accounts are tracked. + provider.init([MOCK_HD_ACCOUNT_1.id, MOCK_HD_ACCOUNT_2.id]); + expect( + provider.isAligned({ entropySource: 'x', groupIndex: 0 }, [ + MOCK_HD_ACCOUNT_2.id, + ]), + ).toBe(true); + + // Account 2 gets removed upstream; a real caller (e.g. the mobile + // app's Authentication flow, which calls + // MultichainAccountService.init() -> provider.init() on every + // unlock) re-initializes the provider with only the surviving + // account. + provider.init([MOCK_HD_ACCOUNT_1.id]); + + // The stale ID for the removed account must not still be tracked. + expect( + provider.isAligned({ entropySource: 'x', groupIndex: 0 }, [ + MOCK_HD_ACCOUNT_2.id, + ]), + ).toBe(false); + expect( + provider.isAligned({ entropySource: 'x', groupIndex: 0 }, [ + MOCK_HD_ACCOUNT_1.id, + ]), + ).toBe(true); + }); + + it('is idempotent when called repeatedly with the same accounts', () => { + const { provider } = setup([MOCK_HD_ACCOUNT_1]); + + provider.init([MOCK_HD_ACCOUNT_1.id]); + provider.init([MOCK_HD_ACCOUNT_1.id]); + provider.init([MOCK_HD_ACCOUNT_1.id]); + + expect( + provider.isAligned({ entropySource: 'x', groupIndex: 0 }, [ + MOCK_HD_ACCOUNT_1.id, + ]), + ).toBe(true); + }); + }); +}); diff --git a/packages/multichain-account-service/src/providers/BaseBip44AccountProvider.ts b/packages/multichain-account-service/src/providers/BaseBip44AccountProvider.ts index 6554a1229fe..392ae3486ab 100644 --- a/packages/multichain-account-service/src/providers/BaseBip44AccountProvider.ts +++ b/packages/multichain-account-service/src/providers/BaseBip44AccountProvider.ts @@ -138,16 +138,18 @@ export abstract class BaseBip44AccountProvider< } /** - * Add accounts to the provider. + * Initialize the provider with the given accounts. * * Note: There's an implicit assumption that the accounts are BIP-44 compatible. * - * @param accounts - The accounts to add. + * This replaces the provider's internal account list rather than adding to + * it, so a re-init (e.g. after an account has been removed) does not leave + * stale IDs behind from a previous call. + * + * @param accounts - The accounts to initialize the provider with. */ init(accounts: Account['id'][]): void { - for (const account of accounts) { - this.accounts.add(account); - } + this.accounts = new Set(accounts); } /** @@ -170,8 +172,14 @@ export abstract class BaseBip44AccountProvider< 'AccountsController:getAccounts', accountsIds, ); - // we cast here because we know that the accounts are BIP-44 compatible - return internalAccounts as unknown as Account[]; + // `AccountsController:getAccounts` returns `undefined` for any ID it no + // longer knows about (e.g. the account was removed but this provider's + // internal ID list hasn't been reconciled yet). Filter those out rather + // than casting them through as if they were real accounts. + // we cast here because we know that the remaining accounts are BIP-44 compatible + return internalAccounts.filter( + (account): account is NonNullable => Boolean(account), + ) as unknown as Account[]; } /** From ec11168a2666b0aaaffffaf0118022df74d5cda9 Mon Sep 17 00:00:00 2001 From: gomesalexandre Date: Wed, 2 Sep 2026 05:13:06 +0200 Subject: [PATCH 2/3] fix(multichain-account-service): stop leaking removed accounts through BaseBip44AccountProvider getAccounts() cast the AccountsController's (InternalAccount | undefined)[] response straight through, so an account ID this provider still tracks but the AccountsController no longer knows about (e.g. it was removed) silently produced an 'account' that was actually undefined at runtime. init() was purely additive, so a re-init after a removal (e.g. every unlock in metamask-mobile's Authentication flow, which calls MultichainAccountService.init() -> provider.init() on each call) left the removed account's ID tracked forever. Fix both: filter undefined entries out of getAccounts(), and make init() replace the tracked account set instead of adding to it. Also update the shared test-provider mock (tests/providers.ts) to model the same replace-not-add init() semantics, so higher-level service/wallet tests don't silently exercise the old (buggy) behavior via the mock. --- packages/multichain-account-service/CHANGELOG.md | 6 ++++++ .../src/providers/BaseBip44AccountProvider.test.ts | 2 +- packages/multichain-account-service/src/tests/providers.ts | 4 +++- 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/packages/multichain-account-service/CHANGELOG.md b/packages/multichain-account-service/CHANGELOG.md index fe40d5794da..22809ad840a 100644 --- a/packages/multichain-account-service/CHANGELOG.md +++ b/packages/multichain-account-service/CHANGELOG.md @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- Fix `BaseBip44AccountProvider` leaking removed accounts and stale account IDs ([#PR_NUMBER](https://github.com/MetaMask/core/pull/PR_NUMBER)) + - `getAccounts()` now filters out entries the `AccountsController` no longer knows about instead of returning them as if they were real accounts. + - `init()` now replaces the tracked account set instead of adding to it, so a re-init after an account removal (e.g. on unlock) no longer leaves the removed account's ID tracked forever. + ### Changed - Bump `@metamask/accounts-controller` from `^39.1.0` to `^39.1.1` ([#9969](https://github.com/MetaMask/core/pull/9969)) diff --git a/packages/multichain-account-service/src/providers/BaseBip44AccountProvider.test.ts b/packages/multichain-account-service/src/providers/BaseBip44AccountProvider.test.ts index b23272626cc..f478d730225 100644 --- a/packages/multichain-account-service/src/providers/BaseBip44AccountProvider.test.ts +++ b/packages/multichain-account-service/src/providers/BaseBip44AccountProvider.test.ts @@ -1,8 +1,8 @@ import type { CreateAccountOptions, KeyringAccount, - KeyringCapabilities, } from '@metamask/keyring-api'; +import type { KeyringCapabilities } from '@metamask/keyring-api/v2'; import type { Bip44Account } from '@metamask/account-api'; import type { InternalAccount } from '@metamask/keyring-internal-api'; diff --git a/packages/multichain-account-service/src/tests/providers.ts b/packages/multichain-account-service/src/tests/providers.ts index 4f09c453b29..cb91f84632b 100644 --- a/packages/multichain-account-service/src/tests/providers.ts +++ b/packages/multichain-account-service/src/tests/providers.ts @@ -111,7 +111,9 @@ export function setupBip44AccountProvider({ mocks.createAccounts.mockResolvedValue([]); mocks.init.mockImplementation( (accountIds: Bip44Account['id'][]) => { - accountIds.forEach((id) => mocks.accounts.add(id)); + // Mirrors the real BaseBip44AccountProvider#init(), which replaces + // (rather than adds to) the tracked account set on every call. + mocks.accounts = new Set(accountIds); }, ); From 9fae227a8d3cabe153578ac83ea3d56edfb7e99a Mon Sep 17 00:00:00 2001 From: gomesalexandre Date: Wed, 2 Sep 2026 05:13:46 +0200 Subject: [PATCH 3/3] chore: backfill real PR number in changelog --- packages/multichain-account-service/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/multichain-account-service/CHANGELOG.md b/packages/multichain-account-service/CHANGELOG.md index 22809ad840a..7451f25bb8f 100644 --- a/packages/multichain-account-service/CHANGELOG.md +++ b/packages/multichain-account-service/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- Fix `BaseBip44AccountProvider` leaking removed accounts and stale account IDs ([#PR_NUMBER](https://github.com/MetaMask/core/pull/PR_NUMBER)) +- Fix `BaseBip44AccountProvider` leaking removed accounts and stale account IDs ([#10069](https://github.com/MetaMask/core/pull/10069)) - `getAccounts()` now filters out entries the `AccountsController` no longer knows about instead of returning them as if they were real accounts. - `init()` now replaces the tracked account set instead of adding to it, so a re-init after an account removal (e.g. on unlock) no longer leaves the removed account's ID tracked forever.