Allow pre_mcp_call guardrail hooks to mutate outbound MCP headers#23889
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR extends the Key changes:
Minor concerns:
Confidence Score: 4/5
|
| Filename | Overview |
|---|---|
| litellm/proxy/_experimental/mcp_server/mcp_server_manager.py | Core change: pre_call_tool_check now returns a Dict[str, Any] containing optional arguments and extra_headers from hook responses. call_tool captures this dict and propagates it downstream; _call_regular_mcp_tool gains a new hook_extra_headers parameter merged last (highest priority) into outbound headers. OpenAPI-backed servers reject hook header injection early via an HTTP 400. One minor defensive-coding concern on the modified_kwargs["arguments"] access pattern. |
| litellm/proxy/utils.py | Small additive change to _convert_mcp_hook_response_to_kwargs: now extracts extra_headers from the hook response dict in addition to modified_arguments. Backward compatible — no extra_headers in the response simply leaves the key absent. |
| litellm/proxy/_types.py | Adds optional jwt_claims: Optional[Dict] = None field to UserAPIKeyAuth. Backward compatible. Minor typing concern: should be Optional[Dict[str, Any]] for consistency with actual usage. |
| litellm/proxy/auth/user_api_key_auth.py | JWT claims are now propagated to UserAPIKeyAuth in three paths: virtual-key fast path (post _resolve_jwt_to_virtual_key), proxy-admin standard JWT path, and non-admin standard JWT path. All changes are additive and backward compatible. |
| tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_hook_extra_headers.py | 701-line mock-only test suite covering all major paths: _convert_mcp_hook_response_to_kwargs, pre_call_tool_check return values, call_tool header/argument propagation, OpenAPI rejection, JWT claims field. Tests use only mocks — no real network calls. Minor quality concern: TestHookHeaderMergePriority tests use broad except Exception: pass which can mask unrelated failures. |
Sequence Diagram
sequenceDiagram
participant Client
participant MCPServerManager
participant ProxyLogging
participant GuardrailHook
participant MCPServer
Client->>MCPServerManager: call_tool(name, arguments, proxy_logging_obj)
MCPServerManager->>MCPServerManager: _get_mcp_server_from_tool_name()
alt proxy_logging_obj present
MCPServerManager->>MCPServerManager: pre_call_tool_check()
MCPServerManager->>ProxyLogging: pre_call_hook(data, call_type)
ProxyLogging->>GuardrailHook: custom hook invoked
GuardrailHook-->>ProxyLogging: {modified_arguments?, extra_headers?}
ProxyLogging-->>MCPServerManager: hook response dict
MCPServerManager->>ProxyLogging: _convert_mcp_hook_response_to_kwargs()
ProxyLogging-->>MCPServerManager: modified_kwargs
MCPServerManager-->>MCPServerManager: hook_result = {arguments?, extra_headers?}
end
alt mcp_server.spec_path AND hook_result.extra_headers
MCPServerManager-->>Client: HTTP 400 (OpenAPI servers unsupported)
end
alt Regular MCP server (no spec_path)
MCPServerManager->>MCPServerManager: _call_regular_mcp_tool(hook_extra_headers=...)
Note over MCPServerManager: Merge order: oauth2 → extra_headers config → static_headers → hook_extra_headers (highest priority)
MCPServerManager->>MCPServer: call_tool(arguments, extra_headers merged)
MCPServer-->>MCPServerManager: CallToolResult
MCPServerManager-->>Client: CallToolResult
else OpenAPI server (spec_path set, no hook headers)
MCPServerManager->>MCPServerManager: _call_openapi_tool_handler()
MCPServerManager-->>Client: CallToolResult
end
Comments Outside Diff (3)
-
tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_hook_extra_headers.py, line 757-775 (link)Silent exception swallowing masks test failures
The broad
except Exception: passblocks in all threeTestHookHeaderMergePrioritytests silently swallow any exception raised during_call_regular_mcp_tool. If the code under test raises an unexpected error (e.g., anAttributeError,TypeError, orKeyErrorunrelated to the test's intent), the test would still pass becausecaptured_extra_headers["value"]was already set byfake_create_mcp_clientbefore the exception occurred.This is also true for
test_no_hook_headers_preserves_existing_behaviorandtest_hook_headers_merge_with_oauth2.Consider only catching expected exceptions, or asserting on the captured headers before the try/except:
try: await manager._call_regular_mcp_tool(...) except (AttributeError, KeyError): # Expected — _call_regular_mcp_tool may fail downstream of client creation pass headers = captured_extra_headers.get("value", {}) assert headers["Authorization"] == "Bearer hook-signed-jwt"
-
litellm/proxy/_types.py, line 2471 (link)Loose typing on
jwt_claimsfieldOptional[Dict]is unparameterized; preferOptional[Dict[str, Any]]to match howjwt_claimsdicts are used throughout the codebase (mixed string keys and arbitrary values like sub, iss, groups, exp).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!
-
litellm/proxy/_experimental/mcp_server/mcp_server_manager.py, line 1994-1995 (link)modified_kwargs["arguments"]could beNoneif the key is absentmodified_kwargs.get("arguments")returnsNonewhen"arguments"is not inmodified_kwargs, but the subsequentmodified_kwargs["arguments"]would then sethook_result["arguments"] = None(which is falsy but not the same as "hook did not modify arguments").In practice this path is safe because
_convert_mcp_hook_response_to_kwargsalways starts withoriginal_kwargs.copy()(which contains"arguments"), so the key is always present. However, the implicit coupling is fragile — if the helper is ever refactored, this line could silently passNonearguments into the tool call.A safer pattern would be:
new_args = modified_kwargs.get("arguments") if new_args is not None and new_args != arguments: hook_result["arguments"] = new_args
Last reviewed commit: 9f443a3
… headers. Update tests to validate argument mutation and header injection behavior, including warnings for OpenAPI-backed servers when headers are present.
… OpenAPI-backed servers. Update tests to reflect this change, ensuring proper exception handling instead of logging warnings.
…or extra headers in OpenAPI-backed servers. Adjust tests to verify the correct status code and exception message.
5253b6c
into
BerriAI:worktree-fluttering-sleeping-cookie
- Add xai/grok-4.20-beta-0309-reasoning (3rd xAI model, was missing) - Update New Model count 11 → 12 - Fix supports_minimal_reasoning_effort description (full gpt-5.x series) - Add Akto guardrail integration (BerriAI#23250) - Add MCP JWT Signer guardrail (BerriAI#23897) - Add pre_mcp_call header mutation (BerriAI#23889) - Add litellm --setup wizard (BerriAI#23644) - Fix ### Bug Fixes → #### Bugs under New Models - Add missing Documentation Updates section - Rename Diff Summary "AI Integrations" → "Logging / Guardrail / Prompt Management Integrations" Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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 reviewDelays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
CI (LiteLLM team)
Branch creation CI run
Link:
CI run for the last commit
Link:
Merge / cherry-pick CI run
Links:
Type
🆕 New Feature
🐛 Bug Fix
🧹 Refactoring
📖 Documentation
🚄 Infrastructure
✅ Test
Changes