modelcontextprotocol / modelcontextprotocol/python-sdk

Critical Token Refresh Bugs - Prevents Proactive Refresh

未關閉
#1,318 9 則留言 6 個 reaction 已指派 0 人 在 GitHub 檢視

還沒有人認領這個 Issue。

auth P1 ready for work
主要語言
Python
星號
24.3k
分支
4k
平均合併
1 天 1 小時
30 天內合併 PR
31

描述

Initial Checks
Description

tldr: Circular logic in mcp/client/auth.py creates two critical issues that prevent proactive token refresh; this would cause frequent re-auth for all users but it creates a third, worse issue for Clients that use TokenStorage (especially noticeable in a multi-user setup)

Background

In mcp/client/auth.py there are only 3 places where OAuthContext.token_expiry_time is set:

  1. During init it is set to None
  2. During full re-auth after receiving a 401 (in call to _handle_token_response)
  3. When a token is deemed invalid in is_token_valid (at start of async_auth_flow)

Issue 1 - Token validity check treats None as Valid

The ultimate problem is that when token_expiry_time is None, a token will pass validity checks, per the below.

def is_token_valid(self) -> bool:
    """Check if current token is valid."""
    return bool(
        self.current_tokens
        and self.current_tokens.access_token
        and (not self.token_expiry_time or time.time() <= self.token_expiry_time)
#                  ^^^^^^^^^^^^^^^^^^ None makes it perpetually valid so never needs pro-active refresh
    )

This is probably intended behavior to enable None to represent 'perpetual tokens'.

It sets up 2 fatal problems.

Issue 2 - Expired tokens get converted to 'perpetual tokens'

In update_token_expiry an expired token with a token.expires time of 0 is 'Falsey' which sets token_expiry_time to None.

def update_token_expiry(self, token: OAuthToken) -> None:
    """Update token expiry time."""
    if token.expires_in:     # <============================ 0 evaluates to False
        self.token_expiry_time = time.time() + token.expires_in
    else:
        self.token_expiry_time = None  # <============================ Makes it perpetually valid

Per Issue 1, when token_expiry_time is None the token is always valid (i.e. perpetual).

Undesirable Result - Only full re-auth can refresh a 'perpetual token' that goes stale

Because the token is now perpetual, it never gets proactively refreshed so it always requires a 401 and full re-auth to refresh.

Issue 3 - Clients with storage always start with token_expiry_time set to None

The OAuth init does not check token expiry:

async def _initialize(self) -> None:
    """Load stored tokens and client info."""
    self.context.current_tokens = await self.context.storage.get_tokens()
    self.context.client_info = await self.context.storage.get_client_info()
    self._initialized = True

This is often fine for Clients that persist.

For newly or frequently instantiated Clients that use Token Storage, token_expiry_time is None by default.

Due to Issue 1 that None value cause the token to become perpetual.

Due to Issue 2 the perpetual token is never checked for refresh.

As a result, tokens from storage (which are probably stale) are never checked prior to their first call. So they always trigger a 401 and revert to full re-auth.

Proposed Fixes

Token update should check if token has an expires_in attribute and for an explicit None. This preserves perpetual tokens while treating expired tokens appropriately.

  def update_token_expiry(self, token: OAuthToken) -> None:
      """Update token expiry time."""
      if hasattr(token, 'expires_in') and token.expires_in is not None: # <================ Fix
          self.token_expiry_time = time.time() + token.expires_in
      else:
          self.token_expiry_time = None

Also, the validity check should only work when self.token_expiry_time=None explicitly instead of when 'Falsey':

def is_token_valid(self) -> bool:
    """Check if current token is valid."""
    return bool(
        self.current_tokens
        and self.current_tokens.access_token
        and (self.token_expiry_time is None or time.time() <= self.token_expiry_time) # <============== Fix
               #^^^^^^^^^^^^^^^^^^^^^^^ None can remain signal for perpetual token
    )

Finally, OAuthContext._initialize should call update_token_expiry so stored tokens aren't treated as perpetual by default:

async _initialize(self):
    """Initialize and properly set token expiry from stored tokens."""
    self.context.current_tokens = await self.context.storage.get_tokens()
    self.context.client_info = await self.context.storage.get_client_info()
    
    # Fix: Update token expiry if tokens loaded from storage
    if self.context.current_tokens:
        self.context.update_token_expiry(self.context.current_tokens)
    self._initialized = True
Example Code

Python & MCP Python SDK
I'm on 1.12.4 but the code is still the same for 1.13.1

貢獻指南

開啟貢獻指南

從這裡開始

  1. 先讀完整個 Issue,再讀專案的貢獻指南。
  2. 在 Issue 下留言說明你要接手 —— 這能避免兩個人做同樣的事。
  3. Fork 儲存庫,在一個分支上完成修改。
  4. 送出 Pull Request,並在描述裡引用這個 Issue 編號。

研究方向

從 mcp/client/auth.py 開始,閱讀 OAuthContext.update_token_expiry、is_token_valid 和 OAuthContext._initialize,接著檢查現有的驗證與 token 儲存涵蓋範圍。完成的標準是:已儲存和已過期的 token 都能正確處理,同時不遺失 perpetual-token 行為,主動重新整理能正常運作,且回歸問題有涵蓋。

由索引模型根據 Issue 內容生成。

評估

技術堆疊
python
領域
authentication, backend
Issue 類型
缺陷
難度
4/5
預估耗時
3-5 天
活躍度
冷清
描述清晰度
基本清楚
新手友好度
48/100

把新 issue 寄到你的電子郵件信箱

精選適合新手參與的 GitHub issue 摘要。