mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
fix(site): sever opener in openAppInNewWindow for slim-window apps (#23117)
Split out from #23000. ## Problem `openAppInNewWindow()` opens workspace apps in a slim popup without `noopener`, leaving `window.opener` intact. Since workspace apps can proxy arbitrary user-hosted content under the dashboard origin, this exposes the Coder dashboard to tabnabbing and same-origin DOM access. We cannot simply pass `"noopener"` to `window.open()` because the WHATWG spec mandates that `window.open()` returns `null` when `"noopener"` is present — indistinguishable from a blocked popup — which would break the popup-blocked error toast. ## Solution Use a two-step approach in `openAppInNewWindow()`: 1. Open `about:blank` first (without `noopener`) so we can detect popup blockers via the `null` return 2. If the popup succeeded, sever the opener reference with `popup.opener = null` 3. Navigate the popup to the target URL via `popup.location.href` This preserves popup-block detection while eliminating the security exposure. ## Tests A vitest case in `AppLink.test.tsx` asserts that `open_in="slim-window"` anchors do not carry `target` or `rel` attributes (since slim-window opening is handled programmatically via `onClick` / `window.open()`, not anchor attributes). --------- Co-authored-by: Kayla はな <kayla@tree.camp>
This commit is contained in:
co-authored by
Kayla はな
parent
839165818b
commit
8c494e2a77
@@ -3,7 +3,12 @@ import {
|
||||
MockWorkspaceAgent,
|
||||
MockWorkspaceApp,
|
||||
} from "#/testHelpers/entities";
|
||||
import { getAppHref, getVSCodeHref, SESSION_TOKEN_PLACEHOLDER } from "./apps";
|
||||
import {
|
||||
getAppHref,
|
||||
getVSCodeHref,
|
||||
openAppInNewWindow,
|
||||
SESSION_TOKEN_PLACEHOLDER,
|
||||
} from "./apps";
|
||||
|
||||
describe("getVSCodeHref", () => {
|
||||
it("includes the chat ID when provided", () => {
|
||||
@@ -176,3 +181,41 @@ describe("getAppHref", () => {
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe("openAppInNewWindow", () => {
|
||||
afterEach(() => {
|
||||
vi.restoreAllMocks();
|
||||
});
|
||||
|
||||
it("severs opener and navigates popup to href on success", () => {
|
||||
const popup = {
|
||||
opener: window,
|
||||
location: { href: "" },
|
||||
};
|
||||
vi.spyOn(window, "open").mockReturnValue(popup as unknown as Window);
|
||||
|
||||
openAppInNewWindow("https://app.example.com");
|
||||
|
||||
expect(popup.opener).toBeNull();
|
||||
expect(popup.location.href).toBe("https://app.example.com");
|
||||
});
|
||||
|
||||
it("still navigates when nulling opener throws", () => {
|
||||
const popup = {
|
||||
location: { href: "" },
|
||||
};
|
||||
Object.defineProperty(popup, "opener", {
|
||||
set() {
|
||||
throw new Error("Electron restriction");
|
||||
},
|
||||
get() {
|
||||
return window;
|
||||
},
|
||||
});
|
||||
vi.spyOn(window, "open").mockReturnValue(popup as unknown as Window);
|
||||
|
||||
openAppInNewWindow("https://app.example.com");
|
||||
|
||||
expect(popup.location.href).toBe("https://app.example.com");
|
||||
});
|
||||
});
|
||||
|
||||
@@ -83,13 +83,25 @@ export const getTerminalHref = ({
|
||||
}/terminal?${params}`;
|
||||
};
|
||||
|
||||
// Open `about:blank` first to detect a popup blocker. If it opens, we
|
||||
// null out `opener` (durable on the opened window); and navigate `popup`
|
||||
// to the target URL. The Coder UI keeps access to `popup`s handle
|
||||
export const openAppInNewWindow = (href: string) => {
|
||||
const popup = window.open(href, "_blank", "width=900,height=600");
|
||||
const popup = window.open("about:blank", "_blank", "width=900,height=600");
|
||||
if (!popup) {
|
||||
toast.error("Failed to open app in new window.", {
|
||||
description: "Popup blocked. Allow popups to open this app.",
|
||||
});
|
||||
return;
|
||||
}
|
||||
try {
|
||||
// Setting the opener to null persists in the `popup` window over refresh
|
||||
// and navigation. The opening window retains its connection to `popup`
|
||||
popup.opener = null;
|
||||
} catch {
|
||||
// Electron can throw
|
||||
}
|
||||
popup.location.href = href;
|
||||
};
|
||||
|
||||
type GetAppHrefParams = {
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
import type { Meta, StoryObj } from "@storybook/react-vite";
|
||||
import { getPreferredProxy } from "contexts/ProxyContext";
|
||||
import { expect, screen, spyOn, userEvent, within } from "storybook/test";
|
||||
import {
|
||||
MockPrimaryWorkspaceProxy,
|
||||
MockWorkspace,
|
||||
@@ -219,3 +220,26 @@ export const WithTooltip: Story = {
|
||||
agent: MockWorkspaceAgent,
|
||||
},
|
||||
};
|
||||
|
||||
export const SlimWindowPopupBlocked: Story = {
|
||||
decorators: [withToaster],
|
||||
args: {
|
||||
workspace: MockWorkspace,
|
||||
app: {
|
||||
...MockWorkspaceApp,
|
||||
open_in: "slim-window",
|
||||
},
|
||||
agent: MockWorkspaceAgent,
|
||||
},
|
||||
play: async ({ canvasElement }) => {
|
||||
spyOn(window, "open").mockReturnValue(null);
|
||||
const canvas = within(canvasElement);
|
||||
const link = await canvas.findByRole("link");
|
||||
const user = userEvent.setup();
|
||||
await user.click(link);
|
||||
const toastMessage = await screen.findByText(
|
||||
"Popup blocked. Allow popups to open this app.",
|
||||
);
|
||||
expect(toastMessage).toBeInTheDocument();
|
||||
},
|
||||
};
|
||||
|
||||
@@ -22,4 +22,14 @@ describe("AppLink", () => {
|
||||
expect(link).toHaveAttribute("target", "_blank");
|
||||
expect(link).toHaveAttribute("rel", "noreferrer");
|
||||
});
|
||||
|
||||
// slim-window apps are opened programmatically via onClick /
|
||||
// window.open(), so the anchor must not carry target or rel
|
||||
// attributes that would interfere with that flow.
|
||||
it("does not set target or rel for slim-window apps", async () => {
|
||||
renderAppLink({ ...MockWorkspaceApp, open_in: "slim-window" });
|
||||
const link = await screen.findByRole("link");
|
||||
expect(link).not.toHaveAttribute("target");
|
||||
expect(link).not.toHaveAttribute("rel");
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user