From a5c1a5a42f054aa83364db1d9eb905e9adb7c7ba Mon Sep 17 00:00:00 2001 From: Cesar Gonzalez Date: Tue, 8 Oct 2024 16:02:49 -0500 Subject: [PATCH] [PM-12548] Fido2 scripts should not load when user is logged out (#11444) * [PM-12548] Fido2 scripts should not load when user is logged out * [PM-12548] Fido2 scripts should not load when user is logged out --- .../autofill/background/overlay.background.ts | 4 +- .../abstractions/fido2.background.ts | 1 - .../fido2/background/fido2.background.spec.ts | 71 +++++++++---------- .../fido2/background/fido2.background.ts | 43 +++++++++-- .../content/fido2-page-script-append.mv2.ts | 1 + .../fido2-page-script-delay-append.mv2.ts | 1 + .../fido2/content/fido2-page-script.ts | 6 ++ .../browser/src/background/main.background.ts | 2 +- .../src/background/runtime.background.ts | 3 - 9 files changed, 82 insertions(+), 50 deletions(-) diff --git a/apps/browser/src/autofill/background/overlay.background.ts b/apps/browser/src/autofill/background/overlay.background.ts index 653d31ca52c..b45a4a25485 100644 --- a/apps/browser/src/autofill/background/overlay.background.ts +++ b/apps/browser/src/autofill/background/overlay.background.ts @@ -1484,9 +1484,7 @@ export class OverlayBackground implements OverlayBackgroundInterface { } /** - * Gets the user's authentication status from the auth service. If the user's authentication - * status has changed, the inline menu button's authentication status will be updated - * and the inline menu list's ciphers will be updated. + * Gets the user's authentication status from the auth service. */ private async getAuthStatus() { return await firstValueFrom(this.authService.activeAccountStatus$); diff --git a/apps/browser/src/autofill/fido2/background/abstractions/fido2.background.ts b/apps/browser/src/autofill/fido2/background/abstractions/fido2.background.ts index d77a60d3c7b..6ad069ad56e 100644 --- a/apps/browser/src/autofill/fido2/background/abstractions/fido2.background.ts +++ b/apps/browser/src/autofill/fido2/background/abstractions/fido2.background.ts @@ -45,7 +45,6 @@ type Fido2BackgroundExtensionMessageHandlers = { interface Fido2Background { init(): void; - injectFido2ContentScriptsInAllTabs(): Promise; } export { diff --git a/apps/browser/src/autofill/fido2/background/fido2.background.spec.ts b/apps/browser/src/autofill/fido2/background/fido2.background.spec.ts index 23d0292e188..99ed4619954 100644 --- a/apps/browser/src/autofill/fido2/background/fido2.background.spec.ts +++ b/apps/browser/src/autofill/fido2/background/fido2.background.spec.ts @@ -1,6 +1,8 @@ import { mock, MockProxy } from "jest-mock-extended"; import { BehaviorSubject } from "rxjs"; +import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; +import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; import { Fido2ActiveRequestManager } from "@bitwarden/common/platform/abstractions/fido2/fido2-active-request-manager.abstraction"; import { @@ -59,6 +61,8 @@ describe("Fido2Background", () => { let scriptInjectorServiceMock!: MockProxy; let configServiceMock!: MockProxy; let enablePasskeysMock$!: BehaviorSubject; + let activeAccountStatusMock$: BehaviorSubject; + let authServiceMock!: MockProxy; let fido2Background!: Fido2Background; beforeEach(() => { @@ -81,6 +85,9 @@ describe("Fido2Background", () => { vaultSettingsService.enablePasskeys$ = enablePasskeysMock$; fido2ActiveRequestManager = mock(); fido2ClientService.isFido2FeatureEnabled.mockResolvedValue(true); + activeAccountStatusMock$ = new BehaviorSubject(AuthenticationStatus.Unlocked); + authServiceMock = mock(); + authServiceMock.activeAccountStatus$ = activeAccountStatusMock$; fido2Background = new Fido2Background( logService, fido2ActiveRequestManager, @@ -88,6 +95,7 @@ describe("Fido2Background", () => { vaultSettingsService, scriptInjectorServiceMock, configServiceMock, + authServiceMock, ); fido2Background["abortManager"] = abortManagerMock; abortManagerMock.runWithAbortController.mockImplementation((_requestId, runner) => @@ -101,55 +109,31 @@ describe("Fido2Background", () => { jest.clearAllMocks(); }); - describe("injectFido2ContentScriptsInAllTabs", () => { - it("does not inject any FIDO2 content scripts when no tabs have a secure url protocol", async () => { - const insecureTab = mock({ id: 789, url: "http://example.com" }); - tabsQuerySpy.mockResolvedValueOnce([insecureTab]); + describe("handleAuthStatusUpdate", () => { + let updateContentScriptRegistrationSpy: jest.SpyInstance; - await fido2Background.injectFido2ContentScriptsInAllTabs(); - - expect(scriptInjectorServiceMock.inject).not.toHaveBeenCalled(); + beforeEach(() => { + updateContentScriptRegistrationSpy = jest + .spyOn(fido2Background as any, "updateContentScriptRegistration") + .mockImplementation(); }); - it("only injects the FIDO2 content script into tabs that contain a secure url protocol", async () => { - const secondTabMock = mock({ id: 456, url: "https://example.com" }); - const insecureTab = mock({ id: 789, url: "http://example.com" }); - const noUrlTab = mock({ id: 101, url: undefined }); - tabsQuerySpy.mockResolvedValueOnce([tabMock, secondTabMock, insecureTab, noUrlTab]); + it("skips triggering the passkeys settings update if the user is logged out", async () => { + activeAccountStatusMock$.next(AuthenticationStatus.LoggedOut); - await fido2Background.injectFido2ContentScriptsInAllTabs(); + fido2Background.init(); await flushPromises(); - expect(scriptInjectorServiceMock.inject).toHaveBeenCalledWith({ - tabId: tabMock.id, - injectDetails: contentScriptDetails, - }); - expect(scriptInjectorServiceMock.inject).toHaveBeenCalledWith({ - tabId: secondTabMock.id, - injectDetails: contentScriptDetails, - }); - expect(scriptInjectorServiceMock.inject).not.toHaveBeenCalledWith({ - tabId: insecureTab.id, - injectDetails: contentScriptDetails, - }); - expect(scriptInjectorServiceMock.inject).not.toHaveBeenCalledWith({ - tabId: noUrlTab.id, - injectDetails: contentScriptDetails, - }); + expect(updateContentScriptRegistrationSpy).not.toHaveBeenCalled(); }); - it("injects the `page-script.js` content script into the provided tab", async () => { - tabsQuerySpy.mockResolvedValueOnce([tabMock]); + it("triggers the passkeys setting update if the user is logged in", async () => { + activeAccountStatusMock$.next(AuthenticationStatus.Unlocked); - await fido2Background.injectFido2ContentScriptsInAllTabs(); + fido2Background.init(); await flushPromises(); - expect(scriptInjectorServiceMock.inject).toHaveBeenCalledWith({ - tabId: tabMock.id, - injectDetails: sharedScriptInjectionDetails, - mv2Details: { file: Fido2ContentScript.PageScriptAppend }, - mv3Details: { file: Fido2ContentScript.PageScript, world: "MAIN" }, - }); + expect(updateContentScriptRegistrationSpy).toHaveBeenCalled(); }); }); @@ -157,6 +141,7 @@ describe("Fido2Background", () => { let portMock!: MockProxy; beforeEach(() => { + jest.spyOn(fido2Background as any, "handleAuthStatusUpdate").mockImplementation(); fido2Background.init(); jest.spyOn(BrowserApi, "registerContentScriptsMv2"); jest.spyOn(BrowserApi, "registerContentScriptsMv3"); @@ -168,6 +153,15 @@ describe("Fido2Background", () => { tabsQuerySpy.mockResolvedValue([tabMock]); }); + it("skips handling the passkey update if the user is logged out", async () => { + activeAccountStatusMock$.next(AuthenticationStatus.LoggedOut); + + enablePasskeysMock$.next(true); + + expect(portMock.disconnect).not.toHaveBeenCalled(); + expect(scriptInjectorServiceMock.inject).not.toHaveBeenCalled(); + }); + it("does not destroy and re-inject the content scripts when triggering `handleEnablePasskeysUpdate` with an undefined currentEnablePasskeysSetting property", async () => { await flushPromises(); @@ -421,6 +415,7 @@ describe("Fido2Background", () => { let portMock!: MockProxy; beforeEach(() => { + jest.spyOn(fido2Background as any, "handleAuthStatusUpdate").mockImplementation(); fido2Background.init(); portMock = createPortSpyMock(Fido2PortName.InjectedScript); triggerRuntimeOnConnectEvent(portMock); diff --git a/apps/browser/src/autofill/fido2/background/fido2.background.ts b/apps/browser/src/autofill/fido2/background/fido2.background.ts index a9d1b314770..810cdf74657 100644 --- a/apps/browser/src/autofill/fido2/background/fido2.background.ts +++ b/apps/browser/src/autofill/fido2/background/fido2.background.ts @@ -1,6 +1,8 @@ -import { firstValueFrom, startWith } from "rxjs"; +import { firstValueFrom, startWith, Subscription } from "rxjs"; import { pairwise } from "rxjs/operators"; +import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; +import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum"; import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; import { Fido2ActiveRequestManager } from "@bitwarden/common/platform/abstractions/fido2/fido2-active-request-manager.abstraction"; @@ -29,6 +31,7 @@ import { } from "./abstractions/fido2.background"; export class Fido2Background implements Fido2BackgroundInterface { + private currentAuthStatus$: Subscription; private abortManager = new AbortManager(); private fido2ContentScriptPortsSet = new Set(); private registeredContentScripts: browser.contentScripts.RegisteredContentScript; @@ -55,6 +58,7 @@ export class Fido2Background implements Fido2BackgroundInterface { private vaultSettingsService: VaultSettingsService, private scriptInjectorService: ScriptInjectorService, private configService: ConfigService, + private authService: AuthService, ) {} /** @@ -68,12 +72,32 @@ export class Fido2Background implements Fido2BackgroundInterface { this.vaultSettingsService.enablePasskeys$ .pipe(startWith(undefined), pairwise()) .subscribe(([previous, current]) => this.handleEnablePasskeysUpdate(previous, current)); + this.currentAuthStatus$ = this.authService.activeAccountStatus$ + .pipe(startWith(undefined), pairwise()) + .subscribe(([_previous, current]) => this.handleAuthStatusUpdate(current)); + } + + /** + * Handles initializing the FIDO2 content scripts based on the current + * authentication status. We only want to inject the FIDO2 content scripts + * if the user is logged in. + * + * @param authStatus - The current authentication status. + */ + private async handleAuthStatusUpdate(authStatus: AuthenticationStatus) { + if (authStatus === AuthenticationStatus.LoggedOut) { + return; + } + + const enablePasskeys = await this.isPasskeySettingEnabled(); + await this.handleEnablePasskeysUpdate(enablePasskeys, enablePasskeys); + this.currentAuthStatus$.unsubscribe(); } /** * Injects the FIDO2 content and page script into all existing browser tabs. */ - async injectFido2ContentScriptsInAllTabs() { + private async injectFido2ContentScriptsInAllTabs() { const tabs = await BrowserApi.tabsQuery({}); for (let index = 0; index < tabs.length; index++) { @@ -85,6 +109,13 @@ export class Fido2Background implements Fido2BackgroundInterface { } } + /** + * Gets the user's authentication status from the auth service. + */ + private async getAuthStatus() { + return await firstValueFrom(this.authService.activeAccountStatus$); + } + /** * Handles reacting to the enablePasskeys setting being updated. If the setting * is enabled, the FIDO2 content scripts are injected into all tabs. If the setting @@ -98,13 +129,17 @@ export class Fido2Background implements Fido2BackgroundInterface { previousEnablePasskeysSetting: boolean, enablePasskeys: boolean, ) { - this.fido2ActiveRequestManager.removeAllActiveRequests(); - await this.updateContentScriptRegistration(); + if ((await this.getAuthStatus()) === AuthenticationStatus.LoggedOut) { + return; + } if (previousEnablePasskeysSetting === undefined) { return; } + this.fido2ActiveRequestManager.removeAllActiveRequests(); + await this.updateContentScriptRegistration(); + this.destroyLoadedFido2ContentScripts(); if (enablePasskeys) { void this.injectFido2ContentScriptsInAllTabs(); diff --git a/apps/browser/src/autofill/fido2/content/fido2-page-script-append.mv2.ts b/apps/browser/src/autofill/fido2/content/fido2-page-script-append.mv2.ts index dd5f33dffb0..f835d2f175b 100644 --- a/apps/browser/src/autofill/fido2/content/fido2-page-script-append.mv2.ts +++ b/apps/browser/src/autofill/fido2/content/fido2-page-script-append.mv2.ts @@ -9,6 +9,7 @@ const script = globalContext.document.createElement("script"); script.src = chrome.runtime.getURL("content/fido2-page-script.js"); + script.async = false; const scriptInsertionPoint = globalContext.document.head || globalContext.document.documentElement; diff --git a/apps/browser/src/autofill/fido2/content/fido2-page-script-delay-append.mv2.ts b/apps/browser/src/autofill/fido2/content/fido2-page-script-delay-append.mv2.ts index 2ada31fdfe2..775bc76266d 100644 --- a/apps/browser/src/autofill/fido2/content/fido2-page-script-delay-append.mv2.ts +++ b/apps/browser/src/autofill/fido2/content/fido2-page-script-delay-append.mv2.ts @@ -9,6 +9,7 @@ const script = globalContext.document.createElement("script"); script.src = chrome.runtime.getURL("content/fido2-page-script.js"); + script.async = false; // We are ensuring that the script injection is delayed in the event that we are loading // within an iframe element. This prevents an issue with web mail clients that load content diff --git a/apps/browser/src/autofill/fido2/content/fido2-page-script.ts b/apps/browser/src/autofill/fido2/content/fido2-page-script.ts index c44c263dd23..dfc2bba681a 100644 --- a/apps/browser/src/autofill/fido2/content/fido2-page-script.ts +++ b/apps/browser/src/autofill/fido2/content/fido2-page-script.ts @@ -4,6 +4,12 @@ import { MessageType } from "./messaging/message"; import { Messenger } from "./messaging/messenger"; (function (globalContext) { + if (globalContext.document.currentScript) { + globalContext.document.currentScript.parentNode.removeChild( + globalContext.document.currentScript, + ); + } + const shouldExecuteContentScript = globalContext.document.contentType === "text/html" && (globalContext.document.location.protocol === "https:" || diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index 85e1ee19b07..318b856b324 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -1103,6 +1103,7 @@ export default class MainBackground { this.vaultSettingsService, this.scriptInjectorService, this.configService, + this.authService, ); const lockService = new DefaultLockService(this.accountService, this.vaultTimeoutService); @@ -1118,7 +1119,6 @@ export default class MainBackground { this.messagingService, this.logService, this.configService, - this.fido2Background, messageListener, this.accountService, lockService, diff --git a/apps/browser/src/background/runtime.background.ts b/apps/browser/src/background/runtime.background.ts index 3e0933942b5..2bc2eadf261 100644 --- a/apps/browser/src/background/runtime.background.ts +++ b/apps/browser/src/background/runtime.background.ts @@ -21,7 +21,6 @@ import { openTwoFactorAuthPopout, } from "../auth/popup/utils/auth-popout-window"; import { LockedVaultPendingNotificationsData } from "../autofill/background/abstractions/notification.background"; -import { Fido2Background } from "../autofill/fido2/background/abstractions/fido2.background"; import { AutofillService } from "../autofill/services/abstractions/autofill.service"; import { BrowserApi } from "../platform/browser/browser-api"; import { BrowserEnvironmentService } from "../platform/services/browser-environment.service"; @@ -46,7 +45,6 @@ export default class RuntimeBackground { private messagingService: MessagingService, private logService: LogService, private configService: ConfigService, - private fido2Background: Fido2Background, private messageListener: MessageListener, private accountService: AccountService, private readonly lockService: LockService, @@ -365,7 +363,6 @@ export default class RuntimeBackground { private async checkOnInstalled() { setTimeout(async () => { - void this.fido2Background.injectFido2ContentScriptsInAllTabs(); void this.autofillService.loadAutofillScriptsOnInstall(); if (this.onInstalledReason != null) {