fix(mcp): stop 404ing internal-only MCP servers when XFF is on without trusted proxy ranges - #31150
fix(mcp): stop 404ing internal-only MCP servers when XFF is on without trusted proxy ranges#31150mateo-berri wants to merge 3 commits into
Conversation
…t trusted proxy ranges get_mcp_client_ip returned "" (fail closed) when use_x_forwarded_for was enabled but mcp_trusted_proxy_ranges was unset. That empty IP made is_internal_ip false, so every available_on_public_internet=false server was hidden and get_mcp_server_by_name returned None, which the MCP routes surfaced as 404 for all internal-only servers behind a reverse proxy. Honor X-Forwarded-For whenever use_x_forwarded_for is set (matching pre-1.89 behavior and the rest of the proxy's XFF handling); keep the spoof defense only when mcp_trusted_proxy_ranges is configured, where an untrusted direct peer falls back to its literal IP. OAuth URL building keeps failing closed via is_request_from_trusted_proxy, unchanged. Fixes LIT-3964
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR fixes a regression introduced in v1.89.0 where all
Confidence Score: 3/5Safe to merge for the 404 regression fix, but the access-control path now trusts X-Forwarded-For without validation or a warning whenever mcp_trusted_proxy_ranges is absent, which is worth resolving before this reaches production deployments that expose the proxy directly. The core logic change in get_mcp_client_ip restores pre-v1.89 behavior and fixes a real regression, and the new regression tests are well-constructed. However, the revised code trusts the raw XFF header for access-control decisions (deciding which available_on_public_internet=false servers are visible) with no warning to operators, while the only existing warning about this configuration targets the OAuth URL-building path and says XFF will NOT be trusted — the opposite of what now happens for IP classification. litellm/proxy/auth/ip_address_utils.py — specifically the get_mcp_client_ip function and the relationship between its XFF-trust behavior and the is_request_from_trusted_proxy warning message.
|
| Filename | Overview |
|---|---|
| litellm/proxy/auth/ip_address_utils.py | Reverts the v1.89 fail-closed behavior in get_mcp_client_ip so that XFF is honored whenever use_x_forwarded_for=True; fixes the 404 regression but reintroduces unconditional XFF trust (no spoof defense, no warning) when mcp_trusted_proxy_ranges is absent |
| tests/test_litellm/proxy/auth/test_mcp_ip_filtering.py | Two existing tests updated to reflect the restored XFF-trusting behavior (assertions changed from "" to the actual forwarded IPs); new TestInternalOnlyServerResolutionRegression class added covering the full get_mcp_client_ip → get_mcp_server_by_name path for both internal and external forwarded clients |
Comments Outside Diff (2)
-
litellm/proxy/auth/ip_address_utils.py, line 147-156 (link)Warning message is misleading for access-control callers
The existing one-shot warning says "X-Forwarded-Host/Proto will NOT be trusted when building MCP OAuth discovery URLs." This is accurate for OAuth URL construction, but it is the opposite of what happens in
get_mcp_client_ipunder the same configuration: theX-Forwarded-Forheader IS trusted (and used for access control) when no trusted ranges are configured. An operator who sees this log line could reasonably conclude that XFF is completely ignored, while in reality it is deciding whichavailable_on_public_internet=falseservers their callers can reach. The warning should explicitly state thatX-Forwarded-Forwill still be used for IP-based access control in this configuration. -
litellm/proxy/auth/ip_address_utils.py, line 197-206 (link)XFF trusted blindly for access control when no trusted-proxy ranges are set
When
use_x_forwarded_for=Trueandmcp_trusted_proxy_rangesis not configured, the new condition short-circuits and the code falls through to_get_request_ip_address(request, use_x_forwarded_for=True), which reads the rawX-Forwarded-Forheader without any peer validation. An external attacker with direct access to the proxy can sendX-Forwarded-For: 192.168.1.1and be classified as an internal host, bypassing everyavailable_on_public_internet=falserestriction. No warning is emitted on this code path — the_warned_xff_without_trusted_rangeswarning only fires fromis_request_from_trusted_proxy(the OAuth URL-building path), so access-control callers get no indication that XFF is being trusted unconditionally. A one-shot warning analogous to_warned_xff_without_trusted_rangesshould fire here so operators know theiravailable_on_public_internet=falseaccess control is based on an unvalidated header.
Reviews (1): Last reviewed commit: "style: apply black formatting to match C..." | Re-trigger Greptile
… proxy ranges Address Greptile review: when use_x_forwarded_for is enabled but mcp_trusted_proxy_ranges is unset, get_mcp_client_ip honors X-Forwarded-For for IP-based access control (intentional, matching the proxy's wider XFF contract and pre-1.89 behavior). Previously the only related warning fired from the OAuth URL-building path and implied XFF was fully ignored, which is misleading for the access-control path. Add a dedicated one-shot warning so operators know access control rests on an unvalidated header and that mcp_trusted_proxy_ranges is the way to validate the proxy. No change to the trust model.
|
Thanks @greptileai. Both points are about observability of the same configuration; addressed in 3d24ec2 without changing the trust model. On the P1 security note (unconditional XFF trust for access control when On the P2 misleading-message note: the existing warning is now scoped to OAuth URL building ("when building MCP OAuth discovery URLs"), and the new access-control warning explicitly states XFF IS used for access control, so an operator in this config sees both facts rather than concluding XFF is fully ignored. Covered by |
|
Good follow-up. Your additions in commit 3d24ec2 properly separate the warnings and clarify what's happening in each path. On the security stance: Your justification is sound— On the warning improvements: The refactored warnings now properly distinguish OAuth URL building (XFF not trusted without ranges) from access control (XFF is trusted). That removes the misleading implication. The one-shot pattern prevents log spam, and the test coverage ( The regression tests targeting the exact 404 path ( Approved. The observability gaps are closed without changing the underlying trust model, which correctly matches the proxy-wide XFF semantics. |
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 |
|
This would cause a security regression. Closing |
Relevant issues
Fixes LIT-3964 (regression: all internal-only MCP servers return 404 Not Found)
Linear ticket
https://linear.app/litellm-ai/issue/LIT-3964/regression-all-internal-only-mcp-servers-are-returning-404-not-found
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
make 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:
Screenshots / Proof of Fix
Minimal repro config (internal-only server, XFF on, no trusted ranges):
Run it:
Probe the named-server resolution path with and without an X-Forwarded-For header (what a load balancer sends):
Before this fix (v1.89.0 / v1.89.2):
with XFF: 404(internal-only server hidden),without XFF:non-404. After this fix: both are non-404 (the server resolves and the request proceeds to normal auth handling), matching v1.88.0.Type
🐛 Bug Fix
Changes
Root cause
IPAddressUtils.get_mcp_client_ipreturned""("fail closed") whenuse_x_forwarded_for: truewas set butmcp_trusted_proxy_rangeswas not configured. That empty IP madeis_internal_ip("")returnFalse, so everyavailable_on_public_internet=falseserver was classified as external, hidden by_is_server_accessible_from_ip, andget_mcp_server_by_namereturnedNone; the MCP discovery / SSE / streamable-HTTP routes then turned thatNoneinto a 404. The combination of XFF enabled plus no trusted ranges is very common (LiteLLM behind a load balancer), so it broke every internal-only server while looking fine on naive localhost testing (no XFF header meansrequest.client.hostis127.0.0.1).The fail-closed branch was introduced in #28356 (squash
6d6eda8101), shipped in v1.89.0; v1.88.x never contained it, matching the working-vs-broken versions reported.Fix
Honor
X-Forwarded-Forwheneveruse_x_forwarded_foris enabled, so a client behind a reverse proxy is classified by its real (forwarded) IP rather than the proxy's peer address. This matches pre-1.89 behavior and how the rest of the proxy already treatsuse_x_forwarded_for(e.g._check_valid_ip). The spoof defense is kept only whenmcp_trusted_proxy_rangesis configured: an untrusted direct peer still falls back to its literal IP so a forged header cannot grant internal access. OAuth discovery URL building is unaffected; it keeps failing closed viais_request_from_trusted_proxyandget_request_base_url.The two existing unit tests that encoded the fail-closed empty-string behavior as intended were corrected to assert the restored behavior, and a
TestInternalOnlyServerResolutionRegressionclass was added that drives the exact 404 boundary (get_mcp_client_ip->get_mcp_server_by_name) for both a forwarded internal client (now resolves) and a forwarded external client (still hidden). A mutation check confirms all four new/updated assertions fail against the oldreturn ""code and pass against the fix.