From 37e49acab2f80ddb7a15360b47efa2b78ca9d9c6 Mon Sep 17 00:00:00 2001 From: Jonathan Prusik Date: Tue, 6 Jan 2026 13:01:07 -0500 Subject: [PATCH] add branching qualification logic for cipher notifications --- .../overlay-notifications.background.ts | 16 +++ .../overlay-notifications.background.spec.ts | 126 ++++++++++++++++++ .../overlay-notifications.background.ts | 71 ++++++---- .../abstractions/notification-bar.ts | 4 + 4 files changed, 193 insertions(+), 24 deletions(-) diff --git a/apps/browser/src/autofill/background/abstractions/overlay-notifications.background.ts b/apps/browser/src/autofill/background/abstractions/overlay-notifications.background.ts index 009f5ced3e3..dcc2520043c 100644 --- a/apps/browser/src/autofill/background/abstractions/overlay-notifications.background.ts +++ b/apps/browser/src/autofill/background/abstractions/overlay-notifications.background.ts @@ -2,6 +2,7 @@ import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; import { SecurityTask } from "@bitwarden/common/vault/tasks"; import AutofillPageDetails from "../../models/autofill-page-details"; +import { NotificationTypes } from "../../notification/abstractions/notification-bar"; export type NotificationTypeData = { isVaultLocked?: boolean; @@ -17,6 +18,21 @@ export type LoginSecurityTaskInfo = { uri: ModifyLoginCipherFormData["uri"]; }; +/** + * Distinguished from `NotificationTypes` in that this represents the + * pre-resolved notification scenario, vs the notification component + * (e.g. "Add" and "Cipher" will be removed post-`useFullCipherTriggeringLogic` + * migration) + */ +export const NotificationScenarios = { + ...NotificationTypes, + /** represents scenarios handling saving new and updated ciphers after form submit */ + Cipher: "cipher", +} as const; + +export type NotificationScenario = + (typeof NotificationScenarios)[keyof typeof NotificationScenarios]; + export type WebsiteOriginsWithFields = Map>; export type ActiveFormSubmissionRequests = Set; diff --git a/apps/browser/src/autofill/background/overlay-notifications.background.spec.ts b/apps/browser/src/autofill/background/overlay-notifications.background.spec.ts index c596a1ba774..8fc34398c24 100644 --- a/apps/browser/src/autofill/background/overlay-notifications.background.spec.ts +++ b/apps/browser/src/autofill/background/overlay-notifications.background.spec.ts @@ -323,6 +323,7 @@ describe("OverlayNotificationsBackground", () => { const pageDetails = mock({ fields: [mock()] }); let notificationChangedPasswordSpy: jest.SpyInstance; let notificationAddLoginSpy: jest.SpyInstance; + let cipherNotificationSpy: jest.SpyInstance; beforeEach(async () => { sender = mock({ @@ -334,6 +335,7 @@ describe("OverlayNotificationsBackground", () => { "triggerChangedPasswordNotification", ); notificationAddLoginSpy = jest.spyOn(notificationBackground, "triggerAddLoginNotification"); + cipherNotificationSpy = jest.spyOn(notificationBackground, "triggerCipherNotification"); sendMockExtensionMessage( { command: "collectPageDetailsResponse", details: pageDetails }, @@ -456,6 +458,7 @@ describe("OverlayNotificationsBackground", () => { const pageDetails = mock({ fields: [mock()] }); beforeEach(async () => { + overlayNotificationsBackground["useUndiscoveredCipherScenarioTriggeringLogic"] = false; sendMockExtensionMessage( { command: "collectPageDetailsResponse", details: pageDetails }, sender, @@ -519,6 +522,44 @@ describe("OverlayNotificationsBackground", () => { expect(notificationAddLoginSpy).toHaveBeenCalled(); }); + it("with `useUndiscoveredCipherScenarioTriggeringLogic` on, waits for the tab's navigation to complete using the web navigation API before initializing the notification", async () => { + overlayNotificationsBackground["useUndiscoveredCipherScenarioTriggeringLogic"] = true; + chrome.tabs.get = jest.fn().mockImplementationOnce((tabId, callback) => { + callback( + mock({ + status: "loading", + url: sender.url, + }), + ); + }); + triggerWebRequestOnCompletedEvent( + mock({ + url: sender.url, + tabId: sender.tab.id, + requestId, + }), + ); + await flushPromises(); + + chrome.tabs.get = jest.fn().mockImplementationOnce((tabId, callback) => { + callback( + mock({ + status: "complete", + url: sender.url, + }), + ); + }); + triggerWebNavigationOnCompletedEvent( + mock({ + tabId: sender.tab.id, + url: sender.url, + }), + ); + await flushPromises(); + + expect(cipherNotificationSpy).toHaveBeenCalled(); + }); + it("initializes the notification immediately when the tab's navigation is complete", async () => { sendMockExtensionMessage( { @@ -552,6 +593,40 @@ describe("OverlayNotificationsBackground", () => { expect(notificationAddLoginSpy).toHaveBeenCalled(); }); + it("with `useUndiscoveredCipherScenarioTriggeringLogic` on, initializes the notification immediately when the tab's navigation is complete", async () => { + overlayNotificationsBackground["useUndiscoveredCipherScenarioTriggeringLogic"] = true; + sendMockExtensionMessage( + { + command: "formFieldSubmitted", + uri: "example.com", + username: "username", + password: "password", + newPassword: "newPassword", + }, + sender, + ); + await flushPromises(); + chrome.tabs.get = jest.fn().mockImplementationOnce((tabId, callback) => { + callback( + mock({ + status: "complete", + url: sender.url, + }), + ); + }); + + triggerWebRequestOnCompletedEvent( + mock({ + url: sender.url, + tabId: sender.tab.id, + requestId, + }), + ); + await flushPromises(); + + expect(cipherNotificationSpy).toHaveBeenCalled(); + }); + it("triggers the notification on the beforeRequest listener when a post-submission redirection is encountered", async () => { sender.tab = mock({ id: 4 }); sendMockExtensionMessage( @@ -601,6 +676,57 @@ describe("OverlayNotificationsBackground", () => { expect(notificationChangedPasswordSpy).toHaveBeenCalled(); }); + + it("with `useUndiscoveredCipherScenarioTriggeringLogic` on, triggers the notification on the beforeRequest listener when a post-submission redirection is encountered", async () => { + overlayNotificationsBackground["useUndiscoveredCipherScenarioTriggeringLogic"] = true; + sender.tab = mock({ id: 4 }); + sendMockExtensionMessage( + { command: "collectPageDetailsResponse", details: pageDetails }, + sender, + ); + await flushPromises(); + sendMockExtensionMessage( + { + command: "formFieldSubmitted", + uri: "example.com", + username: "", + password: "password", + newPassword: "newPassword", + }, + sender, + ); + await flushPromises(); + chrome.tabs.get = jest.fn().mockImplementation((tabId, callback) => { + callback( + mock({ + status: "complete", + url: sender.url, + }), + ); + }); + + triggerWebRequestOnBeforeRequestEvent( + mock({ + url: sender.url, + tabId: sender.tab.id, + method: "POST", + requestId, + }), + ); + await flushPromises(); + + triggerWebRequestOnBeforeRequestEvent( + mock({ + url: "https://example.com/redirect", + tabId: sender.tab.id, + method: "GET", + requestId, + }), + ); + await flushPromises(); + + expect(cipherNotificationSpy).toHaveBeenCalled(); + }); }); }); diff --git a/apps/browser/src/autofill/background/overlay-notifications.background.ts b/apps/browser/src/autofill/background/overlay-notifications.background.ts index 4f55e68fb41..ae32c367e4a 100644 --- a/apps/browser/src/autofill/background/overlay-notifications.background.ts +++ b/apps/browser/src/autofill/background/overlay-notifications.background.ts @@ -4,7 +4,6 @@ import { CLEAR_NOTIFICATION_LOGIN_DATA_DURATION } from "@bitwarden/common/autofi import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; import { BrowserApi } from "../../platform/browser/browser-api"; -import { NotificationType, NotificationTypes } from "../notification/abstractions/notification-bar"; import { generateDomainMatchPatterns, isInvalidResponseStatusCode } from "../utils"; import { @@ -14,6 +13,8 @@ import { OverlayNotificationsBackground as OverlayNotificationsBackgroundInterface, OverlayNotificationsExtensionMessage, OverlayNotificationsExtensionMessageHandlers, + NotificationScenarios, + NotificationScenario, WebsiteOriginsWithFields, } from "./abstractions/overlay-notifications.background"; import NotificationBackground from "./notification.background"; @@ -32,6 +33,8 @@ export class OverlayNotificationsBackground implements OverlayNotificationsBackg collectPageDetailsResponse: ({ message, sender }) => this.handleCollectPageDetailsResponse(message, sender), }; + // @TODO update via feature-flag + private useUndiscoveredCipherScenarioTriggeringLogic = false; constructor( private logService: LogService, @@ -281,7 +284,7 @@ export class OverlayNotificationsBackground implements OverlayNotificationsBackg const shouldAttemptAddNotification = this.shouldAttemptNotification( modifyLoginData, - NotificationTypes.Add, + NotificationScenarios.Add, ); if (shouldAttemptAddNotification) { @@ -290,7 +293,7 @@ export class OverlayNotificationsBackground implements OverlayNotificationsBackg const shouldAttemptChangeNotification = this.shouldAttemptNotification( modifyLoginData, - NotificationTypes.Change, + NotificationScenarios.Change, ); if (shouldAttemptChangeNotification) { @@ -445,29 +448,41 @@ export class OverlayNotificationsBackground implements OverlayNotificationsBackg requestId: chrome.webRequest.WebRequestDetails["requestId"], modifyLoginData: ModifyLoginCipherFormData, tab: chrome.tabs.Tab, - config: { skippable: NotificationType[] } = { skippable: [] }, + config: { skippable: NotificationScenario[] } = { skippable: [] }, ) => { - const notificationCandidates = [ - { - type: NotificationTypes.Change, - trigger: this.notificationBackground.triggerChangedPasswordNotification, - }, - { - type: NotificationTypes.Add, - trigger: this.notificationBackground.triggerAddLoginNotification, - }, - { - type: NotificationTypes.AtRiskPassword, - trigger: this.notificationBackground.triggerAtRiskPasswordNotification, - }, - ].filter( + const notificationCandidates = this.useUndiscoveredCipherScenarioTriggeringLogic + ? [ + { + type: NotificationScenarios.Cipher, + trigger: this.notificationBackground.triggerCipherNotification, + }, + { + type: NotificationScenarios.AtRiskPassword, + trigger: this.notificationBackground.triggerAtRiskPasswordNotification, + }, + ] + : [ + { + type: NotificationScenarios.Change, + trigger: this.notificationBackground.triggerChangedPasswordNotification, + }, + { + type: NotificationScenarios.Add, + trigger: this.notificationBackground.triggerAddLoginNotification, + }, + { + type: NotificationScenarios.AtRiskPassword, + trigger: this.notificationBackground.triggerAtRiskPasswordNotification, + }, + ]; + const filteredNotificationCandidates = notificationCandidates.filter( (candidate) => this.shouldAttemptNotification(modifyLoginData, candidate.type) || config.skippable.includes(candidate.type), ); const results: string[] = []; - for (const { trigger, type } of notificationCandidates) { + for (const { trigger, type } of filteredNotificationCandidates) { const success = await trigger.bind(this.notificationBackground)(modifyLoginData, tab); if (success) { results.push(`Success: ${type}`); @@ -489,8 +504,16 @@ export class OverlayNotificationsBackground implements OverlayNotificationsBackg */ private shouldAttemptNotification = ( modifyLoginData: ModifyLoginCipherFormData, - notificationType: NotificationType, + notificationType: NotificationScenario, ): boolean => { + if (notificationType === NotificationScenarios.Cipher) { + // The logic after this block pre-qualifies some cipher add/update scenarios + // prematurely (where matching against vault contents is required) and should be + // skipped for this case (these same checks are performed early in the + // notification triggering logic). + return true; + } + // Intentionally not stripping whitespace characters here as they // represent user entry. const usernameFieldHasValue = !!(modifyLoginData?.username || "").length; @@ -504,15 +527,15 @@ export class OverlayNotificationsBackground implements OverlayNotificationsBackg // `Add` case included because all forms with cached usernames (from previous // visits) will appear to be "password only" and otherwise trigger the new login // save notification. - case NotificationTypes.Add: + case NotificationScenarios.Add: // Can be values for nonstored login or account creation return usernameFieldHasValue && (passwordFieldHasValue || newPasswordFieldHasValue); - case NotificationTypes.Change: + case NotificationScenarios.Change: // Can be login with nonstored login changes or account password update return canBeUserLogin || canBePasswordUpdate; - case NotificationTypes.AtRiskPassword: + case NotificationScenarios.AtRiskPassword: return !newPasswordFieldHasValue; - case NotificationTypes.Unlock: + case NotificationScenarios.Unlock: // Unlock notifications are handled separately and do not require form data return false; default: diff --git a/apps/browser/src/autofill/notification/abstractions/notification-bar.ts b/apps/browser/src/autofill/notification/abstractions/notification-bar.ts index b23c3c17abb..923db8d4b5c 100644 --- a/apps/browser/src/autofill/notification/abstractions/notification-bar.ts +++ b/apps/browser/src/autofill/notification/abstractions/notification-bar.ts @@ -8,9 +8,13 @@ import { } from "../../../autofill/content/components/common-types"; const NotificationTypes = { + /** represents scenarios handling saving new ciphers after form submit */ Add: "add", + /** represents scenarios handling saving updated ciphers after form submit */ Change: "change", + /** represents scenarios where user has interacted with an unlock action prompt or action otherwise requiring unlock as a prerequisite */ Unlock: "unlock", + /** represents scenarios where the user has security tasks after updating ciphers */ AtRiskPassword: "at-risk-password", } as const;