Skip to content

fix(msteams): bound graph-upload JSON response reads to prevent OOM#97711

Closed
cxbAsDev wants to merge 2 commits into
openclaw:mainfrom
cxbAsDev:fix/bound-msteams-graph-upload-response
Closed

fix(msteams): bound graph-upload JSON response reads to prevent OOM#97711
cxbAsDev wants to merge 2 commits into
openclaw:mainfrom
cxbAsDev:fix/bound-msteams-graph-upload-response

Conversation

@cxbAsDev

@cxbAsDev cxbAsDev commented Jun 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

Replace 7 unbounded res.json() calls with readProviderJsonResponse (16 MiB cap) in MSTeams OneDrive/SharePoint upload, sharing link, driveItem properties, chat listing, and chat member fetch paths. Without a bound, an oversized or malicious JSON response can OOM the process.

Why This Change Was Made

All JSON response reads from external HTTP endpoints should use readProviderJsonResponse or readResponseWithLimit to prevent unbounded memory consumption. This is the same pattern already used in bot-framework.ts and graph.ts.

User Impact

MS Teams file upload, SharePoint sharing, chat resolution, and member fetching are now protected against oversized JSON responses. No functional change for normal-sized responses.

Evidence

Environment: Linux, Node 22, OpenClaw main

All existing tests pass (12/12 in graph-upload.test.ts):

 ✓ graph upload helpers > uploads to OneDrive with the personal drive path
 ✓ graph upload helpers > uploads to SharePoint with the site drive path
 ✓ graph upload helpers > rejects upload responses missing required fields
 ✓ graph upload helpers > bounds upload error bodies without requiring response.text()
 ✓ resolveGraphChatId > returns the ID directly when it already starts with 19:
 ✓ resolveGraphChatId > resolves personal DM chat ID via Graph API using user AAD object ID
 ✓ resolveGraphChatId > resolves personal DM chat ID without user AAD object ID (lists all 1:1 chats)
 ✓ resolveGraphChatId > returns null when Graph API returns no chats
 ✓ resolveGraphChatId > returns null when Graph API call fails
 ✓ resolveGraphChatId > rejects oversized OneDrive upload responses and cancels the stream
 ✓ buildTeamsFileInfoCard > extracts a unique id from quoted etags and lowercases file extensions
 ✓ buildTeamsFileInfoCard > keeps the raw etag when no version suffix exists and handles extensionless files

Oversized response test proves stream cancellation on 18 MiB payload:

const stream = new ReadableStream<Uint8Array>({ cancel, start(controller) {
  for (let i = 0; i < 18; i++) controller.enqueue(new Uint8Array(ONE_MIB));
}});
// bounded reader throws "JSON response exceeds 16 MiB", stream is cancelled

Bundle lint: clean.

Risk

Low. Same readProviderJsonResponse helper already used in bot-framework.ts and graph.ts. Pure replacement with 16 MiB cap matching codebase convention.

🤖 Generated with Claude Code

@cxbAsDev
fix(msteams): bound graph-upload JSON response reads to prevent OOM
5c9a3ba 
Replace 7 unbounded res.json() calls with readProviderJsonResponse
(16 MiB cap) in OneDrive/SharePoint upload, sharing link, driveItem,
chat listing, and chat member fetch paths.
@openclaw-barnacle openclaw-barnacle Bot added channel: msteams Channel integration: msteams size: XS triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. labels Jun 29, 2026
@clawsweeper

clawsweeper Bot commented Jun 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Thanks for the context here. I swept through the related work, and this is now duplicate or superseded.

Close as superseded: #97784 covers the same seven Microsoft Teams graph-upload JSON reads, adds stronger loopback proof, and is already marked proof-sufficient while this branch has no unique runtime fix left.

Root-cause cluster
Relationship: duplicate
Canonical: #97784
Summary: The same Microsoft Teams graph-upload bounded-read bug is covered by a stronger open PR with positive proof; broader bounded-read sweeps and attachment hardening are adjacent but not the same root fix.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Canonical path: Use #97784 as the canonical graph-upload bounded-read path, then land or reject that PR after maintainer acceptance of the 16 MiB cap and exact-head validation.

So I’m closing this here and keeping the remaining discussion on #97784.

Review details

Best possible solution:

Use #97784 as the canonical graph-upload bounded-read path, then land or reject that PR after maintainer acceptance of the 16 MiB cap and exact-head validation.

Do we have a high-confidence way to reproduce the issue?

Yes, source-level: current main has seven raw res.json() success reads reachable from the Teams graph-upload helpers, and an oversized successful JSON stream would exercise the unbounded path. I did not run a repro because this review was read-only.

Is this the best way to solve the issue?

No for this branch as the landing vehicle: the implementation shape is right, but #97784 is the better solution because it covers the same code with stronger real HTTP loopback proof. A custom reader or broader multi-plugin sweep would be riskier for this narrow bug.

Security review:

Security review cleared: The diff reuses an existing bounded SDK HTTP helper and adds a test; it does not change dependencies, workflows, package metadata, permissions, endpoints, auth headers, or secret handling.

AGENTS.md: found and applied where relevant.

What I checked:

  • Repository policy applied: Root and extension-scoped AGENTS guidance was read; the extension boundary, whole-path review, proof, and compatibility-sensitive fail-closed behavior rules apply to this bundled plugin PR. (AGENTS.md:1, 72f837a4a478)
  • Current main behavior: Current main still has raw res.json() success reads in the affected graph-upload paths, including OneDrive upload, sharing links, SharePoint upload, driveItem properties, chat listing, and chat member fetches. (extensions/msteams/src/graph-upload.ts:57, 72f837a4a478)
  • Shared helper contract: readProviderJsonResponse uses the shared 16 MiB provider JSON cap and delegates to readResponseWithLimit, which cancels the response body reader on overflow before throwing. (src/agents/provider-http-errors.ts:315, 72f837a4a478)
  • This PR diff: The PR changes the same seven graph-upload success reads to readProviderJsonResponse and adds a synthetic ReadableStream oversized-response test; the useful runtime change is the same surface as the canonical PR. (extensions/msteams/src/graph-upload.ts:55, 728788cf9fad)
  • Canonical duplicate candidate: fix(msteams): bound Microsoft Graph API response reads in graph-upload to prevent OOM #97784 is open, mergeable, proof-sufficient, and ready for maintainer look; its diff covers the same seven graph-upload reads and adds real node:http loopback proof for normal, malformed, and oversized JSON responses. (extensions/msteams/src/graph-upload.test.ts:251, cea2c0cabb17)
  • Related search: Live related-PR searches found this PR, the proof-positive graph-upload duplicate, and the broader lower-signal multi-plugin bounded-read sweep; the tracker PR is bookkeeping rather than a canonical implementation path.

Likely related people:

  • vincentkoc: Live path history shows recent merged work bounding Microsoft Teams service error bodies in graph-upload.ts, directly adjacent to this response-body hardening. (role: recent area contributor; confidence: high; commits: a5eddb91bbd8; files: extensions/msteams/src/graph-upload.ts)
  • steipete: Live path history shows recent refactors trimming Microsoft Teams helper exports and sharing the Teams drive upload flow around this module. (role: recent refactor contributor; confidence: high; commits: 3f002b10d281, 6b04ab1e35ed; files: extensions/msteams/src/graph-upload.ts)
  • sudie-codes: Live path history shows the Graph chat ID resolution work that introduced one of the affected graph-upload JSON read paths. (role: feature introducer; confidence: medium; commits: 06845a1974a3; files: extensions/msteams/src/graph-upload.ts)
  • Alix-007: This contributor owns the currently proof-sufficient duplicate implementation at fix(msteams): bound Microsoft Graph API response reads in graph-upload to prevent OOM #97784, which is the stronger landing candidate for this same surface. (role: canonical follow-up owner; confidence: medium; commits: cea2c0cabb17; files: extensions/msteams/src/graph-upload.ts, extensions/msteams/src/graph-upload.test.ts)

Codex review notes: model internal, reasoning high; reviewed against 72f837a4a478.

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. labels Jun 29, 2026
@BradGroux BradGroux mentioned this pull request Jun 29, 2026
Draft
@Alix-007 Alix-007 mentioned this pull request Jun 29, 2026
Merged
@openclaw-barnacle openclaw-barnacle Bot added size: S and removed size: XS triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. labels Jun 30, 2026
@clawsweeper clawsweeper Bot added merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. labels Jun 30, 2026
@cxbAsDev cxbAsDev closed this Jun 30, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jul 2, 2026
Closed
@cxbAsDev
cxbAsDev deleted the fix/bound-msteams-graph-upload-response branch July 15, 2026 03:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

channel: msteams Channel integration: msteams merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P2 Normal backlog priority with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. size: S status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@cxbAsDev