From 28fbddb63f80653ea7f6bc3efed8545b3d2ae5ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anders=20=C3=85berg?= Date: Wed, 3 Dec 2025 20:40:55 +0100 Subject: [PATCH] fix(passkeys): [PM-28324] Add a guard that conditionally forces a popout depending on platform * Add a guard that conditionally forces a popout depending on platform * Test the routeguard * Use mockImplementation instead. * autoclose popout --- .../guards/platform-popout.guard.spec.ts | 193 ++++++++++++++++++ .../popup/guards/platform-popout.guard.ts | 46 +++++ apps/browser/src/popup/app-routing.module.ts | 3 +- .../login-via-webauthn.component.ts | 21 +- 4 files changed, 261 insertions(+), 2 deletions(-) create mode 100644 apps/browser/src/auth/popup/guards/platform-popout.guard.spec.ts create mode 100644 apps/browser/src/auth/popup/guards/platform-popout.guard.ts diff --git a/apps/browser/src/auth/popup/guards/platform-popout.guard.spec.ts b/apps/browser/src/auth/popup/guards/platform-popout.guard.spec.ts new file mode 100644 index 0000000000..d39012fd88 --- /dev/null +++ b/apps/browser/src/auth/popup/guards/platform-popout.guard.spec.ts @@ -0,0 +1,193 @@ +import { TestBed } from "@angular/core/testing"; +import { ActivatedRouteSnapshot, RouterStateSnapshot } from "@angular/router"; + +import { BrowserApi } from "../../../platform/browser/browser-api"; +import BrowserPopupUtils from "../../../platform/browser/browser-popup-utils"; + +import { platformPopoutGuard } from "./platform-popout.guard"; + +describe("platformPopoutGuard", () => { + let getPlatformInfoSpy: jest.SpyInstance; + let inPopoutSpy: jest.SpyInstance; + let inSidebarSpy: jest.SpyInstance; + let openPopoutSpy: jest.SpyInstance; + let closePopupSpy: jest.SpyInstance; + + const mockRoute = {} as ActivatedRouteSnapshot; + const mockState: RouterStateSnapshot = { + url: "/login-with-passkey?param=value", + } as RouterStateSnapshot; + + beforeEach(() => { + getPlatformInfoSpy = jest.spyOn(BrowserApi, "getPlatformInfo"); + inPopoutSpy = jest.spyOn(BrowserPopupUtils, "inPopout"); + inSidebarSpy = jest.spyOn(BrowserPopupUtils, "inSidebar"); + openPopoutSpy = jest.spyOn(BrowserPopupUtils, "openPopout").mockImplementation(); + closePopupSpy = jest.spyOn(BrowserApi, "closePopup").mockImplementation(); + + TestBed.configureTestingModule({}); + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + describe("when platform matches", () => { + beforeEach(() => { + getPlatformInfoSpy.mockResolvedValue({ os: "linux" }); + inPopoutSpy.mockReturnValue(false); + inSidebarSpy.mockReturnValue(false); + }); + + it("should open popout and block navigation when not already in popout or sidebar", async () => { + const guard = platformPopoutGuard(["linux"]); + const result = await TestBed.runInInjectionContext(() => guard(mockRoute, mockState)); + + expect(getPlatformInfoSpy).toHaveBeenCalled(); + expect(inPopoutSpy).toHaveBeenCalledWith(window); + expect(inSidebarSpy).toHaveBeenCalledWith(window); + expect(openPopoutSpy).toHaveBeenCalledWith( + "popup/index.html#/login-with-passkey?param=value&autoClosePopout=true", + ); + expect(closePopupSpy).toHaveBeenCalledWith(window); + expect(result).toBe(false); + }); + + it("should allow navigation when already in popout", async () => { + inPopoutSpy.mockReturnValue(true); + + const guard = platformPopoutGuard(["linux"]); + const result = await TestBed.runInInjectionContext(() => guard(mockRoute, mockState)); + + expect(openPopoutSpy).not.toHaveBeenCalled(); + expect(closePopupSpy).not.toHaveBeenCalled(); + expect(result).toBe(true); + }); + + it("should allow navigation when already in sidebar", async () => { + inSidebarSpy.mockReturnValue(true); + + const guard = platformPopoutGuard(["linux"]); + const result = await TestBed.runInInjectionContext(() => guard(mockRoute, mockState)); + + expect(openPopoutSpy).not.toHaveBeenCalled(); + expect(closePopupSpy).not.toHaveBeenCalled(); + expect(result).toBe(true); + }); + }); + + describe("when platform does not match", () => { + beforeEach(() => { + getPlatformInfoSpy.mockResolvedValue({ os: "win" }); + inPopoutSpy.mockReturnValue(false); + inSidebarSpy.mockReturnValue(false); + }); + + it("should allow navigation without opening popout", async () => { + const guard = platformPopoutGuard(["linux"]); + const result = await TestBed.runInInjectionContext(() => guard(mockRoute, mockState)); + + expect(getPlatformInfoSpy).toHaveBeenCalled(); + expect(openPopoutSpy).not.toHaveBeenCalled(); + expect(result).toBe(true); + }); + }); + + describe("when forcePopout is true", () => { + beforeEach(() => { + getPlatformInfoSpy.mockResolvedValue({ os: "win" }); + inPopoutSpy.mockReturnValue(false); + inSidebarSpy.mockReturnValue(false); + }); + + it("should open popout regardless of platform", async () => { + const guard = platformPopoutGuard(["linux"], true); + const result = await TestBed.runInInjectionContext(() => guard(mockRoute, mockState)); + + expect(openPopoutSpy).toHaveBeenCalledWith( + "popup/index.html#/login-with-passkey?param=value&autoClosePopout=true", + ); + expect(closePopupSpy).toHaveBeenCalledWith(window); + expect(result).toBe(false); + }); + + it("should not open popout when already in popout", async () => { + inPopoutSpy.mockReturnValue(true); + + const guard = platformPopoutGuard(["linux"], true); + const result = await TestBed.runInInjectionContext(() => guard(mockRoute, mockState)); + + expect(openPopoutSpy).not.toHaveBeenCalled(); + expect(result).toBe(true); + }); + }); + + describe("with multiple platforms", () => { + beforeEach(() => { + inPopoutSpy.mockReturnValue(false); + inSidebarSpy.mockReturnValue(false); + }); + + it.each(["linux", "mac", "win"])( + "should open popout when platform is %s and included in platforms array", + async (platform) => { + getPlatformInfoSpy.mockResolvedValue({ os: platform }); + + const guard = platformPopoutGuard(["linux", "mac", "win"]); + const result = await TestBed.runInInjectionContext(() => guard(mockRoute, mockState)); + + expect(openPopoutSpy).toHaveBeenCalledWith( + "popup/index.html#/login-with-passkey?param=value&autoClosePopout=true", + ); + expect(closePopupSpy).toHaveBeenCalledWith(window); + expect(result).toBe(false); + }, + ); + + it("should not open popout when platform is not in the array", async () => { + getPlatformInfoSpy.mockResolvedValue({ os: "android" }); + + const guard = platformPopoutGuard(["linux", "mac"]); + const result = await TestBed.runInInjectionContext(() => guard(mockRoute, mockState)); + + expect(openPopoutSpy).not.toHaveBeenCalled(); + expect(result).toBe(true); + }); + }); + + describe("url handling", () => { + beforeEach(() => { + getPlatformInfoSpy.mockResolvedValue({ os: "linux" }); + inPopoutSpy.mockReturnValue(false); + inSidebarSpy.mockReturnValue(false); + }); + + it("should preserve query parameters in the popout url", async () => { + const stateWithQuery: RouterStateSnapshot = { + url: "/path?foo=bar&baz=qux", + } as RouterStateSnapshot; + + const guard = platformPopoutGuard(["linux"]); + await TestBed.runInInjectionContext(() => guard(mockRoute, stateWithQuery)); + + expect(openPopoutSpy).toHaveBeenCalledWith( + "popup/index.html#/path?foo=bar&baz=qux&autoClosePopout=true", + ); + expect(closePopupSpy).toHaveBeenCalledWith(window); + }); + + it("should handle urls without query parameters", async () => { + const stateWithoutQuery: RouterStateSnapshot = { + url: "/simple-path", + } as RouterStateSnapshot; + + const guard = platformPopoutGuard(["linux"]); + await TestBed.runInInjectionContext(() => guard(mockRoute, stateWithoutQuery)); + + expect(openPopoutSpy).toHaveBeenCalledWith( + "popup/index.html#/simple-path?autoClosePopout=true", + ); + expect(closePopupSpy).toHaveBeenCalledWith(window); + }); + }); +}); diff --git a/apps/browser/src/auth/popup/guards/platform-popout.guard.ts b/apps/browser/src/auth/popup/guards/platform-popout.guard.ts new file mode 100644 index 0000000000..aad005e141 --- /dev/null +++ b/apps/browser/src/auth/popup/guards/platform-popout.guard.ts @@ -0,0 +1,46 @@ +import { ActivatedRouteSnapshot, CanActivateFn, RouterStateSnapshot } from "@angular/router"; + +import { BrowserApi } from "../../../platform/browser/browser-api"; +import BrowserPopupUtils from "../../../platform/browser/browser-popup-utils"; + +/** + * Guard that forces a popout window for specific platforms. + * Useful when popup context would close during operations (e.g., WebAuthn on Linux). + * + * @param platforms - Array of platform OS strings (e.g., ["linux", "mac", "win"]) + * @param forcePopout - If true, always force popout regardless of platform (useful for testing) + * @returns CanActivateFn that opens popout and blocks navigation if conditions met + */ +export function platformPopoutGuard( + platforms: string[], + forcePopout: boolean = false, +): CanActivateFn { + return async (route: ActivatedRouteSnapshot, state: RouterStateSnapshot) => { + // Check if current platform matches + const platformInfo = await BrowserApi.getPlatformInfo(); + const isPlatformMatch = platforms.includes(platformInfo.os); + + // Check if already in popout/sidebar + const inPopout = BrowserPopupUtils.inPopout(window); + const inSidebar = BrowserPopupUtils.inSidebar(window); + + // Open popout if conditions met + if ((isPlatformMatch || forcePopout) && !inPopout && !inSidebar) { + // Add autoClosePopout query param to signal the popout should close after completion + const [path, existingQuery] = state.url.split("?"); + const params = new URLSearchParams(existingQuery || ""); + params.set("autoClosePopout", "true"); + const urlWithAutoClose = `${path}?${params.toString()}`; + + // Open the popout window + await BrowserPopupUtils.openPopout(`popup/index.html#${urlWithAutoClose}`); + + // Close the original popup window + BrowserApi.closePopup(window); + + return false; // Block navigation - popout will reload + } + + return true; // Allow navigation + }; +} diff --git a/apps/browser/src/popup/app-routing.module.ts b/apps/browser/src/popup/app-routing.module.ts index a36396afa1..48f06147cd 100644 --- a/apps/browser/src/popup/app-routing.module.ts +++ b/apps/browser/src/popup/app-routing.module.ts @@ -48,6 +48,7 @@ import { LockComponent, ConfirmKeyConnectorDomainComponent } from "@bitwarden/ke import { AccountSwitcherComponent } from "../auth/popup/account-switching/account-switcher.component"; import { AuthExtensionRoute } from "../auth/popup/constants/auth-extension-route.constant"; import { fido2AuthGuard } from "../auth/popup/guards/fido2-auth.guard"; +import { platformPopoutGuard } from "../auth/popup/guards/platform-popout.guard"; import { AccountSecurityComponent } from "../auth/popup/settings/account-security.component"; import { ExtensionDeviceManagementComponent } from "../auth/popup/settings/extension-device-management.component"; import { Fido2Component } from "../autofill/popup/fido2/fido2.component"; @@ -414,7 +415,7 @@ const routes: Routes = [ }, { path: AuthRoute.LoginWithPasskey, - canActivate: [unauthGuardFn(unauthRouteOverrides)], + canActivate: [unauthGuardFn(unauthRouteOverrides), platformPopoutGuard(["linux"])], data: { pageIcon: TwoFactorAuthSecurityKeyIcon, pageTitle: { diff --git a/libs/angular/src/auth/login-via-webauthn/login-via-webauthn.component.ts b/libs/angular/src/auth/login-via-webauthn/login-via-webauthn.component.ts index 764d9fe773..b4d856309e 100644 --- a/libs/angular/src/auth/login-via-webauthn/login-via-webauthn.component.ts +++ b/libs/angular/src/auth/login-via-webauthn/login-via-webauthn.component.ts @@ -2,7 +2,7 @@ // @ts-strict-ignore import { CommonModule } from "@angular/common"; import { Component, OnInit } from "@angular/core"; -import { Router, RouterModule } from "@angular/router"; +import { ActivatedRoute, Router, RouterModule } from "@angular/router"; import { firstValueFrom } from "rxjs"; import { JslibModule } from "@bitwarden/angular/jslib.module"; @@ -19,6 +19,7 @@ import { ClientType } from "@bitwarden/common/enums"; import { ErrorResponse } from "@bitwarden/common/models/response/error.response"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; +import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { ValidationService } from "@bitwarden/common/platform/abstractions/validation.service"; import { @@ -49,6 +50,7 @@ export type State = "assert" | "assertFailed"; }) export class LoginViaWebAuthnComponent implements OnInit { protected currentState: State = "assert"; + private shouldAutoClosePopout = false; protected readonly Icons = { TwoFactorAuthSecurityKeyIcon, @@ -70,6 +72,7 @@ export class LoginViaWebAuthnComponent implements OnInit { constructor( private webAuthnLoginService: WebAuthnLoginServiceAbstraction, private router: Router, + private route: ActivatedRoute, private logService: LogService, private validationService: ValidationService, private i18nService: I18nService, @@ -77,9 +80,14 @@ export class LoginViaWebAuthnComponent implements OnInit { private keyService: KeyService, private platformUtilsService: PlatformUtilsService, private anonLayoutWrapperDataService: AnonLayoutWrapperDataService, + private messagingService: MessagingService, ) {} ngOnInit(): void { + // Check if we should auto-close the popout after successful authentication + this.shouldAutoClosePopout = + this.route.snapshot.queryParamMap.get("autoClosePopout") === "true"; + // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling. // eslint-disable-next-line @typescript-eslint/no-floating-promises this.authenticate(); @@ -123,6 +131,17 @@ export class LoginViaWebAuthnComponent implements OnInit { await this.loginSuccessHandlerService.run(authResult.userId, null); } + // If autoClosePopout is enabled and we're in a browser extension, + // re-open the regular popup and close this popout window + if ( + this.shouldAutoClosePopout && + this.platformUtilsService.getClientType() === ClientType.Browser + ) { + this.messagingService.send("openPopup"); + window.close(); + return; + } + await this.router.navigate([this.successRoute]); } catch (error) { if (error instanceof ErrorResponse) {