fix(matrix): bound non-raw JSON response body in transport#95240
Conversation
|
Codex review: needs maintainer review before merge. Reviewed June 23, 2026, 10:28 AM ET / 14:28 UTC. Summary PR surface: Source +24, Tests +136. Total +160 across 2 files. Reproducibility: yes. by source inspection and contributor proof: current main and v2026.6.9 still use 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:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land this focused Do we have a high-confidence way to reproduce the issue? Yes by source inspection and contributor proof: current main and v2026.6.9 still use Is this the best way to solve the issue? Yes for the narrow AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 8e6624cb6cb8. Label changesLabel justifications:
Evidence reviewedPR surface: Source +24, Tests +136. Total +160 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
proof: sufficient
|
@clawsweeper re-review — Addressed the P3 finding: the JSON read path now passes a JSON-specific |
|
🦞🧹 I asked ClawSweeper to review this item again. |
…N read
The non-raw JSON read in performMatrixRequest fell back to the bound
reader's default media idle-timeout message ('Matrix media download
stalled: ...'), which is misleading for a JSON control-plane read. Pass
a JSON-specific onIdleTimeout so a stalled JSON stream now rejects with
'Matrix JSON response stalled: no data received for {ms}ms', letting the
timeout diagnostic distinguish a stalled JSON read from a stalled
raw/media read. Update the regression assertion accordingly.
8b6d491 to
6277cb3
Compare
|
Maintenance update: rebased this branch linearly onto the latest upstream/main. The previous prod-types/lint failures pointed at src/config/sessions/session-accessor.ts, which is outside this PR's Matrix diff and is already updated on current main. The PR diff remains limited to Matrix transport/test files, with author/committer email set to [email protected]. |
openclaw-barnacle
Bot
added
docs
openclaw-barnacle
Bot
added
extensions: anthropic-vertex
extensions: brave
extensions: chutes
extensions: elevenlabs
extensions: github-copilot
extensions: google
extensions: mistral
extensions: ollama
extensions: opencode
extensions: opencode-go
extensions: openrouter
extensions: perplexity
extensions: xai
extensions: copilot
labels
github-actions
Bot
added
the
dependencies-changed
Dependency GuardThis PR changes dependency-related files. Maintainers should confirm these changes are intentional. Changed files:
Maintainer follow-up:
|
Dependency graph changes are blockedOpenClaw does not accept dependency graph changes through PRs unless a repository admin or security explicitly authorizes the current head SHA. Dependency updates are generated internally by maintainers so external PRs cannot change the resolved graph. Detected dependency graph changes:
Auto-scrub was not attempted because this PR changes package manifest dependency graph fields:
Dependency graph changes must be reviewed by security or handled by maintainers internally. Please remove lockfile changes manually if they are not needed. To remove lockfile changes, restore them from the target branch: git fetch origin
git checkout 'origin/main' -- 'extensions/acpx/npm-shrinkwrap.json' 'pnpm-lock.yaml'
git commit -m 'chore: remove dependency lockfile change'
git pushIf this PR intentionally needs a dependency graph change, ask a repository admin or member of The action will approve the current head SHA ( |
|
Maintainer verification complete on exact head
One unrelated agent session-lock timing test failed in hosted CI while 1,530 sibling tests passed. |
|
Merged via rebase.
|
|
Thanks @steipete for the detailed Matrix verification and rebase merge. |
2 participants
Summary
performMatrixRequestalready bounds the raw media path (transport.ts): it precheckscontent-lengthand streams throughreadResponseWithLimit(response, params.maxBytes, …)so a malicious/huge media body is cancelled at the cap. The non-raw JSON path, however, fell through to a plainconst text = await response.text();, which buffers the entire body with no ceiling and no idle timeout.That JSON path serves control-plane calls against the homeserver —
whoami, read receipts, room key-backup status, directory search, and the genericdoRequest— and the Matrix homeserver is an untrusted source. A hostile or buggy homeserver can stream an unbounded JSON body and exhaust memory beforeJSON.parseis ever reached.This is the symmetric counterpart to #95108 (which hardened the Anthropic error-stream read with a byte/idle cap): same class of unbounded
response.text()on an untrusted upstream, fixed by reusing the existing bounded reader.Changes
extensions/matrix/src/matrix/sdk/transport.ts: bound the non-raw JSON read using the same in-filereadResponseWithLimithelper andparseMediaContentLengthprecheck already used by the raw media path — no new abstraction.MATRIX_JSON_RESPONSE_MAX_BYTES = 8 * 1024 * 1024as the default ceiling for control-plane JSON responses (generous for whoami/receipts/directory/key-backup, far below an OOM).params.maxByteswhen a caller provides one (falls back to the default), and threadsparams.readIdleTimeoutMsaschunkTimeoutMsso a stalled stream is cancelled, mirroring the raw path exactly.Matrix JSON response exceeds configured size limit (…)error instead of using the media-specificMatrixMediaSizeLimitError(semantically a JSON response, not media).onIdleTimeoutso a stalled stream rejects withMatrix JSON response stalled: no data received for {ms}msrather than the raw path'sMatrix media download stalled: …default — the timeout diagnostic now distinguishes a stalled JSON read from a stalled raw/media read.extensions/matrix/src/matrix/sdk/transport.test.ts: added regression coverage for the JSON path — content-length precheck rejection, streamed-overflow rejection without content-length, configured idle-timeout on a stalled stream (asserting the JSON-specific stalled message), and an under-cap body returned intact.Real behavior proof
Label: bounded non-raw JSON transport read
performMatrixRequestnon-raw JSON path buffered an unbounded response body viaawait response.text(); an untrusted homeserver could OOM the process. Now the body is read under a byte cap with stream cancellation and an optional idle timeout.performMatrixRequestJSON path (real undici fetch + SSRF-pinned dispatcher) against a realnode:httpserver on127.0.0.1.node --import tsx proof-bound-json.mts— a tsx harness importingperformMatrixRequestfrom source and hitting a live localhost HTTP server.maxBytes: 8 KiB.content-length: 65536, client capmaxBytes: 1 KiB.node scripts/run-vitest.mjs extensions/matrix/src/matrix/sdk/transport.test.ts --run(11/11 pass, incl. 4 new).node scripts/run-vitest.mjs extensions/matrix/src/matrix/sdk/http-client.test.ts --run(caller, 4/4 pass).request rejectedwithMatrix JSON response exceeds configured size limit (12288 bytes > 8192 bytes); the server observed the connection close (stream cancelled, socket released) after ~16 KiB instead of streaming forever.result.textandresult.buffermatch the payload).Matrix JSON response exceeds configured size limit (65536 bytes > 1024 bytes)— body not buffered.ALL PROOF CASES PASSED. Negative control: temporarily reverting the source fix to plainawait response.text()and re-running the infinite-stream case showed the bug — within a 1.5 s window the unbounded path did NOT reject; the server pushed ~5 MB and the read kept buffering (would continue to OOM/timeout). Restoring the fix returned toALL PROOF CASES PASSED./synclong-poll path is unaffected (it goes throughcreateMatrixGuardedFetch→arrayBuffer(), a separate buffering site not in scope here); no live remote homeserver was exercised (localhost server used as the untrusted upstream).Reviewer notes (for clawsweeper findings)
MATRIX_JSON_RESPONSE_MAX_BYTES) — maintainer decision requested: This is a fail-closed default, not a value I want to set unilaterally. It is sized to sit far above any legitimate Matrix control-plane JSON response (whoami, read receipts, key-backup status, directory search, genericdoRequest— all kilobyte-scale), while staying far below a memory-exhaustion threshold, and it lines up with the order of magnitude used by the other bound-stream caps in this repo. Callers can already override it per-request viaparams.maxBytes; the constant is only the floor for callers that pass nothing. If maintainers prefer a different ceiling (or want it sourced from config), it is a one-line change to the constant — please accept or tune as you see fit.createMatrixGuardedFetch(matrix-js-sdk/syncpath) — explicitly out of scope here: It buffers viaarrayBuffer()and is a separate, distinct buffering site. As clawsweeper noted, that should be tracked separately; this PR intentionally does not touch it to keep the change focused on theperformMatrixRequestJSON path. Happy to open a follow-up for the guarded-fetch path once this lands.This change is AI-assisted.