fix: Remove invalid providerConfig id (#26310)

## **Description** 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. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/26310?quickstart=1) ## **Related issues** Relates to #26309 and #26320 ## **Manual testing steps** N/A ## **Screenshots/Recordings** N/A ## **Pre-merge author checklist** - [x] 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). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] 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.
MetaMask · Aug 12, 2024 · 4446aa7 · 4446aa7
1 parent 8ac32bd
commit 4446aa7
Show file tree

Hide file tree

Showing 2 changed files with 222 additions and 3 deletions.
diff --git a/app/scripts/migrations/120.2.test.ts b/app/scripts/migrations/120.2.test.ts
@@ -250,9 +250,105 @@ 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 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: {
+          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 +385,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
@@ -1,5 +1,6 @@
 import { cloneDeep } from 'lodash';
 import { hasProperty, isObject } from '@metamask/utils';
+import log from 'loglevel';
 
 type VersionedData = {
   meta: { version: number };
@@ -40,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}'`,
       ),
@@ -93,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}'`,
       ),
@@ -103,6 +104,53 @@ function removeObsoleteNetworkControllerState(
 
   const networkControllerState = state.NetworkController;
 
+  // Check for invalid `providerConfig.id`, and remove if found
+  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(
+          `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;