Skip to content

fix(msteams): bound User Token service JSON response reads to prevent OOM#101585

Closed
LeonidasLux wants to merge 1 commit into
openclaw:mainfrom
LeonidasLux:fix/msteams-bounded-json-response
Closed

fix(msteams): bound User Token service JSON response reads to prevent OOM#101585
LeonidasLux wants to merge 1 commit into
openclaw:mainfrom
LeonidasLux:fix/msteams-bounded-json-response

Conversation

@LeonidasLux

@LeonidasLux LeonidasLux commented Jul 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Related: #95218, #96322, #96495, #96889

What Problem This Solves

Fixes an issue where the MSTeams SSO callUserTokenService path uses
unbounded response.json() to parse the Bot Framework User Token
service authorization success body. A misbehaving, compromised, or
self-hosted User Token service endpoint can stream an arbitrarily
large JSON response, causing the gateway process to buffer it all
into memory — leading to memory pressure or OOM on the Teams SSO
token-exchange path.

The shared bounded JSON reader (#95218) already covers the main
provider pipeline, image generation (#96495), video generation
(#96889), and the MiniMax OAuth path (#96322). The MSTeams SSO User
Token service call was missed in those rounds and is closed here.

Why This Change Was Made

Replace response.json() with
readProviderJsonResponse(response, "msteams.sso-user-token") —
the same shared helper already used across the codebase. This
enforces a 16 MiB default cap and, on overflow, cancels the
underlying stream and throws a bounded error
(msteams.sso-user-token: JSON response exceeds 16777216 bytes).
Malformed-JSON error messages are preserved for backward
compatibility.

No new abstraction — reuses the existing plugin-sdk bounded reader.
The error-response path (readMSTeamsHttpErrorDetail) already used
the bounded extractProviderErrorDetail helper and is unchanged.

User Impact

No user-visible impact for normal operation. Operators who configure
MSTeams SSO against a self-hosted or proxied User Token service
endpoint are now protected against oversized authorization responses
instead of silently buffering them unbounded.

Evidence

Unit tests

The in-repo Vitest suite for the MSTeams SSO handler passes in full
(6/6), covering the happy path, HTTP service errors, missing-user
rejection, and the new malformed-JSON error path.

 ✓ extensions/msteams/src/monitor-handler.sso.test.ts (6 tests) 76ms

 Test Files  1 passed (1)
      Tests  6 passed (6)

Error response path already bounded

readMSTeamsHttpErrorDetail (called on non-OK responses) delegates
to extractProviderErrorDetail from plugin-sdk/provider-http,
which uses the same bounded reader.

L2 — Real behavior proof: oversized JSON capped at 16 MiB

A proof script (evidence/pr-101585/oversized-json-proof.ts) exercises
the exact readProviderJsonResponse call used in this fix with a
16.96 MiB JSON payload (well over the 16 MiB PROVIDER_JSON_RESPONSE_MAX_BYTES
default) to confirm the bounded reader cancels the stream before
buffering the full response.

Oversized JSON payload: 16.96 MiB (17784699 bytes, 139811 entries)

========== AFTER FIX: readProviderJsonResponse ==========
✅ PASS: bounded reader rejected oversized JSON in 7.1 ms
   → Error: msteams.sso-user-token: JSON response exceeds 16777216 bytes

========== BEFORE FIX: response.json() ==========
response.json() buffered 16.96 MiB successfully in 51.9 ms
   → Parsed 139811 entries — no byte cap, full buffering
   ❌ RISK: a malicious or compromised User Token service can stream
      unlimited data, causing memory pressure / OOM on the gateway.

Result: After this fix, an oversized User Token service JSON response
is detected at 16 MiB and the underlying stream is cancelled immediately
(7.1 ms). Before this fix, response.json() would buffer the entire
untrusted payload uncontested (51.9 ms for 16.96 MiB; no upper bound).

What was not tested

  • End-to-end integration against a live Bot Framework User Token service
    endpoint (requires AAD app registration and Teams client context).
  • The handleSigninVerifyStateInvoke path — it calls the same
    callUserTokenService helper and is covered by the same
    readProviderJsonResponse swap; the regression test focuses on
    handleSigninTokenExchangeInvoke as the more exercised code path.

AI-assisted.

@LeonidasLux @claude
fix(msteams): bound User Token service JSON response reads to prevent…
250d1e8 
… OOM

Replace unbounded `response.json()` in `callUserTokenService` with
`readProviderJsonResponse(response, "msteams.sso-user-token")` —
the same shared bounded reader already deployed across the codebase
(openclaw#95218, openclaw#96322, openclaw#96495, openclaw#96889). Enforces a 16 MiB default cap
and cancels the underlying stream on overflow.

Closes the last unbounded JSON read on the MSTeams SSO token-exchange
path. Error responses were already bounded via `extractProviderErrorDetail`.

Co-Authored-By: Claude <[email protected]>
@openclaw-barnacle openclaw-barnacle Bot added channel: msteams Channel integration: msteams size: XS labels Jul 7, 2026
@clawsweeper

clawsweeper Bot commented Jul 7, 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: this helper-level branch overlaps the same Microsoft Teams SSO User Token JSON-read hardening already owned by the open, proof-positive route-level PR that covers the live registered sign-in paths.

Root-cause cluster
Relationship: superseded
Canonical: #97781
Summary: The open canonical PR owns the same Microsoft Teams SSO bounded-read problem and additionally fixes the production registered sign-in route path; this PR is another helper-level duplicate.

Members:

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

Canonical path: Close this duplicate and continue review on #97781 as the canonical route-level Microsoft Teams SSO bounded-read fix.

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

Review details

Best possible solution:

Close this duplicate and continue review on #97781 as the canonical route-level Microsoft Teams SSO bounded-read fix.

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

Yes, from source inspection: current main still has raw response.json() in the helper, and the live registered sign-in routes still delegate through SDK handlers. I did not run a live Teams tenant repro in this read-only review.

Is this the best way to solve the issue?

No for this branch as the best landing path. The helper swap is plausible, but #97781 covers the same helper plus the live registered route boundary and sovereign-cloud parity with stronger proof.

Security review:

Security review cleared: The diff narrows an external JSON response-read boundary and adds no dependencies, scripts, workflow changes, permissions, package-resolution changes, or secret handling.

AGENTS.md: found and applied where relevant.

What I checked:

  • Repository policy read: Root and scoped extension policy were read; plugin-boundary and PR duplicate/supersession guidance shaped this review. (AGENTS.md:1, 2ba622ca3019)
  • Current helper behavior: Current main still parses successful Bot Framework User Token responses with raw response.json() inside callUserTokenService, which is the helper-level read this PR changes. (extensions/msteams/src/sso.ts:133, 2ba622ca3019)
  • Current live route behavior: Current main registers signin.token-exchange and signin.verify-state to handleSdkSigninInvoke, which delegates to SDK sign-in handlers rather than the helper changed by this PR. (extensions/msteams/src/monitor.ts:516, 2ba622ca3019)
  • Current PR diff: The PR diff changes only extensions/msteams/src/sso.ts at the helper parse point and adds a malformed-JSON unit test in monitor-handler.sso.test.ts. (extensions/msteams/src/sso.ts:133, 250d1e826fb6)
  • Canonical PR state: The earlier Microsoft Teams SSO bounded-read PR is open, mergeable, labeled proof sufficient, and its review identifies it as the canonical route-level fix with live registered route coverage. (efffccc03c29)
  • Prior duplicate cleanup: Two earlier helper-level Microsoft Teams SSO bounded-read PRs were already closed or marked superseded by the same canonical PR, confirming the cluster direction.

Likely related people:

  • sudie-codes: Authored the merged Teams SSO implementation that added callUserTokenService, handleSigninTokenExchangeInvoke, and the adjacent SSO tests. (role: introduced behavior; confidence: high; commits: 828ebd43d47b; files: extensions/msteams/src/sso.ts, extensions/msteams/src/monitor-handler.sso.test.ts)
  • heyitsaamir: Authored the merged Teams SDK migration that established the current SDK sign-in delegate boundary for the live routes. (role: major refactor author; confidence: high; commits: 04c29825356f; files: extensions/msteams/src/monitor.ts, extensions/msteams/src/sdk.ts, extensions/msteams/src/sso.ts)
  • steipete: Merged the Teams SDK migration and appears in current GitHub commit metadata for recent carry-forward work on the touched Teams files. (role: merger and recent area contributor; confidence: medium; commits: 04c29825356f, 6b76a306d472; files: extensions/msteams/src/monitor.ts, extensions/msteams/src/sso.ts)
  • Alix-007: Authored the shared bounded provider JSON reader PR and owns the open canonical Microsoft Teams SSO route-level bounded-read PR. (role: canonical fix author and adjacent owner; confidence: high; commits: 2592f8a51a4e, efffccc03c29; files: src/agents/provider-http-errors.ts, src/agents/provider-http-errors.test.ts, extensions/msteams/src/sso.ts)
  • BradGroux: Merged the original Teams SSO implementation PR that created the helper surface being hardened. (role: merger; confidence: medium; commits: 828ebd43d47b; files: extensions/msteams/src/sso.ts, extensions/msteams/src/monitor-handler.sso.test.ts)

Codex review notes: model internal, reasoning high; reviewed against 2ba622ca3019.

@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 Jul 7, 2026
@BradGroux BradGroux mentioned this pull request Jul 7, 2026
Draft
@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: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed 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. labels Jul 7, 2026
@clawsweeper clawsweeper Bot closed this Jul 7, 2026
@clawsweeper

clawsweeper Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

🦞✅
ClawSweeper autoclose is complete.

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

Closed:

This was referenced Jul 7, 2026
Closed
Closed
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 P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. size: XS 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

@LeonidasLux