fix(tlon): cap SSE payload JSON.parse at 16 MiB to prevent OOM#101274
Conversation
76b9d74 to
71e32d0
Compare
|
Codex review: needs maintainer review before merge. Reviewed July 12, 2026, 4:35 AM ET / 08:35 UTC. Summary PR surface: Source +57, Tests +130. Total +187 across 2 files. Reproducibility: yes. at the source boundary: current main retains unterminated event data without a limit and sends extracted JSON to JSON.parse without a size guard. The submitted negative control also demonstrates both failures, although no authenticated ship reproduction was run. Review metrics: 1 noteworthy metric.
Stored data model Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. 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: Land the incremental per-event and JSON limits after the remaining exact-head required checks pass, preserving the explicit fail-closed errors and comprehensive split-boundary regression coverage. Do we have a high-confidence way to reproduce the issue? Yes at the source boundary: current main retains unterminated event data without a limit and sends extracted JSON to JSON.parse without a size guard. The submitted negative control also demonstrates both failures, although no authenticated ship reproduction was run. Is this the best way to solve the issue? Yes. Incremental per-event bounding preserves a long-lived SSE connection better than the earlier whole-response cap proposal, and the exact-head tests cover the important byte, Unicode, delimiter, overflow, and multi-event boundaries. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against d31eeea4080c. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +57, Tests +130. Total +187 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
Review history (10 earlier review cycles; latest 8 shown)
|
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
…101274) Adds extensions/tlon/proof-sse-bounded.mts which drives the real UrbitSSEClient.processStream and processEvent against Node Readable streams (not unit-test mocks) to demonstrate: - normal SSE events are still delivered through the stream path; - an unterminated stream that would grow beyond 16 MiB is rejected before unbounded accumulation; - a single SSE payload above 16 MiB is rejected before JSON.parse. The script passes on this branch and fails 2/3 assertions when run against origin/main's sse-client.ts, providing the before/after proof ClawSweeper requested.
c4efb73 to
91cd6bf
Compare
|
@clawsweeper re-review Real behavior proof is now supplied. Pushed After fix — production-style proof: Negative control — same script against Local verification:
|
|
🦞🧹 I asked ClawSweeper to review this item again. |
…101274) Adds extensions/tlon/proof-sse-bounded.mts which drives the real UrbitSSEClient.processStream and processEvent against Node Readable streams (not unit-test mocks) to demonstrate: - normal SSE events are still delivered through the stream path; - an unterminated stream that would grow beyond 16 MiB is rejected before unbounded accumulation; - a single SSE payload above 16 MiB is rejected before JSON.parse. The script passes on this branch and fails 2/3 assertions when run against origin/main's sse-client.ts, providing the before/after proof ClawSweeper requested.
91cd6bf to
5993428
Compare
clawsweeper
Bot
added
proof: sufficient
|
@clawsweeper re-review Pushed 32f7c18 addressing the P1 rank-up blocker:
After fix — 22/22 tests pass: Guard before/after: - buffer += chunkStr;
- bufferBytes += Buffer.byteLength(chunkStr, "utf8");
- if (bufferBytes > MAX_SSE_PAYLOAD_BYTES) {
+ const chunkBytes = Buffer.byteLength(chunkStr, "utf8");
+ // Reject before concatenating so an oversized chunk never lands in the
+ // pending buffer — the guard protects memory, not just the byte counter.
+ if (bufferBytes + chunkBytes > MAX_SSE_PAYLOAD_BYTES) {
+ throw new Error("...");
+ }
+ buffer += chunkStr;
+ bufferBytes += chunkBytes; |
|
🦞🧹 I asked ClawSweeper to review this item again. |
clawsweeper
Bot
added
rating: 🐚 platinum hermit
|
Superseded by the final exact-head proof: #101274 (comment) The repository landing gate required keeping root |
…101274) Adds extensions/tlon/proof-sse-bounded.mts which drives the real UrbitSSEClient.processStream and processEvent against Node Readable streams (not unit-test mocks) to demonstrate: - normal SSE events are still delivered through the stream path; - an unterminated stream that would grow beyond 16 MiB is rejected before unbounded accumulation; - a single SSE payload above 16 MiB is rejected before JSON.parse. The script passes on this branch and fails 2/3 assertions when run against origin/main's sse-client.ts, providing the before/after proof ClawSweeper requested.
e0b1523 to
7f3de02
Compare
The proof script was review-only validation for openclaw#101274. Durable behavior coverage lives in sse-client.test.ts (stream buffer bounding, oversized chunk rejection before concatenation, 16 MiB boundary, normal delivery, 1000 small events). Per ClawSweeper P3 finding, drop the one-off script from the plugin tree.
…101274) Adds extensions/tlon/proof-sse-bounded.mts which drives the real UrbitSSEClient.processStream and processEvent against Node Readable streams (not unit-test mocks) to demonstrate: - normal SSE events are still delivered through the stream path; - an unterminated stream that would grow beyond 16 MiB is rejected before unbounded accumulation; - a single SSE payload above 16 MiB is rejected before JSON.parse. The script passes on this branch and fails 2/3 assertions when run against origin/main's sse-client.ts, providing the before/after proof ClawSweeper requested.
Move the stream byte-limit check before buffer += chunkStr so a single oversized chunk never lands in the pending buffer. The old guard ran after concatenation, which still allowed the memory spike this hardening is meant to prevent. Add a single-oversized-chunk test to prove the guard fires before the chunk is concatenated into the pending buffer. Co-Authored-By: Claude Opus 4.7 <[email protected]>
The proof script was review-only validation for openclaw#101274. Durable behavior coverage lives in sse-client.test.ts (stream buffer bounding, oversized chunk rejection before concatenation, 16 MiB boundary, normal delivery, 1000 small events). Per ClawSweeper P3 finding, drop the one-off script from the plugin tree.
7f3de02 to
c895c97
Compare
clawsweeper
Bot
added
rating: 🐚 platinum hermit
|
Land-ready at Proof:
Accepted proof gap: no authenticated Tlon/Urbit ship is configured on this host, so a real subscription smoke could not run. The focused sanitized-AWS proof covers all 22 boundary, split-chunk, and rejection cases, and the maintainer explicitly directed landing after this gap was disclosed. |
|
Merged via squash.
|
2 participants
What Problem This Solves
extensions/tlon/src/urbit/sse-client.tsaccepted two unbounded inputs from an external Urbit ship:JSON.parsewithout a size limit.Either path could exhaust the OpenClaw process before the event was rejected.
Why This Change Was Made
TextDecoder, preserving UTF-8 code points split by transport boundaries.The 16 MiB value is an OpenClaw safety boundary, not an Urbit protocol limit. The official Eyre documentation and
urbit/js-http-apiparser do not specify a maximum event size.User Impact
\n\ndelimiter chunk boundaries.Evidence
c895c97d66d030fcd1d694249f89c8677ba0e74f.8a472770ce305c005a46753b7dfea92b05908975.run_c6cf3a56ae87: public networking, no Tailscale, no instance profile, Linux Node 24.pnpm test extensions/tlon/src/urbit/sse-client.test.ts; 22/22 passed in 335 ms.29185926126: completed without failing jobs after rebasing onto the extensionsnoUncheckedIndexedAccessand Plugin SDK declaration-budget fixes; Control UI locale parity was not scheduled for this unrelated change.CHANGELOG.mdrelease-owned.Proof gap
A real authenticated Tlon/Urbit ship connect and subscription smoke has not run. This host has no configured Tlon channel, and exact credential lookups for
TlonandUrbitfound no item. The maintainer explicitly directed landing after this gap was disclosed; focused sanitized-AWS proof and exact-head hosted CI are complete.