fix(msteams): bound SSO API JSON response reads to prevent OOM#97781
fix(msteams): bound SSO API JSON response reads to prevent OOM#97781Alix-007 wants to merge 19 commits into
Conversation
|
Codex review: needs changes before merge. Reviewed July 12, 2026, 11:51 PM ET / July 13, 2026, 03:51 UTC. Summary PR surface: Source +428, Tests +444. Total +872 across 6 files. Reproducibility: yes. from source inspection: the live SDK handlers read the User Token success body without OpenClaw's byte limit, and the branch's registered-route tests exercise the replacement path. A live tenant-level OOM reproduction was not attempted. Review metrics: 1 noteworthy metric.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Copy recommended automerge instructionNext step before merge
Security Review findings
Review detailsBest possible solution: Keep the bounded local handlers, but make their invoke status and response body exactly match the pinned Teams/Bot Framework contract for success, fallback, malformed input, missing token, and service failure; resolve the non-callable CodeQL path and prove all cases through the registered routes. Do we have a high-confidence way to reproduce the issue? Yes from source inspection: the live SDK handlers read the User Token success body without OpenClaw's byte limit, and the branch's registered-route tests exercise the replacement path. A live tenant-level OOM reproduction was not attempted. Is this the best way to solve the issue? Not yet. Moving both live routes onto the bounded helper is the correct ownership boundary, but it is only the best fix once the replacement preserves the SDK's complete invoke-response contract and the latest CodeQL alert is resolved. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 6dfa4c27a3ad. Label changesLabel justifications:
Evidence reviewedPR surface: Source +428, Tests +444. Total +872 across 6 files. View PR surface stats
Acceptance criteria:
What I checked:
Likely related people:
What the crustacean ranks mean
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
Review history (11 earlier review cycles; latest 8 shown)
|
25ab029 to
fb04443
Compare
|
@clawsweeper re-review Addressed the blocking review feedback:
I also attempted the extension test type graph directly; the previous missing-export error is gone, and the remaining failures are unrelated existing acpx/qa-lab type/dependency errors outside this PR's touched files. |
|
🦞🧹 I asked ClawSweeper to review this item again. |
NianJiuZst
left a comment
There was a problem hiding this comment.
This change moves the successful Microsoft Teams SSO User Token response read onto the shared bounded response reader. That is the right owner boundary for the OOM guard, and the 16 MiB cap matches the existing provider JSON cap pattern, but the submitted test does not pass against the current base.
LOC: +176/-1 (2 files)
Findings: one blocking test/base compatibility issue, inline.
Best-fix verdict: acceptable mitigation once the test helper reference is fixed and proof is refreshed.
Alternatives considered: readProviderJsonResponse would also bound the body but would require reshaping this helper's existing error strings; keeping readResponseWithLimit local to extensions/msteams/src/sso.ts is fine. Re-exporting a production memory store just for this test is less clean than reusing the local in-memory-store pattern already in extensions/msteams/src/monitor-handler.sso.test.ts, unless the helper is intentionally promoted for multiple tests.
Code read: extensions/msteams/src/sso.ts, extensions/msteams/src/sso-token-store.ts, extensions/msteams/src/monitor-handler.sso.test.ts, extensions/msteams/src/http-error.ts, src/agents/provider-http-errors.ts, packages/media-core/src/read-response-with-limit.ts, packages/media-core/src/read-response-with-limit.test.ts, extensions/exa/src/exa-web-search-provider.runtime.ts, extensions/AGENTS.md.
Evidence: git fetch upstream pull/97781/merge:refs/remotes/upstream/pr/97781-merge; OPENCLAW_VITEST_FS_MODULE_CACHE_PATH=/tmp/openclaw-pr-97781-merge.XyNbsk/.artifacts/vitest-cache node scripts/run-vitest.mjs extensions/msteams/src/sso.test.ts extensions/msteams/src/monitor-handler.sso.test.ts fails on the merge ref with TypeError: createMSTeamsSsoTokenStoreMemory is not a function; GitHub check-test-types also fails on the same missing export. git diff --check upstream/main...refs/remotes/upstream/pr/97781-merge is clean.
Remaining uncertainty: I did not run a live Teams SSO flow; the runtime path review is based on source, sibling tests, the shared bounded-reader contract, and the merge-ref loopback proof.
| // but it would NOT surface the "msteams.sso" label in the error message. | ||
| import http from "node:http"; | ||
| import { afterEach, describe, expect, it, vi } from "vitest"; | ||
| import { createMSTeamsSsoTokenStoreMemory } from "./sso-token-store.js"; |
There was a problem hiding this comment.
This test imports createMSTeamsSsoTokenStoreMemory, but that helper is not exported on the current base. The PR head was cut from an older merge-base where the helper existed; once GitHub merges it with current main, extensions/msteams/src/sso-token-store.ts only exports makeMSTeamsSsoTokenStoreKey and createMSTeamsSsoTokenStoreFs, so the new loopback tests fail before they exercise the bounded-read path. I reproduced this on refs/pull/97781/merge: node scripts/run-vitest.mjs extensions/msteams/src/sso.test.ts extensions/msteams/src/monitor-handler.sso.test.ts fails with TypeError: createMSTeamsSsoTokenStoreMemory is not a function, and CI check-test-types reports TS2724 for this import. Please inline the same small memory-store pattern already used in extensions/msteams/src/monitor-handler.sso.test.ts or intentionally add/export a current test helper before relying on this import.
fb04443 to
b393217
Compare
|
Rebased onto latest upstream/main to refresh the stale Real behavior proof check. Local validation remains green:\n- |
|
@clawsweeper re-review Current head is clean and the previous missing Focused validation passed locally:
The PR body/test includes real loopback-server proof for the bounded 16 MiB User Token service response path. |
…lict # Conflicts: # extensions/msteams/src/monitor.ts # extensions/msteams/src/sso.ts
…pair/pr97781-conflict # Conflicts: # extensions/msteams/src/sso.ts
|
Closing this contribution because the change expanded from a bounded response read into replacing two production SSO invoke routes. The latest review still identifies authentication response-compatibility and static-analysis concerns, and completing the change would require broader tenant-level validation than is appropriate for this narrow hardening PR. The branch remains available for reference. A future attempt should either use an upstream or injectable SDK transport seam, or preserve the pinned SDK status and response-body contract with focused route-level and live tenant proof. |
Summary
signin.token-exchangeandsignin.verify-stateroutes through OpenClaw's bounded SSO User Token helper instead of the Teams SDK's unbounded axiosres.datapath.activity.from.idandactivity.from.aadObjectIdaliases.Linked context
No linked issue; this is a narrow follow-up to the bounded-response-read hardening series for Microsoft Teams SSO.
Real behavior proof
Behavior addressed: The previous PR version bounded only the direct SSO helper path. The production monitor-registered
signin.token-exchangeandsignin.verify-stateroutes still delegated to the Teams SDK handlers, whose User Token client reads axiosres.datawithout a response-size cap. This update moves those live routes onto OpenClaw's bounded helper while preserving the SDK cloud token-service URL viaapp.cloud.tokenServiceUrl.Real environment tested: Linux x86-64, Node 22.22.0, local OpenClaw worktree with installed repo dependencies, real Node/Web
Responsestreams, and the msteams extension test runtime.Exact steps or command run after this patch:
Evidence after fix:
The new monitor lifecycle coverage calls the actual registered
signin.token-exchangeandsignin.verify-stateroute callbacks. The success case verifies both routes callfetchthrough the bounded OpenClaw helper, persist both Teams user-id aliases, and do not callapp.onTokenExchange/app.onVerifyState. The oversized case streams >16 MiB responses through those same registered routes and verifies they fail closed without saving a token or delegating to the SDK handlers. The USGov case verifies those same registered routes callhttps://tokengcch.botframework.azure.us/api/usertoken/..., proving the bounded replacement preserves SDK cloud token routing instead of falling back to the public Bot Framework token host.Observed result after fix: Oversized successful Bot Framework User Token responses are capped before JSON parsing on the live SSO route path. Normal token exchange and verify-state responses still return
{ status: 200 }and persist tokens for later OpenClaw use. Non-public Teams cloud SSO keeps the SDK's configured User Token service host.What was not tested: A live Microsoft Teams tenant / Bot Framework OAuth connection. No tenant credentials are available in this environment.
Tests and validation
node scripts/run-vitest.mjs extensions/msteams/src/sso.test.ts extensions/msteams/src/monitor-handler.sso.test.ts extensions/msteams/src/monitor.lifecycle.test.ts extensions/msteams/src/sdk.test.ts -- --run- 4 files, 41 tests passed../node_modules/.bin/oxfmt --check ...- all matched files use the correct format.node scripts/run-oxlint.mjs ...- passed.Risk checklist
res.data.Current review state
b770b4b30122caed9624b6366d4ad8880dc6dc04.