fix(signal): bound container REST reads so a hostile signal-cli-rest-api host cannot exhaust memory#97539
Conversation
|
Codex review: needs maintainer review before merge. Reviewed June 28, 2026, 10:25 PM ET / 02:25 UTC. Summary PR surface: Source +9, Tests +107. Total +116 across 2 files. Reproducibility: yes. Source inspection shows current main and Review metrics: 1 noteworthy metric.
Stored data model Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Merge the central bounded-read fix after maintainers accept the shared 16 MiB fail-closed cap for pathological Signal container REST responses. Do we have a high-confidence way to reproduce the issue? Yes. Source inspection shows current main and Is this the best way to solve the issue? Yes. Fixing the central Signal container REST helper with existing bounded readers is narrower and more maintainable than patching each caller or inventing Signal-specific read machinery; the remaining question is maintainer acceptance of the cap. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 63fe5c74021d. Label changesLabel justifications:
Evidence reviewedPR surface: Source +9, Tests +107. Total +116 across 2 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
clawsweeper
Bot
added
rating: 🦪 silver shellfish
b9e7e7e to
01197e6
Compare
Alix-007
changed the title
clawsweeper
Bot
added
proof: sufficient
…api host cannot exhaust memory
01197e6 to
d75b9ea
Compare
2 participants
AI-assisted: implemented with Claude Code (Anthropic). Degree: Claude Code wrote the source change, the tests, and a standalone real-HTTP proof harness under human direction; a human reviewed the diff, ran the focused test suite, the type-check, and the real-behavior proof (including a load-bearing mutation/negative-control), and can explain each line. Session prompt: bound the central Signal container RPC reads against OOM by reusing existing repo helpers, then prove it against a real HTTP server with a negative control, without touching any shared test mocks or shared infra.
What Problem This Solves
Fixes an issue where an operator running OpenClaw's Signal channel in container mode (the bbernhard
signal-cli-rest-apicontainer) could have the OpenClaw runtime's memory exhausted by a single oversized or runaway HTTP response from that container.The Signal container is an external, user-configured HTTP dependency that OpenClaw does not control. The central RPC helper
containerRestRequestread both the error body and the success body withres.text(), which buffers the entire response into memory before parsing — with no byte ceiling, even when the response carries noContent-Length. A hostile, compromised, or merely buggy container that streams an unbounded body could push the runtime into an out-of-memory condition. Becausesend,sendTyping,sendReceipt,sendReaction/removeReaction, andversionall funnel throughcontainerRestRequest, every container-mode Signal operation was exposed — not just attachment downloads, which were already bounded.Why This Change Was Made
Both reads in
containerRestRequestare now bounded with the same readers already used elsewhere in this file and across the provider stack, so the fix adds no new size-limit machinery:readProviderTextResponse(res, "Signal REST")— reads under the shared 16 MiB provider cap (PROVIDER_TEXT_RESPONSE_MAX_BYTES) and fails closed (rejects, cancelling the stream) on overflow. The existing empty-body →undefinedandJSON.parsesemantics are preserved exactly.readResponseTextLimited(res)— reads only a bounded diagnostic prefix for the thrown error message instead of the full body. This matches how the shared provider error path and other extensions (openrouter/chutes/minimax/qqbot) already read error bodies.This mirrors the attachment path in the same file (
readCappedResponseBuffer→readResponseWithLimit) and the JSON readers in the recent response-limit campaign (#95103/#96495/#96873). Non-goal: this PR does not change protocol behavior, timeouts, or the 16 MiB cap value; it only changes how the body bytes are read.Compatibility (please confirm the tradeoff): No public API, config, or behavior change for well-behaved containers — same return values, same error strings, same
undefined-on-empty andJSON.parse-on-malformed behavior, and legitimate large responses (verified up to ~4 MiB and proven against the live cap) still read in full. The only observable difference is that a pathologically large body (> 16 MiB) now fails the single affected operation with a bounded error instead of buffering unbounded. This is the deliberate fail-closed tradeoff for the untrusted container boundary; maintainers are asked to accept it. No schema, CLI, or migration impact (the test-only.test.tschange is not a data-model/serialized-state change).User Impact
Operators running the Signal channel in container mode are no longer exposed to an OOM/denial-of-service vector from their
signal-cli-rest-apiendpoint. A malformed or oversized response now fails the single affected operation with a clear bounded error instead of risking the whole runtime. No action required; existing setups behave identically for normal responses.Security & Privacy: defense-in-depth hardening of an untrusted external boundary (the container response); reduces a memory-exhaustion DoS surface. No new data is logged or transmitted — the error path now logs less (a bounded prefix rather than the full body). No secrets or PII handling changes.
Blast radius: low. Single source file + its own co-located test file; no shared SDK, no shared HTTP test mocks, no shared infra touched. Companion to the merged
#96495/#96873response-limit work, reusing the exact same readers.Evidence
1) Real-behavior proof (live HTTP, end-to-end, no mocks)
A real
node:httpserver (createServer) bound to127.0.0.1streams a body with noContent-Lengthin 64 KiB chunks, driving the real exportedcontainerRestRequestend-to-end over a real socket on Node v22.22.0 (onlyfetch/undici is real; nothing is mocked). It includes a negative control running the OLDawait res.text()path against the same body:Signal REST: text response exceeds 16777216 bytes; the server is cancelled after ~18 MiB instead of draining the full 24 MiB body.res.text()buffers the full 25,165,824 bytes (>23× the bounded read), proving the cap is what stops the OOM.2) Mutation test (proves the tests/fix are load-bearing)
Reverting the success path to
const text = await res.text()(fail-open) makes the real-HTTP proof reportFAILURES: 3and the focused Vitest suite go red (20 failed, including fails closed when the success body exceeds the response size cap and parses a large but under-cap success body without truncation). Restoring the fix returns to all-green. The guards are not vacuous.3) Focused test suite
The tests drive the real
containerRestRequest(the readers run real; onlyfetchis mocked via this file's existing local mock plus a localbodyStreamhelper — the shared provider HTTP test mocks were intentionally not modified). Beyond the oversized success/error cases, this adds a large-but-under-cap round-trip guard (~4 MiB valid JSON, exact count preserved) and an empty-body → undefined contract guard:4) Type-check
AI-assisted.