mirror of
https://github.com/tiennm99/litellm.git
synced 2026-08-18 08:25:10 +00:00
Merge pull request #26929 from BerriAI/litellm_fix_service_account_user_id_bypass
fix(proxy): reject user_id=None on non-admin analytics endpoints (cross-tenant disclosure)
This commit is contained in:
@@ -1,5 +1,7 @@
|
||||
from typing import TYPE_CHECKING, Any, Dict, List, Optional, Union
|
||||
|
||||
from fastapi import HTTPException, status
|
||||
|
||||
from litellm._logging import verbose_proxy_logger
|
||||
from litellm.caching import DualCache
|
||||
from litellm.proxy._types import (
|
||||
@@ -29,6 +31,34 @@ def _user_has_admin_view(user_api_key_dict: UserAPIKeyAuth) -> bool:
|
||||
)
|
||||
|
||||
|
||||
def require_caller_user_id_for_non_admin(
|
||||
user_api_key_dict: UserAPIKeyAuth,
|
||||
) -> str:
|
||||
"""Return the caller's user_id, or raise 403 if missing.
|
||||
|
||||
Non-admin analytics endpoints scope queries by the caller's own user_id.
|
||||
Service-account keys are deliberately created with user_id=None
|
||||
(key_management_endpoints.py forces ``data.user_id = None`` at key
|
||||
creation). Without this guard, that None value flows through to the
|
||||
daily-activity builder, which treats ``entity_id is None`` as "no filter"
|
||||
and returns every tenant's data.
|
||||
|
||||
Callers must check is_admin first; this helper is only valid on the
|
||||
non-admin scoping branch.
|
||||
"""
|
||||
if user_api_key_dict.user_id is None:
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_403_FORBIDDEN,
|
||||
detail={
|
||||
"error": (
|
||||
"Service-account keys cannot query user analytics. "
|
||||
"Use a user-bound key, or call as a proxy admin."
|
||||
)
|
||||
},
|
||||
)
|
||||
return user_api_key_dict.user_id
|
||||
|
||||
|
||||
def _is_user_team_admin(
|
||||
user_api_key_dict: UserAPIKeyAuth, team_obj: LiteLLM_TeamTable
|
||||
) -> bool:
|
||||
|
||||
@@ -35,6 +35,7 @@ from litellm.proxy.management_endpoints.common_daily_activity import (
|
||||
from litellm.proxy.management_endpoints.common_utils import (
|
||||
_is_user_team_admin,
|
||||
_user_has_admin_view,
|
||||
require_caller_user_id_for_non_admin,
|
||||
)
|
||||
from litellm.proxy.management_endpoints.key_management_endpoints import (
|
||||
generate_key_helper_fn,
|
||||
@@ -2587,9 +2588,10 @@ async def get_user_daily_activity(
|
||||
if is_admin:
|
||||
entity_id = user_id # None means global view, otherwise filter by user
|
||||
else:
|
||||
caller_user_id = require_caller_user_id_for_non_admin(user_api_key_dict)
|
||||
if user_id is None:
|
||||
user_id = user_api_key_dict.user_id
|
||||
if user_id != user_api_key_dict.user_id:
|
||||
user_id = caller_user_id
|
||||
if user_id != caller_user_id:
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_403_FORBIDDEN,
|
||||
detail={
|
||||
@@ -2684,9 +2686,10 @@ async def get_user_daily_activity_aggregated(
|
||||
if is_admin:
|
||||
entity_id = user_id # None means global view, otherwise filter by user
|
||||
else:
|
||||
caller_user_id = require_caller_user_id_for_non_admin(user_api_key_dict)
|
||||
if user_id is None:
|
||||
user_id = user_api_key_dict.user_id
|
||||
if user_id != user_api_key_dict.user_id:
|
||||
user_id = caller_user_id
|
||||
if user_id != caller_user_id:
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_403_FORBIDDEN,
|
||||
detail={
|
||||
|
||||
@@ -440,6 +440,15 @@ def _resolve_fetch_kwargs(
|
||||
kwargs: Dict[str, Any] = {"start_date": start_date, "end_date": end_date}
|
||||
if fn_name == "get_usage_data":
|
||||
if not is_admin:
|
||||
if user_id is None:
|
||||
# Defense-in-depth: the endpoint guard in usage_endpoints/endpoints.py
|
||||
# should have already rejected this. If we ever reach here it means
|
||||
# a future caller invoked the helper without scoping — fail loudly
|
||||
# rather than issuing an unfiltered global query.
|
||||
raise ValueError(
|
||||
"Non-admin caller has user_id=None; refusing to issue an "
|
||||
"unscoped query. Endpoint-level guard missing."
|
||||
)
|
||||
kwargs["user_id"] = user_id
|
||||
elif fn_args.get("user_id"):
|
||||
kwargs["user_id"] = fn_args["user_id"]
|
||||
|
||||
@@ -44,13 +44,17 @@ async def usage_ai_chat(
|
||||
"""
|
||||
from litellm.proxy.management_endpoints.common_utils import (
|
||||
_user_has_admin_view,
|
||||
require_caller_user_id_for_non_admin,
|
||||
)
|
||||
from litellm.proxy.management_endpoints.usage_endpoints.ai_usage_chat import (
|
||||
stream_usage_ai_chat,
|
||||
)
|
||||
|
||||
is_admin = _user_has_admin_view(user_api_key_dict)
|
||||
user_id = user_api_key_dict.user_id
|
||||
if is_admin:
|
||||
user_id = user_api_key_dict.user_id
|
||||
else:
|
||||
user_id = require_caller_user_id_for_non_admin(user_api_key_dict)
|
||||
messages = [{"role": m.role, "content": m.content} for m in data.messages]
|
||||
|
||||
return StreamingResponse(
|
||||
|
||||
@@ -481,3 +481,39 @@ class TestSetObjectMetadataField:
|
||||
):
|
||||
_set_object_metadata_field(team, "model_rpm_limit", {"x": 1})
|
||||
assert team.metadata == {"model_rpm_limit": {"x": 1}}
|
||||
|
||||
|
||||
class TestRequireCallerUserIdForNonAdmin:
|
||||
"""
|
||||
Security regression: service-account keys (user_id=None) must not bypass
|
||||
the non-admin scoping branch on analytics endpoints.
|
||||
"""
|
||||
|
||||
def test_returns_user_id_when_present(self):
|
||||
from litellm.proxy.management_endpoints.common_utils import (
|
||||
require_caller_user_id_for_non_admin,
|
||||
)
|
||||
|
||||
key_dict = UserAPIKeyAuth(
|
||||
user_id="user-abc",
|
||||
user_role=LitellmUserRoles.INTERNAL_USER,
|
||||
)
|
||||
assert require_caller_user_id_for_non_admin(key_dict) == "user-abc"
|
||||
|
||||
def test_raises_403_when_user_id_is_none(self):
|
||||
from fastapi import HTTPException
|
||||
|
||||
from litellm.proxy.management_endpoints.common_utils import (
|
||||
require_caller_user_id_for_non_admin,
|
||||
)
|
||||
|
||||
# Simulates a service-account key (user_id forced to None at key creation)
|
||||
service_account_key = UserAPIKeyAuth(
|
||||
user_id=None,
|
||||
user_role=LitellmUserRoles.INTERNAL_USER,
|
||||
)
|
||||
with pytest.raises(HTTPException) as exc_info:
|
||||
require_caller_user_id_for_non_admin(service_account_key)
|
||||
|
||||
assert exc_info.value.status_code == 403
|
||||
assert "Service-account keys" in str(exc_info.value.detail)
|
||||
|
||||
@@ -1732,6 +1732,107 @@ async def test_get_user_daily_activity_non_admin_cannot_view_other_users(monkeyp
|
||||
assert call_kwargs.kwargs["entity_id"] == "regular-user-123"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_user_daily_activity_rejects_service_account_caller(monkeypatch):
|
||||
"""
|
||||
Security regression: a non-admin caller with user_id=None (a service-account
|
||||
key, where user_id is forced to None at key creation) must not be able to
|
||||
bypass the entity filter and read every tenant's daily spend.
|
||||
|
||||
Before the fix, the endpoint silently defaulted user_id to
|
||||
user_api_key_dict.user_id, which is itself None for service-account keys.
|
||||
None != None is False, the same-user check passed, and entity_id=None
|
||||
flowed into get_daily_activity, where the SQL builder treats None as
|
||||
"no filter".
|
||||
"""
|
||||
from unittest.mock import AsyncMock, MagicMock
|
||||
|
||||
from fastapi import HTTPException
|
||||
|
||||
from litellm.proxy.management_endpoints.internal_user_endpoints import (
|
||||
get_user_daily_activity,
|
||||
)
|
||||
|
||||
mock_prisma_client = MagicMock()
|
||||
monkeypatch.setattr("litellm.proxy.proxy_server.prisma_client", mock_prisma_client)
|
||||
|
||||
# Tripwire: ensure get_daily_activity is never reached
|
||||
mock_get_daily = AsyncMock()
|
||||
monkeypatch.setattr(
|
||||
"litellm.proxy.management_endpoints.internal_user_endpoints.get_daily_activity",
|
||||
mock_get_daily,
|
||||
)
|
||||
|
||||
service_account_key = UserAPIKeyAuth(
|
||||
user_id=None, # service-account keys have user_id forced to None
|
||||
user_role=LitellmUserRoles.INTERNAL_USER,
|
||||
)
|
||||
|
||||
with pytest.raises(HTTPException) as exc_info:
|
||||
await get_user_daily_activity(
|
||||
start_date="2025-01-01",
|
||||
end_date="2025-01-31",
|
||||
model=None,
|
||||
api_key=None,
|
||||
user_id=None,
|
||||
page=1,
|
||||
page_size=50,
|
||||
timezone=None,
|
||||
user_api_key_dict=service_account_key,
|
||||
)
|
||||
|
||||
assert exc_info.value.status_code == 403
|
||||
assert "Service-account keys" in str(exc_info.value.detail)
|
||||
mock_get_daily.assert_not_called()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_user_daily_activity_aggregated_rejects_service_account_caller(
|
||||
monkeypatch,
|
||||
):
|
||||
"""
|
||||
Same security regression as
|
||||
test_get_user_daily_activity_rejects_service_account_caller, on the
|
||||
aggregated route. Same shape, raw-SQL builder, same fix.
|
||||
"""
|
||||
from unittest.mock import AsyncMock, MagicMock
|
||||
|
||||
from fastapi import HTTPException
|
||||
|
||||
from litellm.proxy.management_endpoints.internal_user_endpoints import (
|
||||
get_user_daily_activity_aggregated,
|
||||
)
|
||||
|
||||
mock_prisma_client = MagicMock()
|
||||
monkeypatch.setattr("litellm.proxy.proxy_server.prisma_client", mock_prisma_client)
|
||||
|
||||
mock_get_daily_agg = AsyncMock()
|
||||
monkeypatch.setattr(
|
||||
"litellm.proxy.management_endpoints.internal_user_endpoints.get_daily_activity_aggregated",
|
||||
mock_get_daily_agg,
|
||||
)
|
||||
|
||||
service_account_key = UserAPIKeyAuth(
|
||||
user_id=None,
|
||||
user_role=LitellmUserRoles.INTERNAL_USER,
|
||||
)
|
||||
|
||||
with pytest.raises(HTTPException) as exc_info:
|
||||
await get_user_daily_activity_aggregated(
|
||||
start_date="2025-01-01",
|
||||
end_date="2025-01-31",
|
||||
model=None,
|
||||
api_key=None,
|
||||
user_id=None,
|
||||
timezone=None,
|
||||
user_api_key_dict=service_account_key,
|
||||
)
|
||||
|
||||
assert exc_info.value.status_code == 403
|
||||
assert "Service-account keys" in str(exc_info.value.detail)
|
||||
mock_get_daily_agg.assert_not_called()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_user_daily_activity_aggregated_admin_global_view(monkeypatch):
|
||||
"""
|
||||
|
||||
@@ -409,3 +409,60 @@ class TestStreamUsageAiChat:
|
||||
end_date="2025-01-31",
|
||||
user_id="my-user-id",
|
||||
)
|
||||
|
||||
|
||||
class TestUsageAiChatServiceAccountGuard:
|
||||
"""
|
||||
Security regression: a non-admin caller with user_id=None (service-account
|
||||
key) must be rejected at the endpoint boundary, before any tool dispatch.
|
||||
"""
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_non_admin_with_user_id_none_is_rejected(self):
|
||||
from fastapi import HTTPException
|
||||
|
||||
from litellm.proxy._types import LitellmUserRoles, UserAPIKeyAuth
|
||||
from litellm.proxy.management_endpoints.usage_endpoints.endpoints import (
|
||||
ChatMessage,
|
||||
UsageAIChatRequest,
|
||||
usage_ai_chat,
|
||||
)
|
||||
|
||||
service_account_key = UserAPIKeyAuth(
|
||||
user_id=None,
|
||||
user_role=LitellmUserRoles.INTERNAL_USER,
|
||||
)
|
||||
request = MagicMock()
|
||||
body = UsageAIChatRequest(
|
||||
messages=[ChatMessage(role="user", content="hi")],
|
||||
model="gpt-4o-mini",
|
||||
)
|
||||
|
||||
with pytest.raises(HTTPException) as exc_info:
|
||||
await usage_ai_chat(
|
||||
data=body,
|
||||
request=request,
|
||||
user_api_key_dict=service_account_key,
|
||||
)
|
||||
|
||||
assert exc_info.value.status_code == 403
|
||||
assert "Service-account keys" in str(exc_info.value.detail)
|
||||
|
||||
def test_resolve_fetch_kwargs_tripwire_fires_on_none_user_id(self):
|
||||
"""
|
||||
Defense-in-depth: if a future endpoint forgets the entry guard and
|
||||
a non-admin caller with user_id=None reaches _resolve_fetch_kwargs,
|
||||
the tripwire must fire rather than issuing an unscoped query.
|
||||
"""
|
||||
from litellm.proxy.management_endpoints.usage_endpoints.ai_usage_chat import (
|
||||
_resolve_fetch_kwargs,
|
||||
)
|
||||
|
||||
with pytest.raises(ValueError) as exc_info:
|
||||
_resolve_fetch_kwargs(
|
||||
fn_name="get_usage_data",
|
||||
fn_args={"start_date": "2025-01-01", "end_date": "2025-01-31"},
|
||||
user_id=None,
|
||||
is_admin=False,
|
||||
)
|
||||
assert "Endpoint-level guard missing" in str(exc_info.value)
|
||||
|
||||
Reference in New Issue
Block a user