fix(mcp): restore PKCE-triggering 401 when no stored per-user token exists - #26032
Conversation
…xists Per-user OAuth MCP requests now only skip pre-emptive 401 when a stored token is available, preserving token-reuse behavior while restoring fast PKCE kickoff for first-time or missing-token users.
|
|
Congrats! CodSpeed is installed 🎉
You will start to see performance impacts in the reports once the benchmarks are run from your default branch.
|
Greptile SummaryThis PR fixes a bug in the MCP per-user OAuth flow where first-time users (with no stored token) were never sent the pre-emptive HTTP 401 + Confidence Score: 5/5Safe to merge — the fix is a targeted, logically sound change with appropriate test coverage and no security regressions. The logic change is minimal and correct: No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/_experimental/mcp_server/server.py | Adds per-user OAuth token check before pre-emptive 401: only skips PKCE 401 when a stored token exists, otherwise falls through to trigger OAuth flow for new users. |
| tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_stale_session.py | Adds two new integration-style tests covering the fix: missing stored token raises 401, existing stored token skips pre-emptive 401 and forwards to session manager. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[handle_streamable_http_mcp] --> B{mcp_servers loop}
B --> C{server.auth_type == oauth2\nAND no oauth2_headers?}
C -- No --> G[Continue to request processing]
C -- Yes --> D{server.needs_user_oauth_token?}
D -- No --> F[Raise 401 + WWW-Authenticate\nPKCE kickoff]
D -- Yes --> E[_get_user_oauth_extra_headers_from_db\nRedis → DB]
E --> H{stored token found?}
H -- Yes --> I[continue loop\nskip 401]
H -- No --> F
I --> G
G --> J[_handle_stale_mcp_session]
J --> K[session_manager.handle_request]
Reviews (1): Last reviewed commit: "fix(mcp): restore PKCE-triggering 401 wh..." | Re-trigger Greptile
| if server.needs_user_oauth_token: | ||
| continue | ||
| stored_oauth_headers = ( | ||
| await _get_user_oauth_extra_headers_from_db( | ||
| server=server, | ||
| user_api_key_auth=user_api_key_auth, | ||
| ) | ||
| ) | ||
| if stored_oauth_headers: | ||
| continue |
There was a problem hiding this comment.
Double DB/cache fetch for stored-token path
When a stored token exists (stored_oauth_headers is truthy and continue is executed), _get_user_oauth_extra_headers_from_db will be invoked a second time during actual request processing at the per-server extra_headers lookup (line ~1315). In the non-test code path this means two round-trips per request, mitigated only by the Redis cache layer. Consider passing the already-fetched stored_oauth_headers value forward (e.g. via a dict keyed by server name) rather than re-fetching it in the downstream path.
This doesn't affect correctness since the Redis cache will serve the second call, but it does add latency when the cache is cold (first request or after eviction).
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
ecff06d
into
litellm_internal_staging
fix(mcp): restore PKCE-triggering 401 when no stored per-user token exists
Relevant issues
Pre-Submission checklist
tests/test_litellm/directory, Adding at least 1 test is a hard requirement - see detailsmake test-unitType
🐛 Bug Fix
Changes
Per-user OAuth MCP requests now only skip the pre-emptive 401 when a stored token is available. This preserves token-reuse behavior while restoring fast PKCE kickoff for first-time or missing-token users.
Before this fix, the 401 that triggers the PKCE flow was never sent when no stored token existed, leaving new users stuck.