From cc0980cb7a8949e29c35c32125a29bbea4fd7ca7 Mon Sep 17 00:00:00 2001 From: wings1029 Date: Wed, 1 Jul 2026 18:05:58 +0800 Subject: [PATCH] fix(browser): bound error body read in fetchHttpJson to prevent OOM (#98455) * fix(browser): bound error body read in fetchHttpJson to prevent OOM * fix(browser): enforce strict error response limit --------- Co-authored-by: Peter Steinberger <58493+steipete@users.noreply.github.com> --- .../client-fetch.error-body-boundary.test.ts | 123 ++++++++++++++++++ .../browser/src/browser/client-fetch.ts | 9 +- 2 files changed, 131 insertions(+), 1 deletion(-) create mode 100644 extensions/browser/src/browser/client-fetch.error-body-boundary.test.ts diff --git a/extensions/browser/src/browser/client-fetch.error-body-boundary.test.ts b/extensions/browser/src/browser/client-fetch.error-body-boundary.test.ts new file mode 100644 index 000000000000..d7d3b9475d18 --- /dev/null +++ b/extensions/browser/src/browser/client-fetch.error-body-boundary.test.ts @@ -0,0 +1,123 @@ +import http from "node:http"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +const authMocks = vi.hoisted(() => ({ + loadConfig: vi.fn(() => ({})), + resolveBrowserControlAuth: vi.fn(() => ({})), + getBridgeAuthForPort: vi.fn(() => undefined), +})); + +vi.mock("../config/config.js", async () => { + const actual = await vi.importActual("../config/config.js"); + return { ...actual, getRuntimeConfig: authMocks.loadConfig, loadConfig: authMocks.loadConfig }; +}); +vi.mock("./control-auth.js", () => ({ + resolveBrowserControlAuth: authMocks.resolveBrowserControlAuth, +})); +vi.mock("./bridge-auth-registry.js", () => ({ + getBridgeAuthForPort: authMocks.getBridgeAuthForPort, +})); + +const { fetchBrowserJson } = await import("./client-fetch.js"); + +const STREAM_CHUNK = Buffer.alloc(4 * 1024, "x"); +const STREAM_BODY_BYTES = 1024 * 1024; + +describe("fetchHttpJson error body boundary", () => { + let server: http.Server; + let baseUrl: string; + let streamClosed: Promise; + let resolveStreamClosed: () => void; + let smallConnectionClosed: Promise; + let resolveSmallConnectionClosed: () => void; + let streamCompleted: boolean; + + beforeEach(async () => { + for (const key of [ + "ALL_PROXY", + "all_proxy", + "HTTP_PROXY", + "http_proxy", + "HTTPS_PROXY", + "https_proxy", + ]) { + vi.stubEnv(key, ""); + } + + streamClosed = new Promise((resolve) => { + resolveStreamClosed = resolve; + }); + smallConnectionClosed = new Promise((resolve) => { + resolveSmallConnectionClosed = resolve; + }); + streamCompleted = false; + server = http.createServer((req, res) => { + if (req.url === "/small") { + req.socket.once("close", () => resolveSmallConnectionClosed()); + res.writeHead(500, { "Content-Type": "text/plain" }); + res.end("session expired"); + return; + } + + res.writeHead(500, { "Content-Type": "text/plain" }); + let written = 0; + let closed = false; + res.once("close", () => { + closed = true; + resolveStreamClosed(); + }); + const writeNext = () => { + if (closed) { + return; + } + if (written >= STREAM_BODY_BYTES) { + streamCompleted = true; + res.end(); + return; + } + written += STREAM_CHUNK.byteLength; + const writeMore = () => setTimeout(writeNext, 2); + if (res.write(STREAM_CHUNK)) { + writeMore(); + } else { + res.once("drain", writeMore); + } + }; + writeNext(); + }); + await new Promise((resolve) => { + server.listen(0, "127.0.0.1", resolve); + }); + const address = server.address(); + if (!address || typeof address === "string") { + throw new Error("expected loopback server address"); + } + baseUrl = `http://127.0.0.1:${address.port}`; + }); + + afterEach(async () => { + vi.unstubAllEnvs(); + server.closeAllConnections(); + await new Promise((resolve) => { + server.close(() => resolve()); + }); + }); + + it("cancels an overflowing stream and releases the guarded fetch", async () => { + const error = await fetchBrowserJson(`${baseUrl}/large`).catch((err: unknown) => err); + + expect(error).toMatchObject({ name: "BrowserServiceError", message: "HTTP 500" }); + await expect(streamClosed).resolves.toBeUndefined(); + expect(streamCompleted).toBe(false); + }); + + it("preserves a complete diagnostic body within the limit", async () => { + const error = await fetchBrowserJson(`${baseUrl}/small`).catch((err: unknown) => err); + + expect(error).toMatchObject({ + name: "BrowserServiceError", + message: "session expired", + }); + await expect(smallConnectionClosed).resolves.toBeUndefined(); + }); +}); diff --git a/extensions/browser/src/browser/client-fetch.ts b/extensions/browser/src/browser/client-fetch.ts index 82f6d1625158..37bfcaa566fb 100644 --- a/extensions/browser/src/browser/client-fetch.ts +++ b/extensions/browser/src/browser/client-fetch.ts @@ -6,6 +6,7 @@ */ import { parseBrowserHttpUrl } from "openclaw/plugin-sdk/browser-config"; import { resolveTimerTimeoutMs } from "openclaw/plugin-sdk/number-runtime"; +import { readResponseWithLimit } from "openclaw/plugin-sdk/response-limit-runtime"; import { fetchWithSsrFGuard } from "openclaw/plugin-sdk/ssrf-runtime"; import { normalizeOptionalString } from "openclaw/plugin-sdk/string-coerce-runtime"; import { normalizeLowercaseStringOrEmpty } from "openclaw/plugin-sdk/string-coerce-runtime"; @@ -104,6 +105,8 @@ const BROWSER_TOOL_MODEL_HINT = "Do NOT retry the browser tool — it will keep failing. " + "Use an alternative approach or inform the user that the browser is currently unavailable."; +const BROWSER_ERROR_BODY_LIMIT_BYTES = 16 * 1024; + function isRateLimitStatus(status: number): boolean { return status === 429; } @@ -267,7 +270,11 @@ async function fetchHttpJson( `${resolveBrowserRateLimitMessage(url)} ${BROWSER_TOOL_MODEL_HINT}`, ); } - const text = await res.text().catch(() => ""); + // Overflow cancels the stream and releases its reader lock before the guarded fetch below. + const body = await readResponseWithLimit(res, BROWSER_ERROR_BODY_LIMIT_BYTES).catch( + () => undefined, + ); + const text = body ? new TextDecoder().decode(body) : ""; throw new BrowserServiceError(text || `HTTP ${res.status}`); } return (await res.json()) as T;