[Fix] /spend/logs: align filter handling with user scoping#25594
[Fix] /spend/logs: align filter handling with user scoping#25594yuneng-berri merged 3 commits intolitellm_yj_apr_11from
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR fixes a user-scoping bypass in Confidence Score: 5/5Safe to merge — the security fix is correctly applied across all code paths and previous review concerns have been addressed. Both previous P1 concerns (missing date-range token hashing, no unit tests) are resolved. The AND-combination logic is correct in all three branches (date-range/find_many, date-range/group_by, non-date-range/find_many). The only remaining gap is the absence of a test for the summarize=True group_by path, which is P2 since the fix there is symmetric and the code is straightforward. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/spend_tracking/spend_management_endpoints.py | Security fix: replaces elif-chain with independent if-blocks so user_id scoping is AND-combined with api_key/request_id in both date-range and non-date-range paths; token hashing also added to date-range path. |
| tests/test_litellm/proxy/spend_tracking/test_spend_management_endpoints.py | Adds five targeted mock tests verifying user+api_key, user+request_id, and sk- hashing for both date-range and non-date-range paths; no real network calls. |
Reviews (3): Last reviewed commit: "refactor: rename filter var to satisfy m..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if api_key is not None and isinstance(api_key, str): | ||
| filter_query["api_key"] = api_key # type: ignore |
There was a problem hiding this comment.
The new non-date-range branch correctly hashes tokens that start with "sk-" before filtering, but the date-range path still stores the raw value. Because the DB stores hashed keys, a call with a raw API key plus a date range will match zero records. The fix is to apply the same hashing logic used below (lines 2381-2385).
| else: | ||
| spend_logs = await prisma_client.get_data( | ||
| table_name="spend", query_type="find_all" | ||
| ) | ||
| filter_query: Dict[str, Any] = {} | ||
| if api_key is not None and isinstance(api_key, str): | ||
| if api_key.startswith("sk-"): | ||
| hashed_token = prisma_client.hash_token(token=api_key) | ||
| else: | ||
| hashed_token = api_key | ||
| filter_query["api_key"] = hashed_token | ||
| if request_id is not None and isinstance(request_id, str): | ||
| filter_query["request_id"] = request_id | ||
| if user_id is not None and isinstance(user_id, str): | ||
| filter_query["user"] = user_id | ||
|
|
||
| if not filter_query: | ||
| spend_logs = await prisma_client.get_data( | ||
| table_name="spend", query_type="find_all" | ||
| ) | ||
| return spend_logs | ||
|
|
||
| return spend_logs | ||
| data = await prisma_client.db.litellm_spendlogs.find_many( | ||
| where=filter_query, # type: ignore | ||
| order={"startTime": "desc"}, | ||
| ) | ||
| return data |
There was a problem hiding this comment.
The PR fixes the scoping bypass for non-admin callers but adds no unit tests for the view_spend_logs endpoint covering the fixed behavior. Specifically, there are no tests verifying that an INTERNAL_USER calling with api_key or request_id still gets results scoped to their own user, and that calling with another user's request id returns empty. Per project guidelines, new security-relevant code paths should have corresponding unit tests.
…ests Brings the date-range branch in line with the non-date-range branch which already hashes sk- prefixed tokens before querying. Adds coverage for filter-combination behavior in view_spend_logs.
yuneng-berri
temporarily deployed
to
integration-postgres
GitHub Actions
Inactive
yuneng-berri
temporarily deployed
to
integration-postgres
GitHub Actions
Inactive
yuneng-berri
had a problem deploying
to
integration-postgres
GitHub Actions
Error
yuneng-berri
had a problem deploying
to
integration-postgres
GitHub Actions
Error
yuneng-berri
temporarily deployed
to
integration-postgres
GitHub Actions
Inactive
yuneng-berri
temporarily deployed
to
integration-postgres
GitHub Actions
Inactive
yuneng-berri
temporarily deployed
to
integration-postgres
GitHub Actions
Inactive
yuneng-berri
had a problem deploying
to
integration-postgres
GitHub Actions
Error
1 participant
Relevant issues
Summary
The
/spend/logsendpoint acceptedapi_key,request_id, anduser_idquery parameters as mutually exclusive filters (if/elif chain). When a non-admin caller'suser_idwas forced to their own identity earlier in the handler, passingapi_keyorrequest_idwould route into a different branch and theuser_idfilter would not be applied.Fix
ifstatements souser_id,api_key, andrequest_idcombine with AND semantics on the samefilter_query.api_key/request_id/user_id/elsebranches are collapsed into a single combinedfind_manycall built from a mergedfilter_query. The no-filterelsecase still falls through to the existingget_datacall.Testing
:4000:request_idorapi_key, in both the date-range and non-date-range paths.request_id,api_key, and date range.poetry run pytest tests/test_litellm/proxy/spend_tracking/test_spend_management_endpoints.py -v— 56 passed.Type
🐛 Bug Fix
Screenshots