fix(spend_logs): store litellm_call_id and match it in request_id lookups - #39068
Merged
mateo-berri merged 16 commits intoSep 13, 2026
Merged
Conversation
…kups Success spend rows are keyed by the upstream provider response id, so the x-litellm-call-id response header value never found them. Add a nullable indexed litellm_call_id column to LiteLLM_SpendLogs, populate it at write time, and widen every request_id lookup surface (/spend/logs, /spend/logs/ui, request details, ownership check) to match either id.
mateo-berri
requested review from
ryan-crabbe-berri and
yuneng-berri
as code owners
September 1, 2026 04:37
Contributor
Contributor
Greptile SummaryThis PR stores the LiteLLM call ID alongside the provider response ID and supports either identifier across spend-log lookup surfaces
Confidence Score: 5/5The PR appears safe to merge No blocking failure remains
|
| Filename | Overview |
|---|---|
| litellm/proxy/spend_tracking/spend_management_endpoints.py | Adds dual-ID spend-log lookup while binding non-admin authorization to the exact database or cold-storage payload returned |
| litellm/proxy/spend_tracking/spend_tracking_utils.py | Persists the resolved LiteLLM call ID in spend-log payloads |
| litellm/proxy/common_request_processing.py | Bounds client-provided call IDs and generates replacements for empty or oversized values |
| litellm-proxy-extras/litellm_proxy_extras/migrations/20260831120000_add_litellm_call_id_spend_logs/migration.sql | Adds the nullable call-ID column without rewriting historical spend rows |
| litellm-proxy-extras/litellm_proxy_extras/migrations/20260831120001_spend_logs_litellm_call_id_index/migration.sql | Adds the call-ID index concurrently in a dedicated migration |
| ui/litellm-dashboard/src/components/view_logs/RequestLogsPanel.tsx | Resolves call-ID deep links by fetching and opening the matching request row |
| ui/litellm-dashboard/src/components/GuardrailsMonitor/LogViewer.tsx | Selects the exact request-ID row when lookup results contain identifier collisions |
Reviews (16): Last reviewed commit: "Merge origin/litellm_internal_staging in..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
Author
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Lookup auth checks only one row
- _assert_user_can_view_request_id now uses find_many and iterates every row the request_id/litellm_call_id OR clause resolves to, so a client-supplied litellm_call_id that collides with another tenant's id is refused (403) before any list or detail query runs.
Or push these changes by commenting:
@cursor push 1fbd2dbb69
Preview (1fbd2dbb69)
diff --git a/litellm/proxy/spend_tracking/spend_management_endpoints.py b/litellm/proxy/spend_tracking/spend_management_endpoints.py
--- a/litellm/proxy/spend_tracking/spend_management_endpoints.py
+++ b/litellm/proxy/spend_tracking/spend_management_endpoints.py
@@ -243,9 +243,14 @@
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,33 +4306,31 @@
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(
prisma_client: PrismaClient,
user_api_key_dict: UserAPIKeyAuth,
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
--- 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 @@
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 @@
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 @@
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 @@
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.You can send follow-ups to the cloud agent here.
litellm_call_id is populated from the client-settable x-litellm-call-id header, so a request_id lookup can match more than one row across tenants. Authorizing on a single arbitrary match let an attacker reuse a victim's request_id as their own call id and read the victim's spend log row. Widen the ownership check to require every matching row to belong to the caller, failing closed on any foreign match.
Contributor
|
🚅 Hi, thanks for the PR! I'm Agent Shin, the automated triage bot for this repository. What's this and why am I getting it? I read the description against our contribution rubric. Here's how it lined up: What you got right:
What's still missing:
If the description isn't updated in the next 2 hours, I'll auto-close this PR. That's not us saying we don't care about the change; we want the open-PR list to mirror what a maintainer can act on right now, so contributors don't get lost in a backlog. A closed PR is a soft "park this for later," not a rejection. Take your time; everything below still works after the close. During the grace period: just update the PR description with the missing pieces. No need to ping me; I'll re-check on the next sweep and skip the auto-close if it now passes. See what counts as QA proof for the full rubric (a linked issue alone isn't enough; it covers context, not proof). If the PR does get auto-closed in 2 hours, you still have easy recovery paths:
Internal BerriAI contributors: this rubric doesn't apply to you; ping a maintainer. (I'm an LLM, so I'm not infallible. If you think I got this wrong, ping a maintainer; they'll override me.) |
…uild call id index concurrently
Contributor
Author
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
Contributor
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 2 · PR risk: 0/10 |
Contributor
Author
Contributor
Author
Contributor
Author
The custom-logger detail branch reads the payload from cold storage, which is written independently of the spend-log table and can outlive its row. The DB owner pre-check then has nothing to verify for an id lookup that matches no row, so a foreign tenant's stored payload could be returned. Authorize the returned payload against the owner recorded inside it (metadata user/team id), failing closed when none is recorded. Also fold the three identical 403 raises into one helper.
Contributor
Author
…/litellm into litellm_spend_log_request_id_call_id
…llisions cannot deny the owner
Contributor
Author
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Deep link opens colliding call-id row
- The deep-link drawer now prefers the exact
request_idmatch over any collidinglitellm_call_id, both in thedisplayLogselection overfilteredLogsand in the by-id fetch (which now pulls enough rows to actually contain the exact-match row).
- The deep-link drawer now prefers the exact
- ✅ Fixed: Cold-storage hit blocks owned row
- The cold-storage payload ownership check is now a boolean that lets the endpoint skip a foreign-owned payload and fall through to the scoped DB fallback, so a colliding cold-storage entry no longer 403s the caller's own matching row.
Or push these changes by commenting:
@cursor push 20d4dcda13
Preview (20d4dcda13)
diff --git a/litellm/proxy/spend_tracking/spend_management_endpoints.py b/litellm/proxy/spend_tracking/spend_management_endpoints.py
--- a/litellm/proxy/spend_tracking/spend_management_endpoints.py
+++ b/litellm/proxy/spend_tracking/spend_management_endpoints.py
@@ -2901,15 +2901,16 @@
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 @@
)
-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
--- 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 @@
@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,13 +2747,72 @@
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)
+
+
+@pytest.mark.asyncio
async def test_ui_view_request_response_custom_logger_allows_own_payload_without_db_row(client, monkeypatch):
"""The payload-owner authorization must not false-deny a legitimate owner whose
spend-log row is already gone from the DB. An empty owner lookup with a cold-storage
diff --git a/ui/litellm-dashboard/src/components/view_logs/RequestLogsPanel.test.tsx b/ui/litellm-dashboard/src/components/view_logs/RequestLogsPanel.test.tsx
--- 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 @@
});
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,9 +301,45 @@
.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 () => {
respondWith([logEntry({ request_id: "chatcmpl-provider", litellm_call_id: "call-1" })]);
renderPanel("?log_id=call-1");
diff --git a/ui/litellm-dashboard/src/components/view_logs/RequestLogsPanel.tsx b/ui/litellm-dashboard/src/components/view_logs/RequestLogsPanel.tsx
--- 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 { 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 @@
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 @@
const displayLog = useMemo<LogEntry | null>(() => {
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<string | null>(() => {You can send follow-ups to the cloud agent here.
… list the exact request_id row first
…itellm_spend_log_request_id_call_id
Contributor
Author
Contributor
Author
|
bugbot run |
…itellm_spend_log_request_id_call_id # Conflicts: # litellm/proxy/spend_tracking/spend_management_endpoints.py
Contributor
Author
Contributor
Author
|
bugbot run |
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Non-internal id lookup 403s on collision
- Extended user_scope_applies to cover every non-admin id lookup (not just INTERNAL_USER roles), so the SQL scopes to rows the caller can view and a foreign litellm_call_id collision can no longer end up in the fetched set that _assert_user_owns_fetched_spend_rows 403s on.
Or push these changes by commenting:
@cursor push 72b9b0c543
Preview (72b9b0c543)
diff --git a/litellm/proxy/spend_tracking/spend_management_endpoints.py b/litellm/proxy/spend_tracking/spend_management_endpoints.py
--- a/litellm/proxy/spend_tracking/spend_management_endpoints.py
+++ b/litellm/proxy/spend_tracking/spend_management_endpoints.py
@@ -2493,7 +2493,12 @@
request_id=request_id,
)
user_scope_applies: Final = (
- not is_admin_view and team_id is None and _can_user_view_spend_log(user_api_key_dict=user_api_key_dict)
+ not is_admin_view
+ and team_id is None
+ and (
+ is_request_id_lookup
+ or _can_user_view_spend_log(user_api_key_dict=user_api_key_dict)
+ )
)
permitted_team_ids: Final = (
await _get_permitted_team_ids_for_spend_logs_or_empty(You can send follow-ups to the cloud agent here.
An org admin or an allowed_routes key reaches /spend/logs/ui without the internal-user row scope, so with either-id matching a foreign row carrying the caller's request_id as its litellm_call_id made the post-fetch owner check 403 the caller's own lookup. Every non-admin id lookup now applies the same SQL owner/team scope internal users get
Contributor
Author
Contributor
Author
|
bugbot run |
…/litellm into litellm_spend_log_request_id_call_id
Closed
7 tasks
Contributor
Author
Contributor
Author
|
bugbot run |
mateo-berri
enabled auto-merge
September 3, 2026 04:43
ryan-crabbe-berri
approved these changes
Sep 7, 2026
…id_call_id Keeps the base's rule that a non-admin id lookup matching no spend-log row answers 403, so the detail route never consults cold storage without an owner row
Contributor
Author
|
bugbot run |
Contributor
Author
mateo-berri
merged commit Sep 13, 2026
a978ad2
into
litellm_internal_staging
127 of 135 checks passed
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 3 potential issues.
Bugbot Autofix is ON, but it could not run because the branch was deleted or merged before autofix could start.
Reviewed by Cursor Bugbot for commit 8608a03. Configure here.
| if max_spend is not None: | ||
| where_conditions["spend"]["lte"] = max_spend | ||
| # A request_id lookup drops the date window, so a non-admin could otherwise | ||
| # reach any single row by id; require they own it, mirroring the detail |
Contributor
There was a problem hiding this comment.
Search omits call-id matches
Medium Severity
The logs page search box sends search, and _build_spend_log_search_condition still matches only request_id plus a few other columns. Pasting an x-litellm-call-id into that box therefore returns no row for success spend logs, even though the deep-link request_id path now finds them.
Reviewed by Cursor Bugbot for commit 8608a03. Configure here.
| return rows | ||
|
|
||
|
|
||
| async def _find_spend_log_row(prisma_client: PrismaClient, request_id: str) -> _SpendLogOwnershipRow | None: |
Contributor
There was a problem hiding this comment.
Summary lookup ignores call id
Medium Severity
GET /spend/logs with dates defaults to summarize=true and _spend_logs_daily_summary_sql still filters only request_id. The same handler’s unsummarized path now matches litellm_call_id, so a call-id lookup that works without dates returns no spend once a date window is supplied.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 8608a03. Configure here.
| status=_get_status_for_spend_log( | ||
| metadata=metadata, | ||
| ), | ||
| litellm_call_id=litellm_call_id, |
Contributor
There was a problem hiding this comment.
Spend writes skip call-id cap
Medium Severity
litellm_call_id is now written to an indexed column, but get_logging_payload stores kwargs as-is. Only ProxyBaseLLMRequestProcessing.pre_call runs resolve_litellm_call_id. MCP, A2A, and batch paths that set a long id can still hit the btree row-size failure this PR is meant to stop, so those spend rows are dropped.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 8608a03. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TLDR
Problem this solves:
x-litellm-call-idheader never finds a success spend rowHow it solves it:
litellm_call_idcolumn on LiteLLM_SpendLogs?log_id=deep link includedallowed_routeskeys included, not only internal users), so a colliding foreign row can neither leak nor turn the caller's own lookup into a 403request_idis the clicked id, so a client-set call id colliding with it cannot swap another request into the drawer?log_id=deep link and any id-filtered list, plain or session-grouped, put the exactrequest_idrow first, so a newer request carrying that id as its call id cannot take its placeUser Flow
Before: a developer saves the
x-litellm-call-idresponse header, and looking the request up with it returns nothingx-litellm-call-id: 7b3e1d99-8d5a-4e51-b186-80d0bed7c257off the response headers[], no matter how long they waitDVOWavnjJ46kq8YP67TZ6QE); for /v1/responses that raw id appears nowhere in the client-visible response, so those rows cannot be looked up at allAfter: the same header value finds the row on every lookup surface
x-litellm-call-id: 7b3e1d99-8d5a-4e51-b186-80d0bed7c257off the response headersRelevant issues
Related to #25952 (covers only client-supplied request ids, not the ids LiteLLM generates)
Linear ticket
Resolves LIT-6302
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
uv run pytest tests/test_litellm/<your_test_file>.py -v. Leave the suites (make test-unit-*,make test-unit) to CI: it finishes in ~15 minutes where a laptop takes an hour or more@greptileaito re-request a review after pushing changes)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
Setup shared by both runs: proxy booted with
--num_workers 2for the before run and--num_workers 1for the after run, a fresh Postgres database per run,gemini-3.8-flash->vertex_ai/gemini-3.8-flash(vertex_location: global) plustext-embedding-005-lit6302->vertex_ai/text-embedding-005, real Vertex AI calls,store_prompts_in_spend_logs: true. Requests use the master key unless a step names a user key. The before proxy runs the merge base4049a075bd, the after proxy runs this PR's tip.Before (4049a07)
Each of the four shapes returned HTTP 200 with an
x-litellm-call-idheader, and none of those ids resolved anything on any surface:The provider-id lookup is the only one that works, and /v1/messages and /v1/responses expose no id that matches the stored one.
Logs page deep link (dashboard build without this PR's panel change, pointed at the after proxy so the backend already answers the call id):
/ui/logs?log_id=7d9a2d5d-41e1-4339-8272-b9067fb587d4renders the full list (93 rows) with no request drawer, while typing the same id into the page's search box does list the rowIYeYatSdKoTcodAP_p2BwAI("Showing 1-1 of 1"). Screenshots:lit6302-before-ui-deeplink-callid.png,lit6302-before-ui-search-callid.pngAfter (55853c1)
Boot applied
20260831120000_add_litellm_call_id_spend_logsthen20260831120001_spend_logs_litellm_call_id_index(the CREATE INDEX CONCURRENTLY migration), no CONCURRENTLY error, no INVALID index.Feature flow (master key)
Every
x-litellm-call-idresolves on all three surfaces,/spend/logs/ui/<id>returns the row whoseresponse.idis the provider id, and the provider-id lookup still works:Tenant safety (internal users, client-settable call id)
The attacker's spoofed call id never surfaces the victim's row to the attacker, the victim keeps resolving their own row, a bystander gets 403, the admin detail view resolves the exact
request_idmatch first, and an id-filtered list withpage_size=1(the fetch the logs page deep link makes) returns the exactrequest_idrow rather than the newer collided one, on both the plain list and the session-grouped list the logs page defaults to:Bounded call id (Postgres btree entry limit)
A 3000-character header no longer poisons the write: the proxy replaces it with a generated id, echoes that id, and the row is stored and found under it (at the merge base the same request left a Postgres 54000
index row size 3016 exceeds btree version 4 maximum 2704in the proxy log and no row):Logs page deep link
Dashboard dev server from this branch against the after proxy:
http://localhost:<ui port>/logs?log_id=7d9a2d5d-41e1-4339-8272-b9067fb587d4opens the drawer "Request IYeYatSdKoTcodAP_p2BwAI details" (gemini-3.8-flash, $0.001208, 326 tokens) while the list behind it still shows "Showing 1-50 of 93" without that row, i.e. the drawer came from the id fetch, not from the loaded page. Screenshot:lit6302-after-ui-deeplink-callid-drawer.png.RequestLogsPanel.test.tsxcovers both the loaded-page and the fetched-by-id paths and fails with the panel change reverted.To reproduce by hand: run the proxy from this branch, open
/ui/logs, send any request, copy itsx-litellm-call-idresponse header, then open/ui/logs?log_id=<that id>Notes
/spend/logs/ui/{request_id}rejects internal users with 401 before the handler runs, on both legs: the route is not ininternal_user_routes, RBAC that predates this PR; the handler's ownership check guards whichever non-admin principals can reach the route/spend/logs/uidate filters are UTC; local-time windows silently return emptymessagescolumn stores{}for chat rows on both legs (only_arealtimerows keep messages there; prompts live inproxy_server_request), unrelated to this PRrequest_id = litellm_call_id, so an HTTP 200 on every leg is what proves the success-row path/spend/logs/ui/{request_id}resolves from the spend row on every leg; the custom-logger path is covered bytest_ui_view_request_response_custom_logger_is_keyed_by_callers_own_request_idand listed under Not verified belowCircleCI at 55853c1
86 checks green and 3 red, and all three reds are the GitHub Actions
Image Scanjobs (runtime-image,ui-image,image-scan). They build the Docker image off the PR merge ref and fail innpm run buildwithCannot find module '../../../../litellm/proxy/public_endpoints/autorouter_presets.json': staging's #39412 moved that JSON out ofui/while a dashboard test mock still imported it by a relative path the ui-builder stage never copies, so every PR merge ref since #39412 landed fails the same way. Nothing in this PR touches that path, they are not required checks, and staging's #39478 has since fixed the import for everyone, so these reds only survive because the run predates that merge. Theproxy-endpointsred on the previous tip was staging's own crowdstrike cadence expectation, fixed by #39467 and green here since this branch merged staging inType
🐛 Bug Fix
Caveats (if any)
Medium
Low
request_idmatch, and id-filtered lists put the exactrequest_idrow firstx-litellm-call-idlonger than 256 characters or empty is replaced by a generated id_cache_hit<ts>suffix; the new column keeps the plain call idresp_...id still never resolvesFinal Attestation
/live-pr-risk report (b69351e, merge-ref legs re-run at 55853c1)
The graph walk and the base-vs-head rig ran at b69351e. The only commit since is the staging merge, and
git diff <merge-base>...<tip>at 55853c1 is byte-identical to the same diff at b69351e apart from blob hashes and one hunk offset, so every finding below still describes what ships. The merge-ref legs (all four request shapes plus the tenant-safety and bounded-call-id checks) were re-run on the merged tree at 55853c1 and are the output quoted above. The later staging merge at 8608a03 resolved one conflict in the spend-log owner lookup by keeping the base's own missing-row 403 (#34099), so it adds no PR-side behavior; the three touched test modules were re-run on the merged tree (784 passed)Verdict: no breaking dependent found; two dashboard consumers and one CircleCI fixture depended on the old single-id contract and are fixed in this PR
Breaking
/spend/logs?request_id=<provider id>returns the identical row on both legsBackward incompatible
SpendLogsPayloadcarries a newlitellm_call_idkey, so every consumer of the raw payload sees one more field: theSPEND_LOGS_URLbatch receiver, the GCS Pub/Sub exporter (litellm/integrations/gcs_pubsub/pub_sub.py), and the spend update writer. The CircleCI Pub/Sub test compares the exported dict key by key and failed on the extra key; itsignored_keysnow lists the new fieldallowed_routeskeys included, not only internal users), so a colliding foreign row can neither leak nor turn the caller's own lookup into a 403x-litellm-call-idvalues over 256 characters or empty are replaced by a generated id; the response header echoes the stored valueRegression risk
previous_response_idlookup (litellm/responses/session_handler.py) still matchesrequest_idonly, unchanged by this PR and untouched by the new columngcs_bucket.pykeys objects by{date}/{provider response id}); the detail route now asks the logger for the storedrequest_idof the caller's own row instead of the raw lookup id, which is what the key builder always keyed by, so call-id lookups now reach the bucket too; not exercised live (no bucket credentials in this run), reasoned from the key builder and covered by a unit test with a recording loggerDependency graph
/spend/logs,/spend/logs/ui,/spend/logs/ui/{request_id},/spend/logs/v2: verified live on both legs (feature and tenant scripts above)RequestLogsPanel?log_id=deep link: was untested and read the loaded page only, fixed here, covered byRequestLogsPanel.test.tsxand verified on the dev dashboardGuardrailsMonitor/LogViewerdrawer: was untested and took the first returned row, fixed here, covered byLogViewer.test.tsxtests/logging_callback_tests/test_gcs_pub_sub.py: CircleCI job, failed on the new key, fixed here and re-run locally with the premium check satisfied_create_spend_logs_with_poison_isolation: verified live (oversized header at the merge base rejected the row with Postgres 54000, isolated from the batch; at the tip the id is replaced before the write)session_handler.pyprevious_response_id matching, cold-storage key builder, autorouter session rollup: unchanged, reasoned onlyget_request_response_payloadimplementers (additional_logging_utils.pybase,gcs_bucket.py,datadog.py,datadog_metrics.py): the detail handler now passes the stored provider id of the caller's own row; GCS keys by that id, the Datadog ones return nothing; reasoned only, unit-tested with a recording logger_spend_log_payload_queryconsumers (the detail handler and the test fakes): the SELECT gainsrequest_id, a row without it falls back to the lookup id; verified live through/spend/logs/ui/{id}ui_view_spend_logsORDER BY under an id filter (/spend/logs/uiand/spend/logs/v2share the handler, the count query is unchanged, the exact-first clause only applies to a string filter and sits on both the plain page and the session-grouped page that fix(ui): paginate request logs by session groups server-side #39257 added on staging): verified live (tenant steps 7b, 7c, and 7d) and unit-tested for both page shapesuser_scope_appliesnow covers every non-admin id lookup. The pre-check and the SQL scope share one team rule (_get_permitted_team_ids_for_spend_logs), callers without auser_idalready answer 403 at the pre-check on both legs, and the only callers newly scoped are user-table rows whose role is outside the four/user/newand/user/updateaccept (org_admin,team,customer), which no API writes today. Covered bytest_ui_view_spend_logs_id_lookup_scopes_every_non_admin_role, which answers 403 without the fixNot verified
/spend/logs/ui/{request_id}custom-logger path (no bucket in this run: the only GCP service account available has no storage permission, a bucket list answers 403); the custom-logger path is covered bytest_ui_view_request_response_custom_logger_is_keyed_by_callers_own_request_idwith a recording loggerSPEND_LOGS_URLexternal receiver (no receiver configured in this run)org_adminby hand (no API writes that role there) is refused at the route check with HTTP 401 on both/spend/logsand/spend/logs/ui(Only proxy admin can be used to generate, delete, update info for new keys/users/teams. Route=/spend/logs/ui. Your role=org_admin) before the handler runs, so the new scope branch was observed only through the unit testNote
Medium Risk
Changes spend-log query and authorization paths (multi-tenant id collisions, cold-storage detail) plus a production DB migration/index; behavior is heavily tested but mis-scoping could leak or deny log access.
Overview
Adds a persisted
litellm_call_idon spend logs (schema + migrations, including a concurrent index) and writes it from request processing.x-litellm-call-idis normalized viaresolve_litellm_call_id(empty or >256 chars → generated UUID) before storage and response headers.Spend and UI lookups that used
request_idonly now matchrequest_idORlitellm_call_id(API/spend/logs, UI list, detail,?log_id=). Lists prefer the row whoserequest_idequals the lookup id when both collide.Because the call id is client-set and not tenant-unique, non-admin id lookups keep user/team scoping, use uncapped owner discovery, re-check ownership on fetched rows, resolve the caller’s row before cold-storage/custom loggers (provider
request_idas the storage key), and authorize logger payloads by embedded owner metadata.Dashboard
RequestLogsPanel/ GuardrailsLogViewerresolve drawers by either id and prefer the exactrequest_idmatch.Reviewed by Cursor Bugbot for commit 8608a03. Bugbot is set up for automated code reviews on this repo. Configure here.