Skip to content

Commit 9241b97

Browse files
Alix-007Alix-007
authored
fix(mattermost): bound successful REST JSON/text response reads (#96033)
* fix(mattermost): bound successful REST JSON/text response reads The Mattermost REST client already bounds error bodies (readResponseTextLimited) and streams guarded responses without buffering, but the success path still called `await res.json()` / `await res.text()`, reading the whole body into memory before parsing. A self-hosted or compromised Mattermost server can return an arbitrarily large (or never-terminating, content-length-less) JSON/text body and force the plugin to buffer it unbounded. Read successful JSON through the shared readProviderJsonResponse (16 MiB cap, cancels the stream and throws a bounded error on overflow, same as the provider HTTP path) and cap non-JSON success bodies with readResponseTextLimited. uploadMattermostFile's file-info JSON is bounded the same way. Symmetric follow-up to the #95103 / #95108 response-limit campaign. AI-assisted. * fix(mattermost): bound probe success JSON reads * fix(mattermost): reject oversized success text bodies
1 parent b5e9179 commit 9241b97

4 files changed

Lines changed: 121 additions & 5 deletions

File tree

‎extensions/mattermost/src/mattermost/client.test.ts‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -201,6 +201,68 @@ describe("createMattermostClient", () => {
201201
expect(release).toHaveBeenCalledTimes(1);
202202
});
203203

204+
it("bounds and cancels oversized guarded Mattermost success JSON bodies", async () => {
205+
const release = vi.fn(async () => {});
206+
let canceled = false;
207+
let pulled = 0;
208+
const oversizeChunk = new Uint8Array(2 * 1024 * 1024).fill(0x7b); // 2 MiB of '{'
209+
const stream = new ReadableStream<Uint8Array>({
210+
pull(controller) {
211+
pulled += 1;
212+
// Flood far past the 16 MiB JSON cap; an unbounded reader would buffer
213+
// the whole stream before parsing.
214+
controller.enqueue(oversizeChunk);
215+
},
216+
cancel() {
217+
canceled = true;
218+
},
219+
});
220+
fetchWithSsrFGuardMock.mockResolvedValueOnce({
221+
response: new Response(stream, {
222+
status: 200,
223+
headers: { "content-type": "application/json" },
224+
}),
225+
release,
226+
});
227+
const client = createMattermostClient({
228+
baseUrl: "https://chat.example.com",
229+
botToken: "test-token",
230+
});
231+
232+
let caught: Error | undefined;
233+
try {
234+
await client.request("/users/me");
235+
} catch (error) {
236+
caught = error as Error;
237+
}
238+
239+
expect(caught?.message).toContain("JSON response exceeds 16777216 bytes");
240+
// The reader is cancelled at the cap instead of draining the flood: ~8
241+
// chunks of 2 MiB reach the 16 MiB ceiling, never the unbounded tail.
242+
expect(canceled).toBe(true);
243+
expect(pulled).toBeLessThanOrEqual(12);
244+
expect(release).toHaveBeenCalledTimes(1);
245+
});
246+
247+
it("rejects oversized guarded Mattermost success text bodies instead of truncating", async () => {
248+
const release = vi.fn(async () => {});
249+
const tracked = cancelTrackedResponse(`${"plain success ".repeat(7000)}tail`, {
250+
status: 200,
251+
headers: { "content-type": "text/plain" },
252+
});
253+
fetchWithSsrFGuardMock.mockResolvedValueOnce({ response: tracked.response, release });
254+
const client = createMattermostClient({
255+
baseUrl: "https://chat.example.com",
256+
botToken: "test-token",
257+
});
258+
259+
await expect(client.request("/users/me")).rejects.toThrow(
260+
"Mattermost API /users/me: text response exceeds 65536 bytes",
261+
);
262+
expect(tracked.wasCanceled()).toBe(true);
263+
expect(release).toHaveBeenCalledTimes(1);
264+
});
265+
204266
it("releases guarded Mattermost responses when upstream body reads fail", async () => {
205267
const release = vi.fn(async () => {});
206268
const stream = new ReadableStream<Uint8Array>({

‎extensions/mattermost/src/mattermost/client.ts‎

Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,10 @@
11
// Mattermost plugin module implements client behavior.
22
import { resolveTimerTimeoutMs } from "openclaw/plugin-sdk/number-runtime";
3-
import { readResponseTextLimited } from "openclaw/plugin-sdk/provider-http";
3+
import {
4+
readProviderJsonResponse,
5+
readResponseTextLimited,
6+
} from "openclaw/plugin-sdk/provider-http";
7+
import { readResponseWithLimit } from "openclaw/plugin-sdk/response-limit-runtime";
48
import { sleep } from "openclaw/plugin-sdk/runtime-env";
59
import {
610
fetchWithSsrFGuard,
@@ -13,6 +17,13 @@ import {
1317
import { z } from "zod";
1418

1519
const MATTERMOST_ERROR_BODY_LIMIT_BYTES = 8 * 1024;
20+
// Mattermost REST control-plane JSON (posts, users, channels, file-upload
21+
// results) stays well under a megabyte; cap successful JSON the same way the
22+
// shared provider path is capped so an untrusted/self-hosted homeserver cannot
23+
// stream an unbounded body into the runtime before parsing.
24+
// Non-JSON success bodies are a rare fallback (the API is JSON-first); keep a
25+
// generous text budget but still bound it instead of buffering the whole stream.
26+
const MATTERMOST_TEXT_RESPONSE_LIMIT_BYTES = 64 * 1024;
1627
const NULL_BODY_STATUSES = new Set([101, 204, 205, 304]);
1728

1829
export type MattermostFetch = (input: RequestInfo | URL, init?: RequestInit) => Promise<Response>;
@@ -84,6 +95,14 @@ function buildMattermostApiUrl(baseUrl: string, path: string): string {
8495
return `${normalized}/api/v4${suffix}`;
8596
}
8697

98+
async function readMattermostSuccessText(res: Response, path: string): Promise<string> {
99+
const bytes = await readResponseWithLimit(res, MATTERMOST_TEXT_RESPONSE_LIMIT_BYTES, {
100+
onOverflow: ({ maxBytes }) =>
101+
new Error(`Mattermost API ${path}: text response exceeds ${maxBytes} bytes`),
102+
});
103+
return new TextDecoder().decode(bytes);
104+
}
105+
87106
export async function readMattermostError(res: Response): Promise<string> {
88107
const contentType = res.headers.get("content-type") ?? "";
89108
if (!res.body) {
@@ -214,9 +233,9 @@ export function createMattermostClient(params: {
214233

215234
const contentType = res.headers.get("content-type") ?? "";
216235
if (contentType.includes("application/json")) {
217-
return (await res.json()) as T;
236+
return await readProviderJsonResponse<T>(res, `Mattermost API ${path}`);
218237
}
219-
return (await res.text()) as T;
238+
return (await readMattermostSuccessText(res, path)) as T;
220239
};
221240

222241
return { baseUrl, apiBaseUrl, token, request, fetchImpl };
@@ -679,7 +698,10 @@ export async function uploadMattermostFile(
679698
const detail = await readMattermostError(res);
680699
throw new Error(`Mattermost API ${res.status} ${res.statusText}: ${detail || "unknown error"}`);
681700
}
682-
const data = (await res.json()) as { file_infos?: MattermostFileInfo[] };
701+
const data = await readProviderJsonResponse<{ file_infos?: MattermostFileInfo[] }>(
702+
res,
703+
"Mattermost API /files",
704+
);
683705
const info = data.file_infos?.[0];
684706
if (!info?.id) {
685707
throw new Error("Mattermost file upload failed");

‎extensions/mattermost/src/mattermost/probe.test.ts‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,37 @@ describe("probeMattermost", () => {
7474
expect(mockRelease).toHaveBeenCalledTimes(1);
7575
});
7676

77+
it("bounds and cancels oversized probe success JSON bodies", async () => {
78+
let canceled = false;
79+
let pulled = 0;
80+
const oversizeChunk = new Uint8Array(2 * 1024 * 1024).fill(0x7b); // 2 MiB of "{"
81+
const stream = new ReadableStream<Uint8Array>({
82+
pull(controller) {
83+
pulled += 1;
84+
controller.enqueue(oversizeChunk);
85+
},
86+
cancel() {
87+
canceled = true;
88+
},
89+
});
90+
mockFetchGuard.mockResolvedValueOnce({
91+
response: new Response(stream, {
92+
status: 200,
93+
headers: { "content-type": "application/json" },
94+
}),
95+
release: mockRelease,
96+
});
97+
98+
const result = await probeMattermost("https://mm.example.com", "bot-token");
99+
100+
expect(result.ok).toBe(false);
101+
expect(result.status).toBeNull();
102+
expect(result.error).toContain("JSON response exceeds 16777216 bytes");
103+
expect(canceled).toBe(true);
104+
expect(pulled).toBeLessThanOrEqual(12);
105+
expect(mockRelease).toHaveBeenCalledTimes(1);
106+
});
107+
77108
it("forwards allowPrivateNetwork to the SSRF guard policy", async () => {
78109
mockFetchGuard.mockResolvedValueOnce({
79110
response: new Response(JSON.stringify({ id: "bot-1" }), {

‎extensions/mattermost/src/mattermost/probe.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// Mattermost plugin module implements probe behavior.
22
import { formatErrorMessage } from "openclaw/plugin-sdk/error-runtime";
33
import { resolveTimerTimeoutMs } from "openclaw/plugin-sdk/number-runtime";
4+
import { readProviderJsonResponse } from "openclaw/plugin-sdk/provider-http";
45
import {
56
fetchWithSsrFGuard,
67
ssrfPolicyFromPrivateNetworkOptIn,
@@ -53,7 +54,7 @@ export async function probeMattermost(
5354
elapsedMs,
5455
};
5556
}
56-
const bot = (await res.json()) as MattermostUser;
57+
const bot = await readProviderJsonResponse<MattermostUser>(res, "Mattermost probe /users/me");
5758
return {
5859
ok: true,
5960
status: res.status,

0 commit comments

Comments
 (0)