From 92a1d24e49f6f7cdfdb77c6fe71112b88b99db78 Mon Sep 17 00:00:00 2001 From: jiexi Date: Tue, 15 Oct 2024 09:00:21 -0700 Subject: [PATCH] Multichain: Do not add permittedChain scope for snaps. Use new networkConfigurationsByChainId property (#27849) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27849?quickstart=1) ## **Related issues** Fixes: ## **Manual testing steps** 1. Go to this page... 2. 3. ## **Screenshots/Recordings** ### **Before** ### **After** ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --- app/scripts/migrations/131.test.ts | 270 ++++++++++++++++++++++++++--- app/scripts/migrations/131.ts | 94 ++++++---- 2 files changed, 305 insertions(+), 59 deletions(-) diff --git a/app/scripts/migrations/131.test.ts b/app/scripts/migrations/131.test.ts index 9b805bde3a48..f65dfe5996b2 100644 --- a/app/scripts/migrations/131.test.ts +++ b/app/scripts/migrations/131.test.ts @@ -189,7 +189,7 @@ describe('migration #131', () => { expect(newStorage.data).toStrictEqual(oldStorage.data); }); - it('does nothing if NetworkController.networkConfigurations is not an object', async () => { + it('does nothing if NetworkController.networkConfigurationsByChainId is not an object', async () => { const oldStorage = { meta: { version: oldVersion }, data: { @@ -198,7 +198,7 @@ describe('migration #131', () => { }, NetworkController: { selectedNetworkClientId: 'mainnet', - networkConfigurations: 'foo', + networkConfigurationsByChainId: 'foo', }, SelectedNetworkController: {}, }, @@ -208,7 +208,7 @@ describe('migration #131', () => { expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( new Error( - `Migration ${version}: typeof state.NetworkController.networkConfigurations is string`, + `Migration ${version}: typeof state.NetworkController.networkConfigurationsByChainId is string`, ), ); expect(newStorage.data).toStrictEqual(oldStorage.data); @@ -223,7 +223,7 @@ describe('migration #131', () => { }, NetworkController: { selectedNetworkClientId: 'mainnet', - networkConfigurations: {}, + networkConfigurationsByChainId: {}, }, SelectedNetworkController: { domains: 'foo', @@ -241,7 +241,7 @@ describe('migration #131', () => { expect(newStorage.data).toStrictEqual(oldStorage.data); }); - it('does nothing if the currently selected network client is neither built in nor exists in NetworkController.networkConfigurations', async () => { + it('does nothing if NetworkController.networkConfigurationsByChainId[] is not an object', async () => { const oldStorage = { meta: { version: oldVersion }, data: { @@ -250,7 +250,98 @@ describe('migration #131', () => { }, NetworkController: { selectedNetworkClientId: 'nonExistentNetworkClientId', - networkConfigurations: {}, + networkConfigurationsByChainId: { + '0x1': 'foo', + }, + }, + SelectedNetworkController: { + domains: {}, + }, + }, + }; + + const newStorage = await migrate(oldStorage); + + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error( + `Migration ${version}: typeof state.NetworkController.networkConfigurationsByChainId["0x1"] is string`, + ), + ); + expect(newStorage.data).toStrictEqual(oldStorage.data); + }); + + it('does nothing if NetworkController.networkConfigurationsByChainId[].rpcEndpoints is not an array', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: { + PermissionController: { + subjects: {}, + }, + NetworkController: { + selectedNetworkClientId: 'nonExistentNetworkClientId', + networkConfigurationsByChainId: { + '0x1': { + rpcEndpoints: 'foo', + }, + }, + }, + SelectedNetworkController: { + domains: {}, + }, + }, + }; + + const newStorage = await migrate(oldStorage); + + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error( + `Migration ${version}: typeof state.NetworkController.networkConfigurationsByChainId["0x1"].rpcEndpoints is string`, + ), + ); + expect(newStorage.data).toStrictEqual(oldStorage.data); + }); + + it('does nothing if NetworkController.networkConfigurationsByChainId[].rpcEndpoints[] is not an object', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: { + PermissionController: { + subjects: {}, + }, + NetworkController: { + selectedNetworkClientId: 'nonExistentNetworkClientId', + networkConfigurationsByChainId: { + '0x1': { + rpcEndpoints: ['foo'], + }, + }, + }, + SelectedNetworkController: { + domains: {}, + }, + }, + }; + + const newStorage = await migrate(oldStorage); + + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error( + `Migration ${version}: typeof state.NetworkController.networkConfigurationsByChainId["0x1"].rpcEndpoints[] is string`, + ), + ); + expect(newStorage.data).toStrictEqual(oldStorage.data); + }); + + it('does nothing if the currently selected network client is neither built in nor exists in NetworkController.networkConfigurationsByChainId', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: { + PermissionController: { + subjects: {}, + }, + NetworkController: { + selectedNetworkClientId: 'nonExistentNetworkClientId', + networkConfigurationsByChainId: {}, }, SelectedNetworkController: { domains: {}, @@ -274,7 +365,7 @@ describe('migration #131', () => { data: { NetworkController: { selectedNetworkClientId: 'mainnet', - networkConfigurations: {}, + networkConfigurationsByChainId: {}, }, SelectedNetworkController: { domains: {}, @@ -303,7 +394,7 @@ describe('migration #131', () => { data: { NetworkController: { selectedNetworkClientId: 'mainnet', - networkConfigurations: {}, + networkConfigurationsByChainId: {}, }, SelectedNetworkController: { domains: {}, @@ -334,7 +425,7 @@ describe('migration #131', () => { data: { NetworkController: { selectedNetworkClientId: 'mainnet', - networkConfigurations: {}, + networkConfigurationsByChainId: {}, }, SelectedNetworkController: { domains: {}, @@ -357,7 +448,7 @@ describe('migration #131', () => { expect(newStorage.data).toStrictEqual({ NetworkController: { selectedNetworkClientId: 'mainnet', - networkConfigurations: {}, + networkConfigurationsByChainId: {}, }, SelectedNetworkController: { domains: {}, @@ -382,7 +473,7 @@ describe('migration #131', () => { 'built-in', { selectedNetworkClientId: 'mainnet', - networkConfigurations: {}, + networkConfigurationsByChainId: {}, }, '1', ], @@ -390,9 +481,13 @@ describe('migration #131', () => { 'custom', { selectedNetworkClientId: 'customId', - networkConfigurations: { - customId: { - chainId: '0xf', + networkConfigurationsByChainId: { + '0xf': { + rpcEndpoints: [ + { + networkClientId: 'customId', + }, + ], }, }, }, @@ -403,7 +498,12 @@ describe('migration #131', () => { ( _type: string, NetworkController: { - networkConfigurations: Record; + networkConfigurationsByChainId: Record< + string, + { + rpcEndpoints: { networkClientId: string }[]; + } + >; } & Record, chainId: string, ) => { @@ -471,6 +571,14 @@ describe('migration #131', () => { methods: [], notifications: [], }, + 'wallet:eip155': { + accounts: [ + 'wallet:eip155:0xdeadbeef', + 'wallet:eip155:0x999', + ], + methods: [], + notifications: [], + }, }, isMultichainOrigin: false, }, @@ -547,6 +655,14 @@ describe('migration #131', () => { methods: [], notifications: [], }, + 'wallet:eip155': { + accounts: [ + 'wallet:eip155:0xdeadbeef', + 'wallet:eip155:0x999', + ], + methods: [], + notifications: [], + }, }, isMultichainOrigin: false, }, @@ -623,6 +739,80 @@ describe('migration #131', () => { methods: [], notifications: [], }, + 'wallet:eip155': { + accounts: [ + 'wallet:eip155:0xdeadbeef', + 'wallet:eip155:0x999', + ], + methods: [], + notifications: [], + }, + }, + isMultichainOrigin: false, + }, + }, + ], + }, + }, + }, + }, + }, + }); + }); + + it('replaces the eth_accounts permission with a CAIP-25 permission using the eth_accounts value without permitted chains when the origin is snapId', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: { + ...baseData(), + PermissionController: { + subjects: { + 'npm:snap': { + permissions: { + unrelated: { + foo: 'bar', + }, + [PermissionNames.eth_accounts]: { + caveats: [ + { + type: 'restrictReturnedAccounts', + value: ['0xdeadbeef', '0x999'], + }, + ], + }, + }, + }, + }, + }, + }, + }; + + const newStorage = await migrate(oldStorage); + expect(newStorage.data).toStrictEqual({ + ...baseData(), + PermissionController: { + subjects: { + 'npm:snap': { + permissions: { + unrelated: { + foo: 'bar', + }, + 'endowment:caip25': { + parentCapability: 'endowment:caip25', + caveats: [ + { + type: 'authorizedScopes', + value: { + requiredScopes: {}, + optionalScopes: { + 'wallet:eip155': { + accounts: [ + 'wallet:eip155:0xdeadbeef', + 'wallet:eip155:0x999', + ], + methods: [], + notifications: [], + }, }, isMultichainOrigin: false, }, @@ -643,10 +833,14 @@ describe('migration #131', () => { ...baseData(), NetworkController: { ...baseData().NetworkController, - networkConfigurations: { - ...baseData().NetworkController.networkConfigurations, - customNetworkClientId: { - chainId: '0xa', + networkConfigurationsByChainId: { + ...baseData().NetworkController.networkConfigurationsByChainId, + '0xa': { + rpcEndpoints: [ + { + networkClientId: 'customNetworkClientId', + }, + ], }, }, }, @@ -682,10 +876,14 @@ describe('migration #131', () => { ...baseData(), NetworkController: { ...baseData().NetworkController, - networkConfigurations: { - ...baseData().NetworkController.networkConfigurations, - customNetworkClientId: { - chainId: '0xa', + networkConfigurationsByChainId: { + ...baseData().NetworkController.networkConfigurationsByChainId, + '0xa': { + rpcEndpoints: [ + { + networkClientId: 'customNetworkClientId', + }, + ], }, }, }, @@ -717,6 +915,14 @@ describe('migration #131', () => { methods: [], notifications: [], }, + 'wallet:eip155': { + accounts: [ + 'wallet:eip155:0xdeadbeef', + 'wallet:eip155:0x999', + ], + methods: [], + notifications: [], + }, }, isMultichainOrigin: false, }, @@ -843,6 +1049,14 @@ describe('migration #131', () => { methods: [], notifications: [], }, + 'wallet:eip155': { + accounts: [ + 'wallet:eip155:0xdeadbeef', + 'wallet:eip155:0x999', + ], + methods: [], + notifications: [], + }, }, isMultichainOrigin: false, }, @@ -912,6 +1126,11 @@ describe('migration #131', () => { methods: [], notifications: [], }, + 'wallet:eip155': { + accounts: ['wallet:eip155:0xdeadbeef'], + methods: [], + notifications: [], + }, }, isMultichainOrigin: false, }, @@ -935,6 +1154,11 @@ describe('migration #131', () => { methods: [], notifications: [], }, + 'wallet:eip155': { + accounts: ['wallet:eip155:0xdeadbeef'], + methods: [], + notifications: [], + }, }, isMultichainOrigin: false, }, diff --git a/app/scripts/migrations/131.ts b/app/scripts/migrations/131.ts index 5d38816ed7f5..fdb0f618f9cd 100644 --- a/app/scripts/migrations/131.ts +++ b/app/scripts/migrations/131.ts @@ -1,10 +1,4 @@ -import { - hasProperty, - Hex, - isObject, - NonEmptyArray, - Json, -} from '@metamask/utils'; +import { hasProperty, isObject, NonEmptyArray, Json } from '@metamask/utils'; import { cloneDeep } from 'lodash'; type CaveatConstraint = { @@ -23,29 +17,19 @@ const PermissionNames = { }; const BUILT_IN_NETWORKS = { - goerli: { - chainId: '0x5', - }, - sepolia: { - chainId: '0xaa36a7', - }, - mainnet: { - chainId: '0x1', - }, - 'linea-goerli': { - chainId: '0xe704', - }, - 'linea-sepolia': { - chainId: '0xe705', - }, - 'linea-mainnet': { - chainId: '0xe708', - }, + goerli: '0x5', + sepolia: '0xaa36a7', + mainnet: '0x1', + 'linea-goerli': '0xe704', + 'linea-sepolia': '0xe705', + 'linea-mainnet': '0xe708', }; const Caip25CaveatType = 'authorizedScopes'; const Caip25EndowmentPermissionName = 'endowment:caip25'; +const snapsPrefixes = ['npm:', 'local:'] as const; + type VersionedData = { meta: { version: number }; data: Record; @@ -113,7 +97,10 @@ function transformState(state: Record) { const { PermissionController: { subjects }, - NetworkController: { selectedNetworkClientId, networkConfigurations }, + NetworkController: { + selectedNetworkClientId, + networkConfigurationsByChainId, + }, SelectedNetworkController: { domains }, } = state; @@ -133,10 +120,10 @@ function transformState(state: Record) { ); return state; } - if (!isObject(networkConfigurations)) { + if (!isObject(networkConfigurationsByChainId)) { global.sentry?.captureException( new Error( - `Migration ${version}: typeof state.NetworkController.networkConfigurations is ${typeof networkConfigurations}`, + `Migration ${version}: typeof state.NetworkController.networkConfigurationsByChainId is ${typeof networkConfigurationsByChainId}`, ), ); return state; @@ -151,12 +138,43 @@ function transformState(state: Record) { } const getChainIdForNetworkClientId = (networkClientId: string) => { - const networkConfiguration = - (networkConfigurations[networkClientId] as { chainId: Hex }) ?? - BUILT_IN_NETWORKS[ - networkClientId as unknown as keyof typeof BUILT_IN_NETWORKS - ]; - return networkConfiguration?.chainId; + for (const [chainId, networkConfiguration] of Object.entries( + networkConfigurationsByChainId, + )) { + if (!isObject(networkConfiguration)) { + global.sentry?.captureException( + new Error( + `Migration ${version}: typeof state.NetworkController.networkConfigurationsByChainId["${chainId}"] is ${typeof networkConfiguration}`, + ), + ); + return null; + } + if (!Array.isArray(networkConfiguration.rpcEndpoints)) { + global.sentry?.captureException( + new Error( + `Migration ${version}: typeof state.NetworkController.networkConfigurationsByChainId["${chainId}"].rpcEndpoints is ${typeof networkConfiguration.rpcEndpoints}`, + ), + ); + return null; + } + for (const rpcEndpoint of networkConfiguration.rpcEndpoints) { + if (!isObject(rpcEndpoint)) { + global.sentry?.captureException( + new Error( + `Migration ${version}: typeof state.NetworkController.networkConfigurationsByChainId["${chainId}"].rpcEndpoints[] is ${typeof rpcEndpoint}`, + ), + ); + return null; + } + if (rpcEndpoint.networkClientId === networkClientId) { + return chainId; + } + } + } + + return BUILT_IN_NETWORKS[ + networkClientId as unknown as keyof typeof BUILT_IN_NETWORKS + ]; }; const currentChainId = getChainIdForNetworkClientId(selectedNetworkClientId); @@ -235,10 +253,14 @@ function transformState(state: Record) { } } + const isSnap = snapsPrefixes.some((prefix) => origin.startsWith(prefix)); const scopes: Record = {}; + const scopeStrings = isSnap + ? [] + : chainIds.map((chainId) => `eip155:${parseInt(chainId, 16)}`); + scopeStrings.push('wallet:eip155'); - chainIds.forEach((chainId) => { - const scopeString = `eip155:${parseInt(chainId, 16)}`; + scopeStrings.forEach((scopeString) => { const caipAccounts = ethAccounts.map( (account) => `${scopeString}:${account}`, );