From b5329fc8456b4a6c2b97269dfb5197ecc49a1a61 Mon Sep 17 00:00:00 2001 From: gantunesr <17601467+gantunesr@users.noreply.github.com> Date: Tue, 1 Sep 2026 15:50:49 -0400 Subject: [PATCH 1/3] fix: filter account data updates by Snap ownership --- packages/snap-account-service/CHANGELOG.md | 4 + .../src/SnapAccountService.test.ts | 77 +++++++++- .../src/SnapAccountService.ts | 135 ++++++++++++++++-- 3 files changed, 199 insertions(+), 17 deletions(-) diff --git a/packages/snap-account-service/CHANGELOG.md b/packages/snap-account-service/CHANGELOG.md index 187439dbec0..cd753b0a5df 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 + +- Filter account data update events (`notify:accountTransactionsUpdated`, `notify:accountBalancesUpdated`, and `notify:accountAssetListUpdated`) to the accounts that the originating Snap actually owns before republishing them ([#8916](https://github.com/MetaMask/core/pull/8916)) + ## [2.1.2] ### Changed diff --git a/packages/snap-account-service/src/SnapAccountService.test.ts b/packages/snap-account-service/src/SnapAccountService.test.ts index fe36efdcf20..6df95961189 100644 --- a/packages/snap-account-service/src/SnapAccountService.test.ts +++ b/packages/snap-account-service/src/SnapAccountService.test.ts @@ -1182,42 +1182,67 @@ describe('SnapAccountService', () => { }); const MOCK_ACCOUNT_ID = '00000000-0000-4000-8000-000000000001'; + // An account ID that the Snap does NOT own. Updates for this ID must be + // stripped before the event is republished, otherwise a Snap could forge + // data for accounts owned by another Snap (or for accounts that do not + // exist at all). + const MOCK_UNOWNED_ACCOUNT_ID = '00000000-0000-4000-8000-000000000002'; it.each([ [ KeyringEvent.AccountBalancesUpdated, 'SnapAccountService:accountBalancesUpdated' as const, + 'balances' as const, { balances: { [MOCK_ACCOUNT_ID]: { 'eip155:1/slip44:60': { amount: '1', unit: 'ETH' }, }, + [MOCK_UNOWNED_ACCOUNT_ID]: { + 'eip155:1/slip44:60': { amount: '99', unit: 'ETH' }, + }, }, } satisfies AccountBalancesUpdatedEventPayload, ], [ KeyringEvent.AccountAssetListUpdated, 'SnapAccountService:accountAssetListUpdated' as const, + 'assets' as const, { assets: { [MOCK_ACCOUNT_ID]: { added: ['eip155:1/slip44:60'], removed: [] }, + [MOCK_UNOWNED_ACCOUNT_ID]: { + added: ['eip155:1/slip44:60'], + removed: [], + }, }, } satisfies AccountAssetListUpdatedEventPayload, ], [ KeyringEvent.AccountTransactionsUpdated, 'SnapAccountService:accountTransactionsUpdated' as const, + 'transactions' as const, { - transactions: { [MOCK_ACCOUNT_ID]: [] }, + transactions: { + [MOCK_ACCOUNT_ID]: [], + [MOCK_UNOWNED_ACCOUNT_ID]: [], + }, } satisfies AccountTransactionsUpdatedEventPayload, ], ] as const)( - 'publishes %s as a service event without touching the keyring', - async (method, event, payload) => { + 'filters %s to accounts owned by the Snap before republishing it', + async (method, event, key, payload) => { const { service, rootMessenger, mocks } = await setup(); const listener = jest.fn(); rootMessenger.subscribe(event, listener); + // The Snap only owns MOCK_ACCOUNT_ID; the unowned ID must be dropped. + mockWithKeyringV2Unsafe(mocks, { + [MOCK_SNAP_ID]: { + hasAccount: (id: string) => id === MOCK_ACCOUNT_ID, + }, + }); + expect(service).toBeDefined(); const result = await service.handleKeyringSnapMessage(MOCK_SNAP_ID, { @@ -1226,13 +1251,55 @@ describe('SnapAccountService', () => { } as unknown as SnapMessage); expect(result).toBeNull(); - expect(listener).toHaveBeenCalledWith(payload); + // Only the owned account survives the ownership filter. + expect(listener).toHaveBeenCalledTimes(1); + expect(listener).toHaveBeenCalledWith({ + [key]: { [MOCK_ACCOUNT_ID]: payload[key][MOCK_ACCOUNT_ID] }, + }); + // The ownership filter must actually run on the live path — this + // assertion previously locked in the bypass (core#8916) and is now + // inverted. expect( mocks.KeyringController.withKeyringV2Unsafe, - ).not.toHaveBeenCalled(); + ).toHaveBeenCalledTimes(1); expect(mocks.KeyringController.withController).not.toHaveBeenCalled(); }, ); + + it('drops the whole update (and does not throw) when the Snap keyring does not exist yet', async () => { + const { service, rootMessenger, mocks } = await setup(); + const listener = jest.fn(); + rootMessenger.subscribe( + 'SnapAccountService:accountBalancesUpdated', + listener, + ); + + // No keyring registered for the Snap -> the ownership lookup throws + // KeyringNotFound. The handler must fail closed (drop the update) + // rather than forward unverified data or let the error escape. + mocks.KeyringController.withKeyringV2Unsafe.mockRejectedValue( + new KeyringControllerError( + KeyringControllerErrorMessage.KeyringNotFound, + ), + ); + + const payload = { + balances: { + [MOCK_ACCOUNT_ID]: { + 'eip155:1/slip44:60': { amount: '1', unit: 'ETH' }, + }, + }, + } satisfies AccountBalancesUpdatedEventPayload; + + const result = await service.handleKeyringSnapMessage(MOCK_SNAP_ID, { + method: KeyringEvent.AccountBalancesUpdated, + params: payload, + } as unknown as SnapMessage); + + expect(result).toBeNull(); + + expect(listener).not.toHaveBeenCalled(); + }); }); describe('on AccountTreeController:selectedAccountGroupChange', () => { diff --git a/packages/snap-account-service/src/SnapAccountService.ts b/packages/snap-account-service/src/SnapAccountService.ts index b82d4fb919a..6dc2b4d7df2 100644 --- a/packages/snap-account-service/src/SnapAccountService.ts +++ b/packages/snap-account-service/src/SnapAccountService.ts @@ -841,46 +841,157 @@ export class SnapAccountService { } /** - * Publishes an account data update event from a Snap. + * Publishes an account data update event from a Snap, filtered to the + * accounts that the Snap actually owns. + * + * A Snap can emit `notify:accountTransactionsUpdated`, + * `notify:accountBalancesUpdated`, and `notify:accountAssetListUpdated` for + * account IDs it does not own. Forwarding those updates verbatim would let + * one Snap forge transactions, balances, or asset-list entries for accounts + * owned by another Snap (or for accounts that do not exist at all). This + * method restores the per-Snap ownership predicate that + * `SnapKeyringV1.handleKeyringSnapMessage` used to enforce before the + * non-keyring short-circuit bypassed it (see core#8916): it asks the Snap's + * own v2 keyring — via the lock-free `withKeyringV2Unsafe` path — which of + * the reported account IDs it tracks, drops any it does not, and only then + * re-emits the (now-verified) payload. Unknown account IDs are dropped with + * a warning rather than throwing, so a malicious or buggy Snap cannot use a + * bogus ID to abort the whole batch (and DoS other consumers). * * @param snapId - ID of the Snap. * @param event - Account data update event. * @param message - Message sent by the Snap. * @returns `null`. */ - #publishAccountDataUpdatedEvent( + async #publishAccountDataUpdatedEvent( snapId: SnapId, event: AccountDataUpdatedKeyringEvent, message: SnapMessage, - ): null { + ): Promise { log( `Forwarding message "${event}" from Snap "${snapId}" as a SnapAccountService event...`, ); if (event === KeyringEvent.AccountAssetListUpdated) { assertStruct(message, AccountAssetListUpdatedEventStruct); - this.#messenger.publish( - 'SnapAccountService:accountAssetListUpdated', - message.params, + const assets = await this.#filterOwnedAccountEntries( + snapId, + event, + message.params.assets, ); + // Nothing verified to forward — drop the event entirely. + if (Object.keys(assets).length > 0) { + this.#messenger.publish('SnapAccountService:accountAssetListUpdated', { + ...message.params, + assets, + }); + } else { + log( + `Dropping "${event}" from Snap "${snapId}": no Snap-owned accounts in the update.`, + ); + } } else if (event === KeyringEvent.AccountBalancesUpdated) { assertStruct(message, AccountBalancesUpdatedEventStruct); - this.#messenger.publish( - 'SnapAccountService:accountBalancesUpdated', - message.params, + const balances = await this.#filterOwnedAccountEntries( + snapId, + event, + message.params.balances, ); + if (Object.keys(balances).length > 0) { + this.#messenger.publish('SnapAccountService:accountBalancesUpdated', { + ...message.params, + balances, + }); + } else { + log( + `Dropping "${event}" from Snap "${snapId}": no Snap-owned accounts in the update.`, + ); + } } else if (event === KeyringEvent.AccountTransactionsUpdated) { assertStruct(message, AccountTransactionsUpdatedEventStruct); - this.#messenger.publish( - 'SnapAccountService:accountTransactionsUpdated', - message.params, + const transactions = await this.#filterOwnedAccountEntries( + snapId, + event, + message.params.transactions, ); + if (Object.keys(transactions).length > 0) { + this.#messenger.publish( + 'SnapAccountService:accountTransactionsUpdated', + { + ...message.params, + transactions, + }, + ); + } else { + log( + `Dropping "${event}" from Snap "${snapId}": no Snap-owned accounts in the update.`, + ); + } } // We need to return a valid JSON value, so we cannot use `undefined` here. return null; } + /** + * Filters an account-keyed map from a Snap's account data update event down + * to the entries whose account ID is owned by the Snap. + * + * Ownership is determined by the Snap's own v2 keyring (`keyring.hasAccount`), + * which is the same per-Snap registry predicate that + * `SnapKeyringV1.handleKeyringSnapMessage` enforced before core#8916 + * short-circuited it. The lookup uses the lock-free + * `withKeyringV2Unsafe` path, so it does not acquire the keyring lock and is + * safe to call from this event path. If no keyring exists for the Snap yet, + * ownership cannot be verified, so the method fails closed (returns an empty + * map) instead of forwarding unverified data. + * + * @param snapId - ID of the Snap that emitted the event. + * @param event - The account data update event being filtered. + * @param entries - The account-keyed map to filter. + * @returns A new map containing only the entries for accounts the Snap owns. + */ + async #filterOwnedAccountEntries( + snapId: SnapId, + event: AccountDataUpdatedKeyringEvent, + entries: Record, + ): Promise> { + const accountIds = Object.keys(entries); + if (accountIds.length === 0) { + return {}; + } + + let ownedAccountIds: Set; + try { + ownedAccountIds = await this.#withKeyringV2Unsafe(snapId, async (keyring) => + new Set(accountIds.filter((id) => keyring.hasAccount(id))), + ); + } catch (error) { + if (isKeyringNotFoundError(error)) { + // No keyring for this Snap yet — we cannot confirm ownership of any + // reported account, so fail closed and drop the whole update rather + // than forward unverified data. + log( + `No Snap keyring found for Snap "${snapId}" while verifying ownership for "${event}". Dropping ${accountIds.length} account update(s).`, + ); + return {}; + } + throw error; + } + + const filtered: Record = {}; + for (const accountId of accountIds) { + if (ownedAccountIds.has(accountId)) { + filtered[accountId] = entries[accountId]; + } else { + log( + `Snap "${snapId}" reported "${event}" for account "${accountId}" it does not own. Skipping.`, + ); + } + } + return filtered; + } + // eslint-disable-next-line jsdoc/require-returns /** * Forwards the accounts of the given account group to the Snap keyring. From 6eba5a491a6523193c9413fae37da64664754c60 Mon Sep 17 00:00:00 2001 From: gantunesr <17601467+gantunesr@users.noreply.github.com> Date: Tue, 1 Sep 2026 15:58:30 -0400 Subject: [PATCH 2/3] fix: lint --- packages/snap-account-service/src/SnapAccountService.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/snap-account-service/src/SnapAccountService.ts b/packages/snap-account-service/src/SnapAccountService.ts index 6dc2b4d7df2..af37d068a74 100644 --- a/packages/snap-account-service/src/SnapAccountService.ts +++ b/packages/snap-account-service/src/SnapAccountService.ts @@ -879,7 +879,6 @@ export class SnapAccountService { event, message.params.assets, ); - // Nothing verified to forward — drop the event entirely. if (Object.keys(assets).length > 0) { this.#messenger.publish('SnapAccountService:accountAssetListUpdated', { ...message.params, From 5f949a322a362eac40e69a775c8ceb3c74f1a1a7 Mon Sep 17 00:00:00 2001 From: gantunesr <17601467+gantunesr@users.noreply.github.com> Date: Wed, 2 Sep 2026 12:18:07 -0400 Subject: [PATCH 3/3] fix!: filter account data updates by Snap ownership via AccountsController --- packages/snap-account-service/CHANGELOG.md | 6 +- packages/snap-account-service/package.json | 1 + .../src/SnapAccountService.test.ts | 152 +++++++++++++++--- .../src/SnapAccountService.ts | 127 +++++++++------ packages/snap-account-service/src/types.ts | 9 ++ yarn.lock | 1 + 6 files changed, 219 insertions(+), 77 deletions(-) diff --git a/packages/snap-account-service/CHANGELOG.md b/packages/snap-account-service/CHANGELOG.md index cd753b0a5df..1d1d94d12e6 100644 --- a/packages/snap-account-service/CHANGELOG.md +++ b/packages/snap-account-service/CHANGELOG.md @@ -7,9 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -### Fixed +### Changed + +- **BREAKING:** `SnapAccountService` now reads `AccountsController` state to filter account data update events by Snap ownership ([#8916](https://github.com/MetaMask/core/pull/8916)). + - Filter account data update events (`notify:accountTransactionsUpdated`, `notify:accountBalancesUpdated`, and `notify:accountAssetListUpdated`) to the accounts that the originating Snap actually owns before republishing them -- Filter account data update events (`notify:accountTransactionsUpdated`, `notify:accountBalancesUpdated`, and `notify:accountAssetListUpdated`) to the accounts that the originating Snap actually owns before republishing them ([#8916](https://github.com/MetaMask/core/pull/8916)) ## [2.1.2] diff --git a/packages/snap-account-service/package.json b/packages/snap-account-service/package.json index b911eeb979a..ac1e1ae8b54 100644 --- a/packages/snap-account-service/package.json +++ b/packages/snap-account-service/package.json @@ -56,6 +56,7 @@ }, "dependencies": { "@metamask/account-api": "^2.0.0", + "@metamask/accounts-controller": "^39.1.0", "@metamask/eth-snap-keyring": "^24.0.0", "@metamask/keyring-api": "^24.0.0", "@metamask/keyring-controller": "^27.1.1", diff --git a/packages/snap-account-service/src/SnapAccountService.test.ts b/packages/snap-account-service/src/SnapAccountService.test.ts index 6df95961189..d2466505b52 100644 --- a/packages/snap-account-service/src/SnapAccountService.test.ts +++ b/packages/snap-account-service/src/SnapAccountService.test.ts @@ -1,4 +1,5 @@ import type { AccountGroupId } from '@metamask/account-api'; +import type { AccountsControllerState } from '@metamask/accounts-controller'; import { SNAP_KEYRING_TYPE } from '@metamask/eth-snap-keyring'; import type { SnapMessage } from '@metamask/eth-snap-keyring'; import type { SnapKeyring as SnapKeyringV2 } from '@metamask/eth-snap-keyring/v2'; @@ -85,6 +86,10 @@ type Mocks = { >; getSelectedAccountGroup: jest.MockedFunction<() => AccountGroupId | ''>; }; + // eslint-disable-next-line @typescript-eslint/naming-convention + AccountsController: { + getState: jest.MockedFunction<() => AccountsControllerState>; + }; }; /** @@ -126,6 +131,7 @@ function getMessenger( 'KeyringController:withKeyringV2Unsafe', 'AccountTreeController:getAccountGroupObject', 'AccountTreeController:getSelectedAccountGroup', + 'AccountsController:getState', ], events: [ 'SnapController:stateChange', @@ -141,6 +147,7 @@ function getMessenger( 'AccountTreeController:accountGroupCreated', 'AccountTreeController:accountGroupUpdated', 'AccountTreeController:accountGroupRemoved', + 'AccountsController:stateChanged', ], }); return messenger; @@ -232,6 +239,49 @@ function buildGroup( return { id, accounts } as MockAccountGroup as AccountGroupObject; } +/** + * Builds a minimal `AccountsControllerState` whose `internalAccounts.accounts` + * maps each given account ID to an account owned by `snapId` (via + * `metadata.snap.id`). Used to seed the service's Snap-ownership cache. + * + * @param accounts - The accounts to include. + * @returns A minimal `AccountsControllerState`. + */ +function buildAccountsState( + accounts: { id: string; snapId?: string }[], +): AccountsControllerState { + const accountsRecord = Object.fromEntries( + accounts.map(({ id, snapId }) => [ + id, + { + id, + metadata: snapId ? { snap: { id: snapId } } : {}, + }, + ]), + ); + return { + internalAccounts: { accounts: accountsRecord }, + } as unknown as AccountsControllerState; +} + +/** + * Publishes an `AccountsController:stateChanged` event on the root messenger, + * rebuilding the service's Snap-ownership cache from the given accounts. + * + * @param rootMessenger - The root messenger. + * @param accounts - The accounts to include in the new state. + */ +function publishAccountsStateChange( + rootMessenger: RootMessenger, + accounts: { id: string; snapId?: string }[], +): void { + rootMessenger.publish( + 'AccountsController:stateChanged', + buildAccountsState(accounts), + [], + ); +} + /** * Publishes an AccountTreeController accountGroupCreated event on the root * messenger. @@ -403,6 +453,7 @@ function mockWithKeyringV2Unsafe( * @param args - The arguments to this function. * @param args.snapIsReady - Initial value of `SnapController.isReady`. * @param args.runnableSnaps - Snaps returned by `SnapController:getRunnableSnaps`. + * @param args.accounts - Initial accounts * @param args.config - Optional service config. * @param args.captureException - Optional method to capture exceptions in Sentry. * @returns The new service, root messenger, service messenger, and mocks. @@ -410,11 +461,13 @@ function mockWithKeyringV2Unsafe( async function setup({ snapIsReady = true, runnableSnaps = [], + accounts = [], config, captureException, }: { snapIsReady?: boolean; runnableSnaps?: TruncatedSnap[]; + accounts?: { id: string; snapId?: string }[]; config?: SnapAccountServiceOptions['config']; captureException?: (error: Error) => void; } = {}): Promise<{ @@ -444,6 +497,9 @@ async function setup({ getAccountGroupObject: jest.fn().mockReturnValue(undefined), getSelectedAccountGroup: jest.fn().mockReturnValue(''), }, + AccountsController: { + getState: jest.fn().mockReturnValue(buildAccountsState(accounts)), + }, }; rootMessenger.registerActionHandler( @@ -482,6 +538,10 @@ async function setup({ 'AccountTreeController:getSelectedAccountGroup', mocks.AccountTreeController.getSelectedAccountGroup, ); + rootMessenger.registerActionHandler( + 'AccountsController:getState', + mocks.AccountsController.getState, + ); const service = new SnapAccountService({ messenger, config }); @@ -1232,17 +1292,18 @@ describe('SnapAccountService', () => { ] as const)( 'filters %s to accounts owned by the Snap before republishing it', async (method, event, key, payload) => { - const { service, rootMessenger, mocks } = await setup(); + const { service, rootMessenger, mocks } = await setup({ + accounts: [ + { id: MOCK_ACCOUNT_ID, snapId: MOCK_SNAP_ID as string }, + { + id: MOCK_UNOWNED_ACCOUNT_ID, + snapId: MOCK_OTHER_SNAP_ID as string, + }, + ], + }); const listener = jest.fn(); rootMessenger.subscribe(event, listener); - // The Snap only owns MOCK_ACCOUNT_ID; the unowned ID must be dropped. - mockWithKeyringV2Unsafe(mocks, { - [MOCK_SNAP_ID]: { - hasAccount: (id: string) => id === MOCK_ACCOUNT_ID, - }, - }); - expect(service).toBeDefined(); const result = await service.handleKeyringSnapMessage(MOCK_SNAP_ID, { @@ -1252,37 +1313,36 @@ describe('SnapAccountService', () => { expect(result).toBeNull(); // Only the owned account survives the ownership filter. + const expectedEntry = ( + payload as Record> + )[key][MOCK_ACCOUNT_ID]; expect(listener).toHaveBeenCalledTimes(1); expect(listener).toHaveBeenCalledWith({ - [key]: { [MOCK_ACCOUNT_ID]: payload[key][MOCK_ACCOUNT_ID] }, + [key]: { [MOCK_ACCOUNT_ID]: expectedEntry }, }); - // The ownership filter must actually run on the live path — this + // The ownership filter is a synchronous AccountsController-state + // cache read — it must NOT touch the keyring on the live path. This // assertion previously locked in the bypass (core#8916) and is now - // inverted. + // inverted to require the cache, not the keyring. expect( mocks.KeyringController.withKeyringV2Unsafe, - ).toHaveBeenCalledTimes(1); + ).not.toHaveBeenCalled(); expect(mocks.KeyringController.withController).not.toHaveBeenCalled(); }, ); - it('drops the whole update (and does not throw) when the Snap keyring does not exist yet', async () => { - const { service, rootMessenger, mocks } = await setup(); + it('drops the whole update when no reported account is owned by the Snap (fail closed)', async () => { + const { service, rootMessenger } = await setup({ + accounts: [ + { id: MOCK_ACCOUNT_ID, snapId: MOCK_OTHER_SNAP_ID as string }, + ], + }); const listener = jest.fn(); rootMessenger.subscribe( 'SnapAccountService:accountBalancesUpdated', listener, ); - // No keyring registered for the Snap -> the ownership lookup throws - // KeyringNotFound. The handler must fail closed (drop the update) - // rather than forward unverified data or let the error escape. - mocks.KeyringController.withKeyringV2Unsafe.mockRejectedValue( - new KeyringControllerError( - KeyringControllerErrorMessage.KeyringNotFound, - ), - ); - const payload = { balances: { [MOCK_ACCOUNT_ID]: { @@ -1297,8 +1357,54 @@ describe('SnapAccountService', () => { } as unknown as SnapMessage); expect(result).toBeNull(); + expect(listener).not.toHaveBeenCalled(); + }); + + it('picks up ownership changes from AccountsController:stateChanged', async () => { + // Initially the Snap does not own the account, so the update is dropped. + const { service, rootMessenger } = await setup({ + accounts: [ + { id: MOCK_ACCOUNT_ID, snapId: MOCK_OTHER_SNAP_ID as string }, + ], + }); + const listener = jest.fn(); + rootMessenger.subscribe( + 'SnapAccountService:accountBalancesUpdated', + listener, + ); + + const payload = { + balances: { + [MOCK_ACCOUNT_ID]: { + 'eip155:1/slip44:60': { amount: '1', unit: 'ETH' }, + }, + }, + } satisfies AccountBalancesUpdatedEventPayload; + let result = await service.handleKeyringSnapMessage(MOCK_SNAP_ID, { + method: KeyringEvent.AccountBalancesUpdated, + params: payload, + } as unknown as SnapMessage); + expect(result).toBeNull(); expect(listener).not.toHaveBeenCalled(); + + // The account is now transferred to this Snap — the cache rebuilds on + // stateChanged and the next update is forwarded. + publishAccountsStateChange(rootMessenger, [ + { id: MOCK_ACCOUNT_ID, snapId: MOCK_SNAP_ID as string }, + ]); + + result = await service.handleKeyringSnapMessage(MOCK_SNAP_ID, { + method: KeyringEvent.AccountBalancesUpdated, + params: payload, + } as unknown as SnapMessage); + expect(result).toBeNull(); + expect(listener).toHaveBeenCalledTimes(1); + expect(listener).toHaveBeenCalledWith({ + balances: { + [MOCK_ACCOUNT_ID]: payload.balances[MOCK_ACCOUNT_ID], + }, + }); }); }); diff --git a/packages/snap-account-service/src/SnapAccountService.ts b/packages/snap-account-service/src/SnapAccountService.ts index af37d068a74..75ac7576ca3 100644 --- a/packages/snap-account-service/src/SnapAccountService.ts +++ b/packages/snap-account-service/src/SnapAccountService.ts @@ -1,4 +1,8 @@ import { AccountGroupId } from '@metamask/account-api'; +import type { + AccountsControllerGetStateAction, + AccountsControllerState, +} from '@metamask/accounts-controller'; import { SnapKeyring as LegacySnapKeyring, SnapMessage, @@ -89,6 +93,7 @@ import type { AccountTreeControllerAccountGroupCreatedEvent, AccountTreeControllerAccountGroupUpdatedEvent, AccountTreeControllerAccountGroupRemovedEvent, + AccountsControllerStateChangedEvent, AccountGroupObject, } from './types.js'; @@ -143,7 +148,8 @@ type AllowedActions = | KeyringControllerWithKeyringV2Action | KeyringControllerWithKeyringV2UnsafeAction | AccountTreeControllerGetAccountGroupObjectAction - | AccountTreeControllerGetSelectedAccountGroupAction; + | AccountTreeControllerGetSelectedAccountGroupAction + | AccountsControllerGetStateAction; /** * Events that {@link SnapAccountService} exposes to other consumers. @@ -184,7 +190,8 @@ type AllowedEvents = | AccountTreeControllerSelectedAccountGroupChangeEvent | AccountTreeControllerAccountGroupCreatedEvent | AccountTreeControllerAccountGroupUpdatedEvent - | AccountTreeControllerAccountGroupRemovedEvent; + | AccountTreeControllerAccountGroupRemovedEvent + | AccountsControllerStateChangedEvent; /** * The messenger which is restricted to actions and events accessed by @@ -271,6 +278,17 @@ export class SnapAccountService { #migratePromise: Promise | null = null; + /** + * Cache mapping each Snap-owned account ID to the ID of the Snap that owns + * it, derived from `AccountsController` state. + */ + #accountSnapIds: Map = new Map(); + + /** + * Whether `#accountSnapIds` has been populated yet. + */ + #accountSnapCacheInitialized = false; + /** * Constructs a new {@link SnapAccountService}. * @@ -298,6 +316,16 @@ export class SnapAccountService { MESSENGER_EXPOSED_METHODS, ); + // Keep the Snap-ownership cache in sync as accounts are added/removed. + // The initial cache is built lazily on first use (see + // `#ensureAccountSnapCache`) rather than in the constructor, so that this + // service does not force clients to instantiate `AccountsController` + // before it. This keeps the account data update event path synchronous — + // the cache is a plain `Map` read. + this.#messenger.subscribe('AccountsController:stateChanged', (state) => + this.#rebuildAccountSnapCache(state), + ); + this.#messenger.subscribe( 'AccountTreeController:selectedAccountGroupChange', (groupId) => this.#handleSelectedAccountGroupChange(groupId), @@ -848,33 +876,30 @@ export class SnapAccountService { * `notify:accountBalancesUpdated`, and `notify:accountAssetListUpdated` for * account IDs it does not own. Forwarding those updates verbatim would let * one Snap forge transactions, balances, or asset-list entries for accounts - * owned by another Snap (or for accounts that do not exist at all). This - * method restores the per-Snap ownership predicate that - * `SnapKeyringV1.handleKeyringSnapMessage` used to enforce before the - * non-keyring short-circuit bypassed it (see core#8916): it asks the Snap's - * own v2 keyring — via the lock-free `withKeyringV2Unsafe` path — which of - * the reported account IDs it tracks, drops any it does not, and only then - * re-emits the (now-verified) payload. Unknown account IDs are dropped with - * a warning rather than throwing, so a malicious or buggy Snap cannot use a - * bogus ID to abort the whole batch (and DoS other consumers). + * owned by another Snap (or for accounts that do not exist at all). + * + * The cache is rebuilt lazily on first use and on every + * `AccountsController:stateChanged`, so the lookup here is a synchronous + * `Map` read — preserving synchronous event handling and avoiding a + * per-event keyring round-trip on a path that fires frequently. * * @param snapId - ID of the Snap. * @param event - Account data update event. * @param message - Message sent by the Snap. * @returns `null`. */ - async #publishAccountDataUpdatedEvent( + #publishAccountDataUpdatedEvent( snapId: SnapId, event: AccountDataUpdatedKeyringEvent, message: SnapMessage, - ): Promise { + ): null { log( `Forwarding message "${event}" from Snap "${snapId}" as a SnapAccountService event...`, ); if (event === KeyringEvent.AccountAssetListUpdated) { assertStruct(message, AccountAssetListUpdatedEventStruct); - const assets = await this.#filterOwnedAccountEntries( + const assets = this.#filterOwnedAccountEntries( snapId, event, message.params.assets, @@ -891,7 +916,7 @@ export class SnapAccountService { } } else if (event === KeyringEvent.AccountBalancesUpdated) { assertStruct(message, AccountBalancesUpdatedEventStruct); - const balances = await this.#filterOwnedAccountEntries( + const balances = this.#filterOwnedAccountEntries( snapId, event, message.params.balances, @@ -908,7 +933,7 @@ export class SnapAccountService { } } else if (event === KeyringEvent.AccountTransactionsUpdated) { assertStruct(message, AccountTransactionsUpdatedEventStruct); - const transactions = await this.#filterOwnedAccountEntries( + const transactions = this.#filterOwnedAccountEntries( snapId, event, message.params.transactions, @@ -936,52 +961,22 @@ export class SnapAccountService { * Filters an account-keyed map from a Snap's account data update event down * to the entries whose account ID is owned by the Snap. * - * Ownership is determined by the Snap's own v2 keyring (`keyring.hasAccount`), - * which is the same per-Snap registry predicate that - * `SnapKeyringV1.handleKeyringSnapMessage` enforced before core#8916 - * short-circuited it. The lookup uses the lock-free - * `withKeyringV2Unsafe` path, so it does not acquire the keyring lock and is - * safe to call from this event path. If no keyring exists for the Snap yet, - * ownership cannot be verified, so the method fails closed (returns an empty - * map) instead of forwarding unverified data. * * @param snapId - ID of the Snap that emitted the event. * @param event - The account data update event being filtered. * @param entries - The account-keyed map to filter. * @returns A new map containing only the entries for accounts the Snap owns. */ - async #filterOwnedAccountEntries( + #filterOwnedAccountEntries( snapId: SnapId, event: AccountDataUpdatedKeyringEvent, entries: Record, - ): Promise> { - const accountIds = Object.keys(entries); - if (accountIds.length === 0) { - return {}; - } - - let ownedAccountIds: Set; - try { - ownedAccountIds = await this.#withKeyringV2Unsafe(snapId, async (keyring) => - new Set(accountIds.filter((id) => keyring.hasAccount(id))), - ); - } catch (error) { - if (isKeyringNotFoundError(error)) { - // No keyring for this Snap yet — we cannot confirm ownership of any - // reported account, so fail closed and drop the whole update rather - // than forward unverified data. - log( - `No Snap keyring found for Snap "${snapId}" while verifying ownership for "${event}". Dropping ${accountIds.length} account update(s).`, - ); - return {}; - } - throw error; - } - + ): Record { + this.#ensureAccountSnapCache(); const filtered: Record = {}; - for (const accountId of accountIds) { - if (ownedAccountIds.has(accountId)) { - filtered[accountId] = entries[accountId]; + for (const [accountId, value] of Object.entries(entries)) { + if (this.#accountSnapIds.get(accountId) === snapId) { + filtered[accountId] = value; } else { log( `Snap "${snapId}" reported "${event}" for account "${accountId}" it does not own. Skipping.`, @@ -991,6 +986,34 @@ export class SnapAccountService { return filtered; } + /** + * Rebuilds the Snap-ownership cache from `AccountsController` state. + * + * @param state - The current `AccountsController` state. + */ + #rebuildAccountSnapCache(state: AccountsControllerState): void { + const cache = new Map(); + for (const account of Object.values(state.internalAccounts.accounts)) { + const snapId = account.metadata?.snap?.id; + if (snapId) { + cache.set(account.id, snapId as SnapId); + } + } + this.#accountSnapIds = cache; + this.#accountSnapCacheInitialized = true; + } + + /** + * Lazily builds the Snap-ownership cache on first use. + */ + #ensureAccountSnapCache(): void { + if (!this.#accountSnapCacheInitialized) { + this.#rebuildAccountSnapCache( + this.#messenger.call('AccountsController:getState'), + ); + } + } + // eslint-disable-next-line jsdoc/require-returns /** * Forwards the accounts of the given account group to the Snap keyring. diff --git a/packages/snap-account-service/src/types.ts b/packages/snap-account-service/src/types.ts index e9485a7d5fe..634641a408e 100644 --- a/packages/snap-account-service/src/types.ts +++ b/packages/snap-account-service/src/types.ts @@ -1,4 +1,5 @@ import type { AccountGroupId } from '@metamask/account-api'; +import type { AccountsControllerState } from '@metamask/accounts-controller'; import { AccountId } from '@metamask/keyring-utils'; /* @@ -76,3 +77,11 @@ export type AccountTreeControllerAccountGroupRemovedEvent = { type: `AccountTreeController:accountGroupRemoved`; payload: [AccountGroupId]; }; + +/** + * Mirror of the `AccountsControllerStateChangedEvent`. + */ +export type AccountsControllerStateChangedEvent = { + type: `AccountsController:stateChanged`; + payload: [AccountsControllerState, unknown[]]; +}; diff --git a/yarn.lock b/yarn.lock index f9a964eee53..843faf8947d 100644 --- a/yarn.lock +++ b/yarn.lock @@ -9017,6 +9017,7 @@ __metadata: resolution: "@metamask/snap-account-service@workspace:packages/snap-account-service" dependencies: "@metamask/account-api": "npm:^2.0.0" + "@metamask/accounts-controller": "npm:^39.1.0" "@metamask/auto-changelog": "npm:^6.1.0" "@metamask/eth-snap-keyring": "npm:^24.0.0" "@metamask/keyring-api": "npm:^24.0.0"