From 8fa05ae7292cd6db2fce73a2e143ce21d4a355ce Mon Sep 17 00:00:00 2001 From: Walid Baharwal Date: Thu, 27 Aug 2026 23:03:26 +0500 Subject: [PATCH 1/3] fix(desktop): oauth popups open from the browser preview Scripted `window.open` calls inside the integrated browser preview were denied and loaded in the preview tab instead. Firebase `signInWithPopup` got a null window handle back and reported `auth/popup-blocked`, and the in-tab load also dropped the opener the popup needs to post the credential back to. Popups with a `new-window` disposition and an http or https URL now get a real window, with context isolation and the sandbox turned back on: a popup is not a webview attach, so the `will-attach-webview` hardening never sees it and an unoverridden child would inherit the picker preload's relaxed posture. `about:blank` popups keep loading in the preview tab, since Chromium copies the guest preferences for them and forbids overriding. Links with `target="_blank"` are unchanged. Fixes #6561 --- apps/desktop/src/preview/Manager.test.ts | 44 +++++++++++++++++ apps/desktop/src/preview/Manager.ts | 63 +++++++++++++++++++++++- 2 files changed, 105 insertions(+), 2 deletions(-) diff --git a/apps/desktop/src/preview/Manager.test.ts b/apps/desktop/src/preview/Manager.test.ts index 3bf6d63051af..d0bf5b91f258 100644 --- a/apps/desktop/src/preview/Manager.test.ts +++ b/apps/desktop/src/preview/Manager.test.ts @@ -58,6 +58,50 @@ describe("isPreviewRefreshShortcut", () => { }); }); +describe("previewWindowOpenAction", () => { + const details = (overrides: { + readonly url?: string; + readonly disposition?: Electron.HandlerDetails["disposition"]; + }) => ({ + url: "https://accounts.google.com/o/oauth2/auth", + disposition: "new-window" as Electron.HandlerDetails["disposition"], + ...overrides, + }); + + it("opens a real window for scripted popups so the opener survives", () => { + // OAuth SDKs read a null `window.open()` as a blocked popup, and they need + // the opener alive to receive the credential back. + expect(PreviewManager.previewWindowOpenAction(details({}))).toBe("popup"); + expect( + PreviewManager.previewWindowOpenAction(details({ url: "http://localhost:5173/auth" })), + ).toBe("popup"); + }); + + it("keeps target=_blank links in the preview tab", () => { + expect( + PreviewManager.previewWindowOpenAction(details({ disposition: "foreground-tab" })), + ).toBe("navigate"); + expect( + PreviewManager.previewWindowOpenAction(details({ disposition: "background-tab" })), + ).toBe("navigate"); + }); + + it("does not hand a window to schemes that cannot be hardened", () => { + // A popup skips the `will-attach-webview` hardening, so it only gets a window + // when its preferences can be overridden. Chromium copies the guest's + // preferences for `about:blank` and forbids overriding them. + for (const url of [ + "about:blank", + "javascript:alert(1)", + "file:///etc/passwd", + "vscode://vscode-remote/ssh-remote+box/tmp", + "not a url", + ]) { + expect(PreviewManager.previewWindowOpenAction(details({ url }))).toBe("navigate"); + } + }); +}); + const { browserWindowConstructor, createFromPath, diff --git a/apps/desktop/src/preview/Manager.ts b/apps/desktop/src/preview/Manager.ts index 0d90e0175fe3..3b091befa9c6 100644 --- a/apps/desktop/src/preview/Manager.ts +++ b/apps/desktop/src/preview/Manager.ts @@ -434,6 +434,62 @@ const APP_FORWARDED_SHORTCUTS: ReadonlyArray<{ { key: "w", meta: true, shift: false, control: false }, ]); +/** + * Protocols a preview page may open in a real popup window. + * + * `about:blank` stays out: Chromium skips browser-side navigation for it, so the + * child copies the guest's `contextIsolation: false` preferences and Electron + * gives no way to override them. Those popups keep loading in the preview tab. + * + * Deliberately not `ElectronShell.parseSafeExternalUrl`: that also admits + * `vscode://vscode-remote/...` deep links, which belong in `shell.openExternal` + * and not in a window spawned by a third-party page in the preview. + */ +const POPUP_PROTOCOLS = new Set(["http:", "https:"]); + +const isPopupUrl = (rawUrl: string): boolean => { + try { + return POPUP_PROTOCOLS.has(new URL(rawUrl).protocol); + } catch { + return false; + } +}; + +/** + * Preferences for a popup a preview page opens. + * + * A popup is not a webview attach, so the `will-attach-webview` hardening in + * `DesktopWindow` never sees it, and an unoverridden child would inherit the + * guest's relaxed posture: the picker preload needs `contextIsolation: false` + * to share `globalThis` with the previewed page, and no OAuth provider should + * get that. The window keeps the opener and the guest session either way. + */ +const POPUP_WINDOW_OPTIONS: Electron.BrowserWindowConstructorOptions = { + webPreferences: { + contextIsolation: true, + sandbox: true, + nodeIntegration: false, + nodeIntegrationInSubFrames: false, + }, +}; + +/** + * Decides what a preview page's `window.open` should do. + * + * `"popup"` opens a real window, which scripted popups need: denying them makes + * `window.open()` return `null` (OAuth SDKs report that as a blocked popup), and + * navigating the preview tab instead destroys the opener the popup has to + * `postMessage` its result back to. + * + * `target="_blank"` links arrive as a tab disposition and keep loading in the + * preview tab, which is what people expect from a link inside a preview. + */ +export const previewWindowOpenAction = (details: { + readonly url: string; + readonly disposition: Electron.HandlerDetails["disposition"]; +}): "popup" | "navigate" => + details.disposition === "new-window" && isPopupUrl(details.url) ? "popup" : "navigate"; + export const isPreviewRefreshShortcut = (input: Electron.Input): boolean => input.type === "keyDown" && input.key.toLowerCase() === "r" && @@ -1704,10 +1760,13 @@ const makeNativeOperations = Effect.fn("PreviewManager.makeOperations")(function wc.on("audio-state-changed", audioStateChanged); wc.ipc.on(HUMAN_INPUT_CHANNEL, humanInput); wc.ipc.on(MOUSE_NAVIGATE_CHANNEL, mouseNavigate); - wc.setWindowOpenHandler(({ url }) => { + wc.setWindowOpenHandler((details) => { + if (previewWindowOpenAction(details) === "popup") { + return { action: "allow", overrideBrowserWindowOptions: POPUP_WINDOW_OPTIONS }; + } runFork( attemptPromise({ operation: "openPreviewWindow", tabId, webContentsId: wc.id }, () => - wc.loadURL(url), + wc.loadURL(details.url), ).pipe(Effect.ignore), ); return { action: "deny" }; From 7c5b69a7d761fb725ded84ba542151d5ebdacc9a Mon Sep 17 00:00:00 2001 From: Walid Baharwal Date: Thu, 27 Aug 2026 23:25:08 +0500 Subject: [PATCH 2/3] fix(desktop): deny nested popups from preview popup windows An allowed popup carried Electron's default window-open behavior, so a page inside it could spawn native windows without limit. The popup now denies its own window.open calls; no OAuth flow opens a second popup. The popup preferences also drop nodeIntegrationInSubFrames, matching the three keys every other hardened window in the app sets. --- apps/desktop/src/preview/Manager.ts | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/apps/desktop/src/preview/Manager.ts b/apps/desktop/src/preview/Manager.ts index 3b091befa9c6..c1adbe7df774 100644 --- a/apps/desktop/src/preview/Manager.ts +++ b/apps/desktop/src/preview/Manager.ts @@ -464,14 +464,13 @@ const isPopupUrl = (rawUrl: string): boolean => { * to share `globalThis` with the previewed page, and no OAuth provider should * get that. The window keeps the opener and the guest session either way. */ -const POPUP_WINDOW_OPTIONS: Electron.BrowserWindowConstructorOptions = { +const POPUP_WINDOW_OPTIONS = { webPreferences: { contextIsolation: true, - sandbox: true, nodeIntegration: false, - nodeIntegrationInSubFrames: false, + sandbox: true, }, -}; +} satisfies Electron.BrowserWindowConstructorOptions; /** * Decides what a preview page's `window.open` should do. @@ -1717,6 +1716,12 @@ const makeNativeOperations = Effect.fn("PreviewManager.makeOperations")(function ], }); }); + // A popup opens with Electron's default handler, so the page inside it could + // otherwise spawn native windows without limit. Nothing in an OAuth flow + // opens a second popup, so the chain stops at the first one. + const windowCreated = (window: Electron.BrowserWindow): void => { + window.webContents.setWindowOpenHandler(() => ({ action: "deny" })); + }; const beforeInput = (event: Electron.Event, input: Electron.Input): void => { if (isPreviewRefreshShortcut(input)) { event.preventDefault(); @@ -1742,6 +1747,7 @@ const makeNativeOperations = Effect.fn("PreviewManager.makeOperations")(function wc.off("did-stop-loading", sync); wc.off("did-fail-load", failed as never); wc.off("audio-state-changed", audioStateChanged); + wc.off("did-create-window", windowCreated); wc.off("before-input-event", beforeInput); wc.ipc.off(HUMAN_INPUT_CHANNEL, humanInput); wc.ipc.off(MOUSE_NAVIGATE_CHANNEL, mouseNavigate); @@ -1771,6 +1777,7 @@ const makeNativeOperations = Effect.fn("PreviewManager.makeOperations")(function ); return { action: "deny" }; }); + wc.on("did-create-window", windowCreated); wc.on("before-input-event", beforeInput); }); yield* Ref.update(attachedRef, (attached) => From 0604de52cad2103f6c3b0f1deb4dc21d7b71ad12 Mon Sep 17 00:00:00 2001 From: Walid Baharwal Date: Fri, 28 Aug 2026 03:15:01 +0500 Subject: [PATCH 3/3] fix(web): set allowpopups before the preview webview attaches Electron reads allowpopups when the guest attaches. The attribute was set from the ref callback, which runs after the element is in the DOM, so every preview guest attached with popups disabled and Electron blocked window.open before the window-open handler ran. Verified in a running desktop build: will-attach-webview reported allowpopups: false before this change and true after. --- apps/web/src/browser/HostedBrowserWebview.tsx | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/apps/web/src/browser/HostedBrowserWebview.tsx b/apps/web/src/browser/HostedBrowserWebview.tsx index ae0526abb15f..6b85ebc829ee 100644 --- a/apps/web/src/browser/HostedBrowserWebview.tsx +++ b/apps/web/src/browser/HostedBrowserWebview.tsx @@ -92,7 +92,6 @@ export function HostedBrowserWebview(props: { const setWebviewRef = useCallback((node: HTMLElement | null) => { webviewRef.current = node as ElectronWebview | null; - if (node && !node.hasAttribute("allowpopups")) node.setAttribute("allowpopups", "true"); }, []); useEffect(() => { @@ -259,6 +258,10 @@ export function HostedBrowserWebview(props: {