fix(browser): validate hook download timeouts

This commit is contained in:
Peter Steinberger
2026-05-29 03:30:46 -04:00
parent 854cb9292d
commit dca86d47e0
5 changed files with 77 additions and 15 deletions

View File

@@ -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");
}

View File

@@ -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");

View File

@@ -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,

View File

@@ -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;
}

View File

@@ -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();