fix!: make scopes more restrictive

packages/keyring-api/src/api/account.ts

@@ -74,7 +74,7 @@ export const KeyringAccountStruct = object({
   address: string(),
 
   /**
-   * Account supported scopes (CAIP-2 chain IDs).
+   * Account supported scopes (CAIP-2 chain IDs or CAIP-2 namespaces).
    */
   scopes: nonempty(array(union([CaipNamespaceStruct, CaipChainIdStruct]))),
 

packages/keyring-api/src/btc/types.test.ts

@@ -1,4 +1,20 @@
-import { BtcP2wpkhAddressStruct } from './types';
+import { BtcScopes } from './constants';
+import type { BtcP2wpkhAccount } from './types';
+import {
+  BtcMethod,
+  BtcP2wpkhAccountStruct,
+  BtcP2wpkhAddressStruct,
+} from './types';
+import { BtcAccountType } from '../api';
+
+const MOCK_ACCOUNT = {
+  id: '55583f38-d81b-48f8-8494-fc543c2b5c95',
+  type: BtcAccountType.P2wpkh,
+  address: 'bc1qw508d6qejxtdg4y5r3zarvary0c5xw7kv8f3t4',
+  methods: [BtcMethod.SendBitcoin],
+  options: {},
+  scopes: [BtcScopes.Mainnet],
+};
 
 describe('types', () => {
   describe('BtcP2wpkhAddressStruct', () => {
@@ -37,5 +53,25 @@ describe('types', () => {
         `${errorPrefix}: No separator character for ${address}`,
       );
     });
+
+    it('throws an error if there is multiple scopes', () => {
+      const account: BtcP2wpkhAccount = {
+        ...MOCK_ACCOUNT,
+        scopes: [BtcScopes.Mainnet, BtcScopes.Testnet],
+      };
+      expect(() => BtcP2wpkhAccountStruct.assert(account)).toThrow(
+        'At path: scopes -- Expected a array with a length of `1` but received one with a length of `2`',
+      );
+    });
+
+    it('throws an error if there is no scope', () => {
+      const account: BtcP2wpkhAccount = {
+        ...MOCK_ACCOUNT,
+        scopes: [],
+      };
+      expect(() => BtcP2wpkhAccountStruct.assert(account)).toThrow(
+        'At path: scopes -- Expected a array with a length of `1` but received one with a length of `0`',
+      );
+    });
   });
 });

packages/keyring-api/src/btc/types.ts

@@ -1,6 +1,14 @@
 import { object } from '@metamask/keyring-utils';
 import type { Infer } from '@metamask/superstruct';
-import { string, array, enums, refine, literal } from '@metamask/superstruct';
+import {
+  string,
+  array,
+  enums,
+  refine,
+  literal,
+  size,
+} from '@metamask/superstruct';
+import { CaipChainIdStruct } from '@metamask/utils';
 import { bech32 } from 'bech32';
 
 import { BtcAccountType, KeyringAccountStruct } from '../api';
@@ -41,6 +49,13 @@ export const BtcP2wpkhAccountStruct = object({
    */
   type: literal(`${BtcAccountType.P2wpkh}`),
 
+  /**
+   * Account supported scope (CAIP-2 chain ID).
+   *
+   * NOTE: We consider a Bitcoin address to be valid on only 1 network at time.
+   */
+  scopes: size(array(CaipChainIdStruct), 1),
+
   /**
    * Account supported methods.
    */

packages/keyring-api/src/eth/constants.ts

@@ -6,4 +6,5 @@
 export enum EthScopes {
   Namespace = 'eip155',
   Mainnet = 'eip155:1',
+  Testnet = 'eip155:11155111',
 }

packages/keyring-api/src/eth/types.ts

@@ -1,6 +1,7 @@
 import { object, definePattern } from '@metamask/keyring-utils';
 import type { Infer } from '@metamask/superstruct';
 import { nonempty, array, enums, literal } from '@metamask/superstruct';
+import { CaipChainIdStruct } from '@metamask/utils';
 
 import { EthScopes } from '.';
 import { EthAccountType, KeyringAccountStruct } from '../api';
@@ -82,6 +83,11 @@ export const EthErc4337AccountStruct = object({
    */
   type: literal(`${EthAccountType.Erc4337}`),
 
+  /**
+   * Account supported scopes (CAIP-2 chain IDs).
+   */
+  scopes: nonempty(array(CaipChainIdStruct)),
+
   /**
    * Account supported methods.
    */

packages/keyring-api/src/sol/types.ts

@@ -1,6 +1,7 @@
 import { object, definePattern } from '@metamask/keyring-utils';
 import type { Infer } from '@metamask/superstruct';
-import { array, enums, literal } from '@metamask/superstruct';
+import { array, enums, literal, nonempty } from '@metamask/superstruct';
+import { CaipChainIdStruct } from '@metamask/utils';
 
 import { KeyringAccountStruct, SolAccountType } from '../api';
 
@@ -35,6 +36,11 @@ export const SolDataAccountStruct = object({
    */
   type: literal(`${SolAccountType.DataAccount}`),
 
+  /**
+   * Account supported scopes (CAIP-2 chain IDs).
+   */
+  scopes: nonempty(array(CaipChainIdStruct)),
+
   /**
    * Account supported methods.
    */

packages/keyring-snap-bridge/src/SnapKeyring.test.ts

@@ -117,7 +117,7 @@ describe('SnapKeyring', () => {
   };
   const ethEoaAccount3 = {
     id: 'c6697bcf-5710-4751-a1cb-340e4b50617a',
-    address: '0xab1G3q98V7C67T9103g30C0417610237A137d763'.toLowerCase(),
+    address: '0xf7bDe8609231033c69E502C08f85153f8A1548F2'.toLowerCase(),
     options: {},
     methods: ETH_EOA_METHODS,
     scopes: [EthScopes.Namespace],
@@ -128,7 +128,7 @@ describe('SnapKeyring', () => {
     address: '0x2f15b30952aebe0ed5fdbfe5bf16fb9ecdb31d9a'.toLowerCase(),
     options: {},
     methods: ETH_4337_METHODS,
-    scopes: [EthScopes.Namespace],
+    scopes: [EthScopes.Mainnet],
     type: EthAccountType.Erc4337,
   };
   const btcP2wpkhAccount = {
@@ -163,7 +163,7 @@ describe('SnapKeyring', () => {
     // For unknown accounts, we consider them as EVM EOA for now, so just re-use the
     // same scopes.
     scopes: [EthScopes.Namespace],
-    // This should be really possible to create such account, but since we potentially
+    // This should not be really possible to create such account, but since we potentially
     // migrate data upon the Snap keyring initialization, we want to cover edge-cases
     // like this one to avoid crashing and blocking everything...
     type: 'unknown:type' as KeyringAccount['type'],
@@ -312,7 +312,7 @@ describe('SnapKeyring', () => {
             params: {
               account: {
                 ...(ethEoaAccount1 as unknown as KeyringAccount),
-                address: '0x0',
+                address: ethEoaAccount2.address,
               },
             },
           }),
@@ -447,7 +447,7 @@ describe('SnapKeyring', () => {
             },
           }),
         ).rejects.toThrow(
-          'Account scopes is required for non-EVM and ERC4337 accounts',
+          'At path: scopes -- Expected an array value, but received: undefined',
         );
       });
 
@@ -633,7 +633,7 @@ describe('SnapKeyring', () => {
 
       it('fails when the EthMethod is not supported after update', async () => {
         // Update first account to remove `EthMethod.PersonalSign`
-        let updatedMethods: EthMethod[] = Object.values(EthMethod).filter(
+        let updatedMethods: EthMethod[] = Object.values(ETH_EOA_METHODS).filter(
           (method) => method !== EthMethod.PersonalSign,
         );
         expect(
@@ -656,7 +656,7 @@ describe('SnapKeyring', () => {
           `Method '${EthMethod.PersonalSign}' not supported for account ${ethEoaAccount1.address}`,
         );
         // Restore `EthMethod.PersonalSign` and remove `EthMethod.SignTransaction`
-        updatedMethods = Object.values(EthMethod).filter(
+        updatedMethods = Object.values(ETH_EOA_METHODS).filter(
           (method) => method !== EthMethod.SignTransaction,
         );
         expect(
@@ -723,7 +723,7 @@ describe('SnapKeyring', () => {
             params: { account },
           }),
         ).rejects.toThrow(
-          'Account scopes is required for non-EVM and ERC4337 accounts',
+          'At path: scopes -- Expected an array value, but received: undefined',
         );
       });
 
@@ -740,7 +740,7 @@ describe('SnapKeyring', () => {
             params: { account },
           }),
         ).rejects.toThrow(
-          'Account scopes is required for non-EVM and ERC4337 accounts',
+          'At path: scopes -- Expected an array value, but received: undefined',
         );
       });
     });

packages/keyring-snap-bridge/src/SnapKeyring.ts

@@ -43,6 +43,7 @@ import {
 import { EventEmitter } from 'events';
 import { v4 as uuid } from 'uuid';
 
+import { transformAccount } from './account';
 import { DeferredPromise } from './DeferredPromise';
 import {
   AccountCreatedEventStruct,
@@ -52,11 +53,7 @@ import {
   RequestRejectedEventStruct,
 } from './events';
 import { projectLogger as log } from './logger';
-import {
-  isAccountV1,
-  migrateAccountV1,
-  transformAccountV1,
-} from './migrations';
+import { isAccountV1, migrateAccountV1 } from './migrations';
 import { SnapIdMap } from './SnapIdMap';
 import type {
   SnapKeyringEvents,
@@ -208,9 +205,8 @@ export class SnapKeyring extends EventEmitter {
       displayConfirmation,
     } = message.params;
 
-    // To keep the retro-compatibility with older keyring-api versions, we mark some fields
-    // as optional and provide them here if they are missing.
-    const account = transformAccountV1(newAccountFromEvent);
+    // Potentially migrate the account.
+    const account = transformAccount(newAccountFromEvent);
 
     // The UI still uses the account address to identify accounts, so we need
     // to block the creation of duplicate accounts for now to prevent accounts
@@ -258,9 +254,8 @@ export class SnapKeyring extends EventEmitter {
       this.#accounts.get(snapId, newAccountFromEvent.id) ??
       throwError(`Account '${newAccountFromEvent.id}' not found`);
 
-    // To keep the retro-compatibility with older keyring-api versions, we mark some fields
-    // as optional and provide them here if they are missing.
-    const newAccount = transformAccountV1(newAccountFromEvent);
+    // Potentially migrate the account.
+    const newAccount = transformAccount(newAccountFromEvent);
 
     // The address of the account cannot be changed. In the future, we will
     // support changing the address of an account since it will be required to

packages/keyring-snap-bridge/src/account.test.ts

@@ -0,0 +1,18 @@
+import type { KeyringAccount } from '@metamask/keyring-api';
+
+import { transformAccount } from './account';
+
+describe('account', () => {
+  it('throws for unknown account type', () => {
+    const unknownAccount = {
+      // This should not be really possible to create such account, but since we potentially
+      // migrate data upon the Snap keyring initialization, we want to cover edge-cases
+      // like this one to avoid crashing and blocking everything...
+      type: 'unknown:type',
+    } as unknown as KeyringAccount; // Just testing the default case.
+
+    expect(() => transformAccount(unknownAccount)).toThrow(
+      "Unknown account type: 'unknown:type'",
+    );
+  });
+});

packages/keyring-snap-bridge/src/account.ts

@@ -1,5 +1,17 @@
-import { KeyringAccountStruct } from '@metamask/keyring-api';
-import { omit, type Infer } from '@metamask/superstruct';
+import type { KeyringAccount, KeyringAccountType } from '@metamask/keyring-api';
+import {
+  BtcAccountType,
+  BtcP2wpkhAccountStruct,
+  EthAccountType,
+  EthEoaAccountStruct,
+  EthErc4337AccountStruct,
+  KeyringAccountStruct,
+  SolAccountType,
+  SolDataAccountStruct,
+} from '@metamask/keyring-api';
+import { assert, omit, type Infer } from '@metamask/superstruct';
+
+import { isAccountV1, transformAccountV1 } from './migrations';
 
 /**
  * A `KeyringAccount` with some optional fields which can be used to keep
@@ -8,3 +20,60 @@ import { omit, type Infer } from '@metamask/superstruct';
 export const KeyringAccountV1Struct = omit(KeyringAccountStruct, ['scopes']);
 
 export type KeyringAccountV1 = Infer<typeof KeyringAccountV1Struct>;
+
+/**
+ * Assert that an account-like object matches its actual account type.
+ *
+ * @param account - The account-like object.
+ * @returns The account as normal `KeyringAccount`.
+ */
+export function assertKeyringAccount<
+  Account extends { type: KeyringAccountType },
+>(account: Account): KeyringAccount {
+  // TODO: We should use a `selectiveUnion` for this and probably use it to define
+  // the `KeyringAccount`. This would also required to have a "generic `KeyringAccount`"
+  // definition.
+  switch (account.type) {
+    case BtcAccountType.P2wpkh: {
+      assert(account, BtcP2wpkhAccountStruct);
+      return account;
+    }
+    case SolAccountType.DataAccount: {
+      assert(account, SolDataAccountStruct);
+      return account;
+    }
+    case EthAccountType.Erc4337: {
+      assert(account, EthErc4337AccountStruct);
+      return account;
+    }
+    case EthAccountType.Eoa: {
+      assert(account, EthEoaAccountStruct);
+      return account;
+    }
+    default: {
+      // For now, we cannot much more than this (this should also, never happen)!
+      // NOTE: We could use a  "generic `KeyringAccount` type" here though.
+      throw new Error(`Unknown account type: '${account.type}'`);
+    }
+  }
+}
+
+/**
+ * Transform any versionned account to a `KeyringAccount`.
+ *
+ * @param accountToTransform - The account to transform.
+ * @returns A valid transformed `KeyringAccount`.
+ */
+export function transformAccount(
+  // eslint-disable-next-line @typescript-eslint/no-redundant-type-constituents
+  accountToTransform: KeyringAccountV1 | KeyringAccount,
+): KeyringAccount {
+  // To keep the retro-compatibility with older keyring-api versions, we identify the account's
+  // version and transform it to the latest `KeyringAccount` representation.
+  const account = isAccountV1(accountToTransform)
+    ? transformAccountV1(accountToTransform)
+    : accountToTransform;
+
+  // We still assert that the converted account is valid according to their account's type.
+  return assertKeyringAccount(account);
+}