mirror of
https://github.com/openclaw/openclaw.git
synced 2026-03-14 11:30:41 +00:00
fix: harden browser existing-session flows
This commit is contained in:
@@ -398,6 +398,10 @@ Notes:
|
||||
session only.
|
||||
- OpenClaw uses the official Chrome DevTools MCP `--autoConnect` flow here, not
|
||||
the legacy default-profile remote debugging port workflow.
|
||||
- Existing-session screenshots support page captures and `--ref` element
|
||||
captures from snapshots, but not CSS `--element` selectors.
|
||||
- Existing-session `wait --url` supports exact, substring, and glob patterns
|
||||
like other browser drivers. `wait --load networkidle` is not supported yet.
|
||||
- Some features still require the extension relay or managed browser path, such
|
||||
as PDF export and download interception.
|
||||
- Leave the relay loopback-only by default. If the relay must be reachable from a different network namespace (for example Gateway in WSL2, Chrome on Windows), set `browser.relayBindHost` to an explicit bind address such as `0.0.0.0` while keeping the surrounding network private and authenticated.
|
||||
|
||||
@@ -129,4 +129,64 @@ describe("chrome MCP page parsing", () => {
|
||||
|
||||
expect(result).toBe(123);
|
||||
});
|
||||
|
||||
it("surfaces MCP tool errors instead of JSON parse noise", async () => {
|
||||
const factory: ChromeMcpSessionFactory = async () => {
|
||||
const session = createFakeSession();
|
||||
const callTool = vi.fn(async ({ name }: ToolCall) => {
|
||||
if (name === "evaluate_script") {
|
||||
return {
|
||||
content: [
|
||||
{
|
||||
type: "text",
|
||||
text: "Cannot read properties of null (reading 'value')",
|
||||
},
|
||||
],
|
||||
isError: true,
|
||||
};
|
||||
}
|
||||
throw new Error(`unexpected tool ${name}`);
|
||||
});
|
||||
session.client.callTool = callTool as typeof session.client.callTool;
|
||||
return session;
|
||||
};
|
||||
setChromeMcpSessionFactoryForTest(factory);
|
||||
|
||||
await expect(
|
||||
evaluateChromeMcpScript({
|
||||
profileName: "chrome-live",
|
||||
targetId: "1",
|
||||
fn: "() => document.getElementById('missing').value",
|
||||
}),
|
||||
).rejects.toThrow(/Cannot read properties of null/);
|
||||
});
|
||||
|
||||
it("reuses a single pending session for concurrent requests", async () => {
|
||||
let factoryCalls = 0;
|
||||
let releaseFactory!: () => void;
|
||||
const factoryGate = new Promise<void>((resolve) => {
|
||||
releaseFactory = resolve;
|
||||
});
|
||||
|
||||
const factory: ChromeMcpSessionFactory = async () => {
|
||||
factoryCalls += 1;
|
||||
await factoryGate;
|
||||
return createFakeSession();
|
||||
};
|
||||
setChromeMcpSessionFactoryForTest(factory);
|
||||
|
||||
const tabsPromise = listChromeMcpTabs("chrome-live");
|
||||
const evalPromise = evaluateChromeMcpScript({
|
||||
profileName: "chrome-live",
|
||||
targetId: "1",
|
||||
fn: "() => 123",
|
||||
});
|
||||
|
||||
releaseFactory();
|
||||
const [tabs, result] = await Promise.all([tabsPromise, evalPromise]);
|
||||
|
||||
expect(factoryCalls).toBe(1);
|
||||
expect(tabs).toHaveLength(2);
|
||||
expect(result).toBe(123);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -39,6 +39,7 @@ const DEFAULT_CHROME_MCP_ARGS = [
|
||||
];
|
||||
|
||||
const sessions = new Map<string, ChromeMcpSession>();
|
||||
const pendingSessions = new Map<string, Promise<ChromeMcpSession>>();
|
||||
let sessionFactory: ChromeMcpSessionFactory | null = null;
|
||||
|
||||
function asRecord(value: unknown): Record<string, unknown> | null {
|
||||
@@ -144,6 +145,11 @@ function extractMessageText(result: ChromeMcpToolResult): string {
|
||||
return blocks.find((block) => block.trim()) ?? "";
|
||||
}
|
||||
|
||||
function extractToolErrorMessage(result: ChromeMcpToolResult, name: string): string {
|
||||
const message = extractMessageText(result).trim();
|
||||
return message || `Chrome MCP tool "${name}" failed.`;
|
||||
}
|
||||
|
||||
function extractJsonMessage(result: ChromeMcpToolResult): unknown {
|
||||
const candidates = [extractMessageText(result), ...extractTextContent(result)].filter((text) =>
|
||||
text.trim(),
|
||||
@@ -207,8 +213,22 @@ async function getSession(profileName: string): Promise<ChromeMcpSession> {
|
||||
session = undefined;
|
||||
}
|
||||
if (!session) {
|
||||
session = await (sessionFactory ?? createRealSession)(profileName);
|
||||
sessions.set(profileName, session);
|
||||
let pending = pendingSessions.get(profileName);
|
||||
if (!pending) {
|
||||
pending = (async () => {
|
||||
const created = await (sessionFactory ?? createRealSession)(profileName);
|
||||
sessions.set(profileName, created);
|
||||
return created;
|
||||
})();
|
||||
pendingSessions.set(profileName, pending);
|
||||
}
|
||||
try {
|
||||
session = await pending;
|
||||
} finally {
|
||||
if (pendingSessions.get(profileName) === pending) {
|
||||
pendingSessions.delete(profileName);
|
||||
}
|
||||
}
|
||||
}
|
||||
try {
|
||||
await session.ready;
|
||||
@@ -229,10 +249,14 @@ async function callTool(
|
||||
): Promise<ChromeMcpToolResult> {
|
||||
const session = await getSession(profileName);
|
||||
try {
|
||||
return (await session.client.callTool({
|
||||
const result = (await session.client.callTool({
|
||||
name,
|
||||
arguments: args,
|
||||
})) as ChromeMcpToolResult;
|
||||
if (result.isError) {
|
||||
throw new Error(extractToolErrorMessage(result, name));
|
||||
}
|
||||
return result;
|
||||
} catch (err) {
|
||||
sessions.delete(profileName);
|
||||
await session.client.close().catch(() => {});
|
||||
@@ -268,6 +292,7 @@ export function getChromeMcpPid(profileName: string): number | null {
|
||||
}
|
||||
|
||||
export async function closeChromeMcpSession(profileName: string): Promise<boolean> {
|
||||
pendingSessions.delete(profileName);
|
||||
const session = sessions.get(profileName);
|
||||
if (!session) {
|
||||
return false;
|
||||
@@ -508,5 +533,6 @@ export function setChromeMcpSessionFactoryForTest(factory: ChromeMcpSessionFacto
|
||||
|
||||
export async function resetChromeMcpSessionsForTest(): Promise<void> {
|
||||
sessionFactory = null;
|
||||
pendingSessions.clear();
|
||||
await stopAllChromeMcpSessions();
|
||||
}
|
||||
|
||||
@@ -1,22 +1,7 @@
|
||||
import { formatCliCommand } from "../cli/command-format.js";
|
||||
import { ensurePageState, getPageForTargetId } from "./pw-session.js";
|
||||
import { normalizeTimeoutMs } from "./pw-tools-core.shared.js";
|
||||
|
||||
function matchUrlPattern(pattern: string, url: string): boolean {
|
||||
const p = pattern.trim();
|
||||
if (!p) {
|
||||
return false;
|
||||
}
|
||||
if (p === url) {
|
||||
return true;
|
||||
}
|
||||
if (p.includes("*")) {
|
||||
const escaped = p.replace(/[|\\{}()[\]^$+?.]/g, "\\$&");
|
||||
const regex = new RegExp(`^${escaped.replace(/\*\*/g, ".*").replace(/\*/g, ".*")}$`);
|
||||
return regex.test(url);
|
||||
}
|
||||
return url.includes(p);
|
||||
}
|
||||
import { matchBrowserUrlPattern } from "./url-pattern.js";
|
||||
|
||||
export async function responseBodyViaPlaywright(opts: {
|
||||
cdpUrl: string;
|
||||
@@ -65,7 +50,7 @@ export async function responseBodyViaPlaywright(opts: {
|
||||
}
|
||||
const r = resp as { url?: () => string };
|
||||
const u = r.url?.() || "";
|
||||
if (!matchUrlPattern(pattern, u)) {
|
||||
if (!matchBrowserUrlPattern(pattern, u)) {
|
||||
return;
|
||||
}
|
||||
done = true;
|
||||
|
||||
@@ -291,6 +291,6 @@ describe("pw-tools-core", () => {
|
||||
targetId: "T1",
|
||||
ref: " ",
|
||||
}),
|
||||
).rejects.toThrow(/ref is required/i);
|
||||
).rejects.toThrow(/ref or selector is required/i);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -12,6 +12,7 @@ import {
|
||||
import type { BrowserActRequest, BrowserFormField } from "../client-actions-core.js";
|
||||
import { normalizeBrowserFormField } from "../form-fields.js";
|
||||
import type { BrowserRouteContext } from "../server-context.js";
|
||||
import { matchBrowserUrlPattern } from "../url-pattern.js";
|
||||
import { registerBrowserAgentActDownloadRoutes } from "./agent.act.download.js";
|
||||
import { registerBrowserAgentActHookRoutes } from "./agent.act.hooks.js";
|
||||
import {
|
||||
@@ -47,7 +48,6 @@ function buildExistingSessionWaitPredicate(params: {
|
||||
text?: string;
|
||||
textGone?: string;
|
||||
selector?: string;
|
||||
url?: string;
|
||||
loadState?: "load" | "domcontentloaded" | "networkidle";
|
||||
fn?: string;
|
||||
}): string | null {
|
||||
@@ -61,9 +61,6 @@ function buildExistingSessionWaitPredicate(params: {
|
||||
if (params.selector) {
|
||||
checks.push(`Boolean(document.querySelector(${JSON.stringify(params.selector)}))`);
|
||||
}
|
||||
if (params.url) {
|
||||
checks.push(`window.location.href === ${JSON.stringify(params.url)}`);
|
||||
}
|
||||
if (params.loadState === "domcontentloaded") {
|
||||
checks.push(`document.readyState === "interactive" || document.readyState === "complete"`);
|
||||
} else if (params.loadState === "load") {
|
||||
@@ -94,17 +91,30 @@ async function waitForExistingSessionCondition(params: {
|
||||
await sleep(params.timeMs);
|
||||
}
|
||||
const predicate = buildExistingSessionWaitPredicate(params);
|
||||
if (!predicate) {
|
||||
if (!predicate && !params.url) {
|
||||
return;
|
||||
}
|
||||
const timeoutMs = Math.max(250, params.timeoutMs ?? 10_000);
|
||||
const deadline = Date.now() + timeoutMs;
|
||||
while (Date.now() < deadline) {
|
||||
const ready = await evaluateChromeMcpScript({
|
||||
profileName: params.profileName,
|
||||
targetId: params.targetId,
|
||||
fn: `async () => ${predicate}`,
|
||||
});
|
||||
let ready = true;
|
||||
if (predicate) {
|
||||
ready = Boolean(
|
||||
await evaluateChromeMcpScript({
|
||||
profileName: params.profileName,
|
||||
targetId: params.targetId,
|
||||
fn: `async () => ${predicate}`,
|
||||
}),
|
||||
);
|
||||
}
|
||||
if (ready && params.url) {
|
||||
const currentUrl = await evaluateChromeMcpScript({
|
||||
profileName: params.profileName,
|
||||
targetId: params.targetId,
|
||||
fn: "() => window.location.href",
|
||||
});
|
||||
ready = typeof currentUrl === "string" && matchBrowserUrlPattern(params.url, currentUrl);
|
||||
}
|
||||
if (ready) {
|
||||
return;
|
||||
}
|
||||
|
||||
@@ -195,4 +195,37 @@ describe("existing-session browser routes", () => {
|
||||
});
|
||||
expect(chromeMcpMocks.evaluateChromeMcpScript).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("supports glob URL waits for existing-session profiles", async () => {
|
||||
chromeMcpMocks.evaluateChromeMcpScript.mockReset();
|
||||
chromeMcpMocks.evaluateChromeMcpScript.mockImplementation(
|
||||
async ({ fn }: { fn: string }) =>
|
||||
(fn === "() => window.location.href" ? "https://example.com/" : true) as never,
|
||||
);
|
||||
|
||||
const { app, postHandlers } = createApp();
|
||||
registerBrowserAgentActRoutes(app, {
|
||||
state: () => ({ resolved: { evaluateEnabled: true } }),
|
||||
} as never);
|
||||
const handler = postHandlers.get("/act");
|
||||
expect(handler).toBeTypeOf("function");
|
||||
|
||||
const response = createResponse();
|
||||
await handler?.(
|
||||
{
|
||||
params: {},
|
||||
query: {},
|
||||
body: { kind: "wait", url: "**/example.com/" },
|
||||
},
|
||||
response.res,
|
||||
);
|
||||
|
||||
expect(response.statusCode).toBe(200);
|
||||
expect(response.body).toMatchObject({ ok: true, targetId: "7" });
|
||||
expect(chromeMcpMocks.evaluateChromeMcpScript).toHaveBeenCalledWith({
|
||||
profileName: "chrome-live",
|
||||
targetId: "7",
|
||||
fn: "() => window.location.href",
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
80
src/browser/routes/basic.existing-session.test.ts
Normal file
80
src/browser/routes/basic.existing-session.test.ts
Normal file
@@ -0,0 +1,80 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { BrowserProfileUnavailableError } from "../errors.js";
|
||||
import { registerBrowserBasicRoutes } from "./basic.js";
|
||||
import type { BrowserResponse, BrowserRouteHandler, BrowserRouteRegistrar } from "./types.js";
|
||||
|
||||
function createApp() {
|
||||
const getHandlers = new Map<string, BrowserRouteHandler>();
|
||||
const postHandlers = new Map<string, BrowserRouteHandler>();
|
||||
const deleteHandlers = new Map<string, BrowserRouteHandler>();
|
||||
const app: BrowserRouteRegistrar = {
|
||||
get: (path, handler) => void getHandlers.set(path, handler),
|
||||
post: (path, handler) => void postHandlers.set(path, handler),
|
||||
delete: (path, handler) => void deleteHandlers.set(path, handler),
|
||||
};
|
||||
return { app, getHandlers };
|
||||
}
|
||||
|
||||
function createResponse() {
|
||||
let statusCode = 200;
|
||||
let jsonBody: unknown;
|
||||
const res: BrowserResponse = {
|
||||
status(code) {
|
||||
statusCode = code;
|
||||
return res;
|
||||
},
|
||||
json(body) {
|
||||
jsonBody = body;
|
||||
},
|
||||
};
|
||||
return {
|
||||
res,
|
||||
get statusCode() {
|
||||
return statusCode;
|
||||
},
|
||||
get body() {
|
||||
return jsonBody;
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
describe("basic browser routes", () => {
|
||||
it("maps existing-session status failures to JSON browser errors", async () => {
|
||||
const { app, getHandlers } = createApp();
|
||||
registerBrowserBasicRoutes(app, {
|
||||
state: () => ({
|
||||
resolved: {
|
||||
enabled: true,
|
||||
headless: false,
|
||||
noSandbox: false,
|
||||
executablePath: undefined,
|
||||
},
|
||||
profiles: new Map(),
|
||||
}),
|
||||
forProfile: () =>
|
||||
({
|
||||
profile: {
|
||||
name: "chrome-live",
|
||||
driver: "existing-session",
|
||||
cdpPort: 18802,
|
||||
cdpUrl: "http://127.0.0.1:18802",
|
||||
color: "#00AA00",
|
||||
attachOnly: true,
|
||||
},
|
||||
isHttpReachable: async () => {
|
||||
throw new BrowserProfileUnavailableError("attach failed");
|
||||
},
|
||||
isReachable: async () => true,
|
||||
}) as never,
|
||||
} as never);
|
||||
|
||||
const handler = getHandlers.get("/");
|
||||
expect(handler).toBeTypeOf("function");
|
||||
|
||||
const response = createResponse();
|
||||
await handler?.({ params: {}, query: { profile: "chrome-live" } }, response.res);
|
||||
|
||||
expect(response.statusCode).toBe(409);
|
||||
expect(response.body).toMatchObject({ error: "attach failed" });
|
||||
});
|
||||
});
|
||||
@@ -54,50 +54,58 @@ export function registerBrowserBasicRoutes(app: BrowserRouteRegistrar, ctx: Brow
|
||||
return jsonError(res, profileCtx.status, profileCtx.error);
|
||||
}
|
||||
|
||||
const [cdpHttp, cdpReady] = await Promise.all([
|
||||
profileCtx.isHttpReachable(300),
|
||||
profileCtx.isReachable(600),
|
||||
]);
|
||||
|
||||
const profileState = current.profiles.get(profileCtx.profile.name);
|
||||
let detectedBrowser: string | null = null;
|
||||
let detectedExecutablePath: string | null = null;
|
||||
let detectError: string | null = null;
|
||||
|
||||
try {
|
||||
const detected = resolveBrowserExecutableForPlatform(current.resolved, process.platform);
|
||||
if (detected) {
|
||||
detectedBrowser = detected.kind;
|
||||
detectedExecutablePath = detected.path;
|
||||
}
|
||||
} catch (err) {
|
||||
detectError = String(err);
|
||||
}
|
||||
const [cdpHttp, cdpReady] = await Promise.all([
|
||||
profileCtx.isHttpReachable(300),
|
||||
profileCtx.isReachable(600),
|
||||
]);
|
||||
|
||||
res.json({
|
||||
enabled: current.resolved.enabled,
|
||||
profile: profileCtx.profile.name,
|
||||
driver: profileCtx.profile.driver,
|
||||
running: cdpReady,
|
||||
cdpReady,
|
||||
cdpHttp,
|
||||
pid:
|
||||
profileCtx.profile.driver === "existing-session"
|
||||
? getChromeMcpPid(profileCtx.profile.name)
|
||||
: (profileState?.running?.pid ?? null),
|
||||
cdpPort: profileCtx.profile.cdpPort,
|
||||
cdpUrl: profileCtx.profile.cdpUrl,
|
||||
chosenBrowser: profileState?.running?.exe.kind ?? null,
|
||||
detectedBrowser,
|
||||
detectedExecutablePath,
|
||||
detectError,
|
||||
userDataDir: profileState?.running?.userDataDir ?? null,
|
||||
color: profileCtx.profile.color,
|
||||
headless: current.resolved.headless,
|
||||
noSandbox: current.resolved.noSandbox,
|
||||
executablePath: current.resolved.executablePath ?? null,
|
||||
attachOnly: profileCtx.profile.attachOnly,
|
||||
});
|
||||
const profileState = current.profiles.get(profileCtx.profile.name);
|
||||
let detectedBrowser: string | null = null;
|
||||
let detectedExecutablePath: string | null = null;
|
||||
let detectError: string | null = null;
|
||||
|
||||
try {
|
||||
const detected = resolveBrowserExecutableForPlatform(current.resolved, process.platform);
|
||||
if (detected) {
|
||||
detectedBrowser = detected.kind;
|
||||
detectedExecutablePath = detected.path;
|
||||
}
|
||||
} catch (err) {
|
||||
detectError = String(err);
|
||||
}
|
||||
|
||||
res.json({
|
||||
enabled: current.resolved.enabled,
|
||||
profile: profileCtx.profile.name,
|
||||
driver: profileCtx.profile.driver,
|
||||
running: cdpReady,
|
||||
cdpReady,
|
||||
cdpHttp,
|
||||
pid:
|
||||
profileCtx.profile.driver === "existing-session"
|
||||
? getChromeMcpPid(profileCtx.profile.name)
|
||||
: (profileState?.running?.pid ?? null),
|
||||
cdpPort: profileCtx.profile.cdpPort,
|
||||
cdpUrl: profileCtx.profile.cdpUrl,
|
||||
chosenBrowser: profileState?.running?.exe.kind ?? null,
|
||||
detectedBrowser,
|
||||
detectedExecutablePath,
|
||||
detectError,
|
||||
userDataDir: profileState?.running?.userDataDir ?? null,
|
||||
color: profileCtx.profile.color,
|
||||
headless: current.resolved.headless,
|
||||
noSandbox: current.resolved.noSandbox,
|
||||
executablePath: current.resolved.executablePath ?? null,
|
||||
attachOnly: profileCtx.profile.attachOnly,
|
||||
});
|
||||
} catch (err) {
|
||||
const mapped = toBrowserErrorResponse(err);
|
||||
if (mapped) {
|
||||
return jsonError(res, mapped.status, mapped.message);
|
||||
}
|
||||
jsonError(res, 500, String(err));
|
||||
}
|
||||
});
|
||||
|
||||
// Start browser (profile-aware)
|
||||
|
||||
@@ -96,10 +96,14 @@ describe("browser control server", () => {
|
||||
headers: { "Content-Type": "application/json" },
|
||||
body: JSON.stringify({ kind: "click", selector: "button.save" }),
|
||||
});
|
||||
expect(clickSelector.status).toBe(400);
|
||||
expect(((await clickSelector.json()) as { error?: string }).error).toMatch(
|
||||
/'selector' is not supported/i,
|
||||
);
|
||||
expect(clickSelector.status).toBe(200);
|
||||
expect(((await clickSelector.json()) as { ok?: boolean }).ok).toBe(true);
|
||||
expect(pwMocks.clickViaPlaywright).toHaveBeenNthCalledWith(2, {
|
||||
cdpUrl: state.cdpBaseUrl,
|
||||
targetId: "abcd1234",
|
||||
selector: "button.save",
|
||||
doubleClick: false,
|
||||
});
|
||||
|
||||
const type = await postJson<{ ok: boolean }>(`${base}/act`, {
|
||||
kind: "type",
|
||||
|
||||
15
src/browser/url-pattern.ts
Normal file
15
src/browser/url-pattern.ts
Normal file
@@ -0,0 +1,15 @@
|
||||
export function matchBrowserUrlPattern(pattern: string, url: string): boolean {
|
||||
const trimmedPattern = pattern.trim();
|
||||
if (!trimmedPattern) {
|
||||
return false;
|
||||
}
|
||||
if (trimmedPattern === url) {
|
||||
return true;
|
||||
}
|
||||
if (trimmedPattern.includes("*")) {
|
||||
const escaped = trimmedPattern.replace(/[|\\{}()[\]^$+?.]/g, "\\$&");
|
||||
const regex = new RegExp(`^${escaped.replace(/\*\*/g, ".*").replace(/\*/g, ".*")}$`);
|
||||
return regex.test(url);
|
||||
}
|
||||
return url.includes(trimmedPattern);
|
||||
}
|
||||
Reference in New Issue
Block a user