fix(security): join repeated X-Forwarded-For lines before taking the last hop - #14425
Conversation
…last hop
Both client-IP resolvers read the forwarded chain with `Headers.get`, which
returns only the first matching header line. Proxies that append their own
`X-Forwarded-For` line rather than extending the client's (HAProxy's
`option forwardfor` among them) therefore leave the caller's line as the one
we parse, so the "rightmost entry is the trusted proxy's last hop" invariant
resolves to an attacker-chosen value.
Under `rate_limit_trust_proxy=True` this lets a remote caller send
`X-Forwarded-For: 127.0.0.1` and pass the local-only gate on
`POST /api/v1/mcp/project/{id}/install`, which writes MCP client config to the
host filesystem - the same bypass GHSA-4f6c-2vvp-gw82 reported, reachable
again through a differently-shaped header. It also lets a caller pin or rotate
their own rate-limit bucket on login and public-flow endpoints.
Join every occurrence in order before taking the last hop, and drop empty
entries so a blank header falls back to the TCP peer instead of resolving to
an empty client IP. Default deployments (`rate_limit_trust_proxy=False`) never
read the header and were not affected.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
WalkthroughThe change adds shared parsing for repeated ChangesForwarded IP Resolution
Estimated code review effort: 2 (Simple) | ~15 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 8 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (8 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Test Coverage AdvisorNo source changes detected without accompanying tests. Thanks for keeping coverage up! 🎉
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-1.11.3 #14425 +/- ##
==================================================
+ Coverage 61.40% 61.60% +0.19%
==================================================
Files 2398 2398
Lines 238302 238306 +4
Branches 35840 33784 -2056
==================================================
+ Hits 146336 146810 +474
+ Misses 90158 89688 -470
Partials 1808 1808
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Context
While confirming GHSA-4f6c-2vvp-gw82 / GHSA-qvvj-g573-9638 (MCP installer trusting client-supplied
X-Forwarded-For) againstrelease-1.11.3, I found the reported bypass is already fixed —1641b28f33(#13530, 2026-07-13) is contained in v1.11.0/v1.11.1/v1.11.2, and release-1.10.3 has it via the separate backport94859df33a(#14071). The advisory PoC no longer reproduces.This PR fixes a residual variant of the same bypass that is still reachable on
release-1.11.3.The bug
Both client-IP resolvers read the forwarded chain with
Headers.get("X-Forwarded-For"):langflow.api.v1.mcp_projects.get_client_iplangflow.services.rate_limit.service.get_client_ipStarlette's
Headers.getreturns only the first matching header line; it does not join repeated occurrences the way uvicorn does. Proxies that append their ownX-Forwarded-Forline rather than extending the client's — HAProxy'soption forwardfordoes this by default — therefore leave the caller's line as the one we parse. The "rightmost entry is the trusted proxy's last hop, which a client cannot forge" invariant then resolves to an attacker-chosen value.Concretely, with
rate_limit_trust_proxy=True:Headers.getreturns"127.0.0.1", its rightmost entry is127.0.0.1, andis_local_ippasses — so a remote caller clears the local-only gate onPOST /api/v1/mcp/project/{project_id}/install, which writes MCP client config (~/.cursor/mcp.jsonand friends) to the host filesystem. That is the impact GHSA-4f6c-2vvp-gw82 describes, reached through a differently-shaped header.The same parsing feeds rate-limit bucket keys for login, public build, and public workflow endpoints, so a caller can also pin or rotate their own bucket.
nginx's
$proxy_add_x_forwarded_forproduces a single joined line and was never affected.Scope
Default deployments are not affected.
rate_limit_trust_proxydefaults toFalse, and neither resolver reads the header in that mode;forwarded_allow_ips=""additionally stops uvicorn'sProxyHeadersMiddlewarefrom rewritingrequest.client. Exposure requires the operator to have opted into a trusted proxy, which is why this is materially lower severity than the original advisory.The fix
One shared
get_last_forwarded_for_hophelper that joins every occurrence of the header in order before taking the last hop, and drops empty entries so a blank header falls back to the TCP peer instead of resolving to an empty client IP. Both call sites use it; the trusted-proxy gate inmcp_projectsis unchanged.Test plan
src/backend/tests/unit/api/v1/test_mcp_projects.py— 55 passed.test_mcp_install_xff_trust.py+test_login_rate_limiting.py+test_rate_limit_bypass_prevention.py— 28 passed.ruff check/ruff formatclean; pre-commit hooks pass.The header mocks in
test_login_rate_limiting.pywere plain dicts, which cannot express a repeated header and have no.getlist; they are now realstarlette.datastructures.Headers, matching what the production code actually receives.Not addressed here
Behind a same-host reverse proxy with
rate_limit_trust_proxy=False,request.client.hostis127.0.0.1for every remote user, so the local-only gate is satisfied with no spoofing at all. A TCP-peer check cannot express "this caller is on the server machine" in a proxied deployment. Ifinstall_mcp_configis meant to be a real trust boundary rather than a convenience guard, it needs a different control (an explicit setting, or a local-only bind/socket). That is a design change rather than a patch, so it is deliberately out of scope.Summary by CodeRabbit
Bug Fixes
Tests