From dca86d47e083bc36125385eb81b764e262023a4e Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Fri, 29 May 2026 03:30:46 -0400 Subject: [PATCH] fix(browser): validate hook download timeouts --- .../src/browser/routes/agent.act.download.ts | 18 +++++++-- .../src/browser/routes/agent.act.hooks.ts | 17 +++++++-- .../browser/src/browser/routes/agent.act.ts | 10 +---- .../src/browser/routes/route-numeric.ts | 9 +++++ ...-contract-form-layout-act-commands.test.ts | 38 +++++++++++++++++++ 5 files changed, 77 insertions(+), 15 deletions(-) create mode 100644 extensions/browser/src/browser/routes/route-numeric.ts diff --git a/extensions/browser/src/browser/routes/agent.act.download.ts b/extensions/browser/src/browser/routes/agent.act.download.ts index 2cb6b5e6f9a..a46db87683b 100644 --- a/extensions/browser/src/browser/routes/agent.act.download.ts +++ b/extensions/browser/src/browser/routes/agent.act.download.ts @@ -1,3 +1,4 @@ +import { formatErrorMessage } from "../../infra/errors.js"; import { getBrowserProfileCapabilities } from "../profile-capabilities.js"; import type { BrowserRouteContext } from "../server-context.js"; import { @@ -9,8 +10,9 @@ import { import { EXISTING_SESSION_LIMITS } from "./existing-session-limits.js"; import { ensureOutputRootDir, resolveWritableOutputPathOrRespond } from "./output-paths.js"; import { DEFAULT_DOWNLOAD_DIR } from "./path-output.js"; +import { readRoutePositiveInteger } from "./route-numeric.js"; import type { BrowserRouteRegistrar } from "./types.js"; -import { asyncBrowserRoute, jsonError, toNumber, toStringOrEmpty } from "./utils.js"; +import { asyncBrowserRoute, jsonError, toStringOrEmpty } from "./utils.js"; function buildDownloadRequestBase(cdpUrl: string, targetId: string, timeoutMs: number | undefined) { return { @@ -30,7 +32,12 @@ export function registerBrowserAgentActDownloadRoutes( const body = readBody(req); const targetId = resolveTargetIdFromBody(body); const out = toStringOrEmpty(body.path) || ""; - const timeoutMs = toNumber(body.timeoutMs); + let timeoutMs: number | undefined; + try { + timeoutMs = readRoutePositiveInteger(body.timeoutMs, "timeoutMs"); + } catch (err) { + return jsonError(res, 400, formatErrorMessage(err)); + } await withRouteTabContext({ req, @@ -78,7 +85,12 @@ export function registerBrowserAgentActDownloadRoutes( const targetId = resolveTargetIdFromBody(body); const ref = toStringOrEmpty(body.ref); const out = toStringOrEmpty(body.path); - const timeoutMs = toNumber(body.timeoutMs); + let timeoutMs: number | undefined; + try { + timeoutMs = readRoutePositiveInteger(body.timeoutMs, "timeoutMs"); + } catch (err) { + return jsonError(res, 400, formatErrorMessage(err)); + } if (!ref) { return jsonError(res, 400, "ref is required"); } diff --git a/extensions/browser/src/browser/routes/agent.act.hooks.ts b/extensions/browser/src/browser/routes/agent.act.hooks.ts index eb100242ad1..93fd3490055 100644 --- a/extensions/browser/src/browser/routes/agent.act.hooks.ts +++ b/extensions/browser/src/browser/routes/agent.act.hooks.ts @@ -1,3 +1,4 @@ +import { formatErrorMessage } from "../../infra/errors.js"; import { evaluateChromeMcpScript, uploadChromeMcpFile } from "../chrome-mcp.js"; import { getBrowserProfileCapabilities } from "../profile-capabilities.js"; import type { BrowserRouteContext } from "../server-context.js"; @@ -9,12 +10,12 @@ import { } from "./agent.shared.js"; import { EXISTING_SESSION_LIMITS } from "./existing-session-limits.js"; import { DEFAULT_UPLOAD_DIR, pathScope } from "./path-output.js"; +import { readRoutePositiveInteger } from "./route-numeric.js"; import type { BrowserRouteRegistrar } from "./types.js"; import { asyncBrowserRoute, jsonError, toBoolean, - toNumber, toStringArray, toStringOrEmpty, } from "./utils.js"; @@ -32,7 +33,12 @@ export function registerBrowserAgentActHookRoutes( const inputRef = toStringOrEmpty(body.inputRef) || undefined; const element = toStringOrEmpty(body.element) || undefined; const paths = toStringArray(body.paths) ?? []; - const timeoutMs = toNumber(body.timeoutMs); + let timeoutMs: number | undefined; + try { + timeoutMs = readRoutePositiveInteger(body.timeoutMs, "timeoutMs"); + } catch (err) { + return jsonError(res, 400, formatErrorMessage(err)); + } if (!paths.length) { return jsonError(res, 400, "paths are required"); } @@ -118,7 +124,12 @@ export function registerBrowserAgentActHookRoutes( const targetId = resolveTargetIdFromBody(body); const accept = toBoolean(body.accept); const promptText = toStringOrEmpty(body.promptText) || undefined; - const timeoutMs = toNumber(body.timeoutMs); + let timeoutMs: number | undefined; + try { + timeoutMs = readRoutePositiveInteger(body.timeoutMs, "timeoutMs"); + } catch (err) { + return jsonError(res, 400, formatErrorMessage(err)); + } const dialogId = toStringOrEmpty(body.dialogId) || undefined; if (accept === undefined) { return jsonError(res, 400, "accept is required"); diff --git a/extensions/browser/src/browser/routes/agent.act.ts b/extensions/browser/src/browser/routes/agent.act.ts index b3b9333e7ab..b56b502b27a 100644 --- a/extensions/browser/src/browser/routes/agent.act.ts +++ b/extensions/browser/src/browser/routes/agent.act.ts @@ -1,4 +1,3 @@ -import { parseStrictPositiveInteger } from "openclaw/plugin-sdk/number-runtime"; import { formatErrorMessage } from "../../infra/errors.js"; import { clickChromeMcpElement, @@ -41,6 +40,7 @@ import { } from "./agent.shared.js"; import { resolveTargetIdAfterNavigate } from "./agent.snapshot-target.js"; import { EXISTING_SESSION_LIMITS } from "./existing-session-limits.js"; +import { readRoutePositiveInteger } from "./route-numeric.js"; import type { BrowserRouteRegistrar } from "./types.js"; import { asyncBrowserRoute, jsonError, toStringOrEmpty } from "./utils.js"; @@ -348,14 +348,6 @@ function getExistingSessionUnsupportedMessage(action: BrowserActRequest): string throw new Error("Unsupported browser act kind"); } -function readRoutePositiveInteger(value: unknown, fieldName: string): number | undefined { - const parsed = parseStrictPositiveInteger(value); - if (parsed === undefined && value != null) { - throw new Error(`${fieldName} must be a positive integer.`); - } - return parsed; -} - export function registerBrowserAgentActRoutes( app: BrowserRouteRegistrar, ctx: BrowserRouteContext, diff --git a/extensions/browser/src/browser/routes/route-numeric.ts b/extensions/browser/src/browser/routes/route-numeric.ts new file mode 100644 index 00000000000..6d69036e458 --- /dev/null +++ b/extensions/browser/src/browser/routes/route-numeric.ts @@ -0,0 +1,9 @@ +import { parseStrictPositiveInteger } from "openclaw/plugin-sdk/number-runtime"; + +export function readRoutePositiveInteger(value: unknown, fieldName: string): number | undefined { + const parsed = parseStrictPositiveInteger(value); + if (parsed === undefined && value != null) { + throw new Error(`${fieldName} must be a positive integer.`); + } + return parsed; +} diff --git a/extensions/browser/src/browser/server.agent-contract-form-layout-act-commands.test.ts b/extensions/browser/src/browser/server.agent-contract-form-layout-act-commands.test.ts index 0d00d64de0d..653165d39de 100644 --- a/extensions/browser/src/browser/server.agent-contract-form-layout-act-commands.test.ts +++ b/extensions/browser/src/browser/server.agent-contract-form-layout-act-commands.test.ts @@ -472,6 +472,44 @@ describe("browser control server", () => { expect(pwMocks.responseBodyViaPlaywright).toHaveBeenCalledTimes(beforeCalls); }); + it("rejects loose hook and download timeout options before dispatch", async () => { + const base = await startServerAndBase(); + const uploadCalls = pwMocks.armFileUploadViaPlaywright.mock.calls.length; + const dialogCalls = pwMocks.armDialogViaPlaywright.mock.calls.length; + const waitCalls = pwMocks.waitForDownloadViaPlaywright.mock.calls.length; + const downloadCalls = pwMocks.downloadViaPlaywright.mock.calls.length; + + const uploadRes = await postJson<{ error?: string }>(`${base}/hooks/file-chooser`, { + paths: ["a.txt"], + timeoutMs: "1e3", + }); + expect(uploadRes.error).toContain("timeoutMs must be a positive integer."); + + const dialogRes = await postJson<{ error?: string }>(`${base}/hooks/dialog`, { + accept: true, + timeoutMs: "0x10", + }); + expect(dialogRes.error).toContain("timeoutMs must be a positive integer."); + + const waitRes = await postJson<{ error?: string }>(`${base}/wait/download`, { + path: "report.pdf", + timeoutMs: "1000ms", + }); + expect(waitRes.error).toContain("timeoutMs must be a positive integer."); + + const downloadRes = await postJson<{ error?: string }>(`${base}/download`, { + ref: "e12", + path: "report.pdf", + timeoutMs: "1.5", + }); + expect(downloadRes.error).toContain("timeoutMs must be a positive integer."); + + expect(pwMocks.armFileUploadViaPlaywright).toHaveBeenCalledTimes(uploadCalls); + expect(pwMocks.armDialogViaPlaywright).toHaveBeenCalledTimes(dialogCalls); + expect(pwMocks.waitForDownloadViaPlaywright).toHaveBeenCalledTimes(waitCalls); + expect(pwMocks.downloadViaPlaywright).toHaveBeenCalledTimes(downloadCalls); + }); + it("agent contract: hooks + response + downloads + screenshot", async () => { const base = await startServerAndBase();