Skip to content

Commit f3fa3c1

Browse files
steipetesteipete
committed
fix(browser): enforce strict error response limit
1 parent 1297933 commit f3fa3c1

2 files changed

Lines changed: 73 additions & 125 deletions

File tree

‎extensions/browser/src/browser/client-fetch.error-body-boundary.test.ts‎

Lines changed: 68 additions & 119 deletions
Original file line numberDiff line numberDiff line change
@@ -1,74 +1,38 @@
1-
/**
2-
* Boundary test: fetchHttpJson error body is read under a byte cap.
3-
*
4-
* Proves that a streaming error response larger than 16 KiB is cancelled
5-
* mid-stream instead of being fully buffered, preventing OOM on malformed
6-
* upstream error pages from the browser control service.
7-
*/
81
import http from "node:http";
92
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
10-
import "../test-support/browser-security.mock.js";
113

12-
// Replace fetchWithSsrFGuard with a lightweight pass-through so the test
13-
// can drive fetchHttpJson against a real loopback HTTP server. The SSRF
14-
// guard is tested separately; this test keeps the focus on the response
15-
// body boundary.
16-
vi.mock("openclaw/plugin-sdk/ssrf-runtime", async () => {
17-
const actual = await vi.importActual<typeof import("openclaw/plugin-sdk/ssrf-runtime")>(
18-
"openclaw/plugin-sdk/ssrf-runtime",
19-
);
20-
return {
21-
...actual,
22-
fetchWithSsrFGuard: async (params: {
23-
url: string;
24-
init?: RequestInit;
25-
signal?: AbortSignal;
26-
}) => ({
27-
response: await fetch(params.url, {
28-
...params.init,
29-
signal: params.signal,
30-
}),
31-
finalUrl: params.url,
32-
release: async () => {},
33-
}),
34-
};
35-
});
36-
37-
// Config/auth mocks — the test hits a loopback URL so the auth path
38-
// tries to look up config; stub it out to avoid side effects.
39-
const mocks = vi.hoisted(() => ({
4+
const authMocks = vi.hoisted(() => ({
405
loadConfig: vi.fn(() => ({})),
416
resolveBrowserControlAuth: vi.fn(() => ({})),
427
getBridgeAuthForPort: vi.fn(() => undefined),
438
}));
449

4510
vi.mock("../config/config.js", async () => {
4611
const actual = await vi.importActual<typeof import("../config/config.js")>("../config/config.js");
47-
return { ...actual, getRuntimeConfig: mocks.loadConfig, loadConfig: mocks.loadConfig };
12+
return { ...actual, getRuntimeConfig: authMocks.loadConfig, loadConfig: authMocks.loadConfig };
4813
});
49-
5014
vi.mock("./control-auth.js", () => ({
51-
resolveBrowserControlAuth: mocks.resolveBrowserControlAuth,
15+
resolveBrowserControlAuth: authMocks.resolveBrowserControlAuth,
5216
}));
53-
5417
vi.mock("./bridge-auth-registry.js", () => ({
55-
getBridgeAuthForPort: mocks.getBridgeAuthForPort,
18+
getBridgeAuthForPort: authMocks.getBridgeAuthForPort,
5619
}));
5720

5821
const { fetchBrowserJson } = await import("./client-fetch.js");
5922

60-
/** Streaming chunks are large enough to exceed the 16 KiB cap in one chunk. */
61-
const CHUNK_SIZE = 64 * 1024;
62-
63-
/** Total bytes the server will stream before closing. */
64-
const TOTAL_BODY_SIZE = 4 * 1024 * 1024;
23+
const STREAM_CHUNK = Buffer.alloc(4 * 1024, "x");
24+
const STREAM_BODY_BYTES = 1024 * 1024;
6525

6626
describe("fetchHttpJson error body boundary", () => {
6727
let server: http.Server;
68-
let port: number;
28+
let baseUrl: string;
29+
let streamClosed: Promise<void>;
30+
let resolveStreamClosed: () => void;
31+
let smallConnectionClosed: Promise<void>;
32+
let resolveSmallConnectionClosed: () => void;
33+
let streamCompleted: boolean;
6934

7035
beforeEach(async () => {
71-
vi.restoreAllMocks();
7236
for (const key of [
7337
"ALL_PROXY",
7438
"all_proxy",
@@ -79,96 +43,81 @@ describe("fetchHttpJson error body boundary", () => {
7943
]) {
8044
vi.stubEnv(key, "");
8145
}
82-
mocks.loadConfig.mockReturnValue({});
83-
mocks.resolveBrowserControlAuth.mockReturnValue({});
84-
mocks.getBridgeAuthForPort.mockReturnValue(undefined);
8546

86-
// Start a loopback server that streams a large error body.
87-
// No Content-Length header — the client must read until the cap,
88-
// not until a known size.
89-
server = http.createServer((_req, res) => {
90-
res.writeHead(500, { "Content-Type": "text/html" });
47+
streamClosed = new Promise<void>((resolve) => {
48+
resolveStreamClosed = resolve;
49+
});
50+
smallConnectionClosed = new Promise<void>((resolve) => {
51+
resolveSmallConnectionClosed = resolve;
52+
});
53+
streamCompleted = false;
54+
server = http.createServer((req, res) => {
55+
if (req.url === "/small") {
56+
req.socket.once("close", () => resolveSmallConnectionClosed());
57+
res.writeHead(500, { "Content-Type": "text/plain" });
58+
res.end("session expired");
59+
return;
60+
}
61+
62+
res.writeHead(500, { "Content-Type": "text/plain" });
9163
let written = 0;
92-
const chunk = Buffer.alloc(CHUNK_SIZE, "x");
93-
function writeChunk() {
94-
if (written >= TOTAL_BODY_SIZE) {
64+
let closed = false;
65+
res.once("close", () => {
66+
closed = true;
67+
resolveStreamClosed();
68+
});
69+
const writeNext = () => {
70+
if (closed) {
71+
return;
72+
}
73+
if (written >= STREAM_BODY_BYTES) {
74+
streamCompleted = true;
9575
res.end();
9676
return;
9777
}
98-
const ok = res.write(chunk);
99-
written += CHUNK_SIZE;
100-
if (ok) {
101-
// Drain the microtask queue so the reader can consume, then
102-
// schedule the next chunk through setTimeout to avoid starving
103-
// the event loop when backpressure isn't applied.
104-
setImmediate(writeChunk);
78+
written += STREAM_CHUNK.byteLength;
79+
const writeMore = () => setTimeout(writeNext, 2);
80+
if (res.write(STREAM_CHUNK)) {
81+
writeMore();
10582
} else {
106-
res.once("drain", writeChunk);
83+
res.once("drain", writeMore);
10784
}
108-
}
109-
writeChunk();
85+
};
86+
writeNext();
11087
});
111-
11288
await new Promise<void>((resolve) => {
113-
server.listen(0, "127.0.0.1", () => resolve());
89+
server.listen(0, "127.0.0.1", resolve);
11490
});
115-
port = (server.address() as { port: number }).port;
91+
const address = server.address();
92+
if (!address || typeof address === "string") {
93+
throw new Error("expected loopback server address");
94+
}
95+
baseUrl = `http://127.0.0.1:${address.port}`;
11696
});
11797

11898
afterEach(async () => {
119-
vi.unstubAllGlobals();
12099
vi.unstubAllEnvs();
121-
if (server) {
122-
await new Promise<void>((resolve) => {
123-
server.close(() => resolve());
124-
});
125-
}
100+
server.closeAllConnections();
101+
await new Promise<void>((resolve) => {
102+
server.close(() => resolve());
103+
});
126104
});
127105

128-
it("cancels the stream after 16 KiB on a non-ok response", async () => {
129-
const url = `http://127.0.0.1:${port}/error`;
130-
131-
const err = await fetchBrowserJson(url, { timeoutMs: 5000 }).catch((e: unknown) => e);
132-
133-
// Must throw — the server returned 500.
134-
expect(err).toBeInstanceOf(Error);
135-
const message = (err as Error).message;
136-
137-
// The error body snippet must be bounded at ~16 KiB. If unguarded,
138-
// the raw res.text() would buffer the full 4 MiB streaming body
139-
// into the error message. This assertion is the core invariant.
140-
const messageBytes = Buffer.byteLength(message, "utf8");
141-
expect(messageBytes).toBeLessThan(32 * 1024); // well under 4 MiB
142-
expect(messageBytes).toBeGreaterThan(0); // body was read
106+
it("cancels an overflowing stream and releases the guarded fetch", async () => {
107+
const error = await fetchBrowserJson(`${baseUrl}/large`).catch((err: unknown) => err);
143108

144-
// The body text should contain a snippet of the padding —
145-
// readResponseTextLimited returns what it read (up to the cap).
146-
expect(message).toContain("x");
109+
expect(error).toMatchObject({ name: "BrowserServiceError", message: "HTTP 500" });
110+
await expect(streamClosed).resolves.toBeUndefined();
111+
expect(streamCompleted).toBe(false);
147112
});
148113

149-
it("preserves the full error body when it fits under the cap", async () => {
150-
// Small error body: the server returns a short text/plain message.
151-
const snippetServer = http.createServer((_req, res) => {
152-
res.writeHead(500, { "Content-Type": "text/plain" });
153-
res.end("session expired");
154-
});
155-
const snippetPort = await new Promise<number>((resolve) => {
156-
snippetServer.listen(0, "127.0.0.1", () =>
157-
resolve((snippetServer.address() as { port: number }).port),
158-
);
159-
});
160-
161-
try {
162-
const err = await fetchBrowserJson(`http://127.0.0.1:${snippetPort}/err`, {
163-
timeoutMs: 5000,
164-
}).catch((e: unknown) => e);
114+
it("preserves a complete diagnostic body within the limit", async () => {
115+
const error = await fetchBrowserJson(`${baseUrl}/small`).catch((err: unknown) => err);
165116

166-
expect(err).toBeInstanceOf(Error);
167-
expect((err as Error).message).toContain("session expired");
168-
} finally {
169-
await new Promise<void>((resolve) => {
170-
snippetServer.close(() => resolve());
171-
});
172-
}
117+
expect(error).toMatchObject({
118+
name: "BrowserServiceError",
119+
message: "session expired",
120+
});
121+
await expect(smallConnectionClosed).resolves.toBeUndefined();
173122
});
174123
});

‎extensions/browser/src/browser/client-fetch.ts‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
*/
77
import { parseBrowserHttpUrl } from "openclaw/plugin-sdk/browser-config";
88
import { resolveTimerTimeoutMs } from "openclaw/plugin-sdk/number-runtime";
9-
import { readResponseTextLimited } from "openclaw/plugin-sdk/provider-http";
9+
import { readResponseWithLimit } from "openclaw/plugin-sdk/response-limit-runtime";
1010
import { fetchWithSsrFGuard } from "openclaw/plugin-sdk/ssrf-runtime";
1111
import { normalizeOptionalString } from "openclaw/plugin-sdk/string-coerce-runtime";
1212
import { normalizeLowercaseStringOrEmpty } from "openclaw/plugin-sdk/string-coerce-runtime";
@@ -105,9 +105,6 @@ const BROWSER_TOOL_MODEL_HINT =
105105
"Do NOT retry the browser tool — it will keep failing. " +
106106
"Use an alternative approach or inform the user that the browser is currently unavailable.";
107107

108-
/** Max bytes to read from a non-ok browser-control error body before cancelling the stream.
109-
* Kept small to avoid OOM on malformed upstream error pages while preserving a
110-
* useful diagnostic snippet. */
111108
const BROWSER_ERROR_BODY_LIMIT_BYTES = 16 * 1024;
112109

113110
function isRateLimitStatus(status: number): boolean {
@@ -273,9 +270,11 @@ async function fetchHttpJson<T>(
273270
`${resolveBrowserRateLimitMessage(url)} ${BROWSER_TOOL_MODEL_HINT}`,
274271
);
275272
}
276-
const text = await readResponseTextLimited(res, BROWSER_ERROR_BODY_LIMIT_BYTES).catch(
277-
() => "",
273+
// Overflow cancels the stream and releases its reader lock before the guarded fetch below.
274+
const body = await readResponseWithLimit(res, BROWSER_ERROR_BODY_LIMIT_BYTES).catch(
275+
() => undefined,
278276
);
277+
const text = body ? new TextDecoder().decode(body) : "";
279278
throw new BrowserServiceError(text || `HTTP ${res.status}`);
280279
}
281280
return (await res.json()) as T;

0 commit comments

Comments
 (0)