mirror of
https://github.com/bitwarden/browser
synced 2026-01-21 11:53:34 +00:00
[PM-30319] [BLOCKER] phish cache freeze (#18157)
* prevent redundant cache updates on account switch Remove automatic cache update triggering that caused UI freezes when switching to accounts with phishing detection access. Root cause: The update$ observable used startWith(undefined) which triggered an immediate cache refresh whenever a new subscription was created. On account switch, phishingDetectionSettingsService.on$ emits true, creating a new subscription and triggering a full ~800K entry fetch that blocks the UI thread. Fix: - Remove startWith(undefined) to prevent auto-triggering on subscription - Add MIN_UPDATE_INTERVAL (5 min) constant for cache freshness checks - Add _updateInProgress flag to prevent concurrent updates - Add filter() to skip updates when one is already in progress - Add cache freshness check (skip if updated within 5 minutes) - Add finalize() to reliably reset _updateInProgress flag (per ADR) - Replace share() with shareReplay() to prevent duplicate work - Add triggerUpdateIfNeeded() public method for explicit update requests The scheduled 24-hour update interval is unaffected - it still calls _triggerUpdate$.next() via the task scheduler. * trigger cache updates asynchronously on account switch Update PhishingDetectionService to explicitly trigger cache updates when phishing detection becomes active for an account, using a non-blocking pattern. Changes: - Add call to phishingDataService.triggerUpdateIfNeeded() when on$ emits true - Use of(null).pipe(delay(0)) to defer update to next event loop tick - This prevents the update from blocking the account switch UI flow The delay(0) pattern is preferred over setTimeout per codebase conventions (RxJS over native JS). The subscription auto-completes since of() emits once and completes, so no manual cleanup is needed. Combined with the previous commit's safeguards (cache freshness check, concurrent update prevention), this ensures: 1. Account switch completes immediately (non-blocking trigger) 2. Cache updates only run when actually needed (< 5 min freshness) 3. Concurrent updates are prevented (_updateInProgress flag) Fixes: PM-30319 * decouple cache update subscription from UI event merge Move phishingDataService.update$ to a separate subscription outside the merge() stream to prevent blocking the service worker during critical initialization and account switch flows. Background: The service worker is single-threaded. When the phishing cache update runs, it downloads a 25MB file and parses 800K entries using .split(), which is CPU-intensive synchronous work. During this parsing, the service worker cannot respond to popup requests, causing the extension UI to appear frozen when the user clicks the extension icon. Previously, update$ was included in the merge() alongside UI event handlers (onTabUpdated$, onContinueCommand$, onCancelCommand$). When on$ emitted true (user has phishing access), the merge subscription was created as part of the same synchronous flow, coupling the heavy cache work with the UI event setup. Changes: - Create separate updateSub subscription at initialization - Remove update$ from merge() - now only contains UI event streams - Keep delay(0) trigger for triggerUpdateIfNeeded() How delay(0) works: JavaScript's event loop must complete all synchronous code before processing async callbacks. delay(0) schedules the trigger for the next event loop tick, meaning: 1. initialize() completes and returns 2. Service worker is 'free' to handle other tasks 3. Next tick: triggerUpdateIfNeeded() fires 4. Cache update runs in background The cache parsing will still block the thread when it eventually runs, but this is now decoupled from the critical initialization path. The window where blocking can affect user interaction is minimized. PM-30319 * comment * account for new changes in spec file * prevent UI blocking during cache updates Problem: - Switching accounts caused 5+ second UI freeze - Even when data unchanged, 789K entries were rewritten to IndexedDB - Set was rebuilt from 789K entries on every state emission Solution: - Skip state update when checksum matches (return null instead of full data) - Cache Set in memory, only rebuild when checksum changes - Track last check time in memory instead of state - Use streaming fetch to prevent Firefox memory explosion - Add comprehensive logging for debugging Performance improvement: - Checksum match: ~5 seconds → ~10ms (no blocking) - Full update: Still required when data changes, but with streaming * pre-populate cache on install/update and optimize Set building Problem: Premium users experienced a 5+ second UI freeze on first login after install because the phishing list (~63MB, 789K entries) was downloaded synchronously when phishing detection was enabled. Solution: 1. Pre-populate cache on extension install/update - Added triggerPhishingCacheUpdate() to MainBackground - RuntimeBackground calls this on "install" and "update" events - Cache is ready before user logs in, eliminating first-login lag 2. Chunked Set building for UI responsiveness - Build Set in 50K-entry chunks with event loop yields - Changed from synchronous map() to async switchMap() + buildSetInChunks() - Prevents UI blocking when Set is rebuilt from cached data 3. Streaming with yields - Added yield after each network chunk during streaming fetch - Keeps service worker responsive to popup messages during download 4. Log cleanup for production - Converted verbose debugging logs from info → debug level - Kept important operational events (daily/full updates) at info - Removed timing logs and progress banners - Fixed comment accuracy: 100MB → 63MB uncompressed Performance impact: - First login after install: 5+ seconds → near-instant (cache pre-populated) - Set rebuild: non-blocking via chunked processing - Subsequent updates: already optimized via checksum matching * spec * add allowlist for bare amazon.com domain Problem: The upstream Phishing.Database contains a false positive entry `https://www.Amazon.com` (line 666495), causing the real Amazon website to be incorrectly blocked. Solution: Add BARE_DOMAIN_ALLOWLIST that skips blocking for exact hostname matches (amazon.com, www.amazon.com) when the URL has no path, query, or hash. This protects users from false positives while still detecting phishing URLs that use Amazon in paths or subdomain tricks. Allowed: - https://amazon.com - https://www.amazon.com Still blocked: - https://amazon.com/phishing/path - https://amazon.com-malicious.xyz - https://fake.com/amazon.com/steal * logging * update our links source url * Fix Chrome memory leak in phishing detection service * reduce memory leaks * optimize phishing detection performance and fix memory leaks This commit addresses critical performance issues and memory leaks in the phishing detection feature, particularly for non-premium users and during extension reloads. Storage Isolation: - Created BrowserIndexedDbStorageService for large data storage - Separated PHISHING_DATA_DISK (60MB+ phishing URLs) from PHISHING_DETECTION_DISK - Prevents popup from loading large dataset when accessing small settings - Fixed UI freeze when navigating to Settings -> Account security -> back arrow Lazy Loading Optimizations: - Converted _cachedState, _webAddresses$, and update$ to lazy getters - Only accesses IndexedDB when phishing detection is actually used - Prevents blocking service worker initialization on extension reload - Added guard in triggerUpdateIfNeeded() to skip if no observers Performance Improvements: - Modified buildEnabledPipeline$() to check available$ first - Uses startWith(true) to emit immediately, preventing on$ from blocking - Skips IndexedDB reads for non-premium users during unlock/account switch - Prevents 3+ second UI freezes for non-premium users Memory Leak Fixes: - Added static interval cleanup to prevent accumulation on service recreation - Fixed tab listener cleanup by storing bound handler reference - Fixed triggerUpdateSub subscription cleanup on account switches - Prevents exponential memory growth from undestroyed subscriptions Test Fixes: - Updated tests to set up available$ prerequisites before testing enabled$ - Fixed tests to wait for actual state values after startWith(true) emission - Uses filter() to wait for expected state values in async tests Files Changed: - apps/browser/src/platform/services/browser-indexed-db-storage.service.ts (new) - apps/browser/src/platform/storage/browser-storage-service.provider.ts - apps/browser/src/dirt/phishing-detection/services/phishing-data.service.ts - apps/browser/src/dirt/phishing-detection/services/phishing-detection.service.ts - apps/browser/src/background/runtime.background.ts - libs/common/src/dirt/services/phishing-detection/phishing-detection-settings.service.ts - libs/common/src/dirt/services/phishing-detection/phishing-detection-settings.service.spec.ts - libs/state/src/core/state-definitions.ts - libs/storage-core/src/client-locations.ts * fix test type errors * remove allowlist * storage isolation revert The initial implementation of storage isolation was used to fix a specific navigation scenario that lead to freezing of the ui ("Settings → Account Security" and clicking the back button) Why disk-large instead of memory-large-object: - **Problem**: Users experienced infinite loading (2+ minute freezes) when navigating to "Settings → Account Security" and clicking the back button. The Popup would freeze because `chrome.storage.local` broadcasts 60MB writes to all contexts, causing the Popup to deserialize data it never requested. - **Fix**: Created `disk-large` storage location using native IndexedDB, which persists data (unlike `memory-large-object`) and doesn't broadcast events (unlike `chrome.storage.local`), isolating large datasets from the Popup context. **Key Difference:** - `memory-large-object`: **Non-persistent** in-memory storage. Data is lost when the service worker restarts or the extension reloads. - `disk-large`: **Persistent** storage using native IndexedDB. Data survives service worker restarts and extension reloads. **Why We Need Persistence:** The phishing dataset (~60MB, 780K entries) must persist across: - Service worker restarts (Chrome terminates service workers after inactivity) - Extension reloads/updates - Browser restarts If we used `memory-large-object`, the extension would need to re-download the entire 60MB dataset every time the service worker restarts, which happens frequently in Chrome. This would: 1. Waste bandwidth (60MB downloads on every restart) 2. Cause UI freezes on every restart (same problem we're trying to fix) 3. Fail offline scenarios **Why Not Use Existing `disk` Location:** The existing `"disk"` location uses `chrome.storage.local`, which has a critical flaw for large datasets: - **Event Broadcasting**: Any write to `chrome.storage.local` triggers `onChanged` events broadcast to **all** extension contexts (Background, Popup, Sidebar) - **The UI/UX Problem**: - Users experienced **infinite loading** or **2+ minute freezes** when navigating to "Settings → Account Security" and clicking the back button - When Background writes 60MB, Chrome serializes and IPCs it to Popup, causing Popup's main thread to freeze while deserializing this massive object, even if Popup never requested the data - The Popup would become completely unresponsive, showing a spinning cursor or blank screen - **The Fix**: Native IndexedDB doesn't broadcast events across processes, isolating the storage so Background can write 60MB without disturbing the Popup * remove implementation comments from jsdoc * renaming * new domains source * remove unnecessary complexity from buildEnabledPipeline and remove all IndexedDB references * fix pre-population on install/update * handle null webAddresses --------- Co-authored-by: maxkpower <mpower@bitwarden.com>
This commit is contained in:
Alex
GitHub
@@ -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
|
||||
*/
|
||||
|
||||
@@ -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$))
|
||||
|
||||
@@ -18,8 +18,7 @@ export const PHISHING_RESOURCES: Record<PhishingResourceType, PhishingResource[]
|
||||
[PhishingResourceType.Domains]: [
|
||||
{
|
||||
name: "Phishing.Database Domains",
|
||||
remoteUrl:
|
||||
"https://raw.githubusercontent.com/Phishing-Database/Phishing.Database/master/phishing-domains-ACTIVE.txt",
|
||||
remoteUrl: "https://phish.co.za/latest/phishing-domains-ACTIVE.txt",
|
||||
checksumUrl:
|
||||
"https://raw.githubusercontent.com/Phishing-Database/checksums/refs/heads/master/phishing-domains-ACTIVE.txt.md5",
|
||||
todayUrl:
|
||||
@@ -46,8 +45,7 @@ export const PHISHING_RESOURCES: Record<PhishingResourceType, PhishingResource[]
|
||||
[PhishingResourceType.Links]: [
|
||||
{
|
||||
name: "Phishing.Database Links",
|
||||
remoteUrl:
|
||||
"https://raw.githubusercontent.com/Phishing-Database/Phishing.Database/master/phishing-links-ACTIVE.txt",
|
||||
remoteUrl: "https://phish.co.za/latest/phishing-links-ACTIVE.txt",
|
||||
checksumUrl:
|
||||
"https://raw.githubusercontent.com/Phishing-Database/checksums/refs/heads/master/phishing-links-ACTIVE.txt.md5",
|
||||
todayUrl:
|
||||
|
||||
@@ -113,7 +113,7 @@ describe("PhishingDataService", () => {
|
||||
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 () => {
|
||||
|
||||
@@ -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<PhishingData>(
|
||||
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<PhishingData> | 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<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(),
|
||||
);
|
||||
/**
|
||||
* 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<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
|
||||
*
|
||||
@@ -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<boolean> {
|
||||
// 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<PhishingData | null> {
|
||||
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<string[]> {
|
||||
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<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;
|
||||
}
|
||||
|
||||
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[];
|
||||
|
||||
@@ -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<PhishingDataService>({
|
||||
update$: EMPTY,
|
||||
});
|
||||
messageListener = mock<MessageListener>({
|
||||
messages$(_commandDefinition) {
|
||||
return new Observable();
|
||||
|
||||
@@ -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<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,
|
||||
@@ -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();
|
||||
};
|
||||
}
|
||||
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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),
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user