From e6ecd956b054a29481071e4eded2f8cd17d137d2 Mon Sep 17 00:00:00 2001 From: Alex Donesky Date: Thu, 30 May 2024 16:03:01 -0500 Subject: [PATCH] Fully remove `eth_sign` (#4319) ## Explanation Months ago, because of phishing risk, we disabled the `eth_sign` API method by default (users could manually enable it with a preference toggle). Now because of additional risk associated with [potentially malicious EIP-3074 invokers](https://ethereum-magicians.org/t/eip-3074-is-unsafe-unnecessary-puts-user-funds-at-risk-while-fragmenting-ux-liquidity-and-the-wallet-stack/19662) we are fully removing support for this method. ## References * Fixes https://github.com/MetaMask/MetaMask-planning/issues/2371 ## Changelog ### `@metamask/signature-controller` - **REMOVED**: Methods related to `eth_sign` signatures - `unapprovedMsgCount`, `messages`, `newUnsignedMessage` - have been removed. - **REMOVED**: constructor argument `isEthSignEnabled` is no longer expected - ### `@metamask/preferences-controller` - **REMOVED**: `disabledRpcMethodPreferences` removed from state - **REMOVED**: `setDisabledRpcMethodPreference` removed ## Checklist - [ ] I've updated the test suite for new or updated code as appropriate - [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've highlighted breaking changes using the "BREAKING" category above as appropriate --- packages/controller-utils/src/constants.ts | 1 - .../src/LoggingController.test.ts | 12 +- .../src/logTypes/EthSignLog.ts | 1 - .../src/AbstractMessageManager.ts | 2 +- .../src/MessageManager.test.ts | 156 ------------ .../message-manager/src/MessageManager.ts | 128 ---------- packages/message-manager/src/index.ts | 1 - packages/message-manager/src/utils.ts | 7 +- .../tests/provider-api-tests/shared-tests.ts | 2 - .../src/PreferencesController.test.ts | 9 - .../src/PreferencesController.ts | 27 -- .../src/SignatureController.test.ts | 238 ++++-------------- .../src/SignatureController.ts | 131 +--------- packages/transaction-controller/src/types.ts | 5 - .../src/utils/validation.test.ts | 2 +- 15 files changed, 57 insertions(+), 665 deletions(-) delete mode 100644 packages/message-manager/src/MessageManager.test.ts delete mode 100644 packages/message-manager/src/MessageManager.ts diff --git a/packages/controller-utils/src/constants.ts b/packages/controller-utils/src/constants.ts index 1c4ae8d499..7bc07e0700 100644 --- a/packages/controller-utils/src/constants.ts +++ b/packages/controller-utils/src/constants.ts @@ -127,7 +127,6 @@ export enum ApprovalType { ConnectAccounts = 'connect_accounts', EthDecrypt = 'eth_decrypt', EthGetEncryptionPublicKey = 'eth_getEncryptionPublicKey', - EthSign = 'eth_sign', EthSignTypedData = 'eth_signTypedData', PersonalSign = 'personal_sign', ResultError = 'result_error', diff --git a/packages/logging-controller/src/LoggingController.test.ts b/packages/logging-controller/src/LoggingController.test.ts index 54405b87b2..2799234e66 100644 --- a/packages/logging-controller/src/LoggingController.test.ts +++ b/packages/logging-controller/src/LoggingController.test.ts @@ -83,9 +83,9 @@ describe('LoggingController', () => { await unrestricted.call('LoggingController:add', { type: LogType.EthSignLog, data: { - signingMethod: SigningMethod.EthSign, + signingMethod: SigningMethod.PersonalSign, stage: SigningStage.Proposed, - signingData: '0x0000000000000', + signingData: 'hello', }, }), ).toBeUndefined(); @@ -97,9 +97,9 @@ describe('LoggingController', () => { log: expect.objectContaining({ type: LogType.EthSignLog, data: { - signingMethod: SigningMethod.EthSign, + signingMethod: SigningMethod.PersonalSign, stage: SigningStage.Proposed, - signingData: '0x0000000000000', + signingData: 'hello', }, }), }); @@ -167,9 +167,9 @@ describe('LoggingController', () => { await unrestricted.call('LoggingController:add', { type: LogType.EthSignLog, data: { - signingMethod: SigningMethod.EthSign, + signingMethod: SigningMethod.PersonalSign, stage: SigningStage.Proposed, - signingData: '0x0000000000000', + signingData: 'Heya', }, }), ).toBeUndefined(); diff --git a/packages/logging-controller/src/logTypes/EthSignLog.ts b/packages/logging-controller/src/logTypes/EthSignLog.ts index 9f4be36b51..4c90e49ce4 100644 --- a/packages/logging-controller/src/logTypes/EthSignLog.ts +++ b/packages/logging-controller/src/logTypes/EthSignLog.ts @@ -4,7 +4,6 @@ import type { LogType } from './LogType'; * An enum of the signing method types that we are interested in logging. */ export enum SigningMethod { - EthSign = 'eth_sign', PersonalSign = 'personal_sign', EthSignTypedData = 'eth_signTypedData', EthSignTypedDataV3 = 'eth_signTypedData_v3', diff --git a/packages/message-manager/src/AbstractMessageManager.ts b/packages/message-manager/src/AbstractMessageManager.ts index 849d9799f6..ee02662926 100644 --- a/packages/message-manager/src/AbstractMessageManager.ts +++ b/packages/message-manager/src/AbstractMessageManager.ts @@ -44,7 +44,7 @@ export interface AbstractMessage { } /** - * @type MessageParams + * @type AbstractMessageParams * * Represents the parameters to pass to the signing method once the signature request is approved. * @property from - Address from which the message is processed diff --git a/packages/message-manager/src/MessageManager.test.ts b/packages/message-manager/src/MessageManager.test.ts deleted file mode 100644 index abdc9075f8..0000000000 --- a/packages/message-manager/src/MessageManager.test.ts +++ /dev/null @@ -1,156 +0,0 @@ -import { MessageManager } from './MessageManager'; - -describe('MessageManager', () => { - let controller: MessageManager; - - const fromMock = '0xc38bf1ad06ef69f0c04e29dbeb4152b4175f0a8d'; - beforeEach(() => { - controller = new MessageManager(); - }); - - it('should set default state', () => { - expect(controller.state).toStrictEqual({ - unapprovedMessages: {}, - unapprovedMessagesCount: 0, - }); - }); - - it('should set default config', () => { - expect(controller.config).toStrictEqual({}); - }); - - it('should add a valid message', async () => { - const messageId = '1'; - const from = '0x0123'; - const messageData = '0x123'; - const messageTime = Date.now(); - const messageStatus = 'unapproved'; - const messageType = 'eth_sign'; - await controller.addMessage({ - id: messageId, - messageParams: { - data: messageData, - from, - }, - status: messageStatus, - time: messageTime, - type: messageType, - }); - const message = controller.getMessage(messageId); - if (!message) { - throw new Error('"message" is falsy'); - } - expect(message.id).toBe(messageId); - expect(message.messageParams.from).toBe(from); - expect(message.messageParams.data).toBe(messageData); - expect(message.time).toBe(messageTime); - expect(message.status).toBe(messageStatus); - expect(message.type).toBe(messageType); - }); - - it('should add a valid unapproved message', async () => { - const messageStatus = 'unapproved'; - const messageType = 'eth_sign'; - const messageParams = { - data: '0x123', - from: fromMock, - }; - const originalRequest = { - origin: 'origin', - securityAlertResponse: { result_type: 'result_type', reason: 'reason' }, - }; - const messageId = await controller.addUnapprovedMessage( - messageParams, - originalRequest, - ); - expect(messageId).toBeDefined(); - const message = controller.getMessage(messageId); - if (!message) { - throw new Error('"message" is falsy'); - } - expect(message.messageParams.from).toBe(messageParams.from); - expect(message.messageParams.data).toBe(messageParams.data); - expect(message.time).toBeDefined(); - expect(message.status).toBe(messageStatus); - expect(message.type).toBe(messageType); - expect(message.securityAlertResponse?.result_type).toBe('result_type'); - expect(message.securityAlertResponse?.reason).toBe('reason'); - }); - - it('should throw when adding invalid message', async () => { - const from = 'foo'; - const messageData = '0x123'; - await expect( - controller.addUnapprovedMessage({ - data: messageData, - from, - }), - ).rejects.toThrow( - `Invalid "from" address: ${from} must be a valid string.`, - ); - }); - - it('should get correct unapproved messages', async () => { - const firstMessage = { - id: '1', - messageParams: { from: '0x1', data: '0x123' }, - status: 'unapproved', - time: 123, - type: 'eth_sign', - }; - const secondMessage = { - id: '2', - messageParams: { from: '0x1', data: '0x321' }, - status: 'unapproved', - time: 123, - type: 'eth_sign', - }; - await controller.addMessage(firstMessage); - await controller.addMessage(secondMessage); - expect(controller.getUnapprovedMessagesCount()).toBe(2); - expect(controller.getUnapprovedMessages()).toStrictEqual({ - [firstMessage.id]: firstMessage, - [secondMessage.id]: secondMessage, - }); - }); - - it('should approve message', async () => { - const firstMessage = { from: fromMock, data: '0x123' }; - const messageId = await controller.addUnapprovedMessage(firstMessage); - const messageParams = await controller.approveMessage({ - ...firstMessage, - metamaskId: messageId, - }); - const message = controller.getMessage(messageId); - expect(messageParams).toStrictEqual(firstMessage); - if (!message) { - throw new Error('"message" is falsy'); - } - expect(message.status).toBe('approved'); - }); - - it('should set message status signed', async () => { - const firstMessage = { from: fromMock, data: '0x123' }; - const rawSig = '0x5f7a0'; - const messageId = await controller.addUnapprovedMessage(firstMessage); - - controller.setMessageStatusSigned(messageId, rawSig); - const message = controller.getMessage(messageId); - if (!message) { - throw new Error('"message" is falsy'); - } - expect(message.rawSig).toStrictEqual(rawSig); - expect(message.status).toBe('signed'); - }); - - it('should reject message', async () => { - const firstMessage = { from: fromMock, data: '0x123' }; - const messageId = await controller.addUnapprovedMessage(firstMessage); - controller.rejectMessage(messageId); - const message = controller.getMessage(messageId); - if (!message) { - throw new Error('"message" is falsy'); - } - expect(message.status).toBe('rejected'); - }); -}); diff --git a/packages/message-manager/src/MessageManager.ts b/packages/message-manager/src/MessageManager.ts deleted file mode 100644 index 8a561cbfe6..0000000000 --- a/packages/message-manager/src/MessageManager.ts +++ /dev/null @@ -1,128 +0,0 @@ -import { v1 as random } from 'uuid'; - -import type { - AbstractMessage, - AbstractMessageParams, - AbstractMessageParamsMetamask, - OriginalRequest, -} from './AbstractMessageManager'; -import { AbstractMessageManager } from './AbstractMessageManager'; -import { normalizeMessageData, validateSignMessageData } from './utils'; - -/** - * @type Message - * - * Represents and contains data about a 'eth_sign' type signature request. - * These are created when a signature for an eth_sign call is requested. - * @property id - An id to track and identify the message object - * @property messageParams - The parameters to pass to the eth_sign method once the signature request is approved - * @property type - The json-prc signing method for which a signature request has been made. - * A 'Message' which always has a 'eth_sign' type - * @property rawSig - Raw data of the signature request - */ -// This interface was created before this ESLint rule was added. -// Convert to a `type` in a future major version. -// eslint-disable-next-line @typescript-eslint/consistent-type-definitions -export interface Message extends AbstractMessage { - messageParams: MessageParams; -} - -/** - * @type PersonalMessageParams - * - * Represents the parameters to pass to the eth_sign method once the signature request is approved. - * @property data - A hex string conversion of the raw buffer data of the signature request - * @property from - Address to sign this message from - * @property origin? - Added for request origin identification - */ -// This interface was created before this ESLint rule was added. -// Convert to a `type` in a future major version. -// eslint-disable-next-line @typescript-eslint/consistent-type-definitions -export interface MessageParams extends AbstractMessageParams { - data: string; -} - -/** - * @type MessageParamsMetamask - * - * Represents the parameters to pass to the eth_sign method once the signature request is approved - * plus data added by MetaMask. - * @property metamaskId - Added for tracking and identification within MetaMask - * @property data - A hex string conversion of the raw buffer data of the signature request - * @property from - Address to sign this message from - * @property origin? - Added for request origin identification - */ -// This interface was created before this ESLint rule was added. -// Convert to a `type` in a future major version. -// eslint-disable-next-line @typescript-eslint/consistent-type-definitions -export interface MessageParamsMetamask extends AbstractMessageParamsMetamask { - data: string; -} - -/** - * Controller in charge of managing - storing, adding, removing, updating - Messages. - */ -export class MessageManager extends AbstractMessageManager< - Message, - MessageParams, - MessageParamsMetamask -> { - /** - * Name of this controller used during composition - */ - override name = 'MessageManager' as const; - - /** - * Creates a new Message with an 'unapproved' status using the passed messageParams. - * this.addMessage is called to add the new Message to this.messages, and to save the - * unapproved Messages. - * - * @param messageParams - The params for the eth_sign call to be made after the message - * is approved. - * @param req - The original request object possibly containing the origin. - * @returns The id of the newly created message. - */ - async addUnapprovedMessage( - messageParams: MessageParams, - req?: OriginalRequest, - ): Promise { - validateSignMessageData(messageParams); - if (req) { - messageParams.origin = req.origin; - } - messageParams.data = normalizeMessageData(messageParams.data); - const messageId = random(); - const messageData: Message = { - id: messageId, - messageParams, - securityAlertResponse: req?.securityAlertResponse, - status: 'unapproved', - time: Date.now(), - type: 'eth_sign', - }; - await this.addMessage(messageData); - this.hub.emit(`unapprovedMessage`, { - ...messageParams, - ...{ metamaskId: messageId }, - }); - return messageId; - } - - /** - * Removes the metamaskId property from passed messageParams and returns a promise which - * resolves the updated messageParams. - * - * @param messageParams - The messageParams to modify. - * @returns Promise resolving to the messageParams with the metamaskId property removed. - */ - prepMessageForSigning( - messageParams: MessageParamsMetamask, - ): Promise { - // Using delete operation will throw an error on frozen messageParams - const { metamaskId: _metamaskId, ...messageParamsWithoutId } = - messageParams; - return Promise.resolve(messageParamsWithoutId); - } -} - -export default MessageManager; diff --git a/packages/message-manager/src/index.ts b/packages/message-manager/src/index.ts index 71e07950c7..3467359316 100644 --- a/packages/message-manager/src/index.ts +++ b/packages/message-manager/src/index.ts @@ -1,5 +1,4 @@ export * from './AbstractMessageManager'; -export * from './MessageManager'; export * from './PersonalMessageManager'; export * from './TypedMessageManager'; export * from './EncryptionPublicKeyManager'; diff --git a/packages/message-manager/src/utils.ts b/packages/message-manager/src/utils.ts index 39b1b4bcf2..0900c3a087 100644 --- a/packages/message-manager/src/utils.ts +++ b/packages/message-manager/src/utils.ts @@ -9,7 +9,6 @@ import { validate } from 'jsonschema'; import type { DecryptMessageParams } from './DecryptMessageManager'; import type { EncryptionPublicKeyParams } from './EncryptionPublicKeyManager'; -import type { MessageParams } from './MessageManager'; import type { PersonalMessageParams } from './PersonalMessageManager'; import type { TypedMessageParams } from './TypedMessageManager'; @@ -48,14 +47,12 @@ export function normalizeMessageData(data: string) { } /** - * Validates a PersonalMessageParams and MessageParams objects for required properties and throws in + * Validates a PersonalMessageParams objects for required properties and throws in * the event of any validation error. * * @param messageData - PersonalMessageParams object to validate. */ -export function validateSignMessageData( - messageData: PersonalMessageParams | MessageParams, -) { +export function validateSignMessageData(messageData: PersonalMessageParams) { const { from, data } = messageData; validateAddress(from, 'from'); diff --git a/packages/network-controller/tests/provider-api-tests/shared-tests.ts b/packages/network-controller/tests/provider-api-tests/shared-tests.ts index 10e8d34ad6..473a0ff243 100644 --- a/packages/network-controller/tests/provider-api-tests/shared-tests.ts +++ b/packages/network-controller/tests/provider-api-tests/shared-tests.ts @@ -70,8 +70,6 @@ export function testsForProviderType(providerType: ProviderType) { { name: 'eth_sendRawTransaction', numberOfParameters: 1 }, { name: 'eth_sendTransaction', numberOfParameters: 1 }, - { name: 'eth_sign', numberOfParameters: 2 }, - { name: 'eth_createAccessList', numberOfParameters: 2 }, { name: 'eth_getLogs', numberOfParameters: 1 }, { name: 'eth_getProof', numberOfParameters: 3 }, diff --git a/packages/preferences-controller/src/PreferencesController.test.ts b/packages/preferences-controller/src/PreferencesController.test.ts index 16e3154319..28cb622a53 100644 --- a/packages/preferences-controller/src/PreferencesController.test.ts +++ b/packages/preferences-controller/src/PreferencesController.test.ts @@ -24,9 +24,6 @@ describe('PreferencesController', () => { useNftDetection: false, openSeaEnabled: false, securityAlertsEnabled: false, - disabledRpcMethodPreferences: { - eth_sign: false, - }, isMultiAccountBalancesEnabled: true, showTestNetworks: false, isIpfsGatewayEnabled: true, @@ -387,12 +384,6 @@ describe('PreferencesController', () => { expect(controller.state.securityAlertsEnabled).toBe(true); }); - it('should set disabledRpcMethodPreferences', () => { - const controller = setupPreferencesController(); - controller.setDisabledRpcMethodPreference('eth_sign', true); - expect(controller.state.disabledRpcMethodPreferences.eth_sign).toBe(true); - }); - it('should set isMultiAccountBalancesEnabled', () => { const controller = setupPreferencesController(); controller.setIsMultiAccountBalancesEnabled(true); diff --git a/packages/preferences-controller/src/PreferencesController.ts b/packages/preferences-controller/src/PreferencesController.ts index 151ebc604d..a59452e47d 100644 --- a/packages/preferences-controller/src/PreferencesController.ts +++ b/packages/preferences-controller/src/PreferencesController.ts @@ -48,12 +48,6 @@ export type EtherscanSupportedHexChainId = * Preferences controller state */ export type PreferencesState = { - /** - * A map of RPC method names to enabled state (true is enabled, false is disabled) - */ - disabledRpcMethodPreferences: { - [methodName: string]: boolean; - }; /** * Map of specific features to enable or disable */ @@ -119,7 +113,6 @@ export type PreferencesState = { }; const metadata = { - disabledRpcMethodPreferences: { persist: true, anonymous: true }, featureFlags: { persist: true, anonymous: true }, identities: { persist: true, anonymous: false }, ipfsGateway: { persist: true, anonymous: false }, @@ -170,9 +163,6 @@ export type PreferencesControllerMessenger = RestrictedControllerMessenger< */ export function getDefaultPreferencesState() { return { - disabledRpcMethodPreferences: { - eth_sign: false, - }, featureFlags: {}, identities: {}, ipfsGateway: 'https://ipfs.io/ipfs/', @@ -440,23 +430,6 @@ export class PreferencesController extends BaseController< }); } - /** - * A setter for the user preferences to enable/disable rpc methods. - * - * @param methodName - The RPC method name to change the setting of. - * @param isEnabled - true to enable the rpc method, false to disable it. - */ - setDisabledRpcMethodPreference(methodName: string, isEnabled: boolean) { - const { disabledRpcMethodPreferences } = this.state; - const newDisabledRpcMethods = { - ...disabledRpcMethodPreferences, - [methodName]: isEnabled, - }; - this.update((state) => { - state.disabledRpcMethodPreferences = newDisabledRpcMethods; - }); - } - /** * A setter for the user preferences to enable/disable fetch of multiple accounts balance. * diff --git a/packages/signature-controller/src/SignatureController.test.ts b/packages/signature-controller/src/SignatureController.test.ts index 0fb258b17e..bb43dcaac1 100644 --- a/packages/signature-controller/src/SignatureController.test.ts +++ b/packages/signature-controller/src/SignatureController.test.ts @@ -9,7 +9,6 @@ import type { OriginalRequest, } from '@metamask/message-manager'; import { - MessageManager, PersonalMessageManager, TypedMessageManager, } from '@metamask/message-manager'; @@ -22,7 +21,6 @@ import type { import { SignatureController } from './SignatureController'; jest.mock('@metamask/message-manager', () => ({ - MessageManager: jest.fn(), PersonalMessageManager: jest.fn(), TypedMessageManager: jest.fn(), })); @@ -132,16 +130,11 @@ const createMessageManagerMock = (prototype?: any): jest.Mocked => { describe('SignatureController', () => { let signatureController: SignatureController; - const messageManagerConstructorMock = MessageManager as jest.MockedClass< - typeof MessageManager - >; const personalMessageManagerConstructorMock = PersonalMessageManager as jest.MockedClass; const typedMessageManagerConstructorMock = TypedMessageManager as jest.MockedClass; - const messageManagerMock = createMessageManagerMock( - MessageManager.prototype, - ); + const personalMessageManagerMock = createMessageManagerMock( PersonalMessageManager.prototype, @@ -184,7 +177,6 @@ describe('SignatureController', () => { addUnapprovedMessageMock.mockResolvedValue(messageIdMock); approveMessageMock.mockResolvedValue(messageParamsWithoutIdMock); - messageManagerConstructorMock.mockReturnValue(messageManagerMock); personalMessageManagerConstructorMock.mockReturnValue( personalMessageManagerMock, ); @@ -205,13 +197,6 @@ describe('SignatureController', () => { } as SignatureControllerOptions); }); - describe('unapprovedMsgCount', () => { - it('returns value from message manager getter', () => { - messageManagerMock.getUnapprovedMessagesCount.mockReturnValueOnce(10); - expect(signatureController.unapprovedMsgCount).toBe(10); - }); - }); - describe('unapprovedPersonalMessagesCount', () => { it('returns value from personal message manager getter', () => { personalMessageManagerMock.getUnapprovedMessagesCount.mockReturnValueOnce( @@ -235,16 +220,12 @@ describe('SignatureController', () => { // eslint-disable-next-line @typescript-eslint/ban-ts-comment // @ts-ignore signatureController.update(() => ({ - // TODO: Replace `any` with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - unapprovedMsgs: { [messageIdMock]: messageMock } as any, // TODO: Replace `any` with type // eslint-disable-next-line @typescript-eslint/no-explicit-any unapprovedPersonalMsgs: { [messageIdMock]: messageMock } as any, // TODO: Replace `any` with type // eslint-disable-next-line @typescript-eslint/no-explicit-any unapprovedTypedMessages: { [messageIdMock]: messageMock } as any, - unapprovedMsgCount: 1, unapprovedPersonalMsgCount: 2, unapprovedTypedMessagesCount: 3, })); @@ -252,10 +233,8 @@ describe('SignatureController', () => { signatureController.resetState(); expect(signatureController.state).toStrictEqual({ - unapprovedMsgs: {}, unapprovedPersonalMsgs: {}, unapprovedTypedMessages: {}, - unapprovedMsgCount: 0, unapprovedPersonalMsgCount: 0, unapprovedTypedMessagesCount: 0, }); @@ -269,11 +248,6 @@ describe('SignatureController', () => { [messageIdMock2]: messageMock, }; - messageManagerMock.getUnapprovedMessages.mockReturnValueOnce( - // TODO: Replace `any` with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - messages as any, - ); personalMessageManagerMock.getUnapprovedMessages.mockReturnValueOnce( // TODO: Replace `any` with type // eslint-disable-next-line @typescript-eslint/no-explicit-any @@ -303,14 +277,6 @@ describe('SignatureController', () => { it('rejects all messages in all message managers', () => { signatureController.rejectUnapproved('Test Reason'); - expect(messageManagerMock.rejectMessage).toHaveBeenCalledTimes(2); - expect(messageManagerMock.rejectMessage).toHaveBeenCalledWith( - messageIdMock, - ); - expect(messageManagerMock.rejectMessage).toHaveBeenCalledWith( - messageIdMock2, - ); - expect(personalMessageManagerMock.rejectMessage).toHaveBeenCalledTimes(2); expect(personalMessageManagerMock.rejectMessage).toHaveBeenCalledWith( messageIdMock, @@ -334,7 +300,7 @@ describe('SignatureController', () => { signatureController.rejectUnapproved('Test Reason'); - expect(listenerMock).toHaveBeenCalledTimes(6); + expect(listenerMock).toHaveBeenCalledTimes(4); expect(listenerMock).toHaveBeenLastCalledWith({ reason: 'Test Reason', message: messageMock, @@ -351,9 +317,6 @@ describe('SignatureController', () => { unapprovedMessagesCount: 0, }; - expect(messageManagerMock.update).toHaveBeenCalledTimes(1); - expect(messageManagerMock.update).toHaveBeenCalledWith(defaultState); - expect(personalMessageManagerMock.update).toHaveBeenCalledTimes(1); expect(personalMessageManagerMock.update).toHaveBeenCalledWith( defaultState, @@ -364,109 +327,6 @@ describe('SignatureController', () => { }); }); - describe('newUnsignedMessage', () => { - it('throws if eth_sign disabled', async () => { - isEthSignEnabledMock.mockReturnValueOnce(false); - - await expect( - signatureController.newUnsignedMessage(messageParamsMock, requestMock), - ).rejects.toThrow( - 'eth_sign has been disabled. You must enable it in the advanced settings', - ); - }); - - it('throws if data has wrong length', async () => { - await expect( - signatureController.newUnsignedMessage( - { ...messageParamsMock, data: '0xFF' }, - requestMock, - ), - ).rejects.toThrow('eth_sign requires 32 byte message hash'); - }); - - it('throws if data has wrong length and is unicode', async () => { - await expect( - signatureController.newUnsignedMessage( - { ...messageParamsMock, data: '1234' }, - requestMock, - ), - ).rejects.toThrow('eth_sign requires 32 byte message hash'); - }); - - it('adds message to message manager', async () => { - // Satisfy one of fallback branches - const { origin: _origin, ...messageParamsWithoutOrigin } = - messageParamsMock; - - await signatureController.newUnsignedMessage( - messageParamsWithoutOrigin, - requestMock, - ); - - expect(messageManagerMock.addUnapprovedMessage).toHaveBeenCalledTimes(1); - expect(messageManagerMock.addUnapprovedMessage).toHaveBeenCalledWith( - messageParamsWithoutOrigin, - requestMock, - undefined, - ); - - expect(messengerMock.call).toHaveBeenCalledTimes(4); - expect(messengerMock.call).toHaveBeenNthCalledWith( - 2, - 'ApprovalController:addRequest', - { - id: messageIdMock, - origin: ORIGIN_METAMASK, - type: 'eth_sign', - requestData: messageParamsWithoutOrigin, - expectsResult: true, - }, - true, - ); - }); - - it('throws if cannot get signature', async () => { - mockMessengerAction('KeyringController:signMessage', async () => { - throw keyringErrorMock; - }); - const listenerMock = jest.fn(); - signatureController.hub.on(`${messageIdMock}:signError`, listenerMock); - - // TODO: Replace `any` with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const error: any = await getError( - async () => - await signatureController.newUnsignedMessage( - messageParamsMock, - requestMock, - ), - ); - - expect(listenerMock).toHaveBeenCalledTimes(1); - expect(listenerMock).toHaveBeenCalledWith({ - error, - }); - expect(messengerMock.call).toHaveBeenCalledTimes(3); - expect(error.message).toBe(keyringErrorMessageMock); - expect(messageManagerMock.rejectMessage).toHaveBeenCalledTimes(1); - expect(messageManagerMock.rejectMessage).toHaveBeenCalledWith( - messageIdMock, - ); - }); - - it('calls success callback once message is signed', async () => { - const { origin: _origin, ...messageParamsWithoutOrigin } = - messageParamsMock; - - await signatureController.newUnsignedMessage( - messageParamsWithoutOrigin, - requestMock, - ); - - expect(resultCallbacksMock.success).toHaveBeenCalledTimes(1); - }); - }); - describe('newUnsignedPersonalMessage', () => { it('adds message to personal message manager', async () => { await signatureController.newUnsignedPersonalMessage( @@ -562,6 +422,10 @@ describe('SignatureController', () => { describe('newUnsignedTypedMessage', () => { it('adds message to typed message manager', async () => { + const messageParamsWithOriginUndefined = { + ...messageParamsMock, + origin: undefined, + }; // eslint-disable-next-line @typescript-eslint/ban-ts-comment // @ts-ignore signatureController.update(() => ({ @@ -571,7 +435,7 @@ describe('SignatureController', () => { })); await signatureController.newUnsignedTypedMessage( - messageParamsMock, + messageParamsWithOriginUndefined, requestMock, versionMock, { parseJsonData: false }, @@ -581,7 +445,7 @@ describe('SignatureController', () => { typedMessageManagerMock.addUnapprovedMessage, ).toHaveBeenCalledTimes(1); expect(typedMessageManagerMock.addUnapprovedMessage).toHaveBeenCalledWith( - messageParamsMock, + messageParamsWithOriginUndefined, requestMock, versionMock, ); @@ -592,9 +456,9 @@ describe('SignatureController', () => { 'ApprovalController:addRequest', { id: messageIdMock, - origin: messageParamsMock.origin, + origin: ORIGIN_METAMASK, type: 'eth_signTypedData', - requestData: messageParamsMock, + requestData: messageParamsWithOriginUndefined, expectsResult: true, }, true, @@ -743,20 +607,20 @@ describe('SignatureController', () => { messageParamsMock.data, ); - expect(messageManagerMock.setMetadata).toHaveBeenCalledTimes(1); - expect(messageManagerMock.setMetadata).toHaveBeenCalledWith( + expect(personalMessageManagerMock.setMetadata).toHaveBeenCalledTimes(1); + expect(personalMessageManagerMock.setMetadata).toHaveBeenCalledWith( messageIdMock, messageParamsWithoutIdMock.data, ); - - expect(personalMessageManagerMock.setMetadata).not.toHaveBeenCalled(); expect(typedMessageManagerMock.setMetadata).not.toHaveBeenCalled(); }); it('should return false when an error occurs', () => { - jest.spyOn(messageManagerMock, 'setMetadata').mockImplementation(() => { - throw new Error('mocked error'); - }); + jest + .spyOn(personalMessageManagerMock, 'setMetadata') + .mockImplementation(() => { + throw new Error('mocked error'); + }); const result = signatureController.setMessageMetadata( messageParamsMock.metamaskId, @@ -764,8 +628,8 @@ describe('SignatureController', () => { ); expect(result).toBeUndefined(); - expect(messageManagerMock.setMetadata).toHaveBeenCalledTimes(1); - expect(messageManagerMock.setMetadata).toHaveBeenCalledWith( + expect(personalMessageManagerMock.setMetadata).toHaveBeenCalledTimes(1); + expect(personalMessageManagerMock.setMetadata).toHaveBeenCalledWith( messageIdMock, messageParamsWithoutIdMock.data, ); @@ -779,25 +643,20 @@ describe('SignatureController', () => { messageParamsMock.data, ); - expect(messageManagerMock.setMessageStatusSigned).toHaveBeenCalledTimes( - 1, - ); - expect(messageManagerMock.setMessageStatusSigned).toHaveBeenCalledWith( - messageIdMock, - messageParamsWithoutIdMock.data, - ); - expect( personalMessageManagerMock.setMessageStatusSigned, - ).not.toHaveBeenCalled(); + ).toHaveBeenCalledTimes(1); + expect( + personalMessageManagerMock.setMessageStatusSigned, + ).toHaveBeenCalledWith(messageIdMock, messageParamsWithoutIdMock.data); expect( typedMessageManagerMock.setMessageStatusSigned, ).not.toHaveBeenCalled(); }); - it('should return false when an error occurs', () => { + it('should return undefined when an error occurs', () => { jest - .spyOn(messageManagerMock, 'setMessageStatusSigned') + .spyOn(personalMessageManagerMock, 'setMessageStatusSigned') .mockImplementation(() => { throw new Error('mocked error'); }); @@ -808,13 +667,12 @@ describe('SignatureController', () => { ); expect(result).toBeUndefined(); - expect(messageManagerMock.setMessageStatusSigned).toHaveBeenCalledTimes( - 1, - ); - expect(messageManagerMock.setMessageStatusSigned).toHaveBeenCalledWith( - messageIdMock, - messageParamsWithoutIdMock.data, - ); + expect( + personalMessageManagerMock.setMessageStatusSigned, + ).toHaveBeenCalledTimes(1); + expect( + personalMessageManagerMock.setMessageStatusSigned, + ).toHaveBeenCalledWith(messageIdMock, messageParamsWithoutIdMock.data); }); }); @@ -822,19 +680,20 @@ describe('SignatureController', () => { it('rejects a message by calling rejectMessage', () => { signatureController.setDeferredSignError(messageParamsMock.metamaskId); - expect(messageManagerMock.rejectMessage).toHaveBeenCalledTimes(1); - expect(messageManagerMock.rejectMessage).toHaveBeenCalledWith( + expect(personalMessageManagerMock.rejectMessage).toHaveBeenCalledTimes(1); + expect(personalMessageManagerMock.rejectMessage).toHaveBeenCalledWith( messageIdMock, ); - expect(personalMessageManagerMock.rejectMessage).not.toHaveBeenCalled(); expect(typedMessageManagerMock.rejectMessage).not.toHaveBeenCalled(); }); it('rejects message on next message manager if first throws', () => { - jest.spyOn(messageManagerMock, 'rejectMessage').mockImplementation(() => { - throw new Error('mocked error'); - }); + jest + .spyOn(personalMessageManagerMock, 'rejectMessage') + .mockImplementation(() => { + throw new Error('mocked error'); + }); jest .spyOn(personalMessageManagerMock, 'rejectMessage') .mockImplementation(() => { @@ -847,9 +706,6 @@ describe('SignatureController', () => { }); it('should throw an error when tryForEachMessageManager fails', () => { - jest.spyOn(messageManagerMock, 'rejectMessage').mockImplementation(() => { - throw new Error('mocked error'); - }); jest .spyOn(personalMessageManagerMock, 'rejectMessage') .mockImplementation(() => { @@ -883,10 +739,9 @@ describe('SignatureController', () => { }, ]; - it('returns all the messages from typed, personal and messageManager', () => { + it('returns all the messages from TypedMessageManager and PersonalMessageManager', () => { typedMessageManagerMock.getAllMessages.mockReturnValueOnce(message); personalMessageManagerMock.getAllMessages.mockReturnValueOnce([]); - messageManagerMock.getAllMessages.mockReturnValueOnce([]); expect(signatureController.messages).toMatchObject({ '1': { id: '1', @@ -906,7 +761,6 @@ describe('SignatureController', () => { describe('message manager events', () => { it.each([ - ['message manager', messageManagerMock], ['personal message manager', personalMessageManagerMock], ['typed message manager', typedMessageManagerMock], ])('bubbles update badge event from %s', (_, messageManager) => { @@ -921,7 +775,7 @@ describe('SignatureController', () => { // eslint-disable-next-line jest/expect-expect it('does not throw if approval request promise throws', async () => { - const mockHub = messageManagerMock.hub.on as jest.Mock; + const mockHub = personalMessageManagerMock.hub.on as jest.Mock; messengerMock.call.mockRejectedValueOnce('Test Error'); @@ -929,7 +783,7 @@ describe('SignatureController', () => { }); it('updates state on message manager state change', async () => { - await messageManagerMock.subscribe.mock.calls[0][0]({ + await personalMessageManagerMock.subscribe.mock.calls[0][0]({ // TODO: Replace `any` with type // eslint-disable-next-line @typescript-eslint/no-explicit-any unapprovedMessages: { [messageIdMock]: coreMessageMock as any }, @@ -939,11 +793,9 @@ describe('SignatureController', () => { expect(await signatureController.state).toStrictEqual({ // TODO: Replace `any` with type // eslint-disable-next-line @typescript-eslint/no-explicit-any - unapprovedMsgs: { [messageIdMock]: stateMessageMock as any }, - unapprovedPersonalMsgs: {}, + unapprovedPersonalMsgs: { [messageIdMock]: stateMessageMock as any }, unapprovedTypedMessages: {}, - unapprovedMsgCount: 3, - unapprovedPersonalMsgCount: 0, + unapprovedPersonalMsgCount: 3, unapprovedTypedMessagesCount: 0, }); }); @@ -957,12 +809,10 @@ describe('SignatureController', () => { }); expect(await signatureController.state).toStrictEqual({ - unapprovedMsgs: {}, // TODO: Replace `any` with type // eslint-disable-next-line @typescript-eslint/no-explicit-any unapprovedPersonalMsgs: { [messageIdMock]: stateMessageMock as any }, unapprovedTypedMessages: {}, - unapprovedMsgCount: 0, unapprovedPersonalMsgCount: 4, unapprovedTypedMessagesCount: 0, }); @@ -977,12 +827,10 @@ describe('SignatureController', () => { }); expect(await signatureController.state).toStrictEqual({ - unapprovedMsgs: {}, unapprovedPersonalMsgs: {}, // TODO: Replace `any` with type // eslint-disable-next-line @typescript-eslint/no-explicit-any unapprovedTypedMessages: { [messageIdMock]: stateMessageMock as any }, - unapprovedMsgCount: 0, unapprovedPersonalMsgCount: 0, unapprovedTypedMessagesCount: 5, }); diff --git a/packages/signature-controller/src/SignatureController.ts b/packages/signature-controller/src/SignatureController.ts index ee73ec4024..acd8158079 100644 --- a/packages/signature-controller/src/SignatureController.ts +++ b/packages/signature-controller/src/SignatureController.ts @@ -23,8 +23,6 @@ import { } from '@metamask/logging-controller'; import type { AddLog } from '@metamask/logging-controller'; import type { - MessageParams, - MessageParamsMetamask, PersonalMessageParams, PersonalMessageParamsMetamask, TypedMessageParams, @@ -37,35 +35,28 @@ import type { OriginalRequest, TypedMessage, PersonalMessage, - Message, } from '@metamask/message-manager'; import { - MessageManager, PersonalMessageManager, TypedMessageManager, } from '@metamask/message-manager'; -import { providerErrors, rpcErrors } from '@metamask/rpc-errors'; +import { providerErrors } from '@metamask/rpc-errors'; import type { Hex, Json } from '@metamask/utils'; -import { bytesToHex } from '@metamask/utils'; import EventEmitter from 'events'; import { cloneDeep } from 'lodash'; const controllerName = 'SignatureController'; const stateMetadata = { - unapprovedMsgs: { persist: false, anonymous: false }, unapprovedPersonalMsgs: { persist: false, anonymous: false }, unapprovedTypedMessages: { persist: false, anonymous: false }, - unapprovedMsgCount: { persist: false, anonymous: false }, unapprovedPersonalMsgCount: { persist: false, anonymous: false }, unapprovedTypedMessagesCount: { persist: false, anonymous: false }, }; const getDefaultState = () => ({ - unapprovedMsgs: {}, unapprovedPersonalMsgs: {}, unapprovedTypedMessages: {}, - unapprovedMsgCount: 0, unapprovedPersonalMsgCount: 0, unapprovedTypedMessagesCount: 0, }); @@ -79,10 +70,8 @@ type StateMessage = Required & { }; type SignatureControllerState = { - unapprovedMsgs: Record; unapprovedPersonalMsgs: Record; unapprovedTypedMessages: Record; - unapprovedMsgCount: number; unapprovedPersonalMsgCount: number; unapprovedTypedMessagesCount: number; }; @@ -145,14 +134,10 @@ export class SignatureController extends BaseController< > { hub: EventEmitter; - #isEthSignEnabled: () => boolean; - // TODO: Replace `any` with type // eslint-disable-next-line @typescript-eslint/no-explicit-any #getAllState: () => any; - #messageManager: MessageManager; - #personalMessageManager: PersonalMessageManager; #typedMessageManager: TypedMessageManager; @@ -162,14 +147,12 @@ export class SignatureController extends BaseController< * * @param options - The controller options. * @param options.messenger - The restricted controller messenger for the sign controller. - * @param options.isEthSignEnabled - Callback to return true if eth_sign is enabled. * @param options.getAllState - Callback to retrieve all user state. * @param options.securityProviderRequest - A function for verifying a message, whether it is malicious or not. * @param options.getCurrentChainId - A function for retrieving the current chainId. */ constructor({ messenger, - isEthSignEnabled, getAllState, securityProviderRequest, getCurrentChainId, @@ -181,15 +164,9 @@ export class SignatureController extends BaseController< state: getDefaultState(), }); - this.#isEthSignEnabled = isEthSignEnabled; this.#getAllState = getAllState; this.hub = new EventEmitter(); - this.#messageManager = new MessageManager( - undefined, - undefined, - securityProviderRequest, - ); this.#personalMessageManager = new PersonalMessageManager( undefined, undefined, @@ -203,7 +180,6 @@ export class SignatureController extends BaseController< getCurrentChainId, ); - this.#handleMessageManagerEvents(this.#messageManager, 'unapprovedMessage'); this.#handleMessageManagerEvents( this.#personalMessageManager, 'unapprovedPersonalMessage', @@ -213,14 +189,6 @@ export class SignatureController extends BaseController< 'unapprovedTypedMessage', ); - this.#subscribeToMessageState( - this.#messageManager, - (state, newMessages, messageCount) => { - state.unapprovedMsgs = newMessages; - state.unapprovedMsgCount = messageCount; - }, - ); - this.#subscribeToMessageState( this.#personalMessageManager, (state, newMessages, messageCount) => { @@ -238,15 +206,6 @@ export class SignatureController extends BaseController< ); } - /** - * A getter for the number of 'unapproved' Messages in this.messages. - * - * @returns The number of 'unapproved' Messages in this.messages - */ - get unapprovedMsgCount(): number { - return this.#messageManager.getUnapprovedMessagesCount(); - } - /** * A getter for the number of 'unapproved' PersonalMessages in this.messages. * @@ -270,15 +229,14 @@ export class SignatureController extends BaseController< * * @returns The object containing all messages. */ - get messages(): { [id: string]: Message | PersonalMessage | TypedMessage } { + get messages(): { [id: string]: PersonalMessage | TypedMessage } { const messages = [ ...this.#typedMessageManager.getAllMessages(), ...this.#personalMessageManager.getAllMessages(), - ...this.#messageManager.getAllMessages(), ]; const messagesObject = messages.reduce<{ - [id: string]: Message | PersonalMessage | TypedMessage; + [id: string]: PersonalMessage | TypedMessage; }>((acc, message) => { acc[message.id] = message; return acc; @@ -300,7 +258,6 @@ export class SignatureController extends BaseController< * @param reason - A message to indicate why. */ rejectUnapproved(reason?: string) { - this.#rejectUnapproved(this.#messageManager, reason); this.#rejectUnapproved(this.#personalMessageManager, reason); this.#rejectUnapproved(this.#typedMessageManager, reason); } @@ -309,43 +266,14 @@ export class SignatureController extends BaseController< * Clears all unapproved messages from memory. */ clearUnapproved() { - this.#clearUnapproved(this.#messageManager); this.#clearUnapproved(this.#personalMessageManager); this.#clearUnapproved(this.#typedMessageManager); } - /** - * Called when a Dapp uses the eth_sign method, to request user approval. - * eth_sign is a pure signature of arbitrary data. It is on a deprecation - * path, since this data can be a transaction, or can leak private key - * information. - * - * @param messageParams - The params passed to eth_sign. - * @param [req] - The original request, containing the origin. - * @returns Promise resolving to the raw data of the signature request. - */ - async newUnsignedMessage( - messageParams: MessageParams, - req: OriginalRequest, - ): Promise { - return this.#newUnsignedAbstractMessage( - this.#messageManager, - ApprovalType.EthSign, - SigningMethod.EthSign, - 'Message', - this.#signMessage.bind(this), - messageParams, - req, - this.#validateUnsignedMessage.bind(this), - ); - } - /** * Called when a dapp uses the personal_sign method. - * This is identical to the Geth eth_sign method, and may eventually replace - * eth_sign. * - * We currently define our eth_sign and personal_sign mostly for legacy Dapps. + * We currently define personal_sign mostly for legacy Dapps. * * @param messageParams - The params of the message to sign & return to the Dapp. * @param req - The original request, containing the origin. @@ -391,7 +319,6 @@ export class SignatureController extends BaseController< this.#signTypedMessage.bind(this), messageParams, req, - undefined, version, signingOpts, ); @@ -444,21 +371,6 @@ export class SignatureController extends BaseController< this.#personalMessageManager.setMessageStatusInProgress(messageId); } - #validateUnsignedMessage(messageParams: MessageParamsMetamask): void { - if (!this.#isEthSignEnabled()) { - throw rpcErrors.methodNotFound( - 'eth_sign has been disabled. You must enable it in the advanced settings', - ); - } - const data = this.#normalizeMsgData(messageParams.data); - // 64 hex + "0x" at the beginning - // This is needed because Ethereum's EcSign works only on 32 byte numbers - // For 67 length see: https://github.com/MetaMask/metamask-extension/pull/12679/files#r749479607 - if (data.length !== 66 && data.length !== 67) { - throw rpcErrors.invalidParams('eth_sign requires 32 byte message hash'); - } - } - async #newUnsignedAbstractMessage< M extends AbstractMessage, P extends AbstractMessageParams, @@ -472,14 +384,9 @@ export class SignatureController extends BaseController< signMessage: (messageParams: PM, signingOpts?: SO) => void, messageParams: PM, req: OriginalRequest, - validateMessage?: (params: PM) => void, version?: string, signingOpts?: SO, ) { - if (validateMessage) { - validateMessage(messageParams); - } - let resultCallbacks: AcceptResultCallbacks | undefined; try { const messageId = await messageManager.addUnapprovedMessage( @@ -542,25 +449,6 @@ export class SignatureController extends BaseController< } } - /** - * Signifies user intent to complete an eth_sign method. - * - * @param msgParams - The params passed to eth_call. - * @returns Signature result from signing. - */ - async #signMessage(msgParams: MessageParamsMetamask) { - return await this.#signAbstractMessage( - this.#messageManager, - ApprovalType.EthSign, - msgParams, - async (cleanMsgParams) => - await this.messagingSystem.call( - 'KeyringController:signMessage', - cleanMsgParams, - ), - ); - } - /** * Signifies a user's approval to sign a personal_sign message in queue. * Triggers signing, and the callback function from newUnsignedPersonalMessage. @@ -630,7 +518,6 @@ export class SignatureController extends BaseController< ...args: any ) { const messageManagers = [ - this.#messageManager, this.#personalMessageManager, this.#typedMessageManager, ]; @@ -855,18 +742,8 @@ export class SignatureController extends BaseController< return stateMessage as StateMessage; } - #normalizeMsgData(data: string) { - if (data.startsWith('0x')) { - // data is already hex - return data; - } - // data is unicode, convert to hex - return bytesToHex(Buffer.from(data, 'utf8')); - } - #getMessage(messageId: string): StateMessage { return { - ...this.state.unapprovedMsgs, ...this.state.unapprovedPersonalMsgs, ...this.state.unapprovedTypedMessages, }[messageId]; diff --git a/packages/transaction-controller/src/types.ts b/packages/transaction-controller/src/types.ts index 776e5ae559..0a24e53ab6 100644 --- a/packages/transaction-controller/src/types.ts +++ b/packages/transaction-controller/src/types.ts @@ -528,11 +528,6 @@ export enum TransactionType { */ simpleSend = 'simpleSend', - /** - * A transaction that is signing a message. - */ - sign = 'eth_sign', - /** * A transaction that is signing typed data. */ diff --git a/packages/user-operation-controller/src/utils/validation.test.ts b/packages/user-operation-controller/src/utils/validation.test.ts index fd78926518..54fcee6dbc 100644 --- a/packages/user-operation-controller/src/utils/validation.test.ts +++ b/packages/user-operation-controller/src/utils/validation.test.ts @@ -337,7 +337,7 @@ describe('validation', () => { 'type', 'wrong type', 123, - 'Expected one of `"cancel","contractInteraction","contractDeployment","eth_decrypt","eth_getEncryptionPublicKey","incoming","personal_sign","retry","simpleSend","eth_sign","eth_signTypedData","smart","swap","swapAndSend","swapApproval","approve","safetransferfrom","transfer","transferfrom","setapprovalforall","increaseAllowance"`, but received: 123', + 'Expected one of `"cancel","contractInteraction","contractDeployment","eth_decrypt","eth_getEncryptionPublicKey","incoming","personal_sign","retry","simpleSend","eth_signTypedData","smart","swap","swapAndSend","swapApproval","approve","safetransferfrom","transfer","transferfrom","setapprovalforall","increaseAllowance"`, but received: 123', ], ])( 'throws if %s is %s',