Skip to content

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

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

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 maintainer review before merge. Reviewed July 19, 2026, 11:38 AM ET / 15:38 UTC.

Summary
The branch passes the Teams OAuth token deadline as fetchWithSsrFGuard’s top-level timeoutMs, removes the request-local abort signal, and adds regression coverage for a stalled DNS-preflight lookup.

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

Reproducibility: yes. from source and posted exact-head proof: a never-resolving guarded DNS lookup on the delegated refresh path now rejects with TimeoutError before HTTP dispatch.

Review metrics: none identified.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🐚 platinum hermit
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.

Next step before merge

  • [P2] No repair lane is needed; the patch has no actionable review finding and is ready for ordinary maintainer merge review.

Security
Cleared: The patch changes no dependencies, workflows, permissions, secrets handling, package resolution, or external execution path; no concrete security or supply-chain regression is evident.

Review details

Best possible solution:

Land the focused guard-owned timeout fix after the usual merge-refresh check, retaining the regression test that proves preflight stalls abort before an HTTP request is dispatched.

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

Yes, from source and posted exact-head proof: a never-resolving guarded DNS lookup on the delegated refresh path now rejects with TimeoutError before HTTP dispatch.

Is this the best way to solve the issue?

Yes: using the shared SSRF guard’s own timeoutMs is the narrowest solution because it covers preflight work that a request-local abort signal does not own.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add proof: sufficient: Contributor real behavior proof is sufficient. The contributor posted exact-head standalone terminal output showing the real refresh entry point times out during a stalled guarded preflight and does not invoke HTTP; keep any future proof redacted.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The contributor posted exact-head standalone terminal output showing the real refresh entry point times out during a stalled guarded preflight and does not invoke HTTP; keep any future proof redacted.
  • remove rating: 🦪 silver shellfish: Current PR rating is rating: 🐚 platinum hermit, so this older rating label is no longer current.
  • remove status: 📣 needs proof: Current PR status label is status: 👀 ready for maintainer look.

Label justifications:

  • P1: A stalled delegated-token refresh can leave an active Microsoft Teams channel unable to recover authentication after token expiry.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit 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 contributor posted exact-head standalone terminal output showing the real refresh entry point times out during a stalled guarded preflight and does not invoke HTTP; keep any future proof redacted.
  • proof: sufficient: Contributor real behavior proof is sufficient. The contributor posted exact-head standalone terminal output showing the real refresh entry point times out during a stalled guarded preflight and does not invoke HTTP; keep any future proof redacted.
Evidence reviewed

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

What I checked:

  • Focused production-path change: fetchMSTeamsTokens moves the 10-second deadline from RequestInit.signal to the guarded-fetch call’s top-level timeoutMs, while preserving the existing token endpoint, request body, and default timeout constant. (extensions/msteams/src/oauth.token.ts:83, bb797ba40c87)
  • Regression coverage: The added focused test calls refreshMSTeamsDelegatedTokens with a never-resolving lookup and verifies TimeoutError: request timed out before HTTP dispatch; the existing OAuth tests also assert timeoutMs is top-level and init.signal is absent. (extensions/msteams/src/oauth.token.preflight-timeout.test.ts:11, bb797ba40c87)
  • After-fix runtime proof: The PR body and follow-up comment provide an exact-head standalone Node transcript for refreshMSTeamsDelegatedTokens → fetchMSTeamsTokens → fetchWithSsrFGuard, reporting a timeout after a stalled lookup and fetch_called=0. (extensions/msteams/src/oauth.token.ts:95, bb797ba40c87)
  • Established shared-helper pattern: The supplied related context identifies merged PR 105549 as prior provenance for the same guarded-fetch contract: the top-level timeoutMs owns timeout behavior rather than a duplicate RequestInit.signal path. (extensions/msteams/src/oauth.token.ts:99, ed16970fc617)

Likely related people:

  • hugenshen: Authored this focused change and the earlier merged guarded-fetch timeout work cited by this PR, giving them direct history with the relevant timeout ownership pattern. (role: recent adjacent contributor; confidence: medium; commits: bb797ba40c87, ed16970fc617; files: extensions/msteams/src/oauth.token.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 (2 earlier review cycles)
  • reviewed 2026-07-19T08:42:10.215Z sha bb797ba :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-19T14:55:27.101Z sha bb797ba :: needs maintainer review before merge. :: none

@BradGroux BradGroux mentioned this pull request Jul 19, 2026
Draft
@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
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 P1 High-priority user-facing bug, regression, or broken workflow. 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@hugenshen