-
-
Notifications
You must be signed in to change notification settings - Fork 11.9k
fix: backport #27878 to litellm_1.84.0rc2 #27904
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,3 @@ | ||
| -- AlterTable | ||
| ALTER TABLE "LiteLLM_TeamMembership" ADD COLUMN "total_spend" DOUBLE PRECISION NOT NULL DEFAULT 0.0; | ||
| ALTER TABLE "LiteLLM_TeamMembership" ADD COLUMN IF NOT EXISTS "total_spend" DOUBLE PRECISION NOT NULL DEFAULT 0.0; | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2186,23 +2186,32 @@ async def _validate_update_key_data( | |
| # - max_budget / spend: always require the admin check, even for the | ||
| # key owner or a team member (matches the existing admin-only | ||
| # budget semantics). | ||
| is_key_owner = ( | ||
| user_api_key_dict.user_id is not None | ||
| and existing_key_row.user_id == user_api_key_dict.user_id | ||
| ) | ||
| _is_budget_change = ( | ||
| data.max_budget is not None and data.max_budget != existing_key_row.max_budget | ||
| ) or ( | ||
| data.spend is not None | ||
| and data.spend != getattr(existing_key_row, "spend", None) | ||
| ) | ||
| is_team_key = existing_key_row.team_id is not None | ||
| can_skip_admin_check_for_non_budget = is_key_owner or is_team_key | ||
| if ( | ||
| (not _is_proxy_admin) | ||
| and prisma_client is not None | ||
| and (_is_budget_change or not can_skip_admin_check_for_non_budget) | ||
| ): | ||
|
|
||
| # Personal-key bypass: the caller both created the key AND still owns it | ||
| # (user_id == caller). Checking only created_by would let a demoted admin | ||
| # who originally created a key for another user continue editing it without | ||
| # admin authorization after the key was reassigned. | ||
| 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 | ||
|
Comment on lines
+2200
to
+2213
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The new bypass condition requires both |
||
| if (not _is_proxy_admin) and prisma_client is not None and not can_skip_admin_check: | ||
| hashed_key = existing_key_row.token | ||
| await _check_key_admin_access( | ||
| user_api_key_dict=user_api_key_dict, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| """ | ||
| Repro: multipart/form-data delivers litellm_embedding_config as a JSON | ||
| string. is_request_body_safe skips the nested banned-param check because | ||
| isinstance(nested, dict) is False for a string value. | ||
|
|
||
| A banned param (api_base, aws_sts_endpoint, etc.) nested inside the | ||
| stringified config is therefore invisible to the bouncer. | ||
| """ | ||
|
|
||
| import json | ||
| import pytest | ||
|
|
||
|
|
||
| class TestMultipartNestedBypass: | ||
|
|
||
| def test_nested_banned_param_caught_when_dict(self): | ||
| """Baseline: nested api_base inside a dict IS caught.""" | ||
| from litellm.proxy.auth.auth_utils import is_request_body_safe | ||
|
|
||
| request_body = { | ||
| "model": "text-embedding-ada-002", | ||
| "litellm_embedding_config": {"api_base": "https://attacker.com"}, | ||
| } | ||
|
|
||
| with pytest.raises(ValueError, match="api_base"): | ||
| is_request_body_safe( | ||
| request_body=request_body, | ||
| general_settings={}, | ||
| llm_router=None, | ||
| model="text-embedding-ada-002", | ||
| ) | ||
|
|
||
| def test_nested_banned_param_blocked_when_json_string(self): | ||
| """ | ||
| Regression: multipart delivers litellm_embedding_config as a JSON string. | ||
| _coerce_metadata_to_dict now parses it before the banned-param check, | ||
| so api_base nested inside the stringified config IS caught. | ||
| """ | ||
| from litellm.proxy.auth.auth_utils import is_request_body_safe | ||
|
|
||
| # Exactly what _read_request_body produces for multipart: | ||
| # dict(await request.form()) gives string values for non-file fields. | ||
| request_body = { | ||
| "model": "text-embedding-ada-002", | ||
| "litellm_embedding_config": json.dumps( | ||
| {"api_base": "https://attacker.com"} | ||
| ), | ||
| } | ||
|
|
||
| with pytest.raises(ValueError, match="api_base"): | ||
| is_request_body_safe( | ||
| request_body=request_body, | ||
| general_settings={}, | ||
| llm_router=None, | ||
| model="text-embedding-ada-002", | ||
| ) | ||
|
|
||
| def test_nested_aws_sts_endpoint_blocked_when_json_string(self): | ||
| """Regression: aws_sts_endpoint nested in JSON-string config is caught.""" | ||
| from litellm.proxy.auth.auth_utils import is_request_body_safe | ||
|
|
||
| request_body = { | ||
| "model": "text-embedding-ada-002", | ||
| "litellm_embedding_config": json.dumps( | ||
| {"aws_sts_endpoint": "https://attacker.com/sts"} | ||
| ), | ||
| } | ||
|
|
||
| with pytest.raises(ValueError, match="aws_sts_endpoint"): | ||
| is_request_body_safe( | ||
| request_body=request_body, | ||
| general_settings={}, | ||
| llm_router=None, | ||
| model="text-embedding-ada-002", | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
str()conversionraw_pathis assigned withstr(scope.get(...)), so it is always astr. The subsequentif not isinstance(raw_path, str)guard is unreachable and misleads readers into thinking the value could be a non-string at that point.