From 92bdf72c272ec234ce36fbc5d31bd46918e7a25b Mon Sep 17 00:00:00 2001 From: Frederik Bolding Date: Thu, 10 Oct 2024 10:05:32 +0200 Subject: [PATCH] fix: Ignore Snap insight response if transaction/signature has already been signed (#2825) Ignore Snap insight responses when a transaction/signature has already been signed/rejected. Otherwise, we are trying to set state that no longer exists. Fixes https://github.com/MetaMask/metamask-extension/issues/27738 --- packages/snaps-controllers/coverage.json | 2 +- .../insights/SnapInsightsController.test.ts | 199 ++++++++++++++++++ .../src/insights/SnapInsightsController.ts | 6 + 3 files changed, 206 insertions(+), 1 deletion(-) diff --git a/packages/snaps-controllers/coverage.json b/packages/snaps-controllers/coverage.json index ed8c826fc4..cb15fa2386 100644 --- a/packages/snaps-controllers/coverage.json +++ b/packages/snaps-controllers/coverage.json @@ -1,5 +1,5 @@ { - "branches": 92.62, + "branches": 92.63, "functions": 96.65, "lines": 97.97, "statements": 97.67 diff --git a/packages/snaps-controllers/src/insights/SnapInsightsController.test.ts b/packages/snaps-controllers/src/insights/SnapInsightsController.test.ts index 318921bff5..f377a305c7 100644 --- a/packages/snaps-controllers/src/insights/SnapInsightsController.test.ts +++ b/packages/snaps-controllers/src/insights/SnapInsightsController.test.ts @@ -5,6 +5,7 @@ import { MOCK_LOCAL_SNAP_ID, MOCK_SNAP_ID, } from '@metamask/snaps-utils/test-utils'; +import { createDeferredPromise } from '@metamask/utils'; import { nanoid } from 'nanoid'; import { @@ -450,4 +451,202 @@ describe('SnapInsightsController', () => { expect(rootMessenger.call).toHaveBeenCalledTimes(8); }); + + it('ignores insight if transaction has already been signed', async () => { + const rootMessenger = getRootSnapInsightsControllerMessenger(); + + rootMessenger.registerActionHandler('SnapController:getAll', () => { + return [getTruncatedSnap(), getTruncatedSnap({ id: MOCK_LOCAL_SNAP_ID })]; + }); + + const { resolve, promise } = createDeferredPromise(); + + rootMessenger.registerActionHandler( + 'SnapController:handleRequest', + async ({ snapId }) => { + await promise; + if (snapId === MOCK_SNAP_ID) { + return { id: nanoid() }; + } + throw new InternalError(); + }, + ); + + rootMessenger.registerActionHandler( + 'PermissionController:getPermissions', + (origin) => { + return origin === MOCK_SNAP_ID + ? MOCK_INSIGHTS_PERMISSIONS + : MOCK_INSIGHTS_PERMISSIONS_NO_ORIGINS; + }, + ); + + const controllerMessenger = + getRestrictedSnapInsightsControllerMessenger(rootMessenger); + + const controller = new SnapInsightsController({ + messenger: controllerMessenger, + }); + + // Simulate transaction added, signed & confirmed + rootMessenger.publish( + 'TransactionController:unapprovedTransactionAdded', + TRANSACTION_META_MOCK, + ); + + rootMessenger.publish('TransactionController:transactionStatusUpdated', { + transactionMeta: { ...TRANSACTION_META_MOCK, status: 'approved' }, + }); + + rootMessenger.publish('TransactionController:transactionStatusUpdated', { + transactionMeta: { ...TRANSACTION_META_MOCK, status: 'confirmed' }, + }); + + resolve(); + + // Wait for promises to resolve + await new Promise(process.nextTick); + + expect(Object.values(controller.state.insights)).toHaveLength(0); + + expect(rootMessenger.call).toHaveBeenCalledTimes(5); + expect(rootMessenger.call).toHaveBeenNthCalledWith( + 4, + 'SnapController:handleRequest', + { + snapId: MOCK_SNAP_ID, + origin: '', + handler: HandlerType.OnTransaction, + request: { + method: '', + params: { + chainId: `eip155:${parseInt(TRANSACTION_META_MOCK.chainId, 16)}`, + transaction: TRANSACTION_META_MOCK.txParams, + transactionOrigin: TRANSACTION_META_MOCK.origin, + }, + }, + }, + ); + expect(rootMessenger.call).toHaveBeenNthCalledWith( + 5, + 'SnapController:handleRequest', + { + snapId: MOCK_LOCAL_SNAP_ID, + origin: '', + handler: HandlerType.OnTransaction, + request: { + method: '', + params: { + chainId: `eip155:${parseInt(TRANSACTION_META_MOCK.chainId, 16)}`, + transaction: TRANSACTION_META_MOCK.txParams, + transactionOrigin: null, + }, + }, + }, + ); + }); + + it('ignores insight if signature has already been signed', async () => { + const rootMessenger = getRootSnapInsightsControllerMessenger(); + const controllerMessenger = + getRestrictedSnapInsightsControllerMessenger(rootMessenger); + + const controller = new SnapInsightsController({ + messenger: controllerMessenger, + }); + + rootMessenger.registerActionHandler('SnapController:getAll', () => { + return [getTruncatedSnap(), getTruncatedSnap({ id: MOCK_LOCAL_SNAP_ID })]; + }); + + const { resolve, promise } = createDeferredPromise(); + + rootMessenger.registerActionHandler( + 'SnapController:handleRequest', + async () => { + await promise; + return { id: nanoid() }; + }, + ); + + rootMessenger.registerActionHandler( + 'PermissionController:getPermissions', + (origin) => { + return origin === MOCK_SNAP_ID + ? MOCK_INSIGHTS_PERMISSIONS + : MOCK_INSIGHTS_PERMISSIONS_NO_ORIGINS; + }, + ); + + rootMessenger.publish( + 'SignatureController:stateChange', + { + unapprovedPersonalMsgCount: 0, + unapprovedTypedMessagesCount: 1, + unapprovedTypedMessages: { '1': TYPED_SIGNATURE_MOCK }, + unapprovedPersonalMsgs: {}, + }, + [], + ); + + rootMessenger.publish( + 'SignatureController:stateChange', + { + unapprovedPersonalMsgCount: 0, + unapprovedTypedMessagesCount: 0, + unapprovedTypedMessages: {}, + unapprovedPersonalMsgs: {}, + }, + [], + ); + + resolve(); + + // Wait for promises to resolve + await new Promise(process.nextTick); + + expect(Object.values(controller.state.insights)).toHaveLength(0); + + expect(rootMessenger.call).toHaveBeenCalledTimes(5); + expect(rootMessenger.call).toHaveBeenNthCalledWith( + 4, + 'SnapController:handleRequest', + { + snapId: MOCK_SNAP_ID, + origin: '', + handler: HandlerType.OnSignature, + request: { + method: '', + params: { + signature: { + from: TYPED_SIGNATURE_MOCK.msgParams.from, + data: JSON.parse(TYPED_SIGNATURE_MOCK.msgParams.data), + signatureMethod: TYPED_SIGNATURE_MOCK.msgParams.signatureMethod, + }, + signatureOrigin: TYPED_SIGNATURE_MOCK.msgParams.origin, + }, + }, + }, + ); + expect(rootMessenger.call).toHaveBeenNthCalledWith( + 5, + 'SnapController:handleRequest', + { + snapId: MOCK_LOCAL_SNAP_ID, + origin: '', + handler: HandlerType.OnSignature, + request: { + method: '', + params: { + signature: { + from: TYPED_SIGNATURE_MOCK.msgParams.from, + data: JSON.parse(TYPED_SIGNATURE_MOCK.msgParams.data), + signatureMethod: TYPED_SIGNATURE_MOCK.msgParams.signatureMethod, + }, + signatureOrigin: null, + }, + }, + }, + ); + }); }); diff --git a/packages/snaps-controllers/src/insights/SnapInsightsController.ts b/packages/snaps-controllers/src/insights/SnapInsightsController.ts index f69839a28f..0354c95047 100644 --- a/packages/snaps-controllers/src/insights/SnapInsightsController.ts +++ b/packages/snaps-controllers/src/insights/SnapInsightsController.ts @@ -390,6 +390,12 @@ export class SnapInsightsController extends BaseController< response?: Record; error?: Error; }) { + // If the insight has been cleaned up already, we can skip setting the state. + // This may happen if a user accepts/rejects a transaction/signature faster than the Snap responds. + if (!this.#hasInsight(id)) { + return; + } + this.update((state) => { state.insights[id][snapId].loading = false; state.insights[id][snapId].interfaceId = response?.id as string;