Skip to content

fix(auth): avoid SSE OAuth refresh deadlock - #2651

Closed
pragnyanramtha wants to merge 1 commit into
modelcontextprotocol:v1.xfrom
pragnyanramtha:codex/fix-sse-oauth-refresh
Closed

pragnyanramtha wants to merge 1 commit into
modelcontextprotocol:v1.xfrom
pragnyanramtha:codex/fix-sse-oauth-refresh

Conversation

@pragnyanramtha

Copy link
Copy Markdown

Summary

  • preflight OAuth token refresh before opening the long-lived SSE GET
  • add the refreshed bearer token directly to the SSE request and disable HTTPX auth for that stream request
  • keep the OAuth-enabled client for normal POST requests so existing refresh/full-auth behavior remains intact

Fixes #1326.

Validation

  • uv run pytest tests/client/test_auth.py tests/shared/test_sse.py -q
  • uv run pyright src/mcp/client/auth/oauth2.py src/mcp/client/sse.py tests/client/test_auth.py tests/shared/test_sse.py
  • uv run ruff check src/mcp/client/auth/oauth2.py src/mcp/client/sse.py tests/client/test_auth.py tests/shared/test_sse.py
  • uv run ruff format --check src/mcp/client/auth/oauth2.py src/mcp/client/sse.py tests/client/test_auth.py tests/shared/test_sse.py
  • git diff --check

@pragnyanramtha
pragnyanramtha force-pushed the codex/fix-sse-oauth-refresh branch from fdfe5d4 to c24d7a1 Compare May 20, 2026 20:05
@pragnyanramtha
pragnyanramtha marked this pull request as ready for review May 20, 2026 20:09
Copilot AI review requested due to automatic review settings May 20, 2026 20:09

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds a preflight OAuth token refresh step before opening SSE streams to avoid OAuth generator/streaming deadlocks, with regression tests covering refreshed/valid/missing-refresh-token/failure scenarios.

Changes:

  • Add _prepare_request_with_refresh() to OAuthClientProvider for refreshing tokens and injecting a Bearer header outside the normal auth flow.
  • Update sse_client() to preflight-refresh OAuth before starting the SSE connection and disable per-request auth when a Bearer header is pre-injected.
  • Add/extend tests for SSE OAuth preflight refresh and for the new OAuth preflight helper behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File Description
src/mcp/client/sse.py Preflights OAuth refresh and injects Authorization header for SSE stream requests.
src/mcp/client/auth/oauth2.py Introduces _prepare_request_with_refresh() helper to refresh and set Bearer header outside async_auth_flow.
tests/shared/test_sse.py Adds regression tests ensuring SSE uses preflight Bearer header and avoids auth flow on the stream.
tests/client/test_auth.py Adds unit tests for _prepare_request_with_refresh() across refresh/skip/init/no-refresh-token/failure cases.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/mcp/client/auth/oauth2.py Outdated
Comment thread src/mcp/client/sse.py
Comment thread tests/shared/test_sse.py Outdated
Comment thread tests/client/test_auth.py
@pragnyanramtha
pragnyanramtha force-pushed the codex/fix-sse-oauth-refresh branch from c24d7a1 to 3f4901e Compare May 20, 2026 20:22
@maxisbey

maxisbey commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

Thanks for the PR, and sorry it sat here without a proper review.

We're closing most of the open PR backlog. v2 is out and changed a lot of the SDK, so many older PRs no longer apply as written, and we're a small team that realistically doesn't have the capacity to work through the rest.

If this still matters to you on v2, the most useful thing you can do is open an issue (or comment on the existing one) with your use case and a repro. Hearing why it matters to you is what we use to decide what to prioritise.

AI Disclaimer

@maxisbey maxisbey closed this Oct 5, 2026
@maxisbey maxisbey added the missing-issue-link Auto-closed: PR needs a linked issue assigned to its author (see CONTRIBUTING.md) label Oct 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

missing-issue-link Auto-closed: PR needs a linked issue assigned to its author (see CONTRIBUTING.md)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants