From e15952f1b2358a3623a8c78d9dc97216ad71d200 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Fri, 2 Aug 2024 10:28:36 -0230 Subject: [PATCH 1/3] fix: Remove invalid providerConfig ids Remove invalid `providerConfig.id` state. This was one possible explanation for some Sentry errors that we have been encountering in production. The diagnostic state attached to the error was not sufficient to confirm that this was the cause. Relates to #26309 --- app/scripts/migrations/120.2.test.ts | 140 ++++++++++++++++++++++++++- app/scripts/migrations/120.2.ts | 43 ++++++++ 2 files changed, 182 insertions(+), 1 deletion(-) diff --git a/app/scripts/migrations/120.2.test.ts b/app/scripts/migrations/120.2.test.ts index b7860a4f03f2..0d3fc66b6119 100644 --- a/app/scripts/migrations/120.2.test.ts +++ b/app/scripts/migrations/120.2.test.ts @@ -250,9 +250,72 @@ describe('migration #120.2', () => { ); }); - it('does nothing if obsolete properties are not set', async () => { + it('captures an error and leaves state unchanged if providerConfig state is corrupted', async () => { const oldState = { NetworkController: { + providerConfig: 'invalid', + }, + }; + + const transformedState = await migrate({ + meta: { version: oldVersion }, + data: cloneDeep(oldState), + }); + + expect(transformedState.data).toEqual(oldState); + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error( + `Migration ${version}: Invalid NetworkController providerConfig state of type 'string'`, + ), + ); + }); + + it('captures an error and leaves state unchanged if networkConfigurations state is corrupted', async () => { + const oldState = { + NetworkController: { + networkConfigurations: 'invalid', + providerConfig: {}, + }, + }; + + const transformedState = await migrate({ + meta: { version: oldVersion }, + data: cloneDeep(oldState), + }); + + expect(transformedState.data).toEqual(oldState); + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error( + `Migration ${version}: Invalid NetworkController networkConfigurations state of type 'string'`, + ), + ); + }); + + it('does nothing if obsolete properties and providerConfig id are not set', async () => { + const oldState = { + NetworkController: { + providerConfig: {}, + selectedNetworkClientId: 'example', + }, + }; + + const transformedState = await migrate({ + meta: { version: oldVersion }, + data: cloneDeep(oldState), + }); + + expect(transformedState.data).toEqual(oldState); + }); + + it('does not remove a valid providerConfig id', async () => { + const oldState = { + NetworkController: { + networkConfigurations: { + 'valid-id': {}, + }, + providerConfig: { + id: 'valid-id', + }, selectedNetworkClientId: 'example', }, }; @@ -289,6 +352,81 @@ describe('migration #120.2', () => { }); }); + it('removes providerConfig id if network configuration is missing', async () => { + const oldState = { + NetworkController: { + providerConfig: { + id: 'invalid-id', + }, + selectedNetworkClientId: 'example', + }, + }; + + const transformedState = await migrate({ + meta: { version: oldVersion }, + data: cloneDeep(oldState), + }); + + expect(transformedState.data.NetworkController).toEqual({ + providerConfig: {}, + selectedNetworkClientId: 'example', + }); + }); + + it('removes providerConfig id that does not match any network configuration', async () => { + const oldState = { + NetworkController: { + networkConfigurations: { + 'valid-id': {}, + }, + providerConfig: { + id: 'invalid-id', + }, + selectedNetworkClientId: 'example', + }, + }; + + const transformedState = await migrate({ + meta: { version: oldVersion }, + data: cloneDeep(oldState), + }); + + expect(transformedState.data.NetworkController).toEqual({ + networkConfigurations: { + 'valid-id': {}, + }, + providerConfig: {}, + selectedNetworkClientId: 'example', + }); + }); + + it('removes providerConfig id with an invalid type', async () => { + const oldState = { + NetworkController: { + networkConfigurations: { + '123': {}, + }, + providerConfig: { + id: 123, + }, + selectedNetworkClientId: 'example', + }, + }; + + const transformedState = await migrate({ + meta: { version: oldVersion }, + data: cloneDeep(oldState), + }); + + expect(transformedState.data.NetworkController).toEqual({ + networkConfigurations: { + '123': {}, + }, + providerConfig: {}, + selectedNetworkClientId: 'example', + }); + }); + it('still migrates NetworkController state if other controllers have invalid state', async () => { const oldState = { NetworkController: { diff --git a/app/scripts/migrations/120.2.ts b/app/scripts/migrations/120.2.ts index 1db33007bb00..c8d34100d413 100644 --- a/app/scripts/migrations/120.2.ts +++ b/app/scripts/migrations/120.2.ts @@ -1,5 +1,6 @@ import { cloneDeep } from 'lodash'; import { hasProperty, isObject } from '@metamask/utils'; +import log from 'loglevel'; type VersionedData = { meta: { version: number }; @@ -103,6 +104,48 @@ function removeObsoleteNetworkControllerState( const networkControllerState = state.NetworkController; + // Check for invalid `providerConfig.id`, and remove if found + if (hasProperty(networkControllerState, 'providerConfig')) { + if (!isObject(networkControllerState.providerConfig)) { + global.sentry.captureException( + new Error( + `Migration ${version}: Invalid NetworkController providerConfig state of type '${typeof state + .NetworkController.providerConfig}'`, + ), + ); + return; + } + const { providerConfig } = networkControllerState; + + const validNetworkConfigurationIds = []; + if (hasProperty(networkControllerState, 'networkConfigurations')) { + if (!isObject(networkControllerState.networkConfigurations)) { + global.sentry.captureException( + new Error( + `Migration ${version}: Invalid NetworkController networkConfigurations state of type '${typeof networkControllerState.networkConfigurations}'`, + ), + ); + return; + } + + validNetworkConfigurationIds.push( + ...Object.keys(networkControllerState.networkConfigurations), + ); + } + + if (hasProperty(providerConfig, 'id')) { + if ( + typeof providerConfig.id !== 'string' || + !validNetworkConfigurationIds.includes(providerConfig.id) + ) { + log.warn( + `Migration ${version}: Removing invalid provider id ${providerConfig.id}`, + ); + delete providerConfig.id; + } + } + } + delete networkControllerState.networkDetails; delete networkControllerState.networkId; delete networkControllerState.networkStatus; From 4ce3bc91734bd22b2b4e0b8707b6e2ed972cee46 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Fri, 2 Aug 2024 10:51:07 -0230 Subject: [PATCH 2/3] Use optional chain operator for global Sentry access --- app/scripts/migrations/120.2.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/scripts/migrations/120.2.ts b/app/scripts/migrations/120.2.ts index c8d34100d413..4b02eb1ecd5d 100644 --- a/app/scripts/migrations/120.2.ts +++ b/app/scripts/migrations/120.2.ts @@ -41,7 +41,7 @@ function removeObsoleteSnapControllerState( if (!hasProperty(state, 'SnapController')) { return; } else if (!isObject(state.SnapController)) { - global.sentry.captureException( + global.sentry?.captureException?.( new Error( `Migration ${version}: Invalid SnapController state of type '${typeof state.SnapController}'`, ), @@ -94,7 +94,7 @@ function removeObsoleteNetworkControllerState( if (!hasProperty(state, 'NetworkController')) { return; } else if (!isObject(state.NetworkController)) { - global.sentry.captureException( + global.sentry?.captureException?.( new Error( `Migration ${version}: Invalid NetworkController state of type '${typeof state.NetworkController}'`, ), @@ -107,7 +107,7 @@ function removeObsoleteNetworkControllerState( // Check for invalid `providerConfig.id`, and remove if found if (hasProperty(networkControllerState, 'providerConfig')) { if (!isObject(networkControllerState.providerConfig)) { - global.sentry.captureException( + global.sentry?.captureException?.( new Error( `Migration ${version}: Invalid NetworkController providerConfig state of type '${typeof state .NetworkController.providerConfig}'`, @@ -120,7 +120,7 @@ function removeObsoleteNetworkControllerState( const validNetworkConfigurationIds = []; if (hasProperty(networkControllerState, 'networkConfigurations')) { if (!isObject(networkControllerState.networkConfigurations)) { - global.sentry.captureException( + global.sentry?.captureException?.( new Error( `Migration ${version}: Invalid NetworkController networkConfigurations state of type '${typeof networkControllerState.networkConfigurations}'`, ), From 90c5763b251f7415b7b5a51c0c88c17b08e149d7 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Fri, 2 Aug 2024 11:03:42 -0230 Subject: [PATCH 3/3] Add undefined providerConfig check --- app/scripts/migrations/120.2.test.ts | 33 ++++++++++++++++++++++++++++ app/scripts/migrations/120.2.ts | 7 +++++- 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/app/scripts/migrations/120.2.test.ts b/app/scripts/migrations/120.2.test.ts index 0d3fc66b6119..9cd64275580b 100644 --- a/app/scripts/migrations/120.2.test.ts +++ b/app/scripts/migrations/120.2.test.ts @@ -291,6 +291,39 @@ describe('migration #120.2', () => { ); }); + it('does nothing if obsolete properties and providerConfig are not set', async () => { + const oldState = { + NetworkController: { + selectedNetworkClientId: 'example', + }, + }; + + const transformedState = await migrate({ + meta: { version: oldVersion }, + data: cloneDeep(oldState), + }); + + expect(transformedState.data).toEqual(oldState); + }); + + it('does nothing if obsolete properties are not set and providerConfig is set to undefined', async () => { + const oldState = { + NetworkController: { + // This should be impossible because `undefined` cannot be returned from persisted state, + // it's not valid JSON. But a bug in migration 14 ends up setting this to `undefined`. + providerConfig: undefined, + selectedNetworkClientId: 'example', + }, + }; + + const transformedState = await migrate({ + meta: { version: oldVersion }, + data: cloneDeep(oldState), + }); + + expect(transformedState.data).toEqual(oldState); + }); + it('does nothing if obsolete properties and providerConfig id are not set', async () => { const oldState = { NetworkController: { diff --git a/app/scripts/migrations/120.2.ts b/app/scripts/migrations/120.2.ts index 4b02eb1ecd5d..06e6cc091b9e 100644 --- a/app/scripts/migrations/120.2.ts +++ b/app/scripts/migrations/120.2.ts @@ -105,7 +105,12 @@ function removeObsoleteNetworkControllerState( const networkControllerState = state.NetworkController; // Check for invalid `providerConfig.id`, and remove if found - if (hasProperty(networkControllerState, 'providerConfig')) { + if ( + hasProperty(networkControllerState, 'providerConfig') && + // This should be impossible because `undefined` cannot be returned from persisted state, + // it's not valid JSON. But a bug in migration 14 ends up setting this to `undefined`. + networkControllerState.providerConfig !== undefined + ) { if (!isObject(networkControllerState.providerConfig)) { global.sentry?.captureException?.( new Error(