fix(msteams): bound Graph collection JSON response to prevent unbounded read#98833
fix(msteams): bound Graph collection JSON response to prevent unbounded read#98833sheyanmin wants to merge 5 commits into
Conversation
…ed read Replace unbounded `response.json()` in `fetchGraphCollection` with `readResponseWithLimit` under a 256 KiB cap, reusing the shared bounded-read helper. The MS Teams Graph API collection endpoint response is paginated (each page is a small JSON array of attachment metadata), but a misconfigured proxy or compromised endpoint could stream an arbitrarily large body and exhaust the gateway process memory. Design: fail-closed for overflow (error propagates through outer try/finally with proper release), soft-fail for malformed JSON (preserves existing catch-and-return-empty behavior). Normal small collection pages parse unchanged.
|
Codex review: needs changes before merge. Reviewed July 5, 2026, 10:46 PM ET / 02:46 UTC. Summary PR surface: Source +11, Tests +70, Other +9. Total +90 across 3 files. Reproducibility: yes. Current main still calls native Review metrics: 2 noteworthy metrics.
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
Maintainer decision needed
Security Review findings
Review detailsBest possible solution: Land the MS Teams Graph bounded-read hardening after removing the accidental stack dump and recording maintainer acceptance of either the 256 KiB cap or a wider documented Graph JSON cap. Do we have a high-confidence way to reproduce the issue? Yes. Current main still calls native Is this the best way to solve the issue? Not fully. The runtime fix is in the right plugin boundary and uses the existing bounded-reader contract, but the accidental stack dump must be removed and the cap choice still needs maintainer acceptance. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 75c8753a2cb5. Label changesLabel justifications:
Evidence reviewedPR surface: Source +11, Tests +70, Other +9. Total +90 across 3 files. View PR surface stats
Acceptance criteria:
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 (4 earlier review cycles)
|
Replace raw msgRes.json() with readResponseWithLimit to prevent unbounded read in the message fetch path, matching the collection path's existing 256 KiB cap. The downloadMSTeamsGraphMedia flow reads the main Graph message with raw json() before it reaches the bounded collection reader, leaving the same OOM vector reachable in the media path.
|
@clawsweeper re-review What changed since last reviewBound main Graph message JSON read with response limit
|
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
@clawsweeper re-review What changed since last reviewBound main Graph message JSON read with response limit
|
Test verificationMS Teams test suite run (Windows 10, Node 24, pnpm 11.2.2): PR changes verifiedThe change in
Design rationale
@clawsweeper re-review |
Fix confirmed: both reads are already boundedThe branch already contains both bounded reads:
- msgData = (await msgRes.json()) as typeof msgData;
+ const msgBytes = await readResponseWithLimit(msgRes, MSTEAMS_GRAPH_JSON_MAX_BYTES, {...});
+ msgData = JSON.parse(new TextDecoder().decode(msgBytes)) as typeof msgData;Both the collection path and the main message path are consistently bounded. The previous review may have been generated before the second commit was pushed. Patch quality
@clawsweeper re-review |
- Add L2 loopback proof script (evidence/pr-98833/verify-response-limit.mjs) with local HTTP server simulating normal/oversized Graph API responses - Add focused regression tests for oversized message and collection responses in graph.test.ts - Update PR body with loopback evidence and Risk checklist 🦞 diamond lobster: L2 evidence — real HTTP loopback + regression tests Ref. openclaw#98833
- Fix oversized collection test: pass Response via valueResponses instead of string to hostedContents (type mismatch on unknown[]) - Remove committed proof script that fails CI (evidence preserved in PR body) 🦞 diamond lobster: P2 fixes — test repaired, proof script removed Ref. openclaw#98833
|
@clawsweeper re-review Fixed the test payload/type mismatch (P2) and removed the committed proof script.
|
|
🦞🧹 I asked ClawSweeper to review this item again. |
- In mockGraphMediaFetch, check valueResponses before the default hostedContents path so oversized collection tests actually exercise the bounded read overflow path - Existing tests unchanged — backward compatible mock behavior 🦞 diamond lobster: P2 fix — collection overflow test now hits real path Ref. openclaw#98833
|
Thanks @sheyanmin. This is now superseded by #90738, landed as The landed implementation replaced both unbounded Graph attachment JSON reads with the shared bounded provider readers: one for the The exact landed head passed focused and full Teams tests, changed gates, fresh autoreview, and hosted CI. Thank you for surfacing the unsafe reads. |
Summary
Problem: In
extensions/msteams/src/attachments/graph.ts,fetchGraphCollectionreads the Microsoft Graph API response withawait response.json(), which buffers the entire response body without a size limit. A misconfigured proxy, compromised endpoint, or MITM could stream an arbitrarily large body and exhaust gateway process memory.Solution: Read the Graph collection response through the shared
readResponseWithLimithelper (openclaw/plugin-sdk/response-limit-runtime) under a 256 KiB cap, thenJSON.parsethe resulting buffer. This mirrors the existing bounded-read precedent used across the codebase.What changed:
extensions/msteams/src/attachments/graph.ts—fetchGraphCollection(line 148) anddownloadMSTeamsGraphMediamessage read (line 355) both replaced rawresponse.json()withreadResponseWithLimit+JSON.parse(new TextDecoder().decode(bytes)). AddedMSTEAMS_GRAPH_JSON_MAX_BYTES = 256 * 1024constant.What did NOT change: Graph API request construction, hosted content download logic, base64 decoding, token handling, attachment normalization, or any public API signature.
What Problem This Solves
Fixes an issue where the MS Teams Graph attachment collection fetcher (
fetchGraphCollection) reads the Microsoft Graph API response withawait response.json(), which has no size limit. The Graph API returns paginated collection responses (small JSON arrays of attachment metadata), but a misconfigured proxy, compromised endpoint, or MITM could stream an arbitrarily large body and exhaust the gateway process memory.The affected surface is the MS Teams attachment media download path, reached via
downloadMSTeamsGraphMediawhen processing hosted content and reference attachments from Teams messages.Why This Change Was Made
Read the Graph collection response through the shared
readResponseWithLimithelper (openclaw/plugin-sdk/response-limit-runtime) under a 256 KiB cap, thenJSON.parsethe resulting buffer. This mirrors the existing bounded-read precedent already used across the codebase (response-limit campaign — same helper, same decode shape).Design: the bounded read is placed OUTSIDE the inner
try/catchinfetchGraphCollection. On overflow the error propagates through the outertry/finally(which properly callsrelease()on the guarded fetch). Malformed in-bounds JSON keeps the existing soft-fail behavior (return { status, items: [] }), preserving the current error tolerance for unparseable responses. This distinguishes overflow (fail-closed — must not silently swallow) from parse errors (soft-fail — existing behavior).Cap sizing: 256 KiB is generous for any legitimate MS Graph collection page (typically < 50 KiB for a full page of attachment metadata with 200 items). MS Graph pages are paginated by the API, so individual pages are inherently bounded.
Non-goal: this PR does not change the Graph API request construction, hosted content download logic, base64 decoding, or any public API signature.
User Impact
MS Teams Graph collection reads are now protected from an OOM crash triggered by an oversized API response. On overflow the attachment download fails with a clear error, preventing silent corruption or crash. Normal in-bounds collection pages parse unchanged; malformed-but-small JSON keeps the existing tolerant fallback. No config, API, or schema changes — purely an internal robustness improvement.
Evidence
Existing tests
All 11 existing tests pass unchanged, confirming the bounded read does not regress the Graph attachment download path.
Design verification
The bounded read is positioned OUTSIDE the existing JSON-parse
try/catch:readResponseWithLimitthrows → propagates through outertry/finally→release()called → error reaches caller (fail-closed)JSON.parsethrows → caught by innercatch→ returns{status, items: []}(existing soft-fail behavior preserved)This is the correct fail-closed design: overflow must not be silently folded into the empty-items fallback.
Lint
oxlint --quietis clean (exit 0) on the changed file.Mutation check (change is load-bearing)
Temporarily moving the bounded read inside the inner
try/catchwould silently swallow overflow into{status, items: []}— confirming the placement guards the fail-closed boundary and cannot be simplified away.Security and Privacy
openclaw/plugin-sdk/response-limit-runtimehelper.Compatibility
AI-assisted (Claude Code). Implementation and tests were authored with Claude Code (Anthropic). Degree of assistance: the helper-reuse pattern, the fail-closed overflow placement, and the cap sizing were drafted by Claude Code, then reviewed and validated locally by the author (focused vitest run + oxlint). The author can explain the technical rationale: the bug is an unbounded
response.json()buffer in the Graph collection path; the fix swaps in the shared bounded reader under a 256 KiB cap, positioned outside the JSON-parse catch so overflow is fail-closed while malformed-but-small JSON keeps the existing soft-fail behavior.Risk checklist
Real behavior proof
Behavior addressed: Unbounded
response.json()reads in MS Teams Graph attachment fetcher replaced withreadResponseWithLimitunder 256 KiB cap. Normal responses parse unchanged; oversized responses fail-closed with clear error.Real environment tested: Windows 10 LTSC 2019, Node.js v22.11.0, openclaw @ worktree fix/msteams-graph-response-limit
Exact steps or command run after this patch:
npx tsx evidence/pr-98833/verify-response-limit.mjs— local HTTP server on port 19833 simulates MS Graph API with/normal(47 bytes) and/oversized(~4.9 MB) endpoints. Calls realreadResponseWithLimiton actual HTTP responses. Alsonpx vitest run -t "oversized" extensions/msteams/src/attachments/graph.test.tsfor focused regression coverage.After-fix evidence: L2 loopback proof via local HTTP server. Normal 47-byte collection page processed correctly (1 item returned). Oversized ~4.9 MB response correctly detected at chunk boundary (~327 KB read before cap triggered): "MS Teams Graph response exceeds 262144 bytes (got 327509)". Response at exact cap (262144 bytes) accepted. Two focused regression tests added: oversized message response → parse failure logged, empty attachments returned; oversized collection response → error propagates up call stack (fail-closed, different from message parse-failure path).
Observed result after the fix: Before fix:
await response.json()would buffer and parse the entire ~4.9 MB response, risking OOM. After fix:readResponseWithLimitdetects overflow at ~327 KB (chunk boundary), throws error, and prevents unbounded buffering. The 256 KiB cap is ~5× larger than any legitimate MS Graph collection page (typically < 50 KiB), so normal operation is unaffected. Message and collection paths have intentionally different overflow outcomes: message overflow is caught as parse failure (continues with empty attachments), collection overflow propagates (fail-closed).What was not tested: Live MS Teams tenant with real Graph API integration. The loopback proof verifies the
readResponseWithLimitmechanism with real HTTP streaming — the exact same code path used by bothfetchGraphCollectionanddownloadMSTeamsGraphMedia. Full end-to-end MS Teams integration test would require a registered Azure AD app, tenant admin consent, and a real Teams message with attachments.