Prevent simultaneous updates

The phishing controller will no longer issue redundant parallel
configuration updates. If a second update request is made while one is
in progress, it will wait for the in-progress update to complete.

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,

src/third-party/PhishingController.test.ts

@@ -23,6 +23,8 @@ describe('PhishingController', () => {
   afterEach(() => {
     nock.cleanAll();
     sinon.restore();
+    jest.resetAllMocks();
+    jest.restoreAllMocks();
   });
 
   it('should set default state to the package phishing configuration', () => {
@@ -600,47 +602,108 @@ 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);
-
-    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,
-      },
-    ]);
-  });
-
-  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);
+  describe('updatedPhishingLists', () => {
+    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);
+
+        expect(secondPromise).toNeverResolve();
+
+        await firstPromise;
+        await secondPromise;
+      });
+    });
   });
 });

src/third-party/PhishingController.ts

@@ -102,6 +102,8 @@ export class PhishingController extends BaseController<
 
   private lastFetched = 0;
 
+  #pendingUpdate: Promise<void> | undefined;
+
   /**
    * Name of this controller used during composition
    */
@@ -190,9 +192,25 @@ export class PhishingController extends BaseController<
   }
 
   /**
-   * Updates lists of approved and unapproved website origins.
+   * Update phishing configuration.
+   *
+   * If an update is in progress, this will wait for the in-progress update to finish.
    */
   async updatePhishingLists() {
+    if (this.#pendingUpdate) {
+      await this.#pendingUpdate;
+      return;
+    }
+
+    try {
+      this.#pendingUpdate = this.#updatePhishingLists();
+      await this.#pendingUpdate;
+    } finally {
+      this.#pendingUpdate = undefined;
+    }
+  }
+
+  async #updatePhishingLists() {
     if (this.disabled) {
       return;
     }

tests/setupAfterEnv.ts

@@ -0,0 +1,85 @@
+declare global {
+  // Using `namespace` here is okay because this is how the Jest types are
+  // defined.
+  /* eslint-disable-next-line @typescript-eslint/no-namespace */
+  namespace jest {
+    interface Matchers<R> {
+      toNeverResolve(): Promise<R>;
+    }
+  }
+}
+
+const UNRESOLVED = Symbol('timedOut');
+// Store this in case it gets stubbed later
+const originalSetTimeout = global.setTimeout;
+const TIME_TO_WAIT_UNTIL_UNRESOLVED = 100;
+
+/**
+ * Produces a sort of dummy promise which can be used in conjunction with a
+ * "real" promise to determine whether the "real" promise was ever resolved. If
+ * the promise that is produced by this function resolves first, then the other
+ * one must be unresolved.
+ *
+ * @param duration - How long to wait before resolving the promise returned by
+ * this function.
+ * @returns A promise that resolves to a symbol.
+ */
+const treatUnresolvedAfter = (duration: number): Promise<typeof UNRESOLVED> => {
+  return new Promise((resolve) => {
+    originalSetTimeout(resolve, duration, UNRESOLVED);
+  });
+};
+
+expect.extend({
+  /**
+   * Tests that the given promise is never fulfilled or rejected past a certain
+   * amount of time (which is the default time that Jest tests wait before
+   * timing out as configured in the Jest configuration file).
+   *
+   * Inspired by <https://stackoverflow.com/a/68409467/260771>.
+   *
+   * @param promise - The promise to test.
+   * @returns The result of the matcher.
+   */
+  async toNeverResolve(promise: Promise<any>) {
+    if (this.isNot) {
+      throw new Error(
+        'Using `.not.toNeverResolve(...)` is not supported. ' +
+          'You probably want to either `await` the promise and test its ' +
+          'resolution value or use `.rejects` to test its rejection value instead.',
+      );
+    }
+
+    let resolutionValue: any;
+    let rejectionValue: any;
+    try {
+      resolutionValue = await Promise.race([
+        promise,
+        treatUnresolvedAfter(TIME_TO_WAIT_UNTIL_UNRESOLVED),
+      ]);
+    } catch (e) {
+      rejectionValue = e;
+    }
+
+    return resolutionValue === UNRESOLVED
+      ? {
+          message: () =>
+            `Expected promise to resolve after ${TIME_TO_WAIT_UNTIL_UNRESOLVED}ms, but it did not`,
+          pass: true,
+        }
+      : {
+          message: () => {
+            return `Expected promise to never resolve after ${TIME_TO_WAIT_UNTIL_UNRESOLVED}ms, but it ${
+              rejectionValue
+                ? `was rejected with ${rejectionValue}`
+                : `resolved with ${resolutionValue}`
+            }`;
+          },
+          pass: false,
+        };
+  },
+});
+
+// Export something so that TypeScript thinks that we are performing type
+// augmentation
+export {};