Skip to content

Commit 0adfc97

Browse files
GudahttGudahtt
authored
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.
1 parent 0b03d58 commit 0adfc97

File tree

4 files changed

+282
-108
lines changed

4 files changed

+282
-108
lines changed

‎jest.config.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ module.exports = {
2525
// modules.
2626
restoreMocks: true,
2727
setupFiles: ['./tests/setupTests.ts'],
28+
setupFilesAfterEnv: ['./tests/setupAfterEnv.ts'],
2829
testEnvironment: 'jsdom',
2930
testRegex: ['\\.test\\.(ts|js)$'],
3031
testTimeout: 5000,

‎src/third-party/PhishingController.test.ts‎

Lines changed: 169 additions & 107 deletions
Original file line numberDiff line numberDiff line change
@@ -162,44 +162,6 @@ describe('PhishingController', () => {
162162
expect(nockScope.isDone()).toBe(true);
163163
});
164164

165-
it('should update lists', async () => {
166-
const mockPhishingBlocklist = ['https://example-blocked-website.com'];
167-
const phishfortHotlist = ['https://another-example-blocked-website.com'];
168-
nock(PHISHING_CONFIG_BASE_URL)
169-
.get(METAMASK_CONFIG_FILE)
170-
.reply(200, {
171-
blacklist: mockPhishingBlocklist,
172-
fuzzylist: [],
173-
tolerance: 0,
174-
whitelist: [],
175-
version: 0,
176-
})
177-
.get(PHISHFORT_HOTLIST_FILE)
178-
.reply(200, phishfortHotlist);
179-
180-
const controller = new PhishingController();
181-
await controller.updatePhishingLists();
182-
183-
expect(controller.state.phishing).toStrictEqual([
184-
{
185-
allowlist: [],
186-
blocklist: mockPhishingBlocklist,
187-
fuzzylist: [],
188-
tolerance: 0,
189-
name: 'MetaMask',
190-
version: 0,
191-
},
192-
{
193-
allowlist: [],
194-
blocklist: phishfortHotlist,
195-
fuzzylist: [],
196-
tolerance: 0,
197-
name: 'PhishFort',
198-
version: 1,
199-
},
200-
]);
201-
});
202-
203165
it('should not bubble up connection errors', async () => {
204166
nock(PHISHING_CONFIG_BASE_URL)
205167
.get(METAMASK_CONFIG_FILE)
@@ -215,38 +177,6 @@ describe('PhishingController', () => {
215177
});
216178
});
217179

218-
it('should not update phishing configuration if disabled', async () => {
219-
const controller = new PhishingController({ disabled: true });
220-
await controller.updatePhishingLists();
221-
222-
expect(controller.state.phishing).toStrictEqual([
223-
{
224-
allowlist: DEFAULT_PHISHING_RESPONSE.whitelist,
225-
blocklist: DEFAULT_PHISHING_RESPONSE.blacklist,
226-
fuzzylist: DEFAULT_PHISHING_RESPONSE.fuzzylist,
227-
tolerance: DEFAULT_PHISHING_RESPONSE.tolerance,
228-
name: `MetaMask`,
229-
version: DEFAULT_PHISHING_RESPONSE.version,
230-
},
231-
]);
232-
});
233-
234-
it('should return negative result for safe domain from default config', async () => {
235-
// Return API failure to ensure default config is not overwritten
236-
nock(PHISHING_CONFIG_BASE_URL)
237-
.get(METAMASK_CONFIG_FILE)
238-
.reply(500)
239-
.get(PHISHFORT_HOTLIST_FILE)
240-
.reply(500);
241-
242-
const controller = new PhishingController();
243-
expect(await controller.test('metamask.io')).toMatchObject({
244-
result: false,
245-
type: 'allowlist',
246-
name: 'MetaMask',
247-
});
248-
});
249-
250180
it('should return negative result for safe unicode domain from default config', async () => {
251181
// Return API failure to ensure default config is not overwritten
252182
nock(PHISHING_CONFIG_BASE_URL)
@@ -708,47 +638,179 @@ describe('PhishingController', () => {
708638
});
709639
});
710640

711-
it('should not update phishing lists if fetch returns 304', async () => {
712-
nock(PHISHING_CONFIG_BASE_URL)
713-
.get(METAMASK_CONFIG_FILE)
714-
.reply(304)
715-
.get(PHISHFORT_HOTLIST_FILE)
716-
.reply(304);
641+
describe('updatePhishingLists', () => {
642+
it('should update lists', async () => {
643+
const mockPhishingBlocklist = ['https://example-blocked-website.com'];
644+
const phishfortHotlist = ['https://another-example-blocked-website.com'];
645+
nock(PHISHING_CONFIG_BASE_URL)
646+
.get(METAMASK_CONFIG_FILE)
647+
.reply(200, {
648+
blacklist: mockPhishingBlocklist,
649+
fuzzylist: [],
650+
tolerance: 0,
651+
whitelist: [],
652+
version: 0,
653+
})
654+
.get(PHISHFORT_HOTLIST_FILE)
655+
.reply(200, phishfortHotlist);
656+
657+
const controller = new PhishingController();
658+
await controller.updatePhishingLists();
659+
660+
expect(controller.state.phishing).toStrictEqual([
661+
{
662+
allowlist: [],
663+
blocklist: mockPhishingBlocklist,
664+
fuzzylist: [],
665+
tolerance: 0,
666+
name: 'MetaMask',
667+
version: 0,
668+
},
669+
{
670+
allowlist: [],
671+
blocklist: phishfortHotlist,
672+
fuzzylist: [],
673+
tolerance: 0,
674+
name: 'PhishFort',
675+
version: 1,
676+
},
677+
]);
678+
});
717679

718-
const controller = new PhishingController();
719-
await controller.updatePhishingLists();
680+
it('should not update phishing configuration if disabled', async () => {
681+
const controller = new PhishingController({ disabled: true });
682+
await controller.updatePhishingLists();
683+
684+
expect(controller.state.phishing).toStrictEqual([
685+
{
686+
allowlist: DEFAULT_PHISHING_RESPONSE.whitelist,
687+
blocklist: DEFAULT_PHISHING_RESPONSE.blacklist,
688+
fuzzylist: DEFAULT_PHISHING_RESPONSE.fuzzylist,
689+
tolerance: DEFAULT_PHISHING_RESPONSE.tolerance,
690+
name: `MetaMask`,
691+
version: DEFAULT_PHISHING_RESPONSE.version,
692+
},
693+
]);
694+
});
720695

721-
expect(controller.state.phishing).toStrictEqual([
722-
{
723-
allowlist: DEFAULT_PHISHING_RESPONSE.whitelist,
724-
blocklist: DEFAULT_PHISHING_RESPONSE.blacklist,
725-
fuzzylist: DEFAULT_PHISHING_RESPONSE.fuzzylist,
726-
tolerance: DEFAULT_PHISHING_RESPONSE.tolerance,
727-
name: `MetaMask`,
728-
version: DEFAULT_PHISHING_RESPONSE.version,
729-
},
730-
]);
731-
});
696+
it('should return negative result for safe domain from default config', async () => {
697+
// Return API failure to ensure default config is not overwritten
698+
nock(PHISHING_CONFIG_BASE_URL)
699+
.get(METAMASK_CONFIG_FILE)
700+
.reply(500)
701+
.get(PHISHFORT_HOTLIST_FILE)
702+
.reply(500);
703+
704+
const controller = new PhishingController();
705+
expect(await controller.test('metamask.io')).toMatchObject({
706+
result: false,
707+
type: 'allowlist',
708+
name: 'MetaMask',
709+
});
710+
});
732711

733-
it('should not update phishing lists if fetch returns 500', async () => {
734-
nock(PHISHING_CONFIG_BASE_URL)
735-
.get(METAMASK_CONFIG_FILE)
736-
.reply(500)
737-
.get(PHISHFORT_HOTLIST_FILE)
738-
.reply(500);
712+
it('should not update phishing lists if fetch returns 304', async () => {
713+
nock(PHISHING_CONFIG_BASE_URL)
714+
.get(METAMASK_CONFIG_FILE)
715+
.reply(304)
716+
.get(PHISHFORT_HOTLIST_FILE)
717+
.reply(304);
718+
719+
const controller = new PhishingController();
720+
await controller.updatePhishingLists();
721+
722+
expect(controller.state.phishing).toStrictEqual([
723+
{
724+
allowlist: DEFAULT_PHISHING_RESPONSE.whitelist,
725+
blocklist: DEFAULT_PHISHING_RESPONSE.blacklist,
726+
fuzzylist: DEFAULT_PHISHING_RESPONSE.fuzzylist,
727+
tolerance: DEFAULT_PHISHING_RESPONSE.tolerance,
728+
name: `MetaMask`,
729+
version: DEFAULT_PHISHING_RESPONSE.version,
730+
},
731+
]);
732+
});
739733

740-
const controller = new PhishingController();
741-
await controller.updatePhishingLists();
734+
it('should not update phishing lists if fetch returns 500', async () => {
735+
nock(PHISHING_CONFIG_BASE_URL)
736+
.get(METAMASK_CONFIG_FILE)
737+
.reply(500)
738+
.get(PHISHFORT_HOTLIST_FILE)
739+
.reply(500);
740+
741+
const controller = new PhishingController();
742+
await controller.updatePhishingLists();
743+
744+
expect(controller.state.phishing).toStrictEqual([
745+
{
746+
allowlist: DEFAULT_PHISHING_RESPONSE.whitelist,
747+
blocklist: DEFAULT_PHISHING_RESPONSE.blacklist,
748+
fuzzylist: DEFAULT_PHISHING_RESPONSE.fuzzylist,
749+
tolerance: DEFAULT_PHISHING_RESPONSE.tolerance,
750+
name: `MetaMask`,
751+
version: DEFAULT_PHISHING_RESPONSE.version,
752+
},
753+
]);
754+
});
742755

743-
expect(controller.state.phishing).toStrictEqual([
744-
{
745-
allowlist: DEFAULT_PHISHING_RESPONSE.whitelist,
746-
blocklist: DEFAULT_PHISHING_RESPONSE.blacklist,
747-
fuzzylist: DEFAULT_PHISHING_RESPONSE.fuzzylist,
748-
tolerance: DEFAULT_PHISHING_RESPONSE.tolerance,
749-
name: `MetaMask`,
750-
version: DEFAULT_PHISHING_RESPONSE.version,
751-
},
752-
]);
756+
describe('an update is in progress', () => {
757+
it('should not fetch configuration again', async () => {
758+
const clock = sinon.useFakeTimers();
759+
const nockScope = nock(PHISHING_CONFIG_BASE_URL)
760+
.get(METAMASK_CONFIG_FILE)
761+
.delay(100)
762+
.reply(200, {
763+
blacklist: [],
764+
fuzzylist: [],
765+
tolerance: 0,
766+
whitelist: [],
767+
version: 0,
768+
})
769+
.get(PHISHFORT_HOTLIST_FILE)
770+
.delay(100)
771+
.reply(200, []);
772+
773+
const controller = new PhishingController();
774+
const firstPromise = controller.updatePhishingLists();
775+
const secondPromise = controller.updatePhishingLists();
776+
777+
clock.tick(100);
778+
779+
await firstPromise;
780+
await secondPromise;
781+
782+
// This second update would throw if it fetched, because the
783+
// nock interceptor was not persisted.
784+
expect(nockScope.isDone()).toBe(true);
785+
});
786+
787+
it('should wait until the in-progress update has completed', async () => {
788+
const clock = sinon.useFakeTimers();
789+
nock(PHISHING_CONFIG_BASE_URL)
790+
.get(METAMASK_CONFIG_FILE)
791+
.delay(100)
792+
.reply(200, {
793+
blacklist: [],
794+
fuzzylist: [],
795+
tolerance: 0,
796+
whitelist: [],
797+
version: 0,
798+
})
799+
.get(PHISHFORT_HOTLIST_FILE)
800+
.delay(100)
801+
.reply(200, []);
802+
803+
const controller = new PhishingController();
804+
const firstPromise = controller.updatePhishingLists();
805+
const secondPromise = controller.updatePhishingLists();
806+
clock.tick(99);
807+
808+
await expect(secondPromise).toNeverResolve();
809+
810+
// Cleanup pending operations
811+
await firstPromise;
812+
await secondPromise;
813+
});
814+
});
753815
});
754816
});

‎src/third-party/PhishingController.ts‎

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,8 @@ export class PhishingController extends BaseController<
102102

103103
private lastFetched = 0;
104104

105+
#inProgressUpdate: Promise<void> | undefined;
106+
105107
/**
106108
* Name of this controller used during composition
107109
*/
@@ -193,9 +195,32 @@ export class PhishingController extends BaseController<
193195
}
194196

195197
/**
196-
* Updates lists of approved and unapproved website origins.
198+
* Update the phishing configuration.
199+
*
200+
* If an update is in progress, no additional update will be made. Instead this will wait until
201+
* the in-progress update has finished.
197202
*/
198203
async updatePhishingLists() {
204+
if (this.#inProgressUpdate) {
205+
await this.#inProgressUpdate;
206+
return;
207+
}
208+
209+
try {
210+
this.#inProgressUpdate = this.#updatePhishingLists();
211+
await this.#inProgressUpdate;
212+
} finally {
213+
this.#inProgressUpdate = undefined;
214+
}
215+
}
216+
217+
/**
218+
* Update the phishing configuration.
219+
*
220+
* This should only be called from the `updatePhishingLists` function, which is a wrapper around
221+
* this function that prevents redundant configuration updates.
222+
*/
223+
async #updatePhishingLists() {
199224
if (this.disabled) {
200225
return;
201226
}

0 commit comments

Comments
 (0)