diff --git a/apps/code/src/main/external-links.test.ts b/apps/code/src/main/external-links.test.ts new file mode 100644 index 0000000000..224fe62c3f --- /dev/null +++ b/apps/code/src/main/external-links.test.ts @@ -0,0 +1,197 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +const mockOpenExternal = vi.hoisted(() => vi.fn(() => Promise.resolve())); +const mockWarn = vi.hoisted(() => vi.fn()); + +vi.mock("electron", () => ({ + shell: { openExternal: mockOpenExternal }, +})); + +vi.mock("./utils/logger.js", () => ({ + logger: { + scope: () => ({ + info: vi.fn(), + error: vi.fn(), + warn: mockWarn, + debug: vi.fn(), + }), + }, +})); + +import { setupExternalLinkHandlers } from "./external-links"; + +type WindowOpenHandler = (details: { url: string }) => { action: string }; +type WillNavigateHandler = ( + event: { preventDefault: () => void }, + url: string, +) => void; + +// 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 = { + webContents: { + setWindowOpenHandler: (handler: WindowOpenHandler) => { + windowOpenHandler = handler; + }, + on: (event: string, handler: WillNavigateHandler) => { + if (event === "will-navigate") willNavigateHandler = handler; + }, + }, + }; + setupExternalLinkHandlers( + window as unknown as Parameters[0], + appHome, + ); + if (!windowOpenHandler || !willNavigateHandler) { + throw new Error("Handlers were not registered"); + } + return { windowOpenHandler, willNavigateHandler }; +} + +const SAFE_URLS = [ + "https://posthog.com/docs", + "http://example.com", + "mailto:support@posthog.com", +]; + +// Schemes that dispatch to OS-registered handlers: smb/file enable NTLM +// credential theft on Windows, ms-msdt-class handlers take attacker args, +// and custom schemes deep-link into arbitrary installed apps. +const UNSAFE_URLS = [ + "smb://attacker.example/share", + "file:///etc/passwd", + "ms-msdt://id/PCWDiagnostic", + "custom-scheme://payload", + "javascript:alert(1)", + "not a url", +]; + +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(PROD_HOME); + + const result = windowOpenHandler({ url }); + + expect(result).toEqual({ action: "deny" }); + expect(mockOpenExternal).toHaveBeenCalledExactlyOnceWith(url); + }); + + it.each(UNSAFE_URLS)("blocks %s without opening it", (url) => { + 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 (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); + + expect(preventDefault).not.toHaveBeenCalled(); + expect(mockOpenExternal).not.toHaveBeenCalled(); + }); + + 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(DEV_HOME); + const preventDefault = vi.fn(); + + willNavigateHandler({ preventDefault }, url); + + expect(preventDefault).not.toHaveBeenCalled(); + expect(mockOpenExternal).not.toHaveBeenCalled(); + }, + ); + + // 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([ + "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).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(); + }); +}); diff --git a/apps/code/src/main/external-links.ts b/apps/code/src/main/external-links.ts new file mode 100644 index 0000000000..4989c16fd4 --- /dev/null +++ b/apps/code/src/main/external-links.ts @@ -0,0 +1,76 @@ +import { isSafeExternalUrl } from "@posthog/shared"; +import { type BrowserWindow, shell } from "electron"; +import { logger } from "./utils/logger"; + +const log = logger.scope("external-links"); + +function urlScheme(url: string): string { + try { + return new URL(url).protocol; + } catch { + return ""; + } +} + +// `shell.openExternal` dispatches to whatever app the OS registered for the +// scheme, so it must never receive a scheme outside the http/https/mailto +// allowlist: renderer content (including sandboxed MCP apps) can reach these +// handlers via window.open/navigation with e.g. smb:, file:, or ms-msdt: URLs. +function openExternalIfSafe(url: string): void { + if (!isSafeExternalUrl(url)) { + log.warn("Blocked externally-opened URL with disallowed scheme", { + scheme: urlScheme(url), + }); + return; + } + // 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 }); + }); +} + +// 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) => { + if (isInAppNavigation(url, appHome)) return; + event.preventDefault(); + openExternalIfSafe(url); + }); +} diff --git a/apps/code/src/main/window.ts b/apps/code/src/main/window.ts index 69f3e8bc01..62edd9f0ea 100644 --- a/apps/code/src/main/window.ts +++ b/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"; @@ -9,9 +9,9 @@ import { Menu, type MenuItemConstructorOptions, screen, - shell, } from "electron"; import { container } from "./di/container"; +import { setupExternalLinkHandlers } from "./external-links"; import { buildApplicationMenu } from "./menu"; import type { ElectronMainWindow } from "./platform-adapters/electron-main-window"; import { posthogNodeAnalytics } from "./platform-adapters/posthog-analytics"; @@ -120,21 +120,6 @@ export function focusMainWindow(reason: string): void { } } -function setupExternalLinkHandlers(window: BrowserWindow): void { - window.webContents.setWindowOpenHandler(({ url }) => { - shell.openExternal(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(); - shell.openExternal(url); - } - }); -} - function setupCrashLogging(window: BrowserWindow): void { window.webContents.on("render-process-gone", (_event, details) => { log.error("Renderer process gone", { @@ -341,7 +326,18 @@ 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(); @@ -349,9 +345,7 @@ export function createWindow(): void { 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", () => { diff --git a/biome.jsonc b/biome.jsonc index f2bf55073a..b5697eaf7b 100644 --- a/biome.jsonc +++ b/biome.jsonc @@ -71,13 +71,14 @@ // Main-process code must not import from "electron" directly. Every Electron // API goes through a port in @posthog/platform, implemented by an adapter in // apps/code/src/main/platform-adapters/. The only files allowed to import - // electron are the adapters and the 8 Electron host files below. + // electron are the adapters and the Electron host files below. "includes": [ "apps/code/src/main/**/*.ts", "!apps/code/src/main/platform-adapters/**", "!apps/code/src/main/bootstrap.ts", "!apps/code/src/main/index.ts", "!apps/code/src/main/window.ts", + "!apps/code/src/main/external-links.ts", "!apps/code/src/main/menu.ts", "!apps/code/src/main/preload.ts", "!apps/code/src/main/deep-links.ts", diff --git a/packages/ui/src/features/mcp-apps/components/McpAppHost.tsx b/packages/ui/src/features/mcp-apps/components/McpAppHost.tsx index c6a095d2ec..30c082ef35 100644 --- a/packages/ui/src/features/mcp-apps/components/McpAppHost.tsx +++ b/packages/ui/src/features/mcp-apps/components/McpAppHost.tsx @@ -239,7 +239,10 @@ export function McpAppHost({