mirror of
https://github.com/openclaw/openclaw.git
synced 2026-07-10 00:11:19 +00:00
fix(browser): notify agent when click triggers download (#93307)
* fix(browser): drain download saves and use monotonic cursor for act response - Add drainPendingDownloadSaves() to wait for in-flight saveAs before sampling - Use monotonic downloadSeq cursor instead of bounded-list length - Propagate downloads info in POST /act response for click/batch/evaluate Fixes #93250 * fix(lint): add braces around single-line if returns Fixes eslint(curly) failures in drainPendingDownloadSaves and pickNewDownloads. PR #93307 required CI gate. Ref: ClawSweeper P1 review finding * fix(browser): scope act download metadata to action * fix(browser): broadcast downloads to all active captures to prevent misattribution When concurrent /act calls overlap on the same page, using state.actionDownloadCaptures.at(-1) assigned downloads to the wrong action's capture. Push managed save promises to all active captures so the triggering action always receives its download metadata. Also adds regression tests: broadcast-to-all-captures, sequential capture isolation, and strengthen the dispose test to assert no re-capture after disposal. * fix(browser): prevent unhandled rejection when download capture action throws before drain Move managedSave.catch() before the captures-branch check so the rejection handler always runs, preventing an unhandled promise rejection when the action throws before drain() is called. Simplify the handler by removing the now-dead return + catch at the bottom. * fix(browser): type downloads in BrowserActResponse, fix lint unused var - Add optional downloads field to BrowserActResponse type contract so typed callers of browserAct() can consume the new payload without casts. - Remove unused afterDispose variable in pw-session tests (lint fix). * fix(test): correct post-dispose download assertion capture.promises is not cleared by dispose(), so re-draining a disposed capture still returns pre-dispose results. This is benign — callers should drain before disposing. Update the test to assert the actual behavior (still shows old results, new download not captured). * test(browser): cover /act download metadata response * refactor(browser): report action-owned downloads safely * fix(browser): close action download ownership races * test(browser): type action download capture mock --------- Co-authored-by: Peter Steinberger <steipete@gmail.com>
This commit is contained in:
parent
0bd2a0b879
commit
ae53570ea8
10 changed files with 684 additions and 16 deletions
|
|
@ -19,6 +19,7 @@ OpenClaw can run a **dedicated Chrome/Brave/Edge/Chromium profile** that the age
|
|||
- 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.
|
||||
- Playwright-backed agent actions return a `downloads` array with the same managed metadata when the action immediately starts one or more downloads.
|
||||
- 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.
|
||||
|
|
|
|||
|
|
@ -21,6 +21,7 @@ import {
|
|||
DEFAULT_BROWSER_ACTION_TIMEOUT_MS,
|
||||
DEFAULT_BROWSER_SCREENSHOT_TIMEOUT_MS,
|
||||
} from "./constants.js";
|
||||
import type { BrowserDownloadResult } from "./download-types.js";
|
||||
|
||||
export type { BrowserFormField } from "./client-actions.types.js";
|
||||
|
||||
|
|
@ -32,6 +33,8 @@ type BrowserActResponse = {
|
|||
results?: Array<{ ok: boolean; error?: string }>;
|
||||
blockedByDialog?: boolean;
|
||||
browserState?: unknown;
|
||||
/** Download info when a click/batch/evaluate action triggers a browser download. */
|
||||
downloads?: BrowserDownloadResult[];
|
||||
};
|
||||
|
||||
const BROWSER_ACT_REQUEST_TIMEOUT_SLACK_MS = 5_000;
|
||||
|
|
|
|||
|
|
@ -210,6 +210,13 @@ describe("browser client", () => {
|
|||
url: "https://x",
|
||||
result: 1,
|
||||
results: [{ ok: true }],
|
||||
downloads: [
|
||||
{
|
||||
url: "https://x/report.pdf",
|
||||
suggestedFilename: "report.pdf",
|
||||
path: "/tmp/openclaw/downloads/report.pdf",
|
||||
},
|
||||
],
|
||||
}),
|
||||
} as unknown as Response;
|
||||
}
|
||||
|
|
@ -346,6 +353,13 @@ describe("browser client", () => {
|
|||
expect(act.ok).toBe(true);
|
||||
expect(act.targetId).toBe("t1");
|
||||
expect(act.results).toEqual([{ ok: true }]);
|
||||
expect(act.downloads).toEqual([
|
||||
{
|
||||
url: "https://x/report.pdf",
|
||||
suggestedFilename: "report.pdf",
|
||||
path: "/tmp/openclaw/downloads/report.pdf",
|
||||
},
|
||||
]);
|
||||
|
||||
const fileChooser = await browserArmFileChooser("http://127.0.0.1:18791", {
|
||||
paths: ["/tmp/a.txt"],
|
||||
|
|
|
|||
|
|
@ -6,6 +6,7 @@ import { afterEach, describe, expect, it, vi } from "vitest";
|
|||
import { DEFAULT_DOWNLOAD_DIR } from "./paths.js";
|
||||
import { createDownloadCaptureForPage } from "./pw-download-capture.js";
|
||||
import {
|
||||
beginActionDownloadCaptureOnPage,
|
||||
ensurePageState,
|
||||
isDownloadStartingNavigationError,
|
||||
refLocator,
|
||||
|
|
@ -15,6 +16,7 @@ import {
|
|||
import { BROWSER_REF_MARKER_ATTRIBUTE } from "./pw-session.page-cdp.js";
|
||||
|
||||
type MutableDownload = {
|
||||
url?: () => string;
|
||||
suggestedFilename: () => string;
|
||||
saveAs: ReturnType<typeof vi.fn>;
|
||||
path?: () => Promise<string>;
|
||||
|
|
@ -250,6 +252,312 @@ describe("pw-session ensurePageState", () => {
|
|||
expect(download.saveAs).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("reports all downloads owned by the active action with managed metadata", async () => {
|
||||
const { page, handlers } = fakePage();
|
||||
ensurePageState(page);
|
||||
const capture = beginActionDownloadCaptureOnPage(page);
|
||||
const firstSave = vi.fn(async (outPath: string) => {
|
||||
await fs.writeFile(outPath, "first-action-download", "utf8");
|
||||
});
|
||||
const secondSave = vi.fn(async (outPath: string) => {
|
||||
await fs.writeFile(outPath, "second-action-download", "utf8");
|
||||
});
|
||||
|
||||
for (const download of [
|
||||
{
|
||||
url: () => "https://example.com/first.txt",
|
||||
suggestedFilename: () => "first.txt",
|
||||
saveAs: firstSave,
|
||||
},
|
||||
{
|
||||
url: () => "https://example.com/second.txt",
|
||||
suggestedFilename: () => "second.txt",
|
||||
saveAs: secondSave,
|
||||
},
|
||||
]) {
|
||||
handlers.get("download")?.[0]?.(download);
|
||||
}
|
||||
|
||||
const result = await capture.drain();
|
||||
capture.dispose();
|
||||
|
||||
expect(result).toEqual([
|
||||
{
|
||||
url: "https://example.com/first.txt",
|
||||
suggestedFilename: "first.txt",
|
||||
path: expect.stringMatching(/-first\.txt$/),
|
||||
},
|
||||
{
|
||||
url: "https://example.com/second.txt",
|
||||
suggestedFilename: "second.txt",
|
||||
path: expect.stringMatching(/-second\.txt$/),
|
||||
},
|
||||
]);
|
||||
await expect(fs.readFile(result?.[0]?.path ?? "", "utf8")).resolves.toBe(
|
||||
"first-action-download",
|
||||
);
|
||||
await expect(fs.readFile(result?.[1]?.path ?? "", "utf8")).resolves.toBe(
|
||||
"second-action-download",
|
||||
);
|
||||
});
|
||||
|
||||
it("waits only the requested first-event grace for a just-late action download", async () => {
|
||||
const { page, handlers } = fakePage();
|
||||
ensurePageState(page);
|
||||
const capture = beginActionDownloadCaptureOnPage(page);
|
||||
const saveAs = vi.fn(async (outPath: string) => {
|
||||
await fs.writeFile(outPath, "late-action-download", "utf8");
|
||||
});
|
||||
const drain = capture.drain({ firstEventGraceMs: 1_000 });
|
||||
|
||||
setImmediate(() => {
|
||||
handlers.get("download")?.[0]?.({
|
||||
url: () => "https://example.com/late.txt",
|
||||
suggestedFilename: () => "late.txt",
|
||||
saveAs,
|
||||
});
|
||||
});
|
||||
|
||||
await expect(drain).resolves.toEqual([
|
||||
expect.objectContaining({ suggestedFilename: "late.txt" }),
|
||||
]);
|
||||
capture.dispose();
|
||||
});
|
||||
|
||||
it("keeps a quiet window open for sibling downloads after the first event", async () => {
|
||||
const { page, handlers } = fakePage();
|
||||
ensurePageState(page);
|
||||
const capture = beginActionDownloadCaptureOnPage(page);
|
||||
const save = (contents: string) =>
|
||||
vi.fn(async (outPath: string) => {
|
||||
await fs.writeFile(outPath, contents, "utf8");
|
||||
});
|
||||
|
||||
handlers.get("download")?.[0]?.({
|
||||
url: () => "https://example.com/first.txt",
|
||||
suggestedFilename: () => "first.txt",
|
||||
saveAs: save("first"),
|
||||
});
|
||||
setImmediate(() => {
|
||||
handlers.get("download")?.[0]?.({
|
||||
url: () => "https://example.com/second.txt",
|
||||
suggestedFilename: () => "second.txt",
|
||||
saveAs: save("second"),
|
||||
});
|
||||
});
|
||||
|
||||
await expect(capture.drain({ quietMs: 1_000 })).resolves.toEqual([
|
||||
expect.objectContaining({ suggestedFilename: "first.txt" }),
|
||||
expect.objectContaining({ suggestedFilename: "second.txt" }),
|
||||
]);
|
||||
capture.dispose();
|
||||
});
|
||||
|
||||
it("detaches ownership before waiting for slow file saves", async () => {
|
||||
const { page, handlers } = fakePage();
|
||||
ensurePageState(page);
|
||||
let releaseFirstSave: (() => void) | undefined;
|
||||
const firstSaveGate = new Promise<void>((resolve) => {
|
||||
releaseFirstSave = resolve;
|
||||
});
|
||||
const beforeSave = vi.fn(async () => {});
|
||||
const capture = beginActionDownloadCaptureOnPage(page, { beforeSave });
|
||||
const firstSave = vi.fn(async (outPath: string) => {
|
||||
await firstSaveGate;
|
||||
await fs.writeFile(outPath, "first", "utf8");
|
||||
});
|
||||
handlers.get("download")?.[0]?.({
|
||||
url: () => "https://example.com/first.txt",
|
||||
suggestedFilename: () => "first.txt",
|
||||
saveAs: firstSave,
|
||||
});
|
||||
|
||||
const drain = capture.drain();
|
||||
const lateSave = vi.fn(async (outPath: string) => {
|
||||
await fs.writeFile(outPath, "late", "utf8");
|
||||
});
|
||||
const lateDownload: MutableDownload = {
|
||||
url: () => "https://example.com/late.txt",
|
||||
suggestedFilename: () => "late.txt",
|
||||
saveAs: lateSave,
|
||||
};
|
||||
handlers.get("download")?.[0]?.(lateDownload);
|
||||
releaseFirstSave?.();
|
||||
|
||||
await expect(drain).resolves.toEqual([
|
||||
expect.objectContaining({ suggestedFilename: "first.txt" }),
|
||||
]);
|
||||
await expect(lateDownload.path?.()).resolves.toMatch(/-late\.txt$/);
|
||||
expect(beforeSave).toHaveBeenCalledOnce();
|
||||
expect(lateSave).toHaveBeenCalledOnce();
|
||||
capture.dispose();
|
||||
});
|
||||
|
||||
it("keeps started saves with their owner and assigns future downloads to the latest action", async () => {
|
||||
const { page, handlers } = fakePage();
|
||||
ensurePageState(page);
|
||||
const first = beginActionDownloadCaptureOnPage(page);
|
||||
const firstSaveAs = vi.fn(async (outPath: string) => {
|
||||
await fs.writeFile(outPath, "first-action-download", "utf8");
|
||||
});
|
||||
handlers.get("download")?.[0]?.({
|
||||
url: () => "https://example.com/first.txt",
|
||||
suggestedFilename: () => "first.txt",
|
||||
saveAs: firstSaveAs,
|
||||
});
|
||||
|
||||
const latest = beginActionDownloadCaptureOnPage(page);
|
||||
first.dispose();
|
||||
const latestSaveAs = vi.fn(async (outPath: string) => {
|
||||
await fs.writeFile(outPath, "latest-action-download", "utf8");
|
||||
});
|
||||
handlers.get("download")?.[0]?.({
|
||||
url: () => "https://example.com/latest.txt",
|
||||
suggestedFilename: () => "latest.txt",
|
||||
saveAs: latestSaveAs,
|
||||
});
|
||||
|
||||
await expect(first.drain()).resolves.toEqual([
|
||||
expect.objectContaining({ suggestedFilename: "first.txt" }),
|
||||
]);
|
||||
await expect(latest.drain()).resolves.toEqual([
|
||||
expect.objectContaining({ suggestedFilename: "latest.txt" }),
|
||||
]);
|
||||
latest.dispose();
|
||||
expect(firstSaveAs).toHaveBeenCalledOnce();
|
||||
expect(latestSaveAs).toHaveBeenCalledOnce();
|
||||
});
|
||||
|
||||
it("leaves action capture empty while an explicit download owner is armed", async () => {
|
||||
const { page, handlers } = fakePage();
|
||||
const state = ensurePageState(page);
|
||||
state.downloadWaiterDepth = 1;
|
||||
const capture = beginActionDownloadCaptureOnPage(page);
|
||||
const download = {
|
||||
suggestedFilename: () => "explicit.txt",
|
||||
saveAs: vi.fn(async () => {}),
|
||||
};
|
||||
|
||||
handlers.get("download")?.[0]?.(download);
|
||||
|
||||
await expect(capture.drain()).resolves.toBeUndefined();
|
||||
capture.dispose();
|
||||
expect(download).not.toHaveProperty("path");
|
||||
expect(download.saveAs).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("validates action-owned download URLs before saving bytes", async () => {
|
||||
const { page, handlers } = fakePage();
|
||||
ensurePageState(page);
|
||||
const blocked = new Error("blocked action download");
|
||||
const beforeSave = vi.fn(async () => {
|
||||
throw blocked;
|
||||
});
|
||||
const capture = beginActionDownloadCaptureOnPage(page, { beforeSave });
|
||||
const saveAs = vi.fn(async (outPath: string) => {
|
||||
await fs.writeFile(outPath, "blocked-action-download", "utf8");
|
||||
});
|
||||
|
||||
handlers.get("download")?.[0]?.({
|
||||
url: () => "http://127.0.0.1/private.txt",
|
||||
suggestedFilename: () => "private.txt",
|
||||
saveAs,
|
||||
});
|
||||
|
||||
await expect(capture.drain()).rejects.toBe(blocked);
|
||||
capture.dispose();
|
||||
expect(beforeSave).toHaveBeenCalledWith({
|
||||
url: "http://127.0.0.1/private.txt",
|
||||
suggestedFilename: "private.txt",
|
||||
});
|
||||
expect(saveAs).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("drains late siblings before surfacing the first download policy failure", async () => {
|
||||
const { page, handlers } = fakePage();
|
||||
ensurePageState(page);
|
||||
const blocked = new Error("blocked action download");
|
||||
const beforeSave = vi.fn(async () => {
|
||||
throw blocked;
|
||||
});
|
||||
const capture = beginActionDownloadCaptureOnPage(page, { beforeSave });
|
||||
const firstSave = vi.fn(async () => {});
|
||||
const secondSave = vi.fn(async () => {});
|
||||
|
||||
handlers.get("download")?.[0]?.({
|
||||
url: () => "http://127.0.0.1/first.txt",
|
||||
suggestedFilename: () => "first.txt",
|
||||
saveAs: firstSave,
|
||||
});
|
||||
setImmediate(() => {
|
||||
handlers.get("download")?.[0]?.({
|
||||
url: () => "http://127.0.0.1/second.txt",
|
||||
suggestedFilename: () => "second.txt",
|
||||
saveAs: secondSave,
|
||||
});
|
||||
});
|
||||
|
||||
await expect(capture.drain({ quietMs: 1_000 })).rejects.toBe(blocked);
|
||||
capture.dispose();
|
||||
expect(beforeSave).toHaveBeenCalledTimes(2);
|
||||
expect(firstSave).not.toHaveBeenCalled();
|
||||
expect(secondSave).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("surfaces a sibling policy denial without waiting for an allowed slow save", async () => {
|
||||
const { page, handlers } = fakePage();
|
||||
ensurePageState(page);
|
||||
const blocked = new Error("blocked sibling download");
|
||||
const beforeSave = vi.fn(async (candidate: { url: string }) => {
|
||||
if (candidate.url.endsWith("/blocked.txt")) {
|
||||
throw blocked;
|
||||
}
|
||||
});
|
||||
const capture = beginActionDownloadCaptureOnPage(page, { beforeSave });
|
||||
let releaseAllowedSave: (() => void) | undefined;
|
||||
const allowedSaveGate = new Promise<void>((resolve) => {
|
||||
releaseAllowedSave = resolve;
|
||||
});
|
||||
const allowedDownload: MutableDownload = {
|
||||
url: () => "https://example.com/allowed.txt",
|
||||
suggestedFilename: () => "allowed.txt",
|
||||
saveAs: vi.fn(async (outPath: string) => {
|
||||
await allowedSaveGate;
|
||||
await fs.writeFile(outPath, "allowed", "utf8");
|
||||
}),
|
||||
};
|
||||
handlers.get("download")?.[0]?.(allowedDownload);
|
||||
setImmediate(() => {
|
||||
handlers.get("download")?.[0]?.({
|
||||
url: () => "https://example.com/blocked.txt",
|
||||
suggestedFilename: () => "blocked.txt",
|
||||
saveAs: vi.fn(async () => {}),
|
||||
});
|
||||
});
|
||||
|
||||
await expect(capture.drain({ quietMs: 100 })).rejects.toBe(blocked);
|
||||
releaseAllowedSave?.();
|
||||
await expect(allowedDownload.path?.()).resolves.toMatch(/-allowed\.txt$/);
|
||||
capture.dispose();
|
||||
});
|
||||
|
||||
it("surfaces action-owned download save failures without an unhandled rejection", async () => {
|
||||
const { page, handlers } = fakePage();
|
||||
ensurePageState(page);
|
||||
const capture = beginActionDownloadCaptureOnPage(page);
|
||||
const error = new Error("action download save failed");
|
||||
|
||||
handlers.get("download")?.[0]?.({
|
||||
suggestedFilename: () => "failed.txt",
|
||||
saveAs: vi.fn(async () => {
|
||||
throw error;
|
||||
}),
|
||||
});
|
||||
|
||||
await expect(capture.drain()).rejects.toBe(error);
|
||||
capture.dispose();
|
||||
});
|
||||
|
||||
it("captures navigation downloads under managed paths", async () => {
|
||||
const { page, handlers } = fakePage();
|
||||
const state = ensurePageState(page);
|
||||
|
|
|
|||
|
|
@ -35,6 +35,7 @@ import {
|
|||
} from "./cdp.helpers.js";
|
||||
import { AX_REF_PATTERN, normalizeCdpWsUrl } from "./cdp.js";
|
||||
import { getChromeWebSocketUrl } from "./chrome.js";
|
||||
import type { BrowserDownloadCandidate, BrowserDownloadResult } from "./download-types.js";
|
||||
import { BrowserTabNotFoundError } from "./errors.js";
|
||||
import {
|
||||
assertBrowserNavigationAllowed,
|
||||
|
|
@ -45,7 +46,11 @@ import {
|
|||
withBrowserNavigationPolicy,
|
||||
} from "./navigation-guard.js";
|
||||
import { playwrightCore } from "./playwright-core.runtime.js";
|
||||
import { saveBrowserDownload, type PlaywrightDownload } from "./pw-download-capture.js";
|
||||
import {
|
||||
saveBrowserDownload,
|
||||
type BrowserDownloadCaptureOptions,
|
||||
type PlaywrightDownload,
|
||||
} from "./pw-download-capture.js";
|
||||
import { BROWSER_REF_MARKER_ATTRIBUTE, withPageScopedCdpClient } from "./pw-session.page-cdp.js";
|
||||
|
||||
const { chromium } = playwrightCore;
|
||||
|
|
@ -145,6 +150,14 @@ type DownloadPayload = PlaywrightDownload & {
|
|||
path?: () => Promise<string>;
|
||||
};
|
||||
|
||||
type ActionDownloadCapture = {
|
||||
beforeSave?: (download: BrowserDownloadCandidate) => Promise<void> | void;
|
||||
lastEventAtMs?: number;
|
||||
pending: Array<Promise<BrowserDownloadResult>>;
|
||||
validations: Array<Promise<void>>;
|
||||
waiters: Array<() => void>;
|
||||
};
|
||||
|
||||
type PageState = {
|
||||
console: BrowserConsoleMessage[];
|
||||
errors: BrowserPageError[];
|
||||
|
|
@ -154,6 +167,7 @@ type PageState = {
|
|||
armIdUpload: number;
|
||||
armIdDownload: number;
|
||||
downloadWaiterDepth: number;
|
||||
actionDownloadCapture?: ActionDownloadCapture;
|
||||
nextObservedDialogId: number;
|
||||
pendingDialogs: PendingObservedDialog[];
|
||||
recentDialogs: BrowserObservedDialogRecord[];
|
||||
|
|
@ -236,6 +250,84 @@ export function isDownloadStartingNavigationError(err: unknown, expectedUrl?: st
|
|||
);
|
||||
}
|
||||
|
||||
/** Capture downloads started synchronously by one Browser action. */
|
||||
export function beginActionDownloadCaptureOnPage(
|
||||
page: Page,
|
||||
opts: {
|
||||
beforeSave?: (download: BrowserDownloadCandidate) => Promise<void> | void;
|
||||
} = {},
|
||||
): {
|
||||
drain: (opts?: {
|
||||
firstEventGraceMs?: number;
|
||||
maxWaitMs?: number;
|
||||
quietMs?: number;
|
||||
}) => Promise<BrowserDownloadResult[] | undefined>;
|
||||
dispose: () => void;
|
||||
} {
|
||||
const state = ensurePageState(page);
|
||||
const capture: ActionDownloadCapture = {
|
||||
pending: [],
|
||||
validations: [],
|
||||
waiters: [],
|
||||
...(opts.beforeSave ? { beforeSave: opts.beforeSave } : {}),
|
||||
};
|
||||
// One page event belongs to one action. A newer overlapping action owns
|
||||
// future events; older captures may still drain saves they already started.
|
||||
state.actionDownloadCapture = capture;
|
||||
const detach = () => {
|
||||
if (state.actionDownloadCapture === capture) {
|
||||
state.actionDownloadCapture = undefined;
|
||||
}
|
||||
for (const finish of capture.waiters.splice(0)) {
|
||||
finish();
|
||||
}
|
||||
};
|
||||
|
||||
return {
|
||||
drain: async (drainOpts = {}) => {
|
||||
const waitForEvent = async (timeoutMs: number) => {
|
||||
await new Promise<void>((resolve) => {
|
||||
const finish = () => {
|
||||
clearTimeout(timer);
|
||||
capture.waiters = capture.waiters.filter((waiter) => waiter !== finish);
|
||||
resolve();
|
||||
};
|
||||
const timer = setTimeout(finish, timeoutMs);
|
||||
capture.waiters.push(finish);
|
||||
});
|
||||
};
|
||||
const firstEventGraceMs = Math.max(0, drainOpts.firstEventGraceMs ?? 0);
|
||||
const maxWaitMs = Math.max(0, drainOpts.maxWaitMs ?? Number.POSITIVE_INFINITY);
|
||||
const deadlineAtMs = Date.now() + maxWaitMs;
|
||||
const remainingBudgetMs = () => Math.max(0, deadlineAtMs - Date.now());
|
||||
if (capture.pending.length === 0 && firstEventGraceMs > 0) {
|
||||
await waitForEvent(Math.min(firstEventGraceMs, remainingBudgetMs()));
|
||||
}
|
||||
const quietMs = Math.max(0, drainOpts.quietMs ?? 0);
|
||||
if (quietMs > 0) {
|
||||
while (capture.lastEventAtMs !== undefined) {
|
||||
const remainingQuietMs = Math.min(
|
||||
quietMs - (Date.now() - capture.lastEventAtMs),
|
||||
remainingBudgetMs(),
|
||||
);
|
||||
if (remainingQuietMs <= 0) {
|
||||
break;
|
||||
}
|
||||
await waitForEvent(remainingQuietMs);
|
||||
}
|
||||
}
|
||||
// Establish event ownership before awaiting file I/O. Slow saves must not
|
||||
// hold the action window open and absorb unrelated later downloads.
|
||||
detach();
|
||||
const pending = capture.pending.slice();
|
||||
await Promise.all(capture.validations.slice());
|
||||
const downloads = await Promise.all(pending);
|
||||
return downloads.length > 0 ? downloads : undefined;
|
||||
},
|
||||
dispose: detach,
|
||||
};
|
||||
}
|
||||
|
||||
function hasCachedPlaywrightBrowserConnection(cdpUrl: string): boolean {
|
||||
return cachedByCdpUrl.has(normalizeCdpUrl(cdpUrl));
|
||||
}
|
||||
|
|
@ -702,9 +794,28 @@ export function ensurePageState(page: Page): PageState {
|
|||
if (state.downloadWaiterDepth > 0) {
|
||||
return;
|
||||
}
|
||||
const managedSave = saveBrowserDownload(download);
|
||||
const actionCapture = state.actionDownloadCapture;
|
||||
const beforeSave = actionCapture?.beforeSave;
|
||||
const captureOptions: BrowserDownloadCaptureOptions | undefined =
|
||||
actionCapture && beforeSave
|
||||
? {
|
||||
beforeSave: (candidate) => {
|
||||
const validation = Promise.resolve().then(() => beforeSave(candidate));
|
||||
actionCapture.validations.push(validation);
|
||||
return validation;
|
||||
},
|
||||
}
|
||||
: undefined;
|
||||
const managedSave = saveBrowserDownload(download, captureOptions);
|
||||
managedSave.catch(() => {});
|
||||
download.path = async () => (await managedSave).path;
|
||||
if (actionCapture) {
|
||||
actionCapture.lastEventAtMs = Date.now();
|
||||
}
|
||||
actionCapture?.pending.push(managedSave);
|
||||
for (const finish of actionCapture?.waiters.splice(0) ?? []) {
|
||||
finish();
|
||||
}
|
||||
});
|
||||
page.on("close", () => {
|
||||
clearArmedDialogResponse(state);
|
||||
|
|
@ -1279,7 +1390,7 @@ export function isPolicyDenyNavigationError(err: unknown): boolean {
|
|||
// page we have already proven is non-compliant. This is a pure bookkeeping
|
||||
// step; it does NOT close the tab. Read-only paths can call this safely on a
|
||||
// user-owned tab without losing the user's content.
|
||||
async function quarantineBlockedTarget(opts: {
|
||||
export async function quarantineBlockedNavigationTarget(opts: {
|
||||
cdpUrl: string;
|
||||
page: Page;
|
||||
targetId?: string;
|
||||
|
|
@ -1304,7 +1415,7 @@ export async function closeBlockedNavigationTarget(opts: {
|
|||
page: Page;
|
||||
targetId?: string;
|
||||
}): Promise<void> {
|
||||
await quarantineBlockedTarget(opts);
|
||||
await quarantineBlockedNavigationTarget(opts);
|
||||
await opts.page.close().catch(() => {});
|
||||
}
|
||||
|
||||
|
|
@ -1333,7 +1444,7 @@ export async function assertPageNavigationCompletedSafely(
|
|||
});
|
||||
} catch (err) {
|
||||
if (isPolicyDenyNavigationError(err)) {
|
||||
await quarantineBlockedTarget({
|
||||
await quarantineBlockedNavigationTarget({
|
||||
cdpUrl: opts.cdpUrl,
|
||||
page: opts.page,
|
||||
targetId: opts.targetId,
|
||||
|
|
|
|||
|
|
@ -1296,4 +1296,115 @@ describe("pw-tools-core interaction navigation guard", () => {
|
|||
vi.useRealTimers();
|
||||
}
|
||||
});
|
||||
|
||||
it("returns click downloads without adding a second policy grace", async () => {
|
||||
const page = { url: vi.fn(() => "https://example.com") };
|
||||
const click = vi.fn(async () => {});
|
||||
const drain = vi.fn(async () => [
|
||||
{
|
||||
url: "https://example.com/report.pdf",
|
||||
suggestedFilename: "report.pdf",
|
||||
path: "/tmp/openclaw/downloads/report.pdf",
|
||||
},
|
||||
]);
|
||||
const dispose = vi.fn();
|
||||
getPwToolsCoreSessionMocks().beginActionDownloadCaptureOnPage.mockReturnValueOnce({
|
||||
drain,
|
||||
dispose,
|
||||
});
|
||||
setPwToolsCoreCurrentPage(page);
|
||||
setPwToolsCoreCurrentRefLocator({ click });
|
||||
|
||||
const result = await mod.executeActViaPlaywright({
|
||||
cdpUrl: "http://127.0.0.1:18792",
|
||||
targetId: "T1",
|
||||
action: { kind: "click", ref: "1" },
|
||||
ssrfPolicy: { allowPrivateNetwork: false },
|
||||
});
|
||||
|
||||
expect(result.downloads).toEqual([
|
||||
{
|
||||
url: "https://example.com/report.pdf",
|
||||
suggestedFilename: "report.pdf",
|
||||
path: "/tmp/openclaw/downloads/report.pdf",
|
||||
},
|
||||
]);
|
||||
expect(drain).toHaveBeenCalledWith({
|
||||
firstEventGraceMs: 0,
|
||||
maxWaitMs: 1_000,
|
||||
quietMs: 250,
|
||||
});
|
||||
expect(dispose).toHaveBeenCalledOnce();
|
||||
expect(getPwToolsCoreSessionMocks().beginActionDownloadCaptureOnPage).toHaveBeenCalledWith(
|
||||
page,
|
||||
{ beforeSave: expect.any(Function) },
|
||||
);
|
||||
});
|
||||
|
||||
it("quarantines the target without closing it when an action download fails policy", async () => {
|
||||
const page = { url: vi.fn(() => "https://example.com") };
|
||||
const click = vi.fn(async () => {});
|
||||
const blocked = new Error("blocked action download");
|
||||
blocked.name = "InvalidBrowserNavigationUrlError";
|
||||
const dispose = vi.fn();
|
||||
getPwToolsCoreSessionMocks().beginActionDownloadCaptureOnPage.mockReturnValueOnce({
|
||||
drain: vi.fn(async () => {
|
||||
throw blocked;
|
||||
}),
|
||||
dispose,
|
||||
});
|
||||
setPwToolsCoreCurrentPage(page);
|
||||
setPwToolsCoreCurrentRefLocator({ click });
|
||||
|
||||
await expect(
|
||||
mod.executeActViaPlaywright({
|
||||
cdpUrl: "http://127.0.0.1:18792",
|
||||
targetId: "T1",
|
||||
action: { kind: "click", ref: "1" },
|
||||
}),
|
||||
).rejects.toBe(blocked);
|
||||
|
||||
expect(getPwToolsCoreSessionMocks().quarantineBlockedNavigationTarget).toHaveBeenCalledWith({
|
||||
cdpUrl: "http://127.0.0.1:18792",
|
||||
page,
|
||||
targetId: "T1",
|
||||
});
|
||||
expect(dispose).toHaveBeenCalledOnce();
|
||||
});
|
||||
|
||||
it("captures key-triggered downloads with a bounded event grace", async () => {
|
||||
const page = {
|
||||
keyboard: { press: vi.fn(async () => {}) },
|
||||
url: vi.fn(() => "https://example.com"),
|
||||
};
|
||||
const drain = vi.fn(async () => [
|
||||
{
|
||||
url: "https://example.com/report.pdf",
|
||||
suggestedFilename: "report.pdf",
|
||||
path: "/tmp/openclaw/downloads/report.pdf",
|
||||
},
|
||||
]);
|
||||
const dispose = vi.fn();
|
||||
getPwToolsCoreSessionMocks().beginActionDownloadCaptureOnPage.mockReturnValueOnce({
|
||||
drain,
|
||||
dispose,
|
||||
});
|
||||
setPwToolsCoreCurrentPage(page);
|
||||
|
||||
const result = await mod.executeActViaPlaywright({
|
||||
cdpUrl: "http://127.0.0.1:18792",
|
||||
targetId: "T1",
|
||||
action: { kind: "press", key: "Enter" },
|
||||
});
|
||||
|
||||
expect(result.downloads).toEqual([
|
||||
expect.objectContaining({ suggestedFilename: "report.pdf" }),
|
||||
]);
|
||||
expect(drain).toHaveBeenCalledWith({
|
||||
firstEventGraceMs: 250,
|
||||
maxWaitMs: 1_000,
|
||||
quietMs: 250,
|
||||
});
|
||||
expect(dispose).toHaveBeenCalledOnce();
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -16,6 +16,7 @@ import {
|
|||
resolveActWaitTimeoutMs,
|
||||
} from "./act-policy.js";
|
||||
import type { BrowserActRequest, BrowserFormField } from "./client-actions.types.js";
|
||||
import type { BrowserDownloadResult } from "./download-types.js";
|
||||
import { normalizeBrowserEvaluateFunctionSource } from "./evaluate-source.js";
|
||||
import { DEFAULT_FILL_FIELD_TYPE } from "./form-fields.js";
|
||||
import {
|
||||
|
|
@ -25,12 +26,15 @@ import {
|
|||
import { resolveStrictExistingUploadPaths } from "./paths.js";
|
||||
import {
|
||||
assertPageNavigationCompletedSafely,
|
||||
beginActionDownloadCaptureOnPage,
|
||||
createObservedDialogAbortSignalForPage,
|
||||
ensurePageState,
|
||||
forceDisconnectPlaywrightForTarget,
|
||||
getPageForTargetId,
|
||||
isBrowserObservedDialogBlockedError,
|
||||
isPolicyDenyNavigationError,
|
||||
markObservedDialogsHandledRemotelyForPage,
|
||||
quarantineBlockedNavigationTarget,
|
||||
refLocator,
|
||||
restoreRoleRefsForTarget,
|
||||
} from "./pw-session.js";
|
||||
|
|
@ -57,6 +61,7 @@ type TargetOpts = {
|
|||
};
|
||||
|
||||
const INTERACTION_NAVIGATION_GRACE_MS = 250;
|
||||
const ACT_DOWNLOAD_MAX_DRAIN_MS = 1_000;
|
||||
|
||||
type NavigationObservablePage = Pick<Page, "url"> & {
|
||||
mainFrame?: () => Frame;
|
||||
|
|
@ -1691,6 +1696,30 @@ async function executeSingleAction(
|
|||
return undefined;
|
||||
}
|
||||
|
||||
function actionNeedsStandaloneDownloadGrace(
|
||||
action: BrowserActRequest,
|
||||
ssrfPolicy?: SsrFPolicy,
|
||||
): boolean {
|
||||
switch (action.kind) {
|
||||
case "close":
|
||||
case "resize":
|
||||
case "wait":
|
||||
return false;
|
||||
case "hover":
|
||||
case "scrollIntoView":
|
||||
case "drag":
|
||||
return true;
|
||||
case "batch":
|
||||
return action.actions.some((nested) =>
|
||||
actionNeedsStandaloneDownloadGrace(nested, ssrfPolicy),
|
||||
);
|
||||
default:
|
||||
// Navigation-aware interactions already hold a 250 ms event window when
|
||||
// policy is active. Policy-free internal callers need that window here.
|
||||
return !ssrfPolicy;
|
||||
}
|
||||
}
|
||||
|
||||
/** Executes one high-level browser act request with bounded recursive actions. */
|
||||
export async function executeActViaPlaywright(opts: {
|
||||
cdpUrl: string;
|
||||
|
|
@ -1704,12 +1733,35 @@ export async function executeActViaPlaywright(opts: {
|
|||
results?: Array<{ ok: boolean; error?: string }>;
|
||||
blockedByDialog?: boolean;
|
||||
browserState?: unknown;
|
||||
downloads?: BrowserDownloadResult[];
|
||||
}> {
|
||||
const page = await getPageForTargetId({
|
||||
cdpUrl: opts.cdpUrl,
|
||||
targetId: opts.targetId,
|
||||
ssrfPolicy: opts.ssrfPolicy,
|
||||
});
|
||||
// Any DOM action can synchronously trigger a download. Capturing all actions
|
||||
// keeps reporting and final-URL policy aligned with the actual file write.
|
||||
const downloadCapture = beginActionDownloadCaptureOnPage(page, {
|
||||
beforeSave: async (download) => {
|
||||
if (!download.url) {
|
||||
throw new Error("Action download URL is unavailable");
|
||||
}
|
||||
await assertBrowserNavigationResultAllowed({
|
||||
url: download.url,
|
||||
...withBrowserNavigationPolicy(opts.ssrfPolicy),
|
||||
});
|
||||
},
|
||||
});
|
||||
const downloadGraceMs = actionNeedsStandaloneDownloadGrace(opts.action, opts.ssrfPolicy)
|
||||
? INTERACTION_NAVIGATION_GRACE_MS
|
||||
: 0;
|
||||
const drainDownloads = async () =>
|
||||
await downloadCapture.drain({
|
||||
firstEventGraceMs: downloadGraceMs,
|
||||
maxWaitMs: ACT_DOWNLOAD_MAX_DRAIN_MS,
|
||||
quietMs: INTERACTION_NAVIGATION_GRACE_MS,
|
||||
});
|
||||
const dialogAbort = createObservedDialogAbortSignalForPage({
|
||||
page,
|
||||
parentSignal: opts.signal,
|
||||
|
|
@ -1725,7 +1777,11 @@ export async function executeActViaPlaywright(opts: {
|
|||
evaluateEnabled: opts.evaluateEnabled,
|
||||
signal: dialogAbort.signal,
|
||||
});
|
||||
return { results: batch.results };
|
||||
const newDownloads = await drainDownloads();
|
||||
return {
|
||||
results: batch.results,
|
||||
...(newDownloads ? { downloads: newDownloads } : {}),
|
||||
};
|
||||
}
|
||||
const result = await executeSingleAction(
|
||||
opts.action,
|
||||
|
|
@ -1736,16 +1792,33 @@ export async function executeActViaPlaywright(opts: {
|
|||
0,
|
||||
dialogAbort.signal,
|
||||
);
|
||||
const newDownloads = await drainDownloads();
|
||||
if (opts.action.kind === "evaluate") {
|
||||
return { result };
|
||||
return { result, ...(newDownloads ? { downloads: newDownloads } : {}) };
|
||||
}
|
||||
return {};
|
||||
return newDownloads ? { downloads: newDownloads } : {};
|
||||
} catch (err) {
|
||||
if (isBrowserObservedDialogBlockedError(err)) {
|
||||
return { blockedByDialog: true, browserState: err.browserState };
|
||||
let failure = err;
|
||||
try {
|
||||
await drainDownloads();
|
||||
} catch (downloadErr) {
|
||||
// A download policy/save failure is the action's network-to-file result;
|
||||
// preserve it even when the initiating interaction also failed.
|
||||
failure = downloadErr;
|
||||
}
|
||||
throw err;
|
||||
if (isBrowserObservedDialogBlockedError(failure)) {
|
||||
return { blockedByDialog: true, browserState: failure.browserState };
|
||||
}
|
||||
if (isPolicyDenyNavigationError(failure)) {
|
||||
await quarantineBlockedNavigationTarget({
|
||||
cdpUrl: opts.cdpUrl,
|
||||
page,
|
||||
targetId: opts.targetId,
|
||||
});
|
||||
}
|
||||
throw failure;
|
||||
} finally {
|
||||
downloadCapture.dispose();
|
||||
dialogAbort.cleanup();
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -34,6 +34,10 @@ let pageState: {
|
|||
|
||||
const sessionMocks = vi.hoisted(() => ({
|
||||
assertPageNavigationCompletedSafely: vi.fn(async () => {}),
|
||||
beginActionDownloadCaptureOnPage: vi.fn(() => ({
|
||||
drain: vi.fn(async (): Promise<HarnessManagedDownload[] | undefined> => undefined),
|
||||
dispose: vi.fn(() => {}),
|
||||
})),
|
||||
closeBlockedNavigationTarget: vi.fn(async () => {}),
|
||||
getPageForTargetId: vi.fn(async () => {
|
||||
if (!currentPage) {
|
||||
|
|
@ -70,6 +74,7 @@ const sessionMocks = vi.hoisted(() => ({
|
|||
}
|
||||
return err.name === "SsrFBlockedError" || err.name === "InvalidBrowserNavigationUrlError";
|
||||
}),
|
||||
quarantineBlockedNavigationTarget: vi.fn(async () => {}),
|
||||
restoreRoleRefsForTarget: vi.fn(() => {}),
|
||||
respondToObservedDialogOnPage: vi.fn(async () => {
|
||||
throw new Error("No dialog is pending.");
|
||||
|
|
|
|||
|
|
@ -673,21 +673,29 @@ export function registerBrowserAgentActRoutes(
|
|||
browserState: result.browserState,
|
||||
});
|
||||
}
|
||||
const downloads = result.downloads;
|
||||
switch (action.kind) {
|
||||
case "batch":
|
||||
return await jsonOk(
|
||||
{ results: result.results ?? [] },
|
||||
{ results: result.results ?? [], ...(downloads ? { downloads } : {}) },
|
||||
{ resolveCurrentTarget: true },
|
||||
);
|
||||
case "evaluate":
|
||||
return await jsonOk({ result: result.result }, { resolveCurrentTarget: true });
|
||||
return await jsonOk(
|
||||
{ result: result.result, ...(downloads ? { downloads } : {}) },
|
||||
{ resolveCurrentTarget: true },
|
||||
);
|
||||
case "click":
|
||||
case "clickCoords":
|
||||
return await jsonOk(undefined, { resolveCurrentTarget: true });
|
||||
return await jsonOk(downloads ? { downloads } : undefined, {
|
||||
resolveCurrentTarget: true,
|
||||
});
|
||||
case "resize":
|
||||
return await jsonOk();
|
||||
return await jsonOk(downloads ? { downloads } : undefined);
|
||||
default:
|
||||
return await jsonOk(undefined, { resolveCurrentTarget: true });
|
||||
return await jsonOk(downloads ? { downloads } : undefined, {
|
||||
resolveCurrentTarget: true,
|
||||
});
|
||||
}
|
||||
},
|
||||
});
|
||||
|
|
|
|||
|
|
@ -313,6 +313,40 @@ describe("browser control server", () => {
|
|||
slowTimeoutMs,
|
||||
);
|
||||
|
||||
it(
|
||||
"returns action download metadata from /act responses",
|
||||
async () => {
|
||||
const base = await startServerAndBase();
|
||||
pwMocks.executeActViaPlaywright.mockResolvedValueOnce({
|
||||
downloads: [
|
||||
{
|
||||
url: "https://example.com/report.pdf",
|
||||
suggestedFilename: "report.pdf",
|
||||
path: "/tmp/openclaw/downloads/report.pdf",
|
||||
},
|
||||
],
|
||||
});
|
||||
|
||||
const response = await postJson<{
|
||||
ok: boolean;
|
||||
downloads?: Array<{ url?: string; suggestedFilename?: string; path?: string }>;
|
||||
}>(`${base}/act`, {
|
||||
kind: "click",
|
||||
ref: "5",
|
||||
});
|
||||
|
||||
expect(response.ok).toBe(true);
|
||||
expect(response.downloads).toEqual([
|
||||
{
|
||||
url: "https://example.com/report.pdf",
|
||||
suggestedFilename: "report.pdf",
|
||||
path: "/tmp/openclaw/downloads/report.pdf",
|
||||
},
|
||||
]);
|
||||
},
|
||||
slowTimeoutMs,
|
||||
);
|
||||
|
||||
it(
|
||||
"returns ACT_SELECTOR_UNSUPPORTED for selector on unsupported action kinds",
|
||||
async () => {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue