Add persistent storage for ignored phishing sites

apps/browser/src/background/main.background.ts

@@ -1473,6 +1473,7 @@ export default class MainBackground {
       this.logService,
       this.phishingDataService,
       messageListener,
+      this.globalStateProvider,
     );
 
     this.ipcContentScriptManagerService = new IpcContentScriptManagerService(this.configService);

apps/browser/src/dirt/phishing-detection/services/phishing-detection.service.spec.ts

@@ -1,14 +1,24 @@
 import { mock, MockProxy } from "jest-mock-extended";
-import { Observable, of } from "rxjs";
+import { firstValueFrom, Observable, of, skip, Subject } from "rxjs";
 
 import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
 import { BillingAccountProfileStateService } from "@bitwarden/common/billing/abstractions";
 import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service";
 import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
+import { FakeGlobalStateProvider } from "@bitwarden/common/spec";
 import { MessageListener } from "@bitwarden/messaging";
 
+import { BrowserApi } from "../../../platform/browser/browser-api";
+
 import { PhishingDataService } from "./phishing-data.service";
-import { PhishingDetectionService } from "./phishing-detection.service";
+import {
+  IGNORED_PHISHING_HOSTNAMES_KEY,
+  PHISHING_DETECTION_CANCEL_COMMAND,
+  PHISHING_DETECTION_CONTINUE_COMMAND,
+  PhishingDetectionService,
+} from "./phishing-detection.service";
+
+jest.mock("../../../platform/browser/browser-api");
 
 describe("PhishingDetectionService", () => {
   let accountService: AccountService;
@@ -17,33 +27,240 @@ describe("PhishingDetectionService", () => {
   let logService: LogService;
   let phishingDataService: MockProxy<PhishingDataService>;
   let messageListener: MockProxy<MessageListener>;
+  let globalStateProvider: FakeGlobalStateProvider;
+  let continueCommandSubject: Subject<{ tabId: number; url: string }>;
+  let cancelCommandSubject: Subject<{ tabId: number }>;
+
+  let cleanupFn: (() => void) | undefined;
 
   beforeEach(() => {
-    accountService = { getAccount$: jest.fn(() => of(null)) } as any;
-    billingAccountProfileStateService = {} as any;
-    configService = { getFeatureFlag$: jest.fn(() => of(false)) } as any;
+    // Mock a premium account with access to phishing detection
+    const mockAccount = { id: "test-user-id" };
+    accountService = {
+      getAccount$: jest.fn(() => of(mockAccount)),
+      activeAccount$: of(mockAccount),
+    } as any;
+    billingAccountProfileStateService = {
+      hasPremiumFromAnySource$: jest.fn(() => of(true)),
+    } as any;
+    configService = { getFeatureFlag$: jest.fn(() => of(true)) } as any;
     logService = { info: jest.fn(), debug: jest.fn(), warning: jest.fn(), error: jest.fn() } as any;
     phishingDataService = mock();
-    messageListener = mock<MessageListener>({
-      messages$(_commandDefinition) {
-        return new Observable();
-      },
+    phishingDataService.update$ = of(undefined);
+
+    continueCommandSubject = new Subject();
+    cancelCommandSubject = new Subject();
+
+    messageListener = mock<MessageListener>();
+    messageListener.messages$.mockImplementation((commandDefinition: any) => {
+      if (commandDefinition === PHISHING_DETECTION_CONTINUE_COMMAND) {
+        return continueCommandSubject.asObservable();
+      } else if (commandDefinition === PHISHING_DETECTION_CANCEL_COMMAND) {
+        return cancelCommandSubject.asObservable();
+      }
+      return new Observable();
     });
+
+    globalStateProvider = new FakeGlobalStateProvider();
+
+    jest.spyOn(BrowserApi, "addListener").mockImplementation(() => {});
+    jest.spyOn(BrowserApi, "removeListener").mockImplementation(() => {});
+    jest.spyOn(BrowserApi, "navigateTabToUrl").mockResolvedValue(undefined);
+    jest.spyOn(BrowserApi, "closeTab").mockResolvedValue(undefined);
+    jest.spyOn(BrowserApi, "getRuntimeURL").mockReturnValue("chrome-extension://test/");
+  });
+
+  afterEach(() => {
+    if (cleanupFn) {
+      cleanupFn();
+      cleanupFn = undefined;
+    }
+    (PhishingDetectionService as any)._didInit = false;
   });
 
   it("should initialize without errors", () => {
     expect(() => {
-      PhishingDetectionService.initialize(
+      cleanupFn = PhishingDetectionService.initialize(
         accountService,
         billingAccountProfileStateService,
         configService,
         logService,
         phishingDataService,
         messageListener,
+        globalStateProvider,
       );
     }).not.toThrow();
   });
 
+  describe("Persistent Storage", () => {
+    it("loads ignored hostnames from storage on initialization", async () => {
+      const state = globalStateProvider.get(IGNORED_PHISHING_HOSTNAMES_KEY);
+      await state.update(() => ["malicious.com", "phishing.net"]);
+
+      cleanupFn = PhishingDetectionService.initialize(
+        accountService,
+        billingAccountProfileStateService,
+        configService,
+        logService,
+        phishingDataService,
+        messageListener,
+        globalStateProvider,
+      );
+
+      await new Promise((resolve) => setTimeout(resolve, 0));
+
+      expect(logService.debug).toHaveBeenCalledWith(
+        expect.stringContaining("Loaded/updated 2 ignored hostnames"),
+      );
+    });
+
+    it("persists ignored hostname when user continues to phishing site", async () => {
+      const state = globalStateProvider.get(IGNORED_PHISHING_HOSTNAMES_KEY);
+
+      cleanupFn = PhishingDetectionService.initialize(
+        accountService,
+        billingAccountProfileStateService,
+        configService,
+        logService,
+        phishingDataService,
+        messageListener,
+        globalStateProvider,
+      );
+
+      await new Promise((resolve) => setTimeout(resolve, 0));
+
+      const stateUpdatePromise = firstValueFrom(state.state$.pipe(skip(1)));
+
+      continueCommandSubject.next({
+        tabId: 123,
+        url: "https://malicious.com/login",
+      });
+
+      const storedHostnames = await stateUpdatePromise;
+      expect(storedHostnames).toContain("malicious.com");
+    });
+
+    it("handles multiple ignored hostnames", async () => {
+      const state = globalStateProvider.get(IGNORED_PHISHING_HOSTNAMES_KEY);
+
+      cleanupFn = PhishingDetectionService.initialize(
+        accountService,
+        billingAccountProfileStateService,
+        configService,
+        logService,
+        phishingDataService,
+        messageListener,
+        globalStateProvider,
+      );
+
+      await new Promise((resolve) => setTimeout(resolve, 0));
+
+      const firstUpdatePromise = firstValueFrom(state.state$.pipe(skip(1)));
+      continueCommandSubject.next({
+        tabId: 123,
+        url: "https://malicious1.com/page",
+      });
+      await firstUpdatePromise;
+
+      const secondUpdatePromise = firstValueFrom(state.state$.pipe(skip(1)));
+      continueCommandSubject.next({
+        tabId: 124,
+        url: "https://malicious2.com/page",
+      });
+      const storedHostnames = await secondUpdatePromise;
+
+      expect(storedHostnames).toContain("malicious1.com");
+      expect(storedHostnames).toContain("malicious2.com");
+      expect((storedHostnames as string[]).length).toBe(2);
+    });
+
+    it("does not duplicate hostnames when same site is ignored multiple times", async () => {
+      const state = globalStateProvider.get(IGNORED_PHISHING_HOSTNAMES_KEY);
+
+      cleanupFn = PhishingDetectionService.initialize(
+        accountService,
+        billingAccountProfileStateService,
+        configService,
+        logService,
+        phishingDataService,
+        messageListener,
+        globalStateProvider,
+      );
+
+      await new Promise((resolve) => setTimeout(resolve, 0));
+
+      const firstUpdatePromise = firstValueFrom(state.state$.pipe(skip(1)));
+      continueCommandSubject.next({
+        tabId: 123,
+        url: "https://malicious.com/page1",
+      });
+      await firstUpdatePromise;
+
+      const secondUpdatePromise = firstValueFrom(state.state$.pipe(skip(1)));
+      continueCommandSubject.next({
+        tabId: 124,
+        url: "https://malicious.com/page2",
+      });
+      const storedHostnames = await secondUpdatePromise;
+
+      const hostnameArray = storedHostnames as string[];
+      const count = hostnameArray.filter((h) => h === "malicious.com").length;
+      expect(count).toBe(1);
+      expect(hostnameArray.length).toBe(1);
+    });
+
+    it("handles storage initialization with empty state", async () => {
+      const state = globalStateProvider.get(IGNORED_PHISHING_HOSTNAMES_KEY);
+
+      expect(() => {
+        cleanupFn = PhishingDetectionService.initialize(
+          accountService,
+          billingAccountProfileStateService,
+          configService,
+          logService,
+          phishingDataService,
+          messageListener,
+          globalStateProvider,
+        );
+      }).not.toThrow();
+
+      await new Promise((resolve) => setTimeout(resolve, 0));
+
+      const stateUpdatePromise = firstValueFrom(state.state$.pipe(skip(1)));
+      continueCommandSubject.next({
+        tabId: 123,
+        url: "https://test.com/page",
+      });
+
+      const storedHostnames = await stateUpdatePromise;
+      expect(storedHostnames).toContain("test.com");
+    });
+
+    it("syncs state updates from other contexts", async () => {
+      const state = globalStateProvider.get(IGNORED_PHISHING_HOSTNAMES_KEY);
+
+      cleanupFn = PhishingDetectionService.initialize(
+        accountService,
+        billingAccountProfileStateService,
+        configService,
+        logService,
+        phishingDataService,
+        messageListener,
+        globalStateProvider,
+      );
+
+      await new Promise((resolve) => setTimeout(resolve, 0));
+
+      await state.update(() => ["external.com", "updated.com"]);
+
+      await new Promise((resolve) => setTimeout(resolve, 0));
+
+      expect(logService.debug).toHaveBeenCalledWith(
+        expect.stringContaining("Loaded/updated 2 ignored hostnames"),
+      );
+    });
+  });
+
   // TODO
   // it("should enable phishing detection for premium account", (done) => {
   //   const premiumAccount = { id: "user1" };

apps/browser/src/dirt/phishing-detection/services/phishing-detection.service.ts

@@ -18,6 +18,7 @@ import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum";
 import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service";
 import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
 import { CommandDefinition, MessageListener } from "@bitwarden/messaging";
+import { GlobalStateProvider, KeyDefinition, PHISHING_DETECTION_DISK } from "@bitwarden/state";
 
 import { BrowserApi } from "../../../platform/browser/browser-api";
 
@@ -44,6 +45,17 @@ export const PHISHING_DETECTION_CANCEL_COMMAND = new CommandDefinition<{
   tabId: number;
 }>("phishing-detection-cancel");
 
+/**
+ * Key definition for storing hostnames that the user has chosen to ignore
+ */
+export const IGNORED_PHISHING_HOSTNAMES_KEY = new KeyDefinition<string[]>(
+  PHISHING_DETECTION_DISK,
+  "ignoredPhishingHostnames",
+  {
+    deserializer: (value: string[]) => value ?? [],
+  },
+);
+
 export class PhishingDetectionService {
   private static _tabUpdated$ = new Subject<PhishingDetectionNavigationEvent>();
   private static _ignoredHostnames = new Set<string>();
@@ -56,6 +68,7 @@ export class PhishingDetectionService {
     logService: LogService,
     phishingDataService: PhishingDataService,
     messageListener: MessageListener,
+    globalStateProvider: GlobalStateProvider,
   ) {
     if (this._didInit) {
       logService.debug("[PhishingDetectionService] Initialize already called. Aborting.");
@@ -64,16 +77,44 @@ export class PhishingDetectionService {
 
     logService.debug("[PhishingDetectionService] Initialize called. Checking prerequisites...");
 
+    const ignoredHostnamesState = globalStateProvider.get(IGNORED_PHISHING_HOSTNAMES_KEY);
+
+    // Subscribe to state changes (including initial state) to keep in-memory Set in sync
+    // This handles both the initial load and any future updates from other contexts
+    const ignoredHostnamesSub = ignoredHostnamesState.state$
+      .pipe(
+        map((hostnames) => hostnames ?? []),
+        tap((hostnames) => {
+          this._ignoredHostnames = new Set(hostnames);
+          logService.debug(
+            `[PhishingDetectionService] Loaded/updated ${hostnames.length} ignored hostnames`,
+          );
+        }),
+      )
+      .subscribe();
+
     BrowserApi.addListener(chrome.tabs.onUpdated, this._handleTabUpdated.bind(this));
 
     const onContinueCommand$ = messageListener.messages$(PHISHING_DETECTION_CONTINUE_COMMAND).pipe(
       tap((message) =>
         logService.debug(`[PhishingDetectionService] user selected continue for ${message.url}`),
       ),
       concatMap(async (message) => {
-        const url = new URL(message.url);
-        this._ignoredHostnames.add(url.hostname);
-        await BrowserApi.navigateTabToUrl(message.tabId, url);
+        try {
+          const url = new URL(message.url);
+          this._ignoredHostnames.add(url.hostname);
+          // Persist to storage
+          await ignoredHostnamesState.update(() => Array.from(this._ignoredHostnames));
+          logService.debug(
+            `[PhishingDetectionService] Added ${url.hostname} to ignored hostnames (persisted)`,
+          );
+          await BrowserApi.navigateTabToUrl(message.tabId, url);
+        } catch (error) {
+          logService.error(
+            `[PhishingDetectionService] Failed to process continue command for URL: ${message.url}`,
+            error,
+          );
+        }
       }),
     );
 
@@ -97,8 +138,10 @@ export class PhishingDetectionService {
       tap((event) => logService.debug(`[PhishingDetectionService] processing event:`, event)),
       concatMap(async ({ tabId, url, ignored }) => {
         if (ignored) {
-          // The next time this host is visited, block again
-          this._ignoredHostnames.delete(url.hostname);
+          // User has previously chosen to continue to this hostname, skip phishing check
+          logService.debug(
+            `[PhishingDetectionService] Skipping phishing check for ignored hostname: ${url.hostname}`,
+          );
           return;
         }
         const isPhishing = await phishingDataService.isPhishingDomain(url);
@@ -116,7 +159,7 @@ export class PhishingDetectionService {
 
     const onCancelCommand$ = messageListener
       .messages$(PHISHING_DETECTION_CANCEL_COMMAND)
-      .pipe(switchMap((message) => BrowserApi.closeTab(message.tabId)));
+      .pipe(concatMap(async (message) => await BrowserApi.closeTab(message.tabId)));
 
     const activeAccountHasAccess$ = combineLatest([
       accountService.activeAccount$,
@@ -158,6 +201,7 @@ export class PhishingDetectionService {
     this._didInit = true;
     return () => {
       initSub.unsubscribe();
+      ignoredHostnamesSub.unsubscribe();
       this._didInit = false;
 
       // Manually type cast to satisfy the listener signature due to the mixture