diff --git a/apps/browser/src/dirt/phishing-detection/phishing-resources.ts b/apps/browser/src/dirt/phishing-detection/phishing-resources.ts index 6595104207a..88068987dd7 100644 --- a/apps/browser/src/dirt/phishing-detection/phishing-resources.ts +++ b/apps/browser/src/dirt/phishing-detection/phishing-resources.ts @@ -7,8 +7,6 @@ export type PhishingResource = { todayUrl: string; /** Matcher used to decide whether a given URL matches an entry from this resource */ match: (url: URL, entry: string) => boolean; - /** Whether to use the custom matcher. If false, only exact hasUrl lookups are used. Default: true */ - useCustomMatcher?: boolean; }; export const PhishingResourceType = Object.freeze({ @@ -58,8 +56,6 @@ export const PHISHING_RESOURCES: Record { if (!entry) { return false; diff --git a/apps/browser/src/dirt/phishing-detection/services/phishing-data.service.spec.ts b/apps/browser/src/dirt/phishing-detection/services/phishing-data.service.spec.ts index 2d6c7a5a651..0cbb765ce0e 100644 --- a/apps/browser/src/dirt/phishing-detection/services/phishing-data.service.spec.ts +++ b/apps/browser/src/dirt/phishing-detection/services/phishing-data.service.spec.ts @@ -186,12 +186,74 @@ describe("PhishingDataService", () => { expect(result).toBe(false); expect(logService.error).toHaveBeenCalledWith( - "[PhishingDataService] IndexedDB lookup via hasUrl failed", + "[PhishingDataService] IndexedDB lookup failed", expect.any(Error), ); // Custom matcher is disabled, so no custom matcher error is expected expect(mockIndexedDbService.findMatchingUrl).not.toHaveBeenCalled(); }); + + it("should use cursor-based search when useCustomMatcher is enabled", async () => { + // Temporarily enable custom matcher for this test + const originalValue = (PhishingDataService as any).USE_CUSTOM_MATCHER; + (PhishingDataService as any).USE_CUSTOM_MATCHER = true; + + try { + // Mock hasUrl to return false (no direct match) + mockIndexedDbService.hasUrl.mockResolvedValue(false); + // Mock findMatchingUrl to return true (custom matcher finds it) + mockIndexedDbService.findMatchingUrl.mockResolvedValue(true); + + const url = new URL("http://phish.com/path"); + const result = await service.isPhishingWebAddress(url); + + expect(result).toBe(true); + expect(mockIndexedDbService.hasUrl).toHaveBeenCalled(); + expect(mockIndexedDbService.findMatchingUrl).toHaveBeenCalled(); + } finally { + // Restore original value + (PhishingDataService as any).USE_CUSTOM_MATCHER = originalValue; + } + }); + + it("should return false when custom matcher finds no match (when enabled)", async () => { + const originalValue = (PhishingDataService as any).USE_CUSTOM_MATCHER; + (PhishingDataService as any).USE_CUSTOM_MATCHER = true; + + try { + mockIndexedDbService.hasUrl.mockResolvedValue(false); + mockIndexedDbService.findMatchingUrl.mockResolvedValue(false); + + const url = new URL("http://safe.com/path"); + const result = await service.isPhishingWebAddress(url); + + expect(result).toBe(false); + expect(mockIndexedDbService.findMatchingUrl).toHaveBeenCalled(); + } finally { + (PhishingDataService as any).USE_CUSTOM_MATCHER = originalValue; + } + }); + + it("should handle custom matcher errors gracefully (when enabled)", async () => { + const originalValue = (PhishingDataService as any).USE_CUSTOM_MATCHER; + (PhishingDataService as any).USE_CUSTOM_MATCHER = true; + + try { + mockIndexedDbService.hasUrl.mockResolvedValue(false); + mockIndexedDbService.findMatchingUrl.mockRejectedValue(new Error("Cursor error")); + + const url = new URL("http://error.com/path"); + const result = await service.isPhishingWebAddress(url); + + expect(result).toBe(false); + expect(logService.error).toHaveBeenCalledWith( + "[PhishingDataService] Custom matcher failed", + expect.any(Error), + ); + } finally { + (PhishingDataService as any).USE_CUSTOM_MATCHER = originalValue; + } + }); }); describe("data updates", () => { 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 c34a94ecced..03759ba14bc 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 @@ -78,6 +78,10 @@ export const PHISHING_DOMAINS_BLOB_KEY = new KeyDefinition( /** Coordinates fetching, caching, and patching of known phishing web addresses */ export class PhishingDataService { + // Cursor-based search is disabled due to performance (6+ minutes on large databases) + // Enable when performance is optimized via indexing or other improvements + private static readonly USE_CUSTOM_MATCHER = false; + // While background scripts do not necessarily need destroying, // processes in PhishingDataService are memory intensive. // We are adding the destroy to guard against accidental leaks. @@ -153,12 +157,8 @@ export class PhishingDataService { * @returns True if the URL is a known phishing web address, false otherwise */ async isPhishingWebAddress(url: URL): Promise { - this.logService.debug("[PhishingDataService] isPhishingWebAddress called for: " + url.href); - // Skip non-http(s) protocols - phishing database only contains web URLs - // This prevents expensive fallback checks for chrome://, about:, file://, etc. if (url.protocol !== "http:" && url.protocol !== "https:") { - this.logService.debug("[PhishingDataService] Skipping non-http(s) protocol: " + url.protocol); return false; } @@ -176,69 +176,37 @@ export class PhishingDataService { const urlHref = url.href; const urlWithoutTrailingSlash = urlHref.endsWith("/") ? urlHref.slice(0, -1) : null; - this.logService.debug("[PhishingDataService] Checking hasUrl on this string: " + urlHref); let hasUrl = await this.indexedDbService.hasUrl(urlHref); - // If not found and URL has trailing slash, try without it if (!hasUrl && urlWithoutTrailingSlash) { - this.logService.debug( - "[PhishingDataService] Checking hasUrl without trailing slash: " + - urlWithoutTrailingSlash, - ); hasUrl = await this.indexedDbService.hasUrl(urlWithoutTrailingSlash); } if (hasUrl) { - this.logService.info( - "[PhishingDataService] Found phishing web address through direct lookup: " + urlHref, - ); + this.logService.info("[PhishingDataService] Found phishing URL: " + urlHref); return true; } } catch (err) { - this.logService.error("[PhishingDataService] IndexedDB lookup via hasUrl failed", err); + this.logService.error("[PhishingDataService] IndexedDB lookup failed", err); } - // If a custom matcher is provided and enabled, use cursor-based search. - // This avoids loading all URLs into memory and allows early exit on first match. - // Can be disabled via useCustomMatcher: false for performance reasons. - if (resource && resource.match && resource.useCustomMatcher !== false) { + // Custom matcher is disabled for performance (see USE_CUSTOM_MATCHER) + if (resource && resource.match && PhishingDataService.USE_CUSTOM_MATCHER) { try { - this.logService.debug( - "[PhishingDataService] Starting cursor-based search for: " + url.href, - ); - const startTime = performance.now(); - const found = await this.indexedDbService.findMatchingUrl((entry) => resource.match(url, entry), ); - const endTime = performance.now(); - const duration = (endTime - startTime).toFixed(2); - this.logService.debug( - `[PhishingDataService] Cursor-based search completed in ${duration}ms for: ${url.href} (found: ${found})`, - ); - if (found) { - this.logService.info( - "[PhishingDataService] Found phishing web address through custom matcher: " + url.href, - ); - } else { - this.logService.debug( - "[PhishingDataService] No match found, returning false for: " + url.href, - ); + this.logService.info("[PhishingDataService] Found phishing URL via matcher: " + url.href); } return found; } catch (err) { - this.logService.error("[PhishingDataService] Error running custom matcher", err); - this.logService.debug( - "[PhishingDataService] Returning false due to error for: " + url.href, - ); + this.logService.error("[PhishingDataService] Custom matcher failed", err); return false; } } - this.logService.debug( - "[PhishingDataService] No custom matcher, returning false for: " + url.href, - ); + return false; } 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 6ca5bad8942..2fa7bf8ec9e 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,4 @@ -import { - distinctUntilChanged, - EMPTY, - filter, - map, - merge, - mergeMap, - Subject, - switchMap, - tap, -} from "rxjs"; +import { distinctUntilChanged, EMPTY, filter, map, merge, Subject, switchMap, tap } from "rxjs"; import { PhishingDetectionSettingsServiceAbstraction } from "@bitwarden/common/dirt/services/abstractions/phishing-detection-settings.service.abstraction"; import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; @@ -43,7 +33,6 @@ export class PhishingDetectionService { private static _tabUpdated$ = new Subject(); private static _ignoredHostnames = new Set(); private static _didInit = false; - private static _activeSearchCount = 0; static initialize( logService: LogService, @@ -64,7 +53,7 @@ export class PhishingDetectionService { tap((message) => logService.debug(`[PhishingDetectionService] user selected continue for ${message.url}`), ), - mergeMap(async (message) => { + switchMap(async (message) => { const url = new URL(message.url); this._ignoredHostnames.add(url.hostname); await BrowserApi.navigateTabToUrl(message.tabId, url); @@ -89,40 +78,25 @@ export class PhishingDetectionService { prev.ignored === curr.ignored, ), tap((event) => logService.debug(`[PhishingDetectionService] processing event:`, event)), - // Use mergeMap for parallel processing - each tab check runs independently - // Concurrency limit of 5 prevents overwhelming IndexedDB - mergeMap(async ({ tabId, url, ignored }) => { - this._activeSearchCount++; - const searchId = `${tabId}-${Date.now()}`; - logService.debug( - `[PhishingDetectionService] Search STARTED [${searchId}] for ${url.href} (active: ${this._activeSearchCount}/5)`, - ); - const startTime = performance.now(); - - try { - if (ignored) { - // The next time this host is visited, block again - this._ignoredHostnames.delete(url.hostname); - return; - } - const isPhishing = await phishingDataService.isPhishingWebAddress(url); - if (!isPhishing) { - return; - } - - const phishingWarningPage = new URL( - BrowserApi.getRuntimeURL("popup/index.html#/security/phishing-warning") + - `?phishingUrl=${url.toString()}`, - ); - await BrowserApi.navigateTabToUrl(tabId, phishingWarningPage); - } finally { - this._activeSearchCount--; - const duration = (performance.now() - startTime).toFixed(2); - logService.debug( - `[PhishingDetectionService] Search FINISHED [${searchId}] for ${url.href} in ${duration}ms (active: ${this._activeSearchCount}/5)`, - ); + // Use switchMap to cancel any in-progress check when navigating to a new URL + // This prevents race conditions where a stale check redirects the user incorrectly + switchMap(async ({ tabId, url, ignored }) => { + if (ignored) { + // The next time this host is visited, block again + this._ignoredHostnames.delete(url.hostname); + return; } - }, 5), + const isPhishing = await phishingDataService.isPhishingWebAddress(url); + if (!isPhishing) { + return; + } + + const phishingWarningPage = new URL( + BrowserApi.getRuntimeURL("popup/index.html#/security/phishing-warning") + + `?phishingUrl=${url.toString()}`, + ); + await BrowserApi.navigateTabToUrl(tabId, phishingWarningPage); + }), ); const onCancelCommand$ = messageListener