fix(mcp): strip scheme default port from get_request_base_url netloc - #32921
Conversation
Greptile SummaryThis PR fixes
Confidence Score: 5/5Safe to merge — the change is a one-line, well-scoped addition of an already-tested helper to the URL emission path, with five new mock test cases that would have caught the regression on the unfixed base. The fix correctly applies an existing normalisation helper (_strip_default_port) to the construction path that previously skipped it, making emission consistent with the validation path that already called the same helper. The five new cases directly exercise the four stripping scenarios and one preservation scenario described in the PR description. No production logic is modified beyond the single call site. No files require special attention — both changed files are straightforward and well-covered by the new test cases.
|
| Filename | Overview |
|---|---|
| litellm/proxy/_experimental/mcp_server/oauth_utils.py | Single-line fix: applies the existing _strip_default_port helper to the netloc in get_request_base_url so that scheme-default ports (:443 for HTTPS, :80 for HTTP) are omitted from emitted URLs, consistent with how the validation path already normalized netlocs. |
| tests/test_litellm/proxy/_experimental/mcp_server/test_discoverable_endpoints.py | Five new parametrized cases added to test_get_request_base_url_comprehensive: stripping :443 via X-Forwarded-Port, stripping :80 for HTTP, stripping :443 when no X-Forwarded-Host is set, preserving non-default :8443, and stripping :443 already embedded in X-Forwarded-Host. All cases are mock-only (no real network calls). |
Reviews (1): Last reviewed commit: "fix(mcp): strip scheme default port from..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Relevant issues
Supersedes #31771
Linear ticket
Resolves LIT-4126
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)CI note: 126 checks pass; the single red job is
ci/circleci: batches_testing, which currently fails identically on every open PR againstlitellm_internal_staging(checked 8 open PRs), so it is unrelated to this changeDelays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
Rig: local proxy on port 4126 with
general_settings.use_x_forwarded_for: trueandmcp_trusted_proxy_ranges: ["127.0.0.0/8"], oneoauth2/authorization_codeMCP server whoseauthorization_url/token_url/registration_urlpoint at a stub IdP on127.0.0.1:9310,PROXY_BASE_URLunset so the header reconstruction path is exercised. The load balancer is simulated by sendingX-Forwarded-Proto: https,X-Forwarded-Host: litellm.example.com,X-Forwarded-Port: 443from a trusted range; ingresses set that port header on every TLS request, which is exactly the customer topology that breaksBefore, on this branch's base (
litellm_internal_staging80c5217), the port leaks into every emitted URL:The second command is the customer visible failure: the
redirect_urihanded to the upstream IdP ishttps://litellm.example.com:443/callback, and providers that store the registered URI canonically and compare per RFC 6749 3.1.2.3 simple string comparison reject the flow with an invalid client or mismatched redirect_uri errorAfter, with this PR's commit, the same curls emit canonical URLs:
Edge cases on the fixed proxy:
X-Forwarded-Port: 8443stayshttps://litellm.example.com:8443;X-Forwarded-Proto: httpwith port 80 becomeshttp://litellm.example.com; a port embedded directly inX-Forwarded-Host: litellm.example.com:443is also stripped; with no forwarding headers the local basehttp://localhost:4126is returned unchangedType
Bug Fix
Changes
get_request_base_urlinoauth_utils.pyrebuilds the public base URL fromX-Forwarded-*headers sent by a trusted load balancer and appendsX-Forwarded-Portto the netloc without eliding the scheme default port, soX-Forwarded-Port: 443(which ingresses send routinely) yieldshttps://host:443. RFC 3986 6.2.3 treats an explicit default port as equivalent to none, and producers should omit it per 3.2.3. The module already has_strip_default_portand already applies it on the comparison side (validate_trusted_redirect_urinormalizes both netlocs before the same origin check), so validation tolerated:443while emission leaked it; this change applies the same helper to the returned netloc, making emission consistent with validation. Every emitter funnels through this function (authorizeredirect_uri, token exchangeredirect_uri, DCRredirect_uris, protected resource and authorization server metadata, WWW-Authenticate challenge URLs, the MCP registry), so the single call site covers them allFive cases added to
test_get_request_base_url_comprehensive::443withX-Forwarded-Hoststripped,:80with http stripped,:443on a portless base withoutX-Forwarded-Hoststripped, non default:8443preserved, and:443embedded directly inX-Forwarded-Hoststripped. On the unfixed base the four stripping cases fail and the preservation case passes; with the fix the full mapped file is 135 passedSupersedes #31771, which contains the same one line fix; reopened here to land it under an internal branch with the live proof of fix attached and one extra regression case (
X-Forwarded-Hostcarrying the port itself). Operators who cannot upgrade can setPROXY_BASE_URLto their canonical origin as a workaround; it takes priority over header reconstruction