fix(browser): manage isolated downloads

Co-authored-by: Pearce Kieser <5055971+Pearcekieser@users.noreply.github.com>
This commit is contained in:
Peter Steinberger
2026-04-25 10:03:08 +01:00
parent b756dfcb2b
commit 41ed7fa535
10 changed files with 243 additions and 9 deletions

View File

@@ -67,6 +67,7 @@ export function isProfileDecorated(
userDataDir: string,
desiredName: string,
desiredColorHex: string,
desiredDownloadDir?: string,
): boolean {
const desiredColorInt = parseHexRgbToSignedArgbInt(desiredColorHex);
@@ -80,12 +81,20 @@ export function isProfileDecorated(
const prefs = safeReadJson(preferencesPath);
const browserTheme = readNestedRecord(prefs?.browser, "theme");
const autogeneratedTheme = readNestedRecord(prefs?.autogenerated, "theme");
const download = readNestedRecord(prefs, "download");
const savefile = readNestedRecord(prefs, "savefile");
const nameOk = typeof info?.name === "string" ? info.name === desiredName : true;
const downloadOk = desiredDownloadDir
? download?.default_directory === desiredDownloadDir &&
download.prompt_for_download === false &&
download.directory_upgrade === true &&
savefile?.default_directory === desiredDownloadDir
: true;
if (desiredColorInt == null) {
// If the user provided a non-#RRGGBB value, we can only do best-effort.
return nameOk;
return nameOk && downloadOk;
}
const localSeedOk =
@@ -98,7 +107,7 @@ export function isProfileDecorated(
browserTheme.user_color2 === desiredColorInt) ||
(typeof autogeneratedTheme?.color === "number" && autogeneratedTheme.color === desiredColorInt);
return nameOk && localSeedOk && prefOk;
return nameOk && localSeedOk && prefOk && downloadOk;
}
/**
@@ -107,7 +116,7 @@ export function isProfileDecorated(
*/
export function decorateOpenClawProfile(
userDataDir: string,
opts?: { name?: string; color?: string },
opts?: { name?: string; color?: string; downloadDir?: string },
) {
const desiredName = opts?.name ?? DEFAULT_OPENCLAW_BROWSER_PROFILE_NAME;
const desiredColor = (opts?.color ?? DEFAULT_OPENCLAW_BROWSER_COLOR).toUpperCase();
@@ -159,6 +168,12 @@ export function decorateOpenClawProfile(
// User-selected browser theme color (pref name: browser.theme.user_color2).
setDeep(prefs, ["browser", "theme", "user_color2"], desiredColorInt);
}
if (opts?.downloadDir) {
setDeep(prefs, ["download", "default_directory"], opts.downloadDir);
setDeep(prefs, ["download", "prompt_for_download"], false);
setDeep(prefs, ["download", "directory_upgrade"], true);
setDeep(prefs, ["savefile", "default_directory"], opts.downloadDir);
}
safeWriteJson(preferencesPath, prefs);
try {

View File

@@ -21,6 +21,7 @@ import {
formatChromeCdpDiagnostic,
buildOpenClawChromeLaunchArgs,
getChromeWebSocketUrl,
isProfileDecorated,
isChromeCdpReady,
isChromeReachable,
resolveBrowserExecutableForPlatform,
@@ -31,6 +32,7 @@ import {
DEFAULT_OPENCLAW_BROWSER_PROFILE_NAME,
} from "./constants.js";
import { BrowserCdpEndpointBlockedError } from "./errors.js";
import { DEFAULT_DOWNLOAD_DIR } from "./paths.js";
type StopChromeTarget = Parameters<typeof stopOpenClawChrome>[0];
@@ -161,6 +163,8 @@ describe("browser chrome profile decoration", () => {
expect(theme.user_color2).toBe(expectedSignedArgb);
expect(autogeneratedTheme.color).toBe(expectedSignedArgb);
expect(prefs.download).toBeUndefined();
expect(prefs.savefile).toBeUndefined();
const marker = await fsp.readFile(
path.join(userDataDir, ".openclaw-profile-decorated"),
@@ -169,6 +173,45 @@ describe("browser chrome profile decoration", () => {
expect(marker.trim()).toMatch(/^\d+$/);
});
it("writes managed download prefs when a download dir is provided", async () => {
const userDataDir = await createUserDataDir();
decorateOpenClawProfile(userDataDir, {
color: DEFAULT_OPENCLAW_BROWSER_COLOR,
downloadDir: DEFAULT_DOWNLOAD_DIR,
});
const prefs = await readJson(path.join(userDataDir, "Default", "Preferences"));
const download = prefs.download as Record<string, unknown>;
const savefile = prefs.savefile as Record<string, unknown>;
expect(download.default_directory).toBe(DEFAULT_DOWNLOAD_DIR);
expect(download.prompt_for_download).toBe(false);
expect(download.directory_upgrade).toBe(true);
expect(savefile.default_directory).toBe(DEFAULT_DOWNLOAD_DIR);
expect(
isProfileDecorated(
userDataDir,
DEFAULT_OPENCLAW_BROWSER_PROFILE_NAME,
DEFAULT_OPENCLAW_BROWSER_COLOR,
DEFAULT_DOWNLOAD_DIR,
),
).toBe(true);
});
it("treats missing managed download prefs as undecorated when required", async () => {
const userDataDir = await createUserDataDir();
decorateOpenClawProfile(userDataDir, { color: DEFAULT_OPENCLAW_BROWSER_COLOR });
expect(
isProfileDecorated(
userDataDir,
DEFAULT_OPENCLAW_BROWSER_PROFILE_NAME,
DEFAULT_OPENCLAW_BROWSER_COLOR,
DEFAULT_DOWNLOAD_DIR,
),
).toBe(false);
});
it("best-effort writes name when color is invalid", async () => {
const userDataDir = await createUserDataDir();
decorateOpenClawProfile(userDataDir, { color: "lobster-orange" });

View File

@@ -51,6 +51,7 @@ import {
DEFAULT_OPENCLAW_BROWSER_COLOR,
DEFAULT_OPENCLAW_BROWSER_PROFILE_NAME,
} from "./constants.js";
import { DEFAULT_DOWNLOAD_DIR } from "./paths.js";
const log = createSubsystemLogger("browser").child("chrome");
const CHROME_SINGLETON_LOCK_PATHS = [
@@ -393,11 +394,13 @@ export async function launchOpenClawChrome(
const userDataDir = resolveOpenClawUserDataDir(profile.name);
fs.mkdirSync(userDataDir, { recursive: true });
fs.mkdirSync(DEFAULT_DOWNLOAD_DIR, { recursive: true });
const needsDecorate = !isProfileDecorated(
userDataDir,
profile.name,
(profile.color ?? DEFAULT_OPENCLAW_BROWSER_COLOR).toUpperCase(),
DEFAULT_DOWNLOAD_DIR,
);
// First launch to create preference files if missing, then decorate and relaunch.
@@ -460,6 +463,7 @@ export async function launchOpenClawChrome(
decorateOpenClawProfile(userDataDir, {
name: profile.name,
color: profile.color,
downloadDir: DEFAULT_DOWNLOAD_DIR,
});
log.info(`🦞 openclaw browser profile decorated (${profile.color})`);
} catch (err) {

View File

@@ -1,5 +1,8 @@
import fs from "node:fs/promises";
import path from "node:path";
import type { Page } from "playwright-core";
import { describe, expect, it, vi } from "vitest";
import { afterEach, describe, expect, it, vi } from "vitest";
import { DEFAULT_DOWNLOAD_DIR } from "./paths.js";
import {
ensurePageState,
refLocator,
@@ -8,6 +11,16 @@ import {
} from "./pw-session.js";
import { BROWSER_REF_MARKER_ATTRIBUTE } from "./pw-session.page-cdp.js";
type MutableDownload = {
suggestedFilename: () => string;
saveAs: ReturnType<typeof vi.fn>;
path?: () => Promise<string>;
};
afterEach(() => {
vi.restoreAllMocks();
});
function fakePage(): {
page: Page;
handlers: Map<string, Array<(...args: unknown[]) => void>>;
@@ -123,6 +136,80 @@ describe("pw-session role refs cache", () => {
});
describe("pw-session ensurePageState", () => {
it("stores unmanaged downloads under unique managed paths", async () => {
const { page, handlers } = fakePage();
const mkdirSpy = vi.spyOn(fs, "mkdir").mockResolvedValue(undefined);
ensurePageState(page);
const saveAsA = vi.fn(async () => {});
const saveAsB = vi.fn(async () => {});
const downloadA: MutableDownload = {
suggestedFilename: () => "report.pdf",
saveAs: saveAsA,
};
const downloadB: MutableDownload = {
suggestedFilename: () => "report.pdf",
saveAs: saveAsB,
};
handlers.get("download")?.[0]?.(downloadA);
handlers.get("download")?.[0]?.(downloadB);
const managedPathA = await downloadA.path?.();
const managedPathB = await downloadB.path?.();
expect(managedPathA).not.toBe(managedPathB);
expect(path.dirname(managedPathA ?? "")).toBe(DEFAULT_DOWNLOAD_DIR);
expect(path.dirname(managedPathB ?? "")).toBe(DEFAULT_DOWNLOAD_DIR);
expect(path.basename(managedPathA ?? "")).toMatch(/-report\.pdf$/);
expect(path.basename(managedPathB ?? "")).toMatch(/-report\.pdf$/);
expect(saveAsA).toHaveBeenCalledWith(managedPathA);
expect(saveAsB).toHaveBeenCalledWith(managedPathB);
expect(mkdirSpy).toHaveBeenCalledWith(DEFAULT_DOWNLOAD_DIR, { recursive: true });
});
it("suppresses unmanaged download save rejections until path is awaited", async () => {
const { page, handlers } = fakePage();
vi.spyOn(fs, "mkdir").mockResolvedValue(undefined);
ensurePageState(page);
const unhandled: unknown[] = [];
const onUnhandled = (reason: unknown) => unhandled.push(reason);
process.on("unhandledRejection", onUnhandled);
const err = new Error("save failed");
const download: MutableDownload = {
suggestedFilename: () => "report.pdf",
saveAs: vi.fn(async () => {
throw err;
}),
};
try {
handlers.get("download")?.[0]?.(download);
await new Promise((resolve) => setImmediate(resolve));
expect(unhandled).toEqual([]);
await expect(download.path?.()).rejects.toThrow("save failed");
} finally {
process.off("unhandledRejection", onUnhandled);
}
});
it("leaves unmanaged download handling to explicit waiters while armed", () => {
const { page, handlers } = fakePage();
const state = ensurePageState(page);
state.downloadWaiterDepth = 1;
const download = {
suggestedFilename: () => "report.pdf",
saveAs: vi.fn(async () => {}),
};
handlers.get("download")?.[0]?.(download);
expect(download).not.toHaveProperty("path");
expect(download.saveAs).not.toHaveBeenCalled();
});
it("tracks page errors and network requests (best-effort)", () => {
const { page, handlers } = fakePage();
const state = ensurePageState(page);

View File

@@ -1,3 +1,6 @@
import crypto from "node:crypto";
import fs from "node:fs/promises";
import path from "node:path";
import { normalizeOptionalString } from "openclaw/plugin-sdk/text-runtime";
import type {
Browser,
@@ -31,7 +34,9 @@ import {
InvalidBrowserNavigationUrlError,
withBrowserNavigationPolicy,
} from "./navigation-guard.js";
import { DEFAULT_DOWNLOAD_DIR } from "./paths.js";
import { BROWSER_REF_MARKER_ATTRIBUTE, withPageScopedCdpClient } from "./pw-session.page-cdp.js";
import { sanitizeUntrustedFileName } from "./safe-filename.js";
export type BrowserConsoleMessage = {
type: string;
@@ -79,6 +84,7 @@ type PageState = {
armIdUpload: number;
armIdDialog: number;
armIdDownload: number;
downloadWaiterDepth: number;
/**
* Role-based refs from the last role snapshot (e.g. e1/e2).
* Mode "role" refs are generated from ariaSnapshot and resolved via getByRole.
@@ -123,6 +129,12 @@ function normalizeCdpUrl(raw: string) {
return raw.replace(/\/$/, "");
}
function buildManagedDownloadPath(fileName: string): string {
const id = crypto.randomUUID();
const safeName = sanitizeUntrustedFileName(fileName, "download.bin");
return path.join(DEFAULT_DOWNLOAD_DIR, `${id}-${safeName}`);
}
function hasCachedPlaywrightBrowserConnection(cdpUrl: string): boolean {
return cachedByCdpUrl.has(normalizeCdpUrl(cdpUrl));
}
@@ -334,6 +346,7 @@ export function ensurePageState(page: Page): PageState {
armIdUpload: 0,
armIdDialog: 0,
armIdDownload: 0,
downloadWaiterDepth: 0,
};
pageStates.set(page, state);
@@ -402,6 +415,30 @@ export function ensurePageState(page: Page): PageState {
rec.failureText = req.failure()?.errorText;
rec.ok = false;
});
page.on(
"download",
(download: {
suggestedFilename?: () => string;
saveAs?: (outPath: string) => Promise<void>;
path?: () => Promise<string>;
}) => {
if (state.downloadWaiterDepth > 0) {
return;
}
const suggested = sanitizeUntrustedFileName(
download.suggestedFilename?.() || "download.bin",
"download.bin",
);
const managedPath = buildManagedDownloadPath(suggested);
const managedSave = (async () => {
await fs.mkdir(DEFAULT_DOWNLOAD_DIR, { recursive: true });
await download.saveAs?.(managedPath);
return managedPath;
})();
managedSave.catch(() => {});
download.path = async () => await managedSave;
},
);
page.on("close", () => {
pageStates.delete(page);
observedPages.delete(page);

View File

@@ -28,11 +28,18 @@ function buildTempDownloadPath(fileName: string): string {
}
function createPageDownloadWaiter(page: Page, timeoutMs: number) {
const state = ensurePageState(page);
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);
}

View File

@@ -7,11 +7,13 @@ let pageState: {
armIdUpload: number;
armIdDialog: number;
armIdDownload: number;
downloadWaiterDepth: number;
} = {
console: [],
armIdUpload: 0,
armIdDialog: 0,
armIdDownload: 0,
downloadWaiterDepth: 0,
};
const sessionMocks = vi.hoisted(() => ({
@@ -81,6 +83,7 @@ export function installPwToolsCoreTestHooks() {
armIdUpload: 0,
armIdDialog: 0,
armIdDownload: 0,
downloadWaiterDepth: 0,
};
for (const fn of Object.values(sessionMocks)) {

View File

@@ -102,21 +102,28 @@ describe("pw-tools-core", () => {
}
function createDownloadEventHarness() {
let downloadHandler: ((download: unknown) => void) | undefined;
const downloadHandlers = new Set<(download: unknown) => void>();
const on = vi.fn((event: string, handler: (download: unknown) => void) => {
if (event === "download") {
downloadHandler = handler;
downloadHandlers.add(handler);
}
});
const off = vi.fn((event: string, handler: (download: unknown) => void) => {
if (event === "download") {
downloadHandlers.delete(handler);
}
});
const off = vi.fn();
setPwToolsCoreCurrentPage({ on, off });
return {
trigger: (download: unknown) => {
downloadHandler?.(download);
for (const handler of downloadHandlers) {
handler(download);
}
},
expectArmed: () => {
expect(downloadHandler).toBeDefined();
expect(downloadHandlers.size).toBeGreaterThan(0);
},
activeHandlerCount: () => downloadHandlers.size,
};
}
@@ -169,6 +176,31 @@ describe("pw-tools-core", () => {
await expect(fs.realpath(res.path)).resolves.toBe(await fs.realpath(targetPath));
});
});
it("marks explicit download waiters as owning the next download until cleanup", async () => {
const harness = createDownloadEventHarness();
const state = sessionMocks.ensurePageState();
expect(state.downloadWaiterDepth).toBe(0);
const p = mod.waitForDownloadViaPlaywright({
cdpUrl: "http://127.0.0.1:18792",
targetId: "T1",
timeoutMs: 1000,
});
await Promise.resolve();
harness.expectArmed();
expect(state.downloadWaiterDepth).toBe(1);
harness.trigger({
url: () => "https://example.com/file.bin",
suggestedFilename: () => "file.bin",
saveAs: vi.fn(async () => {}),
});
await p;
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();