From 541d216df12c041f7d4d59cce464b1bfa0121ad0 Mon Sep 17 00:00:00 2001
From: John Harrington <84741727+harr1424@users.noreply.github.com>
Date: Fri, 12 Dec 2025 08:28:53 -0700
Subject: [PATCH] enforce popout on Safari with test coverage

---
 .../guards/file-picker-popout.guard.spec.ts   | 75 +++++++++++--------
 .../popup/guards/file-picker-popout.guard.ts  | 26 +++----
 2 files changed, 54 insertions(+), 47 deletions(-)

diff --git a/apps/browser/src/tools/popup/guards/file-picker-popout.guard.spec.ts b/apps/browser/src/tools/popup/guards/file-picker-popout.guard.spec.ts
index 8f5a7178699..4764b787393 100644
--- a/apps/browser/src/tools/popup/guards/file-picker-popout.guard.spec.ts
+++ b/apps/browser/src/tools/popup/guards/file-picker-popout.guard.spec.ts
@@ -84,15 +84,40 @@ describe("filePickerPopoutGuard", () => {
       inSidebarSpy.mockReturnValue(false);
     });
 
-    it("should allow navigation without popout requirement", async () => {
+    it("should open popout and block navigation when not in popout", async () => {
       const guard = filePickerPopoutGuard();
       const result = await TestBed.runInInjectionContext(() => guard(mockRoute, mockState));
 
       expect(getDeviceSpy).toHaveBeenCalledWith(window);
+      expect(inPopoutSpy).toHaveBeenCalledWith(window);
+      expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#/add-send?type=1");
+      expect(closePopupSpy).toHaveBeenCalledWith(window);
+      expect(result).toBe(false);
+    });
+
+    it("should allow navigation when already in popout", async () => {
+      inPopoutSpy.mockReturnValue(true);
+
+      const guard = filePickerPopoutGuard();
+      const result = await TestBed.runInInjectionContext(() => guard(mockRoute, mockState));
+
       expect(openPopoutSpy).not.toHaveBeenCalled();
       expect(closePopupSpy).not.toHaveBeenCalled();
       expect(result).toBe(true);
     });
+
+    it("should not allow sidebar bypass (Safari doesn't support sidebar)", async () => {
+      inSidebarSpy.mockReturnValue(true);
+      inPopoutSpy.mockReturnValue(false);
+
+      const guard = filePickerPopoutGuard();
+      const result = await TestBed.runInInjectionContext(() => guard(mockRoute, mockState));
+
+      // Safari requires popout, sidebar is not sufficient
+      expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#/add-send?type=1");
+      expect(closePopupSpy).toHaveBeenCalledWith(window);
+      expect(result).toBe(false);
+    });
   });
 
   describe("Chromium browsers on Linux", () => {
@@ -167,28 +192,24 @@ describe("filePickerPopoutGuard", () => {
       });
     });
 
-    it("should open popout and block navigation for Chrome on Mac when not in popout or sidebar", async () => {
-      getDeviceSpy.mockReturnValue(DeviceType.ChromeExtension);
+    it.each([
+      { deviceType: DeviceType.ChromeExtension, name: "Chrome" },
+      { deviceType: DeviceType.EdgeExtension, name: "Edge" },
+      { deviceType: DeviceType.OperaExtension, name: "Opera" },
+      { deviceType: DeviceType.VivaldiExtension, name: "Vivaldi" },
+    ])(
+      "should open popout and block navigation for $name on Mac when not in popout or sidebar",
+      async ({ deviceType }) => {
+        getDeviceSpy.mockReturnValue(deviceType);
 
-      const guard = filePickerPopoutGuard();
-      const result = await TestBed.runInInjectionContext(() => guard(mockRoute, mockState));
+        const guard = filePickerPopoutGuard();
+        const result = await TestBed.runInInjectionContext(() => guard(mockRoute, mockState));
 
-      expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#/add-send?type=1");
-      expect(closePopupSpy).toHaveBeenCalledWith(window);
-      expect(result).toBe(false);
-    });
-
-    it("should not require popout for Edge on Mac (not Chrome)", async () => {
-      getDeviceSpy.mockReturnValue(DeviceType.EdgeExtension);
-
-      const guard = filePickerPopoutGuard();
-      const result = await TestBed.runInInjectionContext(() => guard(mockRoute, mockState));
-
-      // Edge/Opera/Vivaldi on Mac don't match the Chrome-specific Mac check
-      expect(openPopoutSpy).not.toHaveBeenCalled();
-      expect(closePopupSpy).not.toHaveBeenCalled();
-      expect(result).toBe(true);
-    });
+        expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#/add-send?type=1");
+        expect(closePopupSpy).toHaveBeenCalledWith(window);
+        expect(result).toBe(false);
+      },
+    );
 
     it("should allow navigation when in popout", async () => {
       getDeviceSpy.mockReturnValue(DeviceType.ChromeExtension);
@@ -299,18 +320,6 @@ describe("filePickerPopoutGuard", () => {
       expect(closePopupSpy).toHaveBeenCalledWith(window);
       expect(result).toBe(false);
     });
-
-    it("should preserve query parameters on file picker routes", async () => {
-      const editSendWithParams: RouterStateSnapshot = {
-        url: "/edit-send?sendId=123&type=1",
-      } as RouterStateSnapshot;
-
-      const guard = filePickerPopoutGuard();
-      await TestBed.runInInjectionContext(() => guard(mockRoute, editSendWithParams));
-
-      expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#/edit-send?sendId=123&type=1");
-      expect(closePopupSpy).toHaveBeenCalledWith(window);
-    });
   });
 
   describe("url handling", () => {
diff --git a/apps/browser/src/tools/popup/guards/file-picker-popout.guard.ts b/apps/browser/src/tools/popup/guards/file-picker-popout.guard.ts
index 461901085e5..1827f0c384e 100644
--- a/apps/browser/src/tools/popup/guards/file-picker-popout.guard.ts
+++ b/apps/browser/src/tools/popup/guards/file-picker-popout.guard.ts
@@ -11,9 +11,9 @@ import { DeviceType } from "@bitwarden/common/enums";
  *
  * Browser-specific requirements:
  * - Firefox: Requires sidebar OR popout (crashes with file picker in popup: https://bugzilla.mozilla.org/show_bug.cgi?id=1292701)
- * - Chromium on Linux/Mac: Requires sidebar OR popout
+ * - Safari: Requires popout only
+ * - All Chromium browsers (Chrome, Edge, Opera, Vivaldi) on Linux/Mac: Requires sidebar OR popout
  * - Chromium on Windows: No special requirement
- * - Safari: No special requirement
  *
  * @returns CanActivateFn that opens popout and blocks navigation when file picker access is needed
  */
@@ -33,8 +33,14 @@ export function filePickerPopoutGuard(): CanActivateFn {
       needsPopout = true;
     }
 
-    // Chromium on Linux: needs sidebar OR popout for file picker access
-    // All Chromium-based browsers (Chrome, Edge, Opera, Vivaldi) on Linux
+    // Safari: needs popout only (sidebar not available)
+    if (deviceType === DeviceType.SafariExtension && !inPopout) {
+      needsPopout = true;
+    }
+
+    // Chromium on Linux/Mac: needs sidebar OR popout for file picker access
+    // All Chromium-based browsers (Chrome, Edge, Opera, Vivaldi)
+    // Brave intentionally reports itself as Chrome for compatibility
     const isChromiumBased = [
       DeviceType.ChromeExtension,
       DeviceType.EdgeExtension,
@@ -43,17 +49,9 @@ export function filePickerPopoutGuard(): CanActivateFn {
     ].includes(deviceType);
 
     const isLinux = window?.navigator?.userAgent?.indexOf("Linux") !== -1;
+    const isMac = window?.navigator?.appVersion?.includes("Mac OS X");
 
-    if (isChromiumBased && isLinux && !inPopout && !inSidebar) {
-      needsPopout = true;
-    }
-
-    // Chrome (specifically) on Mac: needs sidebar OR popout for file picker access
-    const isMac =
-      deviceType === DeviceType.ChromeExtension &&
-      window?.navigator?.appVersion.includes("Mac OS X");
-
-    if (isMac && !inPopout && !inSidebar) {
+    if (isChromiumBased && (isLinux || isMac) && !inPopout && !inSidebar) {
       needsPopout = true;
     }