Prevent parallel phishing configuration updates (#930)

When a phishing configuration update is requested while an update is in-progress, we now wait for the in-progress update to finish instead of initiating another redundant update.
MetaMask · Oct 11, 2023 · 168b6b3 · 168b6b3
1 parent 42d79e4
commit 168b6b3
Show file tree

Hide file tree

Showing 4 changed files with 282 additions and 108 deletions.
diff --git a/jest.config.js b/jest.config.js
@@ -25,6 +25,7 @@ module.exports = {
   // modules.
   restoreMocks: true,
   setupFiles: ['./tests/setupTests.ts'],
+  setupFilesAfterEnv: ['./tests/setupAfterEnv.ts'],
   testEnvironment: 'jsdom',
   testRegex: ['\\.test\\.(ts|js)$'],
   testTimeout: 5000,

diff --git a/src/third-party/PhishingController.test.ts b/src/third-party/PhishingController.test.ts
@@ -162,44 +162,6 @@ describe('PhishingController', () => {
     expect(nockScope.isDone()).toBe(true);
   });
 
-  it('should update lists', async () => {
-    const mockPhishingBlocklist = ['https://example-blocked-website.com'];
-    const phishfortHotlist = ['https://another-example-blocked-website.com'];
-    nock(PHISHING_CONFIG_BASE_URL)
-      .get(METAMASK_CONFIG_FILE)
-      .reply(200, {
-        blacklist: mockPhishingBlocklist,
-        fuzzylist: [],
-        tolerance: 0,
-        whitelist: [],
-        version: 0,
-      })
-      .get(PHISHFORT_HOTLIST_FILE)
-      .reply(200, phishfortHotlist);
-
-    const controller = new PhishingController();
-    await controller.updatePhishingLists();
-
-    expect(controller.state.phishing).toStrictEqual([
-      {
-        allowlist: [],
-        blocklist: mockPhishingBlocklist,
-        fuzzylist: [],
-        tolerance: 0,
-        name: 'MetaMask',
-        version: 0,
-      },
-      {
-        allowlist: [],
-        blocklist: phishfortHotlist,
-        fuzzylist: [],
-        tolerance: 0,
-        name: 'PhishFort',
-        version: 1,
-      },
-    ]);
-  });
-
   it('should not bubble up connection errors', async () => {
     nock(PHISHING_CONFIG_BASE_URL)
       .get(METAMASK_CONFIG_FILE)
@@ -215,38 +177,6 @@ describe('PhishingController', () => {
     });
   });
 
-  it('should not update phishing configuration if disabled', async () => {
-    const controller = new PhishingController({ disabled: true });
-    await controller.updatePhishingLists();
-
-    expect(controller.state.phishing).toStrictEqual([
-      {
-        allowlist: DEFAULT_PHISHING_RESPONSE.whitelist,
-        blocklist: DEFAULT_PHISHING_RESPONSE.blacklist,
-        fuzzylist: DEFAULT_PHISHING_RESPONSE.fuzzylist,
-        tolerance: DEFAULT_PHISHING_RESPONSE.tolerance,
-        name: `MetaMask`,
-        version: DEFAULT_PHISHING_RESPONSE.version,
-      },
-    ]);
-  });
-
-  it('should return negative result for safe domain from default config', async () => {
-    // Return API failure to ensure default config is not overwritten
-    nock(PHISHING_CONFIG_BASE_URL)
-      .get(METAMASK_CONFIG_FILE)
-      .reply(500)
-      .get(PHISHFORT_HOTLIST_FILE)
-      .reply(500);
-
-    const controller = new PhishingController();
-    expect(await controller.test('metamask.io')).toMatchObject({
-      result: false,
-      type: 'allowlist',
-      name: 'MetaMask',
-    });
-  });
-
   it('should return negative result for safe unicode domain from default config', async () => {
     // Return API failure to ensure default config is not overwritten
     nock(PHISHING_CONFIG_BASE_URL)
@@ -708,47 +638,179 @@ describe('PhishingController', () => {
     });
   });
 
-  it('should not update phishing lists if fetch returns 304', async () => {
-    nock(PHISHING_CONFIG_BASE_URL)
-      .get(METAMASK_CONFIG_FILE)
-      .reply(304)
-      .get(PHISHFORT_HOTLIST_FILE)
-      .reply(304);
+  describe('updatePhishingLists', () => {
+    it('should update lists', async () => {
+      const mockPhishingBlocklist = ['https://example-blocked-website.com'];
+      const phishfortHotlist = ['https://another-example-blocked-website.com'];
+      nock(PHISHING_CONFIG_BASE_URL)
+        .get(METAMASK_CONFIG_FILE)
+        .reply(200, {
+          blacklist: mockPhishingBlocklist,
+          fuzzylist: [],
+          tolerance: 0,
+          whitelist: [],
+          version: 0,
+        })
+        .get(PHISHFORT_HOTLIST_FILE)
+        .reply(200, phishfortHotlist);
+
+      const controller = new PhishingController();
+      await controller.updatePhishingLists();
+
+      expect(controller.state.phishing).toStrictEqual([
+        {
+          allowlist: [],
+          blocklist: mockPhishingBlocklist,
+          fuzzylist: [],
+          tolerance: 0,
+          name: 'MetaMask',
+          version: 0,
+        },
+        {
+          allowlist: [],
+          blocklist: phishfortHotlist,
+          fuzzylist: [],
+          tolerance: 0,
+          name: 'PhishFort',
+          version: 1,
+        },
+      ]);
+    });
 
-    const controller = new PhishingController();
-    await controller.updatePhishingLists();
+    it('should not update phishing configuration if disabled', async () => {
+      const controller = new PhishingController({ disabled: true });
+      await controller.updatePhishingLists();
+
+      expect(controller.state.phishing).toStrictEqual([
+        {
+          allowlist: DEFAULT_PHISHING_RESPONSE.whitelist,
+          blocklist: DEFAULT_PHISHING_RESPONSE.blacklist,
+          fuzzylist: DEFAULT_PHISHING_RESPONSE.fuzzylist,
+          tolerance: DEFAULT_PHISHING_RESPONSE.tolerance,
+          name: `MetaMask`,
+          version: DEFAULT_PHISHING_RESPONSE.version,
+        },
+      ]);
+    });
 
-    expect(controller.state.phishing).toStrictEqual([
-      {
-        allowlist: DEFAULT_PHISHING_RESPONSE.whitelist,
-        blocklist: DEFAULT_PHISHING_RESPONSE.blacklist,
-        fuzzylist: DEFAULT_PHISHING_RESPONSE.fuzzylist,
-        tolerance: DEFAULT_PHISHING_RESPONSE.tolerance,
-        name: `MetaMask`,
-        version: DEFAULT_PHISHING_RESPONSE.version,
-      },
-    ]);
-  });
+    it('should return negative result for safe domain from default config', async () => {
+      // Return API failure to ensure default config is not overwritten
+      nock(PHISHING_CONFIG_BASE_URL)
+        .get(METAMASK_CONFIG_FILE)
+        .reply(500)
+        .get(PHISHFORT_HOTLIST_FILE)
+        .reply(500);
+
+      const controller = new PhishingController();
+      expect(await controller.test('metamask.io')).toMatchObject({
+        result: false,
+        type: 'allowlist',
+        name: 'MetaMask',
+      });
+    });
 
-  it('should not update phishing lists if fetch returns 500', async () => {
-    nock(PHISHING_CONFIG_BASE_URL)
-      .get(METAMASK_CONFIG_FILE)
-      .reply(500)
-      .get(PHISHFORT_HOTLIST_FILE)
-      .reply(500);
+    it('should not update phishing lists if fetch returns 304', async () => {
+      nock(PHISHING_CONFIG_BASE_URL)
+        .get(METAMASK_CONFIG_FILE)
+        .reply(304)
+        .get(PHISHFORT_HOTLIST_FILE)
+        .reply(304);
+
+      const controller = new PhishingController();
+      await controller.updatePhishingLists();
+
+      expect(controller.state.phishing).toStrictEqual([
+        {
+          allowlist: DEFAULT_PHISHING_RESPONSE.whitelist,
+          blocklist: DEFAULT_PHISHING_RESPONSE.blacklist,
+          fuzzylist: DEFAULT_PHISHING_RESPONSE.fuzzylist,
+          tolerance: DEFAULT_PHISHING_RESPONSE.tolerance,
+          name: `MetaMask`,
+          version: DEFAULT_PHISHING_RESPONSE.version,
+        },
+      ]);
+    });
 
-    const controller = new PhishingController();
-    await controller.updatePhishingLists();
+    it('should not update phishing lists if fetch returns 500', async () => {
+      nock(PHISHING_CONFIG_BASE_URL)
+        .get(METAMASK_CONFIG_FILE)
+        .reply(500)
+        .get(PHISHFORT_HOTLIST_FILE)
+        .reply(500);
+
+      const controller = new PhishingController();
+      await controller.updatePhishingLists();
+
+      expect(controller.state.phishing).toStrictEqual([
+        {
+          allowlist: DEFAULT_PHISHING_RESPONSE.whitelist,
+          blocklist: DEFAULT_PHISHING_RESPONSE.blacklist,
+          fuzzylist: DEFAULT_PHISHING_RESPONSE.fuzzylist,
+          tolerance: DEFAULT_PHISHING_RESPONSE.tolerance,
+          name: `MetaMask`,
+          version: DEFAULT_PHISHING_RESPONSE.version,
+        },
+      ]);
+    });
 
-    expect(controller.state.phishing).toStrictEqual([
-      {
-        allowlist: DEFAULT_PHISHING_RESPONSE.whitelist,
-        blocklist: DEFAULT_PHISHING_RESPONSE.blacklist,
-        fuzzylist: DEFAULT_PHISHING_RESPONSE.fuzzylist,
-        tolerance: DEFAULT_PHISHING_RESPONSE.tolerance,
-        name: `MetaMask`,
-        version: DEFAULT_PHISHING_RESPONSE.version,
-      },
-    ]);
+    describe('an update is in progress', () => {
+      it('should not fetch configuration again', async () => {
+        const clock = sinon.useFakeTimers();
+        const nockScope = nock(PHISHING_CONFIG_BASE_URL)
+          .get(METAMASK_CONFIG_FILE)
+          .delay(100)
+          .reply(200, {
+            blacklist: [],
+            fuzzylist: [],
+            tolerance: 0,
+            whitelist: [],
+            version: 0,
+          })
+          .get(PHISHFORT_HOTLIST_FILE)
+          .delay(100)
+          .reply(200, []);
+
+        const controller = new PhishingController();
+        const firstPromise = controller.updatePhishingLists();
+        const secondPromise = controller.updatePhishingLists();
+
+        clock.tick(100);
+
+        await firstPromise;
+        await secondPromise;
+
+        // This second update would throw if it fetched, because the
+        // nock interceptor was not persisted.
+        expect(nockScope.isDone()).toBe(true);
+      });
+
+      it('should wait until the in-progress update has completed', async () => {
+        const clock = sinon.useFakeTimers();
+        nock(PHISHING_CONFIG_BASE_URL)
+          .get(METAMASK_CONFIG_FILE)
+          .delay(100)
+          .reply(200, {
+            blacklist: [],
+            fuzzylist: [],
+            tolerance: 0,
+            whitelist: [],
+            version: 0,
+          })
+          .get(PHISHFORT_HOTLIST_FILE)
+          .delay(100)
+          .reply(200, []);
+
+        const controller = new PhishingController();
+        const firstPromise = controller.updatePhishingLists();
+        const secondPromise = controller.updatePhishingLists();
+        clock.tick(99);
+
+        await expect(secondPromise).toNeverResolve();
+
+        // Cleanup pending operations
+        await firstPromise;
+        await secondPromise;
+      });
+    });
   });
 });
diff --git a/src/third-party/PhishingController.ts b/src/third-party/PhishingController.ts
@@ -102,6 +102,8 @@ export class PhishingController extends BaseController<
 
   private lastFetched = 0;
 
+  #inProgressUpdate: Promise<void> | undefined;
+
   /**
    * Name of this controller used during composition
    */
@@ -193,9 +195,32 @@ export class PhishingController extends BaseController<
   }
 
   /**
-   * Updates lists of approved and unapproved website origins.
+   * Update the phishing configuration.
+   *
+   * If an update is in progress, no additional update will be made. Instead this will wait until
+   * the in-progress update has finished.
    */
   async updatePhishingLists() {
+    if (this.#inProgressUpdate) {
+      await this.#inProgressUpdate;
+      return;
+    }
+
+    try {
+      this.#inProgressUpdate = this.#updatePhishingLists();
+      await this.#inProgressUpdate;
+    } finally {
+      this.#inProgressUpdate = undefined;
+    }
+  }
+
+  /**
+   * Update the phishing configuration.
+   *
+   * This should only be called from the `updatePhishingLists` function, which is a wrapper around
+   * this function that prevents redundant configuration updates.
+   */
+  async #updatePhishingLists() {
     if (this.disabled) {
       return;
     }