From 9dbebf27a68a4eaa5f202a7160011306e6347f0c Mon Sep 17 00:00:00 2001 From: Tin Chi Lo Date: Mon, 13 Jul 2026 16:22:16 -0700 Subject: [PATCH] fix(mcp): detect upstream invalid_grant by the RFC 6749 error field, not a body substring The bridge refresh path decided whether an upstream token-endpoint rejection was invalid_grant by substring-matching the raw response body, so a rejection whose actual error is something else but whose error_description merely contains the string invalid_grant would false-match, map to invalid_grant, and trigger a needless authorization_code re-run Parse the RFC 6749 section 5.2 error object and compare the error field. A non-JSON body, or an error that is not invalid_grant, now propagates as the upstream error rather than being reinterpreted. The regression test drives an invalid_client rejection whose description contains the string invalid_grant and asserts it is not mapped, mutation-checked against the substring match --- .../mcp_server/discoverable_endpoints.py | 17 ++++++- .../mcp_server/test_discoverable_endpoints.py | 48 +++++++++++++++++++ 2 files changed, 64 insertions(+), 1 deletion(-) diff --git a/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py b/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py index 441982f84f9..80ffad487c5 100644 --- a/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py +++ b/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py @@ -1338,6 +1338,21 @@ def _mint_refresh_envelope_value( return None +def _upstream_oauth_error(response: httpx.Response) -> str | None: + """The RFC 6749 5.2 ``error`` code from an upstream token-endpoint error body, or ``None`` when the + body is not a JSON object carrying a string ``error``. Reading the field beats substring-matching the + raw text, which would false-match a code that only appears inside ``error_description`` (a false + invalid_grant would trigger a needless authorization_code re-run).""" + try: + body = json.loads(response.text) + except (ValueError, TypeError): + return None + if not isinstance(body, dict): + return None + error = body.get("error") + return error if isinstance(error, str) else None + + async def exchange_token_with_server( request: Request, mcp_server: MCPServer, @@ -1479,7 +1494,7 @@ async def exchange_token_with_server( is_bridge and grant_type == "refresh_token" and exc.response.status_code == 400 - and "invalid_grant" in exc.response.text + and _upstream_oauth_error(exc.response) == "invalid_grant" ) if upstream_rejected_bridge_refresh: verbose_logger.info( diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/test_discoverable_endpoints.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_discoverable_endpoints.py index 3fd87820d3f..437fbc435a3 100644 --- a/tests/test_litellm/proxy/_experimental/mcp_server/test_discoverable_endpoints.py +++ b/tests/test_litellm/proxy/_experimental/mcp_server/test_discoverable_endpoints.py @@ -5176,6 +5176,54 @@ async def test_bridge_refresh_upstream_invalid_grant_maps_to_invalid_grant(): assert json.loads(response.body)["error"] == "invalid_grant" +@pytest.mark.asyncio +async def test_bridge_refresh_upstream_error_detection_parses_json_not_substring(): + """The upstream invalid_grant detection parses the RFC 6749 5.2 error field, not a substring of the + body. An upstream error whose code is not invalid_grant (here invalid_client, with the string + invalid_grant only inside error_description) must NOT be mistaken for a dead refresh token, so it + propagates as the upstream error rather than triggering a spurious authorization_code re-run.""" + import httpx + + from litellm.proxy._experimental.mcp_server.discoverable_endpoints import exchange_token_with_server + from litellm.types.mcp import MCPAuth + + server = _bridge_server(auth_type=MCPAuth.oauth_delegate) + refresh_env = _mint_test_refresh_envelope(server_id=server.server_id, upstream_refresh="UP") + + error_response = MagicMock() + error_response.status_code = 400 + error_response.text = '{"error": "invalid_client", "error_description": "this is not an invalid_grant problem"}' + error_response.raise_for_status = MagicMock( + side_effect=httpx.HTTPStatusError("bad", request=MagicMock(), response=error_response) + ) + fake_http_client = MagicMock() + fake_http_client.post = AsyncMock(return_value=error_response) + + with ( + patch( + "litellm.proxy._experimental.mcp_server.discoverable_endpoints.get_async_httpx_client", + return_value=fake_http_client, + ), + patch( + "litellm.proxy._experimental.mcp_server.discoverable_endpoints._revalidate_active_subject", + new=AsyncMock(return_value=None), + ), + patch("litellm.proxy.proxy_server.master_key", _BRIDGE_MASTER_KEY), + ): + with pytest.raises(httpx.HTTPStatusError): + await exchange_token_with_server( + request=_bridge_mock_request(), + mcp_server=server, + grant_type="refresh_token", + code=None, + redirect_uri=None, + client_id="dcr-client-123", + client_secret=None, + code_verifier=None, + refresh_token=refresh_env, + ) + + @pytest.mark.asyncio async def test_revalidate_key_subject_revoked_when_owner_scim_deactivated(proxy_globals): """A key_hash refresh envelope whose key is still active but whose OWNING user was SCIM-deactivated must