Skip to content

fix(msteams): token refresh hangs past deadline when DNS preflight stalls - #111317

Closed
hugenshen wants to merge 1 commit into
openclaw:mainfrom
hugenshen:fix/msteams-token-timeout-ms
Closed

fix(msteams): token refresh hangs past deadline when DNS preflight stalls#111317
hugenshen wants to merge 1 commit into
openclaw:mainfrom
hugenshen:fix/msteams-token-timeout-ms

Conversation

@hugenshen

@hugenshen hugenshen commented Jul 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes an issue where Microsoft Teams delegated token refresh / exchange could hang past the 10s token-fetch deadline when SSRF DNS/proxy preflight never completes. fetchMSTeamsTokens only aborted via AbortSignal.timeout() on init.signal, which does not cover guarded-fetch preflight, so a stalled lookup can leave the Teams channel unable to refresh auth and stay connected.

Why This Change Was Made

Pass MSTEAMS_DEFAULT_TOKEN_FETCH_TIMEOUT_MS as top-level fetchWithSsrFGuard({ timeoutMs }), matching sibling graph.ts / Graph attachment callers and the post-#105549 guard-owned timeout contract. Remove AbortSignal.timeout() from RequestInit. Optional test-only fetchImpl / lookupFn / timeoutMs hooks let the refresh entry point exercise the real guarded-fetch owner without mocking it away.

User Impact

Before: Teams token refresh could hang indefinitely during DNS/proxy preflight despite the 10s token-fetch budget, leaving the channel disconnected after token expiry.

After: Refresh/exchange rejects with TimeoutError / request timed out when preflight stalls, and HTTP fetch is never called.

Evidence

  • Exact head: bb797ba40c8751caaa17bc6aa7e23d2e13da1949
  • Changed: extensions/msteams/src/oauth.token.ts
  • Production caller under test: refreshMSTeamsDelegatedTokens → fetchMSTeamsTokens → fetchWithSsrFGuard({ timeoutMs }) with never-resolving lookupFn
  • Regression: extensions/msteams/src/oauth.token.preflight-timeout.test.ts + updated oauth.test.ts (asserts top-level timeoutMs, no init.signal)
  • Sibling on same plugin: graph.ts already forwards guard timeoutMs

Unit + preflight proof (exit 0)

node scripts/run-vitest.mjs extensions/msteams/src/oauth.token.preflight-timeout.test.ts extensions/msteams/src/oauth.test.ts --reporter=verbose
 ✓ |extension-msteams| extensions/msteams/src/oauth.test.ts > exchangeMSTeamsCodeForTokens > exchanges an authorization code for delegated tokens
 ✓ |extension-msteams| extensions/msteams/src/oauth.test.ts > refreshMSTeamsDelegatedTokens > refreshes tokens using refresh_token grant and keeps old refresh token when Azure omits it
stdout | extensions/msteams/src/oauth.token.preflight-timeout.test.ts > refreshMSTeamsDelegatedTokens preflight timeout > times out when preflight lookup stalls before HTTP dispatch
[msteams token refresh preflight stall proof] timed_out=true name=TimeoutError message=request timed out elapsed_ms=82 fetch_called=0

 ✓ |extension-msteams| extensions/msteams/src/oauth.token.preflight-timeout.test.ts > refreshMSTeamsDelegatedTokens preflight timeout > times out when preflight lookup stalls before HTTP dispatch

 Test Files  2 passed (2)
      Tests  20 passed (20)

Standalone exact-head runtime proof (exit 0, non-Vitest)

PROOF_ROOT=. PROOF_HEAD=$(git rev-parse HEAD) PROOF_TIMEOUT_MS=80 \
  node --import tsx /tmp/msteams-token-preflight-timeout-proof.mjs
[standalone] exact_head=bb797ba40c8751caaa17bc6aa7e23d2e13da1949 caller=refreshMSTeamsDelegatedTokens→fetchMSTeamsTokens→fetchWithSsrFGuard timeoutMs=80
[fetch-timeout] fetch timeout after 80ms (elapsed 82ms) operation=fetchWithSsrFGuard url=https://login.microsoftonline.com/tenant-1/oauth2/v2.0/token
[standalone] RESULT timed_out=true name=TimeoutError message=request timed out elapsed_ms=112 fetch_called=0
[standalone] PASS

Real behavior proof

  • Behavior or issue addressed: MS Teams token refresh/exchange no longer hangs when SSRF DNS/proxy preflight never completes; deadline is owned by fetchWithSsrFGuard timeoutMs.

  • Canonical reachability path: Teams auth refresh → refreshMSTeamsDelegatedTokens → fetchMSTeamsTokens → fetchWithSsrFGuard({ timeoutMs: MSTEAMS_DEFAULT_TOKEN_FETCH_TIMEOUT_MS }) → preflight lookup abort before HTTP dispatch.

  • Boundary crossed: Azure AD token endpoint DNS/proxy preflight → local guarded-fetch timeout → TimeoutError (request timed out).

  • Shared helper / provider constraint check: Reuses fetchWithSsrFGuard top-level timeoutMs (not init.signal / AbortSignal.timeout()). Aligns with extensions/msteams/src/graph.ts and fix(feishu): pass timeoutMs through app-registration guarded fetch #105549. Default remains MSTEAMS_DEFAULT_TOKEN_FETCH_TIMEOUT_MS (10s). No new config/env.

  • Real environment tested: macOS, Node via node --import tsx standalone harness on exact head bb797ba40c8751caaa17bc6aa7e23d2e13da1949 (plus Vitest regression).

  • Exact steps or command run after this patch: PROOF_ROOT=. PROOF_HEAD=$(git rev-parse HEAD) PROOF_TIMEOUT_MS=80 node --import tsx /tmp/msteams-token-preflight-timeout-proof.mjs

  • Evidence after fix: Standalone transcript above: timed_out=true name=TimeoutError message=request timed out elapsed_ms=112 fetch_called=0 then PASS.

  • Observed result after fix: Stalled preflight rejects with TimeoutError / request timed out before HTTP dispatch (fetch_called=0).

  • What was not tested: Live Azure AD token refresh with real tenant credentials; full 10s production-floor duration (proof uses an 80ms stand-in).

  • Fix classification: Root cause fix

  • AI-assisted (Cursor)

  • I understand what the code does

  • Change is focused and does not mix unrelated concerns

@hugenshen
hugenshen marked this pull request as ready for review July 19, 2026 08:38
@openclaw-barnacle openclaw-barnacle Bot added channel: msteams Channel integration: msteams size: S labels Jul 19, 2026
@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. P1 High-priority user-facing bug, regression, or broken workflow. labels Jul 19, 2026
@clawsweeper

clawsweeper Bot commented Jul 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed August 30, 2026, 12:33 AM ET / 04:33 UTC.

ClawSweeper review

What this changes

Moves Microsoft Teams OAuth token exchange and refresh deadlines into guarded fetch and adds regression coverage for a stalled DNS preflight.

Merge readiness

⛔ Blocked before merge - 4 items remain

This remains a useful P1 Teams delegated-auth repair: current main still places the deadline only on the HTTP request signal, while the guarded fetcher needs its top-level timeout to abort DNS/proxy preflight. The conflicting branch must be rebased without losing current-main token-envelope validation, and its injected transport proof is not sufficient real-behavior proof for an external PR.

Priority: P1
Reviewed head: bb797ba40c8751caaa17bc6aa7e23d2e13da1949

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The repair follows the correct timeout boundary, but a conflicting rebase and non-mocked production-transport proof remain before merge confidence is adequate.
Proof confidence 🦪 silver shellfish (2/6) Needs real behavior proof before merge: The changed owner is the Teams delegated-token path, but the supplied standalone run reaches it with injected lookupFn and fetchImpl behavior; it proves the guard's timeout control flow but not an after-fix production transport result. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🦐 gold shrimp (3/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: The changed owner is the Teams delegated-token path, but the supplied standalone run reaches it with injected lookupFn and fetchImpl behavior; it proves the guard's timeout control flow but not an after-fix production transport result. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 5 items Current main remains affected: Current main passes the 10-second deadline only as RequestInit.signal and does not supply fetchWithSsrFGuard's top-level timeoutMs, so its preflight cannot use that deadline.
Guarded-fetch contract: The guard constructs its timeout signal from top-level timeoutMs before wrapping pinned DNS lookup in runAbortablePreflight; the shared SSRF test specifically covers a stalled DNS preflight with timeoutMs.
Current-main token validation must survive rebase: Current main rejects null and array token envelopes before field access, with regression tests for both shapes. History identifies the later current-main change as #111638.
Findings None None.
Security None None.

How this fits together

The Microsoft Teams plugin exchanges and refreshes delegated Azure tokens before Teams operations can use them. Its token request passes through OpenClaw's guarded fetcher, which performs DNS/proxy safety checks before dispatching HTTP.

flowchart LR
A[Teams delegated auth] --> B[Token exchange or refresh]
B --> C[Guarded fetch]
C --> D[DNS and proxy preflight]
D --> E[Microsoft token endpoint]
C --> F[Delegated token or timeout error]
Loading

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: The changed owner is the Teams delegated-token path, but the supplied standalone run reaches it with injected lookupFn and fetchImpl behavior; it proves the guard's timeout control flow but not an after-fix production transport result. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Resolve merge risk (P1) - The supplied GitHub snapshot reports a dirty merge state; rebasing must retain the current-main object guard and null/array envelope tests, but no pinned three-way merge result is available.
  • Resolve merge risk (P1) - The posted terminal evidence injects both DNS lookup and fetch behavior, so it does not yet show after-fix recovery through a production transport client.
  • Complete next step (P2) - A rebase plus contributor-supplied production-boundary proof are needed; this conflicting external branch is not a safe automated repair target.
Agent review details

Security

None.

PR surface

Source +33, Tests +58. Total +91 across 3 files.

View PR surface stats
Area Files Added Removed Net
Source 1 50 17 +33
Tests 2 59 1 +58
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 3 109 18 +91

Review metrics

Metric Value Why it matters
Production versus regression coverage production +50/-17; tests +59/-1 The branch adds a focused timeout path plus internal test injection, so the rebase should keep the added production surface bounded.

Merge-risk options

Maintainer options:

  1. Rebase with auth-envelope protection (recommended)
    Rebase onto current main, preserve its malformed-envelope guard and tests, then add redacted production-transport proof for the preflight timeout.

Technical review

Best possible solution:

Rebase the focused timeout change onto current main, preserve token-envelope validation, and attach a redacted after-fix production-boundary transport trace showing stalled preflight timing out before HTTP dispatch.

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

Yes, from source: current main applies its deadline only to RequestInit.signal, while the guarded fetcher uses top-level timeoutMs to abort its DNS preflight. The submitted run supports that mechanism but is injected rather than a production transport reproduction.

Is this the best way to solve the issue?

Yes, conditionally: placing the timeout at the existing guarded-fetch owner is the narrow fix because that owner wraps DNS/proxy preflight. The branch must first be rebased so it preserves current-main token-response shape validation.

AGENTS.md: found and applied where relevant.

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

Labels

Label justifications:

  • P1: A stalled delegated-token refresh can leave an active Microsoft Teams channel unable to recover authentication after token expiry.
  • merge-risk: 🚨 auth-provider: The conflicting branch changes delegated-token request behavior and must retain current-main malformed token-envelope handling during rebase.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The changed owner is the Teams delegated-token path, but the supplied standalone run reaches it with injected lookupFn and fetchImpl behavior; it proves the guard's timeout control flow but not an after-fix production transport result. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

Likely related people:

  • Alix-007: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Peter Steinberger: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Rebase onto current main while retaining its null/array token-envelope validation and tests.
  • Add a redacted after-fix production-boundary transport trace; update the PR body to trigger re-review, or ask a maintainer for @clawsweeper re-review if it does not.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
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.

Workflow

  • 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.

History

Review history (12 earlier review cycles; latest 8 shown)
  • reviewed 2026-08-04T17:23:06.748Z sha bb797ba :: needs maintainer review before merge. :: none
  • reviewed 2026-08-04T17:31:37.405Z sha bb797ba :: needs maintainer review before merge. :: none
  • reviewed 2026-08-04T18:29:22.146Z sha bb797ba :: needs maintainer review before merge. :: none
  • reviewed 2026-08-13T09:28:02.390Z sha bb797ba :: needs real behavior proof before merge. :: [P2] Remove test-only transport hooks from OAuth entrypoints
  • reviewed 2026-08-15T11:27:10.286Z sha bb797ba :: needs real behavior proof before merge. :: [P2] Remove test-only transport hooks from OAuth entrypoints
  • reviewed August 16, 2026, 4:43 AM ET / 08:43 UTC sha unknown :: needs real behavior proof before merge. :: [P2] Remove test-only transport hooks from OAuth entrypoints
  • reviewed 2026-08-18T12:40:14.105Z sha bb797ba :: needs real behavior proof before merge. :: [P2] Remove test-only transport hooks from OAuth entrypoints
  • reviewed 2026-08-24T04:51:05.441Z sha bb797ba :: needs real behavior proof before merge. :: [P1] Retain token-response shape validation | [P2] Remove test transport overrides from OAuth entrypoints

@BradGroux BradGroux mentioned this pull request Jul 19, 2026
Closed
@hugenshen

hugenshen commented Jul 19, 2026

Copy link
Copy Markdown
Contributor Author

Added standalone exact-head runtime proof (non-Vitest) on bb797ba40c8751caaa17bc6aa7e23d2e13da1949:

[standalone] exact_head=bb797ba40c8751caaa17bc6aa7e23d2e13da1949 caller=refreshMSTeamsDelegatedTokens→fetchMSTeamsTokens→fetchWithSsrFGuard timeoutMs=80
[fetch-timeout] fetch timeout after 80ms (elapsed 82ms) operation=fetchWithSsrFGuard url=https://login.microsoftonline.com/tenant-1/oauth2/v2.0/token
[standalone] RESULT timed_out=true name=TimeoutError message=request timed out elapsed_ms=112 fetch_called=0
[standalone] PASS

PR body updated with the transcript. @clawsweeper re-review

@hugenshen

hugenshen commented Jul 19, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Please finish review against the standalone exact-head non-Vitest proof already posted and reflected in the PR body ([standalone] PASS). The earlier "review started" placeholder never completed; labels still show status: 📣 needs proof from the pre-proof review.

Standalone proof comment: #111317 (comment)

@clawsweeper

clawsweeper Bot commented Jul 19, 2026 •
edited
Loading

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.

Re-review progress:

@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. 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 19, 2026
@clawsweeper clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. merge-risk: 🚨 auth-provider 🚨 May break OAuth, tokens, provider routing, model choice, or credentials. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. labels Jul 31, 2026
@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. and removed 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. labels Aug 13, 2026
@hugenshen hugenshen closed this Sep 7, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Sep 13, 2026
Open
3 tasks
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: 🚨 auth-provider 🚨 May break OAuth, tokens, provider routing, model choice, or credentials. P1 High-priority user-facing bug, regression, or broken workflow. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. size: S status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@hugenshen