From 6040bf06b0d57970ce18323deed0aab54ffa739c Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Mon, 10 Oct 2022 11:50:36 -0230 Subject: [PATCH 1/3] Prevent parallel phishing configuration updates 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. --- jest.config.js | 1 + src/third-party/PhishingController.test.ts | 275 +++++++++++++-------- src/third-party/PhishingController.ts | 27 +- tests/setupAfterEnv.ts | 86 +++++++ 4 files changed, 281 insertions(+), 108 deletions(-) create mode 100644 tests/setupAfterEnv.ts diff --git a/jest.config.js b/jest.config.js index 53ebdf6de8..2eb2fe4143 100644 --- 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 index f3d2e98c65..535a280533 100644 --- 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,178 @@ 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); + + expect(secondPromise).toNeverResolve(); + + await firstPromise; + await secondPromise; + }); + }); }); }); diff --git a/src/third-party/PhishingController.ts b/src/third-party/PhishingController.ts index 49f878d7f2..4bcd02eac8 100644 --- 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 | 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; } diff --git a/tests/setupAfterEnv.ts b/tests/setupAfterEnv.ts new file mode 100644 index 0000000000..f2c3c8470b --- /dev/null +++ b/tests/setupAfterEnv.ts @@ -0,0 +1,86 @@ +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 { + // eslint-disable-next-line @typescript-eslint/no-unused-vars, @typescript-eslint/ban-types + interface Matchers { + toNeverResolve(): Promise; + } + } +} + +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 => { + 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 . + * + * @param promise - The promise to test. + * @returns The result of the matcher. + */ + async toNeverResolve(promise: Promise) { + 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 {}; From 6f5f2802fc8475c23a31c1326d980bb2d38b29cd Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Mon, 10 Oct 2022 13:26:28 -0230 Subject: [PATCH 2/3] Add comment to explain cleanup steps --- src/third-party/PhishingController.test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/third-party/PhishingController.test.ts b/src/third-party/PhishingController.test.ts index 535a280533..df245d7e6e 100644 --- a/src/third-party/PhishingController.test.ts +++ b/src/third-party/PhishingController.test.ts @@ -807,6 +807,7 @@ describe('PhishingController', () => { expect(secondPromise).toNeverResolve(); + // Cleanup pending operations await firstPromise; await secondPromise; }); From 314a8d4bef1c73044ec48639d1a219660ba8888d Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Mon, 10 Oct 2022 13:37:50 -0230 Subject: [PATCH 3/3] Add missing `await` in test --- src/third-party/PhishingController.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/third-party/PhishingController.test.ts b/src/third-party/PhishingController.test.ts index df245d7e6e..77a2dff615 100644 --- a/src/third-party/PhishingController.test.ts +++ b/src/third-party/PhishingController.test.ts @@ -805,7 +805,7 @@ describe('PhishingController', () => { const secondPromise = controller.updatePhishingLists(); clock.tick(99); - expect(secondPromise).toNeverResolve(); + await expect(secondPromise).toNeverResolve(); // Cleanup pending operations await firstPromise;