-
-
Notifications
You must be signed in to change notification settings - Fork 11.8k
fix: tighten budget field validation and authorization checks #27897
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
d0368bc
a0724d5
09bff75
e154a36
62b1c47
c3b33b1
e4010a8
bf9c279
e66e369
cc0751a
b89f1ae
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 |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ | |
| """ | ||
|
|
||
| import asyncio | ||
| import math | ||
| import re | ||
| import time | ||
| from typing import TYPE_CHECKING, Any, Dict, List, Literal, Optional, Type, Union, cast | ||
|
|
@@ -328,7 +329,10 @@ def _global_proxy_budget_check( | |
| and route != "/v1/models" | ||
| and route != "/models" | ||
| ): | ||
| if global_proxy_spend > litellm.max_budget: | ||
| if ( | ||
| math.isfinite(litellm.max_budget) | ||
| and global_proxy_spend > litellm.max_budget | ||
| ): | ||
| raise litellm.BudgetExceededError( | ||
| current_cost=global_proxy_spend, max_budget=litellm.max_budget | ||
| ) | ||
|
|
@@ -645,7 +649,7 @@ async def common_checks( # noqa: PLR0915 | |
| counter_key=f"spend:user:{user_object.user_id}", | ||
| fallback_spend=user_object.spend or 0.0, | ||
| ) | ||
| if user_spend >= user_budget: | ||
| if math.isfinite(user_budget) and user_spend >= user_budget: | ||
| raise litellm.BudgetExceededError( | ||
| current_cost=user_spend, | ||
| max_budget=user_budget, | ||
|
|
@@ -3280,7 +3284,10 @@ async def _virtual_key_max_budget_check( | |
| # collect information for alerting # | ||
| #################################### | ||
|
|
||
| if spend >= valid_token.max_budget: | ||
| # Defense-in-depth (GHSA-2rv4-xv66-fpjg): spend >= NaN is always False, | ||
| # so a NaN max_budget would silently disable enforcement. Treat a | ||
| # non-finite max_budget as "no configured limit" rather than as a bypass. | ||
| if math.isfinite(valid_token.max_budget) and spend >= valid_token.max_budget: | ||
| raise litellm.BudgetExceededError( | ||
| current_cost=spend, | ||
| max_budget=valid_token.max_budget, | ||
|
|
@@ -3313,7 +3320,7 @@ async def _virtual_key_multi_budget_check( | |
| counter_key=counter_key, | ||
| fallback_spend=0.0, | ||
| ) | ||
| if window_spend >= w["max_budget"]: | ||
| if math.isfinite(w["max_budget"]) and window_spend >= w["max_budget"]: | ||
|
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. High: Non-finite window budgets are treated as unlimited
|
||
| raise litellm.BudgetExceededError( | ||
| current_cost=window_spend, | ||
| max_budget=w["max_budget"], | ||
|
|
@@ -3568,7 +3575,10 @@ async def _check_team_member_budget( | |
| fallback_spend=team_member_spend, | ||
| ) | ||
|
|
||
| if team_member_spend >= team_member_budget: | ||
| if ( | ||
| math.isfinite(team_member_budget) | ||
| and team_member_spend >= team_member_budget | ||
| ): | ||
| raise litellm.BudgetExceededError( | ||
| current_cost=team_member_spend, | ||
| max_budget=team_member_budget, | ||
|
|
@@ -3650,7 +3660,7 @@ async def _team_max_budget_check( | |
| fallback_spend=team_object.spend or 0.0, | ||
| ) | ||
|
|
||
| if spend > team_object.max_budget: | ||
| if math.isfinite(team_object.max_budget) and spend > team_object.max_budget: | ||
| if valid_token: | ||
| call_info = CallInfo( | ||
| token=valid_token.token, | ||
|
|
@@ -3698,7 +3708,7 @@ async def _team_multi_budget_check( | |
| counter_key=counter_key, | ||
| fallback_spend=0.0, | ||
| ) | ||
| if window_spend >= w["max_budget"]: | ||
| if math.isfinite(w["max_budget"]) and window_spend >= w["max_budget"]: | ||
| raise litellm.BudgetExceededError( | ||
| current_cost=window_spend, | ||
| max_budget=w["max_budget"], | ||
|
|
@@ -3812,6 +3822,7 @@ async def _project_max_budget_check( | |
| if ( | ||
| max_budget is not None | ||
| and project_object.spend is not None | ||
| and math.isfinite(max_budget) | ||
| and project_object.spend > max_budget | ||
| ): | ||
| if valid_token: | ||
|
|
@@ -4004,7 +4015,7 @@ async def _organization_max_budget_check( | |
| ) | ||
|
|
||
| # Check if organization spend exceeds max budget | ||
| if org_spend >= org_max_budget: | ||
| if math.isfinite(org_max_budget) and org_spend >= org_max_budget: | ||
| # Trigger budget alert | ||
| call_info = CallInfo( | ||
| token=valid_token.token, | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -1152,6 +1152,46 @@ def _update_internal_user_params( | |||||
| return non_default_values | ||||||
|
|
||||||
|
|
||||||
| async def _schedule_user_update_audit_log( | ||||||
| response: Dict[str, Any], | ||||||
| existing_user_row: Optional[BaseModel], | ||||||
| litellm_changed_by: Optional[str], | ||||||
| user_api_key_dict: UserAPIKeyAuth, | ||||||
| litellm_proxy_admin_name: Optional[str], | ||||||
| ) -> None: | ||||||
| from litellm.proxy.proxy_server import prisma_client | ||||||
|
|
||||||
| if prisma_client is None: | ||||||
| return | ||||||
| try: | ||||||
| updated_user_row = await prisma_client.db.litellm_usertable.find_first( | ||||||
| where={"user_id": response["user_id"]} | ||||||
| ) | ||||||
| if updated_user_row: | ||||||
| user_row_typed = LiteLLM_UserTable( | ||||||
| **updated_user_row.model_dump(exclude_none=True) | ||||||
| ) | ||||||
| asyncio.create_task( | ||||||
| UserManagementEventHooks.create_internal_user_audit_log( | ||||||
| user_id=user_row_typed.user_id, | ||||||
| action="updated", | ||||||
| litellm_changed_by=litellm_changed_by or user_api_key_dict.user_id, | ||||||
| user_api_key_dict=user_api_key_dict, | ||||||
| litellm_proxy_admin_name=litellm_proxy_admin_name, | ||||||
| before_value=( | ||||||
| existing_user_row.model_dump_json(exclude_none=True) | ||||||
| if existing_user_row | ||||||
| else None | ||||||
| ), | ||||||
| after_value=user_row_typed.model_dump_json(exclude_none=True), | ||||||
| ) | ||||||
| ) | ||||||
| except Exception as audit_error: | ||||||
| verbose_proxy_logger.warning( | ||||||
| f"Failed to create audit log for user {response.get('user_id')}: {audit_error}" | ||||||
| ) | ||||||
|
|
||||||
|
|
||||||
| def _check_user_update_authz( | ||||||
| user_request: UpdateUserRequest, | ||||||
| user_api_key_dict: UserAPIKeyAuth, | ||||||
|
|
@@ -1229,6 +1269,32 @@ async def _update_single_user_helper( | |||||
| **existing_user_row.model_dump(exclude_none=True) | ||||||
| ) | ||||||
|
|
||||||
| # Prevent budget self-escalation (GHSA-wvg4-6222-3q4r): non-admin callers | ||||||
| # must not be able to raise their own budget/spend fields. | ||||||
| # can_user_call_user_update() already restricts non-admins to self-updates, | ||||||
| # so this guard only fires for self-escalation attempts. | ||||||
| _target_user_id = user_request.user_id or ( | ||||||
| getattr(existing_user_row, "user_id", None) | ||||||
| if existing_user_row is not None | ||||||
| else None | ||||||
| ) | ||||||
| _is_self_update = ( | ||||||
| _target_user_id is not None and user_api_key_dict.user_id == _target_user_id | ||||||
| ) | ||||||
| if ( | ||||||
| _is_self_update | ||||||
| and user_api_key_dict.user_role != LitellmUserRoles.PROXY_ADMIN.value | ||||||
| ): | ||||||
| _protected_fields = ("max_budget", "soft_budget", "spend") | ||||||
|
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. High: User budget reset bypass
Suggested change
|
||||||
| for _field in _protected_fields: | ||||||
| if _field in non_default_values: | ||||||
| raise HTTPException( | ||||||
| status_code=403, | ||||||
| detail={ | ||||||
| "error": f"Non-admin users cannot modify '{_field}' on their own record. Contact your proxy admin." | ||||||
| }, | ||||||
| ) | ||||||
|
|
||||||
| existing_metadata = ( | ||||||
| cast(Dict, getattr(existing_user_row, "metadata", {}) or {}) | ||||||
| if existing_user_row is not None | ||||||
|
|
@@ -1280,39 +1346,14 @@ async def _update_single_user_helper( | |||||
| data=non_default_values, table_name="user" | ||||||
| ) | ||||||
|
|
||||||
| # Create audit log for successful update | ||||||
| if response is not None: | ||||||
| try: | ||||||
| updated_user_row = await prisma_client.db.litellm_usertable.find_first( | ||||||
| where={"user_id": response["user_id"]} | ||||||
| ) | ||||||
|
|
||||||
| if updated_user_row: | ||||||
| user_row_typed = LiteLLM_UserTable( | ||||||
| **updated_user_row.model_dump(exclude_none=True) | ||||||
| ) | ||||||
|
|
||||||
| # Create audit log asynchronously | ||||||
| asyncio.create_task( | ||||||
| UserManagementEventHooks.create_internal_user_audit_log( | ||||||
| user_id=user_row_typed.user_id, | ||||||
| action="updated", | ||||||
| litellm_changed_by=litellm_changed_by | ||||||
| or user_api_key_dict.user_id, | ||||||
| user_api_key_dict=user_api_key_dict, | ||||||
| litellm_proxy_admin_name=litellm_proxy_admin_name, | ||||||
| before_value=( | ||||||
| existing_user_row.model_dump_json(exclude_none=True) | ||||||
| if existing_user_row | ||||||
| else None | ||||||
| ), | ||||||
| after_value=user_row_typed.model_dump_json(exclude_none=True), | ||||||
| ) | ||||||
| ) | ||||||
| except Exception as audit_error: | ||||||
| verbose_proxy_logger.warning( | ||||||
| f"Failed to create audit log for user {response.get('user_id')}: {audit_error}" | ||||||
| ) | ||||||
| await _schedule_user_update_audit_log( | ||||||
| response=response, | ||||||
| existing_user_row=existing_user_row, | ||||||
| litellm_changed_by=litellm_changed_by, | ||||||
| user_api_key_dict=user_api_key_dict, | ||||||
| litellm_proxy_admin_name=litellm_proxy_admin_name, | ||||||
| ) | ||||||
|
|
||||||
| if response is None: | ||||||
| raise HTTPException( | ||||||
|
|
||||||
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.
Medium: Non-finite user budget bypass
The runtime budget check skips enforcement for non-finite
user_budgetvalues. If/user/newacceptsmax_budgetwithout finite-number validation, a caller allowed to create users can sendNaNorInfinityand receive a user key whose user-level spend cap is skipped here. Fail closed at enforcement or add equivalent validation on user creation.