modelcontextprotocol / modelcontextprotocol/python-sdk

OAuth client sends client_id in token body under client_secret_basic (strict servers reject as multiple auth methods)

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

還沒有人認領這個 Issue。

auth bug needs confirmation P1 v1 v2
主要語言
Python
星號
24.3k
分支
4k
平均合併
1 天 1 小時
30 天內合併 PR
31

描述

Initial Checks
  • I confirm that I'm using the latest version of MCP Python SDK
  • I confirm that I searched for my issue before opening this issue
Description

Under token_endpoint_auth_method = client_secret_basic, the OAuth client sends the client credentials in the HTTP Authorization: Basic header and also leaves client_id in the token-exchange POST body. Strict token endpoints reject this as two authentication methods in a single request.

OAuthClientProvider.prepare_token_auth (src/mcp/client/auth/oauth2.py) strips only client_secret from the body:

headers["Authorization"] = f"Basic {encoded_credentials}"
# Don't include client_secret in body for basic auth
data = {k: v for k, v in data.items() if k != "client_secret"}   # client_id remains in the body

Expected: with Basic auth, the request presents exactly one authentication method — the Authorization header — with no client credentials or identifier duplicated in the body.

Actual: client_id stays in the body.

Against Notion's MCP server (https://mcp.notion.com/mcp), where dynamic client registration returns token_endpoint_auth_method = client_secret_basic, the token exchange fails with:

Token exchange failed (400): "Client must not use multiple authentication methods"

There is also a secondary, misleading cascade (not the root cause): the failed first attempt starts a second OAuth flow that reuses the loopback callback port, and stale browser tabs redirect old approvals into it, surfacing OAuthFlowError: State parameter mismatch.

I want to frame this as an interop issue rather than a compliance accusation. RFC 6749 §2.3.1 presents the HTTP Basic header and body credentials as alternatives; sending only the header is unambiguously valid, removes the ambiguity, and loses no information (the client_id is already present, base64-encoded, in the Basic header). Stripping client_id from the body makes the SDK work against strict servers like Notion at no cost to lenient ones.

The one-line fix in the client_secret_basic branch:

data = {k: v for k, v in data.items() if k not in ("client_secret", "client_id")}

Note: the existing test test_basic_auth_token_exchange currently asserts client_id ... in content # client_id still in body — this behaviour was codified in the same commit that introduced Basic auth support (#1334), so the fix inverts that assertion.

Update: this is the CLIENT half — it depends on the server side (#1847)

The one-line client change cannot land alone. Body client_id is load-bearing on this SDK's own server side in three places, so a client that omits it fails against any server built on this SDK:

  1. ClientAuthenticator.authenticate_request (src/mcp/server/auth/middleware/client_auth.py) requires client_id in the body.
  2. TokenHandler's request models (AuthorizationCodeRequest / RefreshTokenRequest, src/mcp/server/auth/handlers/token.py) declare client_id: str as required — a missing value returns a Pydantic 400 invalid_request: client_id: Field required.
  3. The grant-ownership checks compare auth_code.client_id != token_request.client_id (and the refresh equivalent).

The matching server-side change is exactly what #1847 already implements: it makes body client_id optional, re-sources the ownership checks from the authenticated client_info.client_id, and reads client_id from the Basic header. So the two are complementary halves of one fix:

  • Client-only (this issue's change) → the SDK's own server rejects with Missing client_id. Verified.
  • Client change + #1847's server change → the previously-failing Basic-auth token exchange passes end-to-end. Verified locally by grafting #1847's server diff onto main and running the client + interaction OAuth suites (green). The only residual failures were #1847's own error-message test updates, which appear stranded by a tests/server/fastmcptests/server/mcpserver directory rename on main (i.e. #1847 looks like it needs a rebase) — unrelated to the client change.

Questions for maintainers:

  1. How would you like the client change sequenced against #1847 — folded into #1847, or as a linked follow-up PR that merges after it?
  2. Which branch(es) — main (v2), and/or a v1.x backport? The live breakage is on v1 (1.27.x) via Notion.

I'm happy to open the client-side PR (one-line change + updated tests) once there's buy-in and a preferred sequencing.

Example Code
# Any client using this SDK against Notion MCP, where dynamic registration
# returns token_endpoint_auth_method = client_secret_basic. For example via mcp2cli:
#   mcp2cli --mcp https://mcp.notion.com/mcp --oauth --list
#
# 1. Complete the browser authorization.
# 2. The token exchange returns:
#    Token exchange failed (400): "Client must not use multiple authentication methods"
Python & MCP Python SDK
Python 3.10
mcp: reproduced on 1.27.2 and 1.28.1; bug also present on main @ 3a6f299
Server: Notion MCP (https://mcp.notion.com/mcp), token_endpoint_auth_method = client_secret_basic

This report was prepared with AI assistance. I've reviewed the analysis and reproduced the failure myself, and can speak to the change directly.

貢獻指南

開啟貢獻指南

從這裡開始

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

研究方向

從 src/mcp/client/auth/oauth2.py 中的 OAuthClientProvider.prepare_token_auth 和 test_basic_auth_token_exchange 開始。閱讀 src/mcp/server/auth/middleware/client_auth.py 中的 ClientAuthenticator.authenticate_request、src/mcp/server/auth/handlers/token.py 中的請求模型,以及 issue #1847;用戶端和相應的伺服器端變更通過 OAuth 互動測試後即視為完成。

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

評估

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

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

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