fix: phishing detector validation to drop invalid configs from detector

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

@@ -75,7 +75,7 @@ describe('PhishingDetector', () => {
         );
       });
 
-      [
+      it.each([
         undefined,
         null,
         true,
@@ -88,46 +88,116 @@ describe('PhishingDetector', () => {
           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'");
-        });
+      ])('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;
+
+        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;
+          },
+        );
+
+        // Ensure the detector is still defined
+        expect(detector).toBeDefined();
+
+        // Check that console.error was called (you can further specify the error if needed)
+        expect(console.error).toHaveBeenCalled();
+
+        // Restore the original console.error implementation
+        consoleErrorMock.mockRestore();
       });
 
-      it('throws an error when tolerance is provided without fuzzylist', async () => {
-        await expect(
-          withPhishingDetector(
-            [
-              // @ts-expect-error testing invalid input
-              {
-                allowlist: [],
-                blocklist: [],
-                name: 'first',
-                tolerance: 2,
-                version: 1,
-              },
-            ],
-            async () => null,
-          ),
-        ).rejects.toThrow('Fuzzylist tolerance provided without fuzzylist');
+      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.each([
         undefined,
         null,
         true,
@@ -137,26 +207,41 @@ describe('PhishingDetector', () => {
           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'");
-        });
-      });
+      ])(
+        '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;
+
+          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;
+            },
+          );
+
+          // 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', () => {

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

@@ -8,6 +8,7 @@ import {
   getHostnameFromUrl,
   matchPartsAgainstList,
   processConfigs,
+  // processConfigs,
   processDomainList,
   roundToNearestMinute,
   sha256Hash,
@@ -287,30 +288,151 @@ describe('domainToParts', () => {
 });
 
 describe('processConfigs', () => {
-  it('correctly converts a list of configs to a list of processed configs', () => {
+  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).toStrictEqual([
+    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: [['com', 'example']],
-        blocklist: [['com', 'example', 'sub']],
-        fuzzylist: [['com', 'example', 'fuzzy']],
+        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('can be called with no arguments', () => {
-    expect(processConfigs()).toStrictEqual([]);
+  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);
   });
 });