bitwarden/browser
1
0
Fork 0
mirror of https://github.com/bitwarden/browser synced 2026-01-21 11:53:34 +00:00
Code Issues Releases Wiki Activity

Revert "[PM-30319] [BLOCKER] phish cache freeze (#18157)" (#18245)

Browse Source 
This reverts commit fcc2844a16.
This commit is contained in:
Tom
2026-01-07 14:25:10 -05:00
committed by GitHub
parent 196db093b2
commit 5832065e96
9 changed files with 124 additions and 543 deletions
Download Patch File Download Diff File Expand all files Collapse all files

34
apps/browser/src/background/main.background.ts
View File

@@ -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
*/

9
apps/browser/src/background/runtime.background.ts
View File

@@ -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$))

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

@@ -18,7 +18,8 @@ export const PHISHING_RESOURCES: Record<PhishingResourceType, PhishingResource[]
[PhishingResourceType.Domains]: [
{
name: "Phishing.Database Domains",
remoteUrl: "https://phish.co.za/latest/phishing-domains-ACTIVE.txt",
remoteUrl:
"https://raw.githubusercontent.com/Phishing-Database/Phishing.Database/master/phishing-domains-ACTIVE.txt",
checksumUrl:
"https://raw.githubusercontent.com/Phishing-Database/checksums/refs/heads/master/phishing-domains-ACTIVE.txt.md5",
todayUrl:
@@ -45,7 +46,8 @@ export const PHISHING_RESOURCES: Record<PhishingResourceType, PhishingResource[]
[PhishingResourceType.Links]: [
{
name: "Phishing.Database Links",
remoteUrl: "https://phish.co.za/latest/phishing-links-ACTIVE.txt",
remoteUrl:
"https://raw.githubusercontent.com/Phishing-Database/Phishing.Database/master/phishing-links-ACTIVE.txt",
checksumUrl:
"https://raw.githubusercontent.com/Phishing-Database/checksums/refs/heads/master/phishing-links-ACTIVE.txt.md5",
todayUrl:

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

@@ -113,7 +113,7 @@ describe("PhishingDataService", () => {
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 () => {

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

@@ -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<PhishingData>(
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<PhishingData> | 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<void>();
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<string> | 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<typeof this.createWebAddresses$> | 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<void>();
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<typeof this.createUpdate$> | 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<boolean> {
// 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<PhishingData | null> {
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<string[]> {
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<Set<string>> {
const CHUNK_SIZE = 50000; // Process 50K entries per chunk (fast, fewer iterations)
const startTime = Date.now();
const set = new Set<string>();
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[];

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

@@ -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<PhishingDataService>({
update$: EMPTY,
});
phishingDataService = mock();
messageListener = mock<MessageListener>({
messages$(_commandDefinition) {
return new Observable();

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

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

31
libs/common/src/dirt/services/phishing-detection/phishing-detection-settings.service.spec.ts
View File

@@ -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);
});
});

12
libs/common/src/dirt/services/phishing-detection/phishing-detection-settings.service.ts
View File

@@ -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),
);
}