Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Prevent parallel phishing configuration updates #930

Merged
merged 3 commits into from
Oct 10, 2022
Merged

Prevent parallel phishing configuration updates #930

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6040bf0
Prevent parallel phishing configuration updates
Gudahtt Oct 10, 2022
6f5f280
Add comment to explain cleanup steps
Gudahtt Oct 10, 2022
314a8d4
Add missing `await` in test
Gudahtt Oct 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
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.
  • Loading branch information
@Gudahtt
Gudahtt committed Oct 10, 2022
commit 6040bf06b0d57970ce18323deed0aab54ffa739c
1 change: 1 addition & 0 deletions jest.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
275 changes: 168 additions & 107 deletions src/third-party/PhishingController.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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.
adonesky1 marked this conversation as resolved.
Show resolved Hide resolved
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();
adonesky1 marked this conversation as resolved.
Show resolved Hide resolved
Gudahtt marked this conversation as resolved.
Show resolved Hide resolved

await firstPromise;
await secondPromise;
adonesky1 marked this conversation as resolved.
Show resolved Hide resolved
});
});
});
});
27 changes: 26 additions & 1 deletion src/third-party/PhishingController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,8 @@ export class PhishingController extends BaseController<

private lastFetched = 0;

#inProgressUpdate: Promise<void> | undefined;

/**
* Name of this controller used during composition
*/
Expand Down Expand Up @@ -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;
}
Expand Down
Loading