From 1fbd2dbb69606dc7233de0bf2310ffc0fa15fed2 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 1 Sep 2026 05:11:57 +0000 Subject: [PATCH] fix(spend_logs): authorize every request_id lookup match, not just one x-litellm-call-id is client-supplied and litellm_call_id is not unique, so a caller can seed their own row with a litellm_call_id that collides with another tenant's request_id. _assert_user_can_view_request_id used find_first, which authorized whichever single row came back, while the follow-up list and detail queries returned every matching row (or, via an unordered LIMIT 1, a different match). Read every row the OR clause resolves to and require the caller to own each one; any unowned match denies the request. --- .../spend_management_endpoints.py | 47 +++++++++-------- .../test_spend_management_endpoints.py | 52 ++++++++++++++++--- 2 files changed, 70 insertions(+), 29 deletions(-) diff --git a/litellm/proxy/spend_tracking/spend_management_endpoints.py b/litellm/proxy/spend_tracking/spend_management_endpoints.py index e7e8e0c5341..b636ef864e9 100644 --- a/litellm/proxy/spend_tracking/spend_management_endpoints.py +++ b/litellm/proxy/spend_tracking/spend_management_endpoints.py @@ -243,9 +243,14 @@ def _request_id_or_call_id_clause(request_id: str) -> tuple[_RequestIdEquals, _L return (request_id_clause, call_id_clause) -async def _find_spend_log_row(prisma_client: PrismaClient, request_id: str) -> _SpendLogOwnershipRow | None: - """Read the single spend log row identified by ``request_id`` or ``litellm_call_id``.""" - return await _spend_logs_table(prisma_client).find_first( +async def _find_spend_log_rows(prisma_client: PrismaClient, request_id: str) -> Sequence[_SpendLogOwnershipRow]: + """Read every spend log row matching ``request_id`` or ``litellm_call_id``. + + ``litellm_call_id`` is client-supplied (``x-litellm-call-id``) and not unique, + so a single id can address more than one row across tenants. Callers that + need to authorize the id must inspect every matching row, not just one. + """ + return await _spend_logs_table(prisma_client).find_many( where={"OR": _request_id_or_call_id_clause(request_id)}, include=None, ) @@ -4301,31 +4306,29 @@ async def _assert_user_can_view_request_id( request_id: str, ) -> None: """ - Verify the requesting non-admin user is allowed to view this spend-log row. - Allowed when the log belongs to the user directly, or to one of their - permitted teams (admin or ``/spend/logs`` permission). + Verify the requesting non-admin user is allowed to view every spend log row + the ``request_id`` lookup can resolve to. Allowed per row when the log + belongs to the user directly, or to one of their permitted teams (admin or + ``/spend/logs`` permission). Because ``litellm_call_id`` is client-supplied + and non-unique, one id can address rows across tenants, so authorization + must hold for every matching row: any unowned match denies the request. Raises HTTP 403 if not. """ - row: Final = await _find_spend_log_row(prisma_client, request_id) - if row is None: - return - - if row.user is not None and row.user == user_api_key_dict.user_id: - return - - if row.team_id: - can_view: Final = await _can_team_member_view_log( + rows: Final = await _find_spend_log_rows(prisma_client, request_id) + caller_user_id: Final = user_api_key_dict.user_id + for row in rows: + if caller_user_id is not None and row.user == caller_user_id: + continue + if row.team_id and await _can_team_member_view_log( prisma_client=prisma_client, user_api_key_dict=user_api_key_dict, team_id=row.team_id, + ): + continue + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail={"error": f"Not authorized to view spend log for request_id={request_id}"}, ) - if can_view: - return - - raise HTTPException( - status_code=status.HTTP_403_FORBIDDEN, - detail={"error": f"Not authorized to view spend log for request_id={request_id}"}, - ) async def _get_permitted_team_ids_for_spend_logs( diff --git a/tests/test_litellm/proxy/spend_tracking/test_spend_management_endpoints.py b/tests/test_litellm/proxy/spend_tracking/test_spend_management_endpoints.py index 6d392fc5b3b..b1dfc3a64e5 100644 --- a/tests/test_litellm/proxy/spend_tracking/test_spend_management_endpoints.py +++ b/tests/test_litellm/proxy/spend_tracking/test_spend_management_endpoints.py @@ -413,8 +413,8 @@ async def test_assert_user_can_view_request_id_rejects_both_users_none(): team_id = None class MockSpendLogs: - async def find_first(self, where=None, include=None): - return MockRow() + async def find_many(self, where=None, include=None): + return [MockRow()] class MockDB: def __init__(self): @@ -432,6 +432,44 @@ async def test_assert_user_can_view_request_id_rejects_both_users_none(): assert exc_info.value.status_code == 403 +@pytest.mark.asyncio +async def test_assert_user_can_view_request_id_denies_cross_tenant_call_id_collision(): + """ + Regression: ``litellm_call_id`` is client-supplied (``x-litellm-call-id``) and + not unique, so a caller can seed their own row with a ``litellm_call_id`` + that collides with another tenant's ``request_id``. The auth check must + inspect every matching row rather than just the first one, otherwise it + would pass on the caller's owned row and the follow-up list/detail query + could return the unowned sibling. + """ + + class Row: + def __init__(self, user, team_id=None): + self.user = user + self.team_id = team_id + + class MockSpendLogs: + async def find_many(self, where=None, include=None): + return [Row("caller_user"), Row("victim_user")] + + class MockDB: + def __init__(self): + self.litellm_spendlogs = MockSpendLogs() + + class MockPrisma: + def __init__(self): + self.db = MockDB() + + auth = UserAPIKeyAuth( + user_role=LitellmUserRoles.INTERNAL_USER, user_id="caller_user" + ) + with pytest.raises(HTTPException) as exc_info: + await spend_management_endpoints._assert_user_can_view_request_id( + MockPrisma(), auth, "colliding-id" + ) + assert exc_info.value.status_code == 403 + + def test_ui_view_request_response_forbids_non_admin_without_db(client, monkeypatch): """ Without prisma, non-admins cannot be authorized to read request/response @@ -2334,8 +2372,8 @@ async def test_ui_view_spend_logs_request_id_blocks_non_owner(client, monkeypatc team_id = None class _SpendLogs: - async def find_first(self, where=None, include=None): - return _ForeignRow() + async def find_many(self, where=None, include=None): + return [_ForeignRow()] class _DB: def __init__(self): @@ -2400,10 +2438,10 @@ async def test_ui_view_spend_logs_request_id_owner_scoped_by_id_only( user = "user_1" team_id = "team1" - async def _find_first(where=None, include=None): - return _OwnedRow() + async def _find_many(where=None, include=None): + return [_OwnedRow()] - mock_prisma.db.find_first = _find_first + mock_prisma.db.find_many = _find_many monkeypatch.setattr("litellm.proxy.proxy_server.prisma_client", mock_prisma) # A 5-day window that EXCLUDES the 90-day-old log, as the dashboard sends.