zach/chore: review feedback

Author: zachdotai

apps/code/src/main/external-links.test.ts

@@ -1,6 +1,6 @@
 import { beforeEach, describe, expect, it, vi } from "vitest";
 
-const mockOpenExternal = vi.hoisted(() => vi.fn());
+const mockOpenExternal = vi.hoisted(() => vi.fn(() => Promise.resolve()));
 const mockWarn = vi.hoisted(() => vi.fn());
 
 vi.mock("electron", () => ({
@@ -26,7 +26,13 @@ type WillNavigateHandler = (
   url: string,
 ) => void;
 
-function setup() {
+// Packaged renderer served from a file: URL, and dev renderer from the Vite origin.
+const PROD_HOME = new URL(
+  "file:///Applications/PostHog.app/resources/renderer/main_window/index.html",
+);
+const DEV_HOME = new URL("http://localhost:5173");
+
+function setup(appHome: URL) {
   let windowOpenHandler: WindowOpenHandler | undefined;
   let willNavigateHandler: WillNavigateHandler | undefined;
   const window = {
@@ -41,6 +47,7 @@ function setup() {
   };
   setupExternalLinkHandlers(
     window as unknown as Parameters<typeof setupExternalLinkHandlers>[0],
+    appHome,
   );
   if (!windowOpenHandler || !willNavigateHandler) {
     throw new Error("Handlers were not registered");
@@ -66,19 +73,14 @@ const UNSAFE_URLS = [
   "not a url",
 ];
 
-// In production the renderer is served from file://, so the will-navigate
-// handler treats file: URLs as in-app navigation rather than external links.
-const NON_FILE_UNSAFE_URLS = UNSAFE_URLS.filter(
-  (url) => !url.startsWith("file:"),
-);
-
 beforeEach(() => {
   vi.clearAllMocks();
+  mockOpenExternal.mockImplementation(() => Promise.resolve());
 });
 
 describe("window open handler", () => {
   it.each(SAFE_URLS)("opens %s externally and denies the window", (url) => {
-    const { windowOpenHandler } = setup();
+    const { windowOpenHandler } = setup(PROD_HOME);
 
     const result = windowOpenHandler({ url });
 
@@ -87,54 +89,109 @@ describe("window open handler", () => {
   });
 
   it.each(UNSAFE_URLS)("blocks %s without opening it", (url) => {
-    const { windowOpenHandler } = setup();
+    const { windowOpenHandler } = setup(PROD_HOME);
 
     const result = windowOpenHandler({ url });
 
     expect(result).toEqual({ action: "deny" });
     expect(mockOpenExternal).not.toHaveBeenCalled();
     expect(mockWarn).toHaveBeenCalledOnce();
   });
+
+  it("swallows an openExternal rejection instead of leaving it unhandled", async () => {
+    mockOpenExternal.mockImplementationOnce(() =>
+      Promise.reject(new Error("no handler")),
+    );
+    const { windowOpenHandler } = setup(PROD_HOME);
+
+    windowOpenHandler({ url: "https://posthog.com" });
+    await new Promise((resolve) => setTimeout(resolve, 0));
+
+    expect(mockWarn).toHaveBeenCalledOnce();
+  });
 });
 
-describe("will-navigate handler", () => {
-  it.each(SAFE_URLS)(
-    "prevents navigation to %s and opens it externally",
-    (url) => {
-      const { willNavigateHandler } = setup();
-      const preventDefault = vi.fn();
+describe("will-navigate (packaged, file: home)", () => {
+  it.each([
+    "file:///Applications/PostHog.app/resources/renderer/main_window/index.html",
+    "file:///Applications/PostHog.app/resources/renderer/main_window/index.html#/tasks/1",
+    "file:///Applications/PostHog.app/resources/renderer/main_window/assets/app.js",
+  ])("treats in-app file %s as internal navigation", (url) => {
+    const { willNavigateHandler } = setup(PROD_HOME);
+    const preventDefault = vi.fn();
 
-      willNavigateHandler({ preventDefault }, url);
+    willNavigateHandler({ preventDefault }, url);
 
-      expect(preventDefault).toHaveBeenCalledOnce();
-      expect(mockOpenExternal).toHaveBeenCalledExactlyOnceWith(url);
-    },
-  );
+    expect(preventDefault).not.toHaveBeenCalled();
+    expect(mockOpenExternal).not.toHaveBeenCalled();
+  });
 
-  it.each(NON_FILE_UNSAFE_URLS)(
-    "prevents navigation to %s without opening it",
+  it.each([
+    "file:///etc/passwd",
+    "file:///Applications/PostHog.app/resources/renderer/other/index.html",
+  ])("blocks out-of-app file %s (not opened externally either)", (url) => {
+    const { willNavigateHandler } = setup(PROD_HOME);
+    const preventDefault = vi.fn();
+
+    willNavigateHandler({ preventDefault }, url);
+
+    expect(preventDefault).toHaveBeenCalledOnce();
+    expect(mockOpenExternal).not.toHaveBeenCalled();
+    expect(mockWarn).toHaveBeenCalledOnce();
+  });
+
+  it("routes an external https link to the browser", () => {
+    const { willNavigateHandler } = setup(PROD_HOME);
+    const preventDefault = vi.fn();
+
+    willNavigateHandler({ preventDefault }, "https://posthog.com");
+
+    expect(preventDefault).toHaveBeenCalledOnce();
+    expect(mockOpenExternal).toHaveBeenCalledExactlyOnceWith(
+      "https://posthog.com",
+    );
+  });
+});
+
+describe("will-navigate (dev server, http: home)", () => {
+  it.each(["http://localhost:5173/", "http://localhost:5173/sessions/42"])(
+    "treats same-origin dev URL %s as internal navigation",
     (url) => {
-      const { willNavigateHandler } = setup();
+      const { willNavigateHandler } = setup(DEV_HOME);
       const preventDefault = vi.fn();
 
       willNavigateHandler({ preventDefault }, url);
 
-      expect(preventDefault).toHaveBeenCalledOnce();
+      expect(preventDefault).not.toHaveBeenCalled();
       expect(mockOpenExternal).not.toHaveBeenCalled();
-      expect(mockWarn).toHaveBeenCalledOnce();
     },
   );
 
+  // The old startsWith check treated these as in-app, so an attacker origin
+  // could load inside the app window. They must now be punted to the browser:
+  // userinfo that resolves to another host, a longer port, and a scheme swap.
   it.each([
-    "file:///app/.vite/renderer/main_window/index.html",
-    "file:///etc/passwd",
-  ])("treats %s as in-app navigation and never opens it externally", (url) => {
-    const { willNavigateHandler } = setup();
+    "http://localhost:5173@evil.example/",
+    "http://localhost:51730/",
+    "https://localhost:5173/",
+  ])("does not treat lookalike origin %s as internal", (url) => {
+    const { willNavigateHandler } = setup(DEV_HOME);
     const preventDefault = vi.fn();
 
     willNavigateHandler({ preventDefault }, url);
 
-    expect(preventDefault).not.toHaveBeenCalled();
+    expect(preventDefault).toHaveBeenCalledOnce();
+    expect(mockOpenExternal).toHaveBeenCalledExactlyOnceWith(url);
+  });
+
+  it("blocks an unsafe scheme in dev too", () => {
+    const { willNavigateHandler } = setup(DEV_HOME);
+    const preventDefault = vi.fn();
+
+    willNavigateHandler({ preventDefault }, "file:///etc/passwd");
+
+    expect(preventDefault).toHaveBeenCalledOnce();
     expect(mockOpenExternal).not.toHaveBeenCalled();
+    expect(mockWarn).toHaveBeenCalledOnce();
   });
 });

apps/code/src/main/external-links.ts

@@ -4,8 +4,6 @@ import { logger } from "./utils/logger";
 
 const log = logger.scope("external-links");
 
-const MAIN_WINDOW_VITE_DEV_SERVER_URL = process.env.ELECTRON_RENDERER_URL;
-
 function urlScheme(url: string): string {
   try {
     return new URL(url).protocol;
@@ -25,20 +23,54 @@ function openExternalIfSafe(url: string): void {
     });
     return;
   }
-  void shell.openExternal(url);
+  // openExternal rejects when the OS has no handler for the scheme (or the user
+  // dismisses the confirmation prompt on some platforms). Swallow it so a failed
+  // open never surfaces as an unhandled rejection in the main process.
+  shell.openExternal(url).catch((error) => {
+    log.warn("shell.openExternal rejected", { scheme: urlScheme(url), error });
+  });
 }
 
-export function setupExternalLinkHandlers(window: BrowserWindow): void {
+// A navigation is "in-app" only when it targets the exact renderer origin (dev
+// server) or a file under the packaged renderer directory. Comparing parsed
+// URLs — rather than a startsWith prefix — stops lookalikes like
+// http://localhost:5173.evil.example or file:///etc/passwd from being treated
+// as internal and skipping the external-link scheme check below.
+function isInAppNavigation(target: string, appHome: URL): boolean {
+  let parsed: URL;
+  try {
+    parsed = new URL(target);
+  } catch {
+    return false;
+  }
+
+  if (appHome.protocol === "file:") {
+    // file: origins are all opaque ("null"), so pin to the directory that holds
+    // index.html instead of comparing origins.
+    if (parsed.protocol !== "file:") return false;
+    const appDir = appHome.pathname.slice(
+      0,
+      appHome.pathname.lastIndexOf("/") + 1,
+    );
+    return parsed.pathname.startsWith(appDir);
+  }
+
+  // Dev server (http/https): pin scheme + host + port exactly.
+  return parsed.origin === appHome.origin;
+}
+
+export function setupExternalLinkHandlers(
+  window: BrowserWindow,
+  appHome: URL,
+): void {
   window.webContents.setWindowOpenHandler(({ url }) => {
     openExternalIfSafe(url);
     return { action: "deny" };
   });
 
   window.webContents.on("will-navigate", (event, url) => {
-    const appUrl = MAIN_WINDOW_VITE_DEV_SERVER_URL || "file://";
-    if (!url.startsWith(appUrl)) {
-      event.preventDefault();
-      openExternalIfSafe(url);
-    }
+    if (isInAppNavigation(url, appHome)) return;
+    event.preventDefault();
+    openExternalIfSafe(url);
   });
 }

apps/code/src/main/window.ts

@@ -1,5 +1,5 @@
 import path from "node:path";
-import { fileURLToPath } from "node:url";
+import { fileURLToPath, pathToFileURL } from "node:url";
 import { createIPCHandler } from "@posthog/electron-trpc/main";
 import { MAIN_WINDOW_SERVICE } from "@posthog/platform/main-window";
 import { DARK_APP_BACKGROUND_COLOR } from "@posthog/shared/constants";
@@ -305,17 +305,26 @@ export function createWindow(): void {
     },
   });
 
-  setupExternalLinkHandlers(mainWindow);
+  const rendererFilePath = path.join(
+    __dirname,
+    `../renderer/${MAIN_WINDOW_VITE_NAME}/index.html`,
+  );
+  // The URL the renderer is served from, used to tell in-app navigations from
+  // external links. In dev it's the Vite server origin; in prod it's the
+  // packaged index.html file URL.
+  const appHome = MAIN_WINDOW_VITE_DEV_SERVER_URL
+    ? new URL(MAIN_WINDOW_VITE_DEV_SERVER_URL)
+    : pathToFileURL(rendererFilePath);
+
+  setupExternalLinkHandlers(mainWindow, appHome);
   setupEditableContextMenu(mainWindow);
   setupCrashLogging(mainWindow);
   buildApplicationMenu();
 
   if (MAIN_WINDOW_VITE_DEV_SERVER_URL) {
     mainWindow.loadURL(MAIN_WINDOW_VITE_DEV_SERVER_URL);
   } else {
-    mainWindow.loadFile(
-      path.join(__dirname, `../renderer/${MAIN_WINDOW_VITE_NAME}/index.html`),
-    );
+    mainWindow.loadFile(rendererFilePath);
   }
 
   mainWindow.on("closed", () => {