From 3e9a6bee82dcd7b8c6c6cdfd7e4a13e07cb09754 Mon Sep 17 00:00:00 2001 From: yucheng Date: Fri, 21 Aug 2026 05:59:55 +0000 Subject: [PATCH] fix(mcp): keep params, query and fragment significant in issuer comparison The shared canonicalizer drops all three, so two issuers differing only outside the path compared equal and a response from another tenant's authorization server would have continued through the flow. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- .../_experimental/mcp_server/oauth_utils.py | 10 ++++++++- .../mcp_server/test_discoverable_endpoints.py | 22 +++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/litellm/proxy/_experimental/mcp_server/oauth_utils.py b/litellm/proxy/_experimental/mcp_server/oauth_utils.py index 676a356991d..10914b6f4a3 100644 --- a/litellm/proxy/_experimental/mcp_server/oauth_utils.py +++ b/litellm/proxy/_experimental/mcp_server/oauth_utils.py @@ -636,7 +636,15 @@ def canonicalize_url_identity(url: str) -> str: def issuer_identities_match(claimed_issuer: str, expected_issuer: str) -> bool: """Issuer equality tolerant only of URL-insignificant differences (scheme/host case, the default port, a trailing slash), through the shared canonicalizer. Used for RFC 8414 ยง3.3 metadata - anchoring and for the RFC 9207 ``iss`` an authorization response carries.""" + anchoring and for the RFC 9207 ``iss`` an authorization response carries. + + An RFC 8414 issuer identifier carries no params, query or fragment, and the canonicalizer drops + all three, so two issuers differing only there would compare equal. That difference is compared + on the raw URLs first, keeping a tenant that a deployment encoded outside the path distinct.""" + claimed: Final = urlparse(claimed_issuer) + expected: Final = urlparse(expected_issuer) + if (claimed.params, claimed.query, claimed.fragment) != (expected.params, expected.query, expected.fragment): + return False return canonicalize_url_identity(claimed_issuer) == canonicalize_url_identity(expected_issuer) 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 5ab825774b8..54bf8d8f680 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 @@ -4139,6 +4139,28 @@ async def test_callback_forwards_when_issuer_is_unknown_or_iss_absent(monkeypatc assert unanchored_response.status_code == 302 +@pytest.mark.asyncio +async def test_callback_rejects_an_issuer_differing_only_outside_the_path(monkeypatch): + """A tenant a deployment encoded in a query string is part of that issuer's identity, so the + comparison must not canonicalize it away and let another tenant's response through.""" + from litellm.proxy._experimental.mcp_server.oauth_utils import issuer_identities_match + + assert not issuer_identities_match("https://idp.example.com/?tenant=b", "https://idp.example.com/?tenant=a") + assert not issuer_identities_match("https://idp.example.com/#b", "https://idp.example.com/#a") + assert issuer_identities_match("https://IDP.example.com:443/?tenant=a", "https://idp.example.com/?tenant=a") + + response, state_data = await _authorize_then_callback( + _issuer_anchored_oauth_server(issuer="https://idp.example.com/?tenant=a"), + iss="https://idp.example.com/?tenant=b", + monkeypatch=monkeypatch, + ) + + assert state_data["expected_issuer"] == "https://idp.example.com/?tenant=a" + assert response.status_code == 400 + assert "location" not in response.headers + assert b"upstream-auth-code" not in response.body + + @pytest.mark.asyncio async def test_callback_accepts_states_minted_before_the_issuer_was_sealed(monkeypatch): """An authorization in flight across the upgrade carries no sealed issuer and must still land."""