fix(mcp): correct misleading no-trusted-proxy warning for XFF access control - #31264
Conversation
Greptile SummaryThis PR corrects the one-shot warning emitted by
Confidence Score: 5/5Safe to merge — only the warning message text changes; all runtime behavior is identical to the base branch. The production change is confined to a single string literal inside a one-shot logging call. No control flow, no data processing, and no auth decisions are touched. The accompanying test resets the relevant global flag, exercises the exact branch under test, and makes assertions that would catch a revert of the message change. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/auth/ip_address_utils.py | Warning message text updated to accurately describe both fail-closed behaviors; no logic changes. |
| tests/test_litellm/proxy/auth/test_mcp_ip_filtering.py | New regression test added that resets the one-shot flag, captures the warning via caplog, asserts the updated phrasing, and validates the actual fail-closed return value of get_mcp_client_ip. |
Reviews (2): Last reviewed commit: "test(mcp): assert the no-trusted-ranges ..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
… relying on StopIteration
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit d2d26ec. Configure here.
257d671
into
litellm_internal_staging
…control (BerriAI#31264) * fix(mcp): correct misleading no-trusted-proxy warning for XFF access control * test(mcp): assert the no-trusted-ranges warning was logged instead of relying on StopIteration
Relevant issues
N/A
Linear ticket
N/A
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 reviewType
🐛 Bug Fix
Changes
IPAddressUtils.is_request_from_trusted_proxyinlitellm/proxy/auth/ip_address_utils.pylogs a one-shot warning whenuse_x_forwarded_foris enabled butmcp_trusted_proxy_rangesis unset. The previous wording claimed that in this state both MCP OAuth discovery URLs and access-control client IPs "will use the proxy's literal request values," which is only half true and misleads operators about the security postureThe method has two consumers that diverge in this exact configuration.
get_request_base_urlinlitellm/proxy/_experimental/mcp_server/oauth_utils.pydoes fall back to the request's literal base URL for OAuth discovery and redirect_uri construction, so that half was accurate.get_mcp_client_ipin the sameip_address_utils.pyfile does the opposite: when XFF is enabled, anx-forwarded-forheader is present, andmcp_trusted_proxy_rangesis unset, it does not return any literal request value. It fails closed and returns"", so the caller is treated as external and only sees servers withavailable_on_public_internet=true. The old message told operators their access-control client IPs would use literal request values, which would have led them to believe internal callers behind an untrusted proxy still resolve to their real IP when they actually get classified as externalThis PR rewords the warning so it describes both behaviors accurately: OAuth discovery URLs fall back to the literal request URL, and access-control client-IP resolution fails closed with callers treated as external. No behavior changes; the message text is the only source change
The added regression test ties the message to the real behavior. It triggers the branch with
use_x_forwarded_foron, anx-forwarded-forheader present, andmcp_trusted_proxy_rangesunset, captures the warning viacaplog, asserts the new fail-closed phrasing is present and the old misleading claim is gone, and asserts in the same scenario thatget_mcp_client_ipreturns"". Reverting the wording makes the assertions fail, and the coupledget_mcp_client_ipassert keeps the message honest about realityScreenshots / Proof of Fix
Ran a local proxy with
use_x_forwarded_for: trueand nomcp_trusted_proxy_ranges, then hit an MCP REST route that resolves the client IP with anX-Forwarded-Forheader. The corrected warning shows up in the proxy logStart (config has
general_settings.use_x_forwarded_for: true, anmcp_serversblock, and crucially nomcp_trusted_proxy_ranges):Trigger:
Corrected warning grep'd from the log:
Note
Low Risk
Log message and test-only change; MCP IP trust and access-control behavior are unchanged.
Overview
Updates the one-shot proxy log warning when
use_x_forwarded_foris on butmcp_trusted_proxy_rangesis unset. The old text implied MCP access-control client IPs would use the proxy’s literal request values; that was wrong forget_mcp_client_ip, which already fails closed ("", callers treated as external) while OAuth discovery can still fall back to the literal request URL.The new wording states both outcomes explicitly and still points operators at
mcp_trusted_proxy_ranges. No runtime behavior changes.Adds
test_no_trusted_ranges_warning_matches_fail_closed_behavior, which captures the warning viacaplog, checks for the fail-closed phrasing and absence of the old line, and assertsget_mcp_client_ipreturns""in the same setup.Reviewed by Cursor Bugbot for commit d2d26ec. Bugbot is set up for automated code reviews on this repo. Configure here.