Return Clear error message why no tools are available / IP Filtering occured - #22142
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR improves error messaging when MCP tools are unavailable due to IP-based access control. It introduces a new
Confidence Score: 4/5
|
| Filename | Overview |
|---|---|
| litellm/proxy/_experimental/mcp_server/mcp_server_manager.py | Added filter_server_ids_by_ip_with_info method returning (allowed_ids, blocked_count) tuple, and refactored filter_server_ids_by_ip to delegate to it. Clean, backward-compatible change. |
| litellm/proxy/_experimental/mcp_server/rest_endpoints.py | Added IP-filtering-specific error messages for both single-server and all-server cases. Calls private method _is_server_accessible_from_ip from outside the class. Error messages expose internal config details to external callers. |
| litellm/proxy/_experimental/mcp_server/server.py | Updated to use filter_server_ids_by_ip_with_info and added debug logging when servers are blocked by IP filtering. Logging-only change with no behavioral impact. |
| tests/test_litellm/proxy/auth/test_mcp_ip_filtering.py | Added 4 new mock-only tests for filter_server_ids_by_ip_with_info covering external IP, internal IP, no IP, and all-private scenarios. Properly mock-based with no real network calls. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Client Request to /mcp-rest/tools/list] --> B{server_id specified?}
B -->|Yes| C[Check server_id in allowed_server_ids]
B -->|No| D{allowed_server_ids empty?}
C -->|Allowed| E[Fetch tools from server]
C -->|Not Allowed| F{IP blocked?}
F -->|Yes| G["403: ip_filtering error\n(server restricted by IP)"]
F -->|No| H["403: access_denied error\n(key not allowed)"]
D -->|Not Empty| I[Query all allowed servers for tools]
D -->|Empty| J{ip_blocked_count > 0?}
J -->|Yes| K["403: ip_filtering error\n(all servers restricted by IP)"]
J -->|No| L["403: access_denied error\n(key has no server access)"]
E --> M[Return tools list]
I --> M
Last reviewed commit: 48f549b
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| _server = global_mcp_server_manager.get_mcp_server_by_id(server_id) | ||
| if ( | ||
| _server is not None | ||
| and _rest_client_ip is not None | ||
| and not global_mcp_server_manager._is_server_accessible_from_ip( | ||
| _server, _rest_client_ip | ||
| ) | ||
| ): | ||
| raise HTTPException( | ||
| status_code=403, | ||
| detail={ | ||
| "error": "ip_filtering", | ||
| "message": ( | ||
| f"MCP server '{server_id}' is not accessible from your IP address " | ||
| f"({_rest_client_ip}). This server is restricted to internal " | ||
| "networks only. To make it externally accessible, set " | ||
| "'available_on_public_internet: true' in the server configuration." | ||
| ), | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Redundant server lookup and private method access
This block re-fetches the server via get_mcp_server_by_id(server_id) and then calls the private method _is_server_accessible_from_ip directly from outside the MCPServerManager class. Both operations were already performed inside filter_server_ids_by_ip_with_info a few lines above.
Consider instead checking whether the server_id was in the original allowed_server_ids_set (before IP filtering) to determine whether the denial is due to IP filtering vs. key-based access. This avoids the redundant lookup and calling a private method from outside its class.
| raise HTTPException( | ||
| status_code=403, | ||
| detail={ | ||
| "error": "ip_filtering", | ||
| "message": ( | ||
| f"MCP server '{server_id}' is not accessible from your IP address " | ||
| f"({_rest_client_ip}). This server is restricted to internal " | ||
| "networks only. To make it externally accessible, set " | ||
| "'available_on_public_internet: true' in the server configuration." | ||
| ), | ||
| }, |
There was a problem hiding this comment.
Error message exposes internal server configuration details
The error detail includes the client IP address, states that the server is "restricted to internal networks only", and tells the caller how to change the server configuration (available_on_public_internet: true). This is appropriate for operator-facing logs but may reveal more than desired to external API consumers, especially the configuration key name. A similar concern applies to the all-servers case on line 361.
Consider returning a more generic message to the API consumer (e.g., "Access denied for this server") and keeping the configuration hints in server-side logs only.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| "No tools from those servers will be returned. " | ||
| "To expose a server externally, set 'available_on_public_internet: true' " | ||
| "in its configuration.", | ||
| _ip_blocked, |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Copilot Autofix
AI 6 months ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
Return Clear error message why no tools are available / IP Filtering occured
2 participants
Relevant issues
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
tests/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 reviewCI (LiteLLM team)
Branch creation CI run
Link:
CI run for the last commit
Link:
Merge / cherry-pick CI run
Links:
Type
🐛 Bug Fix
Changes