Skip to content

fix(msteams): bound graph upload JSON reads#98948

Closed
sunlit-deng wants to merge 1 commit into
openclaw:mainfrom
sunlit-deng:sunlit/fix/msteams-graph-upload-bound-json
Closed

fix(msteams): bound graph upload JSON reads#98948
sunlit-deng wants to merge 1 commit into
openclaw:mainfrom
sunlit-deng:sunlit/fix/msteams-graph-upload-bound-json

Conversation

@sunlit-deng

@sunlit-deng sunlit-deng commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes an issue where MS Teams Graph upload and sharing helpers could buffer an unbounded successful JSON response when reading drive item, sharing link, or chat metadata.

Why This Change Was Made

The Graph upload helpers now use the shared bounded provider JSON reader that other MS Teams Graph paths already use. Error responses were already bounded; this closes the matching successful-response path.

User Impact

A faulty or hostile Graph-compatible endpoint cannot force OpenClaw to buffer an oversized success metadata response, while normal Graph metadata responses continue to parse normally.

Evidence

  • Red test before the fix: node scripts/run-vitest.mjs extensions/msteams/src/graph-upload.test.ts -- --run failed with unbounded response.json should not be called.
  • node scripts/run-vitest.mjs extensions/msteams/src/graph-upload.test.ts -- --run -> 1 file / 12 tests passed.
  • node_modules/.bin/oxfmt --check extensions/msteams/src/graph-upload.ts extensions/msteams/src/graph-upload.test.ts -> clean.
  • node scripts/run-oxlint.mjs extensions/msteams/src/graph-upload.ts extensions/msteams/src/graph-upload.test.ts -> clean.
  • git diff --check -> clean.

Loopback proof with a real node:http server streaming a 32 MiB successful Graph upload JSON response:

error=SharePoint upload failed: JSON response exceeds 16777216 bytes
chunks_sent=18
total_chunks_available=32
response_closed=true

AI-assisted: built with Codex

@openclaw-barnacle openclaw-barnacle Bot added channel: msteams Channel integration: msteams size: S labels Jul 2, 2026
@sunlit-deng
sunlit-deng marked this pull request as ready for review July 2, 2026 06:27
@clawsweeper

clawsweeper Bot commented Jul 2, 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 a duplicate: this PR is a valid bounded-read fix, but #97784 already owns the same seven MS Teams graph-upload success JSON reads with stronger loopback proof and a clean mergeable state.

Root-cause cluster
Relationship: duplicate
Canonical: #97784
Summary: This PR and the canonical PR target the same seven MS Teams graph-upload successful JSON reads; the canonical PR is open, cleanly mergeable, proof-positive, and already reviewed as the strongest landing candidate.

Members:

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

Canonical path: Close this duplicate and keep #97784 as the landing path for the seven graph-upload bounded reads.

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

Review details

Best possible solution:

Close this duplicate and keep #97784 as the landing path for the seven graph-upload bounded reads.

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

Yes, source-level. Current main still has raw successful res.json() reads in the affected graph-upload paths, and the PR body includes after-fix loopback output showing oversized JSON rejection.

Is this the best way to solve the issue?

No for landing this branch. Reusing the shared helper is the right fix shape, but #97784 is the better landing path because it already covers the same seven reads with stronger proof.

Security review:

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

AGENTS.md: found and applied where relevant.

What I checked:

  • Repository policy read: Root AGENTS.md and extensions/AGENTS.md were read fully; extension code is expected to use public openclaw/plugin-sdk/* seams and fail-closed behavior is compatibility-sensitive review input. (AGENTS.md:1, 2913d3aee90c)
  • Current main still has the unbounded reads: Current main has raw successful res.json() reads in graph-upload paths such as OneDrive upload, SharePoint upload, Graph chat resolution, chat members, and sharing links, so the hardening is still useful but not unique to this PR. (extensions/msteams/src/graph-upload.ts:57, 2913d3aee90c)
  • This PR overlaps the same seven graph-upload reads: The diff imports readProviderJsonResponse and replaces the same seven successful graph-upload JSON parse sites, with one new oversized SharePoint success-body test. (extensions/msteams/src/graph-upload.ts:55, dfee6bdfb80c)
  • Canonical duplicate is already open and viable: fix(msteams): bound Microsoft Graph API response reads in graph-upload to prevent OOM #97784 is open, mergeable/CLEAN, labeled proof:sufficient and ready for maintainer look, and its prior ClawSweeper review identifies it as the canonical graph-upload bounded-read fix covering the same seven reads. (122f0083bd94)
  • Shared helper contract: readProviderJsonResponse reads through the shared 16 MiB cap, and readResponseWithLimit cancels the stream when the next chunk would exceed the cap. (src/agents/provider-http-errors.ts:315, 2913d3aee90c)
  • Related PR search: Search found the canonical open duplicate, the closed older graph-upload attempt, and broader/adjacent bounded-read work; no merged PR has already fixed this exact graph-upload surface on main.

Likely related people:

  • Alix-007: Authored the open canonical PR that covers the same seven graph-upload success JSON reads and has stronger loopback proof plus proof:sufficient review state. (role: canonical fix owner; confidence: high; commits: ba99033b0fe0, 122f0083bd94; files: extensions/msteams/src/graph-upload.ts, extensions/msteams/src/graph-upload.test.ts)
  • sudie-codes: Merged Microsoft Teams Graph chat ID/file-upload work added related resolveGraphChatId behavior and tests in the graph-upload path. (role: introduced related chat-resolution behavior; confidence: medium; commits: d852cf2fc6f7, 06845a1974a3; files: extensions/msteams/src/graph-upload.ts, extensions/msteams/src/graph-upload.test.ts, extensions/msteams/src/send.ts)
  • heyitsaamir: Carried a large merged MS Teams SDK/runtime refactor that touched messenger.ts, send.ts, graph.ts, and adjacent Teams runtime surfaces. (role: recent area contributor; confidence: medium; commits: 04c29825356f, 8e90628cb684; files: extensions/msteams/src/messenger.ts, extensions/msteams/src/send.ts, extensions/msteams/src/graph.ts)
  • Ayaan Zaidi: The shallow/grafted checkout blames the current graph-upload and shared bounded-reader lines to this current-main commit, so it is a routing clue but not full historical provenance. (role: current-main blame signal; confidence: low; commits: 3558391a75fd; files: extensions/msteams/src/graph-upload.ts, src/agents/provider-http-errors.ts, packages/media-core/src/read-response-with-limit.ts)

Codex review notes: model internal, reasoning high; reviewed against 2913d3aee90c.

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal backlog priority with limited blast radius. 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 Jul 2, 2026
@clawsweeper clawsweeper Bot closed this Jul 2, 2026
@clawsweeper

clawsweeper Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

🦞✅
ClawSweeper autoclose is complete.

Reason: structured ClawSweeper close marker: close-required (sha=dfee6bdfb80c8cb702e1c99125c626911169c227)

Closed:

@clawsweeper clawsweeper Bot mentioned this pull request Jul 2, 2026
Merged
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. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: S status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@sunlit-deng