From 85c8ff04a1af93d869175c4ee92a9fd7a023fcfb Mon Sep 17 00:00:00 2001 From: Cesar Gonzalez Date: Wed, 31 Jul 2024 12:51:41 -0500 Subject: [PATCH] [PM-10138] Inline menu not properly validating as the top-most element within mutation observer (#10292) * [PM-10138] Inline menu not properly validating as the top-most element within mutation observer * [PM-10138] Identity inline menu appearing for invalid field types * [PM-10138] Identity inline menu appearing for invalid field types * [PM-10138] Fixing an issue present with references to card and identity ciphers * [PM-10138] Fixing issues with initialization of the inline menu * [PM-10138] Fixing issues with initialization of the inline menu * [PM-10138] Removing inclusion of file protocol when injecting scripts --- .../abstractions/overlay.background.ts | 1 - .../background/overlay.background.spec.ts | 1 - .../autofill/background/overlay.background.ts | 5 ++- ...tofill-inline-menu-content.service.spec.ts | 15 +++++-- .../autofill-inline-menu-content.service.ts | 43 +++++++++++++++---- ...utofill-inline-menu-iframe.service.spec.ts | 15 +++++++ .../autofill-inline-menu-iframe.service.ts | 13 +++--- ...inline-menu-field-qualification.service.ts | 16 +++++-- .../browser/src/background/main.background.ts | 23 ++++++++-- .../src/background/runtime.background.ts | 2 + 10 files changed, 106 insertions(+), 28 deletions(-) diff --git a/apps/browser/src/autofill/background/abstractions/overlay.background.ts b/apps/browser/src/autofill/background/abstractions/overlay.background.ts index f1bfd3642f..401196b256 100644 --- a/apps/browser/src/autofill/background/abstractions/overlay.background.ts +++ b/apps/browser/src/autofill/background/abstractions/overlay.background.ts @@ -190,7 +190,6 @@ export type OverlayBackgroundExtensionMessageHandlers = { }: BackgroundOnMessageHandlerParams) => void; collectPageDetailsResponse: ({ message, sender }: BackgroundOnMessageHandlerParams) => void; unlockCompleted: ({ message }: BackgroundMessageParam) => void; - doFullSync: () => void; addedCipher: () => void; addEditCipherSubmitted: () => void; editedCipher: () => void; diff --git a/apps/browser/src/autofill/background/overlay.background.spec.ts b/apps/browser/src/autofill/background/overlay.background.spec.ts index 3a3bb7dd5e..b3c8699206 100644 --- a/apps/browser/src/autofill/background/overlay.background.spec.ts +++ b/apps/browser/src/autofill/background/overlay.background.spec.ts @@ -2014,7 +2014,6 @@ describe("OverlayBackground", () => { describe("extension messages that trigger an update of the inline menu ciphers", () => { const extensionMessages = [ - "doFullSync", "addedCipher", "addEditCipherSubmitted", "editedCipher", diff --git a/apps/browser/src/autofill/background/overlay.background.ts b/apps/browser/src/autofill/background/overlay.background.ts index eea72979dd..029d2d69ac 100644 --- a/apps/browser/src/autofill/background/overlay.background.ts +++ b/apps/browser/src/autofill/background/overlay.background.ts @@ -120,7 +120,6 @@ export class OverlayBackground implements OverlayBackgroundInterface { this.triggerDestroyInlineMenuListeners(sender.tab, message.subFrameData.frameId), collectPageDetailsResponse: ({ message, sender }) => this.storePageDetails(message, sender), unlockCompleted: ({ message }) => this.unlockCompleted(message), - doFullSync: () => this.updateOverlayCiphers(), addedCipher: () => this.updateOverlayCiphers(), addEditCipherSubmitted: () => this.updateOverlayCiphers(), editedCipher: () => this.updateOverlayCiphers(), @@ -273,7 +272,9 @@ export class OverlayBackground implements OverlayBackgroundInterface { await this.cipherService.getAllDecryptedForUrl(currentTab?.url || "") ).sort((a, b) => this.cipherService.sortCiphersByLastUsedThenName(a, b)); - return cipherViews.concat(...this.cardAndIdentityCiphers); + return this.cardAndIdentityCiphers + ? cipherViews.concat(...this.cardAndIdentityCiphers) + : cipherViews; } /** diff --git a/apps/browser/src/autofill/overlay/inline-menu/content/autofill-inline-menu-content.service.spec.ts b/apps/browser/src/autofill/overlay/inline-menu/content/autofill-inline-menu-content.service.spec.ts index 616c883f18..688b327d29 100644 --- a/apps/browser/src/autofill/overlay/inline-menu/content/autofill-inline-menu-content.service.spec.ts +++ b/apps/browser/src/autofill/overlay/inline-menu/content/autofill-inline-menu-content.service.spec.ts @@ -399,6 +399,11 @@ describe("AutofillInlineMenuContentService", () => { }); it("sets the z-index of to a lower value", async () => { + autofillInlineMenuContentService["handlePersistentLastChildOverrideTimeout"] = setTimeout( + jest.fn(), + 1000, + ); + await autofillInlineMenuContentService["handleBodyElementMutationObserverUpdate"](); await waitForIdleCallback(); @@ -411,8 +416,9 @@ describe("AutofillInlineMenuContentService", () => { }); globalThis.document.elementFromPoint = jest.fn(() => persistentLastChild); - await autofillInlineMenuContentService["handleBodyElementMutationObserverUpdate"](); - await waitForIdleCallback(); + await autofillInlineMenuContentService["verifyInlineMenuIsNotObscured"]( + persistentLastChild, + ); expect(sendExtensionMessageSpy).toHaveBeenCalledWith("autofillOverlayElementClosed", { overlayElement: AutofillOverlayElement.Button, @@ -425,8 +431,9 @@ describe("AutofillInlineMenuContentService", () => { }); globalThis.document.elementFromPoint = jest.fn(() => persistentLastChild); - await autofillInlineMenuContentService["handleBodyElementMutationObserverUpdate"](); - await waitForIdleCallback(); + await autofillInlineMenuContentService["verifyInlineMenuIsNotObscured"]( + persistentLastChild, + ); expect(sendExtensionMessageSpy).toHaveBeenCalledWith("autofillOverlayElementClosed", { overlayElement: AutofillOverlayElement.List, diff --git a/apps/browser/src/autofill/overlay/inline-menu/content/autofill-inline-menu-content.service.ts b/apps/browser/src/autofill/overlay/inline-menu/content/autofill-inline-menu-content.service.ts index ae94741591..02d3ae052c 100644 --- a/apps/browser/src/autofill/overlay/inline-menu/content/autofill-inline-menu-content.service.ts +++ b/apps/browser/src/autofill/overlay/inline-menu/content/autofill-inline-menu-content.service.ts @@ -33,6 +33,7 @@ export class AutofillInlineMenuContentService implements AutofillInlineMenuConte private bodyElementMutationObserver: MutationObserver; private mutationObserverIterations = 0; private mutationObserverIterationsResetTimeout: number | NodeJS.Timeout; + private handlePersistentLastChildOverrideTimeout: number | NodeJS.Timeout; private lastElementOverrides: WeakMap = new WeakMap(); private readonly customElementDefaultStyles: Partial = { all: "initial", @@ -405,7 +406,7 @@ export class AutofillInlineMenuContentService implements AutofillInlineMenuConte } if (this.lastElementOverrides.get(lastChild) >= 3) { - await this.handlePersistentLastChildOverride(lastChild); + this.handlePersistentLastChildOverride(lastChild); return; } @@ -430,6 +431,26 @@ export class AutofillInlineMenuContentService implements AutofillInlineMenuConte globalThis.document.body.insertBefore(lastChild, this.buttonElement); }; + /** + * Handles the behavior of a persistent child element that is forcing itself to + * the bottom of the body element. This method will ensure that the inline menu + * elements are not obscured by the persistent child element. + * + * @param lastChild - The last child of the body element. + */ + private handlePersistentLastChildOverride(lastChild: Element) { + const lastChildZIndex = parseInt((lastChild as HTMLElement).style.zIndex); + if (lastChildZIndex >= 2147483647) { + (lastChild as HTMLElement).style.zIndex = "2147483646"; + } + + this.clearPersistentLastChildOverrideTimeout(); + this.handlePersistentLastChildOverrideTimeout = globalThis.setTimeout( + () => this.verifyInlineMenuIsNotObscured(lastChild), + 500, + ); + } + /** * Verifies if the last child of the body element is overlaying the inline menu elements. * This is triggered when the last child of the body is being forced by some script to @@ -437,12 +458,7 @@ export class AutofillInlineMenuContentService implements AutofillInlineMenuConte * * @param lastChild - The last child of the body element. */ - private async handlePersistentLastChildOverride(lastChild: Element) { - const lastChildZIndex = parseInt((lastChild as HTMLElement).style.zIndex); - if (lastChildZIndex >= 2147483647) { - (lastChild as HTMLElement).style.zIndex = "2147483646"; - } - + private verifyInlineMenuIsNotObscured = async (lastChild: Element) => { const inlineMenuPosition: InlineMenuPosition = await this.sendExtensionMessage( "getAutofillInlineMenuPosition", ); @@ -456,7 +472,7 @@ export class AutofillInlineMenuContentService implements AutofillInlineMenuConte if (!!list && this.elementAtCenterOfInlineMenuPosition(list) === lastChild) { this.closeInlineMenu(); } - } + }; /** * Returns the element present at the center of the inline menu position. @@ -470,6 +486,16 @@ export class AutofillInlineMenuContentService implements AutofillInlineMenuConte ); } + /** + * Clears the timeout that is used to verify that the last child of the body element + * is not overlaying the inline menu elements. + */ + private clearPersistentLastChildOverrideTimeout() { + if (this.handlePersistentLastChildOverrideTimeout) { + globalThis.clearTimeout(this.handlePersistentLastChildOverrideTimeout); + } + } + /** * Identifies if the mutation observer is triggering excessive iterations. * Will trigger a blur of the most recently focused field and remove the @@ -503,5 +529,6 @@ export class AutofillInlineMenuContentService implements AutofillInlineMenuConte */ destroy() { this.closeInlineMenu(); + this.clearPersistentLastChildOverrideTimeout(); } } diff --git a/apps/browser/src/autofill/overlay/inline-menu/iframe-content/autofill-inline-menu-iframe.service.spec.ts b/apps/browser/src/autofill/overlay/inline-menu/iframe-content/autofill-inline-menu-iframe.service.spec.ts index 7408e0d6fb..f3f6f73cdb 100644 --- a/apps/browser/src/autofill/overlay/inline-menu/iframe-content/autofill-inline-menu-iframe.service.spec.ts +++ b/apps/browser/src/autofill/overlay/inline-menu/iframe-content/autofill-inline-menu-iframe.service.spec.ts @@ -377,6 +377,21 @@ describe("AutofillInlineMenuIframeService", () => { autofillInlineMenuIframeService["ariaAlertElement"], ); }); + + it("resets the fade in timeout if it is set", () => { + autofillInlineMenuIframeService["fadeInTimeout"] = setTimeout(jest.fn, 100); + const styles = { top: "100px", left: "100px" }; + jest.spyOn(autofillInlineMenuIframeService as any, "handleFadeInInlineMenuIframe"); + + sendPortMessage(portSpy, { + command: "updateAutofillInlineMenuPosition", + styles, + }); + + expect( + autofillInlineMenuIframeService["handleFadeInInlineMenuIframe"], + ).toHaveBeenCalled(); + }); }); it("updates the visibility of the iframe", () => { 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 fd305d23c9..99519281bc 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 @@ -260,10 +260,13 @@ export class AutofillInlineMenuIframeService implements AutofillInlineMenuIframe return; } + const styles = this.fadeInTimeout ? Object.assign(position, { opacity: "0" }) : position; + this.updateElementStyles(this.iframe, styles); + if (this.fadeInTimeout) { this.handleFadeInInlineMenuIframe(); } - this.updateElementStyles(this.iframe, position); + this.announceAriaAlert(); } @@ -320,10 +323,10 @@ export class AutofillInlineMenuIframeService implements AutofillInlineMenuIframe */ private handleFadeInInlineMenuIframe() { this.clearFadeInTimeout(); - this.fadeInTimeout = globalThis.setTimeout( - () => this.updateElementStyles(this.iframe, { display: "block", opacity: "1" }), - 10, - ); + this.fadeInTimeout = globalThis.setTimeout(() => { + this.updateElementStyles(this.iframe, { display: "block", opacity: "1" }); + this.clearFadeInTimeout(); + }, 10); } /** 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 12392cbd44..151b8010f7 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 @@ -16,7 +16,7 @@ export class InlineMenuFieldQualificationService implements InlineMenuFieldQualificationServiceInterface { private searchFieldNamesSet = new Set(AutoFillConstants.SearchFieldNames); - private excludedAutofillLoginTypesSet = new Set(AutoFillConstants.ExcludedAutofillLoginTypes); + private excludedAutofillFieldTypesSet = new Set(AutoFillConstants.ExcludedAutofillLoginTypes); private usernameFieldTypes = new Set(["text", "email", "number", "tel"]); private usernameAutocompleteValue = "username"; private emailAutocompleteValue = "email"; @@ -244,6 +244,10 @@ export class InlineMenuFieldQualificationService * @param pageDetails - The details of the page that the field is on. */ isFieldForAccountCreationForm(field: AutofillField, pageDetails: AutofillPageDetails): boolean { + if (this.isExcludedFieldType(field, this.excludedAutofillFieldTypesSet)) { + return false; + } + if (!this.isUsernameField(field) && !this.isPasswordField(field)) { return false; } @@ -286,6 +290,10 @@ export class InlineMenuFieldQualificationService * @param _pageDetails - Currently unused, will likely be required in the future */ isFieldForIdentityForm(field: AutofillField, _pageDetails: AutofillPageDetails): boolean { + if (this.isExcludedFieldType(field, this.excludedAutofillFieldTypesSet)) { + return false; + } + if (this.fieldContainsAutocompleteValues(field, this.identityAutocompleteValues)) { return true; } @@ -833,7 +841,7 @@ export class InlineMenuFieldQualificationService isUsernameField = (field: AutofillField): boolean => { if ( !this.usernameFieldTypes.has(field.type) || - this.isExcludedFieldType(field, this.excludedAutofillLoginTypesSet) + this.isExcludedFieldType(field, this.excludedAutofillFieldTypesSet) ) { return false; } @@ -852,7 +860,7 @@ export class InlineMenuFieldQualificationService } return ( - !this.isExcludedFieldType(field, this.excludedAutofillLoginTypesSet) && + !this.isExcludedFieldType(field, this.excludedAutofillFieldTypesSet) && this.keywordsFoundInFieldData(field, AutoFillConstants.EmailFieldNames) ); }; @@ -898,7 +906,7 @@ export class InlineMenuFieldQualificationService const isInputPasswordType = field.type === "password"; if ( (!isInputPasswordType && - this.isExcludedFieldType(field, this.excludedAutofillLoginTypesSet)) || + this.isExcludedFieldType(field, this.excludedAutofillFieldTypesSet)) || this.fieldHasDisqualifyingAttributeValue(field) ) { return false; diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index 21b02564e3..e876755f75 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -1245,6 +1245,13 @@ export default class MainBackground { } } + async updateOverlayCiphers() { + // overlayBackground null in popup only contexts + if (this.overlayBackground) { + await this.overlayBackground.updateOverlayCiphers(); + } + } + /** * Switch accounts to indicated userId -- null is no active user */ @@ -1273,7 +1280,7 @@ export default class MainBackground { if (userId == null) { await this.refreshBadge(); await this.refreshMenu(); - await this.overlayBackground?.updateOverlayCiphers(); // null in popup only contexts + await this.updateOverlayCiphers(); this.messagingService.send("goHome"); return; } @@ -1296,7 +1303,7 @@ export default class MainBackground { this.messagingService.send("unlocked", { userId: userId }); await this.refreshBadge(); await this.refreshMenu(); - await this.overlayBackground?.updateOverlayCiphers(); // null in popup only contexts + await this.updateOverlayCiphers(); await this.syncService.fullSync(false); } } finally { @@ -1480,7 +1487,17 @@ export default class MainBackground { * Temporary solution to handle initialization of the overlay background behind a feature flag. * Will be reverted to instantiation within the constructor once the feature flag is removed. */ - private async initOverlayAndTabsBackground() { + async initOverlayAndTabsBackground() { + if ( + this.popupOnlyContext || + this.overlayBackground || + this.tabsBackground || + (await firstValueFrom(this.authService.activeAccountStatus$)) === + AuthenticationStatus.LoggedOut + ) { + return; + } + const inlineMenuPositioningImprovementsEnabled = await this.configService.getFeatureFlag( FeatureFlag.InlineMenuPositioningImprovements, ); diff --git a/apps/browser/src/background/runtime.background.ts b/apps/browser/src/background/runtime.background.ts index ccf8205744..94500964af 100644 --- a/apps/browser/src/background/runtime.background.ts +++ b/apps/browser/src/background/runtime.background.ts @@ -200,6 +200,7 @@ export default class RuntimeBackground { let item: LockedVaultPendingNotificationsData; if (msg.command === "loggedIn") { + await this.main.initOverlayAndTabsBackground(); await this.sendBwInstalledMessageToVault(); await this.autofillService.reloadAutofillScripts(); } @@ -246,6 +247,7 @@ export default class RuntimeBackground { await this.main.refreshMenu(); }, 2000); await this.configService.ensureConfigFetched(); + await this.main.updateOverlayCiphers(); } break; case "openPopup":