mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-07 08:26:10 +00:00
fix(proxy): resolve log-id collisions on deep-link drawer and cold-storage payload lookup
- prefer exact request_id match over litellm_call_id collision in the request-logs deep-link drawer and fetch enough rows to find the exact match
- skip a foreign-owned cold-storage payload on GET /spend/logs/ui/{request_id} so the scoped DB fallback still serves the caller's own matching row
This commit is contained in:
parent
0f628f78e7
commit
20d4dcda13
4 changed files with 139 additions and 33 deletions
|
|
@ -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(
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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 () => {
|
||||
|
|
|
|||
|
|
@ -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<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>(() => {
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue