fix(mcp): surface tools/list auth failures as a 401 challenge on single-server routes - #31921
Conversation
Greptile SummaryThis PR fixes a bug where upstream 401 auth failures on MCP tools/list were silently converted to a 200 with an empty tool list on single-server routes. The fix removes the
Confidence Score: 5/5Safe to merge. The behavioral change is intentional, well-scoped (single-server routes only), and regression-tested across both Python and TypeScript layers. The routing contract — single-server routes raise, multi-server aggregator absorbs — is consistently enforced at every callsite and covered by the new test suite. The two test files that flip swallow behavior to raise accurately reflect the intentional removal of the auth_type carve-out rather than masking a regression. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/_experimental/mcp_server/mcp_server_manager.py | Removes the auth_type carve-out from _fetch_tools_with_timeout (always raise_on_error=True; drops server param) and adds an HTTPException(401+WWW-Authenticate) → MCPUpstreamAuthError conversion at the _get_tools_from_server boundary. Logic is consistent with the routing contract: single-server routes re-raise, multi-server aggregator absorbs. |
| tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py | Adds TestMCPToolsListAuthSurfacing with six focused tests covering 401 raise, 403 absorb, non-auth error absorb, HTTPException(401+challenge) conversion, non-challenge HTTPException absorb, and aggregate list absorption of a failing server. |
| tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_oauth_passthrough_tools.py | Removes server= keyword argument from _fetch_tools_with_timeout calls (matches dropped parameter), and flips the two swallow-to-empty-list tests for client_credentials and gateway-managed servers to verify they now raise MCPUpstreamAuthError, correctly reflecting the intentional behavior change. |
| tests/test_litellm/proxy/_experimental/mcp_server/test_rest_endpoints.py | Adds test_aggregate_list_absorbs_one_server_auth_failure; class already carries pytestmark = pytest.mark.asyncio so the new async test runs correctly. Patches _get_tools_for_single_server, which is the actual module-level function called in the aggregate loop. |
| ui/litellm-dashboard/src/components/mcp_tools/mcp_tools.tsx | Adds oboTokenRejected derived from mcpToolsError.status === 401, wired into authGateActive and the OBO gate JSX. Works correctly with the existing queryFn that converts result.error + status:401 into a thrown Error with .status=401. |
| ui/litellm-dashboard/src/components/mcp_tools/mcp_tools.test.tsx | Adds a test for the expired-no-refresh OBO scenario: credential exists, list call returns {error, status:401}, gate is shown. Test correctly uses mockResolvedValue so the queryFn result.error path triggers the thrown error. |
Reviews (4): Last reviewed commit: "fix(mcp): surface tools/list 401 auth fa..." | Re-trigger Greptile
This comment was marked as outdated.
This comment was marked as outdated.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
7745f9e to
1a77917
Compare
|
Addressed the 403 consistency point. The only 403 raised during client creation is the stdio-command-allowlist rejection, which is not an auth challenge and should keep degrading to an empty list; blanket-converting 403 would surface it as a spurious challenge. The handler now gates on the presence of a |
|
The However, there's an issue with the existing
The three tests that assert the old |
|
Updated: scoped the challenge to 401 only (a 403, including insufficient_scope, degrades to an empty list rather than prompting re-auth, since re-authenticating does not resolve a forbidden result); fixed the test_mcp_oauth_passthrough_tools.py tests for the removed |
1a77917 to
6bf5877
Compare
…le-server routes A 401 while listing tools (a missing or expired per-user OAuth token, or an upstream 401 for any auth_type) was swallowed to an empty tool list, so a single-server client got a 200 with no tools and no WWW-Authenticate challenge instead of a 401 it could re-authenticate against. Only oauth pass-through and delegate-to-upstream oauth2 servers surfaced it; every other auth_type, and the missing-token case for all of them, masked it. The surface-vs-absorb decision now keys on the route, not the auth_type. An upstream 401 in _fetch_tools_with_timeout becomes an MCPUpstreamAuthError regardless of auth_type, and the per-user OAuth challenge raised during client creation (a bare HTTPException 401 carrying a WWW-Authenticate header) is converted to the same type in _get_tools_from_server. The challenge is scoped to 401: a 403 (authenticated but forbidden, e.g. insufficient scope) is not a re-auth signal and degrades to an empty list like any other non-auth error, and the stdio-allowlist 403 (no challenge header) stays absorbed. The existing routing then does the right thing: single-server routes turn the error into a 401 + WWW-Authenticate, while the multi-server aggregator absorbs it to an empty list so one unauthenticated server does not fail the whole listing. On the UI tools page, an OBO (per-user authorization_code) server now shows the Authorize gate when the list call returns 401, not only when no credential row exists. The backend already refreshes a still-refreshable token on the list call, so a 401 means there is no valid token and none could be minted (expired with no usable refresh token), which is exactly when the user must reauthorize.
6bf5877 to
5810cf1
Compare
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 5810cf1. Configure here.
…le-server routes (BerriAI#31921) A 401 while listing tools (a missing or expired per-user OAuth token, or an upstream 401 for any auth_type) was swallowed to an empty tool list, so a single-server client got a 200 with no tools and no WWW-Authenticate challenge instead of a 401 it could re-authenticate against. Only oauth pass-through and delegate-to-upstream oauth2 servers surfaced it; every other auth_type, and the missing-token case for all of them, masked it. The surface-vs-absorb decision now keys on the route, not the auth_type. An upstream 401 in _fetch_tools_with_timeout becomes an MCPUpstreamAuthError regardless of auth_type, and the per-user OAuth challenge raised during client creation (a bare HTTPException 401 carrying a WWW-Authenticate header) is converted to the same type in _get_tools_from_server. The challenge is scoped to 401: a 403 (authenticated but forbidden, e.g. insufficient scope) is not a re-auth signal and degrades to an empty list like any other non-auth error, and the stdio-allowlist 403 (no challenge header) stays absorbed. The existing routing then does the right thing: single-server routes turn the error into a 401 + WWW-Authenticate, while the multi-server aggregator absorbs it to an empty list so one unauthenticated server does not fail the whole listing. On the UI tools page, an OBO (per-user authorization_code) server now shows the Authorize gate when the list call returns 401, not only when no credential row exists. The backend already refreshes a still-refreshable token on the list call, so a 401 means there is no valid token and none could be minted (expired with no usable refresh token), which is exactly when the user must reauthorize. (cherry picked from commit b9df7fa)
Relevant issues
Linear ticket
N/A
Pre-Submission checklist
@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewScreenshots / Proof of Fix
Local proxy on
:4010with two config MCP servers, bothauth_type: none:linear_unauthpoints at an OAuth-protected remote MCP server that returns 401 for an unauthenticated request, anddeepwiki_healthypoints at a public no-auth MCP server that lists tools. Launched on the unfixed commit, curled, then on this branch and curled again.Before (unfixed): the single-server route hides the auth failure as a 200 with an empty list
After (this branch): the single-server route returns 401 with the upstream WWW-Authenticate challenge
The multi-server aggregate listing degrades the unauthenticated server to an empty contribution and still returns the healthy server's tools, identically before and after (200, three tools, only
deepwiki_healthyrepresented)Type
🐛 Bug Fix
Changes
A 401 while listing MCP tools was swallowed to an empty tool list, so a single-server client saw a 200 with no tools and no
WWW-Authenticatechallenge instead of a 401 it could re-authenticate against._fetch_tools_with_timeoutonly converted an upstream 401 intoMCPUpstreamAuthErrorwhen the server was oauth pass-through or a delegate-to-upstream oauth2 server; every otherauth_typereturned[]. Separately, the missing or expired per-user OAuth token surfaces during client creation as a bareHTTPException(401)carrying aWWW-Authenticateheader, which_get_tools_from_servercaught in itsexcept Exception: return [], so that case masked for every modeThis makes the surface-vs-absorb decision key on the route rather than the
auth_type. An upstream 401 now becomesMCPUpstreamAuthErrorregardless ofauth_type, and the per-user OAuth challenge is converted to the same type at the_get_tools_from_serverboundary while non-auth errors still degrade to[]. The challenge is scoped to 401: a 403 (authenticated but forbidden, for example insufficient scope) is not a re-auth signal, so it degrades to an empty list like any other non-auth error, and the stdio-allowlist 403 (which carries no challenge header) stays absorbed. The existing routing then behaves as intended without further change: the single-server REST and MCP-protocol routes turnMCPUpstreamAuthErrorinto a 401 with theWWW-Authenticatechallenge, while the multi-server aggregator keeps absorbing it to an empty list so one unauthenticated server does not fail the whole listingThe dashboard tools page consumes the new 401 for OBO (per-user authorization_code) servers: the Authorize gate now shows when the list call returns 401, not only when no credential row exists. The backend already refreshes a still-refreshable token on the list call, so a 401 there means there is no valid token and none could be minted (expired with no usable refresh token), which is exactly when the user must reauthorize
Regression coverage was added to the mapped tests.
test_mcp_server_manager.pycovers_fetch_tools_with_timeoutraising on an upstream 401 and absorbing a 403, returning[]on a non-auth error,_get_tools_from_serverconverting the per-user token 401 challenge and absorbing a non-challenge 403, and the aggregatelist_toolsabsorbing a failing server while still returning the healthy one.test_rest_endpoints.pycovers the aggregate REST route degrading one server's auth failure.test_mcp_oauth_passthrough_tools.pywas updated for the removedserver=parameter and the always-raise-on-401 contract.mcp_tools.test.tsxcovers the OBO tools page showing the Authorize gate on a 401 while a refreshable expired token still lists silentlyNote
Medium Risk
Changes MCP proxy authentication error handling for all server auth types on tools/list; behavior shifts from silent empty lists to 401 challenges on single-server routes, with aggregate routes unchanged.
Overview
MCP tools/list no longer turns upstream auth failures into a 200 with an empty tool list on single-server paths. Upstream 401 is always raised as
MCPUpstreamAuthError(dropping the oldauth_typecarve-out), while 403 and other errors still degrade to[]._fetch_tools_with_timeoutalways callslist_tools(raise_on_error=True)and no longer takes aserverargument._get_tools_from_servermaps client-creationHTTPException(401)with aWWW-Authenticateheader into the same error type. Single-server REST/MCP routes can return 401 + challenge; multi-server aggregate listings keep absorbing one bad server.The dashboard OBO tools view shows the Authorize gate when the list API returns 401, not only when no credential row exists (e.g. expired token with no refresh).
Reviewed by Cursor Bugbot for commit 5810cf1. Bugbot is set up for automated code reviews on this repo. Configure here.