feat(mcp): relay upstream 401 on client-forwarded pass-through tool calls - #32556
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR relays upstream HTTP 401s from client-forwarded (
Confidence Score: 5/5Safe to merge; changes are tightly scoped to the two client-forwarded auth modes and leave all other auth paths untouched. The relay logic is correct and consistent with the existing list-path behavior: only HTTP 401 is treated as a re-auth signal, 403 and transport errors remain as graceful isError results. Logging demotion is end-to-end (client layer, manager, REST endpoint, streamable handler) and verified by mutation-checked mock tests. The exception ordering in rest_endpoints.py ensures the upstream-auth catch is handled before the generic HTTPException catch, so the relayed 401 is never double-logged at error level. All new test code uses mocks only, with no real network calls. No files require special attention. The one test parametrization that covers an unreachable manager state is a clarity concern only and does not affect runtime behavior.
|
| Filename | Overview |
|---|---|
| litellm/proxy/_experimental/mcp_server/mcp_server_manager.py | Adds upstream 401 relay for true_passthrough and oauth_delegate modes in _call_regular_mcp_tool; only 401 becomes MCPUpstreamAuthError, 403/5xx stay as graceful isError with a warning. |
| litellm/proxy/_experimental/mcp_server/rest_endpoints.py | Adds a single except MCPUpstreamAuthError handler covering both the direct and virtual tool-call branches; locally generated HTTPExceptions keep error-level logging; virtual branch extracted to _handle_virtual_mcp_tool. |
| litellm/proxy/_experimental/mcp_server/server.py | Two targeted additions: MCPUpstreamAuthError is re-raised in call_mcp_tool without post_call_failure_hook, and converted to an informational isError in mcp_server_tool_call for the streamable-HTTP path. |
| litellm/experimental_mcp_client/client.py | Adds quiet_on_error to run_with_session and demotes logging to debug in call_tool/list_tools under raise_on_error, so an expected pass-through 401 re-auth does not emit error-level lines. |
| tests/test_litellm/proxy/_experimental/mcp_server/test_rest_endpoints.py | New tests cover relay, local-denial error-level retention, and virtual-branch relay; the test_call_tool_rest_relays_upstream_auth_failure parametrization with upstream_status=403 tests a state the manager cannot currently produce. |
| tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py | Adds mutation-checked tests covering 401 relay, tool-result passthrough, non-auth 403/503 isError degradation, and non-passthrough mode isolation; all use mocks only. |
| tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server.py | Adds tests verifying MCPUpstreamAuthError becomes an informational isError in mcp_server_tool_call and that call_mcp_tool skips post_call_failure_hook for expected re-auth. |
| tests/test_litellm/experimental_mcp_client/test_mcp_client.py | Adds three new tests validating debug-vs-error log demotion for raise_on_error in call_tool, list_tools, and run_with_session; existing tests reformatted only. |
Reviews (3): Last reviewed commit: "feat(mcp): relay upstream 401 on client-..." | Re-trigger Greptile
bd2bc06 to
a0c3bfd
Compare
ac53c31 to
1f173f0
Compare
a0c3bfd to
df0e855
Compare
22a9a3e to
e5daf8b
Compare
9a33c2d to
454fde6
Compare
e5daf8b to
b4db640
Compare
454fde6 to
2a5a9bd
Compare
b4db640 to
a07c158
Compare
2a5a9bd to
a3f1873
Compare
29c86ca to
60443db
Compare
46a4d6f to
bff2c95
Compare
29c86ca to
7542b1a
Compare
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 1 · PR risk: 0/10 |
a8d9e63 to
bc99ae6
Compare
02157d7 to
d4e02ac
Compare
bc99ae6 to
1e224a4
Compare
705f939 to
25eb550
Compare
|
@greptileai rereview |
|
bugbot run |
b7b1521 to
e506eb5
Compare
…alls The multi-server list path already relays an upstream 401 from a client-forwarded server (true_passthrough / oauth_delegate) as an MCPUpstreamAuthError so the caller re-runs its own upstream OAuth. The single-server REST call path did not: an upstream 401 was masked as a graceful isError result, so an MCP client holding an expired upstream token never learned it had to re-authenticate Relay the upstream 401 on the call path too. For these modes the manager calls the client with raise_on_error=True, extracts the WWW-Authenticate through the existing upstream-auth exception walk, and raises MCPUpstreamAuthError; the REST endpoint turns it into a real 401 + WWW-Authenticate. Only 401 is treated as a re-auth signal (a 403 is a genuine authorization failure that re-auth will not fix, so it stays a masked isError with a visible warning), matching the list path and MCPUpstreamAuthError's contract. The legacy oauth2 + delegate_auth_to_upstream mode is deliberately left off the call-path relay since it is being removed To keep this expected caller-must-reauth signal from tripping error-rate alerts, the client layer logs at debug when the caller opted into raise_on_error and therefore owns the exception (both call_tool/list_tools and the run_with_session helper they share, so an expected re-auth emits no warning per call either), the manager's non-auth branch logs the exception type only (never str(e), which for an httpx error embeds the upstream URL a credential can hide in), and the streamable and REST handlers log the relayed 401 at info rather than as an error with a traceback Tests cover the manager raising on a client-forwarded 401 while keeping a 403/503 as a masked isError, the client-layer debug-vs-error logging split, the streamable handler's informational isError, and the REST endpoint relaying both the direct and virtual mcp_tool_call branches as a real 401 + WWW-Authenticate; each was mutation-checked to fail when the corresponding behavior is broken
e506eb5 to
e33654b
Compare
|
Pushed a refactor and re-verified live, so the tree is different from the last review. Summary of what changed since then The REST relay no longer uses a marker HTTPException subclass or a per-call wrapper. A single The call path now treats only 401 as a re-auth signal. A 403 is a genuine authorization failure that re-auth will not fix, so it takes the same non-auth branch as a transport error and stays a masked Client-layer logging is synchronized so an expected re-auth is quiet end to end: Every new and strengthened test was mutation-checked to fail when the behavior it guards is broken. The Proof of Fix in the description is a fresh live run against a stub upstream on this branch showing the 401 relayed with its |
|
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 e33654b. Configure here.
Relevant issues
Linear ticket
Pre-Submission checklist
Screenshots / Proof of Fix
Verified live on a local proxy running this branch, against a stub upstream MCP server on :9200 that returns 200 for a valid bearer, 401 (with an RFC 9728
resource_metadatachallenge) forEXPIRED, 403 forFORBIDDEN, and 503 forTRIGGER503, and logs every Authorization it receives so we can prove which token the proxy forwarded. Atrue_passthroughserverrelay_ptpoints at it; the caller's upstream token rides the per-serverx-mcp-relay_pt-authorizationheader, never confused with the gateway admission key. Every case is a tool CALL throughPOST /mcp-rest/tools/callThe success call forwards the caller's token byte-for-byte; an upstream 401 becomes a real 401 with the upstream
WWW-Authenticatepreserved (so a standards-compliant MCP client re-runs the upstream OAuth flow, instead of the pre-change maskedHTTP 200 {"isError":true,"content":[{"text":"HTTPStatusError: Client error '401 Unauthorized' ..."}]}). Only 401 is a re-auth signal; a 403 is a genuine authorization failure that re-authorizing will not fix, so like a 503 it keeps the gracefulisErrordegradation rather than being mistaken for a challengeLogging, checked in the same run: the expected re-auth 401 must not trip operator error-rate alerts, while a genuine failure stays visible
The relayed 401 produces zero error-level and zero warning-level lines (it logs at info at the endpoint, and at debug at the client layer since the caller owns the exception under
raise_on_error; the sharedrun_with_sessionhelper is quieted the same way, so no stray warning per call). Only the genuine non-auth 403 and 503 emit a WARNING, one each, and that line carries the exception type only, neverstr(e), which for an httpx error would embed the upstream URL a credential can hide inType
🐛 Bug Fix
Changes
A
true_passthrough/oauth_delegatetool call carries the caller's own upstream token, so an upstream 401 on the call is the caller's to resolve. The single-server REST call path masked it as a genericisErrortool result, so a standards-compliant MCP client never saw the challenge and could not re-run the upstream OAuth flow; the multi-server list path already relayed it_call_regular_mcp_toolnow, for the client-forwarded modes only, opts intoraise_on_errorand turns an upstream 401 intoMCPUpstreamAuthErrorwith the upstreamWWW-Authenticatepreserved. Only 401 is treated as a re-auth signal; a 403 is a genuine authorization failure that re-auth will not fix, so it takes the same non-auth branch as any transport error and keeps the defaultisErrordegradation viaerror_tool_resultbehind a visible warning. Every other auth type (api_key, M2M, OBO with its own 401 retry) is left untouched, and the legacy oauth2 + delegate mode being removed is deliberately not added to the call-path relay, so the change cannot regress themThe REST call endpoint relays that into a real 401 with the challenge preserved, mirroring the existing
tools/listbehavior. A singleexcept MCPUpstreamAuthErroron the endpoint covers both the directexecute_mcp_toolcall and the virtualmcp_tool_callbranch, so neither can fall through the catch-all into a generic 500; locally generated permission denials (tool, server, IP filtering) raise a plainHTTPExceptionand keep their error-level logging, since only the typed upstream-auth relay is demoted to info. The streamable-HTTP protocol path cannot emit a raw 401 mid-session, because the MCP session manager serializes handler exceptions as JSON-RPC errors (the same reason the connect-time 401 is done preemptively), so it returns an explicitisErrornaming the upstream status instead of a masked 500 or a logged traceback, andcall_mcp_toolre-raisesMCPUpstreamAuthErrorbefore its generic handler so the expected re-auth does not firepost_call_failure_hookand its LLM-exception alertAn expected pass-through re-auth is not an operator-actionable error, so it is kept off the error and warning levels end to end: info at the endpoint and the streamable handler, and debug at the client layer, where
MCPClient.call_tool/list_toolsand therun_with_sessionhelper they share all demote their failure line when the caller opted intoraise_on_errorand therefore owns the exception. The relay's non-auth branch still logs a warning before it degrades a genuine upstream outage toisError, so a real failure stays visible while normal re-authentication no longer trips error-rate alertsNote
Medium Risk
Touches MCP proxy auth relay and tool-call error paths for pass-through modes only, but changes HTTP status and logging behavior that clients and alerts depend on.
Overview
Client-forwarded pass-through tool calls (
true_passthrough/oauth_delegate) now surface an upstream HTTP 401 asMCPUpstreamAuthErrorwith preservedWWW-Authenticate, instead of masking it as a genericisErrortool result. Only 401 triggers re-auth relay; 403 and other failures still degrade toisErrorwith a warning.On REST
POST /mcp-rest/tools/call, that exception becomes a real 401 + challenge (direct and virtualmcp_tool_callpaths). On the streamable MCP session path, handlers return an explicitisErrornaming the upstream status, andcall_mcp_toolskipspost_call_failure_hookfor this case.Logging: expected re-auth is info/debug (not error/warning) via
quiet_on_erroronrun_with_sessionand demoted logs incall_tool/list_toolswhenraise_on_error=True; genuine non-auth failures stay visible.Reviewed by Cursor Bugbot for commit e33654b. Bugbot is set up for automated code reviews on this repo. Configure here.