fix(proxy): unpinned upstream servers pooled behind one shared HTTP client (#587)

* Update sbom.yml

* initial commit

Signed-off-by: rajnisht7 <rajnishtiwari9787@gmail.com>

* remove empty line

---------

Signed-off-by: rajnisht7 <rajnishtiwari9787@gmail.com>

Author: rajnisht7

CHANGELOG.md

@@ -16,6 +16,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+**`CMCPProxy._client_for_upstream` pooled every unpinned upstream server in a session behind one shared `httpx.AsyncClient` (#281).** The pinned branch already keys its cached client on the fingerprint (a matching pin *is* the same verified peer, so sharing there is correct), but both unpinned branches plain `http://` and the `PLACEHOLDER_FINGERPRINT` dev-mode pin collapsed to the single literal key `"unpinned"`, regardless of which server the entry actually pointed at. A gateway session whose catalog lists two or more unrelated unpinned upstreams (the supported, if discouraged, dev/demo path this same method's docstring describes) got one `httpx.AsyncClient` for all of them: one cookie jar, and one shared pool of `max_connections`/`max_keepalive_connections`, across servers whose only thing in common was that neither presented a real pin.
+
 - **`POST /mcp` `tools/call` 500'd on a non-string `name`, and silently accepted a non-object `arguments`.** `_handle_tool_call` read `tool_name: str = params.get("name", "").lower()` and `arguments: dict[str, Any] = params.get("arguments", {})`: the `.get(field, default)` default only covers a genuinely *absent* field, so a caller-supplied `name` that is present but not a string (an int, a list, a bool, `null`) reached `.lower()` and raised an unhandled `AttributeError`, caught only by the outermost `_unhandled_error_handler` and logged as `UNHANDLED_EXCEPTION`/`INTERNAL_ERROR` for what is ordinary client input validation, not an internal failure. `arguments` had the matching gap on the other side: `_arg_shape_violation` (the #518/#562 depth/key-count/string-length gate) only recognizes `dict`, `list` and `str`, so a scalar `arguments` (an int, for instance) silently returned "no violation" and reached `call_tool` with a shape its own type annotation says cannot occur.
 
 Both fields are exactly as caller-controlled as `_cmcp` a few lines below, already guarded with "A malformed `_cmcp` (string, list, number) must not 500 the call path" `name` and `arguments` were the two places that same reasoning wasn't applied. Both now return the same `-32602 Invalid params` JSON-RPC error the adjacent depth/key/string-length and non-dict-`params` checks already return, before `call_tool` is ever reached.

src/cmcp_runtime/mcp/proxy.py

@@ -317,24 +317,38 @@ def _client_for_upstream(self, entry: CatalogEntry) -> httpx.AsyncClient:
           cannot be compared must never silently degrade to unpinned.
         - http: pinning is impossible without TLS; warn once per server
           (dev/demo only) and proceed.
+
+        Every branch keys the cached client on ``_server_execution_key(entry)``
+        (the same "security-relevant identity used to pool one upstream" that
+        ``_stdio_for`` keys its spawned children on), not only the pinned
+        branch. A cache keyed on the literal string ``"unpinned"`` would give
+        every plain-http and placeholder-pinned server in this session's
+        catalog the *same* ``httpx.AsyncClient`` - and therefore the same
+        cookie jar and connection-pool limits - regardless of how unrelated
+        those upstreams are. Two catalog entries that happen to share one real
+        pinned fingerprint correctly share a client: a matching pin *is* the
+        same verified peer. Two that merely share the fact that neither is
+        pinned are not the same peer, and must not share state meant to be
+        scoped to one.
         """
         server_url = entry.server.url
         fingerprint = entry.server.tls_fingerprint
         scheme = httpx.URL(server_url).scheme.lower()
+        identity = _server_execution_key(entry)
         if scheme != "https":
             self._warn_pin_unenforced(
                 server_url,
                 "upstream is not https, TLS fingerprint pinning is impossible - "
                 "plain-http upstreams are for local dev/demo only",
             )
-            key = "unpinned"
+            key = f"unpinned:{identity}"
         elif fingerprint == tls_pinning.PLACEHOLDER_FINGERPRINT:
             self._warn_pin_unenforced(
                 server_url,
                 "catalog tls_fingerprint is the unpinned-dev placeholder - peer "
                 "identity is verified by CA trust only, not pinned to the catalog",
             )
-            key = "unpinned"
+            key = f"unpinned:{identity}"
         elif not tls_pinning.FINGERPRINT_PATTERN.match(fingerprint):
             raise UpstreamUnavailable(
                 f"Catalog tls_fingerprint for {server_url} is malformed - refusing to connect",
@@ -346,7 +360,7 @@ def _client_for_upstream(self, entry: CatalogEntry) -> httpx.AsyncClient:
         client = self._http_clients.get(key)
         if client is None:
             timeout = httpx.Timeout(30.0)
-            if key == "unpinned":
+            if key.startswith("unpinned:"):
                 client = httpx.AsyncClient(
                     timeout=timeout, verify=tls_pinning.default_ssl_context()
                 )

tests/unit/test_mcp_proxy.py

@@ -62,6 +62,68 @@ def test_server_cache_keys_bind_actual_identity_not_display_label():
     assert _server_provenance_key(first) != _server_provenance_key(second)
 
 
+def _make_unpinned_entry(tool_name: str, url: str) -> CatalogEntry:
+    """An entry with no real TLS pin -- the placeholder, dev-mode value."""
+    from cmcp_runtime.mcp import tls_pinning
+
+    entry = _make_entry(tool_name)
+    entry.server.url = url
+    entry.server.tls_fingerprint = tls_pinning.PLACEHOLDER_FINGERPRINT
+    return entry
+
+
+def test_unpinned_upstreams_do_not_share_an_http_client():
+    """Two distinct, unpinned upstream servers must not be pooled behind the
+    same httpx.AsyncClient: that client carries a cookie jar and connection
+    limits, and two catalog entries that merely agree on having no real pin
+    are not the same peer. _server_execution_key already exists for exactly
+    this ("the security-relevant identity used to pool one upstream") and
+    _stdio_for already keys spawned children on it; _client_for_upstream must
+    key on it too, not on the constant string "unpinned"."""
+    entry_a = _make_unpinned_entry("a.tool", "https://server-a.example.com/mcp")
+    entry_b = _make_unpinned_entry("b.tool", "https://server-b.example.com/mcp")
+    catalog = ToolCatalog(
+        entries={"a.tool": entry_a, "b.tool": entry_b}, catalog_hash="sha256:" + "1" * 64
+    )
+    proxy, _, _ = _make_proxy(catalog=catalog)
+
+    client_a = proxy._client_for_upstream(entry_a)
+    client_b = proxy._client_for_upstream(entry_b)
+
+    assert client_a is not client_b
+    assert client_a.cookies is not client_b.cookies
+
+
+def test_same_unpinned_upstream_still_reuses_its_client():
+    """Two tools on the *same* unpinned server correctly share one client -
+    this is pooling by server identity, not a blanket per-tool client."""
+    entry_a1 = _make_unpinned_entry("a1.tool", "https://server-a.example.com/mcp")
+    entry_a2 = _make_unpinned_entry("a2.tool", "https://server-a.example.com/mcp")
+    catalog = ToolCatalog(
+        entries={"a1.tool": entry_a1, "a2.tool": entry_a2}, catalog_hash="sha256:" + "1" * 64
+    )
+    proxy, _, _ = _make_proxy(catalog=catalog)
+
+    assert proxy._client_for_upstream(entry_a1) is proxy._client_for_upstream(entry_a2)
+
+
+def test_different_servers_sharing_one_real_pin_still_share_a_client():
+    """The pinned branch is unchanged: a matching TLS fingerprint *is* the
+    same verified peer, so sharing a client there is correct, not a leak."""
+    entry_p1 = _make_entry("p1.tool")
+    entry_p1.server.url = "https://pinned-1.example.com/mcp"
+    entry_p1.server.tls_fingerprint = "SHA256:" + "A" * 43 + "B"
+    entry_p2 = _make_entry("p2.tool")
+    entry_p2.server.url = "https://pinned-2.example.com/mcp"
+    entry_p2.server.tls_fingerprint = entry_p1.server.tls_fingerprint
+    catalog = ToolCatalog(
+        entries={"p1.tool": entry_p1, "p2.tool": entry_p2}, catalog_hash="sha256:" + "1" * 64
+    )
+    proxy, _, _ = _make_proxy(catalog=catalog)
+
+    assert proxy._client_for_upstream(entry_p1) is proxy._client_for_upstream(entry_p2)
+
+
 def test_same_server_identity_is_reused_across_tools():
     first = _make_entry("first.tool")
     second = _make_entry("second.tool")