From c4b5d5aa558dd902291414f5ebc731bfc2bc3ebf Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Wed, 20 May 2026 14:41:34 +0200 Subject: [PATCH 1/5] fix(snap-account-service)!: prevent double-lock when handling KeyringEvent(s) --- packages/snap-account-service/package.json | 2 + .../src/SnapAccountService.ts | 80 ++++++++++++++++++- yarn.lock | 2 + 3 files changed, 80 insertions(+), 4 deletions(-) diff --git a/packages/snap-account-service/package.json b/packages/snap-account-service/package.json index 16265c0054..835f031167 100644 --- a/packages/snap-account-service/package.json +++ b/packages/snap-account-service/package.json @@ -56,7 +56,9 @@ "@metamask/account-api": "^1.0.4", "@metamask/account-tree-controller": "^7.4.0", "@metamask/eth-snap-keyring": "^22.0.1", + "@metamask/keyring-api": "^23.1.0", "@metamask/keyring-controller": "^25.5.0", + "@metamask/keyring-snap-sdk": "^9.0.1", "@metamask/messenger": "^1.2.0", "@metamask/snaps-controllers": "^19.0.0", "@metamask/snaps-sdk": "^11.0.0", diff --git a/packages/snap-account-service/src/SnapAccountService.ts b/packages/snap-account-service/src/SnapAccountService.ts index 7f73096178..2fb2bec497 100644 --- a/packages/snap-account-service/src/SnapAccountService.ts +++ b/packages/snap-account-service/src/SnapAccountService.ts @@ -1,5 +1,6 @@ import { AccountGroupId } from '@metamask/account-api'; -import type { +import { SnapManageAccountsMethod } from '@metamask/keyring-snap-sdk'; +import { SnapKeyring as LegacySnapKeyring, SnapMessage, } from '@metamask/eth-snap-keyring'; @@ -10,7 +11,7 @@ import type { KeyringControllerWithControllerAction, KeyringEntry, } from '@metamask/keyring-controller'; -import { KeyringTypes } from '@metamask/keyring-controller'; +import { isKeyringNotFoundError, KeyringTypes } from '@metamask/keyring-controller'; import type { AccountId, BaseKeyring } from '@metamask/keyring-utils'; import type { Messenger } from '@metamask/messenger'; import type { @@ -47,6 +48,8 @@ import type { AccountTreeControllerAccountGroupRemovedEvent, AccountGroupObject, } from './types'; +import { KeyringEvent } from '@metamask/keyring-api'; +import { KeyringControllerWithKeyringUnsafeAction } from '@metamask/keyring-controller'; /** * The name of the {@link SnapAccountService}, used to namespace the service's @@ -83,6 +86,7 @@ type AllowedActions = | SnapControllerGetRunnableSnapsAction | KeyringControllerGetStateAction | KeyringControllerWithControllerAction + | KeyringControllerWithKeyringUnsafeAction | AccountTreeControllerGetAccountGroupObjectAction | AccountTreeControllerGetSelectedAccountGroupAction; @@ -348,6 +352,34 @@ export class SnapAccountService { return (result as Result).snapKeyring; } + + /** + * Gets the legacy (v1) Snap keyring but do not auto-create it if it doesn't exist. + * + * @returns The existing Snap keyring instance, or undefined if it doesn't exist. + */ + async #getLegacySnapKeyringIfAvailable(): Promise { + type Result = { + snapKeyring: LegacySnapKeyring; + }; + + try { + const result = await this.#messenger.call('KeyringController:withKeyringUnsafe', { filter: isLegacySnapKeyring }, async ({ keyring }): Promise => { + // The legacy Snap keyring is not compatible with `EthKeyring`, so we need to cast here. + return { snapKeyring: keyring } as unknown as Result; + }); + + return (result as Result).snapKeyring; + } catch (error) { + if (isKeyringNotFoundError(error)) { + log('Legacy Snap keyring not available yet.'); + return undefined; + } + + throw error; + } + } + /** * Handle a message from a Snap. * @@ -359,7 +391,42 @@ export class SnapAccountService { snapId: SnapId, message: SnapMessage, ): Promise { - const snapKeyring = await this.getLegacySnapKeyring(); + let snapKeyring: LegacySnapKeyring | undefined = await this.#getLegacySnapKeyringIfAvailable(); + + // Handle specific methods first. + if (message.method === SnapManageAccountsMethod.GetSelectedAccounts) { + if (snapKeyring) { + // The legacy Snap keyring already maintain a local list of selected accounts per Snaps, so we + // just delegate the call. + return snapKeyring.handleKeyringSnapMessage(snapId, message); + } + + // Some Snaps might be using `getSelectedAccounts` early in their lifecycle, before the keyring is created. So we + // do not throw in that case to avoid messing up their lifecycle. + return []; + } + + const event = message.method as KeyringEvent; // We assume the Snap platform always sends a valid `KeyringEvent` here. + log( + `Forwarding message "${event}" from Snap "${snapId}" to its keyring...`, + ); + + // We can create a new keyring if the message is an AccountCreated event. + const isAccountCreatedMessage = event === KeyringEvent.AccountCreated; + + // Create the Snap keyring if it doesn't exist yet (in an atomic way). We cannot assume + // the keyring exists (e.g for the MMI Snap). + // NOTE: We only auto-create it for v1 account creation flows. + if (isAccountCreatedMessage && !snapKeyring) { + snapKeyring = await this.getLegacySnapKeyring(); + } + + if (!snapKeyring) { + throw new Error( + `Legacy Snap keyring does not exist yet for snap "${snapId}".`, + ); + } + return snapKeyring.handleKeyringSnapMessage(snapId, message); } @@ -397,7 +464,12 @@ export class SnapAccountService { log(`Clearing selected accounts (from "${groupId}")`); } - const snapKeyring = await this.getLegacySnapKeyring(); + const snapKeyring = await this.#getLegacySnapKeyringIfAvailable(); + if (!snapKeyring) { + log('No legacy Snap keyring available, skipping forwarding selected accounts.'); + return; + } + await snapKeyring.setSelectedAccounts(accounts); }; diff --git a/yarn.lock b/yarn.lock index 9eff13004d..3489069ede 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5465,7 +5465,9 @@ __metadata: "@metamask/account-tree-controller": "npm:^7.4.0" "@metamask/auto-changelog": "npm:^6.1.0" "@metamask/eth-snap-keyring": "npm:^22.0.1" + "@metamask/keyring-api": "npm:^23.1.0" "@metamask/keyring-controller": "npm:^25.5.0" + "@metamask/keyring-snap-sdk": "npm:^9.0.1" "@metamask/keyring-utils": "npm:^3.2.1" "@metamask/messenger": "npm:^1.2.0" "@metamask/snaps-controllers": "npm:^19.0.0" From 340122b337a12b0622faecff01d45a7540319097 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Wed, 20 May 2026 15:09:39 +0200 Subject: [PATCH 2/5] chore: lint --- .../src/SnapAccountService.ts | 35 ++++++++++++------- 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/packages/snap-account-service/src/SnapAccountService.ts b/packages/snap-account-service/src/SnapAccountService.ts index 2fb2bec497..24f41aa1fc 100644 --- a/packages/snap-account-service/src/SnapAccountService.ts +++ b/packages/snap-account-service/src/SnapAccountService.ts @@ -1,9 +1,9 @@ import { AccountGroupId } from '@metamask/account-api'; -import { SnapManageAccountsMethod } from '@metamask/keyring-snap-sdk'; import { SnapKeyring as LegacySnapKeyring, SnapMessage, } from '@metamask/eth-snap-keyring'; +import { KeyringEvent } from '@metamask/keyring-api'; import type { KeyringControllerGetStateAction, KeyringControllerStateChangeEvent, @@ -11,7 +11,12 @@ import type { KeyringControllerWithControllerAction, KeyringEntry, } from '@metamask/keyring-controller'; -import { isKeyringNotFoundError, KeyringTypes } from '@metamask/keyring-controller'; +import { + isKeyringNotFoundError, + KeyringTypes, +} from '@metamask/keyring-controller'; +import { KeyringControllerWithKeyringUnsafeAction } from '@metamask/keyring-controller'; +import { SnapManageAccountsMethod } from '@metamask/keyring-snap-sdk'; import type { AccountId, BaseKeyring } from '@metamask/keyring-utils'; import type { Messenger } from '@metamask/messenger'; import type { @@ -48,8 +53,6 @@ import type { AccountTreeControllerAccountGroupRemovedEvent, AccountGroupObject, } from './types'; -import { KeyringEvent } from '@metamask/keyring-api'; -import { KeyringControllerWithKeyringUnsafeAction } from '@metamask/keyring-controller'; /** * The name of the {@link SnapAccountService}, used to namespace the service's @@ -352,22 +355,27 @@ export class SnapAccountService { return (result as Result).snapKeyring; } - /** * Gets the legacy (v1) Snap keyring but do not auto-create it if it doesn't exist. * * @returns The existing Snap keyring instance, or undefined if it doesn't exist. */ - async #getLegacySnapKeyringIfAvailable(): Promise { + async #getLegacySnapKeyringIfAvailable(): Promise< + LegacySnapKeyring | undefined + > { type Result = { snapKeyring: LegacySnapKeyring; }; try { - const result = await this.#messenger.call('KeyringController:withKeyringUnsafe', { filter: isLegacySnapKeyring }, async ({ keyring }): Promise => { - // The legacy Snap keyring is not compatible with `EthKeyring`, so we need to cast here. - return { snapKeyring: keyring } as unknown as Result; - }); + const result = await this.#messenger.call( + 'KeyringController:withKeyringUnsafe', + { filter: isLegacySnapKeyring }, + async ({ keyring }): Promise => { + // The legacy Snap keyring is not compatible with `EthKeyring`, so we need to cast here. + return { snapKeyring: keyring } as unknown as Result; + }, + ); return (result as Result).snapKeyring; } catch (error) { @@ -391,7 +399,8 @@ export class SnapAccountService { snapId: SnapId, message: SnapMessage, ): Promise { - let snapKeyring: LegacySnapKeyring | undefined = await this.#getLegacySnapKeyringIfAvailable(); + let snapKeyring: LegacySnapKeyring | undefined = + await this.#getLegacySnapKeyringIfAvailable(); // Handle specific methods first. if (message.method === SnapManageAccountsMethod.GetSelectedAccounts) { @@ -466,7 +475,9 @@ export class SnapAccountService { const snapKeyring = await this.#getLegacySnapKeyringIfAvailable(); if (!snapKeyring) { - log('No legacy Snap keyring available, skipping forwarding selected accounts.'); + log( + 'No legacy Snap keyring available, skipping forwarding selected accounts.', + ); return; } From 7af6eb479e562cde1c15950022826b83cac8f9f8 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Wed, 20 May 2026 15:10:28 +0200 Subject: [PATCH 3/5] test: add tests --- .../src/SnapAccountService.test.ts | 187 +++++++++++++++++- 1 file changed, 185 insertions(+), 2 deletions(-) diff --git a/packages/snap-account-service/src/SnapAccountService.test.ts b/packages/snap-account-service/src/SnapAccountService.test.ts index e1569d63a0..5e560fea12 100644 --- a/packages/snap-account-service/src/SnapAccountService.test.ts +++ b/packages/snap-account-service/src/SnapAccountService.test.ts @@ -1,6 +1,9 @@ import type { AccountGroupId } from '@metamask/account-api'; import type { SnapKeyring, SnapMessage } from '@metamask/eth-snap-keyring'; +import { KeyringEvent } from '@metamask/keyring-api'; import { + KeyringControllerError, + KeyringControllerErrorMessage, KeyringControllerState, KeyringTypes, } from '@metamask/keyring-controller'; @@ -8,6 +11,7 @@ import type { KeyringEntry, RestrictedController, } from '@metamask/keyring-controller'; +import { SnapManageAccountsMethod } from '@metamask/keyring-snap-sdk'; import { Messenger, MOCK_ANY_NAMESPACE } from '@metamask/messenger'; import type { MockAnyNamespace, @@ -62,6 +66,7 @@ type Mocks = { KeyringController: { getState: jest.MockedFunction<() => { keyrings: { type: string }[] }>; withController: jest.Mock; + withKeyringUnsafe: jest.Mock; }; // eslint-disable-next-line @typescript-eslint/naming-convention AccountTreeController: { @@ -103,6 +108,7 @@ function getMessenger( 'SnapController:getRunnableSnaps', 'KeyringController:getState', 'KeyringController:withController', + 'KeyringController:withKeyringUnsafe', 'AccountTreeController:getAccountGroupObject', 'AccountTreeController:getSelectedAccountGroup', ], @@ -323,6 +329,7 @@ function mockWithController( * @param keyring - The mocked Snap keyring methods. * @param keyring.handleKeyringSnapMessage - The mocked implementation. * @param keyring.setSelectedAccounts - The mocked implementation. + * @returns The mocked Snap keyring for assertions. */ function mockLegacySnapKeyring( mocks: Mocks, @@ -337,7 +344,7 @@ function mockLegacySnapKeyring( SnapKeyring['setSelectedAccounts'] >; }, -): void { +): MockSnapKeyring { const snapKeyring: MockSnapKeyring = { type: KeyringTypes.snap, handleKeyringSnapMessage, @@ -357,6 +364,28 @@ function mockLegacySnapKeyring( removeKeyring: jest.fn(), }), ); + mocks.KeyringController.withKeyringUnsafe.mockImplementation( + async (_selector, operation) => + operation({ + keyring: snapKeyring as KeyringEntry['keyring'], + metadata: { id: 'id-snap', name: KeyringTypes.snap }, + }), + ); + return snapKeyring; +} + +/** + * Configures `mocks.KeyringController.withKeyringUnsafe` to reject as if the + * legacy Snap keyring did not exist yet. + * + * @param mocks - The mocks object from {@link setup}. + */ +function mockLegacySnapKeyringMissing(mocks: Mocks): void { + mocks.KeyringController.withKeyringUnsafe.mockImplementation(async () => { + throw new KeyringControllerError( + KeyringControllerErrorMessage.KeyringNotFound, + ); + }); } /** @@ -398,6 +427,7 @@ function setup({ KeyringController: { getState: jest.fn().mockReturnValue({ keyrings }), withController: jest.fn(), + withKeyringUnsafe: jest.fn(), }, AccountTreeController: { getAccountGroupObject: jest.fn().mockReturnValue(undefined), @@ -421,6 +451,10 @@ function setup({ 'KeyringController:withController', mocks.KeyringController.withController, ); + rootMessenger.registerActionHandler( + 'KeyringController:withKeyringUnsafe', + mocks.KeyringController.withKeyringUnsafe, + ); rootMessenger.registerActionHandler( 'AccountTreeController:getAccountGroupObject', mocks.AccountTreeController.getAccountGroupObject, @@ -634,7 +668,7 @@ describe('SnapAccountService', () => { describe('handleKeyringSnapMessage', () => { const MOCK_MESSAGE = { - method: 'keyring_listAccounts', + method: KeyringEvent.AccountUpdated, params: {}, } as unknown as SnapMessage; @@ -689,6 +723,133 @@ describe('SnapAccountService', () => { ); expect(result).toBe('pong'); }); + + it('throws when the legacy Snap keyring does not exist yet for a non-AccountCreated message', async () => { + const { service, mocks } = setup(); + mockLegacySnapKeyringMissing(mocks); + + await expect( + service.handleKeyringSnapMessage(MOCK_SNAP_ID, MOCK_MESSAGE), + ).rejects.toThrow( + `Legacy Snap keyring does not exist yet for snap "${MOCK_SNAP_ID}".`, + ); + expect(mocks.KeyringController.withController).not.toHaveBeenCalled(); + }); + + it('propagates non-KeyringNotFound errors from withKeyringUnsafe', async () => { + const { service, mocks } = setup(); + mocks.KeyringController.withKeyringUnsafe.mockImplementation(async () => { + throw new Error('boom'); + }); + + await expect( + service.handleKeyringSnapMessage(MOCK_SNAP_ID, MOCK_MESSAGE), + ).rejects.toThrow('boom'); + }); + + describe('when the message is an AccountCreated event', () => { + const ACCOUNT_CREATED_MESSAGE = { + method: KeyringEvent.AccountCreated, + params: {}, + } as unknown as SnapMessage; + + it('auto-creates the legacy Snap keyring when it does not exist yet', async () => { + const { service, mocks } = setup(); + mockLegacySnapKeyringMissing(mocks); + const handleKeyringSnapMessage = jest + .fn() + .mockResolvedValue({ ok: true }); + // `getLegacySnapKeyring` goes through `withController` and creates the + // keyring if missing. + mocks.KeyringController.withController.mockImplementation( + async (operation) => + operation({ + get keyrings() { + return Object.freeze([]); + }, + addNewKeyring: jest.fn(async () => ({ + keyring: { + type: KeyringTypes.snap, + handleKeyringSnapMessage, + } as unknown as KeyringEntry['keyring'], + metadata: { id: 'id-snap', name: KeyringTypes.snap }, + })), + removeKeyring: jest.fn(), + }), + ); + + const result = await service.handleKeyringSnapMessage( + MOCK_SNAP_ID, + ACCOUNT_CREATED_MESSAGE, + ); + + expect(mocks.KeyringController.withController).toHaveBeenCalled(); + expect(handleKeyringSnapMessage).toHaveBeenCalledWith( + MOCK_SNAP_ID, + ACCOUNT_CREATED_MESSAGE, + ); + expect(result).toStrictEqual({ ok: true }); + }); + + it('uses the existing legacy Snap keyring when it is already available', async () => { + const { service, mocks } = setup(); + const handleKeyringSnapMessage = jest + .fn() + .mockResolvedValue({ ok: true }); + mockLegacySnapKeyring(mocks, { handleKeyringSnapMessage }); + + const result = await service.handleKeyringSnapMessage( + MOCK_SNAP_ID, + ACCOUNT_CREATED_MESSAGE, + ); + + expect(mocks.KeyringController.withController).not.toHaveBeenCalled(); + expect(handleKeyringSnapMessage).toHaveBeenCalledWith( + MOCK_SNAP_ID, + ACCOUNT_CREATED_MESSAGE, + ); + expect(result).toStrictEqual({ ok: true }); + }); + }); + + describe('when the message is a GetSelectedAccounts request', () => { + const GET_SELECTED_ACCOUNTS_MESSAGE = { + method: SnapManageAccountsMethod.GetSelectedAccounts, + params: {}, + } as unknown as SnapMessage; + + it('delegates to the legacy Snap keyring when it is available', async () => { + const { service, mocks } = setup(); + const handleKeyringSnapMessage = jest + .fn() + .mockResolvedValue(['account-1']); + mockLegacySnapKeyring(mocks, { handleKeyringSnapMessage }); + + const result = await service.handleKeyringSnapMessage( + MOCK_SNAP_ID, + GET_SELECTED_ACCOUNTS_MESSAGE, + ); + + expect(handleKeyringSnapMessage).toHaveBeenCalledWith( + MOCK_SNAP_ID, + GET_SELECTED_ACCOUNTS_MESSAGE, + ); + expect(result).toStrictEqual(['account-1']); + }); + + it('returns an empty list when the legacy Snap keyring does not exist yet', async () => { + const { service, mocks } = setup(); + mockLegacySnapKeyringMissing(mocks); + + const result = await service.handleKeyringSnapMessage( + MOCK_SNAP_ID, + GET_SELECTED_ACCOUNTS_MESSAGE, + ); + + expect(result).toStrictEqual([]); + expect(mocks.KeyringController.withController).not.toHaveBeenCalled(); + }); + }); }); describe('on AccountTreeController:selectedAccountGroupChange', () => { @@ -773,6 +934,28 @@ describe('SnapAccountService', () => { consoleErrorSpy.mockRestore(); }); + + it('skips silently when the legacy Snap keyring does not exist yet', async () => { + const { service, rootMessenger, mocks } = setup(); + mockLegacySnapKeyringMissing(mocks); + mocks.AccountTreeController.getAccountGroupObject.mockReturnValue( + buildGroup(MOCK_GROUP_ID, MOCK_ACCOUNTS), + ); + const consoleErrorSpy = jest + .spyOn(console, 'error') + .mockImplementation(() => undefined); + expect(service).toBeDefined(); + + publishSelectedAccountGroupChange(rootMessenger, MOCK_GROUP_ID); + await flushMicrotasks(); + + // The forwarder must NOT auto-create the keyring through `withController`. + expect(mocks.KeyringController.withController).not.toHaveBeenCalled(); + // And it must NOT bubble the "keyring not found" condition as an error. + expect(consoleErrorSpy).not.toHaveBeenCalled(); + + consoleErrorSpy.mockRestore(); + }); }); describe('on KeyringController:unlock', () => { From 8ceff621542e5f24e01f0fdf6caa7e9973361ab2 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Wed, 20 May 2026 15:17:45 +0200 Subject: [PATCH 4/5] chore: changelog --- packages/snap-account-service/CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/snap-account-service/CHANGELOG.md b/packages/snap-account-service/CHANGELOG.md index 44fcb39bcd..abd991365c 100644 --- a/packages/snap-account-service/CHANGELOG.md +++ b/packages/snap-account-service/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- Prevent double-lock in `:handleKeyringSnapMessage` for some events/methods ([#8860](https://github.com/MetaMask/core/pull/8860)) + ## [0.2.0] ### Added From bfa94c06015353d8593d318f9aacbdee95cc2021 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Wed, 20 May 2026 15:19:18 +0200 Subject: [PATCH 5/5] chore: changelog --- packages/snap-account-service/CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/snap-account-service/CHANGELOG.md b/packages/snap-account-service/CHANGELOG.md index abd991365c..9372edbbd4 100644 --- a/packages/snap-account-service/CHANGELOG.md +++ b/packages/snap-account-service/CHANGELOG.md @@ -10,6 +10,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - Prevent double-lock in `:handleKeyringSnapMessage` for some events/methods ([#8860](https://github.com/MetaMask/core/pull/8860)) + - The service messenger now requires the `KeyringController:withKeyringUnsafe` action. + - We now check if the keyring is available before delegating those messages. + - We still auto-create the keyring in some specific calls (e.g `notify:accountCreated`). ## [0.2.0]