diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index b9b41943b04..09ae9deb8f1 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -506,6 +506,7 @@ export default class MainBackground { // DIRT private phishingDataService: PhishingDataService; private phishingDetectionSettingsService: PhishingDetectionSettingsServiceAbstraction; + private phishingDetectionCleanup: (() => void) | null = null; constructor() { const logoutCallback = async (logoutReason: LogoutReason, userId?: UserId) => @@ -1515,7 +1516,12 @@ export default class MainBackground { this.stateProvider, ); - PhishingDetectionService.initialize( + // Call cleanup from previous initialization if it exists (service worker restart scenario) + if (this.phishingDetectionCleanup) { + this.phishingDetectionCleanup(); + } + + this.phishingDetectionCleanup = PhishingDetectionService.initialize( this.logService, this.phishingDataService, this.phishingDetectionSettingsService, @@ -1674,6 +1680,32 @@ 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 eba6b01fe90..e4d3c428802 100644 --- a/apps/browser/src/background/runtime.background.ts +++ b/apps/browser/src/background/runtime.background.ts @@ -433,6 +433,15 @@ 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 262d6cf833b..4cd155c8ae3 100644 --- a/apps/browser/src/dirt/phishing-detection/phishing-resources.ts +++ b/apps/browser/src/dirt/phishing-detection/phishing-resources.ts @@ -18,8 +18,7 @@ export const PHISHING_RESOURCES: Record { expect(result!.applicationVersion).toBe("2.0.0"); }); - it("only updates timestamp if checksum matches", async () => { + it("returns null if checksum matches (no update needed)", async () => { const prev: PhishingData = { webAddresses: ["a.com"], timestamp: Date.now() - 60000, @@ -122,9 +122,8 @@ describe("PhishingDataService", () => { }; fetchChecksumSpy.mockResolvedValue("abc"); const result = await service.getNextWebAddresses(prev); - expect(result!.webAddresses).toEqual(prev.webAddresses); - expect(result!.checksum).toBe("abc"); - expect(result!.timestamp).not.toBe(prev.timestamp); + // When checksum matches, return null to signal "skip state update" + expect(result).toBeNull(); }); 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 21fe74f1873..6f5e6dc63f3 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,13 +1,17 @@ import { catchError, + distinctUntilChanged, EMPTY, + filter, + finalize, first, firstValueFrom, - map, + from, + of, retry, - share, - startWith, + shareReplay, Subject, + Subscription, switchMap, tap, timer, @@ -18,7 +22,12 @@ 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 { GlobalStateProvider, KeyDefinition, PHISHING_DETECTION_DISK } from "@bitwarden/state"; +import { + GlobalState, + GlobalStateProvider, + KeyDefinition, + PHISHING_DETECTION_DISK, +} from "@bitwarden/state"; import { getPhishingResources, PhishingResourceType } from "../phishing-resources"; @@ -38,70 +47,31 @@ export const PHISHING_DOMAINS_KEY = new KeyDefinition( PHISHING_DETECTION_DISK, "phishingDomains", { - deserializer: (value: PhishingData) => - value ?? { webAddresses: [], timestamp: 0, checksum: "", applicationVersion: "" }, + deserializer: (value: PhishingData) => { + return 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 _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 - ), - ), - ), - ); + private _cachedPhishingDataStateInstance: GlobalState | null = null; - // 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(), - ); + /** + * 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; + } constructor( private apiService: ApiService, @@ -114,12 +84,182 @@ export class PhishingDataService { this.taskSchedulerService.registerTaskHandler(ScheduledTaskNames.phishingDomainUpdate, () => { this._triggerUpdate$.next(); }); - this.taskSchedulerService.setInterval( + + // 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( 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 * @@ -127,13 +267,16 @@ export class PhishingDataService { * @returns True if the URL is a known phishing web address, false otherwise */ async isPhishingWebAddress(url: URL): Promise { - // Use domain (hostname) matching for domain resources, and link matching for links resources + // 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}`); 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; } } @@ -144,44 +287,72 @@ 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.info(`[PhishingDataService] Cache age: ${prevAge}`); + + this.logService.debug( + `[PhishingDataService] Cache: ${prev.webAddresses?.length ?? 0} entries, age ${Math.round(prevAge / 1000 / 60)}min`, + ); const applicationVersion = await this.platformUtilsService.getApplicationVersion(); - // If checksum matches, return existing data with new timestamp & version + // STEP 1: Fetch the remote checksum (tiny file, ~32 bytes) const remoteChecksum = await this.fetchPhishingChecksum(this.resourceType); - if (remoteChecksum && prev.checksum === remoteChecksum) { - this.logService.info( - `[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 + // 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) { + if ( + isOneDayOldMax && + applicationVersion === prev.applicationVersion && + (prev.webAddresses?.length ?? 0) > 0 + ) { const webAddressesTodayUrl = getPhishingResources(this.resourceType)!.todayUrl; - const dailyWebAddresses: string[] = - await this.fetchPhishingWebAddresses(webAddressesTodayUrl); + const dailyWebAddresses = await this.fetchPhishingWebAddresses(webAddressesTodayUrl); this.logService.info( - `[PhishingDataService] ${dailyWebAddresses.length} new phishing web addresses added`, + `[PhishingDataService] Daily update: +${dailyWebAddresses.length} entries`, ); return { - webAddresses: prev.webAddresses.concat(dailyWebAddresses), + webAddresses: (prev.webAddresses ?? []).concat(dailyWebAddresses), checksum: remoteChecksum, timestamp, applicationVersion, }; } - // Approach 2: Fetch all web addresses + // Approach 2: Fetch entire list (cache is stale or empty) 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, @@ -190,23 +361,136 @@ 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}`); } - return response.text(); + const checksum = await response.text(); + return checksum.trim(); // MD5 checksums are 32 hex characters } - private async fetchPhishingWebAddresses(url: string) { + /** + * 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 { const response = await this.apiService.nativeFetch(new Request(url)); if (!response.ok) { throw new Error(`[PhishingDataService] Failed to fetch web addresses: ${response.status}`); } - return response.text().then((text) => text.split("\n")); + // 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; } private getTestWebAddresses() { @@ -218,7 +502,7 @@ export class PhishingDataService { const webAddresses = devFlagValue("testPhishingUrls") as unknown[]; if (webAddresses && webAddresses instanceof Array) { this.logService.debug( - "[PhishingDetectionService] Dev flag enabled for testing phishing detection. Adding test phishing web addresses:", + "[PhishingDataService] 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 06a37f12faa..cebdd4c9c73 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 { Observable, of } from "rxjs"; +import { EMPTY, 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,7 +16,9 @@ describe("PhishingDetectionService", () => { beforeEach(() => { logService = { info: jest.fn(), debug: jest.fn(), warning: jest.fn(), error: jest.fn() } as any; - phishingDataService = mock(); + phishingDataService = mock({ + update$: EMPTY, + }); 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 d90e872eef8..744540f9ec8 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,11 +1,14 @@ import { concatMap, + delay, distinctUntilChanged, EMPTY, filter, map, merge, + of, Subject, + Subscription, switchMap, tap, } from "rxjs"; @@ -43,6 +46,8 @@ 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, @@ -50,18 +55,34 @@ 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. Aborting."); - return; + 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 called. Checking prerequisites..."); - - BrowserApi.addListener(chrome.tabs.onUpdated, this._handleTabUpdated.bind(this)); + this._boundTabHandler = this._handleTabUpdated.bind(this) as ( + ...args: readonly unknown[] + ) => unknown; + BrowserApi.addListener(chrome.tabs.onUpdated, this._boundTabHandler); 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); @@ -87,7 +108,9 @@ export class PhishingDetectionService { prev.tabId === curr.tabId && prev.ignored === curr.ignored, ), - tap((event) => logService.debug(`[PhishingDetectionService] processing event:`, event)), + tap((event) => + logService.debug(`[PhishingDetectionService] Processing navigation event:`, event), + ), concatMap(async ({ tabId, url, ignored }) => { if (ignored) { // The next time this host is visited, block again @@ -113,23 +136,58 @@ 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"); - return merge( - phishingDataService.update$, - onContinueCommand$, - onTabUpdated$, - onCancelCommand$, - ); + // 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$); } }), ) @@ -137,16 +195,26 @@ 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; - // 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, - ); + if (this._boundTabHandler) { + BrowserApi.removeListener(chrome.tabs.onUpdated, this._boundTabHandler); + this._boundTabHandler = null; + } + + // Clear accumulated state to prevent memory leaks + this._ignoredHostnames.clear(); }; } 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 e6363b490cb..f9cb93d05b8 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,5 +1,6 @@ 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"; @@ -97,19 +98,32 @@ 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); - const resultDisabled = await firstValueFrom(service.enabled$); + // Wait for the next emission after state update + const resultDisabled = await firstValueFrom( + service.enabled$.pipe(filter((v) => v === false)), + ); expect(resultDisabled).toBe(false); await service.setEnabled(mockUserId, true); - const resultEnabled = await firstValueFrom(service.enabled$); + // Wait for the next emission after state update + const resultEnabled = await firstValueFrom(service.enabled$.pipe(filter((v) => v === true))); expect(resultEnabled).toBe(true); }); }); @@ -117,12 +131,21 @@ 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); - let result = await firstValueFrom(service.enabled$); + // Wait for the next emission after state update + let result = await firstValueFrom(service.enabled$.pipe(filter((v) => v === false))); expect(result).toBe(false); await service.setEnabled(mockUserId, true); - result = await firstValueFrom(service.enabled$); + // Wait for the next emission after state update + result = await firstValueFrom(service.enabled$.pipe(filter((v) => v === true))); 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 e30592b2f68..9927e099f24 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 } from "rxjs/operators"; +import { catchError, distinctUntilChanged, map, shareReplay, startWith } 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,7 +18,9 @@ const ENABLE_PHISHING_DETECTION = new UserKeyDefinition( PHISHING_DETECTION_DISK, "enablePhishingDetection", { - deserializer: (value: boolean) => value ?? true, // Default: enabled + deserializer: (value: boolean) => { + return value ?? true; + }, // Default: enabled clearOn: [], }, ); @@ -97,9 +99,11 @@ export class PhishingDetectionSettingsService implements PhishingDetectionSettin if (!account) { return of(false); } - return this.stateProvider.getUserState$(ENABLE_PHISHING_DETECTION, account.id); + return this.stateProvider.getUserState$(ENABLE_PHISHING_DETECTION, account.id).pipe( + startWith(true), // Default: enabled (matches deserializer default) + map((enabled) => enabled ?? true), + ); }), - map((enabled) => enabled ?? true), ); }