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,然后检查现有的身份验证和令牌存储覆盖情况。完成的标准是:已存储和已过期的令牌都能得到正确处理,同时不丢失 perpetual-token 行为,主动刷新能够正常工作,并且回归问题得到覆盖。

由索引模型根据 Issue 内容生成。

评估

技术栈
python
领域
authentication, backend
Issue 类型
缺陷
难度
4/5
预计耗时
3-5 天
活跃度
冷清
描述清晰度
基本清楚
新手友好度
48/100

把新 issue 发到你的邮箱

精选适合新手参与的 GitHub issue 摘要。