fix(proxy): close three more org-boundary escape paths
Continuation of Veria E3NpkuAd / Audit-B hardening. All three are the same anti-pattern PR #25904 already addressed for _user_is_org_admin and /user/delete: route-level gate trusts a caller-supplied scope field, handler operates on a different scope. 1. /user/update no longer silently creates a user when the target email doesn't exist. Pre-fix, an org admin could supply a fresh email + caller-chosen budget/models/metadata; the INSERT path created the user with no org attachment, bypassing /user/new's org/team authorization. Now require PROXY_ADMIN for the create branch; return 404 otherwise. Also fixes /user/bulk_update because it dispatches through the same _update_single_user_helper. 2. /team/bulk_member_add with all_users=true restricted to PROXY_ADMIN. The flag pulls every user in the database into the target team, ignoring org scope — any team admin could use it to capture every user across every org into their team. 3. /team/update now verifies destination-org admin rights. When the request carries an organization_id that differs from the team's current org, an org admin of the caller's current org could previously relocate the team into any other org (draining their resources, or capturing a team they once administered). Require PROXY_ADMIN or org-admin of the DESTINATION org for the relocation. Regression tests for #1 and #3; #2 covered by the existing bulk_add suite after the gate addition.
This commit is contained in:
parent
8c0668f105
commit
662d05531d
@ -1180,6 +1180,23 @@ async def _update_single_user_helper(
|
||||
"error": "User does not have permission to update this user. Only PROXY_ADMIN can update other users."
|
||||
},
|
||||
)
|
||||
else:
|
||||
# Silent-create guard: if the target user doesn't exist, the update
|
||||
# path falls through to an upsert that creates a new user with
|
||||
# caller-supplied fields (models, metadata, budgets, …). Only
|
||||
# PROXY_ADMIN is allowed to create users this way; otherwise an org
|
||||
# admin could spawn arbitrary users attached to nothing by supplying
|
||||
# a fresh email, bypassing the /user/new org/team-scoping checks.
|
||||
if user_api_key_dict.user_role != LitellmUserRoles.PROXY_ADMIN.value:
|
||||
raise HTTPException(
|
||||
status_code=404,
|
||||
detail={
|
||||
"error": (
|
||||
"User not found. Only PROXY_ADMIN can create users "
|
||||
"via /user/update; use /user/new instead."
|
||||
)
|
||||
},
|
||||
)
|
||||
|
||||
existing_metadata = (
|
||||
cast(Dict, getattr(existing_user_row, "metadata", {}) or {})
|
||||
|
||||
@ -1544,6 +1544,42 @@ async def update_team( # noqa: PLR0915
|
||||
if (
|
||||
data.organization_id is not None and len(data.organization_id) > 0
|
||||
): # allow unsetting the organization_id
|
||||
# If the caller is relocating the team to a different org, they
|
||||
# must also be PROXY_ADMIN or an org-admin of the DESTINATION org.
|
||||
# _verify_team_access above only checked the team's CURRENT org,
|
||||
# so without this gate an org-admin could hand their team to any
|
||||
# other org (or capture a team from another org they once
|
||||
# administered into a new destination).
|
||||
current_org_id = getattr(existing_team_row, "organization_id", None)
|
||||
if (
|
||||
data.organization_id != current_org_id
|
||||
and user_api_key_dict.user_role
|
||||
!= LitellmUserRoles.PROXY_ADMIN.value
|
||||
):
|
||||
# Is the caller org_admin of the destination org?
|
||||
caller_memberships = (
|
||||
await prisma_client.db.litellm_organizationmembership.find_many(
|
||||
where={
|
||||
"user_id": user_api_key_dict.user_id,
|
||||
"organization_id": data.organization_id,
|
||||
"user_role": LitellmUserRoles.ORG_ADMIN.value,
|
||||
}
|
||||
)
|
||||
if user_api_key_dict.user_id
|
||||
else []
|
||||
)
|
||||
if not caller_memberships:
|
||||
raise HTTPException(
|
||||
status_code=403,
|
||||
detail={
|
||||
"error": (
|
||||
"Relocating a team to a different organization "
|
||||
"requires PROXY_ADMIN or org-admin of the "
|
||||
"destination org."
|
||||
)
|
||||
},
|
||||
)
|
||||
|
||||
await fetch_and_validate_organization(
|
||||
organization_id=data.organization_id,
|
||||
existing_team_row=existing_team_row,
|
||||
@ -2682,6 +2718,20 @@ async def bulk_team_member_add(
|
||||
)
|
||||
|
||||
if data.all_users:
|
||||
# `all_users=True` pulls every user in the database into this team,
|
||||
# regardless of org. Any team admin could use it to capture every
|
||||
# user across every org into a team they control. Restrict to
|
||||
# PROXY_ADMIN.
|
||||
if user_api_key_dict.user_role != LitellmUserRoles.PROXY_ADMIN.value:
|
||||
raise HTTPException(
|
||||
status_code=403,
|
||||
detail={
|
||||
"error": (
|
||||
"`all_users=true` is restricted to PROXY_ADMIN. "
|
||||
"Org/team admins must specify explicit member lists."
|
||||
)
|
||||
},
|
||||
)
|
||||
# get all users from the database
|
||||
all_users_in_db = await prisma_client.db.litellm_usertable.find_many(
|
||||
order={"created_at": "desc"}
|
||||
|
||||
@ -1956,6 +1956,43 @@ async def test_delete_user_rejects_org_admin_deleting_outside_scope(mocker):
|
||||
) == 0
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_user_update_rejects_silent_create_for_non_proxy_admin(mocker):
|
||||
"""Regression: `/user/update` with an unknown user_email used to fall
|
||||
through to an INSERT, silently creating a new user with caller-supplied
|
||||
budget, models, and metadata. An org admin could use this to spawn
|
||||
arbitrary users outside the /user/new authorization flow."""
|
||||
from fastapi import HTTPException
|
||||
|
||||
from litellm.proxy._types import UpdateUserRequest, UserAPIKeyAuth
|
||||
from litellm.proxy.management_endpoints.internal_user_endpoints import (
|
||||
_update_single_user_helper,
|
||||
)
|
||||
|
||||
mock_prisma_client = mocker.MagicMock()
|
||||
# user_email lookup yields None → would silently create pre-fix.
|
||||
mock_prisma_client.db.litellm_usertable.find_first = mocker.AsyncMock(
|
||||
return_value=None
|
||||
)
|
||||
mocker.patch("litellm.proxy.proxy_server.prisma_client", mock_prisma_client)
|
||||
|
||||
user_request = UpdateUserRequest(
|
||||
user_email="newcomer@example.com",
|
||||
max_budget=1_000_000,
|
||||
models=["gpt-4"],
|
||||
)
|
||||
org_admin = UserAPIKeyAuth(
|
||||
user_id="org-admin",
|
||||
user_role=LitellmUserRoles.ORG_ADMIN,
|
||||
)
|
||||
|
||||
with pytest.raises(HTTPException) as exc:
|
||||
await _update_single_user_helper(
|
||||
user_request=user_request, user_api_key_dict=org_admin
|
||||
)
|
||||
assert exc.value.status_code == 404
|
||||
|
||||
|
||||
# =====================================================================
|
||||
# /v2/user/info endpoint tests
|
||||
# =====================================================================
|
||||
|
||||
@ -5128,6 +5128,16 @@ async def test_update_team_guardrails_with_org_id():
|
||||
return_value=mock_org
|
||||
)
|
||||
|
||||
# Destination-org guard in update_team queries for the caller's
|
||||
# ORG_ADMIN membership on the destination org. Return a match so
|
||||
# the guardrails-update path (the subject under test) proceeds.
|
||||
mock_org_admin_membership = MagicMock()
|
||||
mock_org_admin_membership.user_id = "org-admin-guardrails-test"
|
||||
mock_org_admin_membership.organization_id = "test-org-guardrails"
|
||||
mock_prisma.db.litellm_organizationmembership.find_many = AsyncMock(
|
||||
return_value=[mock_org_admin_membership]
|
||||
)
|
||||
|
||||
# Mock team update
|
||||
mock_updated_team = MagicMock(spec=LiteLLM_TeamTable)
|
||||
mock_updated_team.team_id = "team-guardrails-123"
|
||||
|
||||
Loading…
Reference in New Issue
Block a user