diff --git a/packages/eip-5792-middleware/CHANGELOG.md b/packages/eip-5792-middleware/CHANGELOG.md index 23b0d3c67c3..87452b2ce2d 100644 --- a/packages/eip-5792-middleware/CHANGELOG.md +++ b/packages/eip-5792-middleware/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Bump `@metamask/transaction-controller` from `^62.7.0` to `^62.13.0` ([#7596](https://github.com/MetaMask/core/pull/7596), [#7602](https://github.com/MetaMask/core/pull/7602), [#7604](https://github.com/MetaMask/core/pull/7604), [#7642](https://github.com/MetaMask/core/pull/7642), [#7737](https://github.com/MetaMask/core/pull/7737), [#7760](https://github.com/MetaMask/core/pull/7760), [#7775](https://github.com/MetaMask/core/pull/7775), [#7802](https://github.com/MetaMask/core/pull/7802)) - Bump `@metamask/utils` from `^11.8.1` to `^11.9.0` ([#7511](https://github.com/MetaMask/core/pull/7511)) +- Make `wallet_sendCalls` more reliable ([#7816](https://github.com/MetaMask/core/pull/7816)) ## [2.1.0] diff --git a/packages/eip-5792-middleware/src/hooks/processSendCalls.test.ts b/packages/eip-5792-middleware/src/hooks/processSendCalls.test.ts index 24f35b3eb41..df81221d9b4 100644 --- a/packages/eip-5792-middleware/src/hooks/processSendCalls.test.ts +++ b/packages/eip-5792-middleware/src/hooks/processSendCalls.test.ts @@ -12,6 +12,7 @@ import type { CustomNetworkClientConfiguration, NetworkControllerGetNetworkClientByIdAction, } from '@metamask/network-controller'; +import { providerErrors } from '@metamask/rpc-errors'; import type { TransactionController } from '@metamask/transaction-controller'; import type { Hex, JsonRpcRequest } from '@metamask/utils'; @@ -89,6 +90,9 @@ describe('EIP-5792', () => { const isAuxiliaryFundsSupportedMock: jest.Mock = jest.fn(); + const getPermittedAccountsForOriginMock: jest.MockedFn<() => Promise> = + jest.fn(); + let rootMessenger: RootMessenger; let messenger: Messenger<'EIP5792', AllActions, never, RootMessenger>; @@ -100,6 +104,7 @@ describe('EIP-5792', () => { getDismissSmartAccountSuggestionEnabledMock, isAtomicBatchSupported: isAtomicBatchSupportedMock, validateSecurity: validateSecurityMock, + getPermittedAccountsForOrigin: getPermittedAccountsForOriginMock, isAuxiliaryFundsSupported: isAuxiliaryFundsSupportedMock, }; @@ -115,11 +120,6 @@ describe('EIP-5792', () => { getNetworkClientByIdMock, ); - rootMessenger.registerActionHandler( - 'AccountsController:getSelectedAccount', - getSelectedAccountMock, - ); - rootMessenger.registerActionHandler( 'AccountsController:getState', getAccountsStateMock, @@ -134,7 +134,6 @@ describe('EIP-5792', () => { messenger, actions: [ 'AccountsController:getState', - 'AccountsController:getSelectedAccount', 'PreferencesController:getState', 'NetworkController:getNetworkClientById', 'NetworkController:getState', @@ -156,6 +155,8 @@ describe('EIP-5792', () => { isAuxiliaryFundsSupportedMock.mockReturnValue(true); + getPermittedAccountsForOriginMock.mockResolvedValue([FROM_MOCK] as Hex[]); + isAtomicBatchSupportedMock.mockResolvedValue([ { chainId: CHAIN_ID_MOCK, @@ -258,7 +259,7 @@ describe('EIP-5792', () => { expect(addTransactionBatchMock).toHaveBeenCalledTimes(1); }); - it('calls adds transaction batch hook with selected account if no from', async () => { + it('calls adds transaction batch hook with selected permitted account if no `from` param is provided', async () => { getSelectedAccountMock.mockReturnValue({ address: SEND_CALLS_MOCK.from, } as InternalAccount); @@ -467,6 +468,19 @@ describe('EIP-5792', () => { ); }); + it('throws if no `from` param is provided and no accounts are returned by `getPermittedAccountsForOrigin`', async () => { + getPermittedAccountsForOriginMock.mockResolvedValueOnce([]); + + await expect( + processSendCalls( + sendCallsHooks, + messenger, + { ...SEND_CALLS_MOCK, from: undefined }, + REQUEST_MOCK, + ), + ).rejects.toThrow(providerErrors.unauthorized()); + }); + it('validates auxiliary funds with unsupported account type', async () => { await expect( processSendCalls( diff --git a/packages/eip-5792-middleware/src/hooks/processSendCalls.ts b/packages/eip-5792-middleware/src/hooks/processSendCalls.ts index 0ff8f926550..e2474bf45d1 100644 --- a/packages/eip-5792-middleware/src/hooks/processSendCalls.ts +++ b/packages/eip-5792-middleware/src/hooks/processSendCalls.ts @@ -1,5 +1,5 @@ import type { KeyringTypes } from '@metamask/keyring-controller'; -import { JsonRpcError, rpcErrors } from '@metamask/rpc-errors'; +import { JsonRpcError, providerErrors, rpcErrors } from '@metamask/rpc-errors'; import type { BatchTransactionParams, IsAtomicBatchSupportedResultEntry, @@ -47,6 +47,7 @@ export type ProcessSendCallsHooks = { request: ValidateSecurityRequest, chainId: Hex, ) => Promise; + getPermittedAccountsForOrigin: () => Promise; /** Function to validate if auxiliary funds capability is supported. */ isAuxiliaryFundsSupported: (chainId: Hex) => boolean; }; @@ -82,6 +83,7 @@ export async function processSendCalls( getDismissSmartAccountSuggestionEnabled, isAtomicBatchSupported, validateSecurity: validateSecurityHook, + getPermittedAccountsForOrigin, isAuxiliaryFundsSupported, } = hooks; @@ -94,9 +96,13 @@ export async function processSendCalls( networkClientId, ).configuration; - const from = - paramFrom ?? - (messenger.call('AccountsController:getSelectedAccount').address as Hex); + // The first account returned by `getPermittedAccountsForOrigin` is the selected account for the origin + const [selectedAccount] = await getPermittedAccountsForOrigin(); + const from = paramFrom ?? selectedAccount; + + if (!from) { + throw providerErrors.unauthorized(); + } const securityAlertId = uuid(); const validateSecurity = validateSecurityHook.bind(null, securityAlertId); diff --git a/packages/eip-5792-middleware/src/methods/wallet_getCapabilities.test.ts b/packages/eip-5792-middleware/src/methods/wallet_getCapabilities.test.ts index aa70bdb90bf..cbee376fc1c 100644 --- a/packages/eip-5792-middleware/src/methods/wallet_getCapabilities.test.ts +++ b/packages/eip-5792-middleware/src/methods/wallet_getCapabilities.test.ts @@ -8,11 +8,12 @@ import type { GetCapabilitiesResult, } from '../types'; -type GetAccounts = (req: JsonRpcRequest) => Promise; +type GetPermittedAccountsForOrigin = () => Promise; const ADDRESS_MOCK = '0x123abc123abc123abc123abc123abc123abc123a'; const CHAIN_ID_MOCK = '0x1'; const CHAIN_ID_2_MOCK = '0x2'; +const ORIGIN_MOCK = 'https://example.com'; const RESULT_MOCK = { testCapability: { @@ -21,14 +22,15 @@ const RESULT_MOCK = { }; const REQUEST_MOCK = { + origin: ORIGIN_MOCK, params: [ADDRESS_MOCK], }; describe('wallet_getCapabilities', () => { - let request: JsonRpcRequest; + let request: JsonRpcRequest & { origin: string }; let params: GetCapabilitiesParams; let response: PendingJsonRpcResponse; - let getAccountsMock: jest.MockedFn; + let getPermittedAccountsForOriginMock: jest.MockedFn; let getCapabilitiesMock: jest.MockedFunction; /** @@ -37,7 +39,7 @@ describe('wallet_getCapabilities', () => { */ async function callMethod() { return walletGetCapabilities(request, response, { - getAccounts: getAccountsMock, + getPermittedAccountsForOrigin: getPermittedAccountsForOriginMock, getCapabilities: getCapabilitiesMock, }); } @@ -45,11 +47,13 @@ describe('wallet_getCapabilities', () => { beforeEach(() => { jest.resetAllMocks(); - request = klona(REQUEST_MOCK) as JsonRpcRequest; + request = klona(REQUEST_MOCK) as JsonRpcRequest & { origin: string }; params = request.params as GetCapabilitiesParams; response = {} as PendingJsonRpcResponse; - getAccountsMock = jest.fn().mockResolvedValue([ADDRESS_MOCK]); + getPermittedAccountsForOriginMock = jest + .fn() + .mockResolvedValue([ADDRESS_MOCK]); getCapabilitiesMock = jest.fn().mockResolvedValue(RESULT_MOCK); }); @@ -82,7 +86,7 @@ describe('wallet_getCapabilities', () => { it('throws if no hook', async () => { await expect( walletGetCapabilities(request, response, { - getAccounts: getAccountsMock, + getPermittedAccountsForOrigin: getPermittedAccountsForOriginMock, }), ).rejects.toMatchInlineSnapshot(`[Error: Method not supported.]`); }); @@ -128,7 +132,7 @@ describe('wallet_getCapabilities', () => { }); it('throws if from is not in accounts', async () => { - getAccountsMock.mockResolvedValueOnce([]); + getPermittedAccountsForOriginMock.mockResolvedValueOnce([]); await expect(callMethod()).rejects.toMatchInlineSnapshot( `[Error: The requested account and/or method has not been authorized by the user.]`, diff --git a/packages/eip-5792-middleware/src/methods/wallet_getCapabilities.ts b/packages/eip-5792-middleware/src/methods/wallet_getCapabilities.ts index e42ce6bd9af..de68a51e325 100644 --- a/packages/eip-5792-middleware/src/methods/wallet_getCapabilities.ts +++ b/packages/eip-5792-middleware/src/methods/wallet_getCapabilities.ts @@ -11,17 +11,17 @@ import { validateAndNormalizeKeyholder, validateParams } from '../utils'; * @param req - The JSON RPC request's end callback. * @param res - The JSON RPC request's pending response object. * @param hooks - The hooks object. - * @param hooks.getAccounts - Function that retrieves available accounts. + * @param hooks.getPermittedAccountsForOrigin - Function that retrieves permitted accounts for the requester's origin. * @param hooks.getCapabilities - Function that retrieves the capabilities for atomic transactions on specified chains. */ export async function walletGetCapabilities( - req: JsonRpcRequest, + req: JsonRpcRequest & { origin: string }, res: PendingJsonRpcResponse, { - getAccounts, + getPermittedAccountsForOrigin, getCapabilities, }: { - getAccounts: (req: JsonRpcRequest) => Promise; + getPermittedAccountsForOrigin: () => Promise; getCapabilities?: GetCapabilitiesHook; }, ): Promise { @@ -34,8 +34,8 @@ export async function walletGetCapabilities( const address = req.params[0]; const chainIds = req.params[1]; - await validateAndNormalizeKeyholder(address, req, { - getAccounts, + await validateAndNormalizeKeyholder(address, { + getPermittedAccountsForOrigin, }); const capabilities = await getCapabilities(address, chainIds, req); diff --git a/packages/eip-5792-middleware/src/methods/wallet_sendCalls.test.ts b/packages/eip-5792-middleware/src/methods/wallet_sendCalls.test.ts index 2f53ce10bec..eff576fa4f9 100644 --- a/packages/eip-5792-middleware/src/methods/wallet_sendCalls.test.ts +++ b/packages/eip-5792-middleware/src/methods/wallet_sendCalls.test.ts @@ -8,13 +8,15 @@ import type { SendCallsParams, } from '../types'; -type GetAccounts = (req: JsonRpcRequest) => Promise; +type GetPermittedAccountsForOrigin = () => Promise; const ADDRESS_MOCK = '0x123abc123abc123abc123abc123abc123abc123a'; const HEX_MOCK = '0x123abc'; const ID_MOCK = '0x12345678'; +const ORIGIN_MOCK = 'https://example.com'; const REQUEST_MOCK = { + origin: ORIGIN_MOCK, params: [ { version: '1.0', @@ -30,13 +32,13 @@ const REQUEST_MOCK = { ], }, ], -} as unknown as JsonRpcRequest; +} as unknown as JsonRpcRequest & { origin: string }; describe('wallet_sendCalls', () => { - let request: JsonRpcRequest; + let request: JsonRpcRequest & { origin: string }; let params: SendCallsParams; let response: PendingJsonRpcResponse; - let getAccountsMock: jest.MockedFn; + let getPermittedAccountsForOriginMock: jest.MockedFn; let processSendCallsMock: jest.MockedFunction; /** @@ -45,7 +47,7 @@ describe('wallet_sendCalls', () => { */ async function callMethod() { return walletSendCalls(request, response, { - getAccounts: getAccountsMock, + getPermittedAccountsForOrigin: getPermittedAccountsForOriginMock, processSendCalls: processSendCallsMock, }); } @@ -57,10 +59,10 @@ describe('wallet_sendCalls', () => { params = request.params as SendCallsParams; response = {} as PendingJsonRpcResponse; - getAccountsMock = jest.fn(); + getPermittedAccountsForOriginMock = jest.fn(); processSendCallsMock = jest.fn(); - getAccountsMock.mockResolvedValue([ADDRESS_MOCK]); + getPermittedAccountsForOriginMock.mockResolvedValue([ADDRESS_MOCK]); processSendCallsMock.mockResolvedValue({ id: ID_MOCK, @@ -108,7 +110,7 @@ describe('wallet_sendCalls', () => { it('throws if no hook', async () => { await expect( walletSendCalls(request, response, { - getAccounts: getAccountsMock, + getPermittedAccountsForOrigin: getPermittedAccountsForOriginMock, }), ).rejects.toMatchInlineSnapshot(`[Error: Method not supported.]`); }); @@ -207,7 +209,7 @@ describe('wallet_sendCalls', () => { }); it('throws if from is not in accounts', async () => { - getAccountsMock.mockResolvedValueOnce([]); + getPermittedAccountsForOriginMock.mockResolvedValueOnce([]); await expect(callMethod()).rejects.toMatchInlineSnapshot( `[Error: The requested account and/or method has not been authorized by the user.]`, diff --git a/packages/eip-5792-middleware/src/methods/wallet_sendCalls.ts b/packages/eip-5792-middleware/src/methods/wallet_sendCalls.ts index b51d20380c8..9c3d391c485 100644 --- a/packages/eip-5792-middleware/src/methods/wallet_sendCalls.ts +++ b/packages/eip-5792-middleware/src/methods/wallet_sendCalls.ts @@ -11,17 +11,17 @@ import { validateAndNormalizeKeyholder, validateParams } from '../utils'; * @param req - The JSON RPC request's end callback. * @param res - The JSON RPC request's pending response object. * @param hooks - The hooks object. - * @param hooks.getAccounts - Function that retrieves available accounts. + * @param hooks.getPermittedAccountsForOrigin - Function that retrieves permitted accounts for the requester's origin. * @param hooks.processSendCalls - Function that processes a sendCalls request for EIP-5792 transactions. */ export async function walletSendCalls( - req: JsonRpcRequest, + req: JsonRpcRequest & { origin: string }, res: PendingJsonRpcResponse, { - getAccounts, + getPermittedAccountsForOrigin, processSendCalls, }: { - getAccounts: (req: JsonRpcRequest) => Promise; + getPermittedAccountsForOrigin: () => Promise; processSendCalls?: ProcessSendCallsHook; }, ): Promise { @@ -34,8 +34,8 @@ export async function walletSendCalls( const params = req.params[0]; const from = params.from - ? await validateAndNormalizeKeyholder(params.from, req, { - getAccounts, + ? await validateAndNormalizeKeyholder(params.from, { + getPermittedAccountsForOrigin, }) : undefined; diff --git a/packages/eip-5792-middleware/src/utils.test.ts b/packages/eip-5792-middleware/src/utils.test.ts index d10a3e1fde5..f9fadc5fa59 100644 --- a/packages/eip-5792-middleware/src/utils.test.ts +++ b/packages/eip-5792-middleware/src/utils.test.ts @@ -2,7 +2,7 @@ import { KeyringTypes } from '@metamask/keyring-controller'; import { JsonRpcError, providerErrors } from '@metamask/rpc-errors'; import type { StructError } from '@metamask/superstruct'; import { any, validate } from '@metamask/superstruct'; -import type { Hex, JsonRpcRequest } from '@metamask/utils'; +import type { Hex } from '@metamask/utils'; import { EIP5792ErrorCode } from './constants'; import type { EIP5792Messenger } from './types'; @@ -306,60 +306,55 @@ describe('getAccountKeyringType', () => { describe('validateAndNormalizeKeyholder', () => { const ADDRESS_MOCK = '0xABCDabcdABCDabcdABCDabcdABCDabcdABCDabcd'; - const REQUEST_MOCK = {} as JsonRpcRequest; - let getAccountsMock: jest.MockedFn< - (req: JsonRpcRequest) => Promise - >; + let getPermittedAccountsForOriginMock: jest.MockedFn<() => Promise>; beforeEach(() => { jest.resetAllMocks(); - getAccountsMock = jest.fn().mockResolvedValue([ADDRESS_MOCK]); + getPermittedAccountsForOriginMock = jest + .fn() + .mockResolvedValue([ADDRESS_MOCK]); }); it('returns lowercase address', async () => { - const result = await validateAndNormalizeKeyholder( - ADDRESS_MOCK, - REQUEST_MOCK, - { - getAccounts: getAccountsMock, - }, - ); + const result = await validateAndNormalizeKeyholder(ADDRESS_MOCK, { + getPermittedAccountsForOrigin: getPermittedAccountsForOriginMock, + }); expect(result).toBe(ADDRESS_MOCK.toLowerCase()); }); - it('throws if address not returned by get accounts hook', async () => { - getAccountsMock.mockResolvedValueOnce([]); + it('throws if address not returned by get permitted accounts for origin hook', async () => { + getPermittedAccountsForOriginMock.mockResolvedValueOnce([]); await expect( - validateAndNormalizeKeyholder(ADDRESS_MOCK, REQUEST_MOCK, { - getAccounts: getAccountsMock, + validateAndNormalizeKeyholder(ADDRESS_MOCK, { + getPermittedAccountsForOrigin: getPermittedAccountsForOriginMock, }), ).rejects.toThrow(providerErrors.unauthorized()); }); it('throws if address is not string', async () => { await expect( - validateAndNormalizeKeyholder(123 as never, REQUEST_MOCK, { - getAccounts: getAccountsMock, + validateAndNormalizeKeyholder(123 as never, { + getPermittedAccountsForOrigin: getPermittedAccountsForOriginMock, }), ).rejects.toThrow('Invalid parameters: must provide an Ethereum address.'); }); it('throws if address is empty string', async () => { await expect( - validateAndNormalizeKeyholder('' as never, REQUEST_MOCK, { - getAccounts: getAccountsMock, + validateAndNormalizeKeyholder('' as never, { + getPermittedAccountsForOrigin: getPermittedAccountsForOriginMock, }), ).rejects.toThrow('Invalid parameters: must provide an Ethereum address.'); }); it('throws if address length is not 40', async () => { await expect( - validateAndNormalizeKeyholder('0x123', REQUEST_MOCK, { - getAccounts: getAccountsMock, + validateAndNormalizeKeyholder('0x123', { + getPermittedAccountsForOrigin: getPermittedAccountsForOriginMock, }), ).rejects.toThrow('Invalid parameters: must provide an Ethereum address.'); }); diff --git a/packages/eip-5792-middleware/src/utils.ts b/packages/eip-5792-middleware/src/utils.ts index 2bd16a46cfb..c888ceafde9 100644 --- a/packages/eip-5792-middleware/src/utils.ts +++ b/packages/eip-5792-middleware/src/utils.ts @@ -2,7 +2,7 @@ import type { KeyringTypes } from '@metamask/keyring-controller'; import { JsonRpcError, providerErrors, rpcErrors } from '@metamask/rpc-errors'; import type { Struct, StructError } from '@metamask/superstruct'; import { validate } from '@metamask/superstruct'; -import type { Hex, JsonRpcRequest } from '@metamask/utils'; +import type { Hex } from '@metamask/utils'; import { EIP5792ErrorCode } from './constants'; import type { EIP5792Messenger } from './types'; @@ -43,17 +43,17 @@ export function getAccountKeyringType( * Validates and normalizes a keyholder address for EIP-5792 operations. * * @param address - The Ethereum address to validate and normalize. - * @param req - The JSON-RPC request object for permission checking. - * @param options - Configuration object containing the getAccounts function. - * @param options.getAccounts - Function to retrieve accounts for the requester. + * @param options - Configuration object containing the getPermittedAccountsForOrigin function. + * @param options.getPermittedAccountsForOrigin - Function to retrieve permitted accounts for the requester's origin. * @returns A normalized (lowercase) hex address if valid and authorized. * @throws JsonRpcError with unauthorized error if the requester doesn't have permission to access the address. * @throws JsonRpcError with invalid params if the address format is invalid. */ export async function validateAndNormalizeKeyholder( address: Hex, - req: JsonRpcRequest, - { getAccounts }: { getAccounts: (req: JsonRpcRequest) => Promise }, + { + getPermittedAccountsForOrigin, + }: { getPermittedAccountsForOrigin: () => Promise }, ): Promise { if ( typeof address === 'string' && @@ -62,7 +62,7 @@ export async function validateAndNormalizeKeyholder( ) { // Ensure that an "unauthorized" error is thrown if the requester // does not have the `eth_accounts` permission. - const accounts = await getAccounts(req); + const accounts = await getPermittedAccountsForOrigin(); const normalizedAccounts: string[] = accounts.map((_address) => _address.toLowerCase(),