From 1037f471c061548eac850367df38c24019b8dcad Mon Sep 17 00:00:00 2001 From: user <70670632+stuxf@users.noreply.github.com> Date: Mon, 11 May 2026 04:19:49 +0000 Subject: [PATCH] fix(mcp): keep public servers reachable when client IP is unknown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fail-closed gate previously denied access for any server when client_ip was None — including legitimately-public servers. A real HTTP request whose IP couldn't be extracted (ASGI middleware nulling request.client, broken X-Forwarded-* parsing, etc.) would 404 against a public OAuth endpoint with no actionable diagnostic for operators. Reorder _is_server_accessible_from_ip to check the public-server short-circuits (available_on_public_internet and litellm.public_mcp_servers) before failing closed on None. Non-public servers still fail closed when client_ip is None; the mcp_allow_unknown_client_ip opt-out still works as a wider escape hatch for operators who can't fix IP extraction. Also remove the None short-circuits in filter_server_ids_by_ip_with_info and get_filtered_registry so they delegate to the per-server gate, which now handles None correctly. Update test_gate_contract to expect public+None → True and add test_public_server_reachable_when_ip_unknown and test_no_ip_lets_public_through_blocks_private for the new semantics. --- .../mcp_server/mcp_server_manager.py | 48 ++++++++++++------- .../mcp_server/test_mcp_server_manager.py | 10 ++-- .../proxy/auth/test_mcp_ip_filtering.py | 30 ++++++++++++ 3 files changed, 67 insertions(+), 21 deletions(-) diff --git a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py index c4f0d10ca2a..8dff40582f9 100644 --- a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py +++ b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py @@ -1107,10 +1107,9 @@ class MCPServerManager: client_ip = self._resolve_unknown_client_ip(client_ip) if client_ip is INTERNAL_REQUEST: return server_ids, 0 - if client_ip is None: - # Fail closed: don't expose internal-only servers when the IP - # couldn't be attributed. - return [], len(server_ids) + # Don't short-circuit on client_ip is None — public servers should + # still be reachable, only internal-only servers fail closed. The + # per-server gate enforces both halves. allowed = [] blocked = 0 for sid in server_ids: @@ -3119,29 +3118,42 @@ class MCPServerManager: - ``INTERNAL_REQUEST`` bypasses IP filtering (internal callers, admin debug, registry maintenance). - - ``None`` fails closed: external request handlers must extract a real - IP via ``IPAddressUtils.get_mcp_client_ip(request)`` and reject the - request when extraction fails. Earlier behaviour treated ``None`` as - "no filter," which let external callers reach internal-only servers - when IP extraction silently failed. Operators behind ASGI middleware - or load-balancer setups where ``request.client`` is legitimately - ``None`` can opt back to the previous fail-open behaviour by setting + - If the server has ``available_on_public_internet=True`` (or is in + ``litellm.public_mcp_servers``), it's always accessible — including + when ``client_ip`` is ``None``. Otherwise a request to a + legitimately-public OAuth endpoint behind ASGI middleware that + nulls ``request.client`` would 404 with no actionable diagnostic. + - For non-public servers, ``None`` fails closed: external request + handlers must extract a real IP via + ``IPAddressUtils.get_mcp_client_ip(request)``. Earlier behaviour + treated ``None`` as "no filter," which let external callers reach + internal-only servers when IP extraction silently failed. Operators + behind ASGI middleware or load-balancer setups where + ``request.client`` is legitimately ``None`` can opt back to the + previous fail-open behaviour by setting ``general_settings.mcp_allow_unknown_client_ip: true``. - - If the server has ``available_on_public_internet=True``, it's - always accessible. - - Otherwise, only internal/private IPs can access it. + - For non-public servers with a real IP, only internal/private IPs + can access it. """ client_ip = self._resolve_unknown_client_ip(client_ip) if client_ip is INTERNAL_REQUEST: return True - if client_ip is None: - return False + # Public servers are reachable regardless of caller IP — and crucially + # even when IP extraction failed (client_ip is None). Otherwise a + # request to a legitimately-public OAuth endpoint behind an ASGI + # middleware that nulls request.client would 404 with no actionable + # diagnostic for operators. if server.available_on_public_internet: return True # Check backwards compat: litellm.public_mcp_servers public_ids = set(litellm.public_mcp_servers or []) if server.server_id in public_ids: return True + if client_ip is None: + # Non-public server with unknown caller IP fails closed. Earlier + # behaviour treated None as "no filter," letting external callers + # reach internal-only servers when IP extraction silently failed. + return False # Non-public server: only accessible from internal IPs. The two # early returns above narrow client_ip to ``str`` for the type # checker; cast keeps mypy happy without a runtime assert. @@ -3301,8 +3313,8 @@ class MCPServerManager: client_ip = self._resolve_unknown_client_ip(client_ip) if client_ip is INTERNAL_REQUEST: return registry - if client_ip is None: - return {} + # Don't short-circuit on client_ip is None — public servers should + # still be reachable, only internal-only servers fail closed. return { k: v for k, v in registry.items() diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py index a407071110c..08afb975d29 100644 --- a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py +++ b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py @@ -3346,10 +3346,14 @@ class TestIPGatingFailClosed: @pytest.mark.parametrize( "server_kind,client_ip_kind,expected", [ - # None fails closed regardless of server visibility — missing IP - # signals an external request that couldn't be attributed. + # None fails closed for non-public servers — missing IP signals + # an external request that couldn't be attributed and must not + # reach internal-only servers. ("internal", "none", False), - ("public", "none", False), + # None still lets public servers through — otherwise a request + # to a public OAuth endpoint behind ASGI middleware that nulls + # request.client would 404 with no actionable diagnostic. + ("public", "none", True), # INTERNAL_REQUEST bypasses gating for both visibilities. ("internal", "internal_request", True), ("public", "internal_request", True), diff --git a/tests/test_litellm/proxy/auth/test_mcp_ip_filtering.py b/tests/test_litellm/proxy/auth/test_mcp_ip_filtering.py index 604af05dd01..078e68ecc8d 100644 --- a/tests/test_litellm/proxy/auth/test_mcp_ip_filtering.py +++ b/tests/test_litellm/proxy/auth/test_mcp_ip_filtering.py @@ -114,6 +114,21 @@ class TestMCPServerIPFiltering: assert manager.filter_server_ids_by_ip(["priv"], client_ip=None) == ["priv"] + @patch("litellm.public_mcp_servers", []) + @patch("litellm.proxy.proxy_server.general_settings", {}) + def test_public_server_reachable_when_ip_unknown(self): + # A legitimately-public server must stay reachable even when IP + # extraction failed (client_ip=None). Otherwise a request to a + # public OAuth endpoint behind ASGI middleware that nulls + # request.client would 404 with no actionable diagnostic. + pub = _make_server("pub", available_on_public_internet=True) + priv = _make_server("priv", available_on_public_internet=False) + manager = _make_manager([pub, priv]) + + assert manager.filter_server_ids_by_ip(["pub", "priv"], client_ip=None) == [ + "pub" + ] + class TestFilterServerIdsByIpWithInfo: """Tests that filter_server_ids_by_ip_with_info returns accurate block counts.""" @@ -166,6 +181,21 @@ class TestFilterServerIdsByIpWithInfo: ) assert (allowed, blocked) == (["priv"], 0) + @patch("litellm.public_mcp_servers", []) + @patch("litellm.proxy.proxy_server.general_settings", {}) + def test_no_ip_lets_public_through_blocks_private(self): + # When IP extraction fails (client_ip=None), public servers stay + # reachable; only non-public servers are blocked. + pub = _make_server("pub", available_on_public_internet=True) + priv = _make_server("priv", available_on_public_internet=False) + manager = _make_manager([pub, priv]) + + allowed, blocked = manager.filter_server_ids_by_ip_with_info( + ["pub", "priv"], client_ip=None + ) + assert allowed == ["pub"] + assert blocked == 1 + @patch("litellm.public_mcp_servers", []) @patch("litellm.proxy.proxy_server.general_settings", {}) def test_all_private_external_ip_reports_all_blocked(self):