From 58ed6af3a4798a07f017f9b2c820510a56b740a5 Mon Sep 17 00:00:00 2001 From: Matthew Walsh Date: Fri, 14 Apr 2023 18:33:53 +0100 Subject: [PATCH] Disable rate limiting for signature approval requests (#18594) --- app/scripts/controllers/sign.test.ts | 16 ++++++++++++++++ app/scripts/controllers/sign.ts | 27 ++++++++++++++++++--------- app/scripts/metamask-controller.js | 7 ++++++- package.json | 2 +- yarn.lock | 14 +++++++------- 5 files changed, 48 insertions(+), 18 deletions(-) diff --git a/app/scripts/controllers/sign.test.ts b/app/scripts/controllers/sign.test.ts index 36dddf65b..dac749466 100644 --- a/app/scripts/controllers/sign.test.ts +++ b/app/scripts/controllers/sign.test.ts @@ -409,6 +409,14 @@ describe('SignController', () => { ); }); + it('does not throw if accepting approval throws', async () => { + messengerMock.call.mockImplementation(() => { + throw new Error('Test Error'); + }); + + await signController[signMethodName](messageParamsMock); + }); + it('rejects message on error', async () => { keyringControllerMock[signMethodName].mockReset(); keyringControllerMock[signMethodName].mockRejectedValue( @@ -468,6 +476,14 @@ describe('SignController', () => { 'Cancel', ); }); + + it('does not throw if rejecting approval throws', async () => { + messengerMock.call.mockImplementation(() => { + throw new Error('Test Error'); + }); + + await signController[cancelMethodName](messageParamsMock); + }); }); describe('message manager events', () => { diff --git a/app/scripts/controllers/sign.ts b/app/scripts/controllers/sign.ts index e04d70c09..6f4b689e4 100644 --- a/app/scripts/controllers/sign.ts +++ b/app/scripts/controllers/sign.ts @@ -33,12 +33,13 @@ import { RejectRequest, } from '@metamask/approval-controller'; import { MetaMetricsEventCategory } from '../../../shared/constants/metametrics'; +import { MESSAGE_TYPE } from '../../../shared/constants/app'; import PreferencesController from './preferences'; const controllerName = 'SignController'; -const methodNameSign = 'eth_sign'; -const methodNamePersonalSign = 'personal_sign'; -const methodNameTypedSign = 'eth_signTypedData'; +const methodNameSign = MESSAGE_TYPE.ETH_SIGN; +const methodNamePersonalSign = MESSAGE_TYPE.PERSONAL_SIGN; +const methodNameTypedSign = MESSAGE_TYPE.ETH_SIGN_TYPED_DATA; const stateMetadata = { unapprovedMsgs: { persist: false, anonymous: false }, @@ -637,14 +638,22 @@ export default class SignController extends BaseControllerV2< } private _acceptApproval(messageId: string) { - this.messagingSystem.call('ApprovalController:acceptRequest', messageId); + try { + this.messagingSystem.call('ApprovalController:acceptRequest', messageId); + } catch (error) { + log.info('Failed to accept signature approval request', error); + } } private _rejectApproval(messageId: string) { - this.messagingSystem.call( - 'ApprovalController:rejectRequest', - messageId, - 'Cancel', - ); + try { + this.messagingSystem.call( + 'ApprovalController:rejectRequest', + messageId, + 'Cancel', + ); + } catch (error) { + log.info('Failed to reject signature approval request', error); + } } } diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 4ef07b80c..c995716bd 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -97,8 +97,8 @@ import { import { MILLISECOND, SECOND } from '../../shared/constants/time'; import { ORIGIN_METAMASK, - ///: BEGIN:ONLY_INCLUDE_IN(flask) MESSAGE_TYPE, + ///: BEGIN:ONLY_INCLUDE_IN(flask) SNAP_DIALOG_TYPES, ///: END:ONLY_INCLUDE_IN POLLING_TOKEN_ENVIRONMENT_TYPES, @@ -259,6 +259,11 @@ export default class MetamaskController extends EventEmitter { name: 'ApprovalController', }), showApprovalRequest: opts.showUserConfirmation, + typesExcludedFromRateLimiting: [ + MESSAGE_TYPE.ETH_SIGN, + MESSAGE_TYPE.PERSONAL_SIGN, + MESSAGE_TYPE.ETH_SIGN_TYPED_DATA, + ], }); const networkControllerMessenger = this.controllerMessenger.getRestricted({ diff --git a/package.json b/package.json index aa0a71c76..ded8880be 100644 --- a/package.json +++ b/package.json @@ -227,7 +227,7 @@ "@material-ui/core": "^4.11.0", "@metamask/address-book-controller": "^1.0.0", "@metamask/announcement-controller": "^1.0.0", - "@metamask/approval-controller": "^1.0.0", + "@metamask/approval-controller": "^2.1.0", "@metamask/assets-controllers": "^4.0.1", "@metamask/base-controller": "^1.0.0", "@metamask/contract-metadata": "^2.3.1", diff --git a/yarn.lock b/yarn.lock index 31bf1eb1b..c1ad0ad48 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3595,7 +3595,7 @@ __metadata: languageName: node linkType: hard -"@metamask/approval-controller@npm:^1.0.0, @metamask/approval-controller@npm:^1.0.1": +"@metamask/approval-controller@npm:^1.0.1": version: 1.1.0 resolution: "@metamask/approval-controller@npm:1.1.0" dependencies: @@ -3608,16 +3608,16 @@ __metadata: languageName: node linkType: hard -"@metamask/approval-controller@npm:^2.0.0": - version: 2.0.0 - resolution: "@metamask/approval-controller@npm:2.0.0" +"@metamask/approval-controller@npm:^2.0.0, @metamask/approval-controller@npm:^2.1.0": + version: 2.1.0 + resolution: "@metamask/approval-controller@npm:2.1.0" dependencies: "@metamask/base-controller": ^2.0.0 - "@metamask/controller-utils": ^3.0.0 + "@metamask/controller-utils": ^3.1.0 eth-rpc-errors: ^4.0.0 immer: ^9.0.6 nanoid: ^3.1.31 - checksum: 1db5f9c21b04fa4688c17cdfb7da0a14b3fee084fbd8c0cfdcc41572e54140ce093c24b811b85e8ee9d3ccd8987db04d9150d7c6d5ab21daf72b4364a05f3428 + checksum: 207380e3ed0007aec3b9efcde62ac3ece9fa46cc7b9e6157c0d54271e0936b5a9a05e59adcfb1e47e3f3df397d2d2dc757f3b97745528695182e7c66a5207aca languageName: node linkType: hard @@ -24278,7 +24278,7 @@ __metadata: "@material-ui/core": ^4.11.0 "@metamask/address-book-controller": ^1.0.0 "@metamask/announcement-controller": ^1.0.0 - "@metamask/approval-controller": ^1.0.0 + "@metamask/approval-controller": ^2.1.0 "@metamask/assets-controllers": ^4.0.1 "@metamask/auto-changelog": ^2.1.0 "@metamask/base-controller": ^1.0.0