Skip to content

Commit e59bf31

Browse files
zachdotaitatoalo
zachdotai
and
tatoalo
authored
fix(security): validate URL scheme before shell.openExternal (MCP-app sandbox escape) (#3416)
Co-authored-by: Alessandro Pogliaghi <alessandro@posthog.com>
1 parent 5e35d6e commit e59bf31

5 files changed

Lines changed: 294 additions & 23 deletions

File tree

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

Lines changed: 197 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,197 @@
1+
import { beforeEach, describe, expect, it, vi } from "vitest";
2+
3+
const mockOpenExternal = vi.hoisted(() => vi.fn(() => Promise.resolve()));
4+
const mockWarn = vi.hoisted(() => vi.fn());
5+
6+
vi.mock("electron", () => ({
7+
shell: { openExternal: mockOpenExternal },
8+
}));
9+
10+
vi.mock("./utils/logger.js", () => ({
11+
logger: {
12+
scope: () => ({
13+
info: vi.fn(),
14+
error: vi.fn(),
15+
warn: mockWarn,
16+
debug: vi.fn(),
17+
}),
18+
},
19+
}));
20+
21+
import { setupExternalLinkHandlers } from "./external-links";
22+
23+
type WindowOpenHandler = (details: { url: string }) => { action: string };
24+
type WillNavigateHandler = (
25+
event: { preventDefault: () => void },
26+
url: string,
27+
) => void;
28+
29+
// Packaged renderer served from a file: URL, and dev renderer from the Vite origin.
30+
const PROD_HOME = new URL(
31+
"file:///Applications/PostHog.app/resources/renderer/main_window/index.html",
32+
);
33+
const DEV_HOME = new URL("http://localhost:5173");
34+
35+
function setup(appHome: URL) {
36+
let windowOpenHandler: WindowOpenHandler | undefined;
37+
let willNavigateHandler: WillNavigateHandler | undefined;
38+
const window = {
39+
webContents: {
40+
setWindowOpenHandler: (handler: WindowOpenHandler) => {
41+
windowOpenHandler = handler;
42+
},
43+
on: (event: string, handler: WillNavigateHandler) => {
44+
if (event === "will-navigate") willNavigateHandler = handler;
45+
},
46+
},
47+
};
48+
setupExternalLinkHandlers(
49+
window as unknown as Parameters<typeof setupExternalLinkHandlers>[0],
50+
appHome,
51+
);
52+
if (!windowOpenHandler || !willNavigateHandler) {
53+
throw new Error("Handlers were not registered");
54+
}
55+
return { windowOpenHandler, willNavigateHandler };
56+
}
57+
58+
const SAFE_URLS = [
59+
"https://posthog.com/docs",
60+
"http://example.com",
61+
"mailto:support@posthog.com",
62+
];
63+
64+
// Schemes that dispatch to OS-registered handlers: smb/file enable NTLM
65+
// credential theft on Windows, ms-msdt-class handlers take attacker args,
66+
// and custom schemes deep-link into arbitrary installed apps.
67+
const UNSAFE_URLS = [
68+
"smb://attacker.example/share",
69+
"file:///etc/passwd",
70+
"ms-msdt://id/PCWDiagnostic",
71+
"custom-scheme://payload",
72+
"javascript:alert(1)",
73+
"not a url",
74+
];
75+
76+
beforeEach(() => {
77+
vi.clearAllMocks();
78+
mockOpenExternal.mockImplementation(() => Promise.resolve());
79+
});
80+
81+
describe("window open handler", () => {
82+
it.each(SAFE_URLS)("opens %s externally and denies the window", (url) => {
83+
const { windowOpenHandler } = setup(PROD_HOME);
84+
85+
const result = windowOpenHandler({ url });
86+
87+
expect(result).toEqual({ action: "deny" });
88+
expect(mockOpenExternal).toHaveBeenCalledExactlyOnceWith(url);
89+
});
90+
91+
it.each(UNSAFE_URLS)("blocks %s without opening it", (url) => {
92+
const { windowOpenHandler } = setup(PROD_HOME);
93+
94+
const result = windowOpenHandler({ url });
95+
96+
expect(result).toEqual({ action: "deny" });
97+
expect(mockOpenExternal).not.toHaveBeenCalled();
98+
expect(mockWarn).toHaveBeenCalledOnce();
99+
});
100+
101+
it("swallows an openExternal rejection instead of leaving it unhandled", async () => {
102+
mockOpenExternal.mockImplementationOnce(() =>
103+
Promise.reject(new Error("no handler")),
104+
);
105+
const { windowOpenHandler } = setup(PROD_HOME);
106+
107+
windowOpenHandler({ url: "https://posthog.com" });
108+
await new Promise((resolve) => setTimeout(resolve, 0));
109+
110+
expect(mockWarn).toHaveBeenCalledOnce();
111+
});
112+
});
113+
114+
describe("will-navigate (packaged, file: home)", () => {
115+
it.each([
116+
"file:///Applications/PostHog.app/resources/renderer/main_window/index.html",
117+
"file:///Applications/PostHog.app/resources/renderer/main_window/index.html#/tasks/1",
118+
"file:///Applications/PostHog.app/resources/renderer/main_window/assets/app.js",
119+
])("treats in-app file %s as internal navigation", (url) => {
120+
const { willNavigateHandler } = setup(PROD_HOME);
121+
const preventDefault = vi.fn();
122+
123+
willNavigateHandler({ preventDefault }, url);
124+
125+
expect(preventDefault).not.toHaveBeenCalled();
126+
expect(mockOpenExternal).not.toHaveBeenCalled();
127+
});
128+
129+
it.each([
130+
"file:///etc/passwd",
131+
"file:///Applications/PostHog.app/resources/renderer/other/index.html",
132+
])("blocks out-of-app file %s (not opened externally either)", (url) => {
133+
const { willNavigateHandler } = setup(PROD_HOME);
134+
const preventDefault = vi.fn();
135+
136+
willNavigateHandler({ preventDefault }, url);
137+
138+
expect(preventDefault).toHaveBeenCalledOnce();
139+
expect(mockOpenExternal).not.toHaveBeenCalled();
140+
expect(mockWarn).toHaveBeenCalledOnce();
141+
});
142+
143+
it("routes an external https link to the browser", () => {
144+
const { willNavigateHandler } = setup(PROD_HOME);
145+
const preventDefault = vi.fn();
146+
147+
willNavigateHandler({ preventDefault }, "https://posthog.com");
148+
149+
expect(preventDefault).toHaveBeenCalledOnce();
150+
expect(mockOpenExternal).toHaveBeenCalledExactlyOnceWith(
151+
"https://posthog.com",
152+
);
153+
});
154+
});
155+
156+
describe("will-navigate (dev server, http: home)", () => {
157+
it.each(["http://localhost:5173/", "http://localhost:5173/sessions/42"])(
158+
"treats same-origin dev URL %s as internal navigation",
159+
(url) => {
160+
const { willNavigateHandler } = setup(DEV_HOME);
161+
const preventDefault = vi.fn();
162+
163+
willNavigateHandler({ preventDefault }, url);
164+
165+
expect(preventDefault).not.toHaveBeenCalled();
166+
expect(mockOpenExternal).not.toHaveBeenCalled();
167+
},
168+
);
169+
170+
// The old startsWith check treated these as in-app, so an attacker origin
171+
// could load inside the app window. They must now be punted to the browser:
172+
// userinfo that resolves to another host, a longer port, and a scheme swap.
173+
it.each([
174+
"http://localhost:5173@evil.example/",
175+
"http://localhost:51730/",
176+
"https://localhost:5173/",
177+
])("does not treat lookalike origin %s as internal", (url) => {
178+
const { willNavigateHandler } = setup(DEV_HOME);
179+
const preventDefault = vi.fn();
180+
181+
willNavigateHandler({ preventDefault }, url);
182+
183+
expect(preventDefault).toHaveBeenCalledOnce();
184+
expect(mockOpenExternal).toHaveBeenCalledExactlyOnceWith(url);
185+
});
186+
187+
it("blocks an unsafe scheme in dev too", () => {
188+
const { willNavigateHandler } = setup(DEV_HOME);
189+
const preventDefault = vi.fn();
190+
191+
willNavigateHandler({ preventDefault }, "file:///etc/passwd");
192+
193+
expect(preventDefault).toHaveBeenCalledOnce();
194+
expect(mockOpenExternal).not.toHaveBeenCalled();
195+
expect(mockWarn).toHaveBeenCalledOnce();
196+
});
197+
});

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

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
import { isSafeExternalUrl } from "@posthog/shared";
2+
import { type BrowserWindow, shell } from "electron";
3+
import { logger } from "./utils/logger";
4+
5+
const log = logger.scope("external-links");
6+
7+
function urlScheme(url: string): string {
8+
try {
9+
return new URL(url).protocol;
10+
} catch {
11+
return "<unparseable>";
12+
}
13+
}
14+
15+
// `shell.openExternal` dispatches to whatever app the OS registered for the
16+
// scheme, so it must never receive a scheme outside the http/https/mailto
17+
// allowlist: renderer content (including sandboxed MCP apps) can reach these
18+
// handlers via window.open/navigation with e.g. smb:, file:, or ms-msdt: URLs.
19+
function openExternalIfSafe(url: string): void {
20+
if (!isSafeExternalUrl(url)) {
21+
log.warn("Blocked externally-opened URL with disallowed scheme", {
22+
scheme: urlScheme(url),
23+
});
24+
return;
25+
}
26+
// openExternal rejects when the OS has no handler for the scheme (or the user
27+
// dismisses the confirmation prompt on some platforms). Swallow it so a failed
28+
// open never surfaces as an unhandled rejection in the main process.
29+
shell.openExternal(url).catch((error) => {
30+
log.warn("shell.openExternal rejected", { scheme: urlScheme(url), error });
31+
});
32+
}
33+
34+
// A navigation is "in-app" only when it targets the exact renderer origin (dev
35+
// server) or a file under the packaged renderer directory. Comparing parsed
36+
// URLs — rather than a startsWith prefix — stops lookalikes like
37+
// http://localhost:5173.evil.example or file:///etc/passwd from being treated
38+
// as internal and skipping the external-link scheme check below.
39+
function isInAppNavigation(target: string, appHome: URL): boolean {
40+
let parsed: URL;
41+
try {
42+
parsed = new URL(target);
43+
} catch {
44+
return false;
45+
}
46+
47+
if (appHome.protocol === "file:") {
48+
// file: origins are all opaque ("null"), so pin to the directory that holds
49+
// index.html instead of comparing origins.
50+
if (parsed.protocol !== "file:") return false;
51+
const appDir = appHome.pathname.slice(
52+
0,
53+
appHome.pathname.lastIndexOf("/") + 1,
54+
);
55+
return parsed.pathname.startsWith(appDir);
56+
}
57+
58+
// Dev server (http/https): pin scheme + host + port exactly.
59+
return parsed.origin === appHome.origin;
60+
}
61+
62+
export function setupExternalLinkHandlers(
63+
window: BrowserWindow,
64+
appHome: URL,
65+
): void {
66+
window.webContents.setWindowOpenHandler(({ url }) => {
67+
openExternalIfSafe(url);
68+
return { action: "deny" };
69+
});
70+
71+
window.webContents.on("will-navigate", (event, url) => {
72+
if (isInAppNavigation(url, appHome)) return;
73+
event.preventDefault();
74+
openExternalIfSafe(url);
75+
});
76+
}

‎apps/code/src/main/window.ts‎

Lines changed: 15 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import path from "node:path";
2-
import { fileURLToPath } from "node:url";
2+
import { fileURLToPath, pathToFileURL } from "node:url";
33
import { createIPCHandler } from "@posthog/electron-trpc/main";
44
import { MAIN_WINDOW_SERVICE } from "@posthog/platform/main-window";
55
import { DARK_APP_BACKGROUND_COLOR } from "@posthog/shared/constants";
@@ -9,9 +9,9 @@ import {
99
Menu,
1010
type MenuItemConstructorOptions,
1111
screen,
12-
shell,
1312
} from "electron";
1413
import { container } from "./di/container";
14+
import { setupExternalLinkHandlers } from "./external-links";
1515
import { buildApplicationMenu } from "./menu";
1616
import type { ElectronMainWindow } from "./platform-adapters/electron-main-window";
1717
import { posthogNodeAnalytics } from "./platform-adapters/posthog-analytics";
@@ -120,21 +120,6 @@ export function focusMainWindow(reason: string): void {
120120
}
121121
}
122122

123-
function setupExternalLinkHandlers(window: BrowserWindow): void {
124-
window.webContents.setWindowOpenHandler(({ url }) => {
125-
shell.openExternal(url);
126-
return { action: "deny" };
127-
});
128-
129-
window.webContents.on("will-navigate", (event, url) => {
130-
const appUrl = MAIN_WINDOW_VITE_DEV_SERVER_URL || "file://";
131-
if (!url.startsWith(appUrl)) {
132-
event.preventDefault();
133-
shell.openExternal(url);
134-
}
135-
});
136-
}
137-
138123
function setupCrashLogging(window: BrowserWindow): void {
139124
window.webContents.on("render-process-gone", (_event, details) => {
140125
log.error("Renderer process gone", {
@@ -341,17 +326,26 @@ export function createWindow(): void {
341326
},
342327
});
343328

344-
setupExternalLinkHandlers(mainWindow);
329+
const rendererFilePath = path.join(
330+
__dirname,
331+
`../renderer/${MAIN_WINDOW_VITE_NAME}/index.html`,
332+
);
333+
// The URL the renderer is served from, used to tell in-app navigations from
334+
// external links. In dev it's the Vite server origin; in prod it's the
335+
// packaged index.html file URL.
336+
const appHome = MAIN_WINDOW_VITE_DEV_SERVER_URL
337+
? new URL(MAIN_WINDOW_VITE_DEV_SERVER_URL)
338+
: pathToFileURL(rendererFilePath);
339+
340+
setupExternalLinkHandlers(mainWindow, appHome);
345341
setupEditableContextMenu(mainWindow);
346342
setupCrashLogging(mainWindow);
347343
buildApplicationMenu();
348344

349345
if (MAIN_WINDOW_VITE_DEV_SERVER_URL) {
350346
mainWindow.loadURL(MAIN_WINDOW_VITE_DEV_SERVER_URL);
351347
} else {
352-
mainWindow.loadFile(
353-
path.join(__dirname, `../renderer/${MAIN_WINDOW_VITE_NAME}/index.html`),
354-
);
348+
mainWindow.loadFile(rendererFilePath);
355349
}
356350

357351
mainWindow.on("closed", () => {

‎biome.jsonc‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,13 +71,14 @@
7171
// Main-process code must not import from "electron" directly. Every Electron
7272
// API goes through a port in @posthog/platform, implemented by an adapter in
7373
// apps/code/src/main/platform-adapters/. The only files allowed to import
74-
// electron are the adapters and the 8 Electron host files below.
74+
// electron are the adapters and the Electron host files below.
7575
"includes": [
7676
"apps/code/src/main/**/*.ts",
7777
"!apps/code/src/main/platform-adapters/**",
7878
"!apps/code/src/main/bootstrap.ts",
7979
"!apps/code/src/main/index.ts",
8080
"!apps/code/src/main/window.ts",
81+
"!apps/code/src/main/external-links.ts",
8182
"!apps/code/src/main/menu.ts",
8283
"!apps/code/src/main/preload.ts",
8384
"!apps/code/src/main/deep-links.ts",

‎packages/ui/src/features/mcp-apps/components/McpAppHost.tsx‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -239,7 +239,10 @@ export function McpAppHost({
239239
<iframe
240240
ref={setIframeEl}
241241
src={sandboxProxyUrl}
242-
sandbox="allow-scripts allow-same-origin allow-forms allow-popups allow-popups-to-escape-sandbox allow-presentation"
242+
// No allow-popups: app JS is same-origin with the proxy realm, so popup
243+
// permission here would let it window.open() past the sandbox. Apps
244+
// open links via ui/open-link, which the host scheme-validates.
245+
sandbox="allow-scripts allow-same-origin allow-forms allow-presentation"
243246
style={{
244247
height: displayMode === "fullscreen" ? "100%" : `${iframeHeight}px`,
245248
}}

0 commit comments

Comments
 (0)