From 1c6a83f31156b7df532008171748f5841c8ee83a Mon Sep 17 00:00:00 2001 From: Daniel James Smith <2670567+djsmith85@users.noreply.github.com> Date: Mon, 29 Dec 2025 21:28:33 +0100 Subject: [PATCH 1/8] [BEEEP][PM-29441] Introduce bitwarden-encrypted-json-importer (#17651) * Introduce bitwarden-encrypted-json-importer An effort to introduce type guards and split the logic between the differently protected bitwarden-json import-formats * Improved stricter types, but not quite ts-strict yet * Add guard to prevent passing password-protected exports to the wrong importer. * Only create one return object instead of multiple * Updated changes afer npm ci and npm run prettier --------- Co-authored-by: Daniel James Smith --- .../bitwarden-encrypted-json-importer.ts | 201 ++++++++++++++++++ .../bitwarden/bitwarden-json-importer.ts | 195 ++++------------- ...warden-password-protected-importer.spec.ts | 11 +- .../bitwarden-password-protected-importer.ts | 23 +- .../src/types/bitwarden-json-export-types.ts | 85 ++++++-- 5 files changed, 328 insertions(+), 187 deletions(-) create mode 100644 libs/importer/src/importers/bitwarden/bitwarden-encrypted-json-importer.ts diff --git a/libs/importer/src/importers/bitwarden/bitwarden-encrypted-json-importer.ts b/libs/importer/src/importers/bitwarden/bitwarden-encrypted-json-importer.ts new file mode 100644 index 00000000000..4771f47b4c9 --- /dev/null +++ b/libs/importer/src/importers/bitwarden/bitwarden-encrypted-json-importer.ts @@ -0,0 +1,201 @@ +// FIXME: Update this file to be type safe and remove this and next line +// @ts-strict-ignore +import { filter, firstValueFrom } from "rxjs"; + +// This import has been flagged as unallowed for this class. It may be involved in a circular dependency loop. +// eslint-disable-next-line no-restricted-imports +import { Collection } from "@bitwarden/admin-console/common"; +import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; +import { EncryptService } from "@bitwarden/common/key-management/crypto/abstractions/encrypt.service"; +import { EncString } from "@bitwarden/common/key-management/crypto/models/enc-string"; +import { + CipherWithIdExport, + CollectionWithIdExport, + FolderWithIdExport, +} from "@bitwarden/common/models/export"; +import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; +import { OrgKey, UserKey } from "@bitwarden/common/types/key"; +import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service"; +import { FolderView } from "@bitwarden/common/vault/models/view/folder.view"; +import { KeyService } from "@bitwarden/key-management"; +import { UserId } from "@bitwarden/user-core"; +import { + BitwardenEncryptedIndividualJsonExport, + BitwardenEncryptedJsonExport, + BitwardenEncryptedOrgJsonExport, + BitwardenJsonExport, + BitwardenPasswordProtectedFileFormat, + isOrgEncrypted, + isPasswordProtected, + isUnencrypted, +} from "@bitwarden/vault-export-core"; + +import { ImportResult } from "../../models/import-result"; +import { Importer } from "../importer"; + +import { BitwardenJsonImporter } from "./bitwarden-json-importer"; + +export class BitwardenEncryptedJsonImporter extends BitwardenJsonImporter implements Importer { + constructor( + protected keyService: KeyService, + protected encryptService: EncryptService, + protected i18nService: I18nService, + private cipherService: CipherService, + private accountService: AccountService, + ) { + super(); + } + + async parse(data: string): Promise { + const results: BitwardenPasswordProtectedFileFormat | BitwardenJsonExport = JSON.parse(data); + + if (isPasswordProtected(results)) { + throw new Error( + "Data is password-protected. Use BitwardenPasswordProtectedImporter instead.", + ); + } + + if (results == null || results.items == null) { + const result = new ImportResult(); + result.success = false; + return result; + } + + if (isUnencrypted(results)) { + return super.parse(data); + } + + return await this.parseEncrypted(results); + } + + private async parseEncrypted(data: BitwardenEncryptedJsonExport): Promise { + const result = new ImportResult(); + const account = await firstValueFrom(this.accountService.activeAccount$); + + if (this.isNullOrWhitespace(data.encKeyValidation_DO_NOT_EDIT)) { + result.success = false; + result.errorMessage = this.i18nService.t("importEncKeyError"); + return result; + } + + const orgKeys = await firstValueFrom(this.keyService.orgKeys$(account.id)); + let keyForDecryption: OrgKey | UserKey | null | undefined = orgKeys?.[this.organizationId]; + if (!keyForDecryption) { + keyForDecryption = await firstValueFrom(this.keyService.userKey$(account.id)); + } + + if (!keyForDecryption) { + result.success = false; + result.errorMessage = this.i18nService.t("importEncKeyError"); + return result; + } + const encKeyValidation = new EncString(data.encKeyValidation_DO_NOT_EDIT); + try { + await this.encryptService.decryptString(encKeyValidation, keyForDecryption); + } catch { + result.success = false; + result.errorMessage = this.i18nService.t("importEncKeyError"); + return result; + } + + let groupingsMap: Map | null = null; + if (isOrgEncrypted(data)) { + groupingsMap = await this.parseEncryptedCollections(account.id, data, result); + } else { + groupingsMap = await this.parseEncryptedFolders(account.id, data, result); + } + + for (const c of data.items) { + const cipher = CipherWithIdExport.toDomain(c); + // reset ids in case they were set for some reason + cipher.id = null; + cipher.organizationId = this.organizationId; + cipher.collectionIds = null; + + // make sure password history is limited + if (cipher.passwordHistory != null && cipher.passwordHistory.length > 5) { + cipher.passwordHistory = cipher.passwordHistory.slice(0, 5); + } + + if (!this.organization && c.folderId != null && groupingsMap.has(c.folderId)) { + result.folderRelationships.push([result.ciphers.length, groupingsMap.get(c.folderId)]); + } else if (this.organization && c.collectionIds != null) { + c.collectionIds.forEach((cId) => { + if (groupingsMap.has(cId)) { + result.collectionRelationships.push([result.ciphers.length, groupingsMap.get(cId)]); + } + }); + } + + const view = await this.cipherService.decrypt(cipher, account.id); + this.cleanupCipher(view); + result.ciphers.push(view); + } + + result.success = true; + return result; + } + + private async parseEncryptedFolders( + userId: UserId, + data: BitwardenEncryptedIndividualJsonExport, + importResult: ImportResult, + ): Promise> { + const groupingsMap = new Map(); + + if (data.folders == null) { + return groupingsMap; + } + + const userKey = await firstValueFrom(this.keyService.userKey$(userId)); + + for (const f of data.folders) { + let folderView: FolderView; + const folder = FolderWithIdExport.toDomain(f); + if (folder != null) { + folderView = await folder.decrypt(userKey); + } + + if (folderView != null) { + groupingsMap.set(f.id, importResult.folders.length); + importResult.folders.push(folderView); + } + } + return groupingsMap; + } + + private async parseEncryptedCollections( + userId: UserId, + data: BitwardenEncryptedOrgJsonExport, + importResult: ImportResult, + ): Promise> { + const groupingsMap = new Map(); + if (data.collections == null) { + return groupingsMap; + } + + const orgKeys = await firstValueFrom( + this.keyService.orgKeys$(userId).pipe(filter((orgKeys) => orgKeys != null)), + ); + + for (const c of data.collections) { + const collection = CollectionWithIdExport.toDomain( + c, + new Collection({ + id: c.id, + name: new EncString(c.name), + organizationId: this.organizationId, + }), + ); + + const orgKey = orgKeys[c.organizationId]; + const collectionView = await collection.decrypt(orgKey, this.encryptService); + + if (collectionView != null) { + groupingsMap.set(c.id, importResult.collections.length); + importResult.collections.push(collectionView); + } + } + return groupingsMap; + } +} diff --git a/libs/importer/src/importers/bitwarden/bitwarden-json-importer.ts b/libs/importer/src/importers/bitwarden/bitwarden-json-importer.ts index 1f5be7f18ab..ddeba885f88 100644 --- a/libs/importer/src/importers/bitwarden/bitwarden-json-importer.ts +++ b/libs/importer/src/importers/bitwarden/bitwarden-json-importer.ts @@ -1,30 +1,17 @@ // FIXME: Update this file to be type safe and remove this and next line // @ts-strict-ignore -import { filter, firstValueFrom } from "rxjs"; - -// This import has been flagged as unallowed for this class. It may be involved in a circular dependency loop. -// eslint-disable-next-line no-restricted-imports -import { Collection, CollectionView } from "@bitwarden/admin-console/common"; -import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; -import { EncryptService } from "@bitwarden/common/key-management/crypto/abstractions/encrypt.service"; -import { EncString } from "@bitwarden/common/key-management/crypto/models/enc-string"; import { CipherWithIdExport, CollectionWithIdExport, FolderWithIdExport, } from "@bitwarden/common/models/export"; -import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; -import { SymmetricCryptoKey } from "@bitwarden/common/platform/models/domain/symmetric-crypto-key"; -import { UserId } from "@bitwarden/common/types/guid"; -import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service"; -import { FolderView } from "@bitwarden/common/vault/models/view/folder.view"; -import { KeyService } from "@bitwarden/key-management"; import { - BitwardenEncryptedIndividualJsonExport, - BitwardenEncryptedOrgJsonExport, BitwardenJsonExport, BitwardenUnEncryptedIndividualJsonExport, + BitwardenUnEncryptedJsonExport, BitwardenUnEncryptedOrgJsonExport, + isOrgUnEncrypted, + isUnencrypted, } from "@bitwarden/vault-export-core"; import { ImportResult } from "../../models/import-result"; @@ -32,103 +19,30 @@ import { BaseImporter } from "../base-importer"; import { Importer } from "../importer"; export class BitwardenJsonImporter extends BaseImporter implements Importer { - private result: ImportResult; - - protected constructor( - protected keyService: KeyService, - protected encryptService: EncryptService, - protected i18nService: I18nService, - protected cipherService: CipherService, - protected accountService: AccountService, - ) { + protected constructor() { super(); } async parse(data: string): Promise { - const account = await firstValueFrom(this.accountService.activeAccount$); - this.result = new ImportResult(); const results: BitwardenJsonExport = JSON.parse(data); if (results == null || results.items == null) { - this.result.success = false; - return this.result; + const result = new ImportResult(); + result.success = false; + return result; } - if (results.encrypted) { - await this.parseEncrypted(results as any, account.id); - } else { - await this.parseDecrypted(results as any, account.id); + if (!isUnencrypted(results)) { + throw new Error("Data is encrypted. Use BitwardenEncryptedJsonImporter instead."); } - - return this.result; + return await this.parseDecrypted(results); } - private async parseEncrypted( - results: BitwardenEncryptedIndividualJsonExport | BitwardenEncryptedOrgJsonExport, - userId: UserId, - ) { - if (results.encKeyValidation_DO_NOT_EDIT != null) { - const orgKeys = await firstValueFrom(this.keyService.orgKeys$(userId)); - let keyForDecryption: SymmetricCryptoKey = orgKeys?.[this.organizationId]; - if (keyForDecryption == null) { - keyForDecryption = await firstValueFrom(this.keyService.userKey$(userId)); - } - const encKeyValidation = new EncString(results.encKeyValidation_DO_NOT_EDIT); - try { - await this.encryptService.decryptString(encKeyValidation, keyForDecryption); - } catch { - this.result.success = false; - this.result.errorMessage = this.i18nService.t("importEncKeyError"); - return; - } - } + private async parseDecrypted(results: BitwardenUnEncryptedJsonExport): Promise { + const importResult = new ImportResult(); - const groupingsMap = this.organization - ? await this.parseCollections(results as BitwardenEncryptedOrgJsonExport, userId) - : await this.parseFolders(results as BitwardenEncryptedIndividualJsonExport, userId); - - for (const c of results.items) { - const cipher = CipherWithIdExport.toDomain(c); - // reset ids in case they were set for some reason - cipher.id = null; - cipher.organizationId = this.organizationId; - cipher.collectionIds = null; - - // make sure password history is limited - if (cipher.passwordHistory != null && cipher.passwordHistory.length > 5) { - cipher.passwordHistory = cipher.passwordHistory.slice(0, 5); - } - - if (!this.organization && c.folderId != null && groupingsMap.has(c.folderId)) { - this.result.folderRelationships.push([ - this.result.ciphers.length, - groupingsMap.get(c.folderId), - ]); - } else if (this.organization && c.collectionIds != null) { - c.collectionIds.forEach((cId) => { - if (groupingsMap.has(cId)) { - this.result.collectionRelationships.push([ - this.result.ciphers.length, - groupingsMap.get(cId), - ]); - } - }); - } - - const view = await this.cipherService.decrypt(cipher, userId); - this.cleanupCipher(view); - this.result.ciphers.push(view); - } - - this.result.success = true; - } - - private async parseDecrypted( - results: BitwardenUnEncryptedIndividualJsonExport | BitwardenUnEncryptedOrgJsonExport, - userId: UserId, - ) { - const groupingsMap = this.organization - ? await this.parseCollections(results as BitwardenUnEncryptedOrgJsonExport, userId) - : await this.parseFolders(results as BitwardenUnEncryptedIndividualJsonExport, userId); + const groupingsMap = isOrgUnEncrypted(results) + ? await this.parseCollections(results, importResult) + : await this.parseFolders(results, importResult); results.items.forEach((c) => { const cipher = CipherWithIdExport.toView(c); @@ -143,15 +57,15 @@ export class BitwardenJsonImporter extends BaseImporter implements Importer { } if (!this.organization && c.folderId != null && groupingsMap.has(c.folderId)) { - this.result.folderRelationships.push([ - this.result.ciphers.length, + importResult.folderRelationships.push([ + importResult.ciphers.length, groupingsMap.get(c.folderId), ]); } else if (this.organization && c.collectionIds != null) { c.collectionIds.forEach((cId) => { if (groupingsMap.has(cId)) { - this.result.collectionRelationships.push([ - this.result.ciphers.length, + importResult.collectionRelationships.push([ + importResult.ciphers.length, groupingsMap.get(cId), ]); } @@ -159,79 +73,48 @@ export class BitwardenJsonImporter extends BaseImporter implements Importer { } this.cleanupCipher(cipher); - this.result.ciphers.push(cipher); + importResult.ciphers.push(cipher); }); - this.result.success = true; + importResult.success = true; + return importResult; } private async parseFolders( - data: BitwardenUnEncryptedIndividualJsonExport | BitwardenEncryptedIndividualJsonExport, - userId: UserId, - ): Promise> | null { + data: BitwardenUnEncryptedIndividualJsonExport, + importResult: ImportResult, + ): Promise> { + const groupingsMap = new Map(); if (data.folders == null) { - return null; + return groupingsMap; } - const userKey = await firstValueFrom(this.keyService.userKey$(userId)); - - const groupingsMap = new Map(); - for (const f of data.folders) { - let folderView: FolderView; - if (data.encrypted) { - const folder = FolderWithIdExport.toDomain(f); - if (folder != null) { - folderView = await folder.decrypt(userKey); - } - } else { - folderView = FolderWithIdExport.toView(f); - } - + const folderView = FolderWithIdExport.toView(f); if (folderView != null) { - groupingsMap.set(f.id, this.result.folders.length); - this.result.folders.push(folderView); + groupingsMap.set(f.id, importResult.folders.length); + importResult.folders.push(folderView); } } return groupingsMap; } private async parseCollections( - data: BitwardenUnEncryptedOrgJsonExport | BitwardenEncryptedOrgJsonExport, - userId: UserId, - ): Promise> | null { + data: BitwardenUnEncryptedOrgJsonExport, + importResult: ImportResult, + ): Promise> { + const groupingsMap = new Map(); if (data.collections == null) { - return null; + return groupingsMap; } - const orgKeys = await firstValueFrom( - this.keyService.orgKeys$(userId).pipe(filter((orgKeys) => orgKeys != null)), - ); - - const groupingsMap = new Map(); - for (const c of data.collections) { - let collectionView: CollectionView; - if (data.encrypted) { - const collection = CollectionWithIdExport.toDomain( - c, - new Collection({ - id: c.id, - name: new EncString(c.name), - organizationId: this.organizationId, - }), - ); - - const orgKey = orgKeys[c.organizationId]; - collectionView = await collection.decrypt(orgKey, this.encryptService); - } else { - collectionView = CollectionWithIdExport.toView(c); - collectionView.organizationId = null; - } + const collectionView = CollectionWithIdExport.toView(c); + collectionView.organizationId = null; if (collectionView != null) { - groupingsMap.set(c.id, this.result.collections.length); - this.result.collections.push(collectionView); + groupingsMap.set(c.id, importResult.collections.length); + importResult.collections.push(collectionView); } } return groupingsMap; diff --git a/libs/importer/src/importers/bitwarden/bitwarden-password-protected-importer.spec.ts b/libs/importer/src/importers/bitwarden/bitwarden-password-protected-importer.spec.ts index fdf92cac751..ff6e3692640 100644 --- a/libs/importer/src/importers/bitwarden/bitwarden-password-protected-importer.spec.ts +++ b/libs/importer/src/importers/bitwarden/bitwarden-password-protected-importer.spec.ts @@ -16,6 +16,7 @@ import { UserId } from "@bitwarden/user-core"; import { emptyAccountEncrypted } from "../spec-data/bitwarden-json/account-encrypted.json"; import { emptyUnencryptedExport } from "../spec-data/bitwarden-json/unencrypted.json"; +import { BitwardenEncryptedJsonImporter } from "./bitwarden-encrypted-json-importer"; import { BitwardenJsonImporter } from "./bitwarden-json-importer"; import { BitwardenPasswordProtectedImporter } from "./bitwarden-password-protected-importer"; @@ -92,7 +93,7 @@ describe("BitwardenPasswordProtectedImporter", () => { describe("Account encrypted", () => { beforeAll(() => { - jest.spyOn(BitwardenJsonImporter.prototype, "parse"); + jest.spyOn(BitwardenEncryptedJsonImporter.prototype, "parse"); }); beforeEach(() => { @@ -114,9 +115,11 @@ describe("BitwardenPasswordProtectedImporter", () => { ); }); - it("Should call BitwardenJsonImporter", async () => { - expect((await importer.parse(emptyAccountEncrypted)).success).toEqual(true); - expect(BitwardenJsonImporter.prototype.parse).toHaveBeenCalledWith(emptyAccountEncrypted); + it("Should call BitwardenEncryptedJsonImporter", async () => { + expect((await importer.parse(emptyAccountEncrypted)).success).toEqual(false); + expect(BitwardenEncryptedJsonImporter.prototype.parse).toHaveBeenCalledWith( + emptyAccountEncrypted, + ); }); }); diff --git a/libs/importer/src/importers/bitwarden/bitwarden-password-protected-importer.ts b/libs/importer/src/importers/bitwarden/bitwarden-password-protected-importer.ts index b685ddf0fb5..cc38c420d9b 100644 --- a/libs/importer/src/importers/bitwarden/bitwarden-password-protected-importer.ts +++ b/libs/importer/src/importers/bitwarden/bitwarden-password-protected-importer.ts @@ -14,14 +14,21 @@ import { KeyService, KdfType, } from "@bitwarden/key-management"; -import { BitwardenPasswordProtectedFileFormat } from "@bitwarden/vault-export-core"; +import { + BitwardenJsonExport, + BitwardenPasswordProtectedFileFormat, + isPasswordProtected, +} from "@bitwarden/vault-export-core"; import { ImportResult } from "../../models/import-result"; import { Importer } from "../importer"; -import { BitwardenJsonImporter } from "./bitwarden-json-importer"; +import { BitwardenEncryptedJsonImporter } from "./bitwarden-encrypted-json-importer"; -export class BitwardenPasswordProtectedImporter extends BitwardenJsonImporter implements Importer { +export class BitwardenPasswordProtectedImporter + extends BitwardenEncryptedJsonImporter + implements Importer +{ private key: SymmetricCryptoKey; constructor( @@ -38,20 +45,14 @@ export class BitwardenPasswordProtectedImporter extends BitwardenJsonImporter im async parse(data: string): Promise { const result = new ImportResult(); - const parsedData: BitwardenPasswordProtectedFileFormat = JSON.parse(data); + const parsedData: BitwardenPasswordProtectedFileFormat | BitwardenJsonExport = JSON.parse(data); if (!parsedData) { result.success = false; return result; } - // File is unencrypted - if (!parsedData?.encrypted) { - return await super.parse(data); - } - - // File is account-encrypted - if (!parsedData?.passwordProtected) { + if (!isPasswordProtected(parsedData)) { return await super.parse(data); } diff --git a/libs/tools/export/vault-export/vault-export-core/src/types/bitwarden-json-export-types.ts b/libs/tools/export/vault-export/vault-export-core/src/types/bitwarden-json-export-types.ts index ab2bcbb9f1f..fd33bf96923 100644 --- a/libs/tools/export/vault-export/vault-export-core/src/types/bitwarden-json-export-types.ts +++ b/libs/tools/export/vault-export/vault-export-core/src/types/bitwarden-json-export-types.ts @@ -5,42 +5,48 @@ import { } from "@bitwarden/common/models/export"; // Base -export type BitwardenJsonExport = { - encrypted: boolean; - items: CipherWithIdExport[]; -}; +export type BitwardenJsonExport = BitwardenUnEncryptedJsonExport | BitwardenEncryptedJsonExport; // Decrypted -export type BitwardenUnEncryptedJsonExport = BitwardenJsonExport & { - encrypted: false; -}; +export type BitwardenUnEncryptedJsonExport = + | BitwardenUnEncryptedIndividualJsonExport + | BitwardenUnEncryptedOrgJsonExport; -export type BitwardenUnEncryptedIndividualJsonExport = BitwardenUnEncryptedJsonExport & { +export type BitwardenUnEncryptedIndividualJsonExport = { + encrypted: false; + items: CipherWithIdExport[]; folders: FolderWithIdExport[]; }; -export type BitwardenUnEncryptedOrgJsonExport = BitwardenUnEncryptedJsonExport & { +export type BitwardenUnEncryptedOrgJsonExport = { + encrypted: false; + items: CipherWithIdExport[]; collections: CollectionWithIdExport[]; }; // Account-encrypted -export type BitwardenEncryptedJsonExport = BitwardenJsonExport & { +export type BitwardenEncryptedJsonExport = + | BitwardenEncryptedIndividualJsonExport + | BitwardenEncryptedOrgJsonExport; + +export type BitwardenEncryptedIndividualJsonExport = { encrypted: true; encKeyValidation_DO_NOT_EDIT: string; -}; - -export type BitwardenEncryptedIndividualJsonExport = BitwardenEncryptedJsonExport & { + items: CipherWithIdExport[]; folders: FolderWithIdExport[]; }; -export type BitwardenEncryptedOrgJsonExport = BitwardenEncryptedJsonExport & { +export type BitwardenEncryptedOrgJsonExport = { + encrypted: true; + encKeyValidation_DO_NOT_EDIT: string; + items: CipherWithIdExport[]; collections: CollectionWithIdExport[]; }; // Password-protected export type BitwardenPasswordProtectedFileFormat = { - encrypted: boolean; - passwordProtected: boolean; + encrypted: true; + passwordProtected: true; salt: string; kdfIterations: number; kdfMemory?: number; @@ -49,3 +55,50 @@ export type BitwardenPasswordProtectedFileFormat = { encKeyValidation_DO_NOT_EDIT: string; data: string; }; + +// Unencrypted type guards +export function isUnencrypted( + data: BitwardenJsonExport | null | undefined, +): data is BitwardenUnEncryptedJsonExport { + return data != null && (data as { encrypted?: unknown }).encrypted !== true; +} + +export function isIndividualUnEncrypted( + data: BitwardenJsonExport | null | undefined, +): data is BitwardenUnEncryptedIndividualJsonExport { + return isUnencrypted(data) && (data as { folders?: unknown }).folders != null; +} + +export function isOrgUnEncrypted( + data: BitwardenJsonExport | null | undefined, +): data is BitwardenUnEncryptedOrgJsonExport { + return isUnencrypted(data) && (data as { collections?: unknown }).collections != null; +} + +// Encrypted type guards +export function isEncrypted( + data: BitwardenJsonExport | null | undefined, +): data is BitwardenEncryptedJsonExport { + return data != null && (data as { encrypted?: unknown }).encrypted === true; +} +export function isPasswordProtected( + data: BitwardenPasswordProtectedFileFormat | BitwardenJsonExport | null | undefined, +): data is BitwardenPasswordProtectedFileFormat { + return ( + data != null && + (data as { encrypted?: unknown }).encrypted === true && + (data as { passwordProtected?: unknown }).passwordProtected === true + ); +} + +export function isIndividualEncrypted( + data: BitwardenJsonExport | null | undefined, +): data is BitwardenEncryptedIndividualJsonExport { + return isEncrypted(data) && (data as { folders?: unknown }).folders != null; +} + +export function isOrgEncrypted( + data: BitwardenJsonExport | null | undefined, +): data is BitwardenEncryptedOrgJsonExport { + return isEncrypted(data) && (data as { collections?: unknown }).collections != null; +} From 7853ac3d9f17e99937126f15cdd1788c04ace8df Mon Sep 17 00:00:00 2001 From: Daniel Riera Date: Mon, 29 Dec 2025 16:16:58 -0500 Subject: [PATCH 2/8] PM-29509 [LO IMPACT] Remove @ts-strict-ignore in fido2/content/messaging/messenger.ts (#17913) * PM-29509 [LO IMPACT] Remove @ts-strict-ignore in fido2/content/messaging/messenger.ts - 1 err, 137 LOC, 11.4 * strip metadata from message * preserve one way handler --- .../fido2/content/messaging/messenger.ts | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/apps/browser/src/autofill/fido2/content/messaging/messenger.ts b/apps/browser/src/autofill/fido2/content/messaging/messenger.ts index 257f7e9efd5..61ed7a8ed08 100644 --- a/apps/browser/src/autofill/fido2/content/messaging/messenger.ts +++ b/apps/browser/src/autofill/fido2/content/messaging/messenger.ts @@ -1,5 +1,3 @@ -// FIXME: Update this file to be type safe and remove this and next line -// @ts-strict-ignore import { Message, MessageTypes } from "./message"; const SENDER = "bitwarden-webauthn"; @@ -25,7 +23,7 @@ type Handler = ( * handling aborts and exceptions across separate execution contexts. */ export class Messenger { - private messageEventListener: (event: MessageEvent) => void | null = null; + private messageEventListener: ((event: MessageEvent) => void) | null = null; private onDestroy = new EventTarget(); /** @@ -60,6 +58,12 @@ export class Messenger { this.broadcastChannel.addEventListener(this.messageEventListener); } + private stripMetadata({ SENDER, senderId, ...message }: MessageWithMetadata): Message { + void SENDER; + void senderId; + return message; + } + /** * Sends a request to the content script and returns the response. * AbortController signals will be forwarded to the content script. @@ -74,7 +78,9 @@ export class Messenger { try { const promise = new Promise((resolve) => { - localPort.onmessage = (event: MessageEvent) => resolve(event.data); + localPort.onmessage = (event: MessageEvent) => { + resolve(this.stripMetadata(event.data)); + }; }); const abortListener = () => @@ -129,7 +135,9 @@ export class Messenger { try { const handlerResponse = await this.handler(message, abortController); - port.postMessage({ ...handlerResponse, SENDER }); + if (handlerResponse !== undefined) { + port.postMessage({ ...handlerResponse, SENDER }); + } } catch (error) { port.postMessage({ SENDER, From 696c53fac7fea2cd977155d0b1688277188c83e1 Mon Sep 17 00:00:00 2001 From: Shane Melton Date: Mon, 29 Dec 2025 16:41:42 -0800 Subject: [PATCH 3/8] [PM-29209] Fix persistent browser settings berry (#18113) * [PM-29209] Introduce new autofill nudge service specific to the Browser client * [PM-29209] Cleanup redundant browser setting checks * [PM-29209] Ensure nudge is dismissed on nudge button click * [PM-29209] Add spec file for browser autofill nudge service * [PM-29209] Cleanup settings-v2 spec file --- .../popup/settings/autofill.component.html | 22 +-- .../popup/settings/autofill.component.ts | 4 + .../src/popup/services/services.module.ts | 8 + .../popup/settings/settings-v2.component.html | 12 +- .../settings/settings-v2.component.spec.ts | 46 +---- .../popup/settings/settings-v2.component.ts | 33 +--- .../browser-autofill-nudge.service.spec.ts | 157 ++++++++++++++++++ .../browser-autofill-nudge.service.ts | 37 +++++ libs/angular/src/vault/index.ts | 1 + .../services/custom-nudges-services/index.ts | 1 + .../noop-nudge.service.ts | 27 +++ .../vault/services/nudge-injection-tokens.ts | 7 + .../src/vault/services/nudges.service.ts | 10 +- 13 files changed, 272 insertions(+), 93 deletions(-) create mode 100644 apps/browser/src/vault/popup/services/browser-autofill-nudge.service.spec.ts create mode 100644 apps/browser/src/vault/popup/services/browser-autofill-nudge.service.ts create mode 100644 libs/angular/src/vault/services/custom-nudges-services/noop-nudge.service.ts create mode 100644 libs/angular/src/vault/services/nudge-injection-tokens.ts diff --git a/apps/browser/src/autofill/popup/settings/autofill.component.html b/apps/browser/src/autofill/popup/settings/autofill.component.html index 1153ad58719..085145adb19 100644 --- a/apps/browser/src/autofill/popup/settings/autofill.component.html +++ b/apps/browser/src/autofill/popup/settings/autofill.component.html @@ -6,16 +6,18 @@
-
- -
+ @if (showSpotlightNudge$ | async) { +
+ +
+ }

{{ "autofillSuggestionsSectionTitle" | i18n }}

diff --git a/apps/browser/src/autofill/popup/settings/autofill.component.ts b/apps/browser/src/autofill/popup/settings/autofill.component.ts index 49be3104dc1..acb2aa7a970 100644 --- a/apps/browser/src/autofill/popup/settings/autofill.component.ts +++ b/apps/browser/src/autofill/popup/settings/autofill.component.ts @@ -611,6 +611,10 @@ export class AutofillComponent implements OnInit { if (this.canOverrideBrowserAutofillSetting) { this.defaultBrowserAutofillDisabled = true; await this.updateDefaultBrowserAutofillDisabled(); + await this.nudgesService.dismissNudge( + NudgeType.AutofillNudge, + await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId)), + ); } else { await this.openURI(event, this.disablePasswordManagerURI); } diff --git a/apps/browser/src/popup/services/services.module.ts b/apps/browser/src/popup/services/services.module.ts index 739166ff6f8..7a2ded5bb83 100644 --- a/apps/browser/src/popup/services/services.module.ts +++ b/apps/browser/src/popup/services/services.module.ts @@ -23,6 +23,8 @@ import { WINDOW, } from "@bitwarden/angular/services/injection-tokens"; import { JslibServicesModule } from "@bitwarden/angular/services/jslib-services.module"; +import { AUTOFILL_NUDGE_SERVICE } from "@bitwarden/angular/vault"; +import { SingleNudgeService } from "@bitwarden/angular/vault/services/default-single-nudge.service"; import { LoginComponentService, TwoFactorAuthComponentService, @@ -208,6 +210,7 @@ import { } from "../../platform/system-notifications/browser-system-notification.service"; import { fromChromeRuntimeMessaging } from "../../platform/utils/from-chrome-runtime-messaging"; import { FilePopoutUtilsService } from "../../tools/popup/services/file-popout-utils.service"; +import { BrowserAutofillNudgeService } from "../../vault/popup/services/browser-autofill-nudge.service"; import { Fido2UserVerificationService } from "../../vault/services/fido2-user-verification.service"; import { ExtensionAnonLayoutWrapperDataService } from "../components/extension-anon-layout-wrapper/extension-anon-layout-wrapper-data.service"; @@ -756,6 +759,11 @@ const safeProviders: SafeProvider[] = [ MessagingServiceAbstraction, ], }), + safeProvider({ + provide: AUTOFILL_NUDGE_SERVICE as SafeInjectionToken, + useClass: BrowserAutofillNudgeService, + deps: [], + }), ]; @NgModule({ diff --git a/apps/browser/src/tools/popup/settings/settings-v2.component.html b/apps/browser/src/tools/popup/settings/settings-v2.component.html index 683b7d70ed6..06c89e15f59 100644 --- a/apps/browser/src/tools/popup/settings/settings-v2.component.html +++ b/apps/browser/src/tools/popup/settings/settings-v2.component.html @@ -34,13 +34,11 @@

{{ "autofill" | i18n }}

- 1 + @if (showAutofillBadge$ | async) { + 1 + }
diff --git a/apps/browser/src/tools/popup/settings/settings-v2.component.spec.ts b/apps/browser/src/tools/popup/settings/settings-v2.component.spec.ts index f51d514289e..4cc3ed0149c 100644 --- a/apps/browser/src/tools/popup/settings/settings-v2.component.spec.ts +++ b/apps/browser/src/tools/popup/settings/settings-v2.component.spec.ts @@ -148,31 +148,7 @@ describe("SettingsV2Component", () => { expect(openSpy).toHaveBeenCalledWith(dialogService); }); - it("isBrowserAutofillSettingOverridden$ emits the value from the AutofillBrowserSettingsService", async () => { - pushActiveAccount(); - - mockAutofillSettings.isBrowserAutofillSettingOverridden.mockResolvedValue(true); - - const fixture = TestBed.createComponent(SettingsV2Component); - const component = fixture.componentInstance; - fixture.detectChanges(); - await fixture.whenStable(); - - const value = await firstValueFrom(component["isBrowserAutofillSettingOverridden$"]); - expect(value).toBe(true); - - mockAutofillSettings.isBrowserAutofillSettingOverridden.mockResolvedValue(false); - - const fixture2 = TestBed.createComponent(SettingsV2Component); - const component2 = fixture2.componentInstance; - fixture2.detectChanges(); - await fixture2.whenStable(); - - const value2 = await firstValueFrom(component2["isBrowserAutofillSettingOverridden$"]); - expect(value2).toBe(false); - }); - - it("showAutofillBadge$ emits true when default autofill is NOT disabled and nudge is true", async () => { + it("showAutofillBadge$ emits true when showNudgeBadge is true", async () => { pushActiveAccount(); mockNudges.showNudgeBadge$.mockImplementation((type: NudgeType) => @@ -184,30 +160,10 @@ describe("SettingsV2Component", () => { fixture.detectChanges(); await fixture.whenStable(); - mockAutofillSettings.defaultBrowserAutofillDisabled$.next(false); - const value = await firstValueFrom(component.showAutofillBadge$); expect(value).toBe(true); }); - it("showAutofillBadge$ emits false when default autofill IS disabled even if nudge is true", async () => { - pushActiveAccount(); - - mockNudges.showNudgeBadge$.mockImplementation((type: NudgeType) => - of(type === NudgeType.AutofillNudge), - ); - - const fixture = TestBed.createComponent(SettingsV2Component); - const component = fixture.componentInstance; - fixture.detectChanges(); - await fixture.whenStable(); - - mockAutofillSettings.defaultBrowserAutofillDisabled$.next(true); - - const value = await firstValueFrom(component.showAutofillBadge$); - expect(value).toBe(false); - }); - it("dismissBadge dismisses when showVaultBadge$ emits true", async () => { const acct = pushActiveAccount(); diff --git a/apps/browser/src/tools/popup/settings/settings-v2.component.ts b/apps/browser/src/tools/popup/settings/settings-v2.component.ts index 95aeeb2f480..e10d41b9445 100644 --- a/apps/browser/src/tools/popup/settings/settings-v2.component.ts +++ b/apps/browser/src/tools/popup/settings/settings-v2.component.ts @@ -1,16 +1,7 @@ import { CommonModule } from "@angular/common"; import { ChangeDetectionStrategy, Component } from "@angular/core"; import { RouterModule } from "@angular/router"; -import { - combineLatest, - filter, - firstValueFrom, - from, - map, - Observable, - shareReplay, - switchMap, -} from "rxjs"; +import { filter, firstValueFrom, Observable, shareReplay, switchMap } from "rxjs"; import { PremiumUpgradeDialogComponent } from "@bitwarden/angular/billing/components"; import { JslibModule } from "@bitwarden/angular/jslib.module"; @@ -28,8 +19,6 @@ import { } from "@bitwarden/components"; import { CurrentAccountComponent } from "../../../auth/popup/account-switching/current-account.component"; -import { AutofillBrowserSettingsService } from "../../../autofill/services/autofill-browser-settings.service"; -import { BrowserApi } from "../../../platform/browser/browser-api"; import { PopOutComponent } from "../../../platform/popup/components/pop-out.component"; import { PopupHeaderComponent } from "../../../platform/popup/layout/popup-header.component"; import { PopupPageComponent } from "../../../platform/popup/layout/popup-page.component"; @@ -55,12 +44,6 @@ import { PopupPageComponent } from "../../../platform/popup/layout/popup-page.co export class SettingsV2Component { NudgeType = NudgeType; - protected isBrowserAutofillSettingOverridden$ = from( - this.autofillBrowserSettingsService.isBrowserAutofillSettingOverridden( - BrowserApi.getBrowserClientVendor(window), - ), - ); - private authenticatedAccount$: Observable = this.accountService.activeAccount$.pipe( filter((account): account is Account => account !== null), shareReplay({ bufferSize: 1, refCount: true }), @@ -82,23 +65,13 @@ export class SettingsV2Component { ), ); - showAutofillBadge$: Observable = combineLatest([ - this.autofillBrowserSettingsService.defaultBrowserAutofillDisabled$, - this.authenticatedAccount$, - ]).pipe( - switchMap(([defaultBrowserAutofillDisabled, account]) => - this.nudgesService.showNudgeBadge$(NudgeType.AutofillNudge, account.id).pipe( - map((badgeStatus) => { - return !defaultBrowserAutofillDisabled && badgeStatus; - }), - ), - ), + showAutofillBadge$: Observable = this.authenticatedAccount$.pipe( + switchMap((account) => this.nudgesService.showNudgeBadge$(NudgeType.AutofillNudge, account.id)), ); constructor( private readonly nudgesService: NudgesService, private readonly accountService: AccountService, - private readonly autofillBrowserSettingsService: AutofillBrowserSettingsService, private readonly accountProfileStateService: BillingAccountProfileStateService, private readonly dialogService: DialogService, ) {} diff --git a/apps/browser/src/vault/popup/services/browser-autofill-nudge.service.spec.ts b/apps/browser/src/vault/popup/services/browser-autofill-nudge.service.spec.ts new file mode 100644 index 00000000000..40782760283 --- /dev/null +++ b/apps/browser/src/vault/popup/services/browser-autofill-nudge.service.spec.ts @@ -0,0 +1,157 @@ +import { TestBed } from "@angular/core/testing"; +import { mock, MockProxy } from "jest-mock-extended"; +import { firstValueFrom } from "rxjs"; + +import { NudgeStatus, NudgeType } from "@bitwarden/angular/vault"; +import { VaultProfileService } from "@bitwarden/angular/vault/services/vault-profile.service"; +import { BrowserClientVendors } from "@bitwarden/common/autofill/constants"; +import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; +import { StateProvider } from "@bitwarden/common/platform/state"; +import { UserId } from "@bitwarden/common/types/guid"; + +import { FakeStateProvider, mockAccountServiceWith } from "../../../../../../libs/common/spec"; +import { BrowserApi } from "../../../platform/browser/browser-api"; + +import { BrowserAutofillNudgeService } from "./browser-autofill-nudge.service"; + +describe("BrowserAutofillNudgeService", () => { + let service: BrowserAutofillNudgeService; + let vaultProfileService: MockProxy; + let fakeStateProvider: FakeStateProvider; + + const userId = "test-user-id" as UserId; + const nudgeType = NudgeType.AutofillNudge; + + const notDismissedStatus: NudgeStatus = { + hasBadgeDismissed: false, + hasSpotlightDismissed: false, + }; + + const dismissedStatus: NudgeStatus = { + hasBadgeDismissed: true, + hasSpotlightDismissed: true, + }; + + // Set profile creation date to now (new account, within 30 days) + const recentProfileDate = new Date(); + + beforeEach(() => { + vaultProfileService = mock(); + vaultProfileService.getProfileCreationDate.mockResolvedValue(recentProfileDate); + + fakeStateProvider = new FakeStateProvider(mockAccountServiceWith(userId)); + + TestBed.configureTestingModule({ + providers: [ + BrowserAutofillNudgeService, + { + provide: VaultProfileService, + useValue: vaultProfileService, + }, + { + provide: StateProvider, + useValue: fakeStateProvider, + }, + { + provide: LogService, + useValue: mock(), + }, + ], + }); + + service = TestBed.inject(BrowserAutofillNudgeService); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + describe("nudgeStatus$", () => { + it("returns parent status when browser client is Unknown", async () => { + jest + .spyOn(BrowserApi, "getBrowserClientVendor") + .mockReturnValue(BrowserClientVendors.Unknown); + jest.spyOn(BrowserApi, "browserAutofillSettingsOverridden").mockResolvedValue(true); + + const result = await firstValueFrom(service.nudgeStatus$(nudgeType, userId)); + + expect(result).toEqual(notDismissedStatus); + }); + + it("returns parent status when browser autofill is not overridden", async () => { + jest.spyOn(BrowserApi, "getBrowserClientVendor").mockReturnValue(BrowserClientVendors.Chrome); + jest.spyOn(BrowserApi, "browserAutofillSettingsOverridden").mockResolvedValue(false); + + const result = await firstValueFrom(service.nudgeStatus$(nudgeType, userId)); + + expect(result).toEqual(notDismissedStatus); + }); + + it("returns dismissed status when browser autofill is overridden", async () => { + jest.spyOn(BrowserApi, "getBrowserClientVendor").mockReturnValue(BrowserClientVendors.Chrome); + jest.spyOn(BrowserApi, "browserAutofillSettingsOverridden").mockResolvedValue(true); + + const result = await firstValueFrom(service.nudgeStatus$(nudgeType, userId)); + + expect(result).toEqual(dismissedStatus); + }); + + it("preserves parent dismissed status when account is older than 30 days", async () => { + // Set profile creation date to more than 30 days ago + const oldProfileDate = new Date(Date.now() - 31 * 24 * 60 * 60 * 1000); + vaultProfileService.getProfileCreationDate.mockResolvedValue(oldProfileDate); + + jest.spyOn(BrowserApi, "getBrowserClientVendor").mockReturnValue(BrowserClientVendors.Chrome); + jest.spyOn(BrowserApi, "browserAutofillSettingsOverridden").mockResolvedValue(false); + + const result = await firstValueFrom(service.nudgeStatus$(nudgeType, userId)); + + expect(result).toEqual(dismissedStatus); + }); + + it("combines parent dismissed and browser autofill overridden status", async () => { + // Set profile creation date to more than 30 days ago (parent dismisses) + const oldProfileDate = new Date(Date.now() - 31 * 24 * 60 * 60 * 1000); + vaultProfileService.getProfileCreationDate.mockResolvedValue(oldProfileDate); + + jest.spyOn(BrowserApi, "getBrowserClientVendor").mockReturnValue(BrowserClientVendors.Chrome); + jest.spyOn(BrowserApi, "browserAutofillSettingsOverridden").mockResolvedValue(true); + + const result = await firstValueFrom(service.nudgeStatus$(nudgeType, userId)); + + expect(result).toEqual(dismissedStatus); + }); + + it.each([ + BrowserClientVendors.Chrome, + BrowserClientVendors.Edge, + BrowserClientVendors.Opera, + BrowserClientVendors.Vivaldi, + ])("checks browser autofill settings for %s browser", async (browserVendor) => { + const getBrowserClientVendorSpy = jest + .spyOn(BrowserApi, "getBrowserClientVendor") + .mockReturnValue(browserVendor); + const browserAutofillSettingsOverriddenSpy = jest + .spyOn(BrowserApi, "browserAutofillSettingsOverridden") + .mockResolvedValue(true); + + await firstValueFrom(service.nudgeStatus$(nudgeType, userId)); + + expect(getBrowserClientVendorSpy).toHaveBeenCalledWith(window); + expect(browserAutofillSettingsOverriddenSpy).toHaveBeenCalled(); + }); + + it("does not check browser autofill settings for Unknown browser", async () => { + jest + .spyOn(BrowserApi, "getBrowserClientVendor") + .mockReturnValue(BrowserClientVendors.Unknown); + const browserAutofillSettingsOverriddenSpy = jest + .spyOn(BrowserApi, "browserAutofillSettingsOverridden") + .mockResolvedValue(true); + + await firstValueFrom(service.nudgeStatus$(nudgeType, userId)); + + expect(browserAutofillSettingsOverriddenSpy).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/apps/browser/src/vault/popup/services/browser-autofill-nudge.service.ts b/apps/browser/src/vault/popup/services/browser-autofill-nudge.service.ts new file mode 100644 index 00000000000..7fe5f527bcb --- /dev/null +++ b/apps/browser/src/vault/popup/services/browser-autofill-nudge.service.ts @@ -0,0 +1,37 @@ +import { Injectable } from "@angular/core"; +import { Observable, switchMap } from "rxjs"; + +import { NudgeStatus, NudgeType } from "@bitwarden/angular/vault"; +import { NewAccountNudgeService } from "@bitwarden/angular/vault/services/custom-nudges-services/new-account-nudge.service"; +import { BrowserClientVendors } from "@bitwarden/common/autofill/constants"; +import { UserId } from "@bitwarden/common/types/guid"; + +import { BrowserApi } from "../../../platform/browser/browser-api"; + +/** + * Browser-specific autofill nudge service. + * Extends NewAccountNudgeService (30-day account age check) and adds + * browser autofill setting detection. + * + * Nudge is dismissed if: + * - Account is older than 30 days (inherited from NewAccountNudgeService) + * - Browser's built-in password manager is already disabled via privacy settings + */ +@Injectable() +export class BrowserAutofillNudgeService extends NewAccountNudgeService { + override nudgeStatus$(nudgeType: NudgeType, userId: UserId): Observable { + return super.nudgeStatus$(nudgeType, userId).pipe( + switchMap(async (status) => { + const browserClient = BrowserApi.getBrowserClientVendor(window); + const browserAutofillOverridden = + browserClient !== BrowserClientVendors.Unknown && + (await BrowserApi.browserAutofillSettingsOverridden()); + + return { + hasBadgeDismissed: status.hasBadgeDismissed || browserAutofillOverridden, + hasSpotlightDismissed: status.hasSpotlightDismissed || browserAutofillOverridden, + }; + }), + ); + } +} diff --git a/libs/angular/src/vault/index.ts b/libs/angular/src/vault/index.ts index cb43fadb3bc..b9131338a45 100644 --- a/libs/angular/src/vault/index.ts +++ b/libs/angular/src/vault/index.ts @@ -1,3 +1,4 @@ // Note: Nudge related code is exported from `libs/angular` because it is consumed by multiple // `libs/*` packages. Exporting from the `libs/vault` package creates circular dependencies. export { NudgesService, NudgeStatus, NudgeType } from "./services/nudges.service"; +export { AUTOFILL_NUDGE_SERVICE } from "./services/nudge-injection-tokens"; diff --git a/libs/angular/src/vault/services/custom-nudges-services/index.ts b/libs/angular/src/vault/services/custom-nudges-services/index.ts index f60592b9c71..d4bfe80a525 100644 --- a/libs/angular/src/vault/services/custom-nudges-services/index.ts +++ b/libs/angular/src/vault/services/custom-nudges-services/index.ts @@ -4,3 +4,4 @@ export * from "./empty-vault-nudge.service"; export * from "./vault-settings-import-nudge.service"; export * from "./new-item-nudge.service"; export * from "./new-account-nudge.service"; +export * from "./noop-nudge.service"; diff --git a/libs/angular/src/vault/services/custom-nudges-services/noop-nudge.service.ts b/libs/angular/src/vault/services/custom-nudges-services/noop-nudge.service.ts new file mode 100644 index 00000000000..eabf1d6fc4a --- /dev/null +++ b/libs/angular/src/vault/services/custom-nudges-services/noop-nudge.service.ts @@ -0,0 +1,27 @@ +import { Injectable } from "@angular/core"; +import { Observable, of } from "rxjs"; + +import { UserId } from "@bitwarden/common/types/guid"; + +import { SingleNudgeService } from "../default-single-nudge.service"; +import { NudgeStatus, NudgeType } from "../nudges.service"; + +/** + * A no-op nudge service that always returns dismissed status. + * Use this for nudges that should be completely ignored/hidden in certain clients. + * For example, browser-specific nudges can use this as the default in non-browser clients. + */ +@Injectable({ providedIn: "root" }) +export class NoOpNudgeService implements SingleNudgeService { + nudgeStatus$(_nudgeType: NudgeType, _userId: UserId): Observable { + return of({ hasBadgeDismissed: true, hasSpotlightDismissed: true }); + } + + async setNudgeStatus( + _nudgeType: NudgeType, + _newStatus: NudgeStatus, + _userId: UserId, + ): Promise { + // No-op: state changes are ignored + } +} diff --git a/libs/angular/src/vault/services/nudge-injection-tokens.ts b/libs/angular/src/vault/services/nudge-injection-tokens.ts new file mode 100644 index 00000000000..52a0838d356 --- /dev/null +++ b/libs/angular/src/vault/services/nudge-injection-tokens.ts @@ -0,0 +1,7 @@ +import { InjectionToken } from "@angular/core"; + +import { SingleNudgeService } from "./default-single-nudge.service"; + +export const AUTOFILL_NUDGE_SERVICE = new InjectionToken( + "AutofillNudgeService", +); diff --git a/libs/angular/src/vault/services/nudges.service.ts b/libs/angular/src/vault/services/nudges.service.ts index 05d565eb499..19acf690d32 100644 --- a/libs/angular/src/vault/services/nudges.service.ts +++ b/libs/angular/src/vault/services/nudges.service.ts @@ -12,8 +12,10 @@ import { NewItemNudgeService, AccountSecurityNudgeService, VaultSettingsImportNudgeService, + NoOpNudgeService, } from "./custom-nudges-services"; import { DefaultSingleNudgeService, SingleNudgeService } from "./default-single-nudge.service"; +import { AUTOFILL_NUDGE_SERVICE } from "./nudge-injection-tokens"; export type NudgeStatus = { hasBadgeDismissed: boolean; @@ -56,6 +58,12 @@ export class NudgesService { private newItemNudgeService = inject(NewItemNudgeService); private newAcctNudgeService = inject(NewAccountNudgeService); + // NoOp service that always returns dismissed + private noOpNudgeService = inject(NoOpNudgeService); + + // Optional Browser-specific service provided via injection token (not all clients have autofill) + private autofillNudgeService = inject(AUTOFILL_NUDGE_SERVICE, { optional: true }); + /** * Custom nudge services to use for specific nudge types * Each nudge type can have its own service to determine when to show the nudge @@ -66,7 +74,7 @@ export class NudgesService { [NudgeType.EmptyVaultNudge]: inject(EmptyVaultNudgeService), [NudgeType.VaultSettingsImportNudge]: inject(VaultSettingsImportNudgeService), [NudgeType.AccountSecurity]: inject(AccountSecurityNudgeService), - [NudgeType.AutofillNudge]: this.newAcctNudgeService, + [NudgeType.AutofillNudge]: this.autofillNudgeService ?? this.noOpNudgeService, [NudgeType.DownloadBitwarden]: this.newAcctNudgeService, [NudgeType.GeneratorNudgeStatus]: this.newAcctNudgeService, [NudgeType.NewLoginItemStatus]: this.newItemNudgeService, From cf1c3226c3f0d9002ec6454330e7f8d1366941c3 Mon Sep 17 00:00:00 2001 From: aj-bw <81774843+aj-bw@users.noreply.github.com> Date: Tue, 30 Dec 2025 09:07:32 +0000 Subject: [PATCH 4/8] replace inline removal with reusable workflow (#18144) --- .github/workflows/build-desktop.yml | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/.github/workflows/build-desktop.yml b/.github/workflows/build-desktop.yml index f3cdf80f710..45297a110a0 100644 --- a/.github/workflows/build-desktop.yml +++ b/.github/workflows/build-desktop.yml @@ -179,18 +179,8 @@ jobs: ref: ${{ github.event.pull_request.head.sha }} persist-credentials: false - - name: Free disk space for build - run: | - sudo rm -rf /usr/share/dotnet - sudo rm -rf /usr/share/swift - sudo rm -rf /usr/local/.ghcup - sudo rm -rf /usr/share/miniconda - sudo rm -rf /usr/share/az_* - sudo rm -rf /usr/local/julia* - sudo rm -rf /usr/lib/mono - sudo rm -rf /usr/lib/heroku - sudo rm -rf /usr/local/aws-cli - sudo rm -rf /usr/local/aws-sam-cli + - name: Free disk space + uses: bitwarden/gh-actions/free-disk-space@main - name: Set up Node uses: actions/setup-node@a0853c24544627f65ddf259abe73b1d18a591444 # v5.0.0 From 8a6f9bfaeb84aa07b6c4d5cdd61305e4373d9b93 Mon Sep 17 00:00:00 2001 From: Daniel Riera Date: Tue, 30 Dec 2025 10:36:08 -0500 Subject: [PATCH 5/8] [PM-29515] Remove ts strict ignore in overlay inline menu iframe content autofill inline menu iframe service (#18030) * use optional chaining and make portkey optional to match the AutofillInlineMenuIframeExtensionMessage * make ariaAlertElement optional * tiemouts are set to null for clearing, updated type to match this * border color is conditionally applied, undefined is acceptable here * check if aria alerts exist before calling * return early if no styles exist for updateElementStyles or no position for updateIframePosition * initilaize timers to null * non null assert iframe since it is initialized in initMenuIframe which makes it safe to assert non null by lifecycle * remove optional chainning --- .../autofill-inline-menu-iframe.service.ts | 39 +++++++++++-------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/apps/browser/src/autofill/overlay/inline-menu/iframe-content/autofill-inline-menu-iframe.service.ts b/apps/browser/src/autofill/overlay/inline-menu/iframe-content/autofill-inline-menu-iframe.service.ts index 64ef7d180ed..ad1241e98d2 100644 --- a/apps/browser/src/autofill/overlay/inline-menu/iframe-content/autofill-inline-menu-iframe.service.ts +++ b/apps/browser/src/autofill/overlay/inline-menu/iframe-content/autofill-inline-menu-iframe.service.ts @@ -1,5 +1,3 @@ -// FIXME: Update this file to be type safe and remove this and next line -// @ts-strict-ignore import { EVENTS } from "@bitwarden/common/autofill/constants"; import { ThemeTypes } from "@bitwarden/common/platform/enums"; @@ -15,14 +13,17 @@ export class AutofillInlineMenuIframeService implements AutofillInlineMenuIframe private readonly setElementStyles = setElementStyles; private readonly sendExtensionMessage = sendExtensionMessage; private port: chrome.runtime.Port | null = null; - private portKey: string; + private portKey?: string; private readonly extensionOrigin: string; private iframeMutationObserver: MutationObserver; - private iframe: HTMLIFrameElement; - private ariaAlertElement: HTMLDivElement; - private ariaAlertTimeout: number | NodeJS.Timeout; - private delayedCloseTimeout: number | NodeJS.Timeout; - private fadeInTimeout: number | NodeJS.Timeout; + /** + * Initialized in initMenuIframe which makes it safe to assert non null by lifecycle. + */ + private iframe!: HTMLIFrameElement; + private ariaAlertElement?: HTMLDivElement; + private ariaAlertTimeout: number | NodeJS.Timeout | null = null; + private delayedCloseTimeout: number | NodeJS.Timeout | null = null; + private fadeInTimeout: number | NodeJS.Timeout | null = null; private readonly fadeInOpacityTransition = "opacity 125ms ease-out 0s"; private readonly fadeOutOpacityTransition = "opacity 65ms ease-out 0s"; private iframeStyles: Partial = { @@ -50,7 +51,7 @@ export class AutofillInlineMenuIframeService implements AutofillInlineMenuIframe }; private foreignMutationsCount = 0; private mutationObserverIterations = 0; - private mutationObserverIterationsResetTimeout: number | NodeJS.Timeout; + private mutationObserverIterationsResetTimeout: number | NodeJS.Timeout | null = null; private readonly backgroundPortMessageHandlers: BackgroundPortMessageHandlers = { initAutofillInlineMenuButton: ({ message }) => this.initAutofillInlineMenu(message), initAutofillInlineMenuList: ({ message }) => this.initAutofillInlineMenu(message), @@ -134,7 +135,9 @@ export class AutofillInlineMenuIframeService implements AutofillInlineMenuIframe this.port.onDisconnect.addListener(this.handlePortDisconnect); this.port.onMessage.addListener(this.handlePortMessage); - this.announceAriaAlert(this.ariaAlert, 2000); + if (this.ariaAlert) { + this.announceAriaAlert(this.ariaAlert, 2000); + } }; /** @@ -155,7 +158,7 @@ export class AutofillInlineMenuIframeService implements AutofillInlineMenuIframe this.ariaAlertTimeout = globalThis.setTimeout(async () => { const isFieldFocused = await this.sendExtensionMessage("checkIsFieldCurrentlyFocused"); - if (isFieldFocused || triggeredByUser) { + if ((isFieldFocused || triggeredByUser) && this.ariaAlertElement) { this.shadow.appendChild(this.ariaAlertElement); } this.ariaAlertTimeout = null; @@ -242,7 +245,7 @@ export class AutofillInlineMenuIframeService implements AutofillInlineMenuIframe */ private initAutofillInlineMenuList(message: AutofillInlineMenuIframeExtensionMessage) { const { theme } = message; - let borderColor: string; + let borderColor: string | undefined; let verifiedTheme = theme; if (verifiedTheme === ThemeTypes.System) { verifiedTheme = globalThis.matchMedia("(prefers-color-scheme: dark)").matches @@ -274,8 +277,8 @@ export class AutofillInlineMenuIframeService implements AutofillInlineMenuIframe * * @param position - The position styles to apply to the iframe */ - private updateIframePosition(position: Partial) { - if (!globalThis.document.hasFocus()) { + private updateIframePosition(position?: Partial) { + if (!position || !globalThis.document.hasFocus()) { return; } @@ -295,7 +298,9 @@ export class AutofillInlineMenuIframeService implements AutofillInlineMenuIframe this.handleFadeInInlineMenuIframe(); } - this.announceAriaAlert(this.ariaAlert, 2000); + if (this.ariaAlert) { + this.announceAriaAlert(this.ariaAlert, 2000); + } } /** @@ -359,8 +364,8 @@ export class AutofillInlineMenuIframeService implements AutofillInlineMenuIframe * @param customElement - The element to update the styles for * @param styles - The styles to apply to the element */ - private updateElementStyles(customElement: HTMLElement, styles: Partial) { - if (!customElement) { + private updateElementStyles(customElement: HTMLElement, styles?: Partial) { + if (!customElement || !styles) { return; } From cee69f85c0da401c6ec572593e15b536cc6d78e9 Mon Sep 17 00:00:00 2001 From: Ben Brooks <56796209+bensbits91@users.noreply.github.com> Date: Tue, 30 Dec 2025 08:21:10 -0800 Subject: [PATCH 6/8] [pm-28077] Add input types to ignoredInputTypes (#17870) * [pm-28077] Add input types to ignoredInputTypes Signed-off-by: Ben Brooks * Merge branch 'main' of github.com:bitwarden/clients into pm-28077-more-ignoredInputTypes-in-CollectAutofillContentService Signed-off-by: Ben Brooks * [pm-28077] Remove month input type from ignored types Signed-off-by: Ben Brooks * [pm-28077] Remove month radio and checkbox types from ignored types Signed-off-by: Ben Brooks * Merge branch 'main' of github.com:bitwarden/clients into pm-28077-more-ignoredInputTypes-in-CollectAutofillContentService Signed-off-by: Ben Brooks * [pm-28077] Fix prettier issues/conflicts Signed-off-by: Ben Brooks * [pm-28077] Add comment regarding datetime depcrecation Signed-off-by: Ben Brooks --------- Signed-off-by: Ben Brooks --- .../services/collect-autofill-content.service.ts | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/apps/browser/src/autofill/services/collect-autofill-content.service.ts b/apps/browser/src/autofill/services/collect-autofill-content.service.ts index 367599f7ad0..18eb8e2baf8 100644 --- a/apps/browser/src/autofill/services/collect-autofill-content.service.ts +++ b/apps/browser/src/autofill/services/collect-autofill-content.service.ts @@ -60,6 +60,15 @@ export class CollectAutofillContentService implements CollectAutofillContentServ "button", "image", "file", + "search", + "url", + "date", + "time", + "datetime", // Note: datetime is deprecated in HTML5; keeping here for backwards compatibility + "datetime-local", + "week", + "color", + "range", ]); constructor( From 800a21d8a3b65369528610ac79517e7a5792e48a Mon Sep 17 00:00:00 2001 From: Leslie Tilton <23057410+Banrion@users.noreply.github.com> Date: Tue, 30 Dec 2025 11:06:30 -0600 Subject: [PATCH 7/8] [PM-28548] Phishing Blocker support links (#18070) * Change domain terminology to web addresses * Added phishing resource file * Finish renaming and adding runtime configuration for domains vs links setting * Update reference * Add matching functions per resource * correct URL matching logic for links-based detection Problem: The phishing link matcher was failing to detect known phishing URLs due to two issues: 1. Protocol mismatch: Entries in the phishing list use `http://` but users typically visit `https://` versions. The matcher was comparing full URLs including protocol, causing legitimate matches to fail. - List entry: `http://smartdapptradxx.pages.dev` - User visits: `https://smartdapptradxx.pages.dev/` - Result: No match (incorrect) 2. Hostname-only matching would have caused false positives: An earlier attempt to fix #1 included hostname-only comparison, which defeats the purpose of links-based detection. The goal of PM-28548 is precise URL matching to avoid blocking entire domains (like pages.dev, github.io) when only specific paths are malicious. Solution: - Always strip protocol (http:// or https://) from both entry and URL before comparison, treating them as equivalent - Remove hostname-only matching to maintain precision - Keep prefix matching for subpaths, query strings, and fragments --------- Co-authored-by: Alex --- .../phishing-detection/phishing-resources.ts | 98 ++++++++++++++++ .../services/phishing-data.service.spec.ts | 62 +++++----- .../services/phishing-data.service.ts | 107 +++++++++--------- .../services/phishing-detection.service.ts | 2 +- 4 files changed, 186 insertions(+), 83 deletions(-) create mode 100644 apps/browser/src/dirt/phishing-detection/phishing-resources.ts diff --git a/apps/browser/src/dirt/phishing-detection/phishing-resources.ts b/apps/browser/src/dirt/phishing-detection/phishing-resources.ts new file mode 100644 index 00000000000..262d6cf833b --- /dev/null +++ b/apps/browser/src/dirt/phishing-detection/phishing-resources.ts @@ -0,0 +1,98 @@ +export type PhishingResource = { + name?: string; + remoteUrl: string; + checksumUrl: string; + todayUrl: string; + /** Matcher used to decide whether a given URL matches an entry from this resource */ + match: (url: URL, entry: string) => boolean; +}; + +export const PhishingResourceType = Object.freeze({ + Domains: "domains", + Links: "links", +} as const); + +export type PhishingResourceType = (typeof PhishingResourceType)[keyof typeof PhishingResourceType]; + +export const PHISHING_RESOURCES: Record = { + [PhishingResourceType.Domains]: [ + { + name: "Phishing.Database Domains", + 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: + "https://raw.githubusercontent.com/Phishing-Database/Phishing.Database/refs/heads/master/phishing-domains-NEW-today.txt", + match: (url: URL, entry: string) => { + if (!entry) { + return false; + } + const candidate = entry.trim().toLowerCase().replace(/\/$/, ""); + // If entry contains a scheme, strip it for comparison + const e = candidate.replace(/^https?:\/\//, ""); + // Compare against hostname or host+path + if (e === url.hostname.toLowerCase()) { + return true; + } + const urlNoProto = url.href + .toLowerCase() + .replace(/https?:\/\//, "") + .replace(/\/$/, ""); + return urlNoProto === e || urlNoProto.startsWith(e + "/"); + }, + }, + ], + [PhishingResourceType.Links]: [ + { + name: "Phishing.Database Links", + 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: + "https://raw.githubusercontent.com/Phishing-Database/Phishing.Database/refs/heads/master/phishing-links-NEW-today.txt", + match: (url: URL, entry: string) => { + if (!entry) { + return false; + } + // Basic HTML entity decode for common cases (the lists sometimes contain &) + const decodeHtml = (s: string) => s.replace(/&/g, "&"); + + const normalizedEntry = decodeHtml(entry.trim()).toLowerCase().replace(/\/$/, ""); + + // Normalize URL for comparison - always strip protocol for consistent matching + const normalizedUrl = decodeHtml(url.href).toLowerCase().replace(/\/$/, ""); + const urlNoProto = normalizedUrl.replace(/^https?:\/\//, ""); + + // Strip protocol from entry if present (http:// and https:// should be treated as equivalent) + const entryNoProto = normalizedEntry.replace(/^https?:\/\//, ""); + + // Compare full path (without protocol) - exact match + if (urlNoProto === entryNoProto) { + return true; + } + + // Check if URL starts with entry (prefix match for subpaths/query/hash) + // e.g., entry "site.com/phish" matches "site.com/phish/subpage" or "site.com/phish?id=1" + if ( + urlNoProto.startsWith(entryNoProto + "/") || + urlNoProto.startsWith(entryNoProto + "?") || + urlNoProto.startsWith(entryNoProto + "#") + ) { + return true; + } + + return false; + }, + }, + ], +}; + +export function getPhishingResources( + type: PhishingResourceType, + index = 0, +): PhishingResource | undefined { + const list = PHISHING_RESOURCES[type] ?? []; + return list[index]; +} diff --git a/apps/browser/src/dirt/phishing-detection/services/phishing-data.service.spec.ts b/apps/browser/src/dirt/phishing-detection/services/phishing-data.service.spec.ts index 94f3e99f8be..30aa947092d 100644 --- a/apps/browser/src/dirt/phishing-detection/services/phishing-data.service.spec.ts +++ b/apps/browser/src/dirt/phishing-detection/services/phishing-data.service.spec.ts @@ -25,7 +25,7 @@ describe("PhishingDataService", () => { }; let fetchChecksumSpy: jest.SpyInstance; - let fetchDomainsSpy: jest.SpyInstance; + let fetchWebAddressesSpy: jest.SpyInstance; beforeEach(() => { jest.useFakeTimers(); @@ -45,113 +45,113 @@ describe("PhishingDataService", () => { platformUtilsService, ); - fetchChecksumSpy = jest.spyOn(service as any, "fetchPhishingDomainsChecksum"); - fetchDomainsSpy = jest.spyOn(service as any, "fetchPhishingDomains"); + fetchChecksumSpy = jest.spyOn(service as any, "fetchPhishingChecksum"); + fetchWebAddressesSpy = jest.spyOn(service as any, "fetchPhishingWebAddresses"); }); - describe("isPhishingDomains", () => { - it("should detect a phishing domain", async () => { + describe("isPhishingWebAddress", () => { + it("should detect a phishing web address", async () => { setMockState({ - domains: ["phish.com", "badguy.net"], + webAddresses: ["phish.com", "badguy.net"], timestamp: Date.now(), checksum: "abc123", applicationVersion: "1.0.0", }); const url = new URL("http://phish.com"); - const result = await service.isPhishingDomain(url); + const result = await service.isPhishingWebAddress(url); expect(result).toBe(true); }); - it("should not detect a safe domain", async () => { + it("should not detect a safe web address", async () => { setMockState({ - domains: ["phish.com", "badguy.net"], + webAddresses: ["phish.com", "badguy.net"], timestamp: Date.now(), checksum: "abc123", applicationVersion: "1.0.0", }); const url = new URL("http://safe.com"); - const result = await service.isPhishingDomain(url); + const result = await service.isPhishingWebAddress(url); expect(result).toBe(false); }); - it("should match against root domain", async () => { + it("should match against root web address", async () => { setMockState({ - domains: ["phish.com", "badguy.net"], + webAddresses: ["phish.com", "badguy.net"], timestamp: Date.now(), checksum: "abc123", applicationVersion: "1.0.0", }); const url = new URL("http://phish.com/about"); - const result = await service.isPhishingDomain(url); + const result = await service.isPhishingWebAddress(url); expect(result).toBe(true); }); it("should not error on empty state", async () => { setMockState(undefined as any); const url = new URL("http://phish.com/about"); - const result = await service.isPhishingDomain(url); + const result = await service.isPhishingWebAddress(url); expect(result).toBe(false); }); }); - describe("getNextDomains", () => { - it("refetches all domains if applicationVersion has changed", async () => { + describe("getNextWebAddresses", () => { + it("refetches all web addresses if applicationVersion has changed", async () => { const prev: PhishingData = { - domains: ["a.com"], + webAddresses: ["a.com"], timestamp: Date.now() - 60000, checksum: "old", applicationVersion: "1.0.0", }; fetchChecksumSpy.mockResolvedValue("new"); - fetchDomainsSpy.mockResolvedValue(["d.com", "e.com"]); + fetchWebAddressesSpy.mockResolvedValue(["d.com", "e.com"]); platformUtilsService.getApplicationVersion.mockResolvedValue("2.0.0"); - const result = await service.getNextDomains(prev); + const result = await service.getNextWebAddresses(prev); - expect(result!.domains).toEqual(["d.com", "e.com"]); + expect(result!.webAddresses).toEqual(["d.com", "e.com"]); expect(result!.checksum).toBe("new"); expect(result!.applicationVersion).toBe("2.0.0"); }); it("only updates timestamp if checksum matches", async () => { const prev: PhishingData = { - domains: ["a.com"], + webAddresses: ["a.com"], timestamp: Date.now() - 60000, checksum: "abc", applicationVersion: "1.0.0", }; fetchChecksumSpy.mockResolvedValue("abc"); - const result = await service.getNextDomains(prev); - expect(result!.domains).toEqual(prev.domains); + const result = await service.getNextWebAddresses(prev); + 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 () => { const prev: PhishingData = { - domains: ["a.com"], + webAddresses: ["a.com"], timestamp: Date.now() - 60000, checksum: "old", applicationVersion: "1.0.0", }; fetchChecksumSpy.mockResolvedValue("new"); - fetchDomainsSpy.mockResolvedValue(["b.com", "c.com"]); - const result = await service.getNextDomains(prev); - expect(result!.domains).toEqual(["a.com", "b.com", "c.com"]); + fetchWebAddressesSpy.mockResolvedValue(["b.com", "c.com"]); + const result = await service.getNextWebAddresses(prev); + expect(result!.webAddresses).toEqual(["a.com", "b.com", "c.com"]); expect(result!.checksum).toBe("new"); }); it("fetches all domains if cache is old", async () => { const prev: PhishingData = { - domains: ["a.com"], + webAddresses: ["a.com"], timestamp: Date.now() - 2 * 24 * 60 * 60 * 1000, checksum: "old", applicationVersion: "1.0.0", }; fetchChecksumSpy.mockResolvedValue("new"); - fetchDomainsSpy.mockResolvedValue(["d.com", "e.com"]); - const result = await service.getNextDomains(prev); - expect(result!.domains).toEqual(["d.com", "e.com"]); + fetchWebAddressesSpy.mockResolvedValue(["d.com", "e.com"]); + const result = await service.getNextWebAddresses(prev); + expect(result!.webAddresses).toEqual(["d.com", "e.com"]); expect(result!.checksum).toBe("new"); }); }); diff --git a/apps/browser/src/dirt/phishing-detection/services/phishing-data.service.ts b/apps/browser/src/dirt/phishing-detection/services/phishing-data.service.ts index 6e1bf07c647..21fe74f1873 100644 --- a/apps/browser/src/dirt/phishing-detection/services/phishing-data.service.ts +++ b/apps/browser/src/dirt/phishing-detection/services/phishing-data.service.ts @@ -20,14 +20,16 @@ import { ScheduledTaskNames, TaskSchedulerService } from "@bitwarden/common/plat import { LogService } from "@bitwarden/logging"; import { GlobalStateProvider, KeyDefinition, PHISHING_DETECTION_DISK } from "@bitwarden/state"; +import { getPhishingResources, PhishingResourceType } from "../phishing-resources"; + export type PhishingData = { - domains: string[]; + webAddresses: string[]; timestamp: number; checksum: string; /** * We store the application version to refetch the entire dataset on a new client release. - * This counteracts daily appends updates not removing inactive or false positive domains. + * This counteracts daily appends updates not removing inactive or false positive web addresses. */ applicationVersion: string; }; @@ -37,34 +39,27 @@ export const PHISHING_DOMAINS_KEY = new KeyDefinition( "phishingDomains", { deserializer: (value: PhishingData) => - value ?? { domains: [], timestamp: 0, checksum: "", applicationVersion: "" }, + value ?? { webAddresses: [], timestamp: 0, checksum: "", applicationVersion: "" }, }, ); -/** Coordinates fetching, caching, and patching of known phishing domains */ +/** Coordinates fetching, caching, and patching of known phishing web addresses */ export class PhishingDataService { - private static readonly RemotePhishingDatabaseUrl = - "https://raw.githubusercontent.com/Phishing-Database/Phishing.Database/master/phishing-domains-ACTIVE.txt"; - private static readonly RemotePhishingDatabaseChecksumUrl = - "https://raw.githubusercontent.com/Phishing-Database/checksums/refs/heads/master/phishing-domains-ACTIVE.txt.md5"; - private static readonly RemotePhishingDatabaseTodayUrl = - "https://raw.githubusercontent.com/Phishing-Database/Phishing.Database/refs/heads/master/phishing-domains-NEW-today.txt"; - - private _testDomains = this.getTestDomains(); + private _testWebAddresses = this.getTestWebAddresses(); private _cachedState = this.globalStateProvider.get(PHISHING_DOMAINS_KEY); - private _domains$ = this._cachedState.state$.pipe( + private _webAddresses$ = this._cachedState.state$.pipe( map( (state) => new Set( - (state?.domains?.filter((line) => line.trim().length > 0) ?? []).concat( - this._testDomains, + (state?.webAddresses?.filter((line) => line.trim().length > 0) ?? []).concat( + this._testWebAddresses, "phishing.testcategory.com", // Included for QA to test in prod ), ), ), ); - // How often are new domains added to the remote? + // How often are new web addresses added to the remote? readonly UPDATE_INTERVAL_DURATION = 24 * 60 * 60 * 1000; // 24 hours private _triggerUpdate$ = new Subject(); @@ -75,7 +70,7 @@ export class PhishingDataService { 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.getNextDomains(cachedState); + const next = await this.getNextWebAddresses(cachedState); if (next) { await this._cachedState.update(() => next); this.logService.info(`[PhishingDataService] cache updated`); @@ -85,7 +80,7 @@ export class PhishingDataService { count: 3, delay: (err, count) => { this.logService.error( - `[PhishingDataService] Unable to update domains. Attempt ${count}.`, + `[PhishingDataService] Unable to update web addresses. Attempt ${count}.`, err, ); return timer(5 * 60 * 1000); // 5 minutes @@ -97,7 +92,7 @@ export class PhishingDataService { 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 domains.", + "[PhishingDataService] Retries unsuccessful. Unable to update web addresses.", err, ); return EMPTY; @@ -114,6 +109,7 @@ export class PhishingDataService { private globalStateProvider: GlobalStateProvider, private logService: LogService, private platformUtilsService: PlatformUtilsService, + private resourceType: PhishingResourceType = PhishingResourceType.Links, ) { this.taskSchedulerService.registerTaskHandler(ScheduledTaskNames.phishingDomainUpdate, () => { this._triggerUpdate$.next(); @@ -125,22 +121,31 @@ export class PhishingDataService { } /** - * Checks if the given URL is a known phishing domain + * Checks if the given URL is a known phishing web address * * @param url The URL to check - * @returns True if the URL is a known phishing domain, false otherwise + * @returns True if the URL is a known phishing web address, false otherwise */ - async isPhishingDomain(url: URL): Promise { - const domains = await firstValueFrom(this._domains$); - const result = domains.has(url.hostname); - if (result) { - return true; + async isPhishingWebAddress(url: URL): Promise { + // 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)) { + return true; + } + } + return false; } - return false; + + // Default/domain behavior: exact hostname match as a fallback + return entries.has(url.hostname); } - async getNextDomains(prev: PhishingData | null): Promise { - prev = prev ?? { domains: [], timestamp: 0, checksum: "", applicationVersion: "" }; + async getNextWebAddresses(prev: PhishingData | null): Promise { + prev = prev ?? { webAddresses: [], timestamp: 0, checksum: "", applicationVersion: "" }; const timestamp = Date.now(); const prevAge = timestamp - prev.timestamp; this.logService.info(`[PhishingDataService] Cache age: ${prevAge}`); @@ -148,7 +153,7 @@ export class PhishingDataService { const applicationVersion = await this.platformUtilsService.getApplicationVersion(); // If checksum matches, return existing data with new timestamp & version - const remoteChecksum = await this.fetchPhishingDomainsChecksum(); + const remoteChecksum = await this.fetchPhishingChecksum(this.resourceType); if (remoteChecksum && prev.checksum === remoteChecksum) { this.logService.info( `[PhishingDataService] Remote checksum matches local checksum, updating timestamp only.`, @@ -157,66 +162,66 @@ export class PhishingDataService { } // Checksum is different, data needs to be updated. - // Approach 1: Fetch only new domains and append + // Approach 1: Fetch only new web addresses and append const isOneDayOldMax = prevAge <= this.UPDATE_INTERVAL_DURATION; if (isOneDayOldMax && applicationVersion === prev.applicationVersion) { - const dailyDomains: string[] = await this.fetchPhishingDomains( - PhishingDataService.RemotePhishingDatabaseTodayUrl, - ); + const webAddressesTodayUrl = getPhishingResources(this.resourceType)!.todayUrl; + const dailyWebAddresses: string[] = + await this.fetchPhishingWebAddresses(webAddressesTodayUrl); this.logService.info( - `[PhishingDataService] ${dailyDomains.length} new phishing domains added`, + `[PhishingDataService] ${dailyWebAddresses.length} new phishing web addresses added`, ); return { - domains: prev.domains.concat(dailyDomains), + webAddresses: prev.webAddresses.concat(dailyWebAddresses), checksum: remoteChecksum, timestamp, applicationVersion, }; } - // Approach 2: Fetch all domains - const domains = await this.fetchPhishingDomains(PhishingDataService.RemotePhishingDatabaseUrl); + // Approach 2: Fetch all web addresses + const remoteUrl = getPhishingResources(this.resourceType)!.remoteUrl; + const remoteWebAddresses = await this.fetchPhishingWebAddresses(remoteUrl); return { - domains, + webAddresses: remoteWebAddresses, timestamp, checksum: remoteChecksum, applicationVersion, }; } - private async fetchPhishingDomainsChecksum() { - const response = await this.apiService.nativeFetch( - new Request(PhishingDataService.RemotePhishingDatabaseChecksumUrl), - ); + private async fetchPhishingChecksum(type: PhishingResourceType = PhishingResourceType.Domains) { + const checksumUrl = getPhishingResources(type)!.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(); } - private async fetchPhishingDomains(url: 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 domains: ${response.status}`); + throw new Error(`[PhishingDataService] Failed to fetch web addresses: ${response.status}`); } return response.text().then((text) => text.split("\n")); } - private getTestDomains() { + private getTestWebAddresses() { const flag = devFlagEnabled("testPhishingUrls"); if (!flag) { return []; } - const domains = devFlagValue("testPhishingUrls") as unknown[]; - if (domains && domains instanceof Array) { + 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 domains:", - domains, + "[PhishingDetectionService] Dev flag enabled for testing phishing detection. Adding test phishing web addresses:", + webAddresses, ); - return domains as string[]; + return webAddresses as string[]; } return []; } diff --git a/apps/browser/src/dirt/phishing-detection/services/phishing-detection.service.ts b/apps/browser/src/dirt/phishing-detection/services/phishing-detection.service.ts index e04d08559ab..d90e872eef8 100644 --- a/apps/browser/src/dirt/phishing-detection/services/phishing-detection.service.ts +++ b/apps/browser/src/dirt/phishing-detection/services/phishing-detection.service.ts @@ -94,7 +94,7 @@ export class PhishingDetectionService { this._ignoredHostnames.delete(url.hostname); return; } - const isPhishing = await phishingDataService.isPhishingDomain(url); + const isPhishing = await phishingDataService.isPhishingWebAddress(url); if (!isPhishing) { return; } From 5b3e083af3013a1c68eafc819115f7de3f234df6 Mon Sep 17 00:00:00 2001 From: Mick Letofsky Date: Tue, 30 Dec 2025 18:14:54 +0100 Subject: [PATCH 8/8] Review Code Triggered by labeled event (#18151) --- .github/workflows/review-code.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/review-code.yml b/.github/workflows/review-code.yml index 0e0597fccf0..908664209d8 100644 --- a/.github/workflows/review-code.yml +++ b/.github/workflows/review-code.yml @@ -2,7 +2,7 @@ name: Code Review on: pull_request: - types: [opened, synchronize, reopened, ready_for_review] + types: [opened, labeled] permissions: {}