From 422e527516ea3165f0cbed1a2b7e841422f0603f Mon Sep 17 00:00:00 2001 From: Jonathan Prusik Date: Wed, 3 Dec 2025 11:46:48 -0500 Subject: [PATCH] [PM-28289] Address false-positives of new login save prompts (#17783) * add values to TotpFieldNames constant * add totp field check to username field qualification * handle checking empty string cases * update tests * require stored username for new cipher notification prompt * drop ambiguous token keyword from authoritative TOTP field names constant * adjust shouldAttemptNotification logic for add and change cases --- .../overlay-notifications.background.ts | 49 ++++++++-- .../autofill/services/autofill-constants.ts | 1 + .../autofill-overlay-content.service.spec.ts | 92 +++++++++++++++++-- ...inline-menu-field-qualification.service.ts | 3 +- 4 files changed, 128 insertions(+), 17 deletions(-) diff --git a/apps/browser/src/autofill/background/overlay-notifications.background.ts b/apps/browser/src/autofill/background/overlay-notifications.background.ts index e08fe54071..86cdbffe05 100644 --- a/apps/browser/src/autofill/background/overlay-notifications.background.ts +++ b/apps/browser/src/autofill/background/overlay-notifications.background.ts @@ -262,11 +262,30 @@ export class OverlayNotificationsBackground implements OverlayNotificationsBackg */ private notificationDataIncompleteOnBeforeRequest = (tabId: number) => { const modifyLoginData = this.modifyLoginCipherFormData.get(tabId); - return ( - !modifyLoginData || - !this.shouldAttemptNotification(modifyLoginData, NotificationTypes.Add) || - !this.shouldAttemptNotification(modifyLoginData, NotificationTypes.Change) + + if (!modifyLoginData) { + return true; + } + + const shouldAttemptAddNotification = this.shouldAttemptNotification( + modifyLoginData, + NotificationTypes.Add, ); + + if (shouldAttemptAddNotification) { + return false; + } + + const shouldAttemptChangeNotification = this.shouldAttemptNotification( + modifyLoginData, + NotificationTypes.Change, + ); + + if (shouldAttemptChangeNotification) { + return false; + } + + return false; }; /** @@ -454,15 +473,27 @@ export class OverlayNotificationsBackground implements OverlayNotificationsBackg modifyLoginData: ModifyLoginCipherFormData, notificationType: NotificationType, ): boolean => { + // Intentionally not stripping whitespace characters here as they + // represent user entry. + const usernameFieldHasValue = !!(modifyLoginData?.username || "").length; + const passwordFieldHasValue = !!(modifyLoginData?.password || "").length; + const newPasswordFieldHasValue = !!(modifyLoginData?.newPassword || "").length; + + const canBeUserLogin = usernameFieldHasValue && passwordFieldHasValue; + const canBePasswordUpdate = passwordFieldHasValue && newPasswordFieldHasValue; + switch (notificationType) { + // `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: - return ( - modifyLoginData?.username && !!(modifyLoginData.password || modifyLoginData.newPassword) - ); + // Can be values for nonstored login or account creation + return usernameFieldHasValue && (passwordFieldHasValue || newPasswordFieldHasValue); case NotificationTypes.Change: - return !!(modifyLoginData.password || modifyLoginData.newPassword); + // Can be login with nonstored login changes or account password update + return canBeUserLogin || canBePasswordUpdate; case NotificationTypes.AtRiskPassword: - return !modifyLoginData.newPassword; + return !newPasswordFieldHasValue; case NotificationTypes.Unlock: // Unlock notifications are handled separately and do not require form data return false; diff --git a/apps/browser/src/autofill/services/autofill-constants.ts b/apps/browser/src/autofill/services/autofill-constants.ts index 51d0513a7f..0fbe2e67ae 100644 --- a/apps/browser/src/autofill/services/autofill-constants.ts +++ b/apps/browser/src/autofill/services/autofill-constants.ts @@ -39,6 +39,7 @@ export class AutoFillConstants { "otpcode", "onetimepassword", "security_code", + "second-factor", "twofactor", "twofa", "twofactorcode", diff --git a/apps/browser/src/autofill/services/autofill-overlay-content.service.spec.ts b/apps/browser/src/autofill/services/autofill-overlay-content.service.spec.ts index 3c19589afe..0fb031b52e 100644 --- a/apps/browser/src/autofill/services/autofill-overlay-content.service.spec.ts +++ b/apps/browser/src/autofill/services/autofill-overlay-content.service.spec.ts @@ -1603,14 +1603,14 @@ describe("AutofillOverlayContentService", () => { it("skips triggering submission if a button is not found", async () => { const submitButton = document.querySelector("button"); - submitButton.remove(); + submitButton?.remove(); await autofillOverlayContentService.setupOverlayListeners( autofillFieldElement, autofillFieldData, pageDetailsMock, ); - submitButton.dispatchEvent(new KeyboardEvent("keyup", { code: "Enter" })); + submitButton?.dispatchEvent(new KeyboardEvent("keyup", { code: "Enter" })); expect(sendExtensionMessageSpy).not.toHaveBeenCalledWith( "formFieldSubmitted", @@ -1627,7 +1627,7 @@ describe("AutofillOverlayContentService", () => { pageDetailsMock, ); await flushPromises(); - submitButton.dispatchEvent(new KeyboardEvent("keyup", { code: "Enter" })); + submitButton?.dispatchEvent(new KeyboardEvent("keyup", { code: "Enter" })); expect(sendExtensionMessageSpy).toHaveBeenCalledWith( "formFieldSubmitted", @@ -1641,7 +1641,7 @@ describe("AutofillOverlayContentService", () => {
`; - const shadowRoot = document.getElementById("shadow-root").attachShadow({ mode: "open" }); + const shadowRoot = document.getElementById("shadow-root")!.attachShadow({ mode: "open" }); shadowRoot.innerHTML = ` `; @@ -1668,7 +1668,7 @@ describe("AutofillOverlayContentService", () => { pageDetailsMock, ); await flushPromises(); - buttonElement.dispatchEvent(new KeyboardEvent("keyup", { code: "Enter" })); + buttonElement?.dispatchEvent(new KeyboardEvent("keyup", { code: "Enter" })); expect(sendExtensionMessageSpy).toHaveBeenCalledWith( "formFieldSubmitted", @@ -1716,6 +1716,85 @@ describe("AutofillOverlayContentService", () => { }); }); + describe("refreshMenuLayerPosition", () => { + it("calls refreshTopLayerPosition on the inline menu content service", () => { + autofillOverlayContentService.refreshMenuLayerPosition(); + + expect(inlineMenuContentService.refreshTopLayerPosition).toHaveBeenCalled(); + }); + + it("does not throw if inline menu content service is not available", () => { + const serviceWithoutInlineMenu = new AutofillOverlayContentService( + domQueryService, + domElementVisibilityService, + inlineMenuFieldQualificationService, + ); + + expect(() => serviceWithoutInlineMenu.refreshMenuLayerPosition()).not.toThrow(); + }); + }); + + describe("getOwnedInlineMenuTagNames", () => { + it("returns tag names from the inline menu content service", () => { + inlineMenuContentService.getOwnedTagNames.mockReturnValue(["div", "span"]); + + const result = autofillOverlayContentService.getOwnedInlineMenuTagNames(); + + expect(result).toEqual(["div", "span"]); + }); + + it("returns an empty array if inline menu content service is not available", () => { + const serviceWithoutInlineMenu = new AutofillOverlayContentService( + domQueryService, + domElementVisibilityService, + inlineMenuFieldQualificationService, + ); + + const result = serviceWithoutInlineMenu.getOwnedInlineMenuTagNames(); + + expect(result).toEqual([]); + }); + }); + + describe("getUnownedTopLayerItems", () => { + it("returns unowned top layer items from the inline menu content service", () => { + const mockElements = document.querySelectorAll("div"); + inlineMenuContentService.getUnownedTopLayerItems.mockReturnValue(mockElements); + + const result = autofillOverlayContentService.getUnownedTopLayerItems(true); + + expect(result).toEqual(mockElements); + expect(inlineMenuContentService.getUnownedTopLayerItems).toHaveBeenCalledWith(true); + }); + + it("returns undefined if inline menu content service is not available", () => { + const serviceWithoutInlineMenu = new AutofillOverlayContentService( + domQueryService, + domElementVisibilityService, + inlineMenuFieldQualificationService, + ); + + const result = serviceWithoutInlineMenu.getUnownedTopLayerItems(); + + expect(result).toBeUndefined(); + }); + }); + + describe("clearUserFilledFields", () => { + it("deletes all user filled fields", () => { + const mockElement1 = document.createElement("input") as FillableFormFieldElement; + const mockElement2 = document.createElement("input") as FillableFormFieldElement; + autofillOverlayContentService["userFilledFields"] = { + username: mockElement1, + password: mockElement2, + }; + + autofillOverlayContentService.clearUserFilledFields(); + + expect(autofillOverlayContentService["userFilledFields"]).toEqual({}); + }); + }); + describe("handleOverlayRepositionEvent", () => { const repositionEvents = [EVENTS.SCROLL, EVENTS.RESIZE]; repositionEvents.forEach((repositionEvent) => { @@ -2049,7 +2128,7 @@ describe("AutofillOverlayContentService", () => { }); it("skips focusing an element if no recently focused field exists", async () => { - autofillOverlayContentService["mostRecentlyFocusedField"] = undefined; + (autofillOverlayContentService as any)["mostRecentlyFocusedField"] = null; sendMockExtensionMessage({ command: "redirectAutofillInlineMenuFocusOut", @@ -2149,7 +2228,6 @@ describe("AutofillOverlayContentService", () => { }); it("returns null if the sub frame URL cannot be parsed correctly", async () => { - delete globalThis.location; globalThis.location = { href: "invalid-base" } as Location; sendMockExtensionMessage( { diff --git a/apps/browser/src/autofill/services/inline-menu-field-qualification.service.ts b/apps/browser/src/autofill/services/inline-menu-field-qualification.service.ts index f7c46a9fa7..f6afaae202 100644 --- a/apps/browser/src/autofill/services/inline-menu-field-qualification.service.ts +++ b/apps/browser/src/autofill/services/inline-menu-field-qualification.service.ts @@ -945,7 +945,8 @@ export class InlineMenuFieldQualificationService !fieldType || !this.usernameFieldTypes.has(fieldType) || this.isExcludedFieldType(field, this.excludedAutofillFieldTypesSet) || - this.fieldHasDisqualifyingAttributeValue(field) + this.fieldHasDisqualifyingAttributeValue(field) || + this.isTotpField(field) ) { return false; }