fix(tlon): bound error response body reads to prevent OOM#98496
Conversation
|
Codex review: needs maintainer review before merge. Reviewed July 1, 2026, 8:24 AM ET / 12:24 UTC. Summary PR surface: Source +5, Tests +101. Total +106 across 5 files. Reproducibility: yes. Current main has source-visible unbounded Review metrics: 2 noteworthy metrics.
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 one canonical Tlon bounded error-body hardening change that covers all three non-ok Urbit paths, then retire any overlapping narrower PRs. Do we have a high-confidence way to reproduce the issue? Yes. Current main has source-visible unbounded Is this the best way to solve the issue? Yes, this is an acceptable narrow fix: the bounded read belongs at the Tlon HTTP error boundaries and uses an exported SDK helper. The main remaining maintainer choice is whether to prefer this truncated diagnostic behavior or the stricter discard-on-overflow shape used by the narrower overlapping PR. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 5e4d71686a6a. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +5, Tests +101. Total +106 across 5 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
|
9153dea to
8dc152c
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
b26b71e to
9f672bf
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
@clawsweeper re-review |
1 similar comment
|
@clawsweeper re-review |
Replace bare response.text() on non-ok paths with readResponseTextLimited capped at 16 KiB so a hostile or misconfigured Urbit ship cannot force the gateway to buffer an arbitrary-size error body into process memory. Affected paths: - pokeUrbitChannel (channel-ops.ts) - channel.runtime.ts poke path - sendSubscription (sse-client.ts)
- Remove unused beforeEach import - Wrap if/else bodies in braces (curly) - Use block body for Promise executors (no-promise-executor-return)
- Fix TS2493 tuple type errors in server-cron-notifications and server-cron tests by adding explicit type annotations on mock.calls - Fix TS2322 in anthropic.test.ts by adding as const to resource content block type
9f672bf to
de6b427
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
vincentkoc
left a comment
There was a problem hiding this comment.
Reviewed the Tlon error-body boundaries and the overlap with #98526. This is the canonical landing path because it bounds all three Tlon non-ok response reads, not just createHttpPokeApi; the 16 KiB bounded diagnostic behavior is acceptable here.
…8496) * fix(tlon): bound error response body reads to prevent OOM Replace bare response.text() on non-ok paths with readResponseTextLimited capped at 16 KiB so a hostile or misconfigured Urbit ship cannot force the gateway to buffer an arbitrary-size error body into process memory. Affected paths: - pokeUrbitChannel (channel-ops.ts) - channel.runtime.ts poke path - sendSubscription (sse-client.ts) * fix(tlon): fix lint issues in error-body-boundary test - Remove unused beforeEach import - Wrap if/else bodies in braces (curly) - Use block body for Promise executors (no-promise-executor-return) * fix(types): resolve pre-existing TS test type errors - Fix TS2493 tuple type errors in server-cron-notifications and server-cron tests by adding explicit type annotations on mock.calls - Fix TS2322 in anthropic.test.ts by adding as const to resource content block type * chore: trigger CI
- Check content-length header before reading body, reject oversized responses early without allocating memory. - Prefer resp.body() + subarray decode over resp.text() on the main path to limit string decoding to maxChars * 4 bytes. - Three-layer defense: content-length pre-check → bounded Buffer decode → post-decode trimmed slice. - Retain resp.text() fallback for older Playwright versions. Fixes openclaw#98496
- Remove resp.text() fallback that still loaded full body unbounded. resp.body() is universally available in Playwright — no environment needs the text() path. - Remove local scratch docs/.local/ files from the PR branch; these are development notes, not intended for upstream. Fixes openclaw#98496
- Remove the content-length pre-reject early return that changed oversized response behavior from truncated prefix to empty body. body() + subarray bounded decode is backward compatible. - Add 3 UT cases: small full decode, large with content-length, large without content-length (chunked). Fixes openclaw#98496
…ion test - Add console.log proof lines showing body length and truncation status for each bounded-read scenario. - Add negative control: proves old text() path buffers full body. - Add mutation test: proves subarray() decode window is load-bearing. Fixes openclaw#98496
- Check content-length header before reading body, reject oversized responses early without allocating memory. - Prefer resp.body() + subarray decode over resp.text() on the main path to limit string decoding to maxChars * 4 bytes. - Three-layer defense: content-length pre-check → bounded Buffer decode → post-decode trimmed slice. - Retain resp.text() fallback for older Playwright versions. Fixes openclaw#98496
- Remove resp.text() fallback that still loaded full body unbounded. resp.body() is universally available in Playwright — no environment needs the text() path. - Remove local scratch docs/.local/ files from the PR branch; these are development notes, not intended for upstream. Fixes openclaw#98496
- Remove the content-length pre-reject early return that changed oversized response behavior from truncated prefix to empty body. body() + subarray bounded decode is backward compatible. - Add 3 UT cases: small full decode, large with content-length, large without content-length (chunked). Fixes openclaw#98496
…ion test - Add console.log proof lines showing body length and truncation status for each bounded-read scenario. - Add negative control: proves old text() path buffers full body. - Add mutation test: proves subarray() decode window is load-bearing. Fixes openclaw#98496
…8496) * fix(tlon): bound error response body reads to prevent OOM Replace bare response.text() on non-ok paths with readResponseTextLimited capped at 16 KiB so a hostile or misconfigured Urbit ship cannot force the gateway to buffer an arbitrary-size error body into process memory. Affected paths: - pokeUrbitChannel (channel-ops.ts) - channel.runtime.ts poke path - sendSubscription (sse-client.ts) * fix(tlon): fix lint issues in error-body-boundary test - Remove unused beforeEach import - Wrap if/else bodies in braces (curly) - Use block body for Promise executors (no-promise-executor-return) * fix(types): resolve pre-existing TS test type errors - Fix TS2493 tuple type errors in server-cron-notifications and server-cron tests by adding explicit type annotations on mock.calls - Fix TS2322 in anthropic.test.ts by adding as const to resource content block type * chore: trigger CI
…8496) * fix(tlon): bound error response body reads to prevent OOM Replace bare response.text() on non-ok paths with readResponseTextLimited capped at 16 KiB so a hostile or misconfigured Urbit ship cannot force the gateway to buffer an arbitrary-size error body into process memory. Affected paths: - pokeUrbitChannel (channel-ops.ts) - channel.runtime.ts poke path - sendSubscription (sse-client.ts) * fix(tlon): fix lint issues in error-body-boundary test - Remove unused beforeEach import - Wrap if/else bodies in braces (curly) - Use block body for Promise executors (no-promise-executor-return) * fix(types): resolve pre-existing TS test type errors - Fix TS2493 tuple type errors in server-cron-notifications and server-cron tests by adding explicit type annotations on mock.calls - Fix TS2322 in anthropic.test.ts by adding as const to resource content block type * chore: trigger CI
…8496) * fix(tlon): bound error response body reads to prevent OOM Replace bare response.text() on non-ok paths with readResponseTextLimited capped at 16 KiB so a hostile or misconfigured Urbit ship cannot force the gateway to buffer an arbitrary-size error body into process memory. Affected paths: - pokeUrbitChannel (channel-ops.ts) - channel.runtime.ts poke path - sendSubscription (sse-client.ts) * fix(tlon): fix lint issues in error-body-boundary test - Remove unused beforeEach import - Wrap if/else bodies in braces (curly) - Use block body for Promise executors (no-promise-executor-return) * fix(types): resolve pre-existing TS test type errors - Fix TS2493 tuple type errors in server-cron-notifications and server-cron tests by adding explicit type annotations on mock.calls - Fix TS2322 in anthropic.test.ts by adding as const to resource content block type * chore: trigger CI (cherry picked from commit 8abd5d4)
Replaces the 3 bare error-path
response.text()calls inextensions/tlon/src/urbit/channel-ops.ts,extensions/tlon/src/channel.runtime.ts, andextensions/tlon/src/urbit/sse-client.tswith the shared boundedreadResponseTextLimitedcapped at 16 KiB.Covers
pokeUrbitChannelerror reads, generic channel error reads, and SSE connection failure error reads. All three paths use the identicalreadResponseTextLimitedhelper; the loopback test validates the helper with real OS TCP traffic, which covers all three call sites.Linked context
No linked issue; this is a narrow follow-up to the bounded-response-read hardening series.
Related: PR #97587 (provider-http bounded layer), PR #98455 (browser-control bounded error reads).
Real behavior proof (required for external PRs)
Behavior or issue addressed: 3 unbounded Urbit API error-body reads in
channel-ops.ts(pokeUrbitChannel),channel.runtime.ts(channel error), andsse-client.ts(SSE connection failure) — all using bareresponse.text()that buffers the entire payload before constructing the error message. All three call sites delegate to the samereadResponseTextLimited(response, 16384)shared helper, so the loopback proof below validates the mechanism used by all three.Real environment tested: Linux x86-64, Node 22, latest upstream/main.
Exact steps or command run after this patch:
Evidence after fix:
Bounded path (
channel-ops.ts pokeUrbitChannel): 4 MiB streaming 500 response from realnode:httploopback server →pokeUrbitChannelerror message < 32 KiB containingX(proves streaming cancellation, not empty fallback).Small body path: server returns
session expired→ error message containssession expired(proves under-cap preservation).Coverage for channel.runtime.ts and sse-client.ts paths: Both call sites invoke the identical
readResponseTextLimited(response, 16384)helper. The loopback test exercises this helper against a realnode:httpserver serving oversized and normal-sized responses through Node's realfetch()/Responsepipeline. The bounded read behavior is helper-invariant, not call-site-dependent, so the single loopback test validates all three runtime paths.Full suite:
pnpm test extensions/tlon/src/urbit/— 7 files, 35 tests passed.What was not tested: Live Urbit ship API calls; no Urbit credentials are available in this environment.
Proof limitations or environment constraints: Loopback proof uses the real OS TCP stack and Node's real
fetch/Responsepath, while thefetchWithSsrFGuardmock replaces the SSRF guard with a real-fetch pass-through inside the test seam.Tests and validation
Risk checklist
Did user-visible behavior change? Only for oversized Urbit error responses over 16 KiB: they are now truncated at the cap instead of being fully materialized. Under-cap error responses are fully preserved.
Did config, environment, or migration behavior change? No.
Did security, auth, secrets, network, or tool execution behavior change? No — the same Urbit ship URLs and auth cookies are used.
What is the highest-risk area?
sse-client.tserror path runs on every SSE stream connection failure; the bounded read replaces a bareresponse.text()that was already behind a.catch(() => ""), so behaviour is strictly additive (capped read instead of unbounded).How is that risk mitigated? New dedicated error-boundary test file covers both oversized and normal-sized error bodies. All 35 existing urbit tests pass unchanged.
CI status
All CI checks pass: check-lint, check-test-types, all test shards, QA smoke. No pre-existing test failures remain.
Mergeability: clean (rebased onto latest upstream/main, no conflicts).