diff --git a/apps/code/src/main/bootstrap.ts b/apps/code/src/main/bootstrap.ts index 87f850cb82..e025a3c5e5 100644 --- a/apps/code/src/main/bootstrap.ts +++ b/apps/code/src/main/bootstrap.ts @@ -42,7 +42,9 @@ app.setName(isDev ? "PostHog Code (Development)" : "PostHog Code"); // Set userData path for @posthog/code const appDataPath = app.getPath("appData"); -const userDataPath = path.join(appDataPath, "@posthog", appName); +const userDataPath = + process.env.POSTHOG_E2E_USER_DATA_DIR ?? + path.join(appDataPath, "@posthog", appName); app.setPath("userData", userDataPath); // Export the electron-derived state to env so utility singletons (utils/*, diff --git a/apps/code/src/main/external-links.test.ts b/apps/code/src/main/external-links.test.ts index 224fe62c3f..81b97be425 100644 --- a/apps/code/src/main/external-links.test.ts +++ b/apps/code/src/main/external-links.test.ts @@ -18,13 +18,33 @@ vi.mock("./utils/logger.js", () => ({ }, })); -import { setupExternalLinkHandlers } from "./external-links"; +import { + setupExternalLinkHandlers, + setupExternalLinkPermissionHandlers, +} from "./external-links"; type WindowOpenHandler = (details: { url: string }) => { action: string }; type WillNavigateHandler = ( event: { preventDefault: () => void }, url: string, ) => void; +type WillFrameNavigateHandler = (details: { + preventDefault: () => void; + isMainFrame: boolean; + url: string; +}) => void; +type PermissionCheckHandler = ( + webContents: unknown, + permission: string, + requestingOrigin: string, + details: { isMainFrame: boolean }, +) => boolean; +type PermissionRequestHandler = ( + webContents: unknown, + permission: string, + callback: (permissionGranted: boolean) => void, + details: Record, +) => void; // Packaged renderer served from a file: URL, and dev renderer from the Vite origin. const PROD_HOME = new URL( @@ -35,13 +55,22 @@ const DEV_HOME = new URL("http://localhost:5173"); function setup(appHome: URL) { let windowOpenHandler: WindowOpenHandler | undefined; let willNavigateHandler: WillNavigateHandler | undefined; + let willFrameNavigateHandler: WillFrameNavigateHandler | undefined; const window = { webContents: { setWindowOpenHandler: (handler: WindowOpenHandler) => { windowOpenHandler = handler; }, - on: (event: string, handler: WillNavigateHandler) => { - if (event === "will-navigate") willNavigateHandler = handler; + on: ( + event: string, + handler: WillNavigateHandler | WillFrameNavigateHandler, + ) => { + if (event === "will-navigate") { + willNavigateHandler = handler as WillNavigateHandler; + } + if (event === "will-frame-navigate") { + willFrameNavigateHandler = handler as WillFrameNavigateHandler; + } }, }, }; @@ -49,10 +78,37 @@ function setup(appHome: URL) { window as unknown as Parameters[0], appHome, ); - if (!windowOpenHandler || !willNavigateHandler) { + if (!windowOpenHandler || !willNavigateHandler || !willFrameNavigateHandler) { throw new Error("Handlers were not registered"); } - return { windowOpenHandler, willNavigateHandler }; + return { + windowOpenHandler, + willNavigateHandler, + willFrameNavigateHandler, + }; +} + +function setupPermissionHandlers() { + let permissionCheckHandler: PermissionCheckHandler | undefined; + let permissionRequestHandler: PermissionRequestHandler | undefined; + const session = { + setPermissionCheckHandler: (handler: PermissionCheckHandler) => { + permissionCheckHandler = handler; + }, + setPermissionRequestHandler: (handler: PermissionRequestHandler) => { + permissionRequestHandler = handler; + }, + }; + + setupExternalLinkPermissionHandlers( + session as unknown as Parameters< + typeof setupExternalLinkPermissionHandlers + >[0], + ); + if (!permissionCheckHandler || !permissionRequestHandler) { + throw new Error("Permission handlers were not registered"); + } + return { permissionCheckHandler, permissionRequestHandler }; } const SAFE_URLS = [ @@ -78,120 +134,199 @@ beforeEach(() => { 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); +describe("external link policies", () => { + 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 }); + const result = windowOpenHandler({ url }); - expect(result).toEqual({ action: "deny" }); - expect(mockOpenExternal).toHaveBeenCalledExactlyOnceWith(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); + it.each(UNSAFE_URLS)("blocks %s without opening it", (url) => { + const { windowOpenHandler } = setup(PROD_HOME); - const result = windowOpenHandler({ url }); + const result = windowOpenHandler({ url }); - expect(result).toEqual({ action: "deny" }); - expect(mockOpenExternal).not.toHaveBeenCalled(); - expect(mockWarn).toHaveBeenCalledOnce(); - }); + 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); + 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)); + windowOpenHandler({ url: "https://posthog.com" }); + await Promise.resolve(); - expect(mockWarn).toHaveBeenCalledOnce(); + 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(); + 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/index.html?source=reload", + ])("treats renderer entry file %s as internal navigation", (url) => { + const { willNavigateHandler } = setup(PROD_HOME); + const preventDefault = vi.fn(); - willNavigateHandler({ preventDefault }, url); + willNavigateHandler({ preventDefault }, url); - expect(preventDefault).not.toHaveBeenCalled(); - expect(mockOpenExternal).not.toHaveBeenCalled(); - }); + expect(preventDefault).not.toHaveBeenCalled(); + expect(mockOpenExternal).not.toHaveBeenCalled(); + }); + + it.each([ + "file:///etc/passwd", + "file:///Applications/PostHog.app/resources/renderer/other/index.html", + "file:///Applications/PostHog.app/resources/renderer/main_window/assets/app.js", + "file://attacker.example/Applications/PostHog.app/resources/renderer/main_window/index.html", + "file:///Applications/PostHog.app/resources/renderer/main_window/index.html%2F..%2Fpayload.html", + ])("blocks non-entry file %s without opening it externally", (url) => { + const { willNavigateHandler } = setup(PROD_HOME); + const preventDefault = vi.fn(); + + willNavigateHandler({ preventDefault }, url); - 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(); + 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 }, url); + willNavigateHandler({ preventDefault }, "https://posthog.com"); - expect(preventDefault).toHaveBeenCalledOnce(); - expect(mockOpenExternal).not.toHaveBeenCalled(); - expect(mockWarn).toHaveBeenCalledOnce(); + expect(preventDefault).toHaveBeenCalledOnce(); + expect(mockOpenExternal).toHaveBeenCalledExactlyOnceWith( + "https://posthog.com", + ); + }); }); - it("routes an external https link to the browser", () => { - const { willNavigateHandler } = setup(PROD_HOME); - const preventDefault = vi.fn(); + 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 }, "https://posthog.com"); + willNavigateHandler({ preventDefault }, url); - expect(preventDefault).toHaveBeenCalledOnce(); - expect(mockOpenExternal).toHaveBeenCalledExactlyOnceWith( - "https://posthog.com", + expect(preventDefault).not.toHaveBeenCalled(); + expect(mockOpenExternal).not.toHaveBeenCalled(); + }, ); - }); -}); -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) => { + // Origin lookalikes must be handled as external URLs: userinfo resolving to + // another host, a longer port, and a scheme change. + 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(); + }); + }); + + describe("will-frame-navigate (subframes)", () => { + it.each([ + "mcp-sandbox://proxy", + "about:blank", + "about:srcdoc", + "https://example.com/embed", + "http://localhost:3000/embed", + "blob:mcp-sandbox://proxy/1234", + "data:text/html,

embedded

", + ])("allows browser-contained navigation to %s", (url) => { + const { willFrameNavigateHandler } = setup(PROD_HOME); + const preventDefault = vi.fn(); + + willFrameNavigateHandler({ preventDefault, isMainFrame: false, url }); + expect(preventDefault).not.toHaveBeenCalled(); expect(mockOpenExternal).not.toHaveBeenCalled(); - }, - ); + }); + + it.each([ + "smb://attacker.example/share", + "file:///etc/passwd", + "mailto:attacker@example.com", + "custom-scheme://payload", + "javascript:alert(1)", + "not a url", + ])("blocks external application navigation to %s", (url) => { + const { willFrameNavigateHandler } = setup(PROD_HOME); + const preventDefault = vi.fn(); - // 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); - }); + willFrameNavigateHandler({ preventDefault, isMainFrame: false, url }); - it("blocks an unsafe scheme in dev too", () => { - const { willNavigateHandler } = setup(DEV_HOME); - const preventDefault = vi.fn(); + expect(preventDefault).toHaveBeenCalledOnce(); + expect(mockOpenExternal).not.toHaveBeenCalled(); + expect(mockWarn).toHaveBeenCalledOnce(); + }); - willNavigateHandler({ preventDefault }, "file:///etc/passwd"); + it("leaves main-frame navigation to the main-frame handler", () => { + const { willFrameNavigateHandler } = setup(PROD_HOME); + const preventDefault = vi.fn(); - expect(preventDefault).toHaveBeenCalledOnce(); - expect(mockOpenExternal).not.toHaveBeenCalled(); - expect(mockWarn).toHaveBeenCalledOnce(); + willFrameNavigateHandler({ + preventDefault, + isMainFrame: true, + url: "custom-scheme://payload", + }); + + expect(preventDefault).not.toHaveBeenCalled(); + expect(mockWarn).not.toHaveBeenCalled(); + }); + }); + + describe("session permission handlers", () => { + it.each([ + { permission: "openExternal", expected: false }, + { permission: "media", expected: true }, + ])( + "returns $expected for $permission permission checks and requests", + ({ permission, expected }) => { + const { permissionCheckHandler, permissionRequestHandler } = + setupPermissionHandlers(); + const callback = vi.fn(); + + expect( + permissionCheckHandler(null, permission, "mcp-sandbox://proxy", { + isMainFrame: false, + }), + ).toBe(expected); + permissionRequestHandler({}, permission, callback, {}); + + expect(callback).toHaveBeenCalledExactlyOnceWith(expected); + }, + ); }); }); diff --git a/apps/code/src/main/external-links.ts b/apps/code/src/main/external-links.ts index 4989c16fd4..cc0ea4b4b6 100644 --- a/apps/code/src/main/external-links.ts +++ b/apps/code/src/main/external-links.ts @@ -1,5 +1,7 @@ +import path from "node:path"; +import { fileURLToPath } from "node:url"; import { isSafeExternalUrl } from "@posthog/shared"; -import { type BrowserWindow, shell } from "electron"; +import { type BrowserWindow, type Session, shell } from "electron"; import { logger } from "./utils/logger"; const log = logger.scope("external-links"); @@ -31,11 +33,6 @@ function openExternalIfSafe(url: string): 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 { @@ -45,20 +42,53 @@ function isInAppNavigation(target: string, appHome: URL): boolean { } 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); + if (parsed.host !== appHome.host) return false; + + try { + const appEntryPath = fileURLToPath(appHome); + const targetPath = fileURLToPath(parsed); + return path.relative(appEntryPath, targetPath) === ""; + } catch { + return false; + } } // Dev server (http/https): pin scheme + host + port exactly. return parsed.origin === appHome.origin; } +function isAllowedSubframeNavigation(target: string): boolean { + let parsed: URL; + try { + parsed = new URL(target); + } catch { + return false; + } + + if (parsed.protocol === "mcp-sandbox:") { + return parsed.host === "proxy"; + } + + if (parsed.protocol === "about:") { + return parsed.pathname === "blank" || parsed.pathname === "srcdoc"; + } + + return ["http:", "https:", "blob:", "data:"].includes(parsed.protocol); +} + +export function setupExternalLinkPermissionHandlers(session: Session): void { + // Electron approves permission requests by default. Preserve that existing + // behavior except for renderer-initiated external application launches, + // which must go through the validated host launcher instead. + session.setPermissionCheckHandler( + (_webContents, permission) => permission !== "openExternal", + ); + session.setPermissionRequestHandler((_webContents, permission, callback) => { + callback(permission !== "openExternal"); + }); +} + export function setupExternalLinkHandlers( window: BrowserWindow, appHome: URL, @@ -73,4 +103,12 @@ export function setupExternalLinkHandlers( event.preventDefault(); openExternalIfSafe(url); }); + + window.webContents.on("will-frame-navigate", (event) => { + if (event.isMainFrame || isAllowedSubframeNavigation(event.url)) return; + event.preventDefault(); + log.warn("Blocked subframe navigation with unsupported scheme", { + scheme: urlScheme(event.url), + }); + }); } diff --git a/apps/code/src/main/index.ts b/apps/code/src/main/index.ts index 9fa4d958ca..6249e93ed2 100644 --- a/apps/code/src/main/index.ts +++ b/apps/code/src/main/index.ts @@ -72,6 +72,7 @@ import { WORKSPACE_SERVER_SERVICE, WORKSPACE_SERVICE, } from "./di/tokens"; +import { setupExternalLinkPermissionHandlers } from "./external-links"; import { posthogNodeAnalytics } from "./platform-adapters/posthog-analytics"; import { registerMcpSandboxProtocol } from "./protocols/mcp-sandbox"; import type { AppLifecycleService } from "./services/app-lifecycle/service"; @@ -360,6 +361,7 @@ app.whenReady().then(async () => { `Logs: main=${getLogFilePath()} chromium=${getChromiumLogFilePath() ?? "(disabled)"} network=${getNetworkLogFilePath()}`, ); ensureClaudeConfigDir(); + setupExternalLinkPermissionHandlers(session.fromPartition("persist:main")); registerMcpSandboxProtocol(); installRendererNetworkLogging( session.fromPartition("persist:main").webRequest, diff --git a/apps/code/tests/e2e/fixtures/electron.ts b/apps/code/tests/e2e/fixtures/electron.ts index e2e7d44e24..8eec6c8e19 100644 --- a/apps/code/tests/e2e/fixtures/electron.ts +++ b/apps/code/tests/e2e/fixtures/electron.ts @@ -1,4 +1,5 @@ -import { existsSync } from "node:fs"; +import { existsSync, mkdirSync, mkdtempSync, rmSync } from "node:fs"; +import os from "node:os"; import path from "node:path"; import { test as base, @@ -69,18 +70,33 @@ export const test = base.extend({ // biome-ignore lint/correctness/noEmptyPattern: Playwright fixture requires empty destructuring electronApp: async ({}, use) => { const appPath = getAppPath(); - - const electronApp = await electron.launch({ - executablePath: appPath, - args: [], - env: { - ...process.env, - ELECTRON_DISABLE_GPU: "1", - }, - }); - - await use(electronApp); - await electronApp.close(); + const e2eHome = mkdtempSync(path.join(os.tmpdir(), "posthog-code-e2e-")); + const e2eAppData = path.join(e2eHome, "app-data"); + const e2eUserData = path.join(e2eHome, "user-data"); + mkdirSync(e2eUserData, { recursive: true }); + let electronApp: ElectronApplication | undefined; + + try { + electronApp = await electron.launch({ + executablePath: appPath, + args: [], + env: { + ...process.env, + APPDATA: e2eAppData, + ELECTRON_DISABLE_GPU: "1", + HOME: e2eHome, + LOCALAPPDATA: e2eAppData, + POSTHOG_E2E_USER_DATA_DIR: e2eUserData, + USERPROFILE: e2eHome, + XDG_CONFIG_HOME: e2eAppData, + }, + }); + + await use(electronApp); + } finally { + await electronApp?.close(); + rmSync(e2eHome, { recursive: true, force: true }); + } }, window: async ({ electronApp }, use) => { diff --git a/apps/code/tests/e2e/tests/main-process.spec.ts b/apps/code/tests/e2e/tests/main-process.spec.ts index 9ce446ace6..09bbd0345a 100644 --- a/apps/code/tests/e2e/tests/main-process.spec.ts +++ b/apps/code/tests/e2e/tests/main-process.spec.ts @@ -38,4 +38,64 @@ test.describe("Main Process", () => { expect(userDataPath).toContain("posthog-code"); }); + + test("blocks external protocol navigation from a renderer subframe", async ({ + electronApp, + window, + }) => { + const targetUrl = "custom-scheme://sandbox-navigation-test/payload"; + const navigationResult = electronApp.evaluate( + async ({ BrowserWindow }, expectedUrl) => { + const mainWindow = BrowserWindow.getAllWindows()[0]; + if (!mainWindow) throw new Error("Main window not found"); + + return await new Promise<{ + defaultPrevented: boolean; + isMainFrame: boolean; + }>((resolve, reject) => { + const timeout = setTimeout(() => { + mainWindow.webContents.off("will-frame-navigate", listener); + reject(new Error("Timed out waiting for subframe navigation")); + }, 5000); + const listener = ( + event: Electron.Event, + ): void => { + if (event.url !== expectedUrl) return; + clearTimeout(timeout); + mainWindow.webContents.off("will-frame-navigate", listener); + resolve({ + defaultPrevented: event.defaultPrevented, + isMainFrame: event.isMainFrame, + }); + }; + mainWindow.webContents.on("will-frame-navigate", listener); + }); + }, + targetUrl, + ); + + const frameHandle = await window.evaluateHandle(() => { + const iframe = document.createElement("iframe"); + iframe.srcdoc = ''; + document.body.appendChild(iframe); + return iframe; + }); + const iframe = frameHandle.asElement(); + if (!iframe) throw new Error("Iframe was not created"); + const frame = await iframe.contentFrame(); + if (!frame) throw new Error("Iframe content frame was not created"); + + await frame.evaluate((url) => { + document.getElementById("navigate")?.addEventListener("click", () => { + window.location.href = url; + }); + }, targetUrl); + + await frame.getByText("Navigate externally").click(); + + await expect(navigationResult).resolves.toEqual({ + defaultPrevented: true, + isMainFrame: false, + }); + }); });