chore(mcp): SSRF guard on OAuth metadata discovery follow-up fetches - #26849
chore(mcp): SSRF guard on OAuth metadata discovery follow-up fetches#26849yuneng-berri merged 3 commits into
Conversation
The OAuth discovery code in mcp_server_manager followed two
attacker-influenceable URLs without validation: the
``resource_metadata`` URL parsed out of a ``WWW-Authenticate``
challenge, and the ``authorization_servers[0]`` field of the
PRM JSON returned by the resource server. A malicious MCP server
could point those at a cloud-instance-metadata service, an internal
admin panel, or a loopback debug endpoint and the proxy would issue
a blind GET on its behalf.
Add ``_is_safe_metadata_url(url, server_url)`` and gate both follow-
up fetch sites on it. A URL is allowed when:
- it shares scheme + host + port with ``server_url`` (well-known
endpoints constructed from the admin's URL, and PRM published at
the resource server itself per RFC 9728 §3.3), or
- it resolves to publicly-routable IPs only (covers federated
authorization servers — Azure Entra, Google, Okta, GitHub —
hosted cross-origin from the resource server).
URLs that resolve to private / loopback / link-local / cloud-metadata
addresses, or that don't resolve at all, are rejected. ``http`` and
``https`` are the only schemes accepted. The IP block list is
provided by the existing ``_is_blocked_ip`` helper from
``litellm_core_utils.url_utils`` so the policy stays consistent with
the rest of the proxy.
The guard does not protect against active DNS rebinding between
this resolution and the subsequent httpx GET — the same-authority
pin remains the primary mitigation; the IP check is defence in
depth. The surface only triggers on config load / add-server, not
per request, so the synchronous ``getaddrinfo`` is acceptable.
Threads ``server_url`` through ``_fetch_oauth_metadata_from_resource``,
``_fetch_authorization_server_metadata``, and
``_fetch_single_authorization_server_metadata``. Existing tests for
those helpers updated for the new signature; new
``TestOAuthDiscoverySSRFGuard`` covers same-authority allow,
private-IP rejection across IPv4 and IPv6, multi-A-record dual-
stack rejection, unresolvable hosts, non-http schemes, and
end-to-end "no network call when guard denies".
Greptile SummaryThis PR guards the two attacker-influenceable OAuth discovery follow-up fetches in Confidence Score: 5/5Safe to merge — the fix is well-scoped, both SSRF entry points are guarded, and the test suite covers the key attack scenarios without real network calls. No P0 or P1 findings. The two previously flagged issues (redirect-based bypass and empty-getaddrinfo edge case) are both resolved: No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/_experimental/mcp_server/mcp_server_manager.py | Adds _is_same_authority_metadata_url and _fetch_oauth_discovery_url helpers; routes cross-origin OAuth metadata fetches through async_safe_get with SSRFError caught at both fetch sites. Same-authority fetches remain direct but have follow_redirects=False. Logic is correct. |
| tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py | Adds TestOAuthDiscoverySSRFGuard with 12 new tests covering same-authority eligibility, cross-origin blocking for private/loopback/cloud-metadata IPs, public federated auth pass-through, redirect revalidation, empty DNS result, non-http scheme rejection, and dual-resolution blocking. All tests are properly mocked with no real network calls. |
Reviews (3): Last reviewed commit: "fix(mcp): reuse safe URL fetch for OAuth..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Three follow-ups to the OAuth-discovery SSRF guard: 1. Greptile P1 (redirect bypass): the validated origin could return a 3xx whose ``Location`` points at an internal address, and httpx would follow without re-checking the new target. Pass ``follow_redirects=False`` to both gated httpx GETs. Spec-compliant OAuth/OIDC metadata endpoints serve the JSON directly, so this doesn't affect legitimate providers. 2. Greptile P2 (empty getaddrinfo): POSIX doesn't strictly forbid an empty success-list from ``getaddrinfo``. Add an explicit ``if not infos: return False`` so the guard fails closed instead of falling through to ``return True``. 3. Mypy: ``info[4][0]`` is typed ``str | int``; narrow at the boundary with an ``isinstance`` check (fail-closed if non-str). Adds two regression tests verifying ``follow_redirects=False`` is passed at both gated fetch sites, and one verifying the empty-list case rejects the URL.
|
@greptileai please re-review |
256e05e
into
BerriAI:litellm_internal_staging
chore(mcp): SSRF guard on OAuth metadata discovery follow-up fetches
2 participants
Relevant issues
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
tests/test_litellm/directory, Adding at least 1 test is a hard requirement - see detailsmake test-unit@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewType
🐛 Bug Fix
Changes
The OAuth discovery code in
MCPServerManagerfollowed two attacker-influenceable URLs:resource_metadataURL parsed out of aWWW-Authenticatechallenge returned by the MCP server.authorization_servers[0]field of the protected-resource-metadata JSON returned by the resource server.A malicious MCP server could point either URL at a cloud-instance-metadata service, an internal admin panel, or a loopback debug endpoint, and the proxy would issue blind GETs during config-load / add-server OAuth discovery.
This PR keeps same-authority metadata fetches direct with
follow_redirects=False. That preserves well-known discovery and administrator-configured internal MCP servers where the metadata URL shares scheme, host, and port with the configured MCPserver_url. Redirects stay disabled on that path because aLocationtarget would not inherit the same-authority guarantee.Cross-origin OAuth metadata fetches now go through the existing
async_safe_get()helper fromlitellm_core_utils.url_utils. That reuses the proxy-wide user URL validation policy instead of adding a second URL safety implementation:http/httpsonly, DNS validation, blocked private / loopback / link-local / cloud-metadata targets, safe Host handling, and redirect revalidation on every hop.SSRFErroris caught at both OAuth metadata fetch sites so denied discovery URLs fail closed and no network call is made for blocked targets.Compatibility
No config or workflow changes are expected for normal public OAuth providers. Federated public auth servers such as Azure Entra, Google, Okta, and GitHub remain supported through
async_safe_get().Same-authority internal MCP servers remain supported. Cross-origin internal / loopback / metadata OAuth discovery URLs are blocked by the existing safe URL policy unless an administrator has explicitly opted into the existing URL validation controls.
Tests
TestOAuthDiscoverySSRFGuardcovers:async_safe_get().http/httpsschemes, and mixed safe/unsafe DNS results fail closed.Validation run locally:
uv run black litellm/proxy/_experimental/mcp_server/mcp_server_manager.py tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.pyuv run pytest tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py -q(119 passed)uv run ruff check litellm/proxy/_experimental/mcp_server/mcp_server_manager.pyuv run ruff check tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py --ignore T201(T201is from a pre-existingprint()elsewhere in the test file)