mirror of
https://github.com/bitwarden/browser
synced 2025-12-29 14:43:31 +00:00
PM-22836 Import popout crashes unexpectedly (#16928)
* follow existing popout guard pattern to force popout on firefox when filepicker is exposed * move firefox guard to tools ownership & revert changes to auth owned file * removed redundant test case
This commit is contained in:
@@ -68,6 +68,7 @@ import { popupRouterCacheGuard } from "../platform/popup/view-cache/popup-router
|
||||
import { RouteCacheOptions } from "../platform/services/popup-view-cache-background.service";
|
||||
import { CredentialGeneratorHistoryComponent } from "../tools/popup/generator/credential-generator-history.component";
|
||||
import { CredentialGeneratorComponent } from "../tools/popup/generator/credential-generator.component";
|
||||
import { firefoxPopoutGuard } from "../tools/popup/guards/firefox-popout.guard";
|
||||
import { SendAddEditComponent as SendAddEditV2Component } from "../tools/popup/send-v2/add-edit/send-add-edit.component";
|
||||
import { SendCreatedComponent } from "../tools/popup/send-v2/send-created/send-created.component";
|
||||
import { SendV2Component } from "../tools/popup/send-v2/send-v2.component";
|
||||
@@ -262,7 +263,7 @@ const routes: Routes = [
|
||||
{
|
||||
path: "import",
|
||||
component: ImportBrowserV2Component,
|
||||
canActivate: [authGuard],
|
||||
canActivate: [authGuard, firefoxPopoutGuard()],
|
||||
data: { elevation: 1 } satisfies RouteDataProperties,
|
||||
},
|
||||
{
|
||||
@@ -340,13 +341,13 @@ const routes: Routes = [
|
||||
{
|
||||
path: "add-send",
|
||||
component: SendAddEditV2Component,
|
||||
canActivate: [authGuard],
|
||||
canActivate: [authGuard, firefoxPopoutGuard()],
|
||||
data: { elevation: 1 } satisfies RouteDataProperties,
|
||||
},
|
||||
{
|
||||
path: "edit-send",
|
||||
component: SendAddEditV2Component,
|
||||
canActivate: [authGuard],
|
||||
canActivate: [authGuard, firefoxPopoutGuard()],
|
||||
data: { elevation: 1 } satisfies RouteDataProperties,
|
||||
},
|
||||
{
|
||||
|
||||
194
apps/browser/src/tools/popup/guards/firefox-popout.guard.spec.ts
Normal file
194
apps/browser/src/tools/popup/guards/firefox-popout.guard.spec.ts
Normal file
@@ -0,0 +1,194 @@
|
||||
import { TestBed } from "@angular/core/testing";
|
||||
import { ActivatedRouteSnapshot, RouterStateSnapshot } from "@angular/router";
|
||||
|
||||
import { DeviceType } from "@bitwarden/common/enums";
|
||||
|
||||
import { BrowserApi } from "../../../platform/browser/browser-api";
|
||||
import BrowserPopupUtils from "../../../platform/browser/browser-popup-utils";
|
||||
import { BrowserPlatformUtilsService } from "../../../platform/services/platform-utils/browser-platform-utils.service";
|
||||
|
||||
import { firefoxPopoutGuard } from "./firefox-popout.guard";
|
||||
|
||||
describe("firefoxPopoutGuard", () => {
|
||||
let getDeviceSpy: 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: "/import?param=value",
|
||||
} as RouterStateSnapshot;
|
||||
|
||||
beforeEach(() => {
|
||||
getDeviceSpy = jest.spyOn(BrowserPlatformUtilsService, "getDevice");
|
||||
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 browser is Firefox", () => {
|
||||
beforeEach(() => {
|
||||
getDeviceSpy.mockReturnValue(DeviceType.FirefoxExtension);
|
||||
inPopoutSpy.mockReturnValue(false);
|
||||
inSidebarSpy.mockReturnValue(false);
|
||||
});
|
||||
|
||||
it("should open popout and block navigation when not already in popout or sidebar", async () => {
|
||||
const guard = firefoxPopoutGuard();
|
||||
const result = await TestBed.runInInjectionContext(() => guard(mockRoute, mockState));
|
||||
|
||||
expect(getDeviceSpy).toHaveBeenCalledWith(window);
|
||||
expect(inPopoutSpy).toHaveBeenCalledWith(window);
|
||||
expect(inSidebarSpy).toHaveBeenCalledWith(window);
|
||||
expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#/import?param=value");
|
||||
expect(closePopupSpy).toHaveBeenCalledWith(window);
|
||||
expect(result).toBe(false);
|
||||
});
|
||||
|
||||
it("should not add autoClosePopout parameter to the url", async () => {
|
||||
const guard = firefoxPopoutGuard();
|
||||
await TestBed.runInInjectionContext(() => guard(mockRoute, mockState));
|
||||
|
||||
expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#/import?param=value");
|
||||
expect(openPopoutSpy).not.toHaveBeenCalledWith(expect.stringContaining("autoClosePopout"));
|
||||
});
|
||||
|
||||
it("should allow navigation when already in popout", async () => {
|
||||
inPopoutSpy.mockReturnValue(true);
|
||||
|
||||
const guard = firefoxPopoutGuard();
|
||||
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 = firefoxPopoutGuard();
|
||||
const result = await TestBed.runInInjectionContext(() => guard(mockRoute, mockState));
|
||||
|
||||
expect(openPopoutSpy).not.toHaveBeenCalled();
|
||||
expect(closePopupSpy).not.toHaveBeenCalled();
|
||||
expect(result).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe("when browser is not Firefox", () => {
|
||||
beforeEach(() => {
|
||||
inPopoutSpy.mockReturnValue(false);
|
||||
inSidebarSpy.mockReturnValue(false);
|
||||
});
|
||||
|
||||
it.each([
|
||||
{ deviceType: DeviceType.ChromeExtension, name: "ChromeExtension" },
|
||||
{ deviceType: DeviceType.EdgeExtension, name: "EdgeExtension" },
|
||||
{ deviceType: DeviceType.OperaExtension, name: "OperaExtension" },
|
||||
{ deviceType: DeviceType.SafariExtension, name: "SafariExtension" },
|
||||
{ deviceType: DeviceType.VivaldiExtension, name: "VivaldiExtension" },
|
||||
])(
|
||||
"should allow navigation without opening popout when device is $name",
|
||||
async ({ deviceType }) => {
|
||||
getDeviceSpy.mockReturnValue(deviceType);
|
||||
|
||||
const guard = firefoxPopoutGuard();
|
||||
const result = await TestBed.runInInjectionContext(() => guard(mockRoute, mockState));
|
||||
|
||||
expect(getDeviceSpy).toHaveBeenCalledWith(window);
|
||||
expect(openPopoutSpy).not.toHaveBeenCalled();
|
||||
expect(closePopupSpy).not.toHaveBeenCalled();
|
||||
expect(result).toBe(true);
|
||||
},
|
||||
);
|
||||
});
|
||||
|
||||
describe("file picker routes", () => {
|
||||
beforeEach(() => {
|
||||
getDeviceSpy.mockReturnValue(DeviceType.FirefoxExtension);
|
||||
inPopoutSpy.mockReturnValue(false);
|
||||
inSidebarSpy.mockReturnValue(false);
|
||||
});
|
||||
|
||||
it("should open popout for /import route", async () => {
|
||||
const importState: RouterStateSnapshot = {
|
||||
url: "/import",
|
||||
} as RouterStateSnapshot;
|
||||
|
||||
const guard = firefoxPopoutGuard();
|
||||
const result = await TestBed.runInInjectionContext(() => guard(mockRoute, importState));
|
||||
|
||||
expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#/import");
|
||||
expect(closePopupSpy).toHaveBeenCalledWith(window);
|
||||
expect(result).toBe(false);
|
||||
});
|
||||
|
||||
it("should open popout for /add-send route", async () => {
|
||||
const addSendState: RouterStateSnapshot = {
|
||||
url: "/add-send",
|
||||
} as RouterStateSnapshot;
|
||||
|
||||
const guard = firefoxPopoutGuard();
|
||||
const result = await TestBed.runInInjectionContext(() => guard(mockRoute, addSendState));
|
||||
|
||||
expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#/add-send");
|
||||
expect(closePopupSpy).toHaveBeenCalledWith(window);
|
||||
expect(result).toBe(false);
|
||||
});
|
||||
|
||||
it("should open popout for /edit-send route", async () => {
|
||||
const editSendState: RouterStateSnapshot = {
|
||||
url: "/edit-send",
|
||||
} as RouterStateSnapshot;
|
||||
|
||||
const guard = firefoxPopoutGuard();
|
||||
const result = await TestBed.runInInjectionContext(() => guard(mockRoute, editSendState));
|
||||
|
||||
expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#/edit-send");
|
||||
expect(closePopupSpy).toHaveBeenCalledWith(window);
|
||||
expect(result).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe("url handling", () => {
|
||||
beforeEach(() => {
|
||||
getDeviceSpy.mockReturnValue(DeviceType.FirefoxExtension);
|
||||
inPopoutSpy.mockReturnValue(false);
|
||||
inSidebarSpy.mockReturnValue(false);
|
||||
});
|
||||
|
||||
it("should preserve query parameters in the popout url", async () => {
|
||||
const stateWithQuery: RouterStateSnapshot = {
|
||||
url: "/import?foo=bar&baz=qux",
|
||||
} as RouterStateSnapshot;
|
||||
|
||||
const guard = firefoxPopoutGuard();
|
||||
await TestBed.runInInjectionContext(() => guard(mockRoute, stateWithQuery));
|
||||
|
||||
expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#/import?foo=bar&baz=qux");
|
||||
expect(closePopupSpy).toHaveBeenCalledWith(window);
|
||||
});
|
||||
|
||||
it("should handle urls without query parameters", async () => {
|
||||
const stateWithoutQuery: RouterStateSnapshot = {
|
||||
url: "/simple-path",
|
||||
} as RouterStateSnapshot;
|
||||
|
||||
const guard = firefoxPopoutGuard();
|
||||
await TestBed.runInInjectionContext(() => guard(mockRoute, stateWithoutQuery));
|
||||
|
||||
expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#/simple-path");
|
||||
expect(closePopupSpy).toHaveBeenCalledWith(window);
|
||||
});
|
||||
});
|
||||
});
|
||||
38
apps/browser/src/tools/popup/guards/firefox-popout.guard.ts
Normal file
38
apps/browser/src/tools/popup/guards/firefox-popout.guard.ts
Normal file
@@ -0,0 +1,38 @@
|
||||
import { ActivatedRouteSnapshot, CanActivateFn, RouterStateSnapshot } from "@angular/router";
|
||||
|
||||
import { BrowserApi } from "@bitwarden/browser/platform/browser/browser-api";
|
||||
import BrowserPopupUtils from "@bitwarden/browser/platform/browser/browser-popup-utils";
|
||||
import { BrowserPlatformUtilsService } from "@bitwarden/browser/platform/services/platform-utils/browser-platform-utils.service";
|
||||
import { DeviceType } from "@bitwarden/common/enums";
|
||||
|
||||
/**
|
||||
* Guard that forces a popout window on Firefox browser when a file picker could be exposed.
|
||||
* Necessary to avoid a crash: https://bugzilla.mozilla.org/show_bug.cgi?id=1292701
|
||||
* Also disallows the user from closing a popout and re-opening the view exposing the file picker.
|
||||
*
|
||||
* @returns CanActivateFn that opens popout and blocks navigation on Firefox
|
||||
*/
|
||||
export function firefoxPopoutGuard(): CanActivateFn {
|
||||
return async (_route: ActivatedRouteSnapshot, state: RouterStateSnapshot) => {
|
||||
// Check if browser is Firefox using the platform utils service
|
||||
const deviceType = BrowserPlatformUtilsService.getDevice(window);
|
||||
const isFirefox = deviceType === DeviceType.FirefoxExtension;
|
||||
|
||||
// Check if already in popout/sidebar
|
||||
const inPopout = BrowserPopupUtils.inPopout(window);
|
||||
const inSidebar = BrowserPopupUtils.inSidebar(window);
|
||||
|
||||
// Open popout if on Firefox and not already in popout/sidebar
|
||||
if (isFirefox && !inPopout && !inSidebar) {
|
||||
// Don't add autoClosePopout for file picker scenarios - user should manually close
|
||||
await BrowserPopupUtils.openPopout(`popup/index.html#${state.url}`);
|
||||
|
||||
// Close the original popup window
|
||||
BrowserApi.closePopup(window);
|
||||
|
||||
return false; // Block navigation - popout will reload
|
||||
}
|
||||
|
||||
return true; // Allow navigation
|
||||
};
|
||||
}
|
||||
Reference in New Issue
Block a user