Skip to content

Commit 5f86c3a

Browse files
Alix-007Alix-007
authored
fix(qa-matrix): bound homeserver response reads
Bounds Matrix QA homeserver JSON and media upload response reads with existing response-limit helpers.
1 parent e4e4b01 commit 5f86c3a

4 files changed

Lines changed: 274 additions & 6 deletions

File tree

‎extensions/qa-matrix/src/substrate/client.test.ts‎

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -384,6 +384,84 @@ describe("matrix driver client", () => {
384384
expect(messageBody["m.mentions"]?.user_ids).toEqual(["@sut:matrix-qa.test"]);
385385
});
386386

387+
it("fails closed when the media upload response streams an over-cap body", async () => {
388+
// Sibling coverage to requestMatrixJson: the /_matrix/media/v3/upload
389+
// response is also parsed from an external homeserver, so an oversized
390+
// upload body must trip the same 16 MiB cap and cancel the stream rather
391+
// than buffering it whole and OOMing the QA runner.
392+
const chunkSize = 1024 * 1024;
393+
const chunkCount = 32; // 32 MiB total, past the 16 MiB cap
394+
let reads = 0;
395+
let canceled = false;
396+
const encoder = new TextEncoder();
397+
const fetchImpl: typeof fetch = async () =>
398+
new Response(
399+
new ReadableStream<Uint8Array>({
400+
pull(controller) {
401+
reads += 1;
402+
controller.enqueue(encoder.encode("a".repeat(chunkSize)));
403+
if (reads >= chunkCount) {
404+
controller.close();
405+
}
406+
},
407+
cancel() {
408+
canceled = true;
409+
},
410+
}),
411+
{ status: 200, headers: { "content-type": "application/json" } },
412+
);
413+
414+
const client = createMatrixQaClient({
415+
accessToken: "token",
416+
baseUrl: "http://127.0.0.1:28008/",
417+
fetchImpl,
418+
});
419+
420+
await expect(
421+
client.sendMediaMessage({
422+
body: "@sut:matrix-qa.test Image understanding check",
423+
buffer: Buffer.from("png-bytes"),
424+
contentType: "image/png",
425+
fileName: "huge.png",
426+
kind: "image",
427+
mentionUserIds: ["@sut:matrix-qa.test"],
428+
roomId: "!room:matrix-qa.test",
429+
}),
430+
).rejects.toThrow(/Matrix homeserver response exceeds 16777216 bytes/);
431+
432+
expect(canceled).toBe(true);
433+
expect(reads).toBeLessThan(chunkCount);
434+
});
435+
436+
it("still tolerates malformed in-bounds media upload JSON", async () => {
437+
// Malformed-but-in-bounds upload bodies fall back to `{}`, so the upload
438+
// surfaces the pre-existing "did not return content_uri" error rather than
439+
// a parse crash — unchanged from before the bound was added.
440+
const fetchImpl: typeof fetch = async () =>
441+
new Response("{ not json", {
442+
status: 200,
443+
headers: { "content-type": "application/json" },
444+
});
445+
446+
const client = createMatrixQaClient({
447+
accessToken: "token",
448+
baseUrl: "http://127.0.0.1:28008/",
449+
fetchImpl,
450+
});
451+
452+
await expect(
453+
client.sendMediaMessage({
454+
body: "@sut:matrix-qa.test Image understanding check",
455+
buffer: Buffer.from("png-bytes"),
456+
contentType: "image/png",
457+
fileName: "bad.png",
458+
kind: "image",
459+
mentionUserIds: ["@sut:matrix-qa.test"],
460+
roomId: "!room:matrix-qa.test",
461+
}),
462+
).rejects.toThrow("Matrix media upload did not return content_uri.");
463+
});
464+
387465
it("adds Matrix room encryption state when provisioning encrypted QA rooms", async () => {
388466
const createRoomBodies: Array<Record<string, unknown>> = [];
389467
const fetchImpl: typeof fetch = async (input, init) => {

‎extensions/qa-matrix/src/substrate/client.ts‎

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,10 @@
22
import { randomUUID } from "node:crypto";
33
import { setTimeout as sleep } from "node:timers/promises";
44
import { formatErrorMessage } from "openclaw/plugin-sdk/error-runtime";
5+
import { readResponseWithLimit } from "openclaw/plugin-sdk/response-limit-runtime";
56
import { uniqueStrings, uniqueValues } from "openclaw/plugin-sdk/string-coerce-runtime";
67
import type { MatrixQaObservedEvent } from "./events.js";
7-
import { requestMatrixJson, type MatrixQaFetchLike } from "./request.js";
8+
import { MATRIX_QA_JSON_MAX_BYTES, requestMatrixJson, type MatrixQaFetchLike } from "./request.js";
89
import {
910
createMatrixQaRoomObserver,
1011
primeMatrixQaRoom,
@@ -339,10 +340,22 @@ async function uploadMatrixQaContent(params: {
339340
body: uploadBody,
340341
signal: AbortSignal.timeout(20_000),
341342
});
342-
const body = (await response.json().catch(() => ({}))) as {
343-
content_uri?: string;
344-
error?: string;
345-
};
343+
// Bound the media-upload response body before parsing, mirroring
344+
// `requestMatrixJson`. The overflow error is read *outside* the parse
345+
// try/catch so it fails closed (propagates) instead of being swallowed into
346+
// `{}`; malformed-but-in-bounds JSON still falls back to `{}` as before.
347+
const uploadBytes = await readResponseWithLimit(response, MATRIX_QA_JSON_MAX_BYTES, {
348+
onOverflow: ({ maxBytes }) => new Error(`Matrix homeserver response exceeds ${maxBytes} bytes`),
349+
});
350+
let body: { content_uri?: string; error?: string };
351+
try {
352+
body = JSON.parse(new TextDecoder().decode(uploadBytes)) as {
353+
content_uri?: string;
354+
error?: string;
355+
};
356+
} catch {
357+
body = {};
358+
}
346359
if (response.status !== 200) {
347360
throw new Error(body.error ?? `Matrix media upload failed with status ${response.status}`);
348361
}

‎extensions/qa-matrix/src/substrate/request.test.ts‎

Lines changed: 164 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,4 +24,168 @@ describe("requestMatrixJson", () => {
2424
expect(timeoutSpy).toHaveBeenCalledWith(MAX_TIMER_TIMEOUT_MS);
2525
expect(fetchImpl).toHaveBeenCalledWith(expect.any(URL), expect.objectContaining({ signal }));
2626
});
27+
28+
it("fails closed when the homeserver streams an over-cap response body", async () => {
29+
// Stream past the 16 MiB cap one chunk at a time so the fixture proves the
30+
// bound trips on the prefix and cancels the body instead of buffering it
31+
// all. `cancel()` flipping `canceled` is what real-behavior fail-closed
32+
// looks like: the stream is torn down, not drained.
33+
const chunkSize = 1024 * 1024;
34+
const chunkCount = 32; // 32 MiB total, well past the 16 MiB limit
35+
let reads = 0;
36+
let canceled = false;
37+
const encoder = new TextEncoder();
38+
const stream = new ReadableStream<Uint8Array>({
39+
pull(controller) {
40+
reads += 1;
41+
controller.enqueue(encoder.encode("a".repeat(chunkSize)));
42+
if (reads >= chunkCount) {
43+
controller.close();
44+
}
45+
},
46+
cancel() {
47+
canceled = true;
48+
},
49+
});
50+
const fetchImpl = vi.fn<MatrixQaFetchLike>(
51+
async () =>
52+
new Response(stream, {
53+
status: 200,
54+
headers: { "content-type": "application/json" },
55+
}),
56+
);
57+
58+
await expect(
59+
requestMatrixJson({
60+
baseUrl: "https://matrix.example.test",
61+
endpoint: "/_matrix/client/v3/sync",
62+
fetchImpl,
63+
method: "GET",
64+
}),
65+
).rejects.toThrow(/Matrix homeserver response exceeds 16777216 bytes/);
66+
67+
// Fail-closed proof: the read stopped before draining all 32 chunks and the
68+
// stream was canceled rather than fully buffered.
69+
expect(canceled).toBe(true);
70+
expect(reads).toBeLessThan(chunkCount);
71+
});
72+
73+
it("rejects an oversized error-status body instead of buffering it whole", async () => {
74+
// Even on a non-2xx status the cap must trip first: an attacker controlling
75+
// the homeserver could otherwise return a 500 with a multi-GiB body knowing
76+
// the helper only inspects `body.error` after fully reading it.
77+
const chunkSize = 1024 * 1024;
78+
const chunkCount = 32;
79+
let reads = 0;
80+
let canceled = false;
81+
const encoder = new TextEncoder();
82+
const stream = new ReadableStream<Uint8Array>({
83+
pull(controller) {
84+
reads += 1;
85+
controller.enqueue(encoder.encode("b".repeat(chunkSize)));
86+
if (reads >= chunkCount) {
87+
controller.close();
88+
}
89+
},
90+
cancel() {
91+
canceled = true;
92+
},
93+
});
94+
const fetchImpl = vi.fn<MatrixQaFetchLike>(
95+
async () =>
96+
new Response(stream, {
97+
status: 500,
98+
headers: { "content-type": "application/json" },
99+
}),
100+
);
101+
102+
await expect(
103+
requestMatrixJson({
104+
baseUrl: "https://matrix.example.test",
105+
endpoint: "/_matrix/client/v3/sync",
106+
fetchImpl,
107+
method: "GET",
108+
}),
109+
).rejects.toThrow(/Matrix homeserver response exceeds 16777216 bytes/);
110+
expect(canceled).toBe(true);
111+
expect(reads).toBeLessThan(chunkCount);
112+
});
113+
114+
it("still falls back to an empty body for malformed in-bounds JSON", async () => {
115+
const fetchImpl = vi.fn<MatrixQaFetchLike>(
116+
async () =>
117+
new Response("{ not valid json", {
118+
status: 200,
119+
headers: { "content-type": "application/json" },
120+
}),
121+
);
122+
123+
const result = await requestMatrixJson<{ ok?: boolean }>({
124+
baseUrl: "https://matrix.example.test",
125+
endpoint: "/_matrix/client/v3/account/whoami",
126+
fetchImpl,
127+
method: "GET",
128+
});
129+
130+
expect(result.status).toBe(200);
131+
expect(result.body).toEqual({});
132+
});
133+
134+
it("treats an empty in-bounds body as an empty object", async () => {
135+
const fetchImpl = vi.fn<MatrixQaFetchLike>(
136+
async () =>
137+
new Response("", {
138+
status: 200,
139+
headers: { "content-type": "application/json" },
140+
}),
141+
);
142+
143+
const result = await requestMatrixJson<Record<string, unknown>>({
144+
baseUrl: "https://matrix.example.test",
145+
endpoint: "/_matrix/client/v3/account/whoami",
146+
fetchImpl,
147+
method: "GET",
148+
});
149+
150+
expect(result.status).toBe(200);
151+
expect(result.body).toEqual({});
152+
});
153+
154+
it("reads a normal in-bounds JSON body unchanged", async () => {
155+
const fetchImpl = vi.fn<MatrixQaFetchLike>(async () => Response.json({ user_id: "@qa:test" }));
156+
157+
const result = await requestMatrixJson<{ user_id: string }>({
158+
baseUrl: "https://matrix.example.test",
159+
endpoint: "/_matrix/client/v3/account/whoami",
160+
fetchImpl,
161+
method: "GET",
162+
});
163+
164+
expect(result.status).toBe(200);
165+
expect(result.body).toEqual({ user_id: "@qa:test" });
166+
});
167+
168+
it("reads an in-bounds body just under the cap without tripping the bound", async () => {
169+
// Boundary guard: a body close to but under 16 MiB must parse normally so
170+
// the cap does not regress legitimate large-but-valid Matrix responses.
171+
const filler = "x".repeat(8 * 1024 * 1024); // 8 MiB string, well under cap
172+
const payload = JSON.stringify({ data: filler });
173+
const fetchImpl = vi.fn<MatrixQaFetchLike>(
174+
async () =>
175+
new Response(payload, {
176+
status: 200,
177+
headers: { "content-type": "application/json" },
178+
}),
179+
);
180+
181+
const result = await requestMatrixJson<{ data: string }>({
182+
baseUrl: "https://matrix.example.test",
183+
endpoint: "/_matrix/client/v3/sync",
184+
fetchImpl,
185+
method: "GET",
186+
});
187+
188+
expect(result.status).toBe(200);
189+
expect(result.body.data).toHaveLength(filler.length);
190+
});
27191
});

‎extensions/qa-matrix/src/substrate/request.ts‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,14 @@
11
// Qa Matrix plugin module implements request behavior.
22
import { resolveTimerTimeoutMs } from "openclaw/plugin-sdk/number-runtime";
3+
import { readResponseWithLimit } from "openclaw/plugin-sdk/response-limit-runtime";
34

45
export type MatrixQaFetchLike = typeof fetch;
56

7+
// Cap how much of a Matrix homeserver response we buffer so a hostile or
8+
// misbehaving server cannot drive this process OOM with an unbounded body.
9+
// Shared across the QA substrate (also reused for media-upload reads in client.ts).
10+
export const MATRIX_QA_JSON_MAX_BYTES = 16 * 1024 * 1024;
11+
612
type MatrixQaRequestResult<T> = {
713
status: number;
814
body: T;
@@ -35,9 +41,16 @@ export async function requestMatrixJson<T>(params: {
3541
...(params.body !== undefined ? { body: JSON.stringify(params.body) } : {}),
3642
signal: AbortSignal.timeout(resolveTimerTimeoutMs(params.timeoutMs, 20_000)),
3743
});
44+
// Read under a byte cap *before* the parse try/catch. The overflow error must
45+
// escape uncaught (fail-closed): swallowing it into `body = {}` would defeat
46+
// the bound and silently accept an oversized payload. Malformed but
47+
// in-bounds JSON still falls back to `{}` exactly as before.
48+
const bytes = await readResponseWithLimit(response, MATRIX_QA_JSON_MAX_BYTES, {
49+
onOverflow: ({ maxBytes }) => new Error(`Matrix homeserver response exceeds ${maxBytes} bytes`),
50+
});
3851
let body: unknown;
3952
try {
40-
body = (await response.json()) as unknown;
53+
body = JSON.parse(new TextDecoder().decode(bytes)) as unknown;
4154
} catch {
4255
body = {};
4356
}

0 commit comments

Comments
 (0)