diff --git a/docs/tools/browser.md b/docs/tools/browser.md index 7373cdae3b05..193ba83f820a 100644 --- a/docs/tools/browser.md +++ b/docs/tools/browser.md @@ -18,6 +18,7 @@ OpenClaw can run a **dedicated Chrome/Brave/Edge/Chromium profile** that the age - A separate browser profile named **openclaw** (orange accent by default). - Deterministic tab control (list/open/focus/close). - Agent actions (click/type/drag/select), snapshots, screenshots, PDFs. +- Playwright-backed profiles save direct attachment navigations under the managed downloads directory and return `{ url, suggestedFilename, path }` metadata after final-URL policy validation. - A bundled `browser-automation` skill that teaches agents the snapshot, stable-tab, stale-ref, and manual-blocker recovery loop when the browser plugin is enabled. diff --git a/extensions/browser/src/browser/client-actions-types.ts b/extensions/browser/src/browser/client-actions-types.ts index 1953158334d9..3617c08309f8 100644 --- a/extensions/browser/src/browser/client-actions-types.ts +++ b/extensions/browser/src/browser/client-actions-types.ts @@ -1,6 +1,7 @@ /** * Shared result types for browser client action helpers. */ +import type { BrowserDownloadResult } from "./download-types.js"; import type { AnnotationItem } from "./screenshot-annotate.js"; /** Generic success result for action endpoints. */ @@ -11,6 +12,7 @@ export type BrowserActionTabResult = { ok: true; targetId: string; url?: string; + download?: BrowserDownloadResult; }; /** Success result carrying a filesystem output path. */ diff --git a/extensions/browser/src/browser/client.test.ts b/extensions/browser/src/browser/client.test.ts index 7e9b3dc846ad..31e44a30bf65 100644 --- a/extensions/browser/src/browser/client.test.ts +++ b/extensions/browser/src/browser/client.test.ts @@ -193,6 +193,11 @@ describe("browser client", () => { ok: true, targetId: "t1", url: "https://y", + download: { + url: "https://y/report.csv", + suggestedFilename: "report.csv", + path: "/tmp/openclaw/downloads/report.csv", + }, }), } as unknown as Response; } @@ -331,6 +336,11 @@ describe("browser client", () => { }); expect(navigation.ok).toBe(true); expect(navigation.targetId).toBe("t1"); + expect(navigation.download).toEqual({ + url: "https://y/report.csv", + suggestedFilename: "report.csv", + path: "/tmp/openclaw/downloads/report.csv", + }); const act = await browserAct("http://127.0.0.1:18791", { kind: "click", ref: "1" }); expect(act.ok).toBe(true); diff --git a/extensions/browser/src/browser/download-types.ts b/extensions/browser/src/browser/download-types.ts new file mode 100644 index 000000000000..1088f680e8e5 --- /dev/null +++ b/extensions/browser/src/browser/download-types.ts @@ -0,0 +1,9 @@ +/** Metadata for a browser download saved under the configured output root. */ +export type BrowserDownloadResult = { + url: string; + suggestedFilename: string; + path: string; +}; + +/** Download metadata available before any bytes are written. */ +export type BrowserDownloadCandidate = Omit; diff --git a/extensions/browser/src/browser/pw-download-capture.ts b/extensions/browser/src/browser/pw-download-capture.ts new file mode 100644 index 000000000000..909673c8a291 --- /dev/null +++ b/extensions/browser/src/browser/pw-download-capture.ts @@ -0,0 +1,139 @@ +/** Shared Playwright download capture and output handling. */ +import crypto from "node:crypto"; +import path from "node:path"; +import type { Page } from "playwright-core"; +import type { BrowserDownloadCandidate, BrowserDownloadResult } from "./download-types.js"; +import { writeExternalFileWithinOutputRoot } from "./output-files.js"; +import { DEFAULT_DOWNLOAD_DIR } from "./paths.js"; +import { sanitizeUntrustedFileName } from "./safe-filename.js"; + +export type BrowserDownloadCaptureState = { + downloadWaiterDepth: number; +}; + +export type BrowserDownloadCaptureOptions = { + beforeSave?: (download: BrowserDownloadCandidate) => Promise | void; + mode?: "passive" | "explicit"; + outputPath?: string; + outputRoot?: string; + timeoutMessage?: string; +}; + +export type PlaywrightDownload = { + url?: () => string; + suggestedFilename?: () => string; + saveAs?: (outPath: string) => Promise; +}; + +function buildManagedDownloadPath(rootDir: string, fileName: string): string { + const id = crypto.randomUUID(); + const safeName = sanitizeUntrustedFileName(fileName, "download.bin"); + return path.join(rootDir, `${id}-${safeName}`); +} + +/** Validate metadata and atomically save one Playwright download. */ +export async function saveBrowserDownload( + download: PlaywrightDownload, + opts: BrowserDownloadCaptureOptions = {}, +): Promise { + const suggestedFilename = download.suggestedFilename?.() || "download.bin"; + const candidate: BrowserDownloadCandidate = { + url: download.url?.() || "", + suggestedFilename, + }; + await opts.beforeSave?.(candidate); + const saveAs = download.saveAs?.bind(download); + if (!saveAs) { + throw new Error("Download cannot be saved"); + } + const requestedPath = opts.outputPath?.trim(); + const implicitRoot = opts.outputRoot ?? DEFAULT_DOWNLOAD_DIR; + const managedPath = requestedPath || buildManagedDownloadPath(implicitRoot, suggestedFilename); + const savedPath = await writeExternalFileWithinOutputRoot({ + rootDir: requestedPath ? opts.outputRoot : implicitRoot, + path: managedPath, + write: async (tempPath) => { + await saveAs(tempPath); + }, + }); + return { ...candidate, path: savedPath }; +} + +/** Arm one page download while maintaining explicit/passive ownership depth. */ +export function createDownloadCaptureForPage( + page: Page, + state: BrowserDownloadCaptureState, + timeoutMs: number, + opts: BrowserDownloadCaptureOptions = {}, +): { + armed: boolean; + promise: Promise; + cancel: () => void; +} { + // Passive action capture yields to an explicit wait/download owner. Explicit + // waiters may overlap; their arm id decides which one is allowed to save. + if (opts.mode !== "explicit" && state.downloadWaiterDepth > 0) { + return { + armed: false, + promise: new Promise(() => {}), + cancel: () => {}, + }; + } + + state.downloadWaiterDepth += 1; + let done = false; + let depthReleased = false; + let timer: NodeJS.Timeout | undefined; + let handler: ((download: unknown) => void) | undefined; + + const cleanup = () => { + if (!depthReleased) { + depthReleased = true; + state.downloadWaiterDepth = Math.max(0, state.downloadWaiterDepth - 1); + } + if (timer) { + clearTimeout(timer); + timer = undefined; + } + if (handler) { + page.off("download", handler as never); + handler = undefined; + } + }; + + const promise = new Promise((resolve, reject) => { + handler = (download: unknown) => { + if (done) { + return; + } + done = true; + cleanup(); + void saveBrowserDownload(download as PlaywrightDownload, opts).then(resolve, reject); + }; + page.on("download", handler as never); + timer = setTimeout( + () => { + if (done) { + return; + } + done = true; + cleanup(); + reject(new Error(opts.timeoutMessage ?? "Timeout waiting for download")); + }, + Math.max(1, timeoutMs), + ); + timer.unref?.(); + }); + + return { + armed: true, + promise, + cancel: () => { + if (done) { + return; + } + done = true; + cleanup(); + }, + }; +} diff --git a/extensions/browser/src/browser/pw-session.test.ts b/extensions/browser/src/browser/pw-session.test.ts index 75f0207ac6d7..28c86af48f64 100644 --- a/extensions/browser/src/browser/pw-session.test.ts +++ b/extensions/browser/src/browser/pw-session.test.ts @@ -4,8 +4,10 @@ import path from "node:path"; import type { Page } from "playwright-core"; import { afterEach, describe, expect, it, vi } from "vitest"; import { DEFAULT_DOWNLOAD_DIR } from "./paths.js"; +import { createDownloadCaptureForPage } from "./pw-download-capture.js"; import { ensurePageState, + isDownloadStartingNavigationError, refLocator, rememberRoleRefsForTarget, restoreRoleRefsForTarget, @@ -39,6 +41,14 @@ function fakePage(): { handlers.set(event, list); return undefined as unknown; }); + const off = vi.fn((event: string, cb: (...args: unknown[]) => void) => { + const list = handlers.get(event) ?? []; + handlers.set( + event, + list.filter((handler) => handler !== cb), + ); + return undefined as unknown; + }); const getByRole = vi.fn(() => ({ nth: vi.fn(() => ({ ok: true })) })); const frameLocator = vi.fn(() => ({ getByRole: vi.fn(() => ({ nth: vi.fn(() => ({ ok: true })) })), @@ -48,6 +58,7 @@ function fakePage(): { const page = { on, + off, getByRole, frameLocator, locator, @@ -239,6 +250,96 @@ describe("pw-session ensurePageState", () => { expect(download.saveAs).not.toHaveBeenCalled(); }); + it("captures navigation downloads under managed paths", async () => { + const { page, handlers } = fakePage(); + const state = ensurePageState(page); + const capture = createDownloadCaptureForPage(page, state, 1_000); + const saveAs = vi.fn(async (outPath: string) => { + await fs.writeFile(outPath, "attachment", "utf8"); + }); + const download = { + url: () => "https://example.com/export.csv", + suggestedFilename: () => "export.csv", + saveAs, + }; + + for (const handler of handlers.get("download") ?? []) { + handler(download); + } + + const result = await capture.promise; + expect(result.url).toBe("https://example.com/export.csv"); + expect(result.suggestedFilename).toBe("export.csv"); + expect(path.dirname(result.path)).toBe(DEFAULT_DOWNLOAD_DIR); + expect(path.basename(result.path)).toMatch(/-export\.csv$/); + expect(firstSavePath(saveAs)).not.toBe(result.path); + await expect(fs.readFile(result.path, "utf8")).resolves.toBe("attachment"); + }); + + it("validates captured navigation downloads before saving managed bytes", async () => { + const { page, handlers } = fakePage(); + const state = ensurePageState(page); + const blocked = new Error("blocked download"); + const beforeSave = vi.fn(async () => { + throw blocked; + }); + const capture = createDownloadCaptureForPage(page, state, 1_000, { beforeSave }); + const saveAs = vi.fn(async (outPath: string) => { + await fs.writeFile(outPath, "blocked", "utf8"); + }); + const download = { + url: () => "http://127.0.0.1:18080/export.csv", + suggestedFilename: () => "export.csv", + saveAs, + }; + + for (const handler of handlers.get("download") ?? []) { + handler(download); + } + + await expect(capture.promise).rejects.toBe(blocked); + expect(beforeSave).toHaveBeenCalledWith({ + url: "http://127.0.0.1:18080/export.csv", + suggestedFilename: "export.csv", + }); + expect(saveAs).not.toHaveBeenCalled(); + }); + + it("lets explicit download owners arm while passive capture yields", () => { + const { page } = fakePage(); + const state = ensurePageState(page); + state.downloadWaiterDepth = 1; + + const passive = createDownloadCaptureForPage(page, state, 1_000); + const explicit = createDownloadCaptureForPage(page, state, 1_000, { mode: "explicit" }); + + expect(passive.armed).toBe(false); + expect(explicit.armed).toBe(true); + expect(state.downloadWaiterDepth).toBe(2); + explicit.cancel(); + expect(state.downloadWaiterDepth).toBe(1); + }); + + it("recognizes Playwright download-starting navigation aborts", () => { + expect(isDownloadStartingNavigationError(new Error("page.goto: Download is starting"))).toBe( + true, + ); + expect(isDownloadStartingNavigationError(new Error("page.goto: net::ERR_ABORTED"))).toBe(false); + expect( + isDownloadStartingNavigationError( + new Error("page.goto: net::ERR_ABORTED at http://127.0.0.1:3333/download"), + "http://127.0.0.1:3333/download", + ), + ).toBe(true); + expect( + isDownloadStartingNavigationError( + new Error("page.goto: net::ERR_ABORTED at http://127.0.0.1:3333/other"), + "http://127.0.0.1:3333/download", + ), + ).toBe(false); + expect(isDownloadStartingNavigationError(new Error("Navigation failed"))).toBe(false); + }); + it("tracks page errors and network requests (best-effort)", () => { const { page, handlers } = fakePage(); const state = ensurePageState(page); diff --git a/extensions/browser/src/browser/pw-session.ts b/extensions/browser/src/browser/pw-session.ts index 19f7292e6817..1ce378d23606 100644 --- a/extensions/browser/src/browser/pw-session.ts +++ b/extensions/browser/src/browser/pw-session.ts @@ -4,8 +4,6 @@ * Manages CDP-backed Playwright connections, page lookup, observed dialogs, * console/network/page state, role refs, and safe navigation handling. */ -import crypto from "node:crypto"; -import path from "node:path"; import { isFutureDateTimestampMs, parseFiniteNumber, @@ -46,11 +44,9 @@ import { InvalidBrowserNavigationUrlError, withBrowserNavigationPolicy, } from "./navigation-guard.js"; -import { writeViaSiblingTempPath } from "./output-atomic.js"; -import { DEFAULT_DOWNLOAD_DIR } from "./paths.js"; import { playwrightCore } from "./playwright-core.runtime.js"; +import { saveBrowserDownload, type PlaywrightDownload } from "./pw-download-capture.js"; import { BROWSER_REF_MARKER_ATTRIBUTE, withPageScopedCdpClient } from "./pw-session.page-cdp.js"; -import { sanitizeUntrustedFileName } from "./safe-filename.js"; const { chromium } = playwrightCore; @@ -145,6 +141,10 @@ type ConnectedBrowser = { onDisconnected?: () => void; }; +type DownloadPayload = PlaywrightDownload & { + path?: () => Promise; +}; + type PageState = { console: BrowserConsoleMessage[]; errors: BrowserPageError[]; @@ -225,10 +225,15 @@ function resolveCdpConnectRetryDelayMs(attempt: number): number { return cdpConnectRetryDelayMsForTests ?? 250 + attempt * 250; } -function buildManagedDownloadPath(fileName: string): string { - const id = crypto.randomUUID(); - const safeName = sanitizeUntrustedFileName(fileName, "download.bin"); - return path.join(DEFAULT_DOWNLOAD_DIR, `${id}-${safeName}`); +export function isDownloadStartingNavigationError(err: unknown, expectedUrl?: string): boolean { + const message = formatErrorMessage(err).toLowerCase(); + if (message.includes("download is starting")) { + return true; + } + const normalizedUrl = normalizeOptionalString(expectedUrl)?.toLowerCase(); + return Boolean( + normalizedUrl && message.includes("net::err_aborted") && message.includes(normalizedUrl), + ); } function hasCachedPlaywrightBrowserConnection(cdpUrl: string): boolean { @@ -693,35 +698,14 @@ export function ensurePageState(page: Page): PageState { page.on("dialog", (dialog: Dialog) => { observeDialog(state, dialog); }); - page.on( - "download", - (download: { - suggestedFilename?: () => string; - saveAs?: (outPath: string) => Promise; - path?: () => Promise; - }) => { - if (state.downloadWaiterDepth > 0) { - return; - } - const suggested = sanitizeUntrustedFileName( - download.suggestedFilename?.() || "download.bin", - "download.bin", - ); - const managedPath = buildManagedDownloadPath(suggested); - const managedSave = (async () => { - await writeViaSiblingTempPath({ - rootDir: DEFAULT_DOWNLOAD_DIR, - targetPath: managedPath, - writeTemp: async (tempPath) => { - await download.saveAs?.(tempPath); - }, - }); - return managedPath; - })(); - managedSave.catch(() => {}); - download.path = async () => await managedSave; - }, - ); + page.on("download", (download: DownloadPayload) => { + if (state.downloadWaiterDepth > 0) { + return; + } + const managedSave = saveBrowserDownload(download); + managedSave.catch(() => {}); + download.path = async () => (await managedSave).path; + }); page.on("close", () => { clearArmedDialogResponse(state); for (const controller of state.dialogAbortControllers) { diff --git a/extensions/browser/src/browser/pw-tools-core.downloads.ts b/extensions/browser/src/browser/pw-tools-core.downloads.ts index 671c03aef9d1..807e1f8cc3b2 100644 --- a/extensions/browser/src/browser/pw-tools-core.downloads.ts +++ b/extensions/browser/src/browser/pw-tools-core.downloads.ts @@ -2,12 +2,12 @@ * File chooser, dialog, and download helpers for Playwright-backed browser * tools. */ -import crypto from "node:crypto"; import path from "node:path"; import type { Page } from "playwright-core"; import { resolvePreferredOpenClawTmpDir } from "../infra/tmp-openclaw-dir.js"; -import { writeExternalFileWithinOutputRoot } from "./output-files.js"; +import type { BrowserDownloadResult } from "./download-types.js"; import { resolveStrictExistingUploadPaths } from "./paths.js"; +import { createDownloadCaptureForPage } from "./pw-download-capture.js"; import { armObservedDialogResponseOnPage, ensurePageState, @@ -23,114 +23,30 @@ import { requireRef, toAIFriendlyError, } from "./pw-tools-core.shared.js"; -import { sanitizeUntrustedFileName } from "./safe-filename.js"; -function buildTempDownloadPath(fileName: string): string { - const id = crypto.randomUUID(); - const safeName = sanitizeUntrustedFileName(fileName, "download.bin"); - return path.join(resolvePreferredOpenClawTmpDir(), "downloads", `${id}-${safeName}`); -} - -function createPageDownloadWaiter(page: Page, timeoutMs: number) { - const state = ensurePageState(page); - // Depth tracks active download waiters so page teardown can distinguish an - // expected download transition from an unobserved lost event. - state.downloadWaiterDepth += 1; - let done = false; - let timer: NodeJS.Timeout | undefined; - let handler: ((download: unknown) => void) | undefined; - let depthReleased = false; - - const cleanup = () => { - if (!depthReleased) { - depthReleased = true; - state.downloadWaiterDepth = Math.max(0, state.downloadWaiterDepth - 1); - } - if (timer) { - clearTimeout(timer); - } - timer = undefined; - if (handler) { - page.off("download", handler as never); - handler = undefined; - } - }; - - const promise = new Promise((resolve, reject) => { - handler = (download: unknown) => { - if (done) { - return; - } - done = true; - cleanup(); - resolve(download); - }; - - page.on("download", handler as never); - timer = setTimeout(() => { - if (done) { - return; - } - done = true; - cleanup(); - reject(new Error("Timeout waiting for download")); - }, timeoutMs); - }); - - return { - promise, - cancel: () => { - if (done) { - return; - } - done = true; - cleanup(); - }, - }; -} - -type DownloadPayload = { - url?: () => string; - suggestedFilename?: () => string; - saveAs?: (outPath: string) => Promise; -}; - -async function saveDownloadPayload(download: DownloadPayload, outPath: string, rootDir?: string) { - const suggested = download.suggestedFilename?.() || "download.bin"; - const requestedPath = outPath?.trim(); - const resolvedOutPath = path.resolve(requestedPath || buildTempDownloadPath(suggested)); - const finalPath = await writeExternalFileWithinOutputRoot({ - rootDir, - path: resolvedOutPath, - write: async (tempPath) => { - await download.saveAs?.(tempPath); - }, - }); - - return { - url: download.url?.() || "", - suggestedFilename: suggested, - path: finalPath, - }; -} - -async function awaitDownloadPayload(params: { - waiter: ReturnType; +function createExplicitDownloadCapture(params: { + page: Page; state: ReturnType; - armId: number; + timeoutMs: number; outPath?: string; rootDir?: string; }) { - try { - const download = (await params.waiter.promise) as DownloadPayload; - if (params.state.armIdDownload !== params.armId) { - throw new Error("Download was superseded by another waiter"); - } - return await saveDownloadPayload(download, params.outPath ?? "", params.rootDir); - } catch (err) { - params.waiter.cancel(); - throw err; - } + params.state.armIdDownload = bumpDownloadArmId(); + const armId = params.state.armIdDownload; + return createDownloadCaptureForPage(params.page, params.state, params.timeoutMs, { + mode: "explicit", + outputPath: params.outPath, + outputRoot: params.rootDir, + beforeSave: () => { + if (params.state.armIdDownload !== armId) { + throw new Error("Download was superseded by another waiter"); + } + }, + }); +} + +function resolveImplicitDownloadRoot(): string { + return path.join(resolvePreferredOpenClawTmpDir(), "downloads"); } /** Arms the next page file chooser and fills it with strict existing paths. */ @@ -237,26 +153,24 @@ export async function waitForDownloadViaPlaywright(opts: { path?: string; rootDir?: string; timeoutMs?: number; -}): Promise<{ - url: string; - suggestedFilename: string; - path: string; -}> { +}): Promise { const page = await getPageForTargetId(opts); const state = ensurePageState(page); const timeout = normalizeTimeoutMs(opts.timeoutMs, 120_000); - state.armIdDownload = bumpDownloadArmId(); - const armId = state.armIdDownload; - - const waiter = createPageDownloadWaiter(page, timeout); - return await awaitDownloadPayload({ - waiter, + const capture = createExplicitDownloadCapture({ + page, state, - armId, + timeoutMs: timeout, outPath: opts.path, - rootDir: opts.rootDir, + rootDir: opts.path?.trim() ? opts.rootDir : (opts.rootDir ?? resolveImplicitDownloadRoot()), }); + try { + return await capture.promise; + } catch (err) { + capture.cancel(); + throw err; + } } /** Clicks an element ref and saves the download triggered by that click. */ @@ -267,11 +181,7 @@ export async function downloadViaPlaywright(opts: { path: string; rootDir?: string; timeoutMs?: number; -}): Promise<{ - url: string; - suggestedFilename: string; - path: string; -}> { +}): Promise { const page = await getPageForTargetId(opts); const state = ensurePageState(page); restoreRoleRefsForTarget({ cdpUrl: opts.cdpUrl, targetId: opts.targetId, page }); @@ -283,10 +193,13 @@ export async function downloadViaPlaywright(opts: { throw new Error("path is required"); } - state.armIdDownload = bumpDownloadArmId(); - const armId = state.armIdDownload; - - const waiter = createPageDownloadWaiter(page, timeout); + const capture = createExplicitDownloadCapture({ + page, + state, + timeoutMs: timeout, + outPath, + rootDir: opts.rootDir, + }); try { const locator = refLocator(page, ref); try { @@ -294,15 +207,9 @@ export async function downloadViaPlaywright(opts: { } catch (err) { throw toAIFriendlyError(err, ref); } - return await awaitDownloadPayload({ - waiter, - state, - armId, - outPath, - rootDir: opts.rootDir, - }); + return await capture.promise; } catch (err) { - waiter.cancel(); + capture.cancel(); throw err; } } diff --git a/extensions/browser/src/browser/pw-tools-core.snapshot.navigate-guard.test.ts b/extensions/browser/src/browser/pw-tools-core.snapshot.navigate-guard.test.ts index a81ab58700ce..c11cf0479f97 100644 --- a/extensions/browser/src/browser/pw-tools-core.snapshot.navigate-guard.test.ts +++ b/extensions/browser/src/browser/pw-tools-core.snapshot.navigate-guard.test.ts @@ -4,9 +4,11 @@ import { SsrFBlockedError } from "../infra/net/ssrf.js"; import "../test-support/browser-security.mock.js"; import { InvalidBrowserNavigationUrlError } from "./navigation-guard.js"; import { + getPwToolsCoreNavigationGuardMocks, getPwToolsCoreSessionMocks, installPwToolsCoreTestHooks, setPwToolsCoreCurrentPage, + setPwToolsCoreDownloadCapture, } from "./pw-tools-core.test-harness.js"; installPwToolsCoreTestHooks(); @@ -84,6 +86,189 @@ describe("pw-tools-core.snapshot navigate guard", () => { expect(result.url).toBe("https://example.com"); }); + it("returns managed download metadata when navigation starts an attachment download", async () => { + const download = { + url: "https://example.com/export.csv", + suggestedFilename: "export.csv", + path: "/tmp/openclaw/downloads/export.csv", + }; + const downloadCapture = { + armed: true, + promise: Promise.resolve(download), + cancel: vi.fn(), + }; + setPwToolsCoreDownloadCapture(downloadCapture); + const page = { + goto: vi.fn(async () => { + throw new Error("page.goto: Download is starting"); + }), + url: vi.fn(() => "https://example.com/start"), + }; + setPwToolsCoreCurrentPage(page); + + const result = await mod.navigateViaPlaywright({ + cdpUrl: "http://127.0.0.1:18792", + targetId: "tab-1", + url: "https://example.com/export.csv", + ssrfPolicy: { allowPrivateNetwork: true }, + }); + + expect(result).toEqual({ url: download.url, download }); + expect(downloadCapture.cancel).not.toHaveBeenCalled(); + expect(getPwToolsCoreSessionMocks().assertPageNavigationCompletedSafely).not.toHaveBeenCalled(); + expect( + getPwToolsCoreNavigationGuardMocks().assertBrowserNavigationResultAllowed, + ).toHaveBeenCalledWith({ + url: download.url, + ssrfPolicy: { allowPrivateNetwork: true }, + }); + }); + + it("returns managed download metadata for matching ERR_ABORTED attachment navigations", async () => { + const download = { + url: "http://127.0.0.1:3333/download", + suggestedFilename: "proof.txt", + path: "/tmp/openclaw/downloads/proof.txt", + }; + const downloadCapture = { + armed: true, + promise: Promise.resolve(download), + cancel: vi.fn(), + }; + setPwToolsCoreDownloadCapture(downloadCapture); + setPwToolsCoreCurrentPage({ + goto: vi.fn(async () => { + throw new Error("page.goto: net::ERR_ABORTED at http://127.0.0.1:3333/download"); + }), + url: vi.fn(() => "about:blank"), + }); + + const result = await mod.navigateViaPlaywright({ + cdpUrl: "http://127.0.0.1:18792", + url: "http://127.0.0.1:3333/download", + ssrfPolicy: { allowPrivateNetwork: true }, + }); + + expect(result).toEqual({ url: download.url, download }); + }); + + it("handles capture timeouts that win before ordinary navigation settles", async () => { + let rejectCapture!: (err: Error) => void; + const downloadCapture = { + armed: true, + promise: new Promise((_, reject) => { + rejectCapture = reject; + }), + cancel: vi.fn(), + }; + setPwToolsCoreDownloadCapture(downloadCapture); + setPwToolsCoreCurrentPage({ + goto: vi.fn(async () => { + rejectCapture(new Error("Timeout waiting for navigation download")); + await Promise.resolve(); + }), + url: vi.fn(() => "https://example.com/final"), + }); + + const result = await mod.navigateViaPlaywright({ + cdpUrl: "http://127.0.0.1:18792", + url: "https://example.com/final", + ssrfPolicy: { allowPrivateNetwork: true }, + }); + + expect(result).toEqual({ url: "https://example.com/final" }); + expect(downloadCapture.cancel).toHaveBeenCalledTimes(1); + }); + + it("closes the tab when captured navigation download resolves to a blocked URL", async () => { + const download = { + url: "http://127.0.0.1:18080/export.csv", + suggestedFilename: "export.csv", + path: "/tmp/openclaw/downloads/export.csv", + }; + const downloadCapture = { + armed: true, + promise: Promise.resolve(download), + cancel: vi.fn(), + }; + setPwToolsCoreDownloadCapture(downloadCapture); + const page = { + goto: vi.fn(async () => { + throw new Error("page.goto: Download is starting"); + }), + url: vi.fn(() => "https://93.184.216.34/start"), + }; + setPwToolsCoreCurrentPage(page); + getPwToolsCoreNavigationGuardMocks().assertBrowserNavigationResultAllowed.mockRejectedValueOnce( + new SsrFBlockedError("Blocked hostname or private/internal/special-use IP address"), + ); + + await expect( + mod.navigateViaPlaywright({ + cdpUrl: "http://127.0.0.1:18792", + targetId: "tab-1", + url: "https://93.184.216.34/export.csv", + ssrfPolicy: { dangerouslyAllowPrivateNetwork: false }, + }), + ).rejects.toBeInstanceOf(SsrFBlockedError); + + expect(getPwToolsCoreSessionMocks().closeBlockedNavigationTarget).toHaveBeenCalledWith({ + cdpUrl: "http://127.0.0.1:18792", + page, + targetId: "tab-1", + }); + }); + + it("surfaces managed download save failures", async () => { + const downloadCapture = { + armed: true, + promise: Promise.reject(new Error("download save failed")), + cancel: vi.fn(), + }; + setPwToolsCoreDownloadCapture(downloadCapture); + setPwToolsCoreCurrentPage({ + goto: vi.fn(async () => { + throw new Error("page.goto: Download is starting"); + }), + url: vi.fn(() => "https://example.com/start"), + }); + + await expect( + mod.navigateViaPlaywright({ + cdpUrl: "http://127.0.0.1:18792", + targetId: "tab-1", + url: "https://example.com/export.csv", + ssrfPolicy: { allowPrivateNetwork: true }, + }), + ).rejects.toThrow("download save failed"); + }); + + it("rethrows download-starting navigation errors when no download is captured", async () => { + const downloadCapture = { + armed: false, + promise: new Promise(() => {}), + cancel: vi.fn(), + }; + setPwToolsCoreDownloadCapture(downloadCapture); + setPwToolsCoreCurrentPage({ + goto: vi.fn(async () => { + throw new Error("page.goto: Download is starting"); + }), + url: vi.fn(() => "https://example.com/start"), + }); + + await expect( + mod.navigateViaPlaywright({ + cdpUrl: "http://127.0.0.1:18792", + targetId: "tab-1", + url: "https://example.com/export.csv", + ssrfPolicy: { allowPrivateNetwork: true }, + }), + ).rejects.toThrow("Download is starting"); + + expect(downloadCapture.cancel).toHaveBeenCalledTimes(1); + }); + it("reconnects and retries once when navigation detaches frame", async () => { const goto = vi .fn<(...args: unknown[]) => Promise>() diff --git a/extensions/browser/src/browser/pw-tools-core.snapshot.test.ts b/extensions/browser/src/browser/pw-tools-core.snapshot.test.ts index e5682530ff95..9a87c0f4fac8 100644 --- a/extensions/browser/src/browser/pw-tools-core.snapshot.test.ts +++ b/extensions/browser/src/browser/pw-tools-core.snapshot.test.ts @@ -8,6 +8,11 @@ const withPageScopedCdpClient = vi.fn(); const markBackendDomRefsOnPage = vi.fn(); const formatAriaSnapshot = vi.fn(); const gotoPageWithNavigationGuard = vi.fn(); +const createDownloadCaptureForPage = vi.fn(() => ({ + armed: true, + promise: new Promise(() => {}), + cancel: vi.fn(), +})); vi.mock("./pw-session.js", () => ({ assertPageNavigationCompletedSafely: vi.fn(), @@ -16,10 +21,15 @@ vi.mock("./pw-session.js", () => ({ forceDisconnectPlaywrightForTarget: vi.fn(), getPageForTargetId, gotoPageWithNavigationGuard, + isDownloadStartingNavigationError: vi.fn(() => false), isPolicyDenyNavigationError: vi.fn(() => false), storeRoleRefsForTarget, })); +vi.mock("./pw-download-capture.js", () => ({ + createDownloadCaptureForPage, +})); + vi.mock("./pw-session.page-cdp.js", () => ({ markBackendDomRefsOnPage, withPageScopedCdpClient, diff --git a/extensions/browser/src/browser/pw-tools-core.snapshot.ts b/extensions/browser/src/browser/pw-tools-core.snapshot.ts index 42280debd966..d6e0b59c85e6 100644 --- a/extensions/browser/src/browser/pw-tools-core.snapshot.ts +++ b/extensions/browser/src/browser/pw-tools-core.snapshot.ts @@ -12,11 +12,14 @@ import type { Page } from "playwright-core"; import type { SsrFPolicy } from "../infra/net/ssrf.js"; import { ACT_MAX_VIEWPORT_DIMENSION } from "./act-policy.js"; import { type AriaSnapshotNode, formatAriaSnapshot, type RawAXNode } from "./cdp.js"; +import type { BrowserDownloadResult } from "./download-types.js"; import { assertBrowserNavigationAllowed, + assertBrowserNavigationResultAllowed, type BrowserNavigationPolicyOptions, withBrowserNavigationPolicy, } from "./navigation-guard.js"; +import { createDownloadCaptureForPage } from "./pw-download-capture.js"; import { buildRoleSnapshotFromAiSnapshot, buildRoleSnapshotFromAriaSnapshot, @@ -31,6 +34,7 @@ import { forceDisconnectPlaywrightForTarget, getPageForTargetId, gotoPageWithNavigationGuard, + isDownloadStartingNavigationError, isPolicyDenyNavigationError, storeRoleRefsForTarget, } from "./pw-session.js"; @@ -397,7 +401,7 @@ export async function navigateViaPlaywright(opts: { timeoutMs?: number; ssrfPolicy?: SsrFPolicy; browserProxyMode?: BrowserNavigationPolicyOptions["browserProxyMode"]; -}): Promise<{ url: string }> { +}): Promise<{ url: string; download?: BrowserDownloadResult }> { const isRetryableNavigateError = (err: unknown): boolean => { const msg = typeof err === "string" @@ -415,15 +419,16 @@ export async function navigateViaPlaywright(opts: { if (!url) { throw new Error("url is required"); } + const navigationPolicy = withBrowserNavigationPolicy(opts.ssrfPolicy, { + browserProxyMode: opts.browserProxyMode, + }); await assertBrowserNavigationAllowed({ url, - ...withBrowserNavigationPolicy(opts.ssrfPolicy, { - browserProxyMode: opts.browserProxyMode, - }), + ...navigationPolicy, }); const timeout = resolveNavigationTimeoutMs(opts.timeoutMs); let page = await getPageForTargetId(opts); - ensurePageState(page); + let pageState = ensurePageState(page); const navigate = async () => await gotoPageWithNavigationGuard({ cdpUrl: opts.cdpUrl, @@ -434,9 +439,54 @@ export async function navigateViaPlaywright(opts: { browserProxyMode: opts.browserProxyMode, targetId: opts.targetId, }); - let response; + const navigateWithDownloadCapture = async (): Promise<{ + response: Awaited> | null; + download?: BrowserDownloadResult; + }> => { + const downloadCapture = createDownloadCaptureForPage(page, pageState, timeout, { + mode: "passive", + timeoutMessage: "Timeout waiting for navigation download", + beforeSave: async (download) => { + await assertBrowserNavigationResultAllowed({ + url: download.url || url, + ...navigationPolicy, + }); + }, + }); + void downloadCapture.promise.catch(() => {}); + try { + const response = await navigate(); + downloadCapture.cancel(); + return { response }; + } catch (err) { + if (!isDownloadStartingNavigationError(err, url) || !downloadCapture.armed) { + downloadCapture.cancel(); + throw err; + } + try { + return { response: null, download: await downloadCapture.promise }; + } catch (downloadErr) { + if ( + downloadErr instanceof Error && + downloadErr.message === "Timeout waiting for navigation download" + ) { + throw err; + } + if (isPolicyDenyNavigationError(downloadErr)) { + await closeBlockedNavigationTarget({ + cdpUrl: opts.cdpUrl, + page, + targetId: opts.targetId, + }); + } + throw downloadErr; + } + } + }; + + let navigationResult: Awaited>; try { - response = await navigate(); + navigationResult = await navigateWithDownloadCapture(); } catch (err) { if (!isRetryableNavigateError(err)) { throw err; @@ -450,18 +500,20 @@ export async function navigateViaPlaywright(opts: { reason: "retry navigate after detached frame", }).catch(() => {}); page = await getPageForTargetId(opts); - ensurePageState(page); - response = await navigate(); + pageState = ensurePageState(page); + navigationResult = await navigateWithDownloadCapture(); } try { - await assertPageNavigationCompletedSafely({ - cdpUrl: opts.cdpUrl, - page, - response, - ssrfPolicy: opts.ssrfPolicy, - browserProxyMode: opts.browserProxyMode, - targetId: opts.targetId, - }); + if (!navigationResult.download) { + await assertPageNavigationCompletedSafely({ + cdpUrl: opts.cdpUrl, + page, + response: navigationResult.response, + ssrfPolicy: opts.ssrfPolicy, + browserProxyMode: opts.browserProxyMode, + targetId: opts.targetId, + }); + } } catch (err) { if (isPolicyDenyNavigationError(err)) { await closeBlockedNavigationTarget({ @@ -472,8 +524,11 @@ export async function navigateViaPlaywright(opts: { } throw err; } - const finalUrl = page.url(); - return { url: finalUrl }; + const finalUrl = navigationResult.download?.url || page.url(); + return { + url: finalUrl, + ...(navigationResult.download ? { download: navigationResult.download } : {}), + }; } /** Resizes the target page viewport within the browser action policy bounds. */ diff --git a/extensions/browser/src/browser/pw-tools-core.test-harness.ts b/extensions/browser/src/browser/pw-tools-core.test-harness.ts index 7eb78181aba1..ed13a67c6998 100644 --- a/extensions/browser/src/browser/pw-tools-core.test-harness.ts +++ b/extensions/browser/src/browser/pw-tools-core.test-harness.ts @@ -6,6 +6,20 @@ import { beforeEach, vi } from "vitest"; let currentPage: Record | null = null; let currentRefLocator: Record | null = null; +type HarnessManagedDownload = { + url: string; + suggestedFilename: string; + path: string; +}; +type HarnessDownloadCapture = { + armed: boolean; + promise: Promise; + cancel: ReturnType; +}; +type HarnessDownloadCaptureOptions = { + beforeSave?: (download: Omit) => Promise | void; +}; +let currentDownloadCapture: HarnessDownloadCapture | undefined; let pageState: { console: unknown[]; armIdUpload: number; @@ -37,6 +51,19 @@ const sessionMocks = vi.hoisted(() => ({ }) => (await opts.page.goto(opts.url, { timeout: opts.timeoutMs })) ?? null, ), // Match by name so mocked errors are recognized without importing real classes. + isDownloadStartingNavigationError: vi.fn((err: unknown, expectedUrl?: string) => { + if (!(err instanceof Error)) { + return false; + } + const message = err.message.toLowerCase(); + if (message.includes("download is starting")) { + return true; + } + const normalizedUrl = expectedUrl?.trim().toLowerCase(); + return Boolean( + normalizedUrl && message.includes("net::err_aborted") && message.includes(normalizedUrl), + ); + }), isPolicyDenyNavigationError: vi.fn((err: unknown) => { if (!(err instanceof Error)) { return false; @@ -63,12 +90,44 @@ const sessionMocks = vi.hoisted(() => ({ rememberRoleRefsForTarget: vi.fn(() => {}), })); +const downloadCaptureMocks = vi.hoisted(() => ({ + createDownloadCaptureForPage: vi.fn(), +})); + const navigationGuardMocks = vi.hoisted(() => ({ assertBrowserNavigationResultAllowed: vi.fn(async () => {}), withBrowserNavigationPolicy: vi.fn((ssrfPolicy?: unknown) => ({ ssrfPolicy })), })); vi.mock("./pw-session.js", () => sessionMocks); +vi.mock("./pw-download-capture.js", async (importOriginal) => { + const actual = await importOriginal(); + downloadCaptureMocks.createDownloadCaptureForPage.mockImplementation( + (page, state, timeoutMs, opts?: HarnessDownloadCaptureOptions) => { + const capture = currentDownloadCapture; + if (!capture) { + return actual.createDownloadCaptureForPage(page, state, timeoutMs, opts); + } + if (!opts?.beforeSave) { + return capture; + } + return { + ...capture, + promise: capture.promise.then(async (download) => { + await opts.beforeSave?.({ + url: download.url, + suggestedFilename: download.suggestedFilename, + }); + return download; + }), + }; + }, + ); + return { + ...actual, + createDownloadCaptureForPage: downloadCaptureMocks.createDownloadCaptureForPage, + }; +}); vi.mock("./navigation-guard.js", async (importOriginal) => { const actual = await importOriginal>(); return { @@ -89,6 +148,10 @@ export function getPwToolsCoreNavigationGuardMocks() { /** Sets the current mocked page returned by getPageForTargetId. */ export function setPwToolsCoreCurrentPage(page: Record | null) { + if (page) { + page.on ??= vi.fn(); + page.off ??= vi.fn(); + } currentPage = page; } @@ -97,11 +160,16 @@ export function setPwToolsCoreCurrentRefLocator(locator: Record currentRefLocator = locator; } +export function setPwToolsCoreDownloadCapture(capture: HarnessDownloadCapture | undefined) { + currentDownloadCapture = capture; +} + /** Installs per-test cleanup for pw-tools-core mocked session state. */ export function installPwToolsCoreTestHooks() { beforeEach(() => { currentPage = null; currentRefLocator = null; + currentDownloadCapture = undefined; pageState = { console: [], armIdUpload: 0, @@ -112,6 +180,9 @@ export function installPwToolsCoreTestHooks() { for (const fn of Object.values(sessionMocks)) { fn.mockClear(); } + for (const fn of Object.values(downloadCaptureMocks)) { + fn.mockClear(); + } for (const fn of Object.values(navigationGuardMocks)) { fn.mockClear(); } diff --git a/extensions/browser/src/browser/pw-tools-core.waits-next-download-saves-it.test.ts b/extensions/browser/src/browser/pw-tools-core.waits-next-download-saves-it.test.ts index 75cbeb7e09b2..7db88e8bc1ba 100644 --- a/extensions/browser/src/browser/pw-tools-core.waits-next-download-saves-it.test.ts +++ b/extensions/browser/src/browser/pw-tools-core.waits-next-download-saves-it.test.ts @@ -173,10 +173,16 @@ describe("pw-tools-core", () => { const harness = createDownloadEventHarness(); const targetPath = path.join(tempDir, "file.bin"); - const saveAs = vi.fn(async (outPath: string) => { + type DownloadFixture = { + url: () => string; + suggestedFilename: () => string; + saveAs: (outPath: string) => Promise; + }; + const saveAs = vi.fn(async function (this: DownloadFixture, outPath: string) { + expect(this).toBe(download); await fs.writeFile(outPath, "file-content", "utf8"); }); - const download = { + const download: DownloadFixture = { url: () => "https://example.com/file.bin", suggestedFilename: () => "file.bin", saveAs, @@ -309,6 +315,41 @@ describe("pw-tools-core", () => { expect(state.downloadWaiterDepth).toBe(0); expect(harness.activeHandlerCount()).toBe(0); }); + + it("lets only the latest overlapping explicit waiter save the download", async () => { + const harness = createDownloadEventHarness(); + const state = sessionMocks.ensurePageState(); + const saveAs = vi.fn(async (outPath: string) => { + await fs.writeFile(outPath, "latest-content", "utf8"); + }); + + const first = mod.waitForDownloadViaPlaywright({ + cdpUrl: "http://127.0.0.1:18792", + targetId: "T1", + timeoutMs: 1000, + }); + void first.catch(() => {}); + const latest = mod.waitForDownloadViaPlaywright({ + cdpUrl: "http://127.0.0.1:18792", + targetId: "T1", + timeoutMs: 1000, + }); + + await Promise.resolve(); + expect(state.downloadWaiterDepth).toBe(2); + harness.trigger({ + url: () => "https://example.com/latest.bin", + suggestedFilename: () => "latest.bin", + saveAs, + }); + + await expect(first).rejects.toThrow("superseded by another waiter"); + await expect(latest).resolves.toMatchObject({ suggestedFilename: "latest.bin" }); + expect(saveAs).toHaveBeenCalledOnce(); + expect(state.downloadWaiterDepth).toBe(0); + expect(harness.activeHandlerCount()).toBe(0); + }); + it("clicks a ref and atomically finalizes explicit download paths", async () => { await withTempDir(async (tempDir) => { const harness = createDownloadEventHarness();