Skip to content

Add persistent storage for ignored phishing sites #17463

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+280 −20
Open

Add persistent storage for ignored phishing sites #17463

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6f6d62c
Add persistent storage for ignored phishing sites
maxkpower Nov 18, 2025
836fb50
resolve merge issues
maxkpower Nov 19, 2025
6b51714
fix race conditions, added tests
maxkpower Nov 20, 2025
23bbfa0
Merge branch 'main' into PM-28391-persistent-ignore-phishing-sites
maxkpower Nov 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions apps/browser/src/background/main.background.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1473,6 +1473,7 @@ export default class MainBackground {
this.logService,
this.phishingDataService,
messageListener,
this.globalStateProvider,
);

this.ipcContentScriptManagerService = new IpcContentScriptManagerService(this.configService);
Expand Down
4 changes: 4 additions & 0 deletions apps/browser/src/dirt/phishing-detection/services/phishing-detection.service.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { AccountService } from "@bitwarden/common/auth/abstractions/account.serv
import { BillingAccountProfileStateService } from "@bitwarden/common/billing/abstractions";
import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service";
import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
import { FakeGlobalStateProvider } from "@bitwarden/common/spec";
import { MessageListener } from "@bitwarden/messaging";

import { PhishingDataService } from "./phishing-data.service";
Expand All @@ -17,6 +18,7 @@ describe("PhishingDetectionService", () => {
let logService: LogService;
let phishingDataService: MockProxy<PhishingDataService>;
let messageListener: MockProxy<MessageListener>;
let globalStateProvider: FakeGlobalStateProvider;

beforeEach(() => {
accountService = { getAccount$: jest.fn(() => of(null)) } as any;
Expand All @@ -29,6 +31,7 @@ describe("PhishingDetectionService", () => {
return new Observable();
},
});
globalStateProvider = new FakeGlobalStateProvider();
});

it("should initialize without errors", () => {
Expand All @@ -40,6 +43,7 @@ describe("PhishingDetectionService", () => {
logService,
phishingDataService,
messageListener,
globalStateProvider,
);
}).not.toThrow();
});
Copy link

@claude claude bot Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Insufficient test coverage (30.43%): This test file only verifies that initialization doesn't throw. Critical functionality is completely untested:

  • Loading ignored hostnames from storage on init
  • Persisting hostnames when user clicks "Continue"
  • Navigation to ignored sites (should skip phishing check)
  • Error handling during storage operations
  • Cleanup/unsubscribe behavior

Please add tests covering the main user flows and error scenarios.

Expand Down
59 changes: 54 additions & 5 deletions apps/browser/src/dirt/phishing-detection/services/phishing-detection.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
distinctUntilChanged,
EMPTY,
filter,
firstValueFrom,
map,
merge,
of,
Expand All @@ -18,6 +19,7 @@ import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum";
import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service";
import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
import { CommandDefinition, MessageListener } from "@bitwarden/messaging";
import { GlobalStateProvider, KeyDefinition, PHISHING_DETECTION_DISK } from "@bitwarden/state";

import { BrowserApi } from "../../../platform/browser/browser-api";

Expand All @@ -44,6 +46,17 @@ export const PHISHING_DETECTION_CANCEL_COMMAND = new CommandDefinition<{
tabId: number;
}>("phishing-detection-cancel");

/**
* Key definition for storing hostnames that the user has chosen to ignore
*/
export const IGNORED_PHISHING_HOSTNAMES_KEY = new KeyDefinition<string[]>(
PHISHING_DETECTION_DISK,
"ignoredPhishingHostnames",
{
deserializer: (value: string[]) => value ?? [],
},
);
Comment on lines +48 to +57
Copy link

@claude claude bot Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Missing documentation: Please add a comment explaining:

  • This is global state (shared across all accounts)
  • Data persists across browser restarts
  • Not synced across devices
Suggested change
/**
* Key definition for storing hostnames that the user has chosen to ignore
*/
export const IGNORED_PHISHING_HOSTNAMES_KEY = new KeyDefinition<string[]>(
PHISHING_DETECTION_DISK,
"ignoredPhishingHostnames",
{
deserializer: (value: string[]) => value ?? [],
},
);
/**
* Key definition for storing hostnames that the user has chosen to ignore.
* This is global state (shared across all accounts) stored in disk storage.
* Data persists across browser restarts but is not synced across devices.
*/
export const IGNORED_PHISHING_HOSTNAMES_KEY = new KeyDefinition<string[]>(
PHISHING_DETECTION_DISK,
"ignoredPhishingHostnames",
{
deserializer: (value: string[]) => value ?? [],
},
);

Comment on lines +51 to +57
Copy link
Contributor

@coroiu coroiu Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion(non-blocking): you can also use the array helper

Suggested change
export const IGNORED_PHISHING_HOSTNAMES_KEY = new KeyDefinition<string[]>(
PHISHING_DETECTION_DISK,
"ignoredPhishingHostnames",
{
deserializer: (value: string[]) => value ?? [],
},
);
export const IGNORED_PHISHING_HOSTNAMES_KEY = KeyDefinition.array<string>(
PHISHING_DETECTION_DISK,
"ignoredPhishingHostnames",
{
deserializer: (value: string) => value,
},
);


export class PhishingDetectionService {
private static _tabUpdated$ = new Subject<PhishingDetectionNavigationEvent>();
private static _ignoredHostnames = new Set<string>();
Expand All @@ -56,6 +69,7 @@ export class PhishingDetectionService {
logService: LogService,
phishingDataService: PhishingDataService,
messageListener: MessageListener,
globalStateProvider: GlobalStateProvider,
) {
if (this._didInit) {
logService.debug("[PhishingDetectionService] Initialize already called. Aborting.");
Expand All @@ -64,16 +78,48 @@ export class PhishingDetectionService {

logService.debug("[PhishingDetectionService] Initialize called. Checking prerequisites...");

// Load previously ignored hostnames from storage (synchronously to avoid race conditions)
const ignoredHostnamesState = globalStateProvider.get(IGNORED_PHISHING_HOSTNAMES_KEY);
firstValueFrom(ignoredHostnamesState.state$.pipe(map((hostnames) => hostnames ?? [])))
.then((initialHostnames) => {
this._ignoredHostnames = new Set(initialHostnames);
logService.debug(
`[PhishingDetectionService] Loaded ${initialHostnames.length} ignored hostnames from storage`,
);
})
.catch((error) => {
logService.error("[PhishingDetectionService] Failed to load ignored hostnames", error);
});
Copy link

@claude claude bot Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Race condition: The initial state load is not awaited, which means the service may start processing navigation events before _ignoredHostnames is populated from disk. This defeats the purpose of persistent storage.

If a user navigates to a previously-ignored phishing site immediately after browser startup, they'll see the warning again.

Suggested change
// Load previously ignored hostnames from storage (synchronously to avoid race conditions)
const ignoredHostnamesState = globalStateProvider.get(IGNORED_PHISHING_HOSTNAMES_KEY);
firstValueFrom(ignoredHostnamesState.state$.pipe(map((hostnames) => hostnames ?? [])))
.then((initialHostnames) => {
this._ignoredHostnames = new Set(initialHostnames);
logService.debug(
`[PhishingDetectionService] Loaded ${initialHostnames.length} ignored hostnames from storage`,
);
})
.catch((error) => {
logService.error("[PhishingDetectionService] Failed to load ignored hostnames", error);
});
// Load previously ignored hostnames from storage (await to avoid race conditions)
const ignoredHostnamesState = globalStateProvider.get(IGNORED_PHISHING_HOSTNAMES_KEY);
try {
const initialHostnames = await firstValueFrom(
ignoredHostnamesState.state$.pipe(map((hostnames) => hostnames ?? []))
);
this._ignoredHostnames = new Set(initialHostnames);
logService.debug(
`[PhishingDetectionService] Loaded ${initialHostnames.length} ignored hostnames from storage`,
);
} catch (error) {
logService.error("[PhishingDetectionService] Failed to load ignored hostnames", error);
}


// Subscribe to future state changes
const ignoredHostnamesSub = ignoredHostnamesState.state$
.pipe(map((hostnames) => hostnames ?? []))
.subscribe((hostnames) => {
this._ignoredHostnames = new Set(hostnames);
});

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}`),
),
concatMap(async (message) => {
const url = new URL(message.url);
this._ignoredHostnames.add(url.hostname);
await BrowserApi.navigateTabToUrl(message.tabId, url);
try {
const url = new URL(message.url);
this._ignoredHostnames.add(url.hostname);
// Persist to storage
await ignoredHostnamesState.update(() => Array.from(this._ignoredHostnames));
Comment on lines +105 to +107
Copy link
Contributor

@coroiu coroiu Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: this is susceptible to race-conditions because awaiting ignoredHostnamesState.update can cause the execution of this function to be pre-empted by e.g. an emission of ignoredHostnamesState.state$ which would cause this._ignoredHostnames to get overwritten before the anonymous function inside update(...) has a chance to fetch the value

Either stash a copy of the Set locally or, better, don't keep a copy of state data outside of state providers and subscribe to it instead

logService.debug(
`[PhishingDetectionService] Added ${url.hostname} to ignored hostnames (persisted)`,
);
await BrowserApi.navigateTabToUrl(message.tabId, url);
} catch (error) {
logService.error(
`[PhishingDetectionService] Failed to process continue command for URL: ${message.url}`,
error,
);
}
}),
);

Expand All @@ -97,8 +143,10 @@ export class PhishingDetectionService {
tap((event) => logService.debug(`[PhishingDetectionService] processing event:`, event)),
concatMap(async ({ tabId, url, ignored }) => {
if (ignored) {
// The next time this host is visited, block again
this._ignoredHostnames.delete(url.hostname);
// User has previously chosen to continue to this hostname, skip phishing check
logService.debug(
`[PhishingDetectionService] Skipping phishing check for ignored hostname: ${url.hostname}`,
);
return;
}
const isPhishing = await phishingDataService.isPhishingDomain(url);
Expand Down Expand Up @@ -158,6 +206,7 @@ export class PhishingDetectionService {
this._didInit = true;
return () => {
initSub.unsubscribe();
ignoredHostnamesSub.unsubscribe();
this._didInit = false;

// Manually type cast to satisfy the listener signature due to the mixture
Expand Down
6 changes: 3 additions & 3 deletions apps/browser/src/platform/browser/browser-api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,11 +268,11 @@ export class BrowserApi {
static async closeTab(tabId: number): Promise<void> {
if (tabId) {
if (BrowserApi.isWebExtensionsApi) {
browser.tabs.remove(tabId).catch((error) => {
await browser.tabs.remove(tabId).catch((error) => {
throw new Error("[BrowserApi] Failed to remove current tab: " + error.message);
});
} else if (BrowserApi.isChromeApi) {
chrome.tabs.remove(tabId).catch((error) => {
await chrome.tabs.remove(tabId).catch((error) => {
throw new Error("[BrowserApi] Failed to remove current tab: " + error.message);
});
}
Expand All @@ -288,7 +288,7 @@ export class BrowserApi {
static async navigateTabToUrl(tabId: number, url: URL): Promise<void> {
if (tabId) {
if (BrowserApi.isWebExtensionsApi) {
browser.tabs.update(tabId, { url: url.href }).catch((error) => {
await browser.tabs.update(tabId, { url: url.href }).catch((error) => {
throw new Error("Failed to navigate tab to URL: " + error.message);
});
} else if (BrowserApi.isChromeApi) {
Expand Down
Loading