Feat/scan url cache

eslint-warning-thresholds.json

@@ -416,11 +416,8 @@
   "packages/permission-log-controller/tests/PermissionLogController.test.ts": {
     "import-x/order": 1
   },
-  "packages/phishing-controller/src/PhishingController.test.ts": {
-    "import-x/no-named-as-default-member": 1
-  },
   "packages/phishing-controller/src/PhishingController.ts": {
-    "jsdoc/check-tag-names": 42,
+    "jsdoc/check-tag-names": 38,
     "jsdoc/tag-lines": 1
   },
   "packages/phishing-controller/src/PhishingDetector.ts": {

packages/phishing-controller/CHANGELOG.md

@@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Added
+
+- Add URL scan cache functionality to improve performance ([#5625](https://github.com/MetaMask/core/pull/5625))
+  - Added `UrlScanCache` class for caching phishing detection scan results
+  - Added methods to `PhishingController`: `setUrlScanCacheTTL`, `setUrlScanCacheMaxSize`, `clearUrlScanCache`
+  - Added URL scan cache state to `PhishingControllerState`
+  - Added configuration options: `urlScanCacheTTL` and `urlScanCacheMaxSize`
+
 ### Changed
 
 - Bump `@metamask/controller-utils` to `^11.7.0` ([#5583](https://github.com/MetaMask/core/pull/5583))

packages/phishing-controller/src/PhishingController.test.ts

@@ -1,6 +1,6 @@
 import { Messenger } from '@metamask/base-controller';
 import { strict as assert } from 'assert';
-import nock from 'nock';
+import nock, { cleanAll, isDone, pendingMocks } from 'nock';
 import sinon from 'sinon';
 
 import {
@@ -54,6 +54,7 @@ function getPhishingController(options?: Partial<PhishingControllerOptions>) {
 describe('PhishingController', () => {
   afterEach(() => {
     sinon.restore();
+    cleanAll();
   });
 
   it('should have no default phishing lists', () => {
@@ -2154,7 +2155,7 @@ describe('PhishingController', () => {
 
   describe('PhishingController - isBlockedRequest', () => {
     afterEach(() => {
-      nock.cleanAll();
+      cleanAll();
     });
 
     it('should return false if c2DomainBlocklist is not defined or empty', async () => {
@@ -2588,3 +2589,323 @@ describe('PhishingController', () => {
     });
   });
 });
+
+describe('URL Scan Cache', () => {
+  let clock: sinon.SinonFakeTimers;
+
+  beforeEach(() => {
+    clock = sinon.useFakeTimers();
+  });
+  afterEach(() => {
+    sinon.restore();
+    cleanAll();
+  });
+
+  it('should cache scan results and return them on subsequent calls', async () => {
+    const testDomain = 'example.com';
+
+    // Spy on the fetch function to track calls
+    const fetchSpy = jest.spyOn(global, 'fetch');
+
+    nock(PHISHING_DETECTION_BASE_URL)
+      .get(
+        `/${PHISHING_DETECTION_SCAN_ENDPOINT}?url=${encodeURIComponent(testDomain)}`,
+      )
+      .reply(200, {
+        recommendedAction: RecommendedAction.None,
+      });
+
+    const controller = getPhishingController();
+
+    const result1 = await controller.scanUrl(`https://${testDomain}`);
+    expect(result1).toStrictEqual({
+      domainName: testDomain,
+      recommendedAction: RecommendedAction.None,
+    });
+
+    const result2 = await controller.scanUrl(`https://${testDomain}`);
+    expect(result2).toStrictEqual({
+      domainName: testDomain,
+      recommendedAction: RecommendedAction.None,
+    });
+
+    // Verify that fetch was called exactly once
+    expect(fetchSpy).toHaveBeenCalledTimes(1);
+
+    fetchSpy.mockRestore();
+  });
+
+  it('should expire cache entries after TTL', async () => {
+    const testDomain = 'example.com';
+    const cacheTTL = 300; // 5 minutes
+
+    nock(PHISHING_DETECTION_BASE_URL)
+      .get(
+        `/${PHISHING_DETECTION_SCAN_ENDPOINT}?url=${encodeURIComponent(testDomain)}`,
+      )
+      .reply(200, {
+        recommendedAction: RecommendedAction.None,
+      })
+      .get(
+        `/${PHISHING_DETECTION_SCAN_ENDPOINT}?url=${encodeURIComponent(testDomain)}`,
+      )
+      .reply(200, {
+        recommendedAction: RecommendedAction.None,
+      });
+
+    const controller = getPhishingController({
+      urlScanCacheTTL: cacheTTL,
+    });
+
+    await controller.scanUrl(`https://${testDomain}`);
+
+    // Before TTL expires, should use cache
+    clock.tick((cacheTTL - 10) * 1000);
+    await controller.scanUrl(`https://${testDomain}`);
+    expect(pendingMocks()).toHaveLength(1); // One mock remaining
+
+    // After TTL expires, should fetch again
+    clock.tick(11 * 1000);
+    await controller.scanUrl(`https://${testDomain}`);
+    expect(pendingMocks()).toHaveLength(0); // All mocks used
+  });
+
+  it('should evict oldest entries when cache exceeds max size', async () => {
+    const maxCacheSize = 2;
+    const domains = ['domain1.com', 'domain2.com', 'domain3.com'];
+
+    // Setup nock to respond to all three domains
+    domains.forEach((domain) => {
+      nock(PHISHING_DETECTION_BASE_URL)
+        .get(
+          `/${PHISHING_DETECTION_SCAN_ENDPOINT}?url=${encodeURIComponent(domain)}`,
+        )
+        .reply(200, {
+          recommendedAction: RecommendedAction.None,
+        });
+    });
+
+    // Setup a second request for the first domain
+    nock(PHISHING_DETECTION_BASE_URL)
+      .get(
+        `/${PHISHING_DETECTION_SCAN_ENDPOINT}?url=${encodeURIComponent(domains[0])}`,
+      )
+      .reply(200, {
+        recommendedAction: RecommendedAction.Warn,
+      });
+
+    const controller = getPhishingController({
+      urlScanCacheMaxSize: maxCacheSize,
+    });
+
+    // Fill the cache
+    await controller.scanUrl(`https://${domains[0]}`);
+    clock.tick(1000); // Ensure different timestamps
+    await controller.scanUrl(`https://${domains[1]}`);
+
+    // This should evict the oldest entry (domain1)
+    clock.tick(1000);
+    await controller.scanUrl(`https://${domains[2]}`);
+
+    // Now domain1 should not be in cache and require a new fetch
+    await controller.scanUrl(`https://${domains[0]}`);
+
+    // All mocks should be used
+    expect(isDone()).toBe(true);
+  });
+
+  it('should clear the cache when clearUrlScanCache is called', async () => {
+    const testDomain = 'example.com';
+
+    nock(PHISHING_DETECTION_BASE_URL)
+      .get(
+        `/${PHISHING_DETECTION_SCAN_ENDPOINT}?url=${encodeURIComponent(testDomain)}`,
+      )
+      .reply(200, {
+        recommendedAction: RecommendedAction.None,
+      })
+      .get(
+        `/${PHISHING_DETECTION_SCAN_ENDPOINT}?url=${encodeURIComponent(testDomain)}`,
+      )
+      .reply(200, {
+        recommendedAction: RecommendedAction.None,
+      });
+
+    const controller = getPhishingController();
+
+    // First call should fetch from API
+    await controller.scanUrl(`https://${testDomain}`);
+
+    // Clear the cache
+    controller.clearUrlScanCache();
+
+    // Should fetch again
+    await controller.scanUrl(`https://${testDomain}`);
+
+    // All mocks should be used
+    expect(isDone()).toBe(true);
+  });
+
+  it('should allow changing the TTL', async () => {
+    const testDomain = 'example.com';
+    const initialTTL = 300; // 5 minutes
+    const newTTL = 60; // 1 minute
+
+    nock(PHISHING_DETECTION_BASE_URL)
+      .get(
+        `/${PHISHING_DETECTION_SCAN_ENDPOINT}?url=${encodeURIComponent(testDomain)}`,
+      )
+      .reply(200, {
+        recommendedAction: RecommendedAction.None,
+      })
+      .get(
+        `/${PHISHING_DETECTION_SCAN_ENDPOINT}?url=${encodeURIComponent(testDomain)}`,
+      )
+      .reply(200, {
+        recommendedAction: RecommendedAction.None,
+      });
+
+    const controller = getPhishingController({
+      urlScanCacheTTL: initialTTL,
+    });
+
+    // First call should fetch from API
+    await controller.scanUrl(`https://${testDomain}`);
+
+    // Change TTL
+    controller.setUrlScanCacheTTL(newTTL);
+
+    // Before new TTL expires, should use cache
+    clock.tick((newTTL - 10) * 1000);
+    await controller.scanUrl(`https://${testDomain}`);
+    expect(pendingMocks()).toHaveLength(1); // One mock remaining
+
+    // After new TTL expires, should fetch again
+    clock.tick(11 * 1000);
+    await controller.scanUrl(`https://${testDomain}`);
+    expect(pendingMocks()).toHaveLength(0); // All mocks used
+  });
+
+  it('should allow changing the max cache size', async () => {
+    const initialMaxSize = 3;
+    const newMaxSize = 2;
+    const domains = [
+      'domain1.com',
+      'domain2.com',
+      'domain3.com',
+      'domain4.com',
+    ];
+
+    // Setup nock to respond to all domains
+    domains.forEach((domain) => {
+      nock(PHISHING_DETECTION_BASE_URL)
+        .get(
+          `/${PHISHING_DETECTION_SCAN_ENDPOINT}?url=${encodeURIComponent(domain)}`,
+        )
+        .reply(200, {
+          recommendedAction: RecommendedAction.None,
+        });
+    });
+
+    const controller = getPhishingController({
+      urlScanCacheMaxSize: initialMaxSize,
+    });
+
+    // Fill the cache to initial size
+    await controller.scanUrl(`https://${domains[0]}`);
+    clock.tick(1000); // Ensure different timestamps
+    await controller.scanUrl(`https://${domains[1]}`);
+    clock.tick(1000);
+    await controller.scanUrl(`https://${domains[2]}`);
+
+    // Verify initial cache size
+    expect(Object.keys(controller.state.urlScanCache)).toHaveLength(
+      initialMaxSize,
+    );
+    // Reduce the max size
+    controller.setUrlScanCacheMaxSize(newMaxSize);
+
+    // Add another entry which should trigger eviction
+    await controller.scanUrl(`https://${domains[3]}`);
+
+    // Verify the cache size doesn't exceed new max size
+    expect(
+      Object.keys(controller.state.urlScanCache).length,
+    ).toBeLessThanOrEqual(newMaxSize);
+  });
+
+  it('should handle fetch errors and not cache them', async () => {
+    const testDomain = 'example.com';
+
+    nock(PHISHING_DETECTION_BASE_URL)
+      .get(
+        `/${PHISHING_DETECTION_SCAN_ENDPOINT}?url=${encodeURIComponent(testDomain)}`,
+      )
+      .reply(500, { error: 'Internal Server Error' })
+      .get(
+        `/${PHISHING_DETECTION_SCAN_ENDPOINT}?url=${encodeURIComponent(testDomain)}`,
+      )
+      .reply(200, {
+        recommendedAction: RecommendedAction.None,
+      });
+
+    const controller = getPhishingController();
+
+    // First call should result in an error response
+    const result1 = await controller.scanUrl(`https://${testDomain}`);
+    expect(result1.fetchError).toBeDefined();
+
+    // Second call should try again (not use cache since errors aren't cached)
+    const result2 = await controller.scanUrl(`https://${testDomain}`);
+    expect(result2.fetchError).toBeUndefined();
+    expect(result2.recommendedAction).toBe(RecommendedAction.None);
+
+    // All mocks should be used
+    expect(isDone()).toBe(true);
+  });
+
+  it('should handle timeout errors and not cache them', async () => {
+    const testDomain = 'example.com';
+
+    // First mock a timeout/error response
+    nock(PHISHING_DETECTION_BASE_URL)
+      .get(
+        `/${PHISHING_DETECTION_SCAN_ENDPOINT}?url=${encodeURIComponent(testDomain)}`,
+      )
+      .replyWithError('connection timeout')
+      .get(
+        `/${PHISHING_DETECTION_SCAN_ENDPOINT}?url=${encodeURIComponent(testDomain)}`,
+      )
+      .reply(200, {
+        recommendedAction: RecommendedAction.None,
+      });
+
+    const controller = getPhishingController();
+
+    // First call should result in an error
+    const result1 = await controller.scanUrl(`https://${testDomain}`);
+    expect(result1.fetchError).toBeDefined();
+
+    // Second call should succeed (not use cache since errors aren't cached)
+    const result2 = await controller.scanUrl(`https://${testDomain}`);
+    expect(result2.fetchError).toBeUndefined();
+    expect(result2.recommendedAction).toBe(RecommendedAction.None);
+
+    // All mocks should be used
+    expect(isDone()).toBe(true);
+  });
+
+  it('should handle invalid URLs and not cache them', async () => {
+    const invalidUrl = 'not-a-valid-url';
+
+    const controller = getPhishingController();
+
+    // First call should return an error for invalid URL
+    const result1 = await controller.scanUrl(invalidUrl);
+    expect(result1.fetchError).toBeDefined();
+
+    // Second call should also return an error (not from cache)
+    const result2 = await controller.scanUrl(invalidUrl);
+    expect(result2.fetchError).toBeDefined();
+  });
+});