mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-11 22:51:28 +00:00
fix(mcp): revalidate loopback at /callback + no-store on /token
Address codex review P0 + P1 findings on the discoverable OAuth proxy: - /callback now re-validates that the decoded base_url is loopback before 302-redirecting to it. State is encrypted but pre-existing states minted before the /authorize validation was added have no expiry and remain valid; validating at the sink closes the open-redirect + code-theft primitive for those stale states too. (VERIA-57 root cause B, P0.) - /token responses now set Cache-Control: no-store + Pragma: no-cache per RFC 6749 §5.1 (P1). - Move TOKEN_NO_CACHE_HEADERS constant from byok_oauth_endpoints into the shared oauth_utils module so both endpoints use the same value.
This commit is contained in:
parent
ef108e79a1
commit
200a38c3af
4 changed files with 106 additions and 13 deletions
|
|
@ -31,6 +31,7 @@ from litellm.proxy._experimental.mcp_server.discoverable_endpoints import (
|
|||
get_request_base_url,
|
||||
)
|
||||
from litellm.proxy._experimental.mcp_server.oauth_utils import (
|
||||
TOKEN_NO_CACHE_HEADERS,
|
||||
validate_loopback_redirect_uri,
|
||||
)
|
||||
from litellm.proxy._types import UserAPIKeyAuth
|
||||
|
|
@ -73,18 +74,13 @@ def _purge_expired_codes() -> None:
|
|||
del _byok_auth_codes[k]
|
||||
|
||||
|
||||
# RFC 6749 §5.1 / OAuth 2.1 draft-15 §4.1.3: token-endpoint responses must
|
||||
# not be cached (both success and error bodies may reveal secrets).
|
||||
_TOKEN_NO_CACHE_HEADERS = {"Cache-Control": "no-store", "Pragma": "no-cache"}
|
||||
|
||||
|
||||
def _oauth_token_error(code: str, status: int = 400) -> JSONResponse:
|
||||
"""RFC 6749 §5.2 token-endpoint error body: ``{"error": "<code>"}``.
|
||||
FastAPI's default ``HTTPException`` renders ``{"detail": ...}`` which
|
||||
spec-compliant OAuth clients parsing the ``error`` field won't recognize.
|
||||
"""
|
||||
return JSONResponse(
|
||||
status_code=status, content={"error": code}, headers=_TOKEN_NO_CACHE_HEADERS
|
||||
status_code=status, content={"error": code}, headers=TOKEN_NO_CACHE_HEADERS
|
||||
)
|
||||
|
||||
|
||||
|
|
@ -876,5 +872,5 @@ async def byok_token(
|
|||
"token_type": "bearer",
|
||||
"expires_in": 3600,
|
||||
},
|
||||
headers=_TOKEN_NO_CACHE_HEADERS,
|
||||
headers=TOKEN_NO_CACHE_HEADERS,
|
||||
)
|
||||
|
|
|
|||
|
|
@ -11,6 +11,7 @@ from litellm.llms.custom_httpx.http_handler import (
|
|||
httpxSpecialProvider,
|
||||
)
|
||||
from litellm.proxy._experimental.mcp_server.oauth_utils import (
|
||||
TOKEN_NO_CACHE_HEADERS,
|
||||
validate_loopback_redirect_uri,
|
||||
)
|
||||
from litellm.proxy.auth.ip_address_utils import IPAddressUtils
|
||||
|
|
@ -481,7 +482,8 @@ async def exchange_token_with_server(
|
|||
if "scope" in token_response and token_response["scope"]:
|
||||
result["scope"] = token_response["scope"]
|
||||
|
||||
return JSONResponse(result)
|
||||
# RFC 6749 §5.1: token responses must not be cached.
|
||||
return JSONResponse(result, headers=TOKEN_NO_CACHE_HEADERS)
|
||||
|
||||
|
||||
async def register_client_with_server(
|
||||
|
|
@ -648,20 +650,26 @@ async def token_endpoint(
|
|||
@router.get("/callback")
|
||||
async def callback(code: str, state: str):
|
||||
try:
|
||||
# Decode the state hash to get base_url, original state, and PKCE params
|
||||
state_data = decode_state_hash(state)
|
||||
base_url = state_data["base_url"]
|
||||
original_state = state_data["original_state"]
|
||||
|
||||
# Forward code and original state back to client
|
||||
params = {"code": code, "state": original_state}
|
||||
# Re-validate loopback at the sink. /authorize rejects non-loopback
|
||||
# redirect_uri before encoding into state, but encrypted states
|
||||
# minted before that check was added have no expiry and remain
|
||||
# valid indefinitely. Validating here blocks the open-redirect +
|
||||
# code-theft primitive even for pre-fix states.
|
||||
validate_loopback_redirect_uri(base_url)
|
||||
|
||||
# Forward to client's callback endpoint
|
||||
params = {"code": code, "state": original_state}
|
||||
complete_returned_url = f"{base_url}?{urlencode(params)}"
|
||||
return RedirectResponse(url=complete_returned_url, status_code=302)
|
||||
|
||||
except HTTPException:
|
||||
# Re-raise so a non-loopback base_url surfaces as 400 instead of
|
||||
# a generic "authentication incomplete" redirect.
|
||||
raise
|
||||
except Exception:
|
||||
# fallback if state hash not found
|
||||
return HTMLResponse(
|
||||
"<html><body>Authentication incomplete. You can close this window.</body></html>"
|
||||
)
|
||||
|
|
|
|||
|
|
@ -6,6 +6,10 @@ from urllib.parse import urlparse
|
|||
|
||||
from fastapi import HTTPException
|
||||
|
||||
# RFC 6749 §5.1 / OAuth 2.1 draft-15 §4.1.3: token-endpoint responses
|
||||
# must not be cached — both success and error bodies may reveal secrets.
|
||||
TOKEN_NO_CACHE_HEADERS = {"Cache-Control": "no-store", "Pragma": "no-cache"}
|
||||
|
||||
|
||||
def validate_loopback_redirect_uri(redirect_uri: str) -> None:
|
||||
"""Require a loopback ``redirect_uri`` (OAuth 2.1 §4.1.2.1 + RFC 8252
|
||||
|
|
|
|||
|
|
@ -1920,3 +1920,88 @@ async def test_authorize_endpoint_accepts_ipv4_loopback_range_and_ipv6_full_form
|
|||
state="s",
|
||||
)
|
||||
assert response.status_code == 307, f"{uri} should be accepted"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_callback_revalidates_loopback_on_decoded_base_url():
|
||||
"""VERIA-57 root cause B defense-in-depth: an encrypted state minted
|
||||
before the /authorize validation was added has no expiry and stays
|
||||
valid. /callback must re-validate the decoded base_url so those
|
||||
stale states can't be used as an open-redirect + code-theft
|
||||
primitive."""
|
||||
from litellm.proxy._experimental.mcp_server.discoverable_endpoints import (
|
||||
callback,
|
||||
)
|
||||
|
||||
with patch(
|
||||
"litellm.proxy._experimental.mcp_server.discoverable_endpoints.decode_state_hash"
|
||||
) as mock_decode:
|
||||
mock_decode.return_value = {
|
||||
"base_url": "https://attacker.example.com/cb",
|
||||
"original_state": "s",
|
||||
"code_challenge": None,
|
||||
"code_challenge_method": None,
|
||||
"client_redirect_uri": "https://attacker.example.com/cb",
|
||||
}
|
||||
with pytest.raises(HTTPException) as exc_info:
|
||||
await callback(code="stolen_code", state="encrypted_stale_state")
|
||||
assert exc_info.value.status_code == 400
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_token_endpoint_sets_no_store_cache_control():
|
||||
"""RFC 6749 §5.1 / OAuth 2.1 draft-15 §4.1.3: the token response
|
||||
contains an access token (and possibly a refresh token) — it MUST
|
||||
NOT be cached by intermediaries or the client."""
|
||||
from fastapi import Request
|
||||
|
||||
from litellm.proxy._experimental.mcp_server.discoverable_endpoints import (
|
||||
exchange_token_with_server,
|
||||
)
|
||||
from litellm.proxy._types import MCPTransport
|
||||
from litellm.types.mcp import MCPAuth
|
||||
from litellm.types.mcp_server.mcp_server_manager import MCPServer
|
||||
|
||||
server = MCPServer(
|
||||
server_id="t",
|
||||
name="t",
|
||||
server_name="t",
|
||||
alias="t",
|
||||
transport=MCPTransport.http,
|
||||
auth_type=MCPAuth.oauth2,
|
||||
client_id="cid",
|
||||
client_secret="cs",
|
||||
authorization_url="https://provider.com/oauth/authorize",
|
||||
token_url="https://provider.com/oauth/token",
|
||||
)
|
||||
mock_request = MagicMock(spec=Request)
|
||||
mock_request.base_url = "https://litellm.example.com/"
|
||||
mock_request.headers = {}
|
||||
|
||||
fake_http_response = MagicMock()
|
||||
fake_http_response.json.return_value = {
|
||||
"access_token": "tok",
|
||||
"token_type": "Bearer",
|
||||
"expires_in": 3600,
|
||||
}
|
||||
fake_http_response.raise_for_status = MagicMock()
|
||||
fake_http_client = MagicMock()
|
||||
fake_http_client.post = AsyncMock(return_value=fake_http_response)
|
||||
|
||||
with patch(
|
||||
"litellm.proxy._experimental.mcp_server.discoverable_endpoints.get_async_httpx_client",
|
||||
return_value=fake_http_client,
|
||||
):
|
||||
response = await exchange_token_with_server(
|
||||
request=mock_request,
|
||||
mcp_server=server,
|
||||
grant_type="authorization_code",
|
||||
code="c",
|
||||
redirect_uri="http://127.0.0.1:3000/cb",
|
||||
client_id="cid",
|
||||
client_secret=None,
|
||||
code_verifier=None,
|
||||
)
|
||||
|
||||
assert response.headers["cache-control"] == "no-store"
|
||||
assert response.headers["pragma"] == "no-cache"
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue