bitwarden/browser
1
0
Fork 0
mirror of https://github.com/bitwarden/browser synced 2026-01-30 08:13:44 +00:00
Code Issues Releases Wiki Activity

[PM-31348] phish cleanup - Address code review feedback from PR #18561 (Cursor-based phishing URL search) (#18638)

Browse Source
This commit is contained in:
Alex
2026-01-28 16:16:06 -05:00
committed by GitHub
parent fa5f62e1bd
commit 3a232c9296
4 changed files with 94 additions and 94 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
apps/browser/src/dirt/phishing-detection/phishing-resources.ts
View File

@@ -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<PhishingResourceType, PhishingResource[]
"https://raw.githubusercontent.com/Phishing-Database/checksums/refs/heads/master/phishing-links-ACTIVE.txt.md5",
todayUrl:
"https://raw.githubusercontent.com/Phishing-Database/Phishing.Database/refs/heads/master/phishing-links-NEW-today.txt",
// Disabled for performance - cursor search takes 6+ minutes on large databases
useCustomMatcher: false,
match: (url: URL, entry: string) => {
if (!entry) {
return false;

64
apps/browser/src/dirt/phishing-detection/services/phishing-data.service.spec.ts
View File

@@ -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", () => {

54
apps/browser/src/dirt/phishing-detection/services/phishing-data.service.ts
View File

@@ -78,6 +78,10 @@ export const PHISHING_DOMAINS_BLOB_KEY = new KeyDefinition<string>(
/** 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<boolean> {
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;
}

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

@@ -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<PhishingDetectionNavigationEvent>();
private static _ignoredHostnames = new Set<string>();
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