diff --git a/litellm/proxy/spend_tracking/spend_management_endpoints.py b/litellm/proxy/spend_tracking/spend_management_endpoints.py index a1f259861e3..b8b639b66db 100644 --- a/litellm/proxy/spend_tracking/spend_management_endpoints.py +++ b/litellm/proxy/spend_tracking/spend_management_endpoints.py @@ -2901,15 +2901,16 @@ async def ui_view_request_response_for_request_id( start_time_utc=start_date_obj, end_time_utc=end_date_obj, ) - if payload is not None: - if not caller_is_admin and prisma_client is not None: - await _assert_user_owns_cold_storage_payload( - prisma_client=prisma_client, - user_api_key_dict=user_api_key_dict, - payload=cast(Mapping[str, object], payload), # cast-ok: custom-logger payload is untyped - request_id=request_id, - ) - return payload + if payload is None: + continue + if not caller_is_admin and prisma_client is not None: + if not await _user_can_view_cold_storage_payload( + prisma_client=prisma_client, + user_api_key_dict=user_api_key_dict, + payload=cast(Mapping[str, object], payload), # cast-ok: custom-logger payload is untyped + ): + continue + return payload # Fallback: the list endpoint omits the heavy columns for performance, so # serve them here. When prompts were offloaded to cold storage the DB holds @@ -4462,23 +4463,24 @@ def _cold_storage_payload_owner(payload: Mapping[str, object]) -> tuple[str | No ) -async def _assert_user_owns_cold_storage_payload( +async def _user_can_view_cold_storage_payload( prisma_client: PrismaClient, user_api_key_dict: UserAPIKeyAuth, payload: Mapping[str, object], - request_id: str, -) -> None: +) -> bool: """ Authorize a cold-storage payload against the owner recorded inside it. The custom logger reads the payload straight from cold storage, written - independently of the spend-log table and able to outlive its row, so a - request_id lookup could otherwise hand back another tenant's stored payload - when no row exists for the pre-check to catch. Verifying the payload's own - owner closes that gap, and a payload that records no owner fails closed. + independently of the spend-log table and able to outlive its row, and cold + storage is keyed by provider ``request_id``, so a lookup id that also exists + as another tenant's provider id would otherwise hand back that tenant's + stored payload. Verifying the payload's own owner closes that gap; a payload + that records no owner fails closed. Callers skip a foreign-owned payload and + fall through to the scoped DB query, so the caller's own matching row is + still served when a colliding cold-storage hit is not theirs to view. """ owner_user, owner_team_id = _cold_storage_payload_owner(payload) - if not await _user_can_view_spend_log_owner(prisma_client, user_api_key_dict, owner_user, owner_team_id): - raise _spend_log_forbidden(request_id) + return await _user_can_view_spend_log_owner(prisma_client, user_api_key_dict, owner_user, owner_team_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 5d677bcdde3..4e8ba4abbef 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 @@ -2709,12 +2709,12 @@ async def test_ui_view_request_response_rejects_foreign_row_inserted_after_owner @pytest.mark.asyncio -async def test_ui_view_request_response_custom_logger_denies_foreign_payload_owner(client, monkeypatch): - """The custom-logger payload comes straight from cold storage, written independently - of the spend-log table and able to outlive its row. When an id lookup matches no row, - the DB owner pre-check has nothing to verify, so the payload is authorized against the - owner recorded inside it. A foreign tenant's stored payload is denied even though no - spend-log row exists for the pre-check to catch.""" +async def test_ui_view_request_response_custom_logger_skips_foreign_payload_owner(client, monkeypatch): + """The custom-logger payload comes straight from cold storage, keyed by provider + request_id, so a lookup id that also exists as another tenant's provider id could + otherwise hand back that tenant's stored payload. A foreign-owned payload is skipped + so the caller's own matching row is still served by the DB fallback; when no such + row exists, the endpoint returns null instead of leaking the foreign payload.""" class MockDB: async def query_raw(self, sql_query, *params): @@ -2747,7 +2747,66 @@ async def test_ui_view_request_response_custom_logger_denies_foreign_payload_own params={"start_date": "2026-01-01 00:00:00"}, headers={"Authorization": "Bearer sk-test"}, ) - assert response.status_code == 403 + assert response.status_code == 200 + assert "victim prompt" not in response.text + assert response.json() is None + finally: + app.dependency_overrides.pop(ps.user_api_key_auth, None) + + +@pytest.mark.asyncio +async def test_ui_view_request_response_custom_logger_falls_through_to_owned_db_row(client, monkeypatch): + """A colliding foreign cold-storage payload must not lock the caller out of their + own matching DB row. Skipping the foreign payload lets the scoped DB query return + the caller's own row, which the pre-check already authorized on the shared id.""" + + class MockDB: + async def query_raw(self, sql_query, *params): + if 'SELECT DISTINCT "user", team_id' in sql_query: + return [ + {"user": "user_1", "team_id": None}, + {"user": "victim_user", "team_id": None}, + ] + return [ + { + "messages": [{"role": "user", "content": "my own prompt"}], + "response": {"id": "r"}, + "proxy_server_request": None, + "metadata": None, + "user": "user_1", + "team_id": None, + } + ] + + class MockPrisma: + def __init__(self): + self.db = MockDB() + + class ColdStorageLogger: + async def get_request_response_payload(self, request_id, start_time_utc, end_time_utc): + return { + "messages": [{"role": "user", "content": "victim prompt"}], + "response": {"id": "r"}, + "metadata": {"user_api_key_user_id": "victim_user", "user_api_key_team_id": None}, + } + + monkeypatch.setattr("litellm.proxy.proxy_server.prisma_client", MockPrisma()) + monkeypatch.setattr( + litellm.logging_callback_manager, + "get_active_additional_logging_utils_from_custom_logger", + lambda: [ColdStorageLogger()], + ) + app.dependency_overrides[ps.user_api_key_auth] = lambda: UserAPIKeyAuth( + user_role=LitellmUserRoles.INTERNAL_USER, user_id="user_1" + ) + try: + response = client.get( + "/spend/logs/ui/shared-id", + params={"start_date": "2026-01-01 00:00:00"}, + headers={"Authorization": "Bearer sk-test"}, + ) + assert response.status_code == 200, response.text + assert "my own prompt" in response.text assert "victim prompt" not in response.text finally: app.dependency_overrides.pop(ps.user_api_key_auth, None) diff --git a/ui/litellm-dashboard/src/components/view_logs/RequestLogsPanel.test.tsx b/ui/litellm-dashboard/src/components/view_logs/RequestLogsPanel.test.tsx index 4f788e180d2..f4b8520341d 100644 --- a/ui/litellm-dashboard/src/components/view_logs/RequestLogsPanel.test.tsx +++ b/ui/litellm-dashboard/src/components/view_logs/RequestLogsPanel.test.tsx @@ -278,9 +278,15 @@ describe("RequestLogsPanel", () => { }); it("fetches the log by request_id and opens the drawer when it is not in the loaded page", async () => { - vi.mocked(uiSpendLogsCall).mockImplementation(async ({ params }) => + vi.mocked(uiSpendLogsCall).mockImplementation(async ({ params, page_size }) => params?.request_id === "req-old" - ? { data: [logEntry({ request_id: "req-old" })], total: 1, page: 1, page_size: 1, total_pages: 1 } + ? { + data: [logEntry({ request_id: "req-old" })], + total: 1, + page: 1, + page_size: page_size ?? 1, + total_pages: 1, + } : { data: [], total: 0, page: 1, page_size: 50, total_pages: 0 }, ); renderPanel("?log_id=req-old"); @@ -295,7 +301,43 @@ describe("RequestLogsPanel", () => { .mock.calls.find(([options]) => options.params?.request_id === "req-old")?.[0]; if (!byIdCall) throw new Error("expected a by-id uiSpendLogsCall"); expect(byIdCall.page).toBe(1); - expect(byIdCall.page_size).toBe(1); + expect(byIdCall.page_size).toBeGreaterThan(1); + }); + + it("prefers the exact request_id row over a colliding litellm_call_id row on the loaded page", async () => { + respondWith([ + logEntry({ request_id: "attacker-req", litellm_call_id: "victim-req" }), + logEntry({ request_id: "victim-req", litellm_call_id: "victim-call-id" }), + ]); + renderPanel("?log_id=victim-req"); + + await waitFor(() => { + expect(drawer()).toHaveTextContent("open"); + }); + expect(drawer()).toHaveAttribute("data-log-id", "victim-req"); + }); + + it("prefers the exact request_id row over a colliding litellm_call_id row from the by-id fetch", async () => { + vi.mocked(uiSpendLogsCall).mockImplementation(async ({ params }) => + params?.request_id === "victim-req" + ? { + data: [ + logEntry({ request_id: "attacker-req", litellm_call_id: "victim-req" }), + logEntry({ request_id: "victim-req", litellm_call_id: "victim-call-id" }), + ], + total: 2, + page: 1, + page_size: 10, + total_pages: 1, + } + : { data: [], total: 0, page: 1, page_size: 50, total_pages: 0 }, + ); + renderPanel("?log_id=victim-req"); + + await waitFor(() => { + expect(drawer()).toHaveTextContent("open"); + }); + expect(drawer()).toHaveAttribute("data-log-id", "victim-req"); }); it("opens the drawer when ?log_id= is the log's litellm_call_id rather than its request_id", async () => { diff --git a/ui/litellm-dashboard/src/components/view_logs/RequestLogsPanel.tsx b/ui/litellm-dashboard/src/components/view_logs/RequestLogsPanel.tsx index cf73b695044..eb30141d1a1 100644 --- a/ui/litellm-dashboard/src/components/view_logs/RequestLogsPanel.tsx +++ b/ui/litellm-dashboard/src/components/view_logs/RequestLogsPanel.tsx @@ -25,8 +25,11 @@ import { LiveTailBanner, LogsTableToolbar } from "./LogsTableToolbar"; import { RequestLogsTable } from "./RequestLogsTable"; const PAGE_SIZE = 50; +const BY_ID_PAGE_SIZE = 10; const DEFAULT_INTERVAL = { value: 24, unit: "hours" }; const matchesLogId = (log: LogEntry, logId: string) => log.request_id === logId || log.litellm_call_id === logId; +const findByLogId = (logs: readonly LogEntry[], logId: string): LogEntry | null => + logs.find((log) => log.request_id === logId) ?? logs.find((log) => matchesLogId(log, logId)) ?? null; interface RequestLogsPanelProps { accessToken: string; @@ -131,12 +134,12 @@ export default function RequestLogsPanel({ accessToken, token, userRole, userID, start_date: window.start_date, end_date: window.end_date, page: 1, - page_size: 1, + page_size: BY_ID_PAGE_SIZE, params: { request_id: urlLogId }, }); - return response.data.find((log) => matchesLogId(log, urlLogId)) ?? null; + return findByLogId(response.data, urlLogId); }, - enabled: urlLogId !== null && !(selectedLog !== null && matchesLogId(selectedLog, urlLogId)), + enabled: urlLogId !== null && selectedLog?.request_id !== urlLogId, staleTime: Infinity, }; @@ -144,8 +147,8 @@ export default function RequestLogsPanel({ accessToken, token, userRole, userID, const displayLog = useMemo(() => { if (urlLogId === null) return null; - if (selectedLog !== null && matchesLogId(selectedLog, urlLogId)) return selectedLog; - return filteredLogs.data.find((log) => matchesLogId(log, urlLogId)) ?? urlLog ?? null; + if (selectedLog?.request_id === urlLogId) return selectedLog; + return findByLogId(filteredLogs.data, urlLogId) ?? urlLog ?? null; }, [urlLogId, selectedLog, filteredLogs.data, urlLog]); const displaySessionId = useMemo(() => {