Repository navigation
fix(guardrails): route custom-code http_request() through SSRF validation - #32870
devin-ai-integration[bot] wants to merge 2 commits into
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
|
Greptile SummaryThis PR adds a method-generic
Confidence Score: 4/5Safe to merge with awareness of one behavioral change in redirect handling. The core security hardening is correctly implemented and well-tested. The one finding is that the redirect loop always preserves the original HTTP method for 301/302/303 responses, where httpx built-in redirect logic would have changed POST to GET. This is a functional deviation from RFC 7231 and from the old code behavior, but it is unlikely to affect most callers and does not weaken the protection. litellm/litellm_core_utils/url_utils.py — the async_safe_request redirect loop (lines 458-463) does not downgrade POST to GET on 301/302/303 responses.
|
| Filename | Overview |
|---|---|
| litellm/litellm_core_utils/url_utils.py | Adds async_safe_request, a method-generic sibling of async_safe_get with identical SSRF protections; correctly adds or {} guard for None headers not present in async_safe_get; redirect loop always preserves the original HTTP method even for 301/302/303 (deviation from RFC 7231 POST to GET semantics) |
| litellm/proxy/guardrails/guardrail_hooks/custom_code/primitives.py | Routes http_request() through async_safe_request instead of the removed _execute_http_request helper; adds SSRFError catch; correctly accesses client.client for per-hop redirect control; HTTPStatusError branch cleanly removed since the raw client never calls raise_for_status() |
| tests/test_litellm/litellm_core_utils/test_url_utils.py | New TestAsyncSafeRequest class covers private IP, cloud-metadata, per-hop redirect validation, and master-switch bypass via monkeypatched DNS and fake clients with no real network calls |
| tests/test_litellm/proxy/guardrails/test_custom_code_primitives.py | End-to-end tests for http_request() cover cloud-metadata blocking, private IP blocking, redirect-to-private blocking, and happy-path validation with Host header; all tests are fully mocked |
Reviews (1): Last reviewed commit: "fix(guardrails): route custom-code http_..." | Re-trigger Greptile
| if not response.is_redirect: | ||
| return response | ||
| # Resolve the next hop against the ORIGINAL (pre-rewrite) URL so | ||
| # relative Location headers keep the original hostname. | ||
| url = _extract_redirect_url(response, url) | ||
| raise SSRFError("Too many redirects") |
There was a problem hiding this comment.
Non-GET redirects always preserve the original method
async_safe_request uses the original method on every hop in the redirect loop. For 301/302/303 responses, RFC 7231 mandates that POST (and other unsafe methods) be reissued as GET to the redirect target; only 307/308 should preserve the method. The old code delegated to httpx with follow_redirects=True, which handled this automatically. Callers that POST to a URL returning a 301/302/303 will now send POST to the redirect target instead of GET, which can result in 405 errors or unintended double-POSTs against external APIs. Consider adding if response.status_code in (301, 302, 303) and method != "GET": method = "GET"; kwargs.pop("json", None); kwargs.pop("data", None) before advancing url.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
ErenAta16
left a comment
There was a problem hiding this comment.
Same underlying fix as #32897 (different issue number, #32862 here vs #32889 there) and #32866 — flagging the duplication, left the fuller comparison on #32897 since I worked through all three together.
Specifically on this one: it shares #32897's design (raw httpx.AsyncClient passed through client.client, client.request(...) dispatch) but does not carry over #32897's cross-origin credential-header stripping on redirect — a validated redirect to a different origin here would still forward the original Authorization/Cookie headers unchanged. Also worth noting this PR removes the old except httpx.HTTPStatusError branch that returned the response even for non-2xx status — did not verify whether anything downstream relied on that behavior for a specific status code, so flagging rather than asserting it's wrong.
|
Closing as superseded by #43280, which routes http_request through the shared SSRF validator with per-hop redirect checks and adds an execution timeout. |
Relevant issues
Fixes #32862
Linear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Screenshots / Proof of Fix
Reproduced end to end against a live proxy on
localhost:4000. Setup is a loopback "internal" service standing in for a cloud metadata / internal endpoint, and a custom-code guardrail (mode: pre_call) whoseapply_guardrailcallshttp_request()at that internal URL and blocks with the fetched result, so whether the SSRF succeeded is visible in the chat responseInternal service (returns a fake secret if reached):
Guardrail in the proxy config:
Before the fix (vulnerable code at
109193f26a) the guardrail reaches the internal service and exfiltrates its response; the internal service hit counter goes from 1 to 2:After the fix (this branch at
d9a7666cb1) the same request is stopped byvalidate_url()insideasync_safe_request, no outbound request is made, and the internal service hit counter stays put:The same block applies to the cloud metadata address from the report (
http://169.254.169.254/latest/meta-data/iam/security-credentials/) and to every verbhttp_request()supports, which the added unit tests cover directlyType
🐛 Bug Fix
Changes
The
http_request()primitive exposed to custom-code guardrails (litellm/proxy/guardrails/guardrail_hooks/custom_code/primitives.py) only ran the target throughis_valid_url(), which is a pureurlparsesyntax check. It never resolved DNS, never consulted the private/cloud-metadata blocklist, and let the underlying client follow redirects on its own (the cachedAsyncHTTPHandleris created withfollow_redirects=True). That let guardrail code reach169.254.169.254, internal services, and IAM credential endpoints, i.e. full SSRF through the guardrail HTTP interfaceThe existing SSRF-safe helper
async_safe_getinurl_utils.pyonly covers GET, buthttp_request()also supports POST/PUT/DELETE/PATCH, so there was no drop-in for the other verbs. This adds a method-generic siblingasync_safe_request(client, method, url, ...)that reuses the exact same protectionasync_safe_getalready applies (resolve-and-rewrite to the validated IP, per-hop redirect validation, Host header preservation, and thelitellm.user_url_validationmaster switch). It operates on the rawhttpx.AsyncClientbecause the per-hopfollow_redirects=Falsecontrol isn't available on the higher-level handler's non-GET methodsBecause the raw client no longer calls
raise_for_status(), the previoushttpx.HTTPStatusErrorbranch is gone; non-2xx responses now flow straight into_http_success_response(which already setssuccess=Falsefor them), and the now-unused_execute_http_requesthelper is removedBehavior for legitimate public URLs is unchanged: HTTPS with SSL verification keeps its hostname (TLS cert binding defeats rebinding), HTTP is rewritten to the validated IP with the original hostname carried in the
Hostheader, and admins can still opt out or allowlist internal hosts viauser_url_validation/user_url_allowed_hostsTests cover the new helper directly (blocks private and cloud-metadata IPs for non-GET verbs, validates every redirect hop, honors the master switch) and the primitive end to end (metadata IP blocked with zero outbound requests, a public host 302-redirecting to a private target blocked at the redirect hop, and the happy path still issuing the validated request to the resolved IP with the original Host header)
Link to Devin session: https://app.devin.ai/sessions/047926398eca4c999369b5f87d67cbae