fix(signal): bound container REST reads so a hostile signal-cli-rest-api host cannot exhaust memory

Alix-007 · Alix-007 · commit d75b9ea642bc · 2026-06-29T10:11:49.000+08:00
diff --git a/extensions/signal/src/client-container.test.ts b/extensions/signal/src/client-container.test.ts
@@ -17,6 +17,23 @@ import {
 
 // spyOn approach works with vitest forks pool for cross-directory imports
 const mockFetch = vi.fn();
+
+// Build a Response-like `body` stream from a string so the production code exercises the
+// bounded body readers (readProviderTextResponse / readResponseTextLimited) instead of an
+// unbounded res.text(). Kept local to this file to avoid touching shared HTTP test mocks.
+function bodyStream(text: string): { body: ReadableStream<Uint8Array> } {
+  const bytes = new TextEncoder().encode(text);
+  return {
+    body: new ReadableStream<Uint8Array>({
+      start(controller) {
+        if (bytes.byteLength > 0) {
+          controller.enqueue(bytes);
+        }
+        controller.close();
+      },
+    }),
+  };
+}
 const wsMockState = vi.hoisted(() => ({
   behavior: "close" as "close" | "open" | "error" | "unexpected-response",
   urls: [] as string[],
@@ -223,7 +240,7 @@ describe("containerRestRequest", () => {
     mockFetch.mockResolvedValue({
       ok: true,
       status: 200,
-      text: async () => JSON.stringify({ version: "1.0" }),
+      ...bodyStream(JSON.stringify({ version: "1.0" })),
     });
 
     const result = await containerRestRequest("/v1/about", { baseUrl: "http://localhost:8080" });
@@ -236,7 +253,7 @@ describe("containerRestRequest", () => {
     mockFetch.mockResolvedValue({
       ok: true,
       status: 201,
-      text: async () => "",
+      ...bodyStream(""),
     });
 
     await containerRestRequest("/v2/send", { baseUrl: "http://localhost:8080" }, "POST", {
@@ -259,7 +276,7 @@ describe("containerRestRequest", () => {
     mockFetch.mockResolvedValue({
       ok: true,
       status: 201,
-      text: async () => JSON.stringify({ timestamp: 1700000000000 }),
+      ...bodyStream(JSON.stringify({ timestamp: 1700000000000 })),
     });
 
     const result = await containerRestRequest(
@@ -289,7 +306,7 @@ describe("containerRestRequest", () => {
       ok: false,
       status: 500,
       statusText: "Internal Server Error",
-      text: async () => "Server error details",
+      ...bodyStream("Server error details"),
     });
 
     await expect(
@@ -301,7 +318,7 @@ describe("containerRestRequest", () => {
     mockFetch.mockResolvedValue({
       ok: true,
       status: 200,
-      text: async () => "",
+      ...bodyStream(""),
     });
 
     const result = await containerRestRequest("/v1/about", { baseUrl: "http://localhost:8080" });
@@ -312,7 +329,7 @@ describe("containerRestRequest", () => {
     mockFetch.mockResolvedValue({
       ok: true,
       status: 200,
-      text: async () => "{}",
+      ...bodyStream("{}"),
     });
 
     await containerRestRequest("/v1/about", { baseUrl: "http://localhost:8080", timeoutMs: 5000 });
@@ -332,7 +349,7 @@ describe("containerRestRequest", () => {
       mockFetch.mockResolvedValue({
         ok: true,
         status: 200,
-        text: async () => "{}",
+        ...bodyStream("{}"),
       });
 
       await containerRestRequest("/v1/about", {
@@ -357,7 +374,7 @@ describe("containerSendMessage", () => {
     mockFetch.mockResolvedValue({
       ok: true,
       status: 200,
-      text: async () => JSON.stringify({ timestamp: "1700000000000" }),
+      ...bodyStream(JSON.stringify({ timestamp: "1700000000000" })),
     });
 
     const result = await containerSendMessage({
@@ -382,7 +399,7 @@ describe("containerSendMessage", () => {
     mockFetch.mockResolvedValue({
       ok: true,
       status: 200,
-      text: async () => JSON.stringify({ timestamp: "not-a-number" }),
+      ...bodyStream(JSON.stringify({ timestamp: "not-a-number" })),
     });
 
     await expect(
@@ -401,7 +418,7 @@ describe("containerSendMessage", () => {
       mockFetch.mockResolvedValue({
         ok: true,
         status: 200,
-        text: async () => JSON.stringify({ timestamp }),
+        ...bodyStream(JSON.stringify({ timestamp })),
       });
 
       await expect(
@@ -419,7 +436,7 @@ describe("containerSendMessage", () => {
     mockFetch.mockResolvedValue({
       ok: true,
       status: 200,
-      text: async () => JSON.stringify({}),
+      ...bodyStream(JSON.stringify({})),
     });
 
     await containerSendMessage({
@@ -440,7 +457,7 @@ describe("containerSendMessage", () => {
     mockFetch.mockResolvedValue({
       ok: true,
       status: 200,
-      text: async () => JSON.stringify({}),
+      ...bodyStream(JSON.stringify({})),
     });
 
     await containerSendMessage({
@@ -459,7 +476,7 @@ describe("containerSendMessage", () => {
     mockFetch.mockResolvedValue({
       ok: true,
       status: 200,
-      text: async () => JSON.stringify({}),
+      ...bodyStream(JSON.stringify({})),
     });
 
     await containerSendMessage({
@@ -488,7 +505,7 @@ describe("containerSendMessage", () => {
     mockFetch.mockResolvedValue({
       ok: true,
       status: 200,
-      text: async () => JSON.stringify({}),
+      ...bodyStream(JSON.stringify({})),
     });
 
     await containerSendMessage({
@@ -799,7 +816,7 @@ describe("containerRestRequest edge cases", () => {
       ok: false,
       status: 500,
       statusText: "Internal Server Error",
-      text: async () => "",
+      ...bodyStream(""),
     });
 
     await expect(
@@ -811,13 +828,103 @@ describe("containerRestRequest edge cases", () => {
     mockFetch.mockResolvedValue({
       ok: true,
       status: 200,
-      text: async () => "not-valid-json",
+      ...bodyStream("not-valid-json"),
     });
 
     await expect(
       containerRestRequest("/v1/about", { baseUrl: "http://localhost:8080" }),
     ).rejects.toThrow();
   });
+
+  it("fails closed when the success body exceeds the response size cap", async () => {
+    // Drive the real bounded reader with a >16 MiB stream. Pull lazily so the cap
+    // (16 MiB) trips and cancels the stream long before 20 MiB is materialized.
+    const ONE_MIB = new Uint8Array(1024 * 1024);
+    let emitted = 0;
+    const stream = new ReadableStream<Uint8Array>({
+      pull(controller) {
+        if (emitted >= 20) {
+          controller.close();
+          return;
+        }
+        emitted += 1;
+        controller.enqueue(ONE_MIB);
+      },
+    });
+    mockFetch.mockResolvedValue({
+      ok: true,
+      status: 200,
+      headers: new Headers(),
+      body: stream,
+    });
+
+    await expect(
+      containerRestRequest("/v1/about", { baseUrl: "http://localhost:8080" }),
+    ).rejects.toThrow(/exceeds \d+ bytes/);
+    // The stream must have been cancelled at the cap, not drained to completion.
+    expect(emitted).toBeLessThan(20);
+  });
+
+  it("bounds the error body so a huge failure response cannot be buffered whole", async () => {
+    const HUGE = "x".repeat(1024 * 1024); // 1 MiB of error text
+    mockFetch.mockResolvedValue({
+      ok: false,
+      status: 500,
+      statusText: "Internal Server Error",
+      ...bodyStream(HUGE),
+    });
+
+    await containerRestRequest("/v2/send", { baseUrl: "http://localhost:8080" }, "POST").then(
+      () => {
+        throw new Error("expected containerRestRequest to reject");
+      },
+      (err: unknown) => {
+        const message = err instanceof Error ? err.message : String(err);
+        expect(message.startsWith("Signal REST 500:")).toBe(true);
+        // readResponseTextLimited truncates the diagnostic body well below the 1 MiB payload.
+        expect(message.length).toBeLessThan(64 * 1024);
+      },
+    );
+  });
+
+  it("parses a large but under-cap success body without truncation", async () => {
+    // Regression guard: a legitimate multi-MiB JSON response (well under the 16 MiB
+    // cap) must still be read in full and parsed intact — the bound must not clip
+    // valid container payloads. Build ~4 MiB of real JSON.
+    const items = Array.from({ length: 50_000 }, (_, i) => ({
+      id: i,
+      note: "signal-container-payload-entry",
+    }));
+    const payload = JSON.stringify({ items });
+    expect(payload.length).toBeGreaterThan(2 * 1024 * 1024);
+    expect(payload.length).toBeLessThan(16 * 1024 * 1024);
+    mockFetch.mockResolvedValue({
+      ok: true,
+      status: 200,
+      ...bodyStream(payload),
+    });
+
+    const result = await containerRestRequest<{ items: Array<{ id: number }> }>("/v1/about", {
+      baseUrl: "http://localhost:8080",
+    });
+    // Full body round-trips: first and last entries survive, count is exact.
+    expect(result.items).toHaveLength(50_000);
+    expect(result.items[0]?.id).toBe(0);
+    expect(result.items[49_999]?.id).toBe(49_999);
+  });
+
+  it("returns undefined for an empty success body via the bounded reader", async () => {
+    // The bounded reader must preserve the existing empty-body -> undefined contract
+    // (no spurious JSON.parse("") throw) so well-behaved 200/empty responses are unchanged.
+    mockFetch.mockResolvedValue({
+      ok: true,
+      status: 200,
+      ...bodyStream(""),
+    });
+
+    const result = await containerRestRequest("/v1/about", { baseUrl: "http://localhost:8080" });
+    expect(result).toBeUndefined();
+  });
 });
 
 describe("streamContainerEvents", () => {
@@ -869,7 +976,7 @@ describe("containerSendReaction", () => {
     mockFetch.mockResolvedValue({
       ok: true,
       status: 200,
-      text: async () => JSON.stringify({ timestamp: 1700000000000 }),
+      ...bodyStream(JSON.stringify({ timestamp: 1700000000000 })),
     });
 
     const result = await containerSendReaction({
@@ -905,7 +1012,7 @@ describe("containerRpcRequest reactions", () => {
     mockFetch.mockResolvedValue({
       ok: true,
       status: 200,
-      text: async () => JSON.stringify({}),
+      ...bodyStream(JSON.stringify({})),
     });
 
     await containerRpcRequest(
@@ -937,7 +1044,7 @@ describe("containerRemoveReaction", () => {
     mockFetch.mockResolvedValue({
       ok: true,
       status: 200,
-      text: async () => JSON.stringify({ timestamp: 1700000000000 }),
+      ...bodyStream(JSON.stringify({ timestamp: 1700000000000 })),
     });
 
     const result = await containerRemoveReaction({
diff --git a/extensions/signal/src/client-container.ts b/extensions/signal/src/client-container.ts
@@ -14,6 +14,10 @@ import {
   parseStrictNonNegativeInteger,
   resolveTimerTimeoutMs,
 } from "openclaw/plugin-sdk/number-runtime";
+import {
+  readProviderTextResponse,
+  readResponseTextLimited,
+} from "openclaw/plugin-sdk/provider-http";
 import { readResponseWithLimit } from "openclaw/plugin-sdk/response-limit-runtime";
 import WebSocket from "ws";
 
@@ -241,11 +245,16 @@ export async function containerRestRequest<T = unknown>(
   }
 
   if (!res.ok) {
-    const errorText = await res.text().catch(() => "");
+    // Bound the error body: signal-cli-rest-api is an untrusted external container,
+    // and a hostile/buggy response must not let an error path buffer an unbounded body.
+    const errorText = await readResponseTextLimited(res).catch(() => "");
     throw new Error(`Signal REST ${res.status}: ${errorText || res.statusText}`);
   }
 
-  const text = await res.text();
+  // Bound the success body under the shared 16 MiB provider cap before JSON.parse so a
+  // malicious/runaway container response cannot OOM the runtime (send/typing/version all
+  // funnel through here). Reuse the same bounded reader family as the attachment path.
+  const text = await readProviderTextResponse(res, "Signal REST");
   if (!text) {
     return undefined as T;
   }
