fix(mcp): warn loudly when X-Forwarded-For is present but use_x_forwarded_for is off - #31266
Conversation
…rded_for is off When a request carries an X-Forwarded-For header but use_x_forwarded_for is unset, get_mcp_client_ip silently falls back to the direct peer's IP (the load balancer / reverse proxy). That peer almost always sits inside mcp_internal_ip_ranges, so the 'Internal network only' (available_on_public_internet: false) restriction trusts every external caller as internal and effectively exposes those servers. Emit a one-shot loud error pointing the operator at use_x_forwarded_for instead of hard-failing: on a deployment with no load balancer, a crafted X-Forwarded-For header must not be able to take the service down, and a one-shot log keeps a flood of crafted headers from spamming the logs.
Greptile SummaryAdds a one-shot
Confidence Score: 5/5Safe to merge — the change only adds a one-shot log statement and leaves all existing request-handling behavior untouched. The change is entirely additive: a module-level boolean flag and a conditional log call that never alters the IP returned to callers. The fallback path (direct peer IP) is identical to pre-PR behavior. The re-arming logic is simple and covered by a dedicated regression test. No authentication or access-control decisions are changed. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/auth/ip_address_utils.py | Adds _warned_xff_present_but_disabled module-level flag with one-shot error logging when XFF header is present but use_x_forwarded_for is disabled; flag correctly re-arms when XFF is later observed as enabled. |
| tests/test_litellm/proxy/auth/test_mcp_ip_filtering.py | Adds TestXffPresentButDisabledWarning class with five focused tests: warns-and-continues, one-shot suppression, re-arms after enabled→disabled cycle, no warning without header, and no warning when XFF is properly enabled. All mocked; no real network calls. |
Reviews (2): Last reviewed commit: "fix(mcp): re-arm XFF-disabled warning on..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…t assertion Address PR review: tie the one-shot warning flag to the observed use_x_forwarded_for value so it re-arms whenever the setting is seen enabled, restoring the diagnostic on a later rollback to disabled. Also assert against str(call_args) so the test survives a positional-to-keyword logger refactor.
…rded_for is off (BerriAI#31266) * fix(mcp): warn loudly when X-Forwarded-For is present but use_x_forwarded_for is off When a request carries an X-Forwarded-For header but use_x_forwarded_for is unset, get_mcp_client_ip silently falls back to the direct peer's IP (the load balancer / reverse proxy). That peer almost always sits inside mcp_internal_ip_ranges, so the 'Internal network only' (available_on_public_internet: false) restriction trusts every external caller as internal and effectively exposes those servers. Emit a one-shot loud error pointing the operator at use_x_forwarded_for instead of hard-failing: on a deployment with no load balancer, a crafted X-Forwarded-For header must not be able to take the service down, and a one-shot log keeps a flood of crafted headers from spamming the logs. * fix(mcp): re-arm XFF-disabled warning on config change and harden test assertion Address PR review: tie the one-shot warning flag to the observed use_x_forwarded_for value so it re-arms whenever the setting is seen enabled, restoring the diagnostic on a later rollback to disabled. Also assert against str(call_args) so the test survives a positional-to-keyword logger refactor.
Relevant issues
Linear ticket
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
Run a proxy with an internal-only MCP server and
use_x_forwarded_forleft off, then send a request carrying anX-Forwarded-Forheader and watch the logs.litellm/proxy/dev_config.yaml(minimal):Expected in
litellm.log(fires once, request still served):Type
🐛 Bug Fix
Changes
When a request carries an
X-Forwarded-Forheader butuse_x_forwarded_foris unset,IPAddressUtils.get_mcp_client_ipsilently falls back to the direct peer's IP (the load balancer / reverse proxy). That peer almost always sits insidegeneral_settings.mcp_internal_ip_ranges, so the "Internal network only" (available_on_public_internet: false) restriction trusts every external caller as internal and effectively exposes those servers. Operators got no signal that their internal-only filtering was a no-op.This emits a one-shot loud error pointing the operator at
use_x_forwarded_for(andmcp_trusted_proxy_ranges) instead of hard-failing. Hard-failing would be a foot-gun on deployments with no load balancer: a malicious user could craft anX-Forwarded-Forheader to take the service down. Keeping it to a single log line also prevents a flood of crafted headers from spamming the logs, which would itself be a DoS vector.Added regression tests in
tests/test_litellm/proxy/auth/test_mcp_ip_filtering.pycovering: the warning fires and the request is still served (falls back to the direct peer), the warning is one-shot, no warning is emitted without the header, and no warning is emitted when XFF is properly enabled.