fix(mcp): gate OAuth authorize/token/register/discovery on auth_type=oauth2 - #31736
Conversation
…oauth2 A non-oauth2 MCP server (notably auth_type=none, access-group gated) has no client_id and no authorization URL, yet the gateway OAuth endpoints did not check auth_type. authorize() raised "client_id is required" before the auth_type was ever examined, and the .well-known discovery builders always advertised authorization_servers / authorization_endpoint / token_endpoint / registration_endpoint, so spec-compliant MCP clients were pointed at an OAuth flow that can never succeed. Add an auth_type != oauth2 guard to the authorize, token, register, protected-resource and authorization-server paths (covering the internal UI OAuth endpoints too). The discovery guard sits after the OAuth pass-through branch so genuine pass-through servers keep proxying their upstream metadata. oauth2 servers are unaffected.
Greptile SummaryGates the MCP OAuth authorize, token, register, and discovery endpoints behind an
Confidence Score: 5/5Safe to merge — the changes are narrowly scoped guards that fire only when auth_type != oauth2; oauth2 servers are completely unaffected, and the pass-through branch is preserved. The auth_type guards are logically correct, the placement of the protected-resource guard (after the pass-through branch) is intentional and explicitly regression-tested, discovery now correctly returns 404 for both unknown and non-oauth2 server names, and all test modifications add specificity rather than weakening coverage. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py | Adds _raise_if_not_oauth2 and _raise_unless_oauth2_discovery_server guards; correctly placed after pass-through branch in protected-resource discovery and before client_id checks in authorize/token/register flows. Logic is sound. |
| litellm/proxy/management_endpoints/mcp_management_endpoints.py | Imports _raise_if_not_oauth2 and inserts it immediately after _get_cached_temporary_mcp_server_or_404 in mcp_authorize and mcp_token handlers; correct placement and import scope. |
| tests/test_litellm/proxy/_experimental/mcp_server/test_discoverable_endpoints.py | Eight new tests added: authorize/token/register reject non-oauth2 servers; both discovery shapes 404 on non-oauth2 and unknown server names; pass-through guard regression is explicitly covered. All mock-only, no network calls. |
| tests/test_litellm/proxy/management_endpoints/test_mcp_management_endpoints.py | Two new rejection tests for mcp_authorize and mcp_token; existing success-path tests correctly updated to set auth_type=MCPAuth.oauth2 so the guard passes — changes are accurate, not coverage-weakening. |
Reviews (5): Last reviewed commit: "Apply suggestions from code review" | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR gates the MCP gateway's OAuth authorize/token/register/discovery endpoints on
Confidence Score: 4/5Safe to merge; the fix correctly blocks non-oauth2 servers from OAuth flows and includes meaningful test coverage for the key paths. The core guard logic is correct and placed at the right points. The two observations are minor: the error message body inaccurately describes token-exchange servers as 'access-group gated', and the mcp_register management endpoint is guarded only indirectly (inside the delegate) rather than at the handler level like its siblings. Neither causes incorrect runtime behavior today. mcp_management_endpoints.py — the mcp_register handler lacks a direct guard call and a dedicated rejection test, in contrast to mcp_authorize and mcp_token which both received explicit guards and tests.
|
| Filename | Overview |
|---|---|
| litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py | Adds _raise_if_not_oauth2 and _raise_if_discovery_server_not_oauth2 guards; wires them into authorize, exchange_token_with_server, register_client_with_server, and both discovery builders. Guard placement relative to the pass-through branch is correct. Error message body is inaccurate for non-none auth types like oauth2_token_exchange. |
| litellm/proxy/management_endpoints/mcp_management_endpoints.py | mcp_authorize and mcp_token now have _raise_if_not_oauth2 directly in the handler and matching tests. mcp_register relies on the guard inside register_client_with_server — functionally correct but inconsistent with the sibling endpoints and lacks a dedicated rejection test. |
| tests/test_litellm/proxy/_experimental/mcp_server/test_discoverable_endpoints.py | Adds six new tests covering authorize, token, register, and both discovery shapes for a none-auth server, plus a regression guard for the passthrough ordering. Existing tests updated to set auth_type=MCPAuth.oauth2 where appropriate. All tests use mocks and make no real network calls. |
| tests/test_litellm/proxy/management_endpoints/test_mcp_management_endpoints.py | Adds rejection tests for mcp_authorize and mcp_token with none-auth servers; updates existing tests to set auth_type=oauth2. No rejection test for mcp_register. |
Comments Outside Diff (1)
-
litellm/proxy/management_endpoints/mcp_management_endpoints.py, line 1703-1720 (link)mcp_registermanagement endpoint has no direct guard and no rejection testmcp_authorizeandmcp_tokenboth received an explicit_raise_if_not_oauth2call directly in the management endpoint handler, and each got a corresponding rejection test.mcp_registerrelies on the guard embedded insideregister_client_with_server, meaning the defence is present but is one level deeper and invisible at the call-site. For consistency with the two sibling endpoints — and to keep the testtest_mcp_management_endpoints.pyaligned with howmcp_authorize/mcp_tokenare tested — adding_raise_if_not_oauth2(mcp_server)immediately after the server fetch and a matching rejection test would round out the coverage.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Reviews (2): Last reviewed commit: "fix(mcp): gate OAuth authorize/token/reg..." | Re-trigger Greptile
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 1 · PR risk: 0/10 |
… close enumeration oracle Address review feedback on the auth_type gate. The 400 message no longer claims access is governed by access groups, which is only true for auth_type=none; it now states that the gateway runs the OAuth client_id/authorize/token/register flow only for oauth2 servers and that the server is reached using its configured auth_type, which is accurate for every non-oauth2 type (api_key, oauth2_token_exchange, etc.). The discovery gate previously 404'd a named non-oauth2 server but still returned 200 metadata for an unknown name, which both serves a broken document for a typo and lets an unauthenticated caller enumerate non-OAuth server names by comparing 404 vs 200. A named discovery request now returns 200 only when it resolves to an oauth2 server; unknown (or hidden) and non-oauth2 names return the same 404. Root discovery and pass-through servers are unaffected.
|
Thanks for the review. Addressed in 0538e52 Error message over-generalizing to access-group servers (P2): correct. The 400 message no longer asserts access-group semantics. It now states that the gateway exposes the OAuth client_id/authorize/token/register flow only for Unknown server name silently passes the discovery guard / name enumeration (P2 + Veria): correct, and this was a real asymmetry the first commit introduced (named non-oauth2 returned 404 while an unknown name still returned 200). The discovery guard now returns 200 only when a named request resolves to an |
|
Both fixes verified — the implementation in Error message ( Discovery enumeration oracle ( Guard placement in Existing tests: Explicitly setting The fix is complete and well-covered. LGTM. |
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Relevant issues
Internally reported MCP bug. When a LiteLLM-hosted MCP server is configured with
auth_type=none(access-group gated, no per-user credential), driving it through the MCP OAuth endpoints returns a misleadingclient_id is requirederror, and the.well-knowndiscovery docs advertise an OAuth flow that cannot succeed. A none-auth server has noclient_idand no authorization URL; it is reached purely by access-group membership, so it should never enter an authorization-code flowLinear ticket
Reported via internal bug report; no Linear ticket
Pre-Submission checklist
@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewScreenshots / Proof of Fix
A/B on a live proxy (
localhost:4010) backed by Postgres, with an access-group gatedauth_type=noneserver (access_group_none) and, for the regression check, anoauth2server (github_oauth). Baseline islitellm_internal_stagingat this branch's base commit59f51b2d72, checked out into the working tree; fixed is this branch's HEAD. Same config, same DB, same commandsThe exact repro from the report (note the literal
redirect_uri=http://localhost:1/cband noclient_id)Baseline (
litellm_internal_staging): the none-auth server dead-ends on the misleading error and is advertised as OAuth-protectedFixed (this branch), identical commands: an accurate 400 on authorize and a 404 on discovery
The token and register endpoints on the none-auth server return the same accurate 400
An unknown server name returns the same 404 as the non-oauth2 server, so the well-known paths cannot be used to enumerate non-OAuth server names
No regression on the
oauth2server or root discovery; authorize still redirects, discovery and registration still serve metadata, and root (unnamed) discovery still returns generic gateway metadataType
🐛 Bug Fix
Changes
The gateway only acts as an OAuth authorization server for
auth_type=oauth2MCP servers. Every other auth type, notablynone(access-group gated), has noclient_idand no authorization URL and reaches its upstream by another mechanism, so it must not be driven through the gateway's client_id/authorize/token/register flow or advertised in discoveryauthorize()checkedclient_idbefore it checkedauth_type, so a none-auth server raisedclient_id is requiredeven though the accurate reason is that it does not use OAuth. The.well-knownbuilders never gated onauth_type, so_build_oauth_protected_resource_responsealways returnedauthorization_serversand_build_oauth_authorization_server_responsealways returnedauthorization_endpoint/token_endpoint/registration_endpoint, regardless of auth typeThis adds a shared
auth_typeguard before theclient_idcheck on the authorize, token and register paths (so the accurate reason wins over the credential error), and a discovery guard on the protected-resource and authorization-server paths. The protected-resource guard is placed after the OAuth pass-through branch so genuine pass-through servers (auth_type=nonewithoauth_passthrough) keep proxying their upstream metadata rather than being 404'd. The same prematureclient_idcheck existed on the internal UI OAuth endpoints (/server/oauth/{server_id}/authorizeand.../token), so those are gated too. The guards fire only whenauth_type != oauth2, so existing oauth2 servers are unaffectedThe discovery guard returns 200 only when a named request resolves to an oauth2 server. A named server that is unknown (or hidden from the caller) and one that exists but is non-oauth2 both return the same 404, which avoids serving a broken metadata document for a typo'd name and prevents using the unauthenticated well-known paths to enumerate non-OAuth server names. Root (unnamed) discovery still returns generic gateway metadata
The non-oauth2 400 message describes the actual constraint (the gateway runs the OAuth flow only for oauth2 servers; the server is reached using its configured
auth_type) so it stays accurate across every non-oauth2 type, includingoauth2_token_exchange, rather than asserting access-group semantics that only apply toauth_type=noneCoverage note:
!= oauth2intentionally also excludesoauth2_token_exchangeand every other non-redirect auth type, which is correct since none of them use the authorization-code flow exposed by these endpointsTests extend the existing mapped files. The discoverable-endpoint tests assert the accurate 400/404 on a none-auth server for authorize, token, register and both discovery shapes, that an unknown server name returns 404 from both discovery shapes, and that a pass-through none-auth server still proxies upstream metadata (so the protected-resource guard cannot be moved before the pass-through branch). The management-endpoint tests assert
mcp_authorizeandmcp_tokenreject a none-auth server before the client_id check and never delegate. Each new test fails on the unfixed code and passes with the fix