fix(tlon): bound Urbit SSE stream reads at 16 MiB#97687
fix(tlon): bound Urbit SSE stream reads at 16 MiB#97687wangmiao0668000666 wants to merge 1 commit into
Conversation
Urbit SSE client processStream() previously did an unbounded
for-await loop on the channel response body. A hostile or broken
Urbit node could return an unbounded event stream and exhaust
OpenClaw memory before any caller-side guard fires.
Replace the chunked buffer accumulation with readByteStreamWithLimit
from openclaw/plugin-sdk/response-limit-runtime at 16 MiB cap,
matching the JSON/OAuth response cap pattern. Events are then
parsed from the bounded buffer via the existing indexOf("\n\n")
boundary scan, preserving event-dispatch semantics.
The cap fires with a labeled error:
tlon Urbit SSE: body exceeds 16777216 bytes (got <size>)
Companion pattern to src/llm/providers/mistral.ts (openclaw#97648) and
src/llm/providers/anthropic.ts (openclaw#96701/openclaw#96723) for non-extension
provider SSE bounds. This is the first extension-side SSE bound.
Co-Authored-By: Claude <[email protected]>
|
Codex review: needs real behavior proof before merge. Reviewed June 29, 2026, 2:41 AM ET / 06:41 UTC. Summary PR surface: Source +10, Tests +112. Total +122 across 2 files. Reproducibility: yes. Current main’s unbounded accumulation is source-reproducible in Review metrics: 1 noteworthy metric.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Bound bytes incrementally inside Do we have a high-confidence way to reproduce the issue? Yes. Current main’s unbounded accumulation is source-reproducible in Is this the best way to solve the issue? No. A finite whole-body bounded reader is the wrong layer for a long-lived SSE parser; the maintainable fix is an incremental byte cap that preserves streaming dispatch. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 05cf49776766. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +10, Tests +112. Total +122 across 2 files. View PR surface stats
Security concerns:
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: 🧂 unranked krab
|
Closing this PR after rereading the ClawSweeper review, the @extensions/tlon/src/urbit/sse-client.ts:244 finding, and the underlying The reviewer is correct: this change has a fundamental design flaw and should not land in its current shape. What the patch actually does
Why my proof missed it The loopback tests only exercised a server that either writes 18 MiB and closes or writes a finite set of events and closes. Both paths satisfy What the right fix looks like Bind bytes incrementally inside the existing
This is the change the reviewer asked for, and it's a much smaller, much safer patch than this one. Lessons I'm taking forward A bounded-read helper that materializes the entire stream before returning is the right seam for finite-body HTTP responses (OAI / Discord / OAuth) but is the wrong seam for long-lived streaming transports (SSE / WebSocket / event-source). The bounded-read campaign needs to track per-transport helper choice explicitly, not just "use the bounded reader". I'll bring that into the SKILL checklist before opening the next PR. Closing in favor of a maintainer- or contributor-led patch that does the incremental version. Sorry for the cycle; thanks to ClawSweeper for the precise diagnosis and to @arthyn, @steipete, and @vincentkoc for the upstream SSE and bounded-read helper groundwork. 🤖 Generated with Claude Code |
|
Closed in favor of an incremental per-chunk bounded-read implementation. See comment above. |
1 participant
What Problem This Solves
extensions/tlon/src/urbit/sse-client.ts:237(theUrbitSSEClient.processStreambody) reads the Urbit/~/channel/<id>SSE response in an unboundedfor await (const chunk of stream)loop, accumulatingchunk.toString()into a singlebufferuntil the stream completes. A hostile or malfunctioning Urbit node can return an SSE body of arbitrary size — the runtime has no upper bound on memory consumption for this read path. There is no fallback reader that enforces a cap before the consumer sees the bytes.This is the same class of bug closed for core LLM SSE providers in #96701, #96723, #96972, #96989, #96772, #97628, and #97648. This PR closes it for the Tlon Urbit channel's SSE client.
Why This Change Was Made
The bounded-read helper
readByteStreamWithLimitis already exported fromopenclaw/plugin-sdk/response-limit-runtime(re-exported from@openclaw/media-core/read-byte-stream-with-limit). It accepts anAsyncIterable<unknown>(both NodeReadableStreamand the result ofReadable.fromWeb(webStream)qualify), tracks accumulated bytes against a hard cap, throws a labeled error on overflow, anddestroy()s the underlying stream so producers stop sending. The tlon extension already imports other helpers fromopenclaw/plugin-sdk/*(ssrf-runtime,number-runtime) — usingresponse-limit-runtimehere is consistent with the established plugin boundary (no core internals, no relative outside-package imports).The existing chunk-parse loop's
\n\nboundary scan is preserved verbatim after the bounded read — only the data source changes from "streamed chunks" to "bounded Buffer". Event dispatch semantics are identical.The cap of 16 MiB matches the shared
PROVIDER_TEXT_RESPONSE_MAX_BYTES/PROVIDER_JSON_RESPONSE_MAX_BYTESconvention used insrc/agents/provider-http-errors.tsand the cap used in the OAuth bounded-read PRs (#97628, mine).User Impact
tlon Urbit SSE: body exceeds 16777216 bytes (got <size>)if the SSE body exceeds 16 MiB. This is the desired safety behavior — the channel does not silently buffer gigabytes before the caller sees the error.extensions/tlon/src/urbit/sse-client.ts); no API, config, migration, ordocs/**change.Changes
extensions/tlon/src/urbit/sse-client.ts:8— addimport { readByteStreamWithLimit } from "openclaw/plugin-sdk/response-limit-runtime";(alphabetical position).extensions/tlon/src/urbit/sse-client.ts:11— addconst TLON_SSE_BODY_MAX_BYTES = 16 * 1024 * 1024;with WHY-comment naming the cap.extensions/tlon/src/urbit/sse-client.ts:228-272— replaceprocessStream'sfor await (chunk)unbounded accumulator withreadByteStreamWithLimit(stream, {maxBytes: TLON_SSE_BODY_MAX_BYTES, onOverflow: ...})followed by the existing\n\nboundary scan onbuffer.toString("utf8"). WHY-comment explains the 16 MiB cap rationale and points to the sibling bounded-read PRs.extensions/tlon/src/urbit/sse-client.bounded.test.ts— new file, 2 inline tests via loopbackhttp.createServer(see## Evidencefor breakdown).No SDK barrel change, no API/config/migration change, no
docs/**change, nopackage.jsonchange, no sibling-file change.Evidence
This section is a structured, verifiable proof that the change works. All commands and outputs are reproducible from a clean clone of
fix/tlon-sse-boundat the head SHA below.Pre-flight gate
gh search prs --repo openclaw/openclaw --state open "tlon"readProviderJsonResponseintlon-api.ts/channel-ops.ts— does NOT touchsse-client.ts(verified viagh pr view 97558 --json files). No SSE overlap.git diff --numstat HEAD~1extensions/tlon/src/urbit/sse-client.ts +22/-12, new filesse-client.bounded.test.ts +116/-0. Prod LoC +10 / Test LoC +116.Two-layer proof (per
loopback-proof-required-bounded-read.mdmemory)Layer 1: Real-wire loopback proof — oversized body cap fires
A real
http.createServerlistening on127.0.0.1:0streams 18 × 1 MiB chunks viasetInterval(..., 1)withContent-Type: text/event-stream. Real globalfetch()is consumed byUrbitSSEClient.processStream. Cap firing is observed end-to-end on actual TCP framing.Layer 2: Real-wire loopback proof — normal-size body events dispatch
Same loopback pattern with a small SSE-shaped body containing 2 events. Events fire through the registered handler correctly.
Quantitative read of Layer 1
TLON_SSE_BODY_MAX_BYTESMAX < got < TOTAL127.0.0.1produces a fixed-but-non-deterministic reported size after cap\n\nboundary scan consumes the bounded Buffer correctly; event handlers fire as beforeCombined evidence
node scripts/run-vitest.mjs run extensions/tlon/src/urbit/sse-client.test.ts extensions/tlon/src/urbit/sse-client.bounded.test.ts— 18/18 tests pass (16 existing + 2 new bounded-read tests) in 3.35s.node scripts/run-tsgo.mjs -p tsconfig.extensions.json— exit 0 (no type errors).node scripts/run-oxlint.mjs --tsconfig config/tsconfig/oxlint.extensions.json extensions/tlon/src/urbit/sse-client.ts extensions/tlon/src/urbit/sse-client.bounded.test.ts— exit 0 (no lint errors, no floating-promise warnings).Reproduce locally:
Layer 1 wire details (so reviewers can re-derive the byte counts):
http.createServerlistens on127.0.0.1:0(random port) and chunks 18 × 1 MiB = 18 874 368 bytes viasetInterval(..., 1)withContent-Type: text/event-stream.fetch("http://127.0.0.1:<port>/")returns aResponsewhosebodyis aReadableStream<Uint8Array>from the underlying undici socket.client.processStream(body)callsReadable.fromWeb(body)(Node Readable) and hands it toreadByteStreamWithLimit(stream, {maxBytes: 16 777 216, onOverflow: ({size}) => Error(...)}).readByteStreamWithLimitaccumulates chunks via for-await. Aftertotal + nextChunk.byteLength > 16 MiB, the helper callsdestroy()on the Readable and throws the labeled error. The exact reportedsizedepends on TCP coalescing at moment-of-throw — hence the invariantMAX < got < TOTALrather than an exact byte assertion.processStreamto the caller's.catch(...)(line 213 insse-client.ts); the existingstreamReleasefinally-block still runs, thestreamControlleris nulled, and auto-reconnect logic is preserved.Related
fix(anthropic): bound SSE stream reads at 16 MiB. Established thecreateSseByteGuardhelper pattern for core LLM providers.fix(anthropic): bound streaming 200 success-body SSE reads at 16 MiB (provider path). Same pattern, alternative wiring for the Anthropic-Messages legacy path.fix(provider-transport-fetch): bound SSE buffer to prevent OOM. Core transport-level cap.fix(mistral): bound SSE response reads via buildGuardedModelFetch. Most recent SSE bounded-read; closest sibling to this PR in shape (SDK fetcher injection → bounded stream), but in a core provider, not an extension.fix(google OAuth): bound arrayBuffer reads. Sameopenclaw/plugin-sdk/response-limit-runtimeimport pattern, but for OAuth response bodies (readResponseWithLimit, notreadByteStreamWithLimit).openclaw/plugin-sdk/response-limit-runtime— re-exportsreadByteStreamWithLimitfrom@openclaw/media-core/read-byte-stream-with-limit. The helper used in this PR. 20+ existing extensions already import from this SDK subpath.extensions/tlon/runtime-api.ts— tlon extension's local SDK facade; existing pattern for re-exporting SDK helpers when multiple local files use them (no re-export added here since only one caller).Label: security
AI-assisted.