Skip to content

Commit 286883c

Browse files
steipetesteipete
committed
fix(browser): cap route timer delays
1 parent b073094 commit 286883c

8 files changed

Lines changed: 91 additions & 13 deletions

File tree

‎extensions/browser/src/browser/cdp.helpers.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import { CDP_HTTP_REQUEST_TIMEOUT_MS, CDP_WS_HANDSHAKE_TIMEOUT_MS } from "./cdp-
1616
import { BrowserCdpEndpointBlockedError } from "./errors.js";
1717
import { resolveBrowserRateLimitMessage } from "./rate-limit-message.js";
1818
import { withAllowedHostname } from "./ssrf-policy-helpers.js";
19+
import { normalizeBrowserTimerDelayMs } from "./timer-delay.js";
1920

2021
export { isLoopbackHost };
2122
export { parseBrowserHttpUrl, redactCdpUrl };
@@ -196,7 +197,7 @@ function createCdpSender(ws: WebSocket, opts?: { commandTimeoutMs?: number }) {
196197
const pending = new Map<number, Pending>();
197198
const commandTimeoutMs =
198199
typeof opts?.commandTimeoutMs === "number" && Number.isFinite(opts.commandTimeoutMs)
199-
? Math.max(1, Math.floor(opts.commandTimeoutMs))
200+
? normalizeBrowserTimerDelayMs(opts.commandTimeoutMs)
200201
: undefined;
201202

202203
const clearPendingTimer = (p: Pending) => {
@@ -306,7 +307,7 @@ export async function fetchCdpChecked(
306307
ssrfPolicy?: SsrFPolicy,
307308
): Promise<CdpFetchResult> {
308309
const ctrl = new AbortController();
309-
const t = setTimeout(ctrl.abort.bind(ctrl), timeoutMs);
310+
const t = setTimeout(ctrl.abort.bind(ctrl), normalizeBrowserTimerDelayMs(timeoutMs));
310311
let guardedRelease: (() => Promise<void>) | undefined;
311312
let released = false;
312313
const release = async () => {

‎extensions/browser/src/browser/routes/agent.snapshot.plan.test.ts‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,16 @@ describe("resolveSnapshotPlan", () => {
5858
expect(plan.timeoutMs).toBe(12345);
5959
});
6060

61+
it("caps timeoutMs from the snapshot query string to Node's safe timer range", () => {
62+
const plan = resolveSnapshotPlan({
63+
profile: profile("openclaw"),
64+
query: { timeoutMs: "3000000000" },
65+
hasPlaywright: true,
66+
});
67+
68+
expect(plan.timeoutMs).toBe(2_147_483_647);
69+
});
70+
6171
it("ignores non-positive timeoutMs values", () => {
6272
expect(
6373
resolveSnapshotPlan({

‎extensions/browser/src/browser/routes/agent.snapshot.plan.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import {
1313
shouldUsePlaywrightForAriaSnapshot,
1414
shouldUsePlaywrightForScreenshot,
1515
} from "../profile-capabilities.js";
16+
import { normalizeBrowserTimerDelayMs } from "../timer-delay.js";
1617
import { toBoolean, toNumber, toStringOrEmpty } from "./utils.js";
1718

1819
type BrowserSnapshotPlan = {
@@ -79,7 +80,7 @@ export function resolveSnapshotPlan(params: {
7980
const timeoutMsRaw = toNumber(params.query.timeoutMs);
8081
const timeoutMs =
8182
timeoutMsRaw !== undefined && Number.isFinite(timeoutMsRaw) && timeoutMsRaw > 0
82-
? Math.max(1, Math.floor(timeoutMsRaw))
83+
? normalizeBrowserTimerDelayMs(timeoutMsRaw)
8384
: undefined;
8485

8586
return {

‎extensions/browser/src/browser/routes/agent.snapshot.timeout.test.ts‎

Lines changed: 46 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,24 @@ vi.mock("./agent.shared.js", () => ({
7272
requirePwAi: vi.fn(async () => null),
7373
resolveProfileContext: vi.fn(() => profileContext),
7474
withPlaywrightRouteContext: vi.fn(),
75-
withRouteTabContext: vi.fn(),
75+
withRouteTabContext: vi.fn(
76+
async (params: {
77+
run: (ctx: {
78+
profileCtx: typeof profileContext;
79+
tab: { targetId: string; url: string; wsUrl: string };
80+
cdpUrl: string;
81+
}) => Promise<void>;
82+
}) =>
83+
await params.run({
84+
profileCtx: profileContext,
85+
tab: {
86+
targetId: "tab-1",
87+
url: "https://example.com",
88+
wsUrl: "ws://127.0.0.1:18800/devtools/page/tab-1",
89+
},
90+
cdpUrl: "http://127.0.0.1:18800",
91+
}),
92+
),
7693
}));
7794

7895
const { registerBrowserAgentSnapshotRoutes } = await import("./agent.snapshot.js");
@@ -87,6 +104,16 @@ function getSnapshotHandler() {
87104
return handler;
88105
}
89106

107+
function getScreenshotHandler() {
108+
const { app, postHandlers } = createBrowserRouteApp();
109+
registerBrowserAgentSnapshotRoutes(app, {
110+
state: () => ({ resolved: { extraArgs: [] } }),
111+
} as never);
112+
const handler = postHandlers.get("/screenshot");
113+
expect(handler).toBeTypeOf("function");
114+
return handler;
115+
}
116+
90117
describe("browser agent snapshot timeout routing", () => {
91118
beforeEach(() => {
92119
cdpMocks.captureScreenshot.mockClear();
@@ -124,4 +151,22 @@ describe("browser agent snapshot timeout routing", () => {
124151
}),
125152
);
126153
});
154+
155+
it("caps screenshot timeoutMs before dispatching to CDP", async () => {
156+
cdpMocks.captureScreenshot.mockResolvedValueOnce(Buffer.from("png"));
157+
const handler = getScreenshotHandler();
158+
const response = createBrowserRouteResponse();
159+
160+
await handler?.(
161+
{ params: {}, query: {}, body: { type: "png", timeoutMs: 3_000_000_000 } },
162+
response.res,
163+
);
164+
165+
expect(response.statusCode).toBe(200);
166+
expect(cdpMocks.captureScreenshot).toHaveBeenCalledWith(
167+
expect.objectContaining({
168+
timeoutMs: 2_147_483_647,
169+
}),
170+
);
171+
});
127172
});

‎extensions/browser/src/browser/routes/agent.snapshot.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ import {
2626
normalizeBrowserScreenshot,
2727
} from "../screenshot.js";
2828
import type { BrowserRouteContext, ProfileContext } from "../server-context.js";
29+
import { normalizeBrowserTimerDelayMs } from "../timer-delay.js";
2930
import {
3031
getPwAiModule,
3132
handleRouteError,
@@ -370,7 +371,7 @@ export function registerBrowserAgentSnapshotRoutes(
370371
const timeoutMsRaw = toNumber(body.timeoutMs);
371372
const timeoutMs =
372373
timeoutMsRaw !== undefined
373-
? Math.max(1, Math.floor(timeoutMsRaw))
374+
? normalizeBrowserTimerDelayMs(timeoutMsRaw)
374375
: DEFAULT_BROWSER_SCREENSHOT_TIMEOUT_MS;
375376

376377
if (fullPage && (ref || element)) {

‎extensions/browser/src/browser/timer-delay.test.ts‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
import { describe, expect, it } from "vitest";
2+
import { MAX_SAFE_TIMEOUT_DELAY_MS, normalizeBrowserTimerDelayMs } from "./timer-delay.js";
3+
4+
describe("normalizeBrowserTimerDelayMs", () => {
5+
it("caps timers to Node's safe delay range", () => {
6+
expect(normalizeBrowserTimerDelayMs(3_000_000_000)).toBe(MAX_SAFE_TIMEOUT_DELAY_MS);
7+
});
8+
9+
it("preserves positive integer timers and applies the minimum", () => {
10+
expect(normalizeBrowserTimerDelayMs(1234.9)).toBe(1234);
11+
expect(normalizeBrowserTimerDelayMs(-5)).toBe(1);
12+
expect(normalizeBrowserTimerDelayMs(0, { minMs: 0 })).toBe(0);
13+
});
14+
});

‎extensions/browser/src/browser/timer-delay.ts‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
export const MAX_SAFE_TIMEOUT_DELAY_MS = 2_147_483_647;
2+
3+
export function normalizeBrowserTimerDelayMs(timeoutMs: number, opts?: { minMs?: number }): number {
4+
const rawMinMs = opts?.minMs ?? 1;
5+
const minMs = Math.min(
6+
MAX_SAFE_TIMEOUT_DELAY_MS,
7+
Math.max(0, Number.isFinite(rawMinMs) ? Math.floor(rawMinMs) : 1),
8+
);
9+
const candidateMs = Number.isFinite(timeoutMs) ? Math.floor(timeoutMs) : minMs;
10+
return Math.min(MAX_SAFE_TIMEOUT_DELAY_MS, Math.max(minMs, candidateMs));
11+
}

‎extensions/browser/src/cli/browser-cli-shared.ts‎

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,9 @@ import {
33
BROWSER_REQUEST_GATEWAY_METHOD,
44
BROWSER_REQUEST_GATEWAY_SCOPES,
55
} from "../browser-gateway-contract.js";
6+
import { normalizeBrowserTimerDelayMs } from "../browser/timer-delay.js";
67
import { callGatewayFromCli, type GatewayRpcOpts } from "./core-api.js";
78

8-
const MAX_SAFE_TIMEOUT_DELAY_MS = 2_147_483_647;
9-
109
export type BrowserParentOpts = GatewayRpcOpts & {
1110
json?: boolean;
1211
browserProfile?: string;
@@ -45,20 +44,16 @@ function parsePositiveInteger(raw: string, flag: string): number {
4544
return parsed;
4645
}
4746

48-
function normalizeCliTimeoutMs(timeoutMs: number): number {
49-
return Math.min(MAX_SAFE_TIMEOUT_DELAY_MS, Math.max(1, Math.floor(timeoutMs)));
50-
}
51-
5247
export async function callBrowserRequest<T>(
5348
opts: BrowserParentOpts,
5449
params: BrowserRequestParams,
5550
extra?: { timeoutMs?: number; progress?: boolean },
5651
): Promise<T> {
5752
const resolvedTimeoutMs =
5853
typeof extra?.timeoutMs === "number" && Number.isFinite(extra.timeoutMs)
59-
? normalizeCliTimeoutMs(extra.timeoutMs)
54+
? normalizeBrowserTimerDelayMs(extra.timeoutMs)
6055
: typeof opts.timeout === "string"
61-
? normalizeCliTimeoutMs(parsePositiveInteger(opts.timeout, "--timeout"))
56+
? normalizeBrowserTimerDelayMs(parsePositiveInteger(opts.timeout, "--timeout"))
6257
: undefined;
6358
const resolvedTimeout =
6459
typeof resolvedTimeoutMs === "number" && Number.isFinite(resolvedTimeoutMs)

0 commit comments

Comments
 (0)