Skip to content

fix(msteams): bound Bot Framework User Token JSON response read#101110

Closed
cxbAsDev wants to merge 1 commit into
openclaw:mainfrom
cxbAsDev:fix/msteams-sso-json-bound
Closed

fix(msteams): bound Bot Framework User Token JSON response read#101110
cxbAsDev wants to merge 1 commit into
openclaw:mainfrom
cxbAsDev:fix/msteams-sso-json-bound

Conversation

@cxbAsDev

@cxbAsDev cxbAsDev commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

callUserTokenService in the MSTeams SSO handler called response.json() directly on the Bot Framework User Token service response. An oversized or malicious JSON body could be buffered without limit, leading to OOM during token exchange.

Why This Change Was Made

Replace the bare response.json() with readProviderJsonResponse capped at 64 KiB. The existing catch path already returns { error: "invalid JSON from User Token service", status: response.status }, so the handler now treats an oversized body as an invalid response instead of crashing.

User Impact

Teams SSO token exchanges are resilient to unexpectedly large User Token service responses.

Evidence

  • Added regression test in extensions/msteams/src/monitor-handler.sso.test.ts asserting the handler returns an unexpected_response error when the JSON body exceeds the cap.
  • Added real behavior proof at scripts/proof/msteams-sso-json-bound.mts that serves an oversized User Token JSON response from a local HTTP server and verifies the handler rejects cleanly.
  • Local run:
    • pnpm test extensions/msteams/src/monitor-handler.sso.test.ts passed
    • OPENCLAW_OXLINT_SKIP_PREPARE=1 node scripts/run-oxlint.mjs extensions/msteams/src/sso.ts extensions/msteams/src/monitor-handler.sso.test.ts scripts/proof/msteams-sso-json-bound.mts passed
    • node --import tsx scripts/proof/msteams-sso-json-bound.mts:
=== Proof: MSTeams SSO JSON response bound ===

PASS: oversized User Token JSON response was rejected without OOM.

@openclaw-barnacle openclaw-barnacle Bot added channel: msteams Channel integration: msteams scripts Repository scripts size: S labels Jul 6, 2026
@clawsweeper

clawsweeper Bot commented Jul 6, 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 branch fixes the same Microsoft Teams SSO User Token JSON read as the earlier canonical open PR, while the canonical PR covers the production registered SSO routes and has stronger proof.

Root-cause cluster
Relationship: superseded
Canonical: #97781
Summary: The current PR is a narrower helper-level duplicate of the canonical Microsoft Teams SSO bounded-read PR, which remains open, clean, and proof-positive.

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, which carries the same bounded-read hardening plus production-route and sovereign-cloud coverage.

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, which carries the same bounded-read hardening plus production-route and sovereign-cloud coverage.

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

Yes by source inspection. Current main uses bare response.json() for successful Bot Framework User Token responses, and the PR body includes terminal proof from a local HTTP server showing oversized responses are rejected after the change.

Is this the best way to solve the issue?

No for this branch as the landing path. The bounded-read direction is useful, but the earlier canonical PR is the better solution because it fixes the same helper and the production registered SSO route boundary with stronger tests.

Security review:

Security review cleared: The diff narrows an external SSO response-read boundary and adds a local proof script; it does not add dependencies, workflows, permissions, or secret-handling changes.

AGENTS.md: found and applied where relevant.

What I checked:

  • Current main still has the unbounded helper read: Current main parses successful Bot Framework User Token service responses with bare response.json() in callUserTokenService, which is the exact helper path this PR targets. (extensions/msteams/src/sso.ts:133, 565097fc1001)
  • Current production routes still delegate to SDK SSO handlers: Current main registers signin.token-exchange and signin.verify-state through SDK delegate handlers, so a helper-only fix does not address the wider live route boundary covered by the canonical PR. (extensions/msteams/src/monitor.ts:516, 565097fc1001)
  • This PR is a narrow helper-level duplicate: The current PR replaces the same success-path JSON read with readProviderJsonResponse and a 64 KiB cap, plus one helper-level regression test and one local proof script. (extensions/msteams/src/sso.ts:134, 0dcb0c4e52ed)
  • Canonical PR is open, clean, and proof-positive: The earlier canonical PR is open, non-draft, mergeable/clean, labeled proof: sufficient, and its current review marks it ready for maintainer look with route-level proof. (efffccc03c29)
  • Canonical PR covers the live SSO route boundary: The canonical PR changes the same helper and also replaces the registered signin.token-exchange and signin.verify-state routes with bounded local handlers, with tests for normal, oversized, and USGov token-host behavior. (extensions/msteams/src/monitor.ts:554, efffccc03c29)
  • Earlier duplicate was already superseded by the same canonical PR: A prior helper-level SSO bounded-read PR was closed by its author after ClawSweeper identified the same canonical PR as the stronger landing path. (ebe27a928b58)

Likely related people:

  • sudie-codes: Authored the merged Microsoft Teams SSO implementation that added callUserTokenService, the SSO invoke handlers, and the adjacent SSO tests. (role: introduced behavior; confidence: high; commits: 828ebd43d47b; files: extensions/msteams/src/sso.ts, extensions/msteams/src/monitor.ts, extensions/msteams/src/monitor-handler.sso.test.ts)
  • heyitsaamir: Authored the merged Teams SDK migration that changed the current monitor route and SDK boundary around the SSO invoke path. (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 that owns the production route compatibility question the canonical PR now handles. (role: merger; confidence: high; commits: 04c29825356f; files: extensions/msteams/src/monitor.ts, extensions/msteams/src/sdk.ts)
  • Vincent Koc: Current blame and history show recent carry-forward work on the Microsoft Teams SSO and monitor files on current main. (role: recent area contributor; confidence: medium; commits: 3ad77774f0e5; files: extensions/msteams/src/sso.ts, extensions/msteams/src/monitor.ts)
  • Alix-007: Owns the earlier open canonical PR for the same Microsoft Teams SSO bounded-read problem, which is mergeable and proof-positive. (role: canonical fix author; confidence: medium; commits: efffccc03c29; files: extensions/msteams/src/sso.ts, extensions/msteams/src/monitor.ts, extensions/msteams/src/monitor.lifecycle.test.ts)

Codex review notes: model internal, reasoning high; reviewed against 565097fc1001.

@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. P2 Normal backlog priority with limited blast radius. labels Jul 6, 2026
@clawsweeper clawsweeper Bot closed this Jul 6, 2026
@clawsweeper

clawsweeper Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

🦞✅
ClawSweeper autoclose is complete.

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

Closed:

This was referenced Jul 7, 2026
Closed
Closed
Closed
@cxbAsDev
cxbAsDev deleted the fix/msteams-sso-json-bound branch July 15, 2026 03:33
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. scripts Repository scripts 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

@cxbAsDev