Skip to content

fix(msteams): bound Microsoft Graph API response reads in graph-upload to prevent OOM#97784

Merged
steipete merged 3 commits into
openclaw:mainfrom
Alix-007:fix/bound-msteams-graph-upload
Jul 7, 2026
Merged

fix(msteams): bound Microsoft Graph API response reads in graph-upload to prevent OOM#97784
steipete merged 3 commits into
openclaw:mainfrom
Alix-007:fix/bound-msteams-graph-upload

Conversation

@Alix-007

@Alix-007 Alix-007 commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Replaces the 7 bare successful-response res.json() calls in extensions/msteams/src/graph-upload.ts with the shared bounded provider JSON reader.
  • Covers OneDrive uploads, SharePoint uploads, sharing links, driveItem properties, chat list resolution, and chat member enumeration.
  • Rebased onto latest upstream/main, where readProviderJsonResponse reads JSON through readResponseWithLimit with the shared 16 MiB cap.
  • Adds real node:http loopback proof for normal chunked JSON, malformed JSON labelling, and oversized success JSON rejection.

Linked context

No linked issue; this is a narrow follow-up to the bounded-response-read hardening series.

Related #97711 (same file family, but this PR includes loopback proof and complete PR context).

Real behavior proof (required for external PRs)

  • Behavior or issue addressed: 7 unbounded successful Microsoft Graph API JSON reads in graph-upload.ts (uploadToOneDrive, createSharingLink, uploadToSharePoint, getDriveItemProperties, resolveGraphChatId, getChatMembers, createSharePointSharingLink).
  • Real environment tested: Linux x86-64, Node 22/24 compatible repo environment, latest upstream/main after rebase on 2026-06-29.
  • Exact steps or command run after this patch:
    PNPM_CONFIG_MODULES_DIR=/media/vdc/0668001470/workSpace/claw-campaign/openclaw/node_modules \
      node scripts/run-vitest.mjs extensions/msteams/src/graph-upload.test.ts -- --run
    PNPM_CONFIG_MODULES_DIR=/media/vdc/0668001470/workSpace/claw-campaign/openclaw/node_modules \
      /media/vdc/0668001470/workSpace/claw-campaign/openclaw/node_modules/.bin/oxlint \
      extensions/msteams/src/graph-upload.ts extensions/msteams/src/graph-upload.test.ts
  • Evidence after fix:
    [msteams graph-upload loopback proof] uploadToOneDrive: returned={"id":"item-lp","webUrl":"https://example.com/lp","name":"lp.txt"} auth_ok=true content_length=none
    [msteams graph-upload loopback proof] uploadToSharePoint: returned={"id":"sp-lp","webUrl":"https://sp.example.com/lp","name":"sp-lp.txt"} content_length=none
    [msteams graph-upload loopback proof] malformed JSON labelled: Error: msteams.graph-upload.uploadOneDriveFile: malformed JSON response
    [msteams graph-upload loopback proof] oversized JSON rejected by bounded reader
    Test Files  1 passed (1)
    Tests  15 passed (15)
    
  • Observed result after fix: Chunked success JSON still parses, malformed JSON is labelled with the Graph upload call site, and an oversized successful JSON response now fails closed with msteams.graph-upload.uploadOneDriveFile: JSON response exceeds 16777216 bytes before buffering the whole body.
  • What was not tested: Live Microsoft Graph API calls against a real Microsoft 365 tenant; no tenant credentials are available in this environment.
  • Proof limitations or environment constraints: Loopback proof uses the real OS TCP stack and Node's real fetch/Response path, while redirecting the hardcoded Graph URL to localhost inside the test seam.
  • Before evidence: The previous PR version had 14 tests and no direct oversized success-body assertion in this file. Current main had raw res.json() call sites in graph-upload before this branch.

Tests and validation

  • node scripts/run-vitest.mjs extensions/msteams/src/graph-upload.test.ts -- --run — 15 tests passed.
  • oxlint extensions/msteams/src/graph-upload.ts extensions/msteams/src/graph-upload.test.ts — 0 problems.
  • Addressed ClawSweeper review feedback by removing the redundant AddressInfo assertion that failed check-lint / check-additional-extension-bundled.

Risk checklist

  • Did user-visible behavior change? Only for oversized successful Microsoft Graph JSON responses: they now fail closed at the shared 16 MiB JSON cap instead of being read unbounded.
  • Did config, environment, or migration behavior change? No
  • Did security, auth, secrets, network, or tool execution behavior change? No — the same Graph endpoints and auth headers are used.
  • What is the highest-risk area? resolveGraphChatId still returns null on Graph lookup failures; bounded JSON errors still flow through the existing outer failure path.
  • How is that risk mitigated? Existing null-return tests still pass, and the new loopback tests cover the changed success-path reader behavior.

Current review state

  • Next action: ClawSweeper re-review, then maintainer review if CI stays green.
  • Waiting on: CI after rebase/push.
  • Bot/reviewer comments addressed: Removed redundant address assertion; added direct oversized success JSON proof; rebased onto latest main so the shared helper is the bounded implementation.

@openclaw-barnacle openclaw-barnacle Bot added channel: msteams Channel integration: msteams size: S labels Jun 29, 2026
@clawsweeper

clawsweeper Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 6, 2026, 1:03 PM ET / 17:03 UTC.

Summary
The PR replaces seven raw res.json() success reads in MS Teams Graph upload/share/chat helpers with readProviderJsonResponse and adds real loopback tests for normal, malformed, and oversized responses.

PR surface: Source +1, Tests +213. Total +214 across 2 files.

Reproducibility: yes. Current main still has seven bare .json() success reads in extensions/msteams/src/graph-upload.ts, and the PR body/test code provides a loopback oversized-response path; I did not run tests in this read-only review.

Review metrics: 1 noteworthy metric.

  • Bounded success JSON readers: 7 replaced, 0 bare .json() reads remain in graph-upload.ts at PR head. This shows the patch covers the complete graph-upload success-response set rather than only the upload path exercised by the oversized loopback assertion.

Root-cause cluster
Relationship: canonical
Canonical: #97784
Summary: This PR is the active canonical graph-upload bounded-success-JSON candidate; older same-surface PRs are closed unmerged, while other open bounded-read PRs address adjacent MS Teams response paths.

Members:

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

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • none.

Risk before merge

  • [P1] Oversized successful Microsoft Graph upload/share/chat metadata that previously buffered and parsed now throws at the shared 16 MiB provider JSON cap; that is intentional hardening but compatibility-sensitive for unusual tenants or proxies.
  • [P1] The proof uses a real Node loopback HTTP/fetch path and green CI, not a live Microsoft 365 tenant, so tenant-specific Graph behavior remains a maintainer confidence choice rather than a contributor blocker.

Maintainer options:

  1. Accept the 16 MiB fail-closed cap (recommended)
    Maintainers can accept the compatibility change because normal Graph metadata remains unchanged and oversized success JSON is no longer buffered without a bound.
  2. Ask for live Microsoft tenant proof
    Maintainers can require a real tenant upload/share smoke before merge if they want service-specific confidence beyond the Node loopback proof.
  3. Pause for a shared MS Teams policy
    Maintainers can pause this branch if one explicit cap policy should cover upload, SSO, collection, and attachment metadata paths before any one lands.

Next step before merge

  • No automated repair is needed; maintainer review should decide whether to accept the 16 MiB fail-closed compatibility tradeoff before merge.

Maintainer decision needed

  • Question: Should this PR land the shared 16 MiB fail-closed JSON cap for successful Microsoft Graph upload/share/chat responses in the bundled MS Teams plugin?
  • Rationale: The patch is narrow and uses an existing bounded-reader contract, but it intentionally changes oversized successful external responses from unbounded buffering to a thrown error on a shipped Teams file-send path.
  • Likely owner: steipete — steipete has the strongest combined repository routing and MS Teams drive-upload history signal for accepting a shipped compatibility/security tradeoff.
  • Options:
    • Accept the shared cap (recommended): Merge with the current helper reuse and treat unusually large successful Graph metadata as a fail-closed error.
    • Require live tenant smoke: Ask for a Microsoft 365 tenant upload/share smoke if service-specific confidence is required beyond loopback reader proof.
    • Defer to a broader cap policy: Pause this branch until maintainers decide one response-size policy across the adjacent MS Teams bounded-read PRs.

Security
Cleared: No supply-chain, dependency, secret, auth-header, endpoint, workflow, or permission expansion was found; the diff reduces an unbounded external-response memory risk by reusing an existing SDK helper.

Review details

Best possible solution:

Land the narrow helper reuse if maintainers accept the shared 16 MiB fail-closed cap for MS Teams Graph upload/share/chat metadata, while leaving SSO and attachment bounded-read paths to their separate PRs.

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

Yes. Current main still has seven bare .json() success reads in extensions/msteams/src/graph-upload.ts, and the PR body/test code provides a loopback oversized-response path; I did not run tests in this read-only review.

Is this the best way to solve the issue?

Yes. Reusing the exported openclaw/plugin-sdk/provider-http bounded JSON reader is the narrowest maintainable fix; a bespoke MS Teams reader would duplicate SDK behavior and a broader Graph wrapper refactor is not needed for this bug.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 5b06eba9fe9e.

Label changes

Label justifications:

  • P2: This is a normal-priority MS Teams hardening fix with limited blast radius in one bundled plugin helper and no config, schema, or migration change.
  • merge-risk: 🚨 compatibility: Oversized successful Graph metadata that previously buffered until parsed now fails closed at the shared 16 MiB provider JSON cap.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body includes after-fix terminal output from a real node:http loopback path showing normal chunked parsing, malformed JSON labelling, oversized JSON rejection, and 15 passing graph-upload tests.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix terminal output from a real node:http loopback path showing normal chunked parsing, malformed JSON labelling, oversized JSON rejection, and 15 passing graph-upload tests.
Evidence reviewed

PR surface:

Source +1, Tests +213. Total +214 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 15 14 +1
Tests 1 214 1 +213
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 229 15 +214

What I checked:

  • Repository policy read: Read the full root AGENTS.md and scoped extensions/AGENTS.md; the plugin boundary and compatibility-risk guidance apply because this PR changes bundled plugin external-response behavior. (AGENTS.md:1, 5b06eba9fe9e)
  • Current main still needs the fix: Current main has bare .json() success reads in all seven graph-upload response paths: OneDrive upload, OneDrive createLink, SharePoint upload, driveItem properties, chat lookup, chat members, and SharePoint createLink. (extensions/msteams/src/graph-upload.ts:57, 5b06eba9fe9e)
  • Latest release still has the same unbounded reads: The latest release tag v2026.6.11 still contains the same seven .json() reads in graph-upload.ts, so this is not already shipped. (extensions/msteams/src/graph-upload.ts:57, e085fa1a3ffd)
  • PR head replaces the full success-read set: At PR head, graph-upload.ts imports readProviderJsonResponse and has seven helper calls with no remaining .json() reads in the file. (extensions/msteams/src/graph-upload.ts:58, cbaa9ee39b7e)
  • Shared bounded reader contract: readProviderJsonResponse defaults to the shared 16 MiB provider JSON cap, reads through readResponseWithLimit, and labels malformed or oversized JSON errors. (src/agents/provider-http-errors.ts:315, 5b06eba9fe9e)
  • Overflow behavior cancels the stream: readResponseWithLimit delegates to readResponsePrefix, which cancels the reader when the next chunk exceeds the cap and then throws the overflow error. (src/infra/http-body.ts:229, 5b06eba9fe9e)

Likely related people:

  • steipete: Peter Steinberger refactored the shared Teams drive upload flow in the central file and is also the strongest repository routing signal from CODEOWNERS for compatibility/security tradeoffs. (role: feature-history contributor; confidence: medium; commits: 6b04ab1e35ed, be328e6cd1c6; files: extensions/msteams/src/graph-upload.ts, extensions/msteams/src/graph-upload.test.ts, .github/CODEOWNERS)
  • vincentkoc: Vincent Koc has the most recent current-main touch carrying this file forward and adjacent MS Teams graph-upload test isolation work. (role: recent area contributor; confidence: medium; commits: e311943700ae, bd1e78ea34a3, e085fa1a3ffd; files: extensions/msteams/src/graph-upload.ts, extensions/msteams/src/graph-upload.test.ts, extensions/msteams/src/attachments/graph.ts)
  • sudie-codes: The Graph chat ID and DM file-upload behavior hardened by this PR traces through the March 2026 Graph API file-upload fix authored by sudie-codes. (role: feature-history contributor; confidence: medium; commits: 06845a1974a3, 355794c24a39; files: extensions/msteams/src/graph-upload.ts, extensions/msteams/src/send-context.ts)
  • feiskyer: PR metadata maps the personal-DM media Graph chat resolution caller work to feiskyer, which is one runtime entry point into resolveGraphChatId. (role: adjacent caller contributor; confidence: medium; commits: 78389b1f02db; files: extensions/msteams/src/monitor-handler/message-handler.ts, extensions/msteams/src/graph-upload.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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 keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (6 earlier review cycles)
  • reviewed 2026-07-02T14:06:11.907Z sha 122f008 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-05T12:25:24.616Z sha 9b05dfb :: needs maintainer review before merge. :: none
  • reviewed 2026-07-05T13:08:41.228Z sha 2523a25 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-06T03:34:16.445Z sha 2e34d7a :: needs maintainer review before merge. :: none
  • reviewed 2026-07-06T04:26:39.523Z sha c6aab05 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-06T06:30:06.137Z sha a0a1914 :: needs maintainer review before merge. :: none

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P2 Normal backlog priority with limited blast radius. labels Jun 29, 2026
@Alix-007
Alix-007 force-pushed the fix/bound-msteams-graph-upload branch from 7b10350 to cea2c0c Compare June 29, 2026 13:41
@Alix-007

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Addressed the review feedback:

  • removed the redundant AddressInfo assertion that was failing lint;
  • rebased onto latest main so readProviderJsonResponse is the bounded 16 MiB implementation;
  • added a real loopback oversized-success-JSON assertion for uploadToOneDrive.

Focused validation now passes locally: graph-upload.test.ts 15/15 and oxlint 0 problems.

@clawsweeper

clawsweeper Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot added 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. 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. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Jun 29, 2026
@Alix-007
Alix-007 force-pushed the fix/bound-msteams-graph-upload branch from cea2c0c to 122f008 Compare June 30, 2026 16:33
@Alix-007

Copy link
Copy Markdown
Contributor Author

Rebased onto latest upstream/main to refresh the unrelated QA Smoke failure. The failed scenario was runtime-inventory-drift-check, outside the MSTeams graph-upload path.\n\nValidation run locally:\n- node scripts/run-vitest.mjs extensions/msteams/src/graph-upload.test.ts -- --run\n- node scripts/generate-docs-map.mjs --check

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 🔁 re-review loop A fresh ClawSweeper review was explicitly requested after the latest review. and removed 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. 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 removed proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Jul 6, 2026
@Alix-007
Alix-007 force-pushed the fix/bound-msteams-graph-upload branch from 0b22167 to c6aab05 Compare July 6, 2026 04:17
@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. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. labels Jul 6, 2026
@Alix-007
Alix-007 force-pushed the fix/bound-msteams-graph-upload branch 2 times, most recently from a0a1914 to cbaa9ee Compare July 6, 2026 16:06
@steipete steipete self-assigned this Jul 7, 2026
@steipete

steipete commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Land-ready maintainer pass complete.

  • Work done: kept the shared readProviderJsonResponse implementation across all seven successful Microsoft Graph JSON reads, reduced the regression from four duplicate loopback cases to one focused real streamed-overflow case, and added the maintainer changelog entry thanking @Alix-007.
  • Focused proof: sanitized AWS Crabbox run run_1708475e8ef0 on prepared head 4f6e0932cf64e8a0554388ba1cea5cccb8b175bd; /usr/local/bin/pnpm test extensions/msteams/src/graph-upload.test.ts passed 12/12 tests (1 file, 120 ms test time).
  • Broad proof: exact-head CI run https://github.com/openclaw/openclaw/actions/runs/28855798227 completed successfully; build, lint, production/test types, extension boundaries, contracts, compact Node shards, and QA smoke all passed.
  • Review: fresh branch autoreview reported no accepted/actionable findings.
  • Before/after: successful Graph bodies previously used unbounded Response.json() reads; the retained loopback proof now confirms a streamed body above 16 MiB fails with msteams.graph-upload.uploadOneDriveFile: JSON response exceeds 16777216 bytes.
  • Known gap: no live Microsoft 365 tenant call; normal Graph response shapes remain covered by existing OneDrive/SharePoint unit tests and the Microsoft Graph contract.

@steipete
steipete merged commit 0f775fa into openclaw:main Jul 7, 2026
102 of 104 checks passed
@steipete

steipete commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Merged via squash.

github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jul 8, 2026
…d to prevent OOM (openclaw#97784)

* fix(msteams): bound Microsoft Graph API response reads in graph-upload to prevent OOM

* test(msteams): prove graph upload oversized JSON rejection

* test(msteams): tighten Graph response bound proof

---------

Co-authored-by: Peter Steinberger <[email protected]>
giodl73-repo pushed a commit to giodl73-repo/openclaw that referenced this pull request Jul 8, 2026
…d to prevent OOM (openclaw#97784)

* fix(msteams): bound Microsoft Graph API response reads in graph-upload to prevent OOM

* test(msteams): prove graph upload oversized JSON rejection

* test(msteams): tighten Graph response bound proof

---------

Co-authored-by: Peter Steinberger <[email protected]>
wheakerd pushed a commit to wheakerd/clawdbot that referenced this pull request Jul 15, 2026
…d to prevent OOM (openclaw#97784)

* fix(msteams): bound Microsoft Graph API response reads in graph-upload to prevent OOM

* test(msteams): prove graph upload oversized JSON rejection

* test(msteams): tighten Graph response bound proof

---------

Co-authored-by: Peter Steinberger <[email protected]>
(cherry picked from commit 0f775fa)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

channel: msteams Channel integration: msteams 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

Development

Successfully merging this pull request may close these issues.

2 participants