Skip to content

feat: scanUrl dedupe logic and flag for cache/in-flight requests only #6032

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.

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions packages/phishing-controller/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Added

- Added deduplication for outbound requests for the same url for `scanUrl`. ([#6032](https://github.com/MetaMask/core/pull/6032))
- Added a `noFetch` flag to only return resolved scans or in-flight scans for `scanUrl`. ([6032](https://github.com/MetaMask/core/pull/6032))

### Changed

- **BREAKING**`scanUrl` hits the v2 endpoint now. Returns `hostname` instead of `domainName` now. ([#5981](https://github.com/MetaMask/core/pull/5981))
Expand Down
36 changes: 36 additions & 0 deletions packages/phishing-controller/src/PhishingController.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2589,6 +2589,42 @@ describe('PhishingController', () => {
expect(response).toMatchObject(mockResponse);
expect(scope.isDone()).toBe(true);
});

it('deduplicates concurrent requests for the same hostname', async () => {
const duplicateTestUrl = 'https://example.com';
const expectedHostname = 'example.com';
const scope = nock(PHISHING_DETECTION_BASE_URL)
.get(`/${PHISHING_DETECTION_SCAN_ENDPOINT}`)
.query({ url: expectedHostname })
.reply(200, mockResponse);

const [result1, result2, result3] = await Promise.all([
controller.scanUrl(duplicateTestUrl),
controller.scanUrl(duplicateTestUrl),
controller.scanUrl(duplicateTestUrl),
]);

expect(result1).toMatchObject(mockResponse);
expect(result2).toMatchObject(mockResponse);
expect(result3).toMatchObject(mockResponse);
expect(scope.isDone()).toBe(true);
// eslint-disable-next-line import-x/no-named-as-default-member
expect(nock.pendingMocks()).toHaveLength(0);
});

it('does not make a request if noFetch is true', async () => {
const scope = nock(PHISHING_DETECTION_BASE_URL)
.get(`/${PHISHING_DETECTION_SCAN_ENDPOINT}`)
.query({ url: 'example.com' })
.reply(200, mockResponse);

const response = await controller.scanUrl(testUrl, true);
expect(response).toMatchObject({
hostname: '',
recommendedAction: RecommendedAction.None,
});
expect(scope.isDone()).toBe(false);
});
});

describe('bulkScanUrls', () => {
Expand Down
42 changes: 40 additions & 2 deletions packages/phishing-controller/src/PhishingController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,11 @@ export class PhishingController extends BaseController<

#isProgressC2DomainBlocklistUpdate?: Promise<void>;

readonly #inProgressScans = new Map<
string,
Promise<PhishingDetectionScanResult>
>();

/**
* Construct a Phishing Controller.
*
Expand Down Expand Up @@ -650,9 +655,14 @@ export class PhishingController extends BaseController<
* web URLs.
*
* @param url - The URL to scan.
* @param noFetch - An optional flag to prevent making an outbound request. If true, the
* function will return the cached result or in-flight requests.
* @returns The phishing detection scan result.
*/
scanUrl = async (url: string): Promise<PhishingDetectionScanResult> => {
scanUrl = async (
url: string,
noFetch?: boolean,
): Promise<PhishingDetectionScanResult> => {
const [hostname, ok] = getHostnameFromWebUrl(url);
if (!ok) {
return {
Expand All @@ -667,6 +677,34 @@ export class PhishingController extends BaseController<
return cachedResult;
}

const inProgressScan = this.#inProgressScans.get(hostname);
if (inProgressScan) {
return inProgressScan;
}

if (noFetch) {
// At this point we know there is no cached result and no in-progress scan so we can return a default result.
return { hostname: '', recommendedAction: RecommendedAction.None };
}

try {
const scanPromise = this.#performScan(hostname);
this.#inProgressScans.set(hostname, scanPromise);
return await scanPromise;
} finally {
// Clean up the in-progress cache when scan completes (success or failure)
this.#inProgressScans.delete(hostname);
}
};

/**
* Perform the actual phishing scan for a hostname.
* This method contains the HTTP request logic.
*
* @param hostname - The hostname to scan.
* @returns The phishing detection scan result.
*/
async #performScan(hostname: string): Promise<PhishingDetectionScanResult> {
const apiResponse = await safelyExecuteWithTimeout(
async () => {
const res = await fetch(
Expand Down Expand Up @@ -713,7 +751,7 @@ export class PhishingController extends BaseController<
this.#urlScanCache.add(hostname, result);

return result;
};
}

/**
* Scan multiple URLs for phishing in bulk. It will only scan the hostnames of the URLs.
Expand Down
Loading