diff --git a/README.md b/README.md index 27f8d326fe2..e3ada501ab1 100644 --- a/README.md +++ b/README.md @@ -609,6 +609,7 @@ linkStyle default opacity:0.5 smart_transactions_controller --> remote_feature_flag_controller; smart_transactions_controller --> transaction_controller; smart_transactions_controller --> json_rpc_engine; + snap_account_service --> accounts_controller; snap_account_service --> keyring_controller; snap_account_service --> messenger; social_controllers --> base_controller; diff --git a/packages/snap-account-service/CHANGELOG.md b/packages/snap-account-service/CHANGELOG.md index a17cceb609f..051a84a5c61 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 ([#10057](https://github.com/MetaMask/core/pull/10057)). + - 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..775238cc177 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.1", "@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..891b914fbf3 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,8 @@ function getMessenger( 'AccountTreeController:accountGroupCreated', 'AccountTreeController:accountGroupUpdated', 'AccountTreeController:accountGroupRemoved', + 'AccountsController:accountsAdded', + 'AccountsController:accountsRemoved', ], }); return messenger; @@ -232,6 +240,66 @@ 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:accountsAdded` event on the root messenger, + * adding the given accounts to the service's Snap-ownership cache. + * + * @param rootMessenger - The root messenger. + * @param accounts - The accounts that were added. + */ +function publishAccountsAdded( + rootMessenger: RootMessenger, + accounts: { id: string; snapId?: string }[], +): void { + rootMessenger.publish( + 'AccountsController:accountsAdded', + accounts.map(({ id, snapId }) => ({ + id, + metadata: snapId ? { snap: { id: snapId } } : {}, + })), + ); +} + +/** + * Publishes an `AccountsController:accountsRemoved` event on the root + * messenger, removing the given account IDs from the service's Snap-ownership + * cache. + * + * @param rootMessenger - The root messenger. + * @param accountIds - The IDs of the accounts that were removed. + */ +function publishAccountsRemoved( + rootMessenger: RootMessenger, + accountIds: string[], +): void { + rootMessenger.publish('AccountsController:accountsRemoved', accountIds); +} + /** * Publishes an AccountTreeController accountGroupCreated event on the root * messenger. @@ -403,6 +471,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 +479,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 +515,9 @@ async function setup({ getAccountGroupObject: jest.fn().mockReturnValue(undefined), getSelectedAccountGroup: jest.fn().mockReturnValue(''), }, + AccountsController: { + getState: jest.fn().mockReturnValue(buildAccountsState(accounts)), + }, }; rootMessenger.registerActionHandler( @@ -482,6 +556,10 @@ async function setup({ 'AccountTreeController:getSelectedAccountGroup', mocks.AccountTreeController.getSelectedAccountGroup, ); + rootMessenger.registerActionHandler( + 'AccountsController:getState', + mocks.AccountsController.getState, + ); const service = new SnapAccountService({ messenger, config }); @@ -1182,39 +1260,71 @@ 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'; + // An account ID that has no Snap owner. Updates for this ID must be + // stripped too — it is owned by nobody. + const MOCK_NO_SNAP_ACCOUNT_ID = '00000000-0000-4000-8000-000000000003'; 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, + }, + // An account with no Snap owner must be skipped when building the + // cache (it is owned by nobody). + { id: MOCK_NO_SNAP_ACCOUNT_ID }, + ], + }); const listener = jest.fn(); rootMessenger.subscribe(event, listener); @@ -1226,13 +1336,137 @@ 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.each([ + [ + KeyringEvent.AccountBalancesUpdated, + 'SnapAccountService:accountBalancesUpdated' as const, + { + balances: { + [MOCK_ACCOUNT_ID]: { + 'eip155:1/slip44:60': { amount: '1', unit: 'ETH' }, + }, + }, + } satisfies AccountBalancesUpdatedEventPayload, + ], + [ + KeyringEvent.AccountAssetListUpdated, + 'SnapAccountService:accountAssetListUpdated' as const, + { + assets: { + [MOCK_ACCOUNT_ID]: { added: ['eip155:1/slip44:60'], removed: [] }, + }, + } satisfies AccountAssetListUpdatedEventPayload, + ], + [ + KeyringEvent.AccountTransactionsUpdated, + 'SnapAccountService:accountTransactionsUpdated' as const, + { + transactions: { + [MOCK_ACCOUNT_ID]: [], + }, + } satisfies AccountTransactionsUpdatedEventPayload, + ], + ] as const)( + 'drops the whole %s update when no reported account is owned by the Snap (fail closed)', + async (method, event, payload) => { + const { service, rootMessenger } = await setup({ + accounts: [ + { id: MOCK_ACCOUNT_ID, snapId: MOCK_OTHER_SNAP_ID as string }, + ], + }); + const listener = jest.fn(); + rootMessenger.subscribe(event, listener); + + const result = await service.handleKeyringSnapMessage(MOCK_SNAP_ID, { + method, + params: payload, + } as unknown as SnapMessage); + + expect(result).toBeNull(); + expect(listener).not.toHaveBeenCalled(); + }, + ); + + it('picks up added/removed accounts from AccountsController:accountsAdded and :accountsRemoved', 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 added for this Snap — the cache picks it up from + // `accountsAdded` and the next update is forwarded. A no-Snap account + // is included to verify such accounts are skipped when updating the + // cache incrementally. + publishAccountsAdded(rootMessenger, [ + { id: MOCK_ACCOUNT_ID, snapId: MOCK_SNAP_ID as string }, + { id: MOCK_NO_SNAP_ACCOUNT_ID }, + ]); + + 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], + }, + }); + + // The account is removed — the cache drops it and the next update is + // dropped again (fail closed). + publishAccountsRemoved(rootMessenger, [MOCK_ACCOUNT_ID]); + + listener.mockClear(); + 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..6675ac02d77 100644 --- a/packages/snap-account-service/src/SnapAccountService.ts +++ b/packages/snap-account-service/src/SnapAccountService.ts @@ -1,4 +1,10 @@ import { AccountGroupId } from '@metamask/account-api'; +import type { + AccountsControllerAccountsAddedEvent, + AccountsControllerAccountsRemovedEvent, + AccountsControllerGetStateAction, + AccountsControllerState, +} from '@metamask/accounts-controller'; import { SnapKeyring as LegacySnapKeyring, SnapMessage, @@ -143,7 +149,8 @@ type AllowedActions = | KeyringControllerWithKeyringV2Action | KeyringControllerWithKeyringV2UnsafeAction | AccountTreeControllerGetAccountGroupObjectAction - | AccountTreeControllerGetSelectedAccountGroupAction; + | AccountTreeControllerGetSelectedAccountGroupAction + | AccountsControllerGetStateAction; /** * Events that {@link SnapAccountService} exposes to other consumers. @@ -184,7 +191,9 @@ type AllowedEvents = | AccountTreeControllerSelectedAccountGroupChangeEvent | AccountTreeControllerAccountGroupCreatedEvent | AccountTreeControllerAccountGroupUpdatedEvent - | AccountTreeControllerAccountGroupRemovedEvent; + | AccountTreeControllerAccountGroupRemovedEvent + | AccountsControllerAccountsAddedEvent + | AccountsControllerAccountsRemovedEvent; /** * The messenger which is restricted to actions and events accessed by @@ -271,6 +280,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 +318,22 @@ 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 + // `#initAccountSnapCache`) 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. The granular `accountsAdded` / + // `accountsRemoved` events (batch-compatible) update the cache + // incrementally instead of rebuilding it from full state on every change. + this.#messenger.subscribe('AccountsController:accountsAdded', (accounts) => + this.#addAccountsToCache(accounts), + ); + this.#messenger.subscribe( + 'AccountsController:accountsRemoved', + (accountIds) => this.#removeAccountsFromCache(accountIds), + ); + this.#messenger.subscribe( 'AccountTreeController:selectedAccountGroupChange', (groupId) => this.#handleSelectedAccountGroupChange(groupId), @@ -841,7 +877,20 @@ 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 built lazily on first use from `AccountsController:getState` + * and kept in sync incrementally via `AccountsController:accountsAdded` / + * `AccountsController:accountsRemoved`, 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 +908,153 @@ 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.#initAccountSnapCache(); + 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. + * + * Used for lazy initialization on first use; subsequent updates are applied + * incrementally by {@link SnapAccountService.#addAccountsToCache} and + * {@link SnapAccountService.#removeAccountsFromCache}. + * + * @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; + } + + /** + * Adds the given accounts to the Snap-ownership cache. + * + * @param accounts - The accounts that were added. + */ + #addAccountsToCache( + accounts: AccountsControllerAccountsAddedEvent['payload'][0], + ): void { + for (const account of accounts) { + const snapId = account.metadata?.snap?.id; + if (snapId) { + this.#accountSnapIds.set(account.id, snapId as SnapId); + } + } + } + + /** + * Removes the given account IDs from the Snap-ownership cache. + * + * @param accountIds - The IDs of the accounts that were removed. + */ + #removeAccountsFromCache( + accountIds: AccountsControllerAccountsRemovedEvent['payload'][0], + ): void { + for (const accountId of accountIds) { + this.#accountSnapIds.delete(accountId); + } + } + + /** + * Lazily builds the Snap-ownership cache on first use. + */ + #initAccountSnapCache(): 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/tsconfig.build.json b/packages/snap-account-service/tsconfig.build.json index 59c9b7a1df4..3e28b0866c7 100644 --- a/packages/snap-account-service/tsconfig.build.json +++ b/packages/snap-account-service/tsconfig.build.json @@ -6,6 +6,7 @@ "rootDir": "./src" }, "references": [ + { "path": "../accounts-controller/tsconfig.build.json" }, { "path": "../keyring-controller/tsconfig.build.json" }, { "path": "../messenger/tsconfig.build.json" } ], diff --git a/packages/snap-account-service/tsconfig.json b/packages/snap-account-service/tsconfig.json index a0556a87d10..6cb1c35cc2d 100644 --- a/packages/snap-account-service/tsconfig.json +++ b/packages/snap-account-service/tsconfig.json @@ -4,6 +4,7 @@ "baseUrl": "./" }, "references": [ + { "path": "../accounts-controller" }, { "path": "../keyring-controller" }, { "path": "../messenger" } ], diff --git a/yarn.lock b/yarn.lock index 22614dd8931..79480eb85dc 100644 --- a/yarn.lock +++ b/yarn.lock @@ -8881,6 +8881,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.1" "@metamask/auto-changelog": "npm:^6.1.0" "@metamask/eth-snap-keyring": "npm:^24.0.0" "@metamask/keyring-api": "npm:^24.0.0"