Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions apps/desktop/src/preview/Manager.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
63 changes: 61 additions & 2 deletions apps/desktop/src/preview/Manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Comment thread
macroscopeapp[bot] marked this conversation as resolved.

export const isPreviewRefreshShortcut = (input: Electron.Input): boolean =>
input.type === "keyDown" &&
input.key.toLowerCase() === "r" &&
Expand Down Expand Up @@ -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") {
Comment thread
macroscopeapp[bot] marked this conversation as resolved.
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" };
Expand Down
Loading