diff --git a/apps/desktop/src/preview/Manager.test.ts b/apps/desktop/src/preview/Manager.test.ts index 354bd597f..5e6b8d25b 100644 --- a/apps/desktop/src/preview/Manager.test.ts +++ b/apps/desktop/src/preview/Manager.test.ts @@ -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, diff --git a/apps/desktop/src/preview/Manager.ts b/apps/desktop/src/preview/Manager.ts index 87539d0b9..404a4845b 100644 --- a/apps/desktop/src/preview/Manager.ts +++ b/apps/desktop/src/preview/Manager.ts @@ -434,6 +434,66 @@ 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 = { + webPreferences: { + contextIsolation: true, + nodeIntegration: false, + sandbox: true, + // `preload` is a webPreference too, so an unset one is inherited from the + // guest. Preview guests load Pylon's pick/annotation preload, which imports + // `ipcRenderer` and was written for the trusted preview surface — it has no + // business running on a third-party sign-in page. + preload: "", + }, +} satisfies Electron.BrowserWindowConstructorOptions; + +/** + * 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"; + export const isPreviewRefreshShortcut = (input: Electron.Input): boolean => input.type === "keyDown" && input.key.toLowerCase() === "r" && @@ -1661,6 +1721,12 @@ const makeNativeOperations = Effect.fn("PreviewManager.makeOperations")(function ], }); }); + // A popup opens with Electron's default handler, so the page inside it could + // otherwise spawn native windows without limit. Nothing in an OAuth flow + // opens a second popup, so the chain stops at the first one. + const windowCreated = (window: Electron.BrowserWindow): void => { + window.webContents.setWindowOpenHandler(() => ({ action: "deny" })); + }; const beforeInput = (event: Electron.Event, input: Electron.Input): void => { if (isPreviewRefreshShortcut(input)) { event.preventDefault(); @@ -1686,6 +1752,7 @@ const makeNativeOperations = Effect.fn("PreviewManager.makeOperations")(function wc.off("did-stop-loading", sync); wc.off("did-fail-load", failed as never); wc.off("audio-state-changed", audioStateChanged); + wc.off("did-create-window", windowCreated); wc.off("before-input-event", beforeInput); wc.ipc.off(HUMAN_INPUT_CHANNEL, humanInput); wc.ipc.off(MOUSE_NAVIGATE_CHANNEL, mouseNavigate); @@ -1704,14 +1771,18 @@ 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") { + 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" }; }); + wc.on("did-create-window", windowCreated); wc.on("before-input-event", beforeInput); }); yield* Ref.update(attachedRef, (attached) => diff --git a/apps/web/src/browser/ElectronBrowserHost.tsx b/apps/web/src/browser/ElectronBrowserHost.tsx index fbf7c14b7..5425bca0b 100644 --- a/apps/web/src/browser/ElectronBrowserHost.tsx +++ b/apps/web/src/browser/ElectronBrowserHost.tsx @@ -29,6 +29,8 @@ export function ElectronBrowserHost() { previewState.serverEpoch, snapshot.tabId, ), + pictureInPicture: + previewState.desktopByTabId[snapshot.tabId]?.pictureInPicture ?? false, zoomFactor: previewState.desktopByTabId[snapshot.tabId]?.zoomFactor ?? 1, })) : []; @@ -80,7 +82,7 @@ export function ElectronBrowserHost() { if (!isElectron) return null; return (
- {sessions.map(({ threadRef, snapshot, runtimeTabId, zoomFactor }) => { + {sessions.map(({ threadRef, snapshot, runtimeTabId, pictureInPicture, zoomFactor }) => { const url = snapshot.navStatus._tag === "Idle" ? null : snapshot.navStatus.url; return ( ); diff --git a/apps/web/src/browser/HostedBrowserWebview.tsx b/apps/web/src/browser/HostedBrowserWebview.tsx index ae0526abb..74db76400 100644 --- a/apps/web/src/browser/HostedBrowserWebview.tsx +++ b/apps/web/src/browser/HostedBrowserWebview.tsx @@ -9,6 +9,7 @@ import { usePreviewBridge } from "~/components/preview/usePreviewBridge"; import { cn } from "~/lib/utils"; import { resolveBrowserSurfacePanelRect, useBrowserSurfaceStore } from "./browserSurfaceStore"; +import { useActiveBrowserRecordingTabIds } from "./browserRecording"; import { browserViewportSettingKey, resolveBrowserViewportLayout, @@ -47,9 +48,11 @@ export function HostedBrowserWebview(props: { readonly runtimeTabId: string; readonly initialUrl: string | null; readonly viewport: PreviewViewportSetting; + readonly pictureInPicture: boolean; readonly zoomFactor: number; }) { - const { threadRef, tabId, runtimeTabId, initialUrl, viewport, zoomFactor } = props; + const { threadRef, tabId, runtimeTabId, initialUrl, viewport, pictureInPicture, zoomFactor } = + props; const config = usePreviewWebviewConfig(threadRef.environmentId); const [initialSrc] = useState(() => initialUrl ?? "about:blank"); const tabLeaseRef = useRef(null); @@ -70,6 +73,10 @@ export function HostedBrowserWebview(props: { }; }), ); + const backgroundActivity = useBrowserSurfaceStore( + (state) => (state.activityByTabId[runtimeTabId] ?? 0) > 0, + ); + const recordingActive = useActiveBrowserRecordingTabIds().has(runtimeTabId); usePreviewBridge({ threadRef, tabId, runtimeTabId }); useEffect(() => { @@ -92,7 +99,6 @@ export function HostedBrowserWebview(props: { const setWebviewRef = useCallback((node: HTMLElement | null) => { webviewRef.current = node as ElectronWebview | null; - if (node && !node.hasAttribute("allowpopups")) node.setAttribute("allowpopups", "true"); }, []); useEffect(() => { @@ -231,8 +237,10 @@ export function HostedBrowserWebview(props: { if (!config) return null; + const renderingActive = active || backgroundActivity || pictureInPicture || recordingActive; const wrapperStyle = resolveHostedBrowserWebviewWrapperStyle({ active, + renderingActive, cornerRadius: presentation.cornerRadius, rect: lastRect, hiddenSize, @@ -244,6 +252,7 @@ export function HostedBrowserWebview(props: { className="fixed overflow-hidden bg-muted/35" style={{ ...wrapperStyle, overscrollBehavior: "contain" }} onScroll={syncContentPresentation} + data-preview-rendering={renderingActive ? "active" : "suspended"} data-preview-viewport={runtimeTabId} >
@@ -259,6 +268,12 @@ export function HostedBrowserWebview(props: { { it("starts recording for a visible tab", async () => { await startBrowserRecording("recording-tab"); - - expect(events).toEqual(["start-screencast", "publish:recording-tab"]); + const startupEvents = [...events]; await stopBrowserRecording("recording-tab"); + expect(startupEvents).toEqual(["publish:recording-tab", "start-screencast"]); }); it("records a hidden tab without requiring it to become visible", async () => { @@ -195,11 +195,11 @@ describe("browser recording", () => { }; await startBrowserRecording("recording-tab"); + const startupEvents = [...events]; expect(startScreencast).toHaveBeenCalledWith("recording-tab"); - expect(events).toEqual(["start-screencast", "publish:recording-tab"]); - await stopBrowserRecording("recording-tab"); + expect(startupEvents).toEqual(["publish:recording-tab", "start-screencast"]); }); it("fails startup instead of locking a fallback size when no frame arrives", async () => { diff --git a/apps/web/src/browser/browserRecording.ts b/apps/web/src/browser/browserRecording.ts index 69297cfdb..33d4610da 100644 --- a/apps/web/src/browser/browserRecording.ts +++ b/apps/web/src/browser/browserRecording.ts @@ -122,6 +122,12 @@ export function useActiveBrowserRecordingTabIds(): ReadonlySet { const activeRecordings = new Map(); let unsubscribeFrames: (() => void) | null = null; +const publishActiveRecordingTabIds = (): void => { + appAtomRegistry.set(activeBrowserRecordingTabIdsAtom, { + tabIds: new Set(activeRecordings.keys()), + }); +}; + export const BROWSER_RECORDING_STARTUP_SETTLE_TIMEOUT_MS = 5_000; export const BROWSER_RECORDING_FIRST_FRAME_SIZE_TIMEOUT_MS = 5_000; @@ -228,9 +234,7 @@ const clearActiveRecording = (recording: ActiveRecording): void => { unsubscribeFrames?.(); unsubscribeFrames = null; } - appAtomRegistry.set(activeBrowserRecordingTabIdsAtom, { - tabIds: new Set(activeRecordings.keys()), - }); + publishActiveRecordingTabIds(); }; const cleanupFailedRecordingStart = async ( @@ -377,6 +381,7 @@ export async function startBrowserRecording( lifecycle: { phase: "starting" }, }; activeRecordings.set(tabId, recording); + publishActiveRecordingTabIds(); try { try { unsubscribeFrames ??= bridge.recording.onFrame(drawFrame); @@ -487,9 +492,6 @@ export async function startBrowserRecording( if (recording.lifecycle.phase === "starting") { recording.lifecycle = { phase: "recording" }; } - appAtomRegistry.set(activeBrowserRecordingTabIdsAtom, { - tabIds: new Set(activeRecordings.keys()), - }); return startedAt; } finally { settleStartup?.(); diff --git a/apps/web/src/browser/browserSurfaceStore.test.ts b/apps/web/src/browser/browserSurfaceStore.test.ts index 12b34dd4b..249d3dcb2 100644 --- a/apps/web/src/browser/browserSurfaceStore.test.ts +++ b/apps/web/src/browser/browserSurfaceStore.test.ts @@ -2,13 +2,25 @@ import { beforeEach, describe, expect, it } from "vite-plus/test"; import { acquireBrowserSurface, + acquireBrowserSurfaceActivity, resolveBrowserSurfacePanelRect, useBrowserSurfaceStore, } from "./browserSurfaceStore"; describe("browserSurfaceStore", () => { beforeEach(() => { - useBrowserSurfaceStore.setState({ byTabId: {} }); + useBrowserSurfaceStore.setState({ activityByTabId: {}, byTabId: {} }); + }); + + it("keeps concurrent background work active until every lease is released", () => { + const first = acquireBrowserSurfaceActivity("background-browser"); + const second = acquireBrowserSurfaceActivity("background-browser"); + + first(); + expect(useBrowserSurfaceStore.getState().activityByTabId["background-browser"]).toBe(1); + + second(); + expect(useBrowserSurfaceStore.getState().activityByTabId["background-browser"]).toBeUndefined(); }); it("freezes the source content dimensions for a fitted presentation", () => { diff --git a/apps/web/src/browser/browserSurfaceStore.ts b/apps/web/src/browser/browserSurfaceStore.ts index 43ae0037c..fe85c9e38 100644 --- a/apps/web/src/browser/browserSurfaceStore.ts +++ b/apps/web/src/browser/browserSurfaceStore.ts @@ -29,7 +29,9 @@ export interface BrowserSurfaceContentPresentation { } interface BrowserSurfaceStoreState { + readonly activityByTabId: Record; readonly byTabId: Record; + readonly acquireActivity: (tabId: string) => () => void; readonly claim: (tabId: string, owner: symbol, fitSourceContent: boolean) => void; readonly present: ( tabId: string, @@ -63,7 +65,28 @@ const rectEquals = (left: BrowserSurfaceRect | null, right: BrowserSurfaceRect): left.height === right.height; export const useBrowserSurfaceStore = create()((set) => ({ + activityByTabId: {}, byTabId: {}, + acquireActivity: (tabId) => { + let released = false; + set((state) => ({ + activityByTabId: { + ...state.activityByTabId, + [tabId]: (state.activityByTabId[tabId] ?? 0) + 1, + }, + })); + return () => { + if (released) return; + released = true; + set((state) => { + const count = state.activityByTabId[tabId] ?? 0; + const activityByTabId = { ...state.activityByTabId }; + if (count <= 1) delete activityByTabId[tabId]; + else activityByTabId[tabId] = count - 1; + return { activityByTabId }; + }); + }; + }, claim: (tabId, owner, fitSourceContent) => set((state) => { const current = state.byTabId[tabId]; @@ -171,6 +194,9 @@ export const useBrowserSurfaceStore = create()((set) = }), })); +export const acquireBrowserSurfaceActivity = (tabId: string): (() => void) => + useBrowserSurfaceStore.getState().acquireActivity(tabId); + export function acquireBrowserSurface( tabId: string, fitSourceContent = false, diff --git a/apps/web/src/browser/hostedBrowserWebviewStyle.test.ts b/apps/web/src/browser/hostedBrowserWebviewStyle.test.ts index d0298dcde..c2c78f4ac 100644 --- a/apps/web/src/browser/hostedBrowserWebviewStyle.test.ts +++ b/apps/web/src/browser/hostedBrowserWebviewStyle.test.ts @@ -10,6 +10,7 @@ describe("resolveHostedBrowserWebviewWrapperStyle", () => { expect( resolveHostedBrowserWebviewWrapperStyle({ active: true, + renderingActive: true, rect: { x: 12, y: 34, width: 800, height: 600 }, hiddenSize: { width: 1280, height: 800 }, }), @@ -27,6 +28,7 @@ describe("resolveHostedBrowserWebviewWrapperStyle", () => { expect( resolveHostedBrowserWebviewWrapperStyle({ active: true, + renderingActive: true, cornerRadius: 12, rect: { x: 12, y: 34, width: 360, height: 203 }, hiddenSize: { width: 1280, height: 800 }, @@ -40,9 +42,10 @@ describe("resolveHostedBrowserWebviewWrapperStyle", () => { }); }); - it("keeps an inactive webview paintable while moving it offscreen", () => { + it("suspends painting for an inactive webview", () => { const style = resolveHostedBrowserWebviewWrapperStyle({ active: false, + renderingActive: false, rect: { x: 12, y: 34, width: 800, height: 600 }, hiddenSize: { width: 393, height: 852 }, }); @@ -54,6 +57,25 @@ describe("resolveHostedBrowserWebviewWrapperStyle", () => { height: 852, zIndex: -1, pointerEvents: "none", + visibility: "hidden", + }); + }); + + it("keeps an active background task paintable offscreen", () => { + const style = resolveHostedBrowserWebviewWrapperStyle({ + active: false, + renderingActive: true, + rect: null, + hiddenSize: { width: 1280, height: 800 }, + }); + + expect(style).toEqual({ + left: HIDDEN_BROWSER_WEBVIEW_OFFSET, + top: HIDDEN_BROWSER_WEBVIEW_OFFSET, + width: 1280, + height: 800, + zIndex: -1, + pointerEvents: "none", visibility: "visible", }); }); diff --git a/apps/web/src/browser/hostedBrowserWebviewStyle.ts b/apps/web/src/browser/hostedBrowserWebviewStyle.ts index f96f4af04..1e6ff0bea 100644 --- a/apps/web/src/browser/hostedBrowserWebviewStyle.ts +++ b/apps/web/src/browser/hostedBrowserWebviewStyle.ts @@ -13,18 +13,19 @@ export interface HostedBrowserWebviewWrapperStyle { readonly zIndex: number; readonly pointerEvents: "auto" | "none"; readonly borderRadius?: number; - readonly visibility?: "visible"; + readonly visibility?: "hidden" | "visible"; } export const HIDDEN_BROWSER_WEBVIEW_OFFSET = -100_000; export function resolveHostedBrowserWebviewWrapperStyle(input: { readonly active: boolean; + readonly renderingActive: boolean; readonly cornerRadius?: number; readonly rect: BrowserSurfaceRect | null; readonly hiddenSize: HostedBrowserWebviewSize; }): HostedBrowserWebviewWrapperStyle { - const { active, cornerRadius = 0, hiddenSize, rect } = input; + const { active, cornerRadius = 0, hiddenSize, rect, renderingActive } = input; if (active && rect) { return { left: rect.x, @@ -44,9 +45,6 @@ export function resolveHostedBrowserWebviewWrapperStyle(input: { height: hiddenSize.height, zIndex: -1, pointerEvents: "none", - // Keep the guest CSS-visible even while physically offscreen. Electron - // webviews can keep metadata/status alive under `visibility:hidden` while - // CDP Runtime/Input commands stall, which breaks offscreen automation. - visibility: "visible", + visibility: renderingActive ? "visible" : "hidden", }; } diff --git a/apps/web/src/components/preview/PreviewAutomationHosts.tsx b/apps/web/src/components/preview/PreviewAutomationHosts.tsx index acf7e52e3..1faf928b1 100644 --- a/apps/web/src/components/preview/PreviewAutomationHosts.tsx +++ b/apps/web/src/components/preview/PreviewAutomationHosts.tsx @@ -37,7 +37,10 @@ import { stopBrowserRecording, } from "~/browser/browserRecording"; import { resolveBrowserRecordingStopTarget } from "~/browser/browserRecordingScope"; -import { useBrowserSurfaceStore } from "~/browser/browserSurfaceStore"; +import { + acquireBrowserSurfaceActivity, + useBrowserSurfaceStore, +} from "~/browser/browserSurfaceStore"; import { browserDefaultOpenViewport, resolveBrowserDefaults } from "~/browser/browserDefaults"; import { runBrowserViewportMutation } from "~/browser/browserViewportActions"; import { previewRuntimeTabId } from "~/browser/previewRuntimeTabId"; @@ -98,7 +101,7 @@ const waitForDesktopOverlay = async ( operation, requestId, }); - if (state.desktopByTabId[tabId] && previewBridge) { + if (state.desktopByTabId[tabId] && previewBridge && isPreviewWebviewRendering(runtimeTabId)) { const status = await previewBridge.automation.status(runtimeTabId); if (status.available) return; } @@ -121,6 +124,11 @@ const findPreviewWebview = (tabId: string): ExecutablePreviewWebview | null => (candidate) => candidate.getAttribute("data-preview-tab") === tabId, ) ?? null; +const isPreviewWebviewRendering = (runtimeTabId: string): boolean => { + const wrapper = findPreviewWebview(runtimeTabId)?.closest("[data-preview-viewport]"); + return wrapper?.getAttribute("data-preview-rendering") === "active"; +}; + const readWebviewViewport = async ( webview: ExecutablePreviewWebview, ): Promise => { @@ -212,8 +220,12 @@ const currentStatus = async ( const visible = runtimeTabId ? (useBrowserSurfaceStore.getState().byTabId[runtimeTabId]?.visible ?? false) : false; + const renderingActive = runtimeTabId ? isPreviewWebviewRendering(runtimeTabId) : false; const viewportSetting = snapshot ? (snapshot.viewport ?? FILL_PREVIEW_VIEWPORT) : undefined; - const viewport = runtimeTabId ? await readRenderedViewport(runtimeTabId).catch(() => null) : null; + const viewport = + runtimeTabId && renderingActive + ? await readRenderedViewport(runtimeTabId).catch(() => null) + : null; const viewportStatus = { ...(viewportSetting === undefined ? {} : { viewportSetting }), ...(viewport === null ? {} : { viewport }), @@ -307,6 +319,7 @@ function PreviewAutomationHost(props: { readonly environmentId: EnvironmentId }) threadId: request.threadId, }; let tabId = request.tabId ?? null; + const browserActivity = { release: null as (() => void) | null }; try { let state = readThreadPreviewState(threadRef); const needsSessionSync = needsPreviewAutomationSessionSync(state, request.tabId); @@ -340,6 +353,7 @@ function PreviewAutomationHost(props: { readonly environmentId: EnvironmentId }) } const readyState = readThreadPreviewState(threadRef); const runtimeTabId = previewRuntimeTabId(threadRef, readyState.serverEpoch, readyTabId); + browserActivity.release ??= acquireBrowserSurfaceActivity(runtimeTabId); await waitForDesktopOverlay( threadRef, request.requestId, @@ -440,14 +454,7 @@ function PreviewAutomationHost(props: { readonly environmentId: EnvironmentId }) usePreviewMiniPlayerStore.getState().open(threadRef, activeTabId); } if (activeSnapshot && previewAutomationOpenNeedsOverlay(input, activeSnapshot)) { - await waitForDesktopOverlay( - threadRef, - request.requestId, - activeTabId, - activeRuntimeTabId, - request.operation, - request.timeoutMs, - ); + await requireReadyTab(); } if (shouldPresentPreview) { // React commits the thread-bound surface asynchronously. Settle @@ -678,6 +685,8 @@ function PreviewAutomationHost(props: { readonly environmentId: EnvironmentId }) tabId, cause, }); + } finally { + browserActivity.release?.(); } }, [environmentId, listPreviews, open, registry, resize],