fix: reject unsupported account methods (#190)

* Added check if method is supported by account to submitRequest in SnapKeyring * Removed .idea folder * Removed .idea folder * Added jest test for removing account supported method and method requests rejected * Updated account support methods in accounts mocks for tests
MetaMask · Dec 18, 2023 · 7ef8c33 · 7ef8c33
1 parent 5ca5546
commit 7ef8c33
Show file tree

Hide file tree

Showing 3 changed files with 77 additions and 13 deletions.
diff --git a/.gitignore b/.gitignore
@@ -67,6 +67,9 @@ node_modules/
 # Stores VSCode versions used for testing VSCode extensions
 .vscode-test
 
+# IntelliJ
+.idea
+
 # yarn v3 (w/o zero-install)
 # See: https://yarnpkg.com/getting-started/qa#which-files-should-be-gitignored
 .pnp.*

diff --git a/src/SnapKeyring.test.ts b/src/SnapKeyring.test.ts
@@ -39,14 +39,14 @@ describe('SnapKeyring', () => {
       id: 'b05d918a-b37c-497a-bb28-3d15c0d56b7a',
       address: '0xC728514Df8A7F9271f4B7a4dd2Aa6d2D723d3eE3'.toLowerCase(),
       options: {},
-      methods: [EthMethod.PersonalSign, EthMethod.SignTransaction],
+      methods: [...Object.values(EthMethod)],
       type: EthAccountType.Eoa,
     },
     {
       id: '33c96b60-2237-488e-a7bb-233576f3d22f',
       address: '0x34b13912eAc00152bE0Cb409A301Ab8E55739e63'.toLowerCase(),
       options: {},
-      methods: [EthMethod.SignTransaction, EthMethod.SignTypedDataV1],
+      methods: [...Object.values(EthMethod)],
       type: EthAccountType.Eoa,
     },
   ] as const;
@@ -224,14 +224,70 @@ describe('SnapKeyring', () => {
       ).toBeNull();
     });
 
-    it('fails when the method is not supported', async () => {
+    it('fails when the method is invalid', async () => {
       await expect(
         keyring.handleKeyringSnapMessage(snapId, {
           method: 'invalid',
         }),
       ).rejects.toThrow('Method not supported: invalid');
     });
 
+    it('fails when the EthMethod is not supported', async () => {
+      // Update first account to remove `EthMethod.PersonalSign`
+      let updatedMethods = Object.values(EthMethod).filter(
+        (method) => method !== EthMethod.PersonalSign,
+      );
+      expect(
+        await keyring.handleKeyringSnapMessage(snapId, {
+          method: KeyringEvent.AccountUpdated,
+          params: {
+            account: {
+              ...accounts[0],
+              methods: updatedMethods,
+            },
+          },
+        }),
+      ).toBeNull();
+      expect(keyring.listAccounts()[0]?.methods).toStrictEqual(updatedMethods);
+      await expect(
+        keyring.signPersonalMessage(accounts[0].address, 'hello'),
+      ).rejects.toThrow(
+        `Method '${EthMethod.PersonalSign}' not supported for account ${accounts[0].address}`,
+      );
+      // Restore `EthMethod.PersonalSign` and remove `EthMethod.SignTransaction`
+      updatedMethods = Object.values(EthMethod).filter(
+        (method) => method !== EthMethod.SignTransaction,
+      );
+      expect(
+        await keyring.handleKeyringSnapMessage(snapId, {
+          method: KeyringEvent.AccountUpdated,
+          params: {
+            account: {
+              ...accounts[0],
+              methods: updatedMethods,
+            },
+          },
+        }),
+      ).toBeNull();
+      expect(keyring.listAccounts()[0]?.methods).toStrictEqual(updatedMethods);
+      const mockTx = {
+        data: '0x0',
+        gasLimit: '0x26259fe',
+        gasPrice: '0x1',
+        nonce: '0xfffffffe',
+        to: '0xccccccccccccd000000000000000000000000000',
+        value: '0x1869e',
+        chainId: '0x1',
+        type: '0x00',
+      };
+      const tx = TransactionFactory.fromTxData(mockTx);
+      await expect(
+        keyring.signTransaction(accounts[0].address, tx),
+      ).rejects.toThrow(
+        `Method '${EthMethod.SignTransaction}' not supported for account ${accounts[0].address}`,
+      );
+    });
+
     it('approves an async request', async () => {
       mockSnapController.handleRequest.mockResolvedValue({
         pending: true,

diff --git a/src/SnapKeyring.ts b/src/SnapKeyring.ts
@@ -3,32 +3,32 @@ import { TransactionFactory } from '@ethereumjs/tx';
 import type { TypedDataV1, TypedMessage } from '@metamask/eth-sig-util';
 import { SignTypedDataVersion } from '@metamask/eth-sig-util';
 import type {
-  KeyringAccount,
-  InternalAccount,
   EthBaseTransaction,
   EthBaseUserOperation,
   EthUserOperation,
   EthUserOperationPatch,
+  InternalAccount,
+  KeyringAccount,
 } from '@metamask/keyring-api';
 import {
-  EthMethod,
-  KeyringSnapControllerClient,
-  KeyringEvent,
   AccountCreatedEventStruct,
-  AccountUpdatedEventStruct,
   AccountDeletedEventStruct,
-  RequestApprovedEventStruct,
-  RequestRejectedEventStruct,
+  AccountUpdatedEventStruct,
   EthBaseUserOperationStruct,
-  EthUserOperationPatchStruct,
   EthBytesStruct,
+  EthMethod,
+  EthUserOperationPatchStruct,
+  KeyringEvent,
+  KeyringSnapControllerClient,
+  RequestApprovedEventStruct,
+  RequestRejectedEventStruct,
 } from '@metamask/keyring-api';
 import type { SnapController } from '@metamask/snaps-controllers';
 import type { SnapId } from '@metamask/snaps-sdk';
 import type { Json } from '@metamask/utils';
 import { bigIntToHex } from '@metamask/utils';
 import { EventEmitter } from 'events';
-import { assert, object, string, mask } from 'superstruct';
+import { assert, mask, object, string } from 'superstruct';
 import { v4 as uuid } from 'uuid';
 
 import { DeferredPromise } from './DeferredPromise';
@@ -396,6 +396,11 @@ export class SnapKeyring extends EventEmitter {
     chainId?: string;
   }): Promise<Json> {
     const { account, snapId } = this.#resolveAddress(address);
+    if (!account.methods.includes(method as EthMethod)) {
+      throw new Error(
+        `Method '${method}' not supported for account ${account.address}`,
+      );
+    }
     const requestId = uuid();
 
     // Create the promise before calling the snap to prevent a race condition