Skip to content

Commit d91e7b0

Browse files
AugmentedModeAugmentedMode
authored
Feat/scan url cache (#5625)
## Explanation The PhishingController's `scanUrl` method currently makes an API call for every URL scan request, even when we've recently scanned the same URL. To address this, I've added a caching layer that: 1. Stores scan results in a persistent cache 2. Checks the cache before making API calls 3. Automatically expires cached entries after a configurable TTL (default: 5 minutes) 4. Manages cache size with an LRU-based eviction policy when it exceeds the maximum size (default: 100 entries) This should reduce redundant API calls while maintaining security by respecting TTL for cached entries. ## References ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've communicated my changes to consumers by [updating changelogs for packages I've changed](https://github.com/MetaMask/core/tree/main/docs/contributing.md#updating-changelogs), highlighting breaking changes as necessary - [ ] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes
1 parent f92caba commit d91e7b0

File tree

6 files changed

+773
-11
lines changed

6 files changed

+773
-11
lines changed

‎eslint-warning-thresholds.json‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -416,11 +416,8 @@
416416
"packages/permission-log-controller/tests/PermissionLogController.test.ts": {
417417
"import-x/order": 1
418418
},
419-
"packages/phishing-controller/src/PhishingController.test.ts": {
420-
"import-x/no-named-as-default-member": 1
421-
},
422419
"packages/phishing-controller/src/PhishingController.ts": {
423-
"jsdoc/check-tag-names": 42,
420+
"jsdoc/check-tag-names": 38,
424421
"jsdoc/tag-lines": 1
425422
},
426423
"packages/phishing-controller/src/PhishingDetector.ts": {

‎packages/phishing-controller/CHANGELOG.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
99

1010
### Added
1111

12+
- Add URL scan cache functionality to improve performance ([#5625](https://github.com/MetaMask/core/pull/5625))
13+
- Added `UrlScanCache` class for caching phishing detection scan results
14+
- Added methods to `PhishingController`: `setUrlScanCacheTTL`, `setUrlScanCacheMaxSize`, `clearUrlScanCache`
15+
- Added URL scan cache state to `PhishingControllerState`
16+
- Added configuration options: `urlScanCacheTTL` and `urlScanCacheMaxSize`
1217
- Add `bulkScanUrls` method to `PhishingController` for scanning multiple URLs for phishing in bulk ([#5682](https://github.com/MetaMask/core/pull/5682))
1318
- Add `BulkPhishingDetectionScanResponse` type for bulk URL scan results ([#5682](https://github.com/MetaMask/core/pull/5682))
1419
- Add `PHISHING_DETECTION_BULK_SCAN_ENDPOINT` constant ([#5682](https://github.com/MetaMask/core/pull/5682))

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

Lines changed: 323 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { Messenger } from '@metamask/base-controller';
22
import { strict as assert } from 'assert';
3-
import nock from 'nock';
3+
import nock, { cleanAll, isDone, pendingMocks } from 'nock';
44
import sinon from 'sinon';
55

66
import {
@@ -56,6 +56,7 @@ function getPhishingController(options?: Partial<PhishingControllerOptions>) {
5656
describe('PhishingController', () => {
5757
afterEach(() => {
5858
sinon.restore();
59+
cleanAll();
5960
});
6061

6162
it('should have no default phishing lists', () => {
@@ -2156,7 +2157,7 @@ describe('PhishingController', () => {
21562157

21572158
describe('PhishingController - isBlockedRequest', () => {
21582159
afterEach(() => {
2159-
nock.cleanAll();
2160+
cleanAll();
21602161
});
21612162

21622163
it('should return false if c2DomainBlocklist is not defined or empty', async () => {
@@ -2871,3 +2872,323 @@ describe('PhishingController', () => {
28712872
});
28722873
});
28732874
});
2875+
2876+
describe('URL Scan Cache', () => {
2877+
let clock: sinon.SinonFakeTimers;
2878+
2879+
beforeEach(() => {
2880+
clock = sinon.useFakeTimers();
2881+
});
2882+
afterEach(() => {
2883+
sinon.restore();
2884+
cleanAll();
2885+
});
2886+
2887+
it('should cache scan results and return them on subsequent calls', async () => {
2888+
const testDomain = 'example.com';
2889+
2890+
// Spy on the fetch function to track calls
2891+
const fetchSpy = jest.spyOn(global, 'fetch');
2892+
2893+
nock(PHISHING_DETECTION_BASE_URL)
2894+
.get(
2895+
`/${PHISHING_DETECTION_SCAN_ENDPOINT}?url=${encodeURIComponent(testDomain)}`,
2896+
)
2897+
.reply(200, {
2898+
recommendedAction: RecommendedAction.None,
2899+
});
2900+
2901+
const controller = getPhishingController();
2902+
2903+
const result1 = await controller.scanUrl(`https://${testDomain}`);
2904+
expect(result1).toStrictEqual({
2905+
domainName: testDomain,
2906+
recommendedAction: RecommendedAction.None,
2907+
});
2908+
2909+
const result2 = await controller.scanUrl(`https://${testDomain}`);
2910+
expect(result2).toStrictEqual({
2911+
domainName: testDomain,
2912+
recommendedAction: RecommendedAction.None,
2913+
});
2914+
2915+
// Verify that fetch was called exactly once
2916+
expect(fetchSpy).toHaveBeenCalledTimes(1);
2917+
2918+
fetchSpy.mockRestore();
2919+
});
2920+
2921+
it('should expire cache entries after TTL', async () => {
2922+
const testDomain = 'example.com';
2923+
const cacheTTL = 300; // 5 minutes
2924+
2925+
nock(PHISHING_DETECTION_BASE_URL)
2926+
.get(
2927+
`/${PHISHING_DETECTION_SCAN_ENDPOINT}?url=${encodeURIComponent(testDomain)}`,
2928+
)
2929+
.reply(200, {
2930+
recommendedAction: RecommendedAction.None,
2931+
})
2932+
.get(
2933+
`/${PHISHING_DETECTION_SCAN_ENDPOINT}?url=${encodeURIComponent(testDomain)}`,
2934+
)
2935+
.reply(200, {
2936+
recommendedAction: RecommendedAction.None,
2937+
});
2938+
2939+
const controller = getPhishingController({
2940+
urlScanCacheTTL: cacheTTL,
2941+
});
2942+
2943+
await controller.scanUrl(`https://${testDomain}`);
2944+
2945+
// Before TTL expires, should use cache
2946+
clock.tick((cacheTTL - 10) * 1000);
2947+
await controller.scanUrl(`https://${testDomain}`);
2948+
expect(pendingMocks()).toHaveLength(1); // One mock remaining
2949+
2950+
// After TTL expires, should fetch again
2951+
clock.tick(11 * 1000);
2952+
await controller.scanUrl(`https://${testDomain}`);
2953+
expect(pendingMocks()).toHaveLength(0); // All mocks used
2954+
});
2955+
2956+
it('should evict oldest entries when cache exceeds max size', async () => {
2957+
const maxCacheSize = 2;
2958+
const domains = ['domain1.com', 'domain2.com', 'domain3.com'];
2959+
2960+
// Setup nock to respond to all three domains
2961+
domains.forEach((domain) => {
2962+
nock(PHISHING_DETECTION_BASE_URL)
2963+
.get(
2964+
`/${PHISHING_DETECTION_SCAN_ENDPOINT}?url=${encodeURIComponent(domain)}`,
2965+
)
2966+
.reply(200, {
2967+
recommendedAction: RecommendedAction.None,
2968+
});
2969+
});
2970+
2971+
// Setup a second request for the first domain
2972+
nock(PHISHING_DETECTION_BASE_URL)
2973+
.get(
2974+
`/${PHISHING_DETECTION_SCAN_ENDPOINT}?url=${encodeURIComponent(domains[0])}`,
2975+
)
2976+
.reply(200, {
2977+
recommendedAction: RecommendedAction.Warn,
2978+
});
2979+
2980+
const controller = getPhishingController({
2981+
urlScanCacheMaxSize: maxCacheSize,
2982+
});
2983+
2984+
// Fill the cache
2985+
await controller.scanUrl(`https://${domains[0]}`);
2986+
clock.tick(1000); // Ensure different timestamps
2987+
await controller.scanUrl(`https://${domains[1]}`);
2988+
2989+
// This should evict the oldest entry (domain1)
2990+
clock.tick(1000);
2991+
await controller.scanUrl(`https://${domains[2]}`);
2992+
2993+
// Now domain1 should not be in cache and require a new fetch
2994+
await controller.scanUrl(`https://${domains[0]}`);
2995+
2996+
// All mocks should be used
2997+
expect(isDone()).toBe(true);
2998+
});
2999+
3000+
it('should clear the cache when clearUrlScanCache is called', async () => {
3001+
const testDomain = 'example.com';
3002+
3003+
nock(PHISHING_DETECTION_BASE_URL)
3004+
.get(
3005+
`/${PHISHING_DETECTION_SCAN_ENDPOINT}?url=${encodeURIComponent(testDomain)}`,
3006+
)
3007+
.reply(200, {
3008+
recommendedAction: RecommendedAction.None,
3009+
})
3010+
.get(
3011+
`/${PHISHING_DETECTION_SCAN_ENDPOINT}?url=${encodeURIComponent(testDomain)}`,
3012+
)
3013+
.reply(200, {
3014+
recommendedAction: RecommendedAction.None,
3015+
});
3016+
3017+
const controller = getPhishingController();
3018+
3019+
// First call should fetch from API
3020+
await controller.scanUrl(`https://${testDomain}`);
3021+
3022+
// Clear the cache
3023+
controller.clearUrlScanCache();
3024+
3025+
// Should fetch again
3026+
await controller.scanUrl(`https://${testDomain}`);
3027+
3028+
// All mocks should be used
3029+
expect(isDone()).toBe(true);
3030+
});
3031+
3032+
it('should allow changing the TTL', async () => {
3033+
const testDomain = 'example.com';
3034+
const initialTTL = 300; // 5 minutes
3035+
const newTTL = 60; // 1 minute
3036+
3037+
nock(PHISHING_DETECTION_BASE_URL)
3038+
.get(
3039+
`/${PHISHING_DETECTION_SCAN_ENDPOINT}?url=${encodeURIComponent(testDomain)}`,
3040+
)
3041+
.reply(200, {
3042+
recommendedAction: RecommendedAction.None,
3043+
})
3044+
.get(
3045+
`/${PHISHING_DETECTION_SCAN_ENDPOINT}?url=${encodeURIComponent(testDomain)}`,
3046+
)
3047+
.reply(200, {
3048+
recommendedAction: RecommendedAction.None,
3049+
});
3050+
3051+
const controller = getPhishingController({
3052+
urlScanCacheTTL: initialTTL,
3053+
});
3054+
3055+
// First call should fetch from API
3056+
await controller.scanUrl(`https://${testDomain}`);
3057+
3058+
// Change TTL
3059+
controller.setUrlScanCacheTTL(newTTL);
3060+
3061+
// Before new TTL expires, should use cache
3062+
clock.tick((newTTL - 10) * 1000);
3063+
await controller.scanUrl(`https://${testDomain}`);
3064+
expect(pendingMocks()).toHaveLength(1); // One mock remaining
3065+
3066+
// After new TTL expires, should fetch again
3067+
clock.tick(11 * 1000);
3068+
await controller.scanUrl(`https://${testDomain}`);
3069+
expect(pendingMocks()).toHaveLength(0); // All mocks used
3070+
});
3071+
3072+
it('should allow changing the max cache size', async () => {
3073+
const initialMaxSize = 3;
3074+
const newMaxSize = 2;
3075+
const domains = [
3076+
'domain1.com',
3077+
'domain2.com',
3078+
'domain3.com',
3079+
'domain4.com',
3080+
];
3081+
3082+
// Setup nock to respond to all domains
3083+
domains.forEach((domain) => {
3084+
nock(PHISHING_DETECTION_BASE_URL)
3085+
.get(
3086+
`/${PHISHING_DETECTION_SCAN_ENDPOINT}?url=${encodeURIComponent(domain)}`,
3087+
)
3088+
.reply(200, {
3089+
recommendedAction: RecommendedAction.None,
3090+
});
3091+
});
3092+
3093+
const controller = getPhishingController({
3094+
urlScanCacheMaxSize: initialMaxSize,
3095+
});
3096+
3097+
// Fill the cache to initial size
3098+
await controller.scanUrl(`https://${domains[0]}`);
3099+
clock.tick(1000); // Ensure different timestamps
3100+
await controller.scanUrl(`https://${domains[1]}`);
3101+
clock.tick(1000);
3102+
await controller.scanUrl(`https://${domains[2]}`);
3103+
3104+
// Verify initial cache size
3105+
expect(Object.keys(controller.state.urlScanCache)).toHaveLength(
3106+
initialMaxSize,
3107+
);
3108+
// Reduce the max size
3109+
controller.setUrlScanCacheMaxSize(newMaxSize);
3110+
3111+
// Add another entry which should trigger eviction
3112+
await controller.scanUrl(`https://${domains[3]}`);
3113+
3114+
// Verify the cache size doesn't exceed new max size
3115+
expect(
3116+
Object.keys(controller.state.urlScanCache).length,
3117+
).toBeLessThanOrEqual(newMaxSize);
3118+
});
3119+
3120+
it('should handle fetch errors and not cache them', async () => {
3121+
const testDomain = 'example.com';
3122+
3123+
nock(PHISHING_DETECTION_BASE_URL)
3124+
.get(
3125+
`/${PHISHING_DETECTION_SCAN_ENDPOINT}?url=${encodeURIComponent(testDomain)}`,
3126+
)
3127+
.reply(500, { error: 'Internal Server Error' })
3128+
.get(
3129+
`/${PHISHING_DETECTION_SCAN_ENDPOINT}?url=${encodeURIComponent(testDomain)}`,
3130+
)
3131+
.reply(200, {
3132+
recommendedAction: RecommendedAction.None,
3133+
});
3134+
3135+
const controller = getPhishingController();
3136+
3137+
// First call should result in an error response
3138+
const result1 = await controller.scanUrl(`https://${testDomain}`);
3139+
expect(result1.fetchError).toBeDefined();
3140+
3141+
// Second call should try again (not use cache since errors aren't cached)
3142+
const result2 = await controller.scanUrl(`https://${testDomain}`);
3143+
expect(result2.fetchError).toBeUndefined();
3144+
expect(result2.recommendedAction).toBe(RecommendedAction.None);
3145+
3146+
// All mocks should be used
3147+
expect(isDone()).toBe(true);
3148+
});
3149+
3150+
it('should handle timeout errors and not cache them', async () => {
3151+
const testDomain = 'example.com';
3152+
3153+
// First mock a timeout/error response
3154+
nock(PHISHING_DETECTION_BASE_URL)
3155+
.get(
3156+
`/${PHISHING_DETECTION_SCAN_ENDPOINT}?url=${encodeURIComponent(testDomain)}`,
3157+
)
3158+
.replyWithError('connection timeout')
3159+
.get(
3160+
`/${PHISHING_DETECTION_SCAN_ENDPOINT}?url=${encodeURIComponent(testDomain)}`,
3161+
)
3162+
.reply(200, {
3163+
recommendedAction: RecommendedAction.None,
3164+
});
3165+
3166+
const controller = getPhishingController();
3167+
3168+
// First call should result in an error
3169+
const result1 = await controller.scanUrl(`https://${testDomain}`);
3170+
expect(result1.fetchError).toBeDefined();
3171+
3172+
// Second call should succeed (not use cache since errors aren't cached)
3173+
const result2 = await controller.scanUrl(`https://${testDomain}`);
3174+
expect(result2.fetchError).toBeUndefined();
3175+
expect(result2.recommendedAction).toBe(RecommendedAction.None);
3176+
3177+
// All mocks should be used
3178+
expect(isDone()).toBe(true);
3179+
});
3180+
3181+
it('should handle invalid URLs and not cache them', async () => {
3182+
const invalidUrl = 'not-a-valid-url';
3183+
3184+
const controller = getPhishingController();
3185+
3186+
// First call should return an error for invalid URL
3187+
const result1 = await controller.scanUrl(invalidUrl);
3188+
expect(result1.fetchError).toBeDefined();
3189+
3190+
// Second call should also return an error (not from cache)
3191+
const result2 = await controller.scanUrl(invalidUrl);
3192+
expect(result2.fetchError).toBeDefined();
3193+
});
3194+
});

0 commit comments

Comments
 (0)