Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
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
21 changes: 18 additions & 3 deletions apps/desktop/src/preview/Manager.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -98,15 +98,30 @@ describe("previewWindowOpenAction", () => {

it("does not hand a window to schemes that cannot be hardened", () => {
for (const url of [
"javascript:alert(1)",
"file:///etc/passwd",
"vscode://vscode-remote/ssh-remote+box/tmp",
"not a url",
"blob:https://example.com/0b6c6a5e-55d4-4b4c-9d47-8f1b0c1d2e3f",
]) {
expect(PreviewManager.previewWindowOpenAction(details({ url }))).toBe("navigate");
}
});

it("denies local and inline schemes that no page could open in place itself", () => {
// `loadURL` is a browser-initiated load, so `data:` would get past the check that
// stops a page from opening it as a top-level document.
for (const url of [
"file:///etc/passwd",
"data:text/html,<h1>hi</h1>",
"javascript:alert(1)",
"view-source:https://example.com",
"chrome://gpu",
"not a url",
]) {
for (const disposition of ["new-window", "foreground-tab"] as const) {
expect(PreviewManager.previewWindowOpenAction(details({ url, disposition }))).toBe("deny");
}
}
});

it("denies a blank window instead of blanking the opener", () => {
for (const url of ["about:blank", ""]) {
for (const disposition of ["new-window", "foreground-tab"] as const) {
Expand Down
32 changes: 26 additions & 6 deletions apps/desktop/src/preview/Manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -423,11 +423,27 @@ type OpenLinkListener = (event: DesktopPreviewOpenLinkEvent) => Effect.Effect<vo
// about:blank inherits relaxed guest preferences; external editor schemes must reach the OS handler.
const POPUP_PROTOCOLS = new Set(["http:", "https:"]);

const isPopupUrl = (rawUrl: string): boolean => {
/**
* The in-place path runs `loadURL`, a browser-initiated load, so a `data:` popup skips the check
* that stops a page from navigating its own top frame to `data:`. The other local schemes are
* denied too, so this does not depend on how Chromium filters them before the handler runs.
*/
const IN_PLACE_DENIED_PROTOCOLS = new Set([
"about:",
"chrome:",
"data:",
"devtools:",
"file:",
"filesystem:",
"javascript:",
"view-source:",
]);

const urlProtocol = (rawUrl: string): string | undefined => {
try {
return POPUP_PROTOCOLS.has(new URL(rawUrl).protocol);
return new URL(rawUrl).protocol;
} catch {
return false;
return undefined;
}
};

Expand All @@ -440,15 +456,19 @@ const POPUP_WINDOW_OPTIONS = {
},
} satisfies Electron.BrowserWindowConstructorOptions;

/** OAuth needs a surviving popup opener; unhardenable schemes and form POSTs must stay in place. */
/**
* OAuth needs a surviving popup opener. `blob:`, app links (behind the session's `openExternal`
* prompt) and form POSTs stay in place.
*/
export const previewWindowOpenAction = (details: {
readonly url: string;
readonly disposition: Electron.HandlerDetails["disposition"];
readonly postBody?: Electron.PostBody;
}): "popup" | "new-tab" | "navigate" | "deny" => {
const protocol = urlProtocol(details.url);
// MSAL redirects on a denied blank popup; navigating the tab would destroy that fallback.
if (details.url === "" || details.url === "about:blank") return "deny";
if (!isPopupUrl(details.url)) return "navigate";
if (protocol === undefined || IN_PLACE_DENIED_PROTOCOLS.has(protocol)) return "deny";
if (!POPUP_PROTOCOLS.has(protocol)) return "navigate";
if (details.disposition === "new-window") return "popup";
return !details.postBody &&
(details.disposition === "foreground-tab" || details.disposition === "background-tab")
Expand Down
Loading