fix: backport #27878 to litellm_1.84.0rc2 - #27904
Conversation
* fix: patch Host-header auth bypass in get_request_route Starlette reconstructs request.url from the Host header. A malformed Host like `localhost/?x=1` causes Starlette to build the full URL as `http://localhost/?x=1/health`, which url-parses to path="/". Since "/" is in LiteLLMRoutes.public_routes, all protected routes became reachable without authentication. Fix: read scope["path"] (set by uvicorn from the HTTP request line, not derivable from headers) instead of request.url.path. Sub-path deployments are handled via scope["app_root_path"] / scope["root_path"], mirroring Starlette's own base_url construction logic. Affected variants confirmed fixed: Host: localhost/?x=1 Host: localhost:4000/?x=1 Host: localhost/#test Host: localhost:4000/#test Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> * style: reduce comments in route fix Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> * fix: block credential fields in RAG ingest vector_store options Credential fields (vertex_credentials, aws_access_key_id, api_key, etc.) in ingest_options.vector_store are now rejected at the API boundary with a 400 error. Credentials must be configured server-side. Previously any authenticated user could supply a vertex_credentials dict with type=external_account pointing credential_source.file at an arbitrary path (e.g. /proc/1/environ) and token_url at an attacker-controlled server. google-auth's identity_pool.Credentials refresh() would read the file and POST its contents to the attacker. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> * fix: block /key/update self-escalation by assigned users Non-admin users who were assigned a key (created_by != caller) could update any non-budget field — models, rpm_limit, guardrails, etc. — without admin authorization, allowing privilege self-escalation. Gate: only the key creator (created_by == caller) may edit their own key without admin check; budget changes always require admin regardless of creator status. All other callers must pass _check_key_admin_access. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> * fix: block user-controlled api_base in RAG ingest vector_store options A user-supplied api_base in ingest_options.vector_store caused the server to forward its configured provider credentials (Gemini, OpenAI) to an attacker-controlled endpoint via SSRF. Add api_base to the blocked credential params set alongside api_key and the existing credential fields. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> * fix: restrict /utils/transform_request to PROXY_ADMIN and apply body safety check Any authenticated internal_user could POST arbitrary provider config (aws_sts_endpoint, api_base, etc.) to /utils/transform_request and have the server forward its credentials to an attacker-controlled endpoint. - Gate the endpoint on PROXY_ADMIN role (403 for all other roles) - Call is_request_body_safe() to reject banned params even for admins - Convert ValueError from safety check to HTTP 400 Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> * fix: apply banned-param check to /utils/transform_request Without is_request_body_safe(), any authenticated user could pass aws_sts_endpoint, api_base, or aws_web_identity_token to /utils/transform_request and have the server forward its configured provider credentials to an attacker-controlled endpoint during SDK credential resolution. Applies the same banned-param blocklist already used by LLM endpoints. Endpoint remains accessible to all authenticated users. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> * fix: block SSRF via api_base in /prompts/test dotprompt YAML frontmatter Any frontmatter key not in ["model","input","output"] flowed into optional_params and was merged into the LLM call data dict, bypassing is_request_body_safe. An attacker with any bearer key could set api_base in YAML to redirect the outbound LLM request — including the provider API key — to an attacker-controlled host. Fix: call is_request_body_safe on the constructed data dict after optional_params are merged, before invoking ProxyBaseLLMRequestProcessing. ValueError from the banned-param check is surfaced as HTTP 400. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> * Update litellm/proxy/rag_endpoints/endpoints.py Co-authored-by: veria-ai[bot] <224490171+veria-ai[bot]@users.noreply.github.com> * fix: coerce nested config strings before banned-param check _NESTED_CONFIG_KEYS descent used isinstance(nested, dict) which silently skipped litellm_embedding_config when delivered as a JSON string via multipart/form-data. Banned params (api_base, aws_sts_endpoint, etc.) nested inside the stringified value were invisible to is_request_body_safe. _NESTED_METADATA_KEYS already used _coerce_metadata_to_dict which parses JSON strings before checking. Apply the same coercion to _NESTED_CONFIG_KEYS. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> * fix: replace substring match with prefix match in is_llm_api_route mapped_pass_through_routes used `_llm_passthrough_route in route` (substring) so any admin-only path whose URL contained a provider name (openai, anthropic, azure, bedrock, etc.) was misclassified as an LLM API route and bypassed the admin gate in non_proxy_admin_allowed_routes_check. Confirmed live: non-admin key could GET /credentials/by_name/openai (read masked provider API key) and DELETE /credentials/openai (delete credential). Fix: use exact match or startswith(prefix + "/") — the same pattern used everywhere else in RouteChecks — so only routes that actually start with a passthrough prefix are allowed through. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> * fix: stabilize PR #27878 test failures - key_management_endpoints: extend can_skip_admin_check to team keys so team members with /key/update permission can update non-budget fields. can_team_member_execute_key_management_endpoint already validates team membership + permission and raises if unauthorized; reaching the admin check on a team key means the caller was authorized. - test: set created_by on mock key in test_update_key_non_budget_fields_allowed_for_internal_user so caller_is_creator resolves correctly (MagicMock default ≠ user_id). - auth_utils.get_request_route: guard against non-dict request.scope (e.g. MagicMock in unit tests) to prevent a MagicMock leaking into UserAPIKeyAuth.request_route and failing Pydantic validation. - ci: assign test_multipart_bypass_repro.py to the proxy-runtime shard in test-unit-proxy-db.yml to satisfy the shard-coverage check. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> * fix(lint): add explicit str() cast in get_request_route for MyPy scope.get() returns Any|None which MyPy cannot coerce to str implicitly. Wrap both scope.get() calls in str() to satisfy the type checker. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> * fix: guard bare-/ root_path strip + make total_spend migration idempotent auth_utils.get_request_route: when Starlette sets scope["app_root_path"] to "/" (e.g. behind some middleware), the old stripping logic would remove the leading slash from every path ("/team/new" → "team/new"), breaking route matching and causing auth to misclassify protected routes. Skip stripping when root_path is bare "/". migration: add IF NOT EXISTS to total_spend ALTER TABLE so the migration is safe to replay when a prior partial run already created the column. Without this guard, prisma migrate deploy fails on CI DBs that were partially migrated, causing all subsequent DB operations (including /team/new) to 500. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> * fix: require creator still owns key for personal-key bypass in /key/update caller_is_creator now requires both created_by == caller AND user_id == caller. Previously checking only created_by let a demoted admin who originally created a key for another user continue editing non-budget fields on it after reassignment, bypassing _check_key_admin_access. Adds regression test: creator whose key was reassigned is blocked (403). Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> * fix: extract auth checks to fix PLR0915 + broaden max_budget assertion internal_user_endpoints._update_single_user_helper exceeded 50 statements (PLR0915). Extract authorization checks into _check_user_update_authz helper to bring statement count under the limit. test_validate_max_budget: assert "negative" (substring of both the local "cannot be negative" and the CI "non-negative finite number" messages) so the test is stable regardless of which exact wording the function uses. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: veria-ai[bot] <224490171+veria-ai[bot]@users.noreply.github.com>
|
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…/27878-litellm_1.84.0rc2 # Conflicts: # tests/test_litellm/proxy/rag_endpoints/test_rag_endpoints.py
Greptile SummaryThis backport of #27878 onto
Confidence Score: 4/5Safe to merge with minor caveats — the security fixes are correct and well-tested; only the All five SSRF/credential-exfil vectors are correctly closed and each has a matching regression test. The
|
| Filename | Overview |
|---|---|
| litellm/proxy/auth/auth_utils.py | Rewrites get_request_route to read directly from ASGI scope instead of request.base_url; contains one dead isinstance guard after a str() cast |
| litellm/proxy/auth/route_checks.py | Fixes CVE-class route-bypass: changes pass-through route matching from substring in to exact/prefix check, preventing provider-name-in-path admin-gate bypass |
| litellm/proxy/management_endpoints/key_management_endpoints.py | Tightens key-update auth by requiring both created_by and user_id to match the caller; keys with NULL created_by lose the self-service bypass silently |
| litellm/proxy/management_endpoints/internal_user_endpoints.py | Extracts /user/update authorization logic into _check_user_update_authz; logic is equivalent to what was inline before, now applied after the DB lookup |
| litellm/proxy/rag_endpoints/endpoints.py | Adds block-list for client-supplied credential fields in ingest_options.vector_store, closing an SSRF / credential-exfiltration vector via google-auth identity_pool |
| litellm/proxy/proxy_server.py | Adds is_request_body_safe guard to /utils/transform_request, preventing banned params from reaching SDK credential resolution |
| litellm/proxy/prompts/prompt_endpoints.py | Adds is_request_body_safe check to /prompts/test, blocking SSRF via dotprompt YAML api_base frontmatter |
| litellm-proxy-extras/litellm_proxy_extras/migrations/20260421135425_add_team_membership_total_spend/migration.sql | Adds IF NOT EXISTS to the ALTER TABLE for total_spend, making the migration idempotent |
| tests/proxy_unit_tests/test_multipart_bypass_repro.py | New unit tests verifying that banned params embedded inside a JSON-string litellm_embedding_config are caught by is_request_body_safe |
| tests/test_litellm/proxy/management_endpoints/test_key_management_endpoints.py | Adds comprehensive privilege-escalation tests; one existing assertion weakened from exact error string to "negative" without a matching implementation change |
| tests/test_litellm/proxy/auth/test_route_checks.py | New parametrized tests covering the route-bypass fix for both the attack paths and the legitimate pass-through routes |
| tests/test_litellm/proxy/rag_endpoints/test_rag_endpoints.py | Adds parametrized test confirming all blocked credential fields in ingest_options.vector_store return HTTP 400 |
Reviews (1): Last reviewed commit: "Merge remote-tracking branch 'origin/lit..." | Re-trigger Greptile
|
|
||
| assert exc_info.value.status_code == 400 | ||
| assert "max_budget cannot be negative" in str(exc_info.value.detail) | ||
| assert "negative" in str(exc_info.value.detail) |
There was a problem hiding this comment.
Weakened test assertion without matching implementation change
The assertion was loosened from "max_budget cannot be negative" to "negative", but _validate_max_budget still emits the full message "max_budget cannot be negative. Received: {value}". The stronger assertion would still pass, so the relaxation serves no purpose and reduces regression protection — any error containing "negative" (e.g., a new "soft_budget cannot be negative" path leaking through) would incorrectly satisfy this check.
Rule Used: What: Flag any modifications to existing tests and... (source)
| if not isinstance(scope, dict): | ||
| return str(request.url.path) | ||
| raw_path: str = str(scope.get("path", request.url.path)) | ||
| root_path: str = str(scope.get("app_root_path", scope.get("root_path", ""))) | ||
| if not isinstance(raw_path, str): |
There was a problem hiding this comment.
Dead type-guard after
str() conversion
raw_path is assigned with str(scope.get(...)), so it is always a str. The subsequent if not isinstance(raw_path, str) guard is unreachable and misleads readers into thinking the value could be a non-string at that point.
| if not isinstance(scope, dict): | |
| return str(request.url.path) | |
| raw_path: str = str(scope.get("path", request.url.path)) | |
| root_path: str = str(scope.get("app_root_path", scope.get("root_path", ""))) | |
| if not isinstance(raw_path, str): | |
| raw_path: str = str(scope.get("path", request.url.path)) | |
| root_path: str = str(scope.get("app_root_path", scope.get("root_path", ""))) | |
| # Only strip root_path when it is a meaningful prefix (not bare "/"). |
| caller_is_creator = ( | ||
| user_api_key_dict.user_id is not None | ||
| and getattr(existing_key_row, "created_by", None) == user_api_key_dict.user_id | ||
| and getattr(existing_key_row, "user_id", None) == user_api_key_dict.user_id | ||
| ) | ||
| # Team keys: can_team_member_execute_key_management_endpoint (called above) | ||
| # already validated team membership + /key/update permission and would have | ||
| # raised if the caller lacked it. Reaching this point on a team key for a | ||
| # non-budget change means the caller was authorized — skip the redundant | ||
| # _check_key_admin_access that would otherwise require team/org admin status. | ||
| _key_is_team_key = getattr(existing_key_row, "team_id", None) is not None | ||
| can_skip_admin_check = ( | ||
| caller_is_creator or _key_is_team_key | ||
| ) and not _is_budget_change |
There was a problem hiding this comment.
caller_is_creator silently fails when created_by is NULL on legacy keys
The new bypass condition requires both created_by == caller and user_id == caller. Keys stored with created_by = NULL (possible for keys created before the field was reliably populated, or created via a master-key path where user_api_key_dict.user_id is None) will never satisfy this condition. A user whose user_id is on the key but whose created_by is NULL will now be directed to _check_key_admin_access instead of the former is_key_owner shortcut, potentially losing self-service update capability on their own personal keys. Consider adding a fallback: treat created_by = NULL AND user_id == caller the same as the creator case to preserve backwards compatibility for legacy keys.
* chore(proxy): cherry-pick #28547 onto patch/v1.84.1 Backport of #28547 (`d480ffda3c`) onto the `patch/v1.84.1` branch. Routes the remaining path-dependent call sites in auth, ACL, routing, and audit-log decisions through `get_request_route(request)` so they read from the ASGI `scope["path"]` instead of `request.url.path`. The helper itself already exists on v1.84.1 (added by #27904 / #27878); this PR extends the helper's usage to the additional sites listed below. Sites routed through get_request_route: - _experimental/mcp_server/auth/user_api_key_auth_mcp.py - management_endpoints/mcp_management_endpoints.py - vector_store_endpoints/utils.py - pass_through_endpoints/pass_through_endpoints.py - auth/route_checks.py - litellm_pre_call_utils.py - spend_tracking/spend_management_endpoints.py - common_utils/http_parsing_utils.py - management_helpers/utils.py - health_endpoints/_health_endpoints.py Regression tests in tests/proxy_unit_tests/test_proxy_routes.py construct a Request with scope["path"] set to a benign route and the Host header crafted so url.path would resolve differently; each site's decision is asserted against scope["path"]. Conflict resolution ------------------- Two files conflicted because v1.84.1's base predates the delegate_auth_to_upstream feature (#27834 — not on v1.84.1): 1. _experimental/mcp_server/auth/user_api_key_auth_mcp.py The cherry-pick brought in a `_target_servers_delegate_auth_to_upstream` elif branch in `process_mcp_request`. That branch is feature drift from #27834 and is irrelevant to the path-resolution change. Dropped the elif block; kept the get_request_route swap on the existing well-known/_target_servers_use_oauth2 call sites. 2. management_endpoints/mcp_management_endpoints.py The cherry-pick brought in the entire `_mcp_oauth_user_api_key_auth` function. That function does not exist on v1.84.1 (added by #27834); the #28547 change inside it is just a `request.url.path` → `get_request_route` swap. Dropped the function entirely. The other 8 production files and the test file auto-merged cleanly and contain only `request.url.path` → `get_request_route(request)` swaps plus the lazy auth_utils import (no feature drift). * bump: version 1.84.1 → 1.84.2 * chore: uv lock after version bump 1.84.1 → 1.84.2
Backport of #27878 (squash commit
8bbc61e03c) ontolitellm_1.84.0rc2.Conflict resolution
Three conflicts arose because the squash diff was rebased on staging content that is not on rc2. Three subagents independently verified each resolution.
.github/workflows/test-unit-proxy-db.ymlproxy-runtimeshard liststest_request_size_limit_middleware.py(added by Fix early proxy request size enforcement #27311, not on rc2). PR fix: harden /key/update authorization checks #27878 itself only addstest_multipart_bypass_repro.py.test_multipart_bypass_repro.py; drop thetest_request_size_limit_middleware.pyline (its source file does not exist on rc2).tests/test_litellm/proxy/rag_endpoints/test_rag_endpoints.pyclass TestRagIngestSSRFBlocked(from Worktree fix mcp byok oauth #27892). PR fix: harden /key/update authorization checks #27878 only addstest_rag_ingest_blocks_clientside_credentials.test_rag_ingest_blocks_clientside_credentials;TestRagIngestSSRFBlockedis part of the separate Worktree fix mcp byok oauth #27892 backport.tests/test_litellm/proxy/test_proxy_server.pytest_realtime_websocket_route_aliases_registered(from5d7b7e7e37, not on rc2). PR fix: harden /key/update authorization checks #27878 only adds theLitellmUserRoles, UserAPIKeyAuthimport and theTestTransformRequestBannedParamsclass.TestTransformRequestBannedParams; droptest_realtime_websocket_route_aliases_registered.Test plan
uv run pytest tests/test_litellm/proxy/management_endpoints/test_key_management_endpoints.py::TestKeyOwnerPrivilegeEscalation -vuv run pytest tests/test_litellm/proxy/rag_endpoints/test_rag_endpoints.py -vuv run pytest tests/test_litellm/proxy/test_proxy_server.py::TestTransformRequestBannedParams -vuv run pytest tests/proxy_unit_tests/test_prompt_test_endpoint.py -vuv run pytest tests/proxy_unit_tests/test_multipart_bypass_repro.py -vmake test-unitpasses on the rc2 base