From 5832065e96c1396e170b397e5a1fa6a09dcbf67b Mon Sep 17 00:00:00 2001 From: Tom <144813356+ttalty@users.noreply.github.com> Date: Wed, 7 Jan 2026 14:25:10 -0500 Subject: [PATCH] Revert "[PM-30319] [BLOCKER] phish cache freeze (#18157)" (#18245) This reverts commit fcc2844a16d7cfb9c6b2ad263156b9d7e7cf46a1. --- .../browser/src/background/main.background.ts | 34 +- .../src/background/runtime.background.ts | 9 - .../phishing-detection/phishing-resources.ts | 6 +- .../services/phishing-data.service.spec.ts | 7 +- .../services/phishing-data.service.ts | 454 ++++-------------- .../phishing-detection.service.spec.ts | 6 +- .../services/phishing-detection.service.ts | 108 +---- ...hishing-detection-settings.service.spec.ts | 31 +- .../phishing-detection-settings.service.ts | 12 +- 9 files changed, 124 insertions(+), 543 deletions(-) diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index 09ae9deb8f1..b9b41943b04 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -506,7 +506,6 @@ export default class MainBackground { // DIRT private phishingDataService: PhishingDataService; private phishingDetectionSettingsService: PhishingDetectionSettingsServiceAbstraction; - private phishingDetectionCleanup: (() => void) | null = null; constructor() { const logoutCallback = async (logoutReason: LogoutReason, userId?: UserId) => @@ -1516,12 +1515,7 @@ export default class MainBackground { this.stateProvider, ); - // Call cleanup from previous initialization if it exists (service worker restart scenario) - if (this.phishingDetectionCleanup) { - this.phishingDetectionCleanup(); - } - - this.phishingDetectionCleanup = PhishingDetectionService.initialize( + PhishingDetectionService.initialize( this.logService, this.phishingDataService, this.phishingDetectionSettingsService, @@ -1680,32 +1674,6 @@ export default class MainBackground { } } - /** - * Triggers a phishing cache update in the background. - * Called on extension install/update to pre-populate the cache - * so it's ready when a premium user logs in. - * - * Creates a temporary subscription to ensure the update executes even if - * there are no other subscribers (install/update scenario). The subscription - * is automatically cleaned up after the update completes or errors. - */ - triggerPhishingCacheUpdate(): void { - // Create a temporary subscription to ensure the update executes - // since update$ uses shareReplay with refCount: true, which requires at least one subscriber - const tempSub = this.phishingDataService.update$.subscribe({ - next: () => { - this.logService.debug("[MainBackground] Phishing cache pre-population completed"); - tempSub.unsubscribe(); - }, - error: (err: unknown) => { - this.logService.error("[MainBackground] Phishing cache pre-population failed", err); - tempSub.unsubscribe(); - }, - }); - // Trigger the update after subscription is created - this.phishingDataService.triggerUpdateIfNeeded(); - } - /** * Switch accounts to indicated userId -- null is no active user */ diff --git a/apps/browser/src/background/runtime.background.ts b/apps/browser/src/background/runtime.background.ts index e4d3c428802..eba6b01fe90 100644 --- a/apps/browser/src/background/runtime.background.ts +++ b/apps/browser/src/background/runtime.background.ts @@ -433,15 +433,6 @@ export default class RuntimeBackground { void this.autofillService.loadAutofillScriptsOnInstall(); if (this.onInstalledReason != null) { - // Pre-populate phishing cache on install/update so it's ready when premium user logs in - // This runs in background and doesn't block the user - if (this.onInstalledReason === "install" || this.onInstalledReason === "update") { - this.logService.debug( - `[RuntimeBackground] Extension ${this.onInstalledReason}: triggering phishing cache pre-population`, - ); - this.main.triggerPhishingCacheUpdate(); - } - if ( this.onInstalledReason === "install" && !(await firstValueFrom(this.browserInitialInstallService.extensionInstalled$)) diff --git a/apps/browser/src/dirt/phishing-detection/phishing-resources.ts b/apps/browser/src/dirt/phishing-detection/phishing-resources.ts index 4cd155c8ae3..262d6cf833b 100644 --- a/apps/browser/src/dirt/phishing-detection/phishing-resources.ts +++ b/apps/browser/src/dirt/phishing-detection/phishing-resources.ts @@ -18,7 +18,8 @@ export const PHISHING_RESOURCES: Record { expect(result!.applicationVersion).toBe("2.0.0"); }); - it("returns null if checksum matches (no update needed)", async () => { + it("only updates timestamp if checksum matches", async () => { const prev: PhishingData = { webAddresses: ["a.com"], timestamp: Date.now() - 60000, @@ -122,8 +122,9 @@ describe("PhishingDataService", () => { }; fetchChecksumSpy.mockResolvedValue("abc"); const result = await service.getNextWebAddresses(prev); - // When checksum matches, return null to signal "skip state update" - expect(result).toBeNull(); + expect(result!.webAddresses).toEqual(prev.webAddresses); + expect(result!.checksum).toBe("abc"); + expect(result!.timestamp).not.toBe(prev.timestamp); }); it("patches daily domains if cache is fresh", async () => { diff --git a/apps/browser/src/dirt/phishing-detection/services/phishing-data.service.ts b/apps/browser/src/dirt/phishing-detection/services/phishing-data.service.ts index 6f5e6dc63f3..21fe74f1873 100644 --- a/apps/browser/src/dirt/phishing-detection/services/phishing-data.service.ts +++ b/apps/browser/src/dirt/phishing-detection/services/phishing-data.service.ts @@ -1,17 +1,13 @@ import { catchError, - distinctUntilChanged, EMPTY, - filter, - finalize, first, firstValueFrom, - from, - of, + map, retry, - shareReplay, + share, + startWith, Subject, - Subscription, switchMap, tap, timer, @@ -22,12 +18,7 @@ import { ApiService } from "@bitwarden/common/abstractions/api.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { ScheduledTaskNames, TaskSchedulerService } from "@bitwarden/common/platform/scheduling"; import { LogService } from "@bitwarden/logging"; -import { - GlobalState, - GlobalStateProvider, - KeyDefinition, - PHISHING_DETECTION_DISK, -} from "@bitwarden/state"; +import { GlobalStateProvider, KeyDefinition, PHISHING_DETECTION_DISK } from "@bitwarden/state"; import { getPhishingResources, PhishingResourceType } from "../phishing-resources"; @@ -47,31 +38,70 @@ export const PHISHING_DOMAINS_KEY = new KeyDefinition( PHISHING_DETECTION_DISK, "phishingDomains", { - deserializer: (value: PhishingData) => { - return value ?? { webAddresses: [], timestamp: 0, checksum: "", applicationVersion: "" }; - }, + deserializer: (value: PhishingData) => + value ?? { webAddresses: [], timestamp: 0, checksum: "", applicationVersion: "" }, }, ); /** Coordinates fetching, caching, and patching of known phishing web addresses */ export class PhishingDataService { - // Static tracking to prevent interval accumulation across service instances (reload scenario) - private static _intervalSubscription: Subscription | null = null; - private _testWebAddresses = this.getTestWebAddresses(); - private _cachedPhishingDataStateInstance: GlobalState | null = null; + private _cachedState = this.globalStateProvider.get(PHISHING_DOMAINS_KEY); + private _webAddresses$ = this._cachedState.state$.pipe( + map( + (state) => + new Set( + (state?.webAddresses?.filter((line) => line.trim().length > 0) ?? []).concat( + this._testWebAddresses, + "phishing.testcategory.com", // Included for QA to test in prod + ), + ), + ), + ); - /** - * Lazy getter for cached phishing data state. Only accesses storage when phishing detection is actually used. - * This prevents blocking service worker initialization on extension reload for non-premium users. - */ - private get _cachedPhishingDataState() { - if (this._cachedPhishingDataStateInstance === null) { - this.logService.debug("[PhishingDataService] Lazy-loading state from storage (first access)"); - this._cachedPhishingDataStateInstance = this.globalStateProvider.get(PHISHING_DOMAINS_KEY); - } - return this._cachedPhishingDataStateInstance; - } + // How often are new web addresses added to the remote? + readonly UPDATE_INTERVAL_DURATION = 24 * 60 * 60 * 1000; // 24 hours + + private _triggerUpdate$ = new Subject(); + update$ = this._triggerUpdate$.pipe( + startWith(undefined), // Always emit once + tap(() => this.logService.info(`[PhishingDataService] Update triggered...`)), + switchMap(() => + this._cachedState.state$.pipe( + first(), // Only take the first value to avoid an infinite loop when updating the cache below + switchMap(async (cachedState) => { + const next = await this.getNextWebAddresses(cachedState); + if (next) { + await this._cachedState.update(() => next); + this.logService.info(`[PhishingDataService] cache updated`); + } + }), + retry({ + count: 3, + delay: (err, count) => { + this.logService.error( + `[PhishingDataService] Unable to update web addresses. Attempt ${count}.`, + err, + ); + return timer(5 * 60 * 1000); // 5 minutes + }, + resetOnSuccess: true, + }), + catchError( + ( + err: unknown /** Eslint actually crashed if you remove this type: https://github.com/cartant/eslint-plugin-rxjs/issues/122 */, + ) => { + this.logService.error( + "[PhishingDataService] Retries unsuccessful. Unable to update web addresses.", + err, + ); + return EMPTY; + }, + ), + ), + ), + share(), + ); constructor( private apiService: ApiService, @@ -84,182 +114,12 @@ export class PhishingDataService { this.taskSchedulerService.registerTaskHandler(ScheduledTaskNames.phishingDomainUpdate, () => { this._triggerUpdate$.next(); }); - - // Clean up previous interval if it exists (prevents accumulation on service recreation) - if (PhishingDataService._intervalSubscription) { - PhishingDataService._intervalSubscription.unsubscribe(); - PhishingDataService._intervalSubscription = null; - } - // Store interval subscription statically to prevent accumulation on reload - PhishingDataService._intervalSubscription = this.taskSchedulerService.setInterval( + this.taskSchedulerService.setInterval( ScheduledTaskNames.phishingDomainUpdate, this.UPDATE_INTERVAL_DURATION, ); } - // In-memory cache to avoid expensive Set rebuilds and state rewrites - private _cachedWebAddressesSet: Set | null = null; - private _cachedSetChecksum: string = ""; - private _lastCheckTime: number = 0; // Track check time in memory, not state - - // Lazy observable: only subscribes to state$ when actually needed (first URL check) - // This prevents blocking service worker initialization on extension reload - // Using a getter with caching to defer access to _cachedPhishingDataState until actually subscribed - private _webAddresses$Instance: ReturnType | null = null; - private get _webAddresses$() { - if (this._webAddresses$Instance === null) { - this._webAddresses$Instance = this.createWebAddresses$(); - } - return this._webAddresses$Instance; - } - - private createWebAddresses$() { - return this._cachedPhishingDataState.state$.pipe( - // Only rebuild Set when checksum changes (actual data change) - distinctUntilChanged((prev, curr) => prev?.checksum === curr?.checksum), - switchMap((state) => { - // Return cached Set if checksum matches - if (this._cachedWebAddressesSet && state?.checksum === this._cachedSetChecksum) { - this.logService.debug( - `[PhishingDataService] Using cached Set (${this._cachedWebAddressesSet.size} entries, checksum: ${state?.checksum.substring(0, 8)}...)`, - ); - return of(this._cachedWebAddressesSet); - } - // Build Set in chunks to avoid blocking UI - this.logService.debug( - `[PhishingDataService] Building Set from ${state?.webAddresses?.length ?? 0} entries`, - ); - return from(this.buildSetInChunks(state?.webAddresses ?? [], state?.checksum ?? "")); - }), - shareReplay({ bufferSize: 1, refCount: true }), - ); - } - - // How often are new web addresses added to the remote? - readonly UPDATE_INTERVAL_DURATION = 24 * 60 * 60 * 1000; // 24 hours - - // Minimum time between updates when triggered by account switch (5 minutes) - private readonly MIN_UPDATE_INTERVAL = 5 * 60 * 1000; - - private _triggerUpdate$ = new Subject(); - private _updateInProgress = false; - - /** - * Observable that handles phishing data updates. - * - * Updates are triggered explicitly via triggerUpdateIfNeeded() or the 24-hour scheduler. - * The observable includes safeguards to prevent redundant updates: - * - Skips if an update is already in progress - * - Skips if cache was updated within MIN_UPDATE_INTERVAL (5 min) - * - * Lazy getter with caching: Only accesses _cachedPhishingDataState when actually subscribed to prevent storage read on reload. - */ - private _update$Instance: ReturnType | null = null; - get update$() { - if (this._update$Instance === null) { - this._update$Instance = this.createUpdate$(); - } - return this._update$Instance; - } - - private createUpdate$() { - return this._triggerUpdate$.pipe( - // Don't use startWith - initial update is handled by triggerUpdateIfNeeded() - filter(() => { - if (this._updateInProgress) { - this.logService.debug("[PhishingDataService] Update already in progress, skipping"); - return false; - } - return true; - }), - tap(() => { - this._updateInProgress = true; - }), - switchMap(async () => { - // Get current state directly without subscribing to state$ observable - // This avoids creating a subscription that stays active - const cachedState = await firstValueFrom( - this._cachedPhishingDataState.state$.pipe(first()), - ); - - // Early exit if we checked recently (using in-memory tracking) - const timeSinceLastCheck = Date.now() - this._lastCheckTime; - if (timeSinceLastCheck < this.MIN_UPDATE_INTERVAL) { - this.logService.debug( - `[PhishingDataService] Checked ${Math.round(timeSinceLastCheck / 1000)}s ago, skipping`, - ); - return; - } - - // Update last check time in memory (not state - avoids expensive write) - this._lastCheckTime = Date.now(); - - try { - const result = await this.getNextWebAddresses(cachedState); - - // result is null when checksum matched - skip state update entirely - if (result === null) { - this.logService.debug("[PhishingDataService] Checksum matched, skipping state update"); - return; - } - - if (result) { - // Yield to event loop before state update - await new Promise((resolve) => setTimeout(resolve, 0)); - await this._cachedPhishingDataState.update(() => result); - this.logService.info( - `[PhishingDataService] State updated with ${result.webAddresses?.length ?? 0} entries`, - ); - } - } catch (err) { - this.logService.error("[PhishingDataService] Unable to update web addresses.", err); - // Retry logic removed - let the 24-hour scheduler handle retries - throw err; - } - }), - retry({ - count: 3, - delay: (err, count) => { - this.logService.error( - `[PhishingDataService] Unable to update web addresses. Attempt ${count}.`, - err, - ); - return timer(5 * 60 * 1000); // 5 minutes - }, - resetOnSuccess: true, - }), - catchError( - ( - err: unknown /** Eslint actually crashed if you remove this type: https://github.com/cartant/eslint-plugin-rxjs/issues/122 */, - ) => { - this.logService.error( - "[PhishingDataService] Retries unsuccessful. Unable to update web addresses.", - err, - ); - return EMPTY; - }, - ), - // Use finalize() to ensure _updateInProgress is reset on success, error, OR completion - // Per ADR: "Use finalize() operator to ensure cleanup code always runs" - finalize(() => { - this._updateInProgress = false; - }), - shareReplay({ bufferSize: 1, refCount: true }), - ); - } - - /** - * Triggers an update if the cache is stale or empty. - * Should be called when phishing detection is enabled for an account or on install/update. - * - * The lazy loading of _cachedPhishingDataState ensures that storage is only accessed - * when the update$ observable chain actually executes (i.e., when there are subscribers). - * If there are no subscribers, the chain doesn't execute and no storage access occurs. - */ - triggerUpdateIfNeeded(): void { - this._triggerUpdate$.next(); - } - /** * Checks if the given URL is a known phishing web address * @@ -267,16 +127,13 @@ export class PhishingDataService { * @returns True if the URL is a known phishing web address, false otherwise */ async isPhishingWebAddress(url: URL): Promise { - // Lazy load: Only now do we subscribe to _webAddresses$ and trigger storage read + Set build - // This ensures we don't block service worker initialization on extension reload - this.logService.debug(`[PhishingDataService] Checking URL: ${url.href}`); + // Use domain (hostname) matching for domain resources, and link matching for links resources const entries = await firstValueFrom(this._webAddresses$); const resource = getPhishingResources(this.resourceType); if (resource && resource.match) { for (const entry of entries) { if (resource.match(url, entry)) { - this.logService.info(`[PhishingDataService] Match: ${url.href} matched entry: ${entry}`); return true; } } @@ -287,72 +144,44 @@ export class PhishingDataService { return entries.has(url.hostname); } - /** - * Determines if the phishing data needs to be updated and fetches new data if necessary. - * - * The CHECKSUM is an MD5 hash of the phishing list file, hosted at: - * For full url see: clients/apps/browser/src/dirt/phishing-detection/phishing-resources.ts - * - Links: https://raw.githubusercontent.com/Phishing-Database/checksums/.../phishing-links-ACTIVE.txt.md5 - * - Domains: https://raw.githubusercontent.com/Phishing-Database/checksums/.../phishing-domains-ACTIVE.txt.md5 - * - * PURPOSE: The checksum allows us to quickly check if the list has changed without - * downloading the entire file (~63MB uncompressed). If checksums match, data is identical. - * - * FLOW: - * 1. Fetch remote checksum (~62 bytes) - fast - * 2. Compare to local cached checksum - * 3. If match: return null (skip expensive state update) - * 4. If different: fetch new data and update state - * - * @returns PhishingData if data changed, null if checksum matched (no update needed) - */ async getNextWebAddresses(prev: PhishingData | null): Promise { prev = prev ?? { webAddresses: [], timestamp: 0, checksum: "", applicationVersion: "" }; const timestamp = Date.now(); const prevAge = timestamp - prev.timestamp; - - this.logService.debug( - `[PhishingDataService] Cache: ${prev.webAddresses?.length ?? 0} entries, age ${Math.round(prevAge / 1000 / 60)}min`, - ); + this.logService.info(`[PhishingDataService] Cache age: ${prevAge}`); const applicationVersion = await this.platformUtilsService.getApplicationVersion(); - // STEP 1: Fetch the remote checksum (tiny file, ~32 bytes) + // If checksum matches, return existing data with new timestamp & version const remoteChecksum = await this.fetchPhishingChecksum(this.resourceType); - - // STEP 2: Compare checksums if (remoteChecksum && prev.checksum === remoteChecksum) { - this.logService.debug("[PhishingDataService] Checksum match, no update needed"); - return null; // Signal to skip state update - no UI blocking! - } - - // STEP 3: Checksum different - data needs to be updated - this.logService.info("[PhishingDataService] Checksum mismatch, fetching new data"); - - // Approach 1: Fetch only today's new entries (if cache is less than 24h old) - const isOneDayOldMax = prevAge <= this.UPDATE_INTERVAL_DURATION; - if ( - isOneDayOldMax && - applicationVersion === prev.applicationVersion && - (prev.webAddresses?.length ?? 0) > 0 - ) { - const webAddressesTodayUrl = getPhishingResources(this.resourceType)!.todayUrl; - const dailyWebAddresses = await this.fetchPhishingWebAddresses(webAddressesTodayUrl); this.logService.info( - `[PhishingDataService] Daily update: +${dailyWebAddresses.length} entries`, + `[PhishingDataService] Remote checksum matches local checksum, updating timestamp only.`, + ); + return { ...prev, timestamp, applicationVersion }; + } + // Checksum is different, data needs to be updated. + + // Approach 1: Fetch only new web addresses and append + const isOneDayOldMax = prevAge <= this.UPDATE_INTERVAL_DURATION; + if (isOneDayOldMax && applicationVersion === prev.applicationVersion) { + const webAddressesTodayUrl = getPhishingResources(this.resourceType)!.todayUrl; + const dailyWebAddresses: string[] = + await this.fetchPhishingWebAddresses(webAddressesTodayUrl); + this.logService.info( + `[PhishingDataService] ${dailyWebAddresses.length} new phishing web addresses added`, ); return { - webAddresses: (prev.webAddresses ?? []).concat(dailyWebAddresses), + webAddresses: prev.webAddresses.concat(dailyWebAddresses), checksum: remoteChecksum, timestamp, applicationVersion, }; } - // Approach 2: Fetch entire list (cache is stale or empty) + // Approach 2: Fetch all web addresses const remoteUrl = getPhishingResources(this.resourceType)!.remoteUrl; const remoteWebAddresses = await this.fetchPhishingWebAddresses(remoteUrl); - this.logService.info(`[PhishingDataService] Full update: ${remoteWebAddresses.length} entries`); return { webAddresses: remoteWebAddresses, timestamp, @@ -361,136 +190,23 @@ export class PhishingDataService { }; } - /** - * Fetches the MD5 checksum of the phishing list from GitHub. - * The checksum file is tiny (~32 bytes) and fast to fetch. - * Used to detect if the phishing list has changed without downloading the full list. - */ private async fetchPhishingChecksum(type: PhishingResourceType = PhishingResourceType.Domains) { const checksumUrl = getPhishingResources(type)!.checksumUrl; - this.logService.debug(`[PhishingDataService] Fetching checksum from: ${checksumUrl}`); const response = await this.apiService.nativeFetch(new Request(checksumUrl)); if (!response.ok) { throw new Error(`[PhishingDataService] Failed to fetch checksum: ${response.status}`); } - const checksum = await response.text(); - return checksum.trim(); // MD5 checksums are 32 hex characters + return response.text(); } - /** - * Fetches phishing web addresses from the given URL. - * Uses streaming to avoid loading the entire file into memory at once, - * which can cause Firefox to freeze due to memory pressure. - */ - private async fetchPhishingWebAddresses(url: string): Promise { + private async fetchPhishingWebAddresses(url: string) { const response = await this.apiService.nativeFetch(new Request(url)); if (!response.ok) { throw new Error(`[PhishingDataService] Failed to fetch web addresses: ${response.status}`); } - // Stream the response to avoid loading entire file into memory at once - // This prevents Firefox from freezing on large phishing lists (~63MB uncompressed) - const reader = response.body?.getReader(); - if (!reader) { - // Fallback for environments without streaming support - this.logService.warning( - "[PhishingDataService] Streaming not available, falling back to full load", - ); - const text = await response.text(); - return text - .split(/\r?\n/) - .map((l) => l.trim()) - .filter((l) => l.length > 0); - } - - const decoder = new TextDecoder(); - const addresses: string[] = []; - let buffer = ""; - - try { - while (true) { - const { done, value } = await reader.read(); - if (done) { - break; - } - - buffer += decoder.decode(value, { stream: true }); - - // Process complete lines from buffer - const lines = buffer.split(/\r?\n/); - buffer = lines.pop() || ""; // Keep incomplete last line in buffer - - for (let i = 0; i < lines.length; i++) { - const trimmed = lines[i].trim(); - if (trimmed.length > 0) { - addresses.push(trimmed); - } - } - // Yield after processing each network chunk to keep service worker responsive - // This allows popup messages to be handled between chunks - await new Promise((resolve) => setTimeout(resolve, 0)); - } - - // Process any remaining buffer content - const remaining = buffer.trim(); - if (remaining.length > 0) { - addresses.push(remaining); - } - } finally { - // Ensure reader is released even if an error occurs - reader.releaseLock(); - } - - this.logService.debug(`[PhishingDataService] Streamed ${addresses.length} addresses`); - return addresses; - } - - /** - * Builds a Set from an array of web addresses in chunks to avoid blocking the UI. - * Yields to the event loop every CHUNK_SIZE entries, keeping the UI responsive - * even when processing 700K+ entries. - * - * @param addresses Array of web addresses to add to the Set - * @param checksum The checksum to associate with this cached Set - * @returns Promise that resolves to the built Set - */ - private async buildSetInChunks(addresses: string[], checksum: string): Promise> { - const CHUNK_SIZE = 50000; // Process 50K entries per chunk (fast, fewer iterations) - const startTime = Date.now(); - const set = new Set(); - - this.logService.debug(`[PhishingDataService] Building Set (${addresses.length} entries)`); - - for (let i = 0; i < addresses.length; i += CHUNK_SIZE) { - const chunk = addresses.slice(i, Math.min(i + CHUNK_SIZE, addresses.length)); - for (const addr of chunk) { - if (addr) { - // Skip empty strings - set.add(addr); - } - } - - // Yield to event loop after each chunk - if (i + CHUNK_SIZE < addresses.length) { - await new Promise((resolve) => setTimeout(resolve, 0)); - } - } - - // Add test addresses - this._testWebAddresses.forEach((addr) => set.add(addr)); - set.add("phishing.testcategory.com"); // For QA testing - - // Cache for future use - this._cachedWebAddressesSet = set; - this._cachedSetChecksum = checksum; - - const buildTime = Date.now() - startTime; - this.logService.debug( - `[PhishingDataService] Set built: ${set.size} entries in ${buildTime}ms (checksum: ${checksum.substring(0, 8)}...)`, - ); - - return set; + return response.text().then((text) => text.split("\n")); } private getTestWebAddresses() { @@ -502,7 +218,7 @@ export class PhishingDataService { const webAddresses = devFlagValue("testPhishingUrls") as unknown[]; if (webAddresses && webAddresses instanceof Array) { this.logService.debug( - "[PhishingDataService] Dev flag enabled for testing phishing detection. Adding test phishing web addresses:", + "[PhishingDetectionService] Dev flag enabled for testing phishing detection. Adding test phishing web addresses:", webAddresses, ); return webAddresses as string[]; diff --git a/apps/browser/src/dirt/phishing-detection/services/phishing-detection.service.spec.ts b/apps/browser/src/dirt/phishing-detection/services/phishing-detection.service.spec.ts index cebdd4c9c73..06a37f12faa 100644 --- a/apps/browser/src/dirt/phishing-detection/services/phishing-detection.service.spec.ts +++ b/apps/browser/src/dirt/phishing-detection/services/phishing-detection.service.spec.ts @@ -1,5 +1,5 @@ import { mock, MockProxy } from "jest-mock-extended"; -import { EMPTY, Observable, of } from "rxjs"; +import { Observable, of } from "rxjs"; import { PhishingDetectionSettingsServiceAbstraction } from "@bitwarden/common/dirt/services/abstractions/phishing-detection-settings.service.abstraction"; import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; @@ -16,9 +16,7 @@ describe("PhishingDetectionService", () => { beforeEach(() => { logService = { info: jest.fn(), debug: jest.fn(), warning: jest.fn(), error: jest.fn() } as any; - phishingDataService = mock({ - update$: EMPTY, - }); + phishingDataService = mock(); messageListener = mock({ messages$(_commandDefinition) { return new Observable(); diff --git a/apps/browser/src/dirt/phishing-detection/services/phishing-detection.service.ts b/apps/browser/src/dirt/phishing-detection/services/phishing-detection.service.ts index 744540f9ec8..d90e872eef8 100644 --- a/apps/browser/src/dirt/phishing-detection/services/phishing-detection.service.ts +++ b/apps/browser/src/dirt/phishing-detection/services/phishing-detection.service.ts @@ -1,14 +1,11 @@ import { concatMap, - delay, distinctUntilChanged, EMPTY, filter, map, merge, - of, Subject, - Subscription, switchMap, tap, } from "rxjs"; @@ -46,8 +43,6 @@ export class PhishingDetectionService { private static _tabUpdated$ = new Subject(); private static _ignoredHostnames = new Set(); private static _didInit = false; - private static _triggerUpdateSub: Subscription | null = null; - private static _boundTabHandler: ((...args: readonly unknown[]) => unknown) | null = null; static initialize( logService: LogService, @@ -55,34 +50,18 @@ export class PhishingDetectionService { phishingDetectionSettingsService: PhishingDetectionSettingsServiceAbstraction, messageListener: MessageListener, ) { - // If already initialized, clean up first to prevent memory leaks on service worker restart if (this._didInit) { - logService.debug( - "[PhishingDetectionService] Initialize already called. Cleaning up previous instance first.", - ); - // Clean up previous state - if (this._triggerUpdateSub) { - this._triggerUpdateSub.unsubscribe(); - this._triggerUpdateSub = null; - } - if (this._boundTabHandler) { - BrowserApi.removeListener(chrome.tabs.onUpdated, this._boundTabHandler); - this._boundTabHandler = null; - } - // Clear accumulated state - this._ignoredHostnames.clear(); - // Reset flag to allow re-initialization - this._didInit = false; + logService.debug("[PhishingDetectionService] Initialize already called. Aborting."); + return; } - this._boundTabHandler = this._handleTabUpdated.bind(this) as ( - ...args: readonly unknown[] - ) => unknown; - BrowserApi.addListener(chrome.tabs.onUpdated, this._boundTabHandler); + logService.debug("[PhishingDetectionService] Initialize called. Checking prerequisites..."); + + 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}`), + logService.debug(`[PhishingDetectionService] user selected continue for ${message.url}`), ), concatMap(async (message) => { const url = new URL(message.url); @@ -108,9 +87,7 @@ export class PhishingDetectionService { prev.tabId === curr.tabId && prev.ignored === curr.ignored, ), - tap((event) => - logService.debug(`[PhishingDetectionService] Processing navigation event:`, event), - ), + tap((event) => logService.debug(`[PhishingDetectionService] processing event:`, event)), concatMap(async ({ tabId, url, ignored }) => { if (ignored) { // The next time this host is visited, block again @@ -136,58 +113,23 @@ export class PhishingDetectionService { const phishingDetectionActive$ = phishingDetectionSettingsService.on$; - // CRITICAL: Only subscribe to update$ if phishing detection is available - // This prevents storage access for non-premium users on extension reload - // The subscription is created lazily when phishing detection becomes active - let updateSub: Subscription | null = null; - const initSub = phishingDetectionActive$ .pipe( distinctUntilChanged(), switchMap((activeUserHasAccess) => { - // Clean up previous trigger subscription if it exists - // This prevents memory leaks when account access changes (switch, lock/unlock) - if (this._triggerUpdateSub) { - this._triggerUpdateSub.unsubscribe(); - this._triggerUpdateSub = null; - } - if (!activeUserHasAccess) { logService.debug( "[PhishingDetectionService] User does not have access to phishing detection service.", ); - // Unsubscribe from update$ if user loses access (e.g., account switch to non-premium) - if (updateSub) { - updateSub.unsubscribe(); - updateSub = null; - } return EMPTY; } else { logService.debug("[PhishingDetectionService] Enabling phishing detection service"); - // Lazy subscription: Only subscribe to update$ when phishing detection becomes active - // This prevents storage access for non-premium users on extension reload - if (!updateSub) { - updateSub = phishingDataService.update$.subscribe({ - next: () => { - logService.debug("[PhishingDetectionService] Update completed"); - }, - error: (err: unknown) => { - logService.error("[PhishingDetectionService] Update error", err); - }, - complete: () => { - logService.debug("[PhishingDetectionService] Update subscription completed"); - }, - }); - } - // Trigger cache update asynchronously using RxJS delay(0) - // This defers to the next event loop tick, preventing UI blocking during account switch - // CRITICAL: Store subscription to prevent memory leaks on account switches - this._triggerUpdateSub = of(null) - .pipe(delay(0)) - .subscribe(() => phishingDataService.triggerUpdateIfNeeded()); - // update$ removed from merge - popup no longer blocks waiting for update - // The actual update runs via updateSub above - return merge(onContinueCommand$, onTabUpdated$, onCancelCommand$); + return merge( + phishingDataService.update$, + onContinueCommand$, + onTabUpdated$, + onCancelCommand$, + ); } }), ) @@ -195,26 +137,16 @@ export class PhishingDetectionService { this._didInit = true; return () => { - logService.debug("[PhishingDetectionService] Cleanup function called"); - if (updateSub) { - updateSub.unsubscribe(); - updateSub = null; - } initSub.unsubscribe(); - // Clean up trigger subscription to prevent memory leaks - if (this._triggerUpdateSub) { - this._triggerUpdateSub.unsubscribe(); - this._triggerUpdateSub = null; - } this._didInit = false; - if (this._boundTabHandler) { - BrowserApi.removeListener(chrome.tabs.onUpdated, this._boundTabHandler); - this._boundTabHandler = null; - } - - // Clear accumulated state to prevent memory leaks - this._ignoredHostnames.clear(); + // Manually type cast to satisfy the listener signature due to the mixture + // of static and instance methods in this class. To be fixed when refactoring + // this class to be instance-based while providing a singleton instance in usage + BrowserApi.removeListener( + chrome.tabs.onUpdated, + PhishingDetectionService._handleTabUpdated as (...args: readonly unknown[]) => unknown, + ); }; } diff --git a/libs/common/src/dirt/services/phishing-detection/phishing-detection-settings.service.spec.ts b/libs/common/src/dirt/services/phishing-detection/phishing-detection-settings.service.spec.ts index f9cb93d05b8..e6363b490cb 100644 --- a/libs/common/src/dirt/services/phishing-detection/phishing-detection-settings.service.spec.ts +++ b/libs/common/src/dirt/services/phishing-detection/phishing-detection-settings.service.spec.ts @@ -1,6 +1,5 @@ import { mock, MockProxy } from "jest-mock-extended"; import { BehaviorSubject, firstValueFrom, Subject } from "rxjs"; -import { filter } from "rxjs/operators"; import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; @@ -98,32 +97,19 @@ describe("PhishingDetectionSettingsService", () => { describe("enabled$", () => { it("should default to true if an account is logged in", async () => { activeAccountSubject.next(account); - featureFlagSubject.next(true); - premiumStatusSubject.next(true); - organizationsSubject.next([]); const result = await firstValueFrom(service.enabled$); expect(result).toBe(true); }); it("should return the stored value", async () => { activeAccountSubject.next(account); - featureFlagSubject.next(true); - premiumStatusSubject.next(true); - organizationsSubject.next([]); - - // Wait for initial emission (startWith(true)) - await firstValueFrom(service.enabled$); await service.setEnabled(mockUserId, false); - // Wait for the next emission after state update - const resultDisabled = await firstValueFrom( - service.enabled$.pipe(filter((v) => v === false)), - ); + const resultDisabled = await firstValueFrom(service.enabled$); expect(resultDisabled).toBe(false); await service.setEnabled(mockUserId, true); - // Wait for the next emission after state update - const resultEnabled = await firstValueFrom(service.enabled$.pipe(filter((v) => v === true))); + const resultEnabled = await firstValueFrom(service.enabled$); expect(resultEnabled).toBe(true); }); }); @@ -131,21 +117,12 @@ describe("PhishingDetectionSettingsService", () => { describe("setEnabled", () => { it("should update the stored value", async () => { activeAccountSubject.next(account); - featureFlagSubject.next(true); - premiumStatusSubject.next(true); - organizationsSubject.next([]); - - // Wait for initial emission (startWith(true)) - await firstValueFrom(service.enabled$); - await service.setEnabled(mockUserId, false); - // Wait for the next emission after state update - let result = await firstValueFrom(service.enabled$.pipe(filter((v) => v === false))); + let result = await firstValueFrom(service.enabled$); expect(result).toBe(false); await service.setEnabled(mockUserId, true); - // Wait for the next emission after state update - result = await firstValueFrom(service.enabled$.pipe(filter((v) => v === true))); + result = await firstValueFrom(service.enabled$); expect(result).toBe(true); }); }); diff --git a/libs/common/src/dirt/services/phishing-detection/phishing-detection-settings.service.ts b/libs/common/src/dirt/services/phishing-detection/phishing-detection-settings.service.ts index 9927e099f24..e30592b2f68 100644 --- a/libs/common/src/dirt/services/phishing-detection/phishing-detection-settings.service.ts +++ b/libs/common/src/dirt/services/phishing-detection/phishing-detection-settings.service.ts @@ -1,5 +1,5 @@ import { combineLatest, Observable, of, switchMap } from "rxjs"; -import { catchError, distinctUntilChanged, map, shareReplay, startWith } from "rxjs/operators"; +import { catchError, distinctUntilChanged, map, shareReplay } from "rxjs/operators"; import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; @@ -18,9 +18,7 @@ const ENABLE_PHISHING_DETECTION = new UserKeyDefinition( PHISHING_DETECTION_DISK, "enablePhishingDetection", { - deserializer: (value: boolean) => { - return value ?? true; - }, // Default: enabled + deserializer: (value: boolean) => value ?? true, // Default: enabled clearOn: [], }, ); @@ -99,11 +97,9 @@ export class PhishingDetectionSettingsService implements PhishingDetectionSettin if (!account) { return of(false); } - return this.stateProvider.getUserState$(ENABLE_PHISHING_DETECTION, account.id).pipe( - startWith(true), // Default: enabled (matches deserializer default) - map((enabled) => enabled ?? true), - ); + return this.stateProvider.getUserState$(ENABLE_PHISHING_DETECTION, account.id); }), + map((enabled) => enabled ?? true), ); }