From 8c494e2a776d6eb2a6265cfd3cf888aae8301bb1 Mon Sep 17 00:00:00 2001 From: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Date: Fri, 27 Mar 2026 15:35:10 -0400 Subject: [PATCH] fix(site): sever opener in openAppInNewWindow for slim-window apps (#23117) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 はな --- site/src/modules/apps/apps.test.ts | 45 ++++++++++++++++++- site/src/modules/apps/apps.ts | 14 +++++- .../resources/AppLink/AppLink.stories.tsx | 24 ++++++++++ .../resources/AppLink/AppLink.test.tsx | 10 +++++ 4 files changed, 91 insertions(+), 2 deletions(-) diff --git a/site/src/modules/apps/apps.test.ts b/site/src/modules/apps/apps.test.ts index e85b4421ed..28d52fb9ff 100644 --- a/site/src/modules/apps/apps.test.ts +++ b/site/src/modules/apps/apps.test.ts @@ -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"); + }); +}); diff --git a/site/src/modules/apps/apps.ts b/site/src/modules/apps/apps.ts index 0cdf3f091c..3bd96c227a 100644 --- a/site/src/modules/apps/apps.ts +++ b/site/src/modules/apps/apps.ts @@ -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 = { diff --git a/site/src/modules/resources/AppLink/AppLink.stories.tsx b/site/src/modules/resources/AppLink/AppLink.stories.tsx index c374e52e4b..d83522c533 100644 --- a/site/src/modules/resources/AppLink/AppLink.stories.tsx +++ b/site/src/modules/resources/AppLink/AppLink.stories.tsx @@ -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(); + }, +}; diff --git a/site/src/modules/resources/AppLink/AppLink.test.tsx b/site/src/modules/resources/AppLink/AppLink.test.tsx index 0efc8574aa..151ab7b86a 100644 --- a/site/src/modules/resources/AppLink/AppLink.test.tsx +++ b/site/src/modules/resources/AppLink/AppLink.test.tsx @@ -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"); + }); });