From cb807344edf510783242edadd3c44a868ccd4f1f Mon Sep 17 00:00:00 2001 From: augmentedmode Date: Sun, 20 Oct 2024 23:49:33 -0400 Subject: [PATCH 1/3] fix: phishing detector validation to drop invalid configs from detector --- .../src/PhishingDetector.test.ts | 224 ++++++++++++------ .../phishing-controller/src/utils.test.ts | 56 ++--- packages/phishing-controller/src/utils.ts | 28 ++- 3 files changed, 201 insertions(+), 107 deletions(-) diff --git a/packages/phishing-controller/src/PhishingDetector.test.ts b/packages/phishing-controller/src/PhishingDetector.test.ts index 2684c5d6f7..4664ee27c8 100644 --- a/packages/phishing-controller/src/PhishingDetector.test.ts +++ b/packages/phishing-controller/src/PhishingDetector.test.ts @@ -75,87 +75,169 @@ describe('PhishingDetector', () => { ); }); - [ - undefined, - null, - true, - false, - 0, - 1, - 1.1, - '', - () => { - return { name: 'test', version: 1 }; - }, - {}, - ].forEach((mockInvalidName) => { - it('throws an error when config name is invalid', async () => { - await expect( - withPhishingDetector( - [ - { - allowlist: [], - blocklist: ['blocked-by-first.com'], - fuzzylist: [], - // @ts-expect-error testing invalid input - name: mockInvalidName, - tolerance: 2, - version: 1, - }, - ], - async () => mockInvalidName, - ), - ).rejects.toThrow("Invalid config parameter: 'name'"); - }); - }); + it('logs an error when config name is invalid for various invalid inputs', async () => { + const invalidNames = [ + undefined, + null, + true, + false, + 0, + 1, + 1.1, + '', + () => { + return { name: 'test', version: 1 }; + }, + {}, + ]; - it('throws an error when tolerance is provided without fuzzylist', async () => { - await expect( - withPhishingDetector( + for (const mockInvalidName of invalidNames) { + const consoleErrorMock = jest.spyOn(console, 'error'); + + let detector; + + await withPhishingDetector( [ - // @ts-expect-error testing invalid input { allowlist: [], - blocklist: [], - name: 'first', + blocklist: ['blocked-by-first.com'], + fuzzylist: [], + // @ts-expect-error testing invalid input + name: mockInvalidName, tolerance: 2, version: 1, }, ], - async () => null, - ), - ).rejects.toThrow('Fuzzylist tolerance provided without fuzzylist'); + async ({ detector: d }) => { + detector = d; + }, + ); + + expect(detector).toBeDefined(); + + expect(console.error).toHaveBeenCalled(); + + consoleErrorMock.mockRestore(); + } }); - [ - undefined, - null, - true, - false, - '', - () => { - return { name: 'test', version: 1 }; - }, - {}, - ].forEach((mockInvalidVersion) => { - it('throws an error when config version is invalid', async () => { - await expect( - withPhishingDetector( - [ - { - allowlist: [], - blocklist: ['blocked-by-first.com'], - fuzzylist: [], - name: 'first', - tolerance: 2, - // @ts-expect-error testing invalid input - version: mockInvalidVersion, - }, - ], - async () => null, - ), - ).rejects.toThrow("Invalid config parameter: 'version'"); - }); + it('drops the invalid config and retains the valid config', async () => { + const consoleErrorMock = jest.spyOn(console, 'error'); + + let detector: PhishingDetector | undefined; + + await withPhishingDetector( + [ + // Invalid config + { + allowlist: [], + blocklist: ['blocked-by-first.com'], + fuzzylist: [], + name: undefined, + tolerance: 2, + version: 1, + }, + // Valid config + { + allowlist: [], + blocklist: ['blocked-by-second.com'], + fuzzylist: [], + name: 'MetaMask', + tolerance: 2, + version: 1, + }, + ], + async ({ detector: d }) => { + detector = d; + }, + ); + + expect(detector).toBeDefined(); + + const result = detector?.check('https://blocked-by-second.com'); + + expect(result).toBeDefined(); + expect(result?.type).toBe('blocklist'); + + const resultInvalid = detector?.check('https://blocked-by-first.com'); + + expect(resultInvalid).toBeDefined(); + expect(resultInvalid?.type).toBe('all'); + + expect(console.error).toHaveBeenCalled(); + + consoleErrorMock.mockRestore(); + }); + + it('logs an error when tolerance is provided without fuzzylist', async () => { + const consoleErrorMock = jest.spyOn(console, 'error'); + + let detector; + + await withPhishingDetector( + [ + // @ts-expect-error testing invalid input + { + allowlist: [], + blocklist: [], + name: 'first', + tolerance: 2, + version: 1, + }, + ], + async ({ detector: d }) => { + detector = d; + }, + ); + + expect(detector).toBeDefined(); + + expect(console.error).toHaveBeenCalledWith(expect.any(Error)); + + consoleErrorMock.mockRestore(); + }); + + it('logs an error when config version is invalid for various invalid inputs', async () => { + const invalidVersions = [ + undefined, + null, + true, + false, + '', + () => { + return { name: 'test', version: 1 }; + }, + {}, + ]; + + for (const mockInvalidVersion of invalidVersions) { + const consoleErrorMock = jest.spyOn(console, 'error'); + + let detector; + + await withPhishingDetector( + [ + { + allowlist: [], + blocklist: ['blocked-by-first.com'], + fuzzylist: [], + name: 'first', + tolerance: 2, + // @ts-expect-error testing invalid input + version: mockInvalidVersion, + }, + ], + async ({ detector: d }) => { + detector = d; + }, + ); + + expect(detector).toBeDefined(); + + expect(console.error).toHaveBeenCalledWith(expect.any(Error)); + + consoleErrorMock.mockRestore(); + } }); }); diff --git a/packages/phishing-controller/src/utils.test.ts b/packages/phishing-controller/src/utils.test.ts index ac8f65d818..228ecf99ac 100644 --- a/packages/phishing-controller/src/utils.test.ts +++ b/packages/phishing-controller/src/utils.test.ts @@ -7,7 +7,7 @@ import { fetchTimeNow, getHostnameFromUrl, matchPartsAgainstList, - processConfigs, + // processConfigs, processDomainList, roundToNearestMinute, sha256Hash, @@ -286,33 +286,33 @@ describe('domainToParts', () => { }); }); -describe('processConfigs', () => { - it('correctly converts a list of configs to a list of processed configs', () => { - const configs = [ - { - allowlist: ['example.com'], - blocklist: ['sub.example.com'], - fuzzylist: ['fuzzy.example.com'], - tolerance: 2, - }, - ]; - - const result = processConfigs(configs); - - expect(result).toStrictEqual([ - { - allowlist: [['com', 'example']], - blocklist: [['com', 'example', 'sub']], - fuzzylist: [['com', 'example', 'fuzzy']], - tolerance: 2, - }, - ]); - }); - - it('can be called with no arguments', () => { - expect(processConfigs()).toStrictEqual([]); - }); -}); +// describe('processConfigs', () => { +// it('correctly converts a list of configs to a list of processed configs', () => { +// const configs = [ +// { +// allowlist: ['example.com'], +// blocklist: ['sub.example.com'], +// fuzzylist: ['fuzzy.example.com'], +// tolerance: 2, +// }, +// ]; + +// const result = processConfigs(configs); + +// expect(result).toStrictEqual([ +// { +// allowlist: [['com', 'example']], +// blocklist: [['com', 'example', 'sub']], +// fuzzylist: [['com', 'example', 'fuzzy']], +// tolerance: 2, +// }, +// ]); +// }); + +// it('can be called with no arguments', () => { +// expect(processConfigs()).toStrictEqual([]); +// }); +// }); describe('processDomainList', () => { it('correctly converts a list of domains to an array of parts', () => { diff --git a/packages/phishing-controller/src/utils.ts b/packages/phishing-controller/src/utils.ts index bffd8f60bd..157505f76c 100644 --- a/packages/phishing-controller/src/utils.ts +++ b/packages/phishing-controller/src/utils.ts @@ -200,16 +200,28 @@ export const getDefaultPhishingDetectorConfig = ({ }); /** - * Processes the configurations for the phishing detector. + * Processes the configurations for the phishing detector, filtering out any invalid configs. * - * @param configs - the configurations to process. - * @returns the processed configurations. + * @param configs - The configurations to process. + * @returns An array of processed and valid configurations. */ -export const processConfigs = (configs: PhishingDetectorList[] = []) => { - return configs.map((config: PhishingDetectorList) => { - validateConfig(config); - return { ...config, ...getDefaultPhishingDetectorConfig(config) }; - }); +export const processConfigs = ( + configs: PhishingDetectorList[] = [], +): PhishingDetectorConfiguration[] => { + return configs + .filter((config) => { + try { + validateConfig(config); + return true; + } catch (error) { + console.error(error); + return false; + } + }) + .map((config) => ({ + ...config, + ...getDefaultPhishingDetectorConfig(config), + })); }; /** From 6b713566f7c25c994dba593e50f85974ecd54265 Mon Sep 17 00:00:00 2001 From: augmentedmode Date: Mon, 21 Oct 2024 00:09:47 -0400 Subject: [PATCH 2/3] chore: add tests for processConfigs in utils --- .../phishing-controller/src/utils.test.ts | 176 +++++++++++++++--- 1 file changed, 149 insertions(+), 27 deletions(-) diff --git a/packages/phishing-controller/src/utils.test.ts b/packages/phishing-controller/src/utils.test.ts index 228ecf99ac..299db2f745 100644 --- a/packages/phishing-controller/src/utils.test.ts +++ b/packages/phishing-controller/src/utils.test.ts @@ -7,6 +7,7 @@ import { fetchTimeNow, getHostnameFromUrl, matchPartsAgainstList, + processConfigs, // processConfigs, processDomainList, roundToNearestMinute, @@ -286,33 +287,154 @@ describe('domainToParts', () => { }); }); -// describe('processConfigs', () => { -// it('correctly converts a list of configs to a list of processed configs', () => { -// const configs = [ -// { -// allowlist: ['example.com'], -// blocklist: ['sub.example.com'], -// fuzzylist: ['fuzzy.example.com'], -// tolerance: 2, -// }, -// ]; - -// const result = processConfigs(configs); - -// expect(result).toStrictEqual([ -// { -// allowlist: [['com', 'example']], -// blocklist: [['com', 'example', 'sub']], -// fuzzylist: [['com', 'example', 'fuzzy']], -// tolerance: 2, -// }, -// ]); -// }); - -// it('can be called with no arguments', () => { -// expect(processConfigs()).toStrictEqual([]); -// }); -// }); +describe('processConfigs', () => { + let consoleErrorMock: jest.SpyInstance; + + beforeEach(() => { + consoleErrorMock = jest.spyOn(console, 'error'); + }); + + afterEach(() => { + consoleErrorMock.mockRestore(); + }); + + it('correctly processes a list of valid configs', () => { + const configs = [ + { + allowlist: ['example.com'], + blocklist: ['sub.example.com'], + fuzzylist: ['fuzzy.example.com'], + tolerance: 2, + version: 1, + name: 'MetaMask', + }, + ]; + + const result = processConfigs(configs); + + expect(result).toHaveLength(1); + expect(result[0].name).toBe('MetaMask'); + + expect(console.error).not.toHaveBeenCalled(); + }); + + it('filters out invalid configs and logs errors', () => { + const configs = [ + { + allowlist: ['example.com'], + blocklist: ['sub.example.com'], + fuzzylist: [], + tolerance: 2, + version: 1, + name: 'MetaMask', + }, + { + allowlist: [], + version: 1, + name: undefined, + }, + ]; + + const result = processConfigs(configs); + + expect(result).toHaveLength(1); + expect(result[0].name).toBe('MetaMask'); + + expect(console.error).toHaveBeenCalledTimes(1); + }); + + it('returns an empty array when called with no arguments', () => { + const result = processConfigs(); + expect(result).toStrictEqual([]); + }); + + it('filters out invalid configs and logs errors with multiple configs', () => { + const configs = [ + { + allowlist: ['example.com'], + blocklist: ['sub.example.com'], + fuzzylist: [], + tolerance: 2, + version: 1, + name: 'MetaMask', + }, + { + allowlist: [], + version: 1, + name: undefined, + }, + { + allowlist: ['example.com'], + blocklist: ['sub.example.com'], + fuzzylist: [], + tolerance: 2, + version: 1, + name: 'name', + }, + { + allowlist: [], + version: 1, + name: '', + }, + ]; + + const result = processConfigs(configs); + + expect(result).toHaveLength(2); + expect(result[0].name).toBe('MetaMask'); + expect(result[1].name).toBe('name'); + + expect(console.error).toHaveBeenCalledTimes(2); + }); + + it('returns an empty array when all configs are invalid', () => { + const configs = [ + { + allowlist: [], + version: 1, + name: undefined, + }, + { + blocklist: [], + fuzzylist: [], + tolerance: 2, + version: null, + name: '', + }, + ]; + + // @ts-expect-error testing invalid input + const result = processConfigs(configs); + + expect(result).toStrictEqual([]); + + expect(console.error).toHaveBeenCalledTimes(2); + }); + + it('logs errors for invalid tolerance or version types', () => { + const configs = [ + { + allowlist: ['example.com'], + blocklist: ['sub.example.com'], + tolerance: 'invalid', + version: 1, + }, + { + allowlist: ['example.com'], + blocklist: ['sub.example.com'], + tolerance: 2, + version: {}, + }, + ]; + + // @ts-expect-error testing invalid input + const result = processConfigs(configs); + + expect(result).toStrictEqual([]); + + expect(console.error).toHaveBeenCalledTimes(2); + }); +}); describe('processDomainList', () => { it('correctly converts a list of domains to an array of parts', () => { From efe21ca43c1591c77e87493a6a00ef0bf69203fa Mon Sep 17 00:00:00 2001 From: augmentedmode Date: Thu, 24 Oct 2024 11:55:48 -0400 Subject: [PATCH 3/3] fix: tests to use it.each --- .../src/PhishingDetector.test.ts | 113 +++++++++--------- 1 file changed, 58 insertions(+), 55 deletions(-) diff --git a/packages/phishing-controller/src/PhishingDetector.test.ts b/packages/phishing-controller/src/PhishingDetector.test.ts index 4664ee27c8..0423cc4a59 100644 --- a/packages/phishing-controller/src/PhishingDetector.test.ts +++ b/packages/phishing-controller/src/PhishingDetector.test.ts @@ -75,50 +75,50 @@ describe('PhishingDetector', () => { ); }); - it('logs an error when config name is invalid for various invalid inputs', async () => { - const invalidNames = [ - undefined, - null, - true, - false, - 0, - 1, - 1.1, - '', - () => { - return { name: 'test', version: 1 }; - }, - {}, - ]; - - for (const mockInvalidName of invalidNames) { - const consoleErrorMock = jest.spyOn(console, 'error'); + it.each([ + undefined, + null, + true, + false, + 0, + 1, + 1.1, + '', + () => { + return { name: 'test', version: 1 }; + }, + {}, + ])('logs an error when config name is %p', async (mockInvalidName) => { + // Mock console.error to track error logs without cluttering test output + const consoleErrorMock = jest.spyOn(console, 'error'); - let detector; + let detector; - await withPhishingDetector( - [ - { - allowlist: [], - blocklist: ['blocked-by-first.com'], - fuzzylist: [], - // @ts-expect-error testing invalid input - name: mockInvalidName, - tolerance: 2, - version: 1, - }, - ], - async ({ detector: d }) => { - detector = d; + await withPhishingDetector( + [ + { + allowlist: [], + blocklist: ['blocked-by-first.com'], + fuzzylist: [], + // @ts-expect-error Testing invalid input + name: mockInvalidName, + tolerance: 2, + version: 1, }, - ); + ], + async ({ detector: d }) => { + detector = d; + }, + ); - expect(detector).toBeDefined(); + // Ensure the detector is still defined + expect(detector).toBeDefined(); - expect(console.error).toHaveBeenCalled(); + // Check that console.error was called (you can further specify the error if needed) + expect(console.error).toHaveBeenCalled(); - consoleErrorMock.mockRestore(); - } + // Restore the original console.error implementation + consoleErrorMock.mockRestore(); }); it('drops the invalid config and retains the valid config', async () => { @@ -197,20 +197,20 @@ describe('PhishingDetector', () => { consoleErrorMock.mockRestore(); }); - it('logs an error when config version is invalid for various invalid inputs', async () => { - const invalidVersions = [ - undefined, - null, - true, - false, - '', - () => { - return { name: 'test', version: 1 }; - }, - {}, - ]; - - for (const mockInvalidVersion of invalidVersions) { + it.each([ + undefined, + null, + true, + false, + '', + () => { + return { name: 'test', version: 1 }; + }, + {}, + ])( + 'logs an error when config version is %p', + async (mockInvalidVersion) => { + // Mock console.error to track error logs without cluttering test output const consoleErrorMock = jest.spyOn(console, 'error'); let detector; @@ -223,7 +223,7 @@ describe('PhishingDetector', () => { fuzzylist: [], name: 'first', tolerance: 2, - // @ts-expect-error testing invalid input + // @ts-expect-error Testing invalid input version: mockInvalidVersion, }, ], @@ -232,13 +232,16 @@ describe('PhishingDetector', () => { }, ); + // Ensure the detector is still defined expect(detector).toBeDefined(); + // Check that console.error was called with an error expect(console.error).toHaveBeenCalledWith(expect.any(Error)); + // Restore the original console.error implementation consoleErrorMock.mockRestore(); - } - }); + }, + ); }); describe('with legacy config', () => {