mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-12 23:01:41 +00:00
fix: address remaining Greptile review issues (round 2)
- mcp_server_manager: warn when hook Authorization overwrites existing header - __init__: remove _mcp_jwt_signer_instance from __all__ (private internal) - discoverable_endpoints: copy dict instead of mutating in-place on OIDC augmentation - test docstring: reflect warn-and-continue behavior for OpenAPI servers - test: update scope assertions for least-privilege (no mcp:tools/list on tool-call JWTs)
This commit is contained in:
parent
18fc3066ee
commit
9cceff757d
6 changed files with 42 additions and 17 deletions
|
|
@ -691,8 +691,11 @@ async def openid_configuration(request: Request):
|
|||
if signer is not None:
|
||||
request_base_url = get_request_base_url(request)
|
||||
if isinstance(response, dict):
|
||||
response["jwks_uri"] = f"{request_base_url}/.well-known/jwks.json"
|
||||
response["id_token_signing_alg_values_supported"] = ["RS256"]
|
||||
response = {
|
||||
**response,
|
||||
"jwks_uri": f"{request_base_url}/.well-known/jwks.json",
|
||||
"id_token_signing_alg_values_supported": ["RS256"],
|
||||
}
|
||||
except ImportError:
|
||||
pass
|
||||
|
||||
|
|
|
|||
|
|
@ -2135,6 +2135,12 @@ class MCPServerManager:
|
|||
if hook_extra_headers:
|
||||
if extra_headers is None:
|
||||
extra_headers = {}
|
||||
if "Authorization" in extra_headers and "Authorization" in hook_extra_headers:
|
||||
verbose_logger.warning(
|
||||
"MCPServerManager: hook_extra_headers contains 'Authorization' which will "
|
||||
"overwrite the existing Authorization header set by static_headers or server "
|
||||
"authentication. The hook JWT will take precedence."
|
||||
)
|
||||
extra_headers.update(hook_extra_headers)
|
||||
|
||||
stdio_env = self._build_stdio_env(mcp_server, raw_headers)
|
||||
|
|
|
|||
|
|
@ -4,7 +4,7 @@ from typing import TYPE_CHECKING
|
|||
|
||||
from litellm.types.guardrails import SupportedGuardrailIntegrations
|
||||
|
||||
from .mcp_jwt_signer import MCPJWTSigner, _mcp_jwt_signer_instance, get_mcp_jwt_signer
|
||||
from .mcp_jwt_signer import MCPJWTSigner, get_mcp_jwt_signer
|
||||
|
||||
if TYPE_CHECKING:
|
||||
from litellm.types.guardrails import Guardrail, LitellmParams
|
||||
|
|
@ -51,6 +51,5 @@ guardrail_class_registry = {
|
|||
__all__ = [
|
||||
"MCPJWTSigner",
|
||||
"initialize_guardrail",
|
||||
"_mcp_jwt_signer_instance",
|
||||
"get_mcp_jwt_signer",
|
||||
]
|
||||
|
|
|
|||
|
|
@ -27,6 +27,7 @@ your own RSA keypair. If unset, an RSA-2048 keypair is auto-generated at startup
|
|||
import base64
|
||||
import hashlib
|
||||
import os
|
||||
import re
|
||||
import time
|
||||
from typing import Any, Dict, Optional, Union
|
||||
|
||||
|
|
@ -169,6 +170,12 @@ class MCPJWTSigner(CustomGuardrail):
|
|||
|
||||
# Register singleton so the JWKS endpoint can access it.
|
||||
global _mcp_jwt_signer_instance
|
||||
if _mcp_jwt_signer_instance is not None:
|
||||
verbose_proxy_logger.warning(
|
||||
"MCPJWTSigner: replacing existing singleton — previously issued tokens "
|
||||
"signed with the old key will fail JWKS verification. "
|
||||
"Avoid configuring multiple mcp_jwt_signer guardrails."
|
||||
)
|
||||
_mcp_jwt_signer_instance = self
|
||||
|
||||
verbose_proxy_logger.info(
|
||||
|
|
@ -265,11 +272,19 @@ class MCPJWTSigner(CustomGuardrail):
|
|||
if end_user_id:
|
||||
claims["end_user_id"] = end_user_id
|
||||
|
||||
# scope: tool-level access
|
||||
tool_name: str = data.get("mcp_tool_name", "")
|
||||
scopes = ["mcp:tools/call", "mcp:tools/list"]
|
||||
# scope: minimal tool-level access.
|
||||
# Only grant mcp:tools/list when no specific tool is being called —
|
||||
# tool call JWTs should not carry enumeration permissions.
|
||||
# Tool names are sanitized (alphanumeric + _ and -) before embedding
|
||||
# so path-traversal or malformed scope values cannot be injected.
|
||||
import re
|
||||
|
||||
raw_tool_name: str = data.get("mcp_tool_name", "")
|
||||
tool_name = re.sub(r"[^a-zA-Z0-9_\-]", "_", raw_tool_name) if raw_tool_name else ""
|
||||
if tool_name:
|
||||
scopes.append(f"mcp:tools/{tool_name}:call")
|
||||
scopes = ["mcp:tools/call", f"mcp:tools/{tool_name}:call"]
|
||||
else:
|
||||
scopes = ["mcp:tools/call", "mcp:tools/list"]
|
||||
claims["scope"] = " ".join(scopes)
|
||||
|
||||
return claims
|
||||
|
|
@ -301,9 +316,11 @@ class MCPJWTSigner(CustomGuardrail):
|
|||
algorithm=self.ALGORITHM,
|
||||
)
|
||||
|
||||
data["extra_headers"] = {
|
||||
"Authorization": f"Bearer {signed_token}",
|
||||
}
|
||||
# Merge into existing extra_headers rather than replacing — a prior guardrail
|
||||
# in the chain may have already injected headers (e.g. tracing, correlation IDs).
|
||||
# MCPJWTSigner sets Authorization last so its JWT takes precedence.
|
||||
existing_headers: Dict[str, str] = data.get("extra_headers") or {}
|
||||
data["extra_headers"] = {**existing_headers, "Authorization": f"Bearer {signed_token}"}
|
||||
|
||||
verbose_proxy_logger.debug(
|
||||
"MCPJWTSigner: signed JWT sub=%s act=%s tool=%s exp=%d",
|
||||
|
|
|
|||
|
|
@ -6,7 +6,7 @@ Validates that:
|
|||
2. pre_call_tool_check returns hook-provided extra_headers AND modified arguments
|
||||
3. call_tool flows hook headers and modified arguments downstream
|
||||
4. Hook-provided headers take highest priority (merge after static_headers)
|
||||
5. OpenAPI-backed servers raise HTTPException when hook headers are present
|
||||
5. OpenAPI-backed servers log a warning and continue (skip injection) when hook headers are present
|
||||
6. JWT claims are propagated in both standard and virtual-key fast paths
|
||||
7. Backward compatibility: hooks without extra_headers continue to work
|
||||
"""
|
||||
|
|
|
|||
|
|
@ -201,12 +201,13 @@ def test_build_claims_scope_with_tool():
|
|||
|
||||
scopes = set(claims["scope"].split())
|
||||
assert "mcp:tools/call" in scopes
|
||||
assert "mcp:tools/list" in scopes
|
||||
assert "mcp:tools/search_web:call" in scopes
|
||||
# Tool-call JWTs must NOT carry mcp:tools/list — least-privilege
|
||||
assert "mcp:tools/list" not in scopes
|
||||
|
||||
|
||||
def test_build_claims_scope_without_tool():
|
||||
"""_build_claims() omits per-tool scope when mcp_tool_name is not set."""
|
||||
"""_build_claims() includes mcp:tools/list when no specific tool is called."""
|
||||
signer = _make_signer()
|
||||
user_dict = _make_user_api_key_dict()
|
||||
data: Dict[str, Any] = {}
|
||||
|
|
@ -216,9 +217,8 @@ def test_build_claims_scope_without_tool():
|
|||
scopes = set(claims["scope"].split())
|
||||
assert "mcp:tools/call" in scopes
|
||||
assert "mcp:tools/list" in scopes
|
||||
# No per-tool scope
|
||||
for scope in scopes:
|
||||
assert ":" not in scope.replace("mcp:", "") or scope.endswith(":call") is False or scope == "mcp:tools/call"
|
||||
# No per-tool call scope when no tool name was given
|
||||
assert not any(s.endswith(":call") and s != "mcp:tools/call" for s in scopes)
|
||||
|
||||
|
||||
def test_build_claims_act_fallback_to_litellm_proxy():
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue