authorgravatar for 114993336+walid-baharwal@users.noreply.github.comMuhammad Waleed <114993336+walid-baharwal@users.noreply.github.com> 2026-08-29 02:41:01+05:00
committergravatar for noreply@github.comGitHub <noreply@github.com> 2026-08-28 14:41:01-07:00
log0e2905eb783fd2385f358a95f0b25bbf07ff7122
tree90663475610e49c13299f4b3994076e620ca6306
parentff1761012af46ceb5f1ecc9b2be00aae288a1691
signature Signed by PGP key B5690EEEBB952194

fix(desktop): oauth popups open from the browser preview (#8435)

Co-authored-by: Julius Marminge <julius0216@outlook.com>

3 files changed, 118 insertions(+), 3 deletions(-)

apps/desktop/src/preview/Manager.test.ts+44
...@@ -58,6 +58,50 @@ describe("isPreviewRefreshShortcut", () => {...@@ -58,6 +58,50 @@ describe("isPreviewRefreshShortcut", () => {
58 });58 });
59});59});
6060
61describe("previewWindowOpenAction", () => {
62 const details = (overrides: {
63 readonly url?: string;
64 readonly disposition?: Electron.HandlerDetails["disposition"];
65 }) => ({
66 url: "https://accounts.google.com/o/oauth2/auth",
67 disposition: "new-window" as Electron.HandlerDetails["disposition"],
68 ...overrides,
69 });
70
71 it("opens a real window for scripted popups so the opener survives", () => {
72 // OAuth SDKs read a null `window.open()` as a blocked popup, and they need
73 // the opener alive to receive the credential back.
74 expect(PreviewManager.previewWindowOpenAction(details({}))).toBe("popup");
75 expect(
76 PreviewManager.previewWindowOpenAction(details({ url: "http://localhost:5173/auth" })),
77 ).toBe("popup");
78 });
79
80 it("keeps target=_blank links in the preview tab", () => {
81 expect(PreviewManager.previewWindowOpenAction(details({ disposition: "foreground-tab" }))).toBe(
82 "navigate",
83 );
84 expect(PreviewManager.previewWindowOpenAction(details({ disposition: "background-tab" }))).toBe(
85 "navigate",
86 );
87 });
88
89 it("does not hand a window to schemes that cannot be hardened", () => {
90 // A popup skips the `will-attach-webview` hardening, so it only gets a window
91 // when its preferences can be overridden. Chromium copies the guest's
92 // preferences for `about:blank` and forbids overriding them.
93 for (const url of [
94 "about:blank",
95 "javascript:alert(1)",
96 "file:///etc/passwd",
97 "vscode://vscode-remote/ssh-remote+box/tmp",
98 "not a url",
99 ]) {
100 expect(PreviewManager.previewWindowOpenAction(details({ url }))).toBe("navigate");
101 }
102 });
103});
104
61const {105const {
62 browserWindowConstructor,106 browserWindowConstructor,
63 createFromPath,107 createFromPath,
apps/desktop/src/preview/Manager.ts+68-2
...@@ -434,6 +434,61 @@ const APP_FORWARDED_SHORTCUTS: ReadonlyArray<{...@@ -434,6 +434,61 @@ const APP_FORWARDED_SHORTCUTS: ReadonlyArray<{
434 { key: "w", meta: true, shift: false, control: false },434 { key: "w", meta: true, shift: false, control: false },
435]);435]);
436436
437/**
438 * Protocols a preview page may open in a real popup window.
439 *
440 * `about:blank` stays out: Chromium skips browser-side navigation for it, so the
441 * child copies the guest's `contextIsolation: false` preferences and Electron
442 * gives no way to override them. Those popups keep loading in the preview tab.
443 *
444 * Deliberately not `ElectronShell.parseSafeExternalUrl`: that also admits
445 * `vscode://vscode-remote/...` deep links, which belong in `shell.openExternal`
446 * and not in a window spawned by a third-party page in the preview.
447 */
448const POPUP_PROTOCOLS = new Set(["http:", "https:"]);
449
450const isPopupUrl = (rawUrl: string): boolean => {
451 try {
452 return POPUP_PROTOCOLS.has(new URL(rawUrl).protocol);
453 } catch {
454 return false;
455 }
456};
457
458/**
459 * Preferences for a popup a preview page opens.
460 *
461 * A popup is not a webview attach, so the `will-attach-webview` hardening in
462 * `DesktopWindow` never sees it, and an unoverridden child would inherit the
463 * guest's relaxed posture: the picker preload needs `contextIsolation: false`
464 * to share `globalThis` with the previewed page, and no OAuth provider should
465 * get that. The window keeps the opener and the guest session either way.
466 */
467const POPUP_WINDOW_OPTIONS = {
468 webPreferences: {
469 contextIsolation: true,
470 nodeIntegration: false,
471 sandbox: true,
472 },
473} satisfies Electron.BrowserWindowConstructorOptions;
474
475/**
476 * Decides what a preview page's `window.open` should do.
477 *
478 * `"popup"` opens a real window, which scripted popups need: denying them makes
479 * `window.open()` return `null` (OAuth SDKs report that as a blocked popup), and
480 * navigating the preview tab instead destroys the opener the popup has to
481 * `postMessage` its result back to.
482 *
483 * `target="_blank"` links arrive as a tab disposition and keep loading in the
484 * preview tab, which is what people expect from a link inside a preview.
485 */
486export const previewWindowOpenAction = (details: {
487 readonly url: string;
488 readonly disposition: Electron.HandlerDetails["disposition"];
489}): "popup" | "navigate" =>
490 details.disposition === "new-window" && isPopupUrl(details.url) ? "popup" : "navigate";
491
437export const isPreviewRefreshShortcut = (input: Electron.Input): boolean =>492export const isPreviewRefreshShortcut = (input: Electron.Input): boolean =>
438 input.type === "keyDown" &&493 input.type === "keyDown" &&
439 input.key.toLowerCase() === "r" &&494 input.key.toLowerCase() === "r" &&
...@@ -1661,6 +1716,12 @@ const makeNativeOperations = Effect.fn("PreviewManager.makeOperations")(function...@@ -1661,6 +1716,12 @@ const makeNativeOperations = Effect.fn("PreviewManager.makeOperations")(function
1661 ],1716 ],
1662 });1717 });
1663 });1718 });
1719 // A popup opens with Electron's default handler, so the page inside it could
1720 // otherwise spawn native windows without limit. Nothing in an OAuth flow
1721 // opens a second popup, so the chain stops at the first one.
1722 const windowCreated = (window: Electron.BrowserWindow): void => {
1723 window.webContents.setWindowOpenHandler(() => ({ action: "deny" }));
1724 };
1664 const beforeInput = (event: Electron.Event, input: Electron.Input): void => {1725 const beforeInput = (event: Electron.Event, input: Electron.Input): void => {
1665 if (isPreviewRefreshShortcut(input)) {1726 if (isPreviewRefreshShortcut(input)) {
1666 event.preventDefault();1727 event.preventDefault();
...@@ -1686,6 +1747,7 @@ const makeNativeOperations = Effect.fn("PreviewManager.makeOperations")(function...@@ -1686,6 +1747,7 @@ const makeNativeOperations = Effect.fn("PreviewManager.makeOperations")(function
1686 wc.off("did-stop-loading", sync);1747 wc.off("did-stop-loading", sync);
1687 wc.off("did-fail-load", failed as never);1748 wc.off("did-fail-load", failed as never);
1688 wc.off("audio-state-changed", audioStateChanged);1749 wc.off("audio-state-changed", audioStateChanged);
1750 wc.off("did-create-window", windowCreated);
1689 wc.off("before-input-event", beforeInput);1751 wc.off("before-input-event", beforeInput);
1690 wc.ipc.off(HUMAN_INPUT_CHANNEL, humanInput);1752 wc.ipc.off(HUMAN_INPUT_CHANNEL, humanInput);
1691 wc.ipc.off(MOUSE_NAVIGATE_CHANNEL, mouseNavigate);1753 wc.ipc.off(MOUSE_NAVIGATE_CHANNEL, mouseNavigate);
...@@ -1704,14 +1766,18 @@ const makeNativeOperations = Effect.fn("PreviewManager.makeOperations")(function...@@ -1704,14 +1766,18 @@ const makeNativeOperations = Effect.fn("PreviewManager.makeOperations")(function
1704 wc.on("audio-state-changed", audioStateChanged);1766 wc.on("audio-state-changed", audioStateChanged);
1705 wc.ipc.on(HUMAN_INPUT_CHANNEL, humanInput);1767 wc.ipc.on(HUMAN_INPUT_CHANNEL, humanInput);
1706 wc.ipc.on(MOUSE_NAVIGATE_CHANNEL, mouseNavigate);1768 wc.ipc.on(MOUSE_NAVIGATE_CHANNEL, mouseNavigate);
1707 wc.setWindowOpenHandler(({ url }) => {1769 wc.setWindowOpenHandler((details) => {
1770 if (previewWindowOpenAction(details) === "popup") {
1771 return { action: "allow", overrideBrowserWindowOptions: POPUP_WINDOW_OPTIONS };
1772 }
1708 runFork(1773 runFork(
1709 attemptPromise({ operation: "openPreviewWindow", tabId, webContentsId: wc.id }, () =>1774 attemptPromise({ operation: "openPreviewWindow", tabId, webContentsId: wc.id }, () =>
1710 wc.loadURL(url),1775 wc.loadURL(details.url),
1711 ).pipe(Effect.ignore),1776 ).pipe(Effect.ignore),
1712 );1777 );
1713 return { action: "deny" };1778 return { action: "deny" };
1714 });1779 });
1780 wc.on("did-create-window", windowCreated);
1715 wc.on("before-input-event", beforeInput);1781 wc.on("before-input-event", beforeInput);
1716 });1782 });
1717 yield* Ref.update(attachedRef, (attached) =>1783 yield* Ref.update(attachedRef, (attached) =>
apps/web/src/browser/HostedBrowserWebview.tsx+6-1
...@@ -99,7 +99,6 @@ export function HostedBrowserWebview(props: {...@@ -99,7 +99,6 @@ export function HostedBrowserWebview(props: {
9999
100 const setWebviewRef = useCallback((node: HTMLElement | null) => {100 const setWebviewRef = useCallback((node: HTMLElement | null) => {
101 webviewRef.current = node as ElectronWebview | null;101 webviewRef.current = node as ElectronWebview | null;
102 if (node && !node.hasAttribute("allowpopups")) node.setAttribute("allowpopups", "true");
103 }, []);102 }, []);
104103
105 useEffect(() => {104 useEffect(() => {
...@@ -269,6 +268,12 @@ export function HostedBrowserWebview(props: {...@@ -269,6 +268,12 @@ export function HostedBrowserWebview(props: {
269 <webview268 <webview
270 key={webviewGeneration}269 key={webviewGeneration}
271 ref={setWebviewRef}270 ref={setWebviewRef}
271 // Must be an attribute on the element itself: Electron reads it when the
272 // guest attaches, so setting it from the ref callback lands too late and
273 // the guest attaches with popups disabled. React types `allowpopups` as a
274 // boolean, but react-dom drops boolean values for unrecognized attributes,
275 // so the literal string has to be spread past the type.
276 {...({ allowpopups: "true" } as unknown as { readonly allowpopups?: boolean })}
272 src={webviewGeneration === 0 ? initialSrc : recoverySrc}277 src={webviewGeneration === 0 ? initialSrc : recoverySrc}
273 partition={config.partition}278 partition={config.partition}
274 webpreferences={config.webPreferences}279 webpreferences={config.webPreferences}