diff --git a/packages/snap-account-service/CHANGELOG.md b/packages/snap-account-service/CHANGELOG.md index a17cceb609f..0c1c2b3cd85 100644 --- a/packages/snap-account-service/CHANGELOG.md +++ b/packages/snap-account-service/CHANGELOG.md @@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### 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 - Bump `@metamask/utils` from `^11.11.0` to `^11.12.0` ([#10076](https://github.com/MetaMask/core/pull/10076)) ## [2.1.2] diff --git a/packages/snap-account-service/package.json b/packages/snap-account-service/package.json index 67df3bb665b..9c73e02d930 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 fe36efdcf20..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 }); @@ -1182,39 +1242,65 @@ 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) => { - const { service, rootMessenger, mocks } = await setup(); + 'filters %s to accounts owned by the Snap before republishing it', + async (method, event, key, payload) => { + 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); @@ -1226,13 +1312,100 @@ describe('SnapAccountService', () => { } as unknown as SnapMessage); expect(result).toBeNull(); - expect(listener).toHaveBeenCalledWith(payload); + // 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]: expectedEntry }, + }); + // 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 to require the cache, not the keyring. expect( mocks.KeyringController.withKeyringV2Unsafe, ).not.toHaveBeenCalled(); expect(mocks.KeyringController.withController).not.toHaveBeenCalled(); }, ); + + 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, + ); + + 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(); + }); + + 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], + }, + }); + }); }); describe('on AccountTreeController:selectedAccountGroupChange', () => { diff --git a/packages/snap-account-service/src/SnapAccountService.ts b/packages/snap-account-service/src/SnapAccountService.ts index b82d4fb919a..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), @@ -841,7 +869,19 @@ 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). + * + * 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. @@ -859,28 +899,121 @@ export class SnapAccountService { if (event === KeyringEvent.AccountAssetListUpdated) { assertStruct(message, AccountAssetListUpdatedEventStruct); - this.#messenger.publish( - 'SnapAccountService:accountAssetListUpdated', - message.params, + const assets = this.#filterOwnedAccountEntries( + snapId, + event, + message.params.assets, ); + 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 = 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 = 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. + * + * + * @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. + */ + #filterOwnedAccountEntries( + snapId: SnapId, + event: AccountDataUpdatedKeyringEvent, + entries: Record, + ): Record { + this.#ensureAccountSnapCache(); + const filtered: Record = {}; + 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.`, + ); + } + } + 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 1e88d4058a7..8863d49594e 100644 --- a/yarn.lock +++ b/yarn.lock @@ -8882,6 +8882,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"