From b9b230b345d93b7107baab5b0d5c74b0bd94f653 Mon Sep 17 00:00:00 2001 From: Ahmed Aly Date: Tue, 14 Jul 2026 02:40:18 -0700 Subject: [PATCH 1/2] fix(security): validate URL scheme before shell.openExternal (MCP-app sandbox escape) Untrusted MCP-app HTML could reach shell.openExternal with an arbitrary URL scheme, bypassing the http/https/mailto allowlist enforced elsewhere. A malicious MCP server could launch OS-registered scheme handlers from inside the sandbox (smb:/file: for NTLM theft, ms-msdt:-class handlers, app deep-links). - Extract the window-open / will-navigate handlers into external-links.ts and validate every URL with isSafeExternalUrl before shell.openExternal; log and drop anything outside http/https/mailto. - Remove allow-popups / allow-popups-to-escape-sandbox from the MCP-apps outer iframe, closing the proxy-realm window.open path that reached the sink. --- apps/code/src/main/external-links.test.ts | 140 ++++++++++++++++++ apps/code/src/main/external-links.ts | 44 ++++++ apps/code/src/main/window.ts | 17 +-- biome.jsonc | 3 +- .../mcp-apps/components/McpAppHost.tsx | 5 +- 5 files changed, 191 insertions(+), 18 deletions(-) create mode 100644 apps/code/src/main/external-links.test.ts create mode 100644 apps/code/src/main/external-links.ts 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..53b6d21788 --- /dev/null +++ b/apps/code/src/main/external-links.test.ts @@ -0,0 +1,140 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +const mockOpenExternal = vi.hoisted(() => vi.fn()); +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; + +function setup() { + 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], + ); + 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", +]; + +// 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(); +}); + +describe("window open handler", () => { + it.each(SAFE_URLS)("opens %s externally and denies the window", (url) => { + const { windowOpenHandler } = setup(); + + 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(); + + const result = windowOpenHandler({ url }); + + expect(result).toEqual({ action: "deny" }); + expect(mockOpenExternal).not.toHaveBeenCalled(); + 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(); + + willNavigateHandler({ preventDefault }, url); + + expect(preventDefault).toHaveBeenCalledOnce(); + expect(mockOpenExternal).toHaveBeenCalledExactlyOnceWith(url); + }, + ); + + it.each(NON_FILE_UNSAFE_URLS)( + "prevents navigation to %s without opening it", + (url) => { + const { willNavigateHandler } = setup(); + const preventDefault = vi.fn(); + + willNavigateHandler({ preventDefault }, url); + + expect(preventDefault).toHaveBeenCalledOnce(); + expect(mockOpenExternal).not.toHaveBeenCalled(); + expect(mockWarn).toHaveBeenCalledOnce(); + }, + ); + + 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(); + const preventDefault = vi.fn(); + + willNavigateHandler({ preventDefault }, url); + + expect(preventDefault).not.toHaveBeenCalled(); + expect(mockOpenExternal).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/code/src/main/external-links.ts b/apps/code/src/main/external-links.ts new file mode 100644 index 0000000000..9c74e14e88 --- /dev/null +++ b/apps/code/src/main/external-links.ts @@ -0,0 +1,44 @@ +import { isSafeExternalUrl } from "@posthog/shared"; +import { type BrowserWindow, shell } from "electron"; +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; + } 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; + } + void shell.openExternal(url); +} + +export function setupExternalLinkHandlers(window: BrowserWindow): 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); + } + }); +} diff --git a/apps/code/src/main/window.ts b/apps/code/src/main/window.ts index 7951613c9a..b0ff10f398 100644 --- a/apps/code/src/main/window.ts +++ b/apps/code/src/main/window.ts @@ -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"; @@ -114,21 +114,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", { 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({