fix(proxy): close project hijacking and key org IDOR (VERIA-55) - #27011
fix(proxy): close project hijacking and key org IDOR (VERIA-55)#27011yuneng-berri merged 1 commit into
Conversation
Two related authorization gaps in management endpoints: 1. `/project/update` evaluated permission against the team_id supplied in the request body. By passing `data.team_id` pointing at a team they admin, a caller could hijack any project — `_check_user_permission_for_project` was given the attacker's team_object and happily checked admin membership against that. Drop the team_object kwarg so the helper re-fetches the existing project's team. Also require admin rights on the destination team when reassigning a project across teams, so a team admin cannot shed projects into another team's namespace. 2. `/key/update` accepted any `organization_id` and only checked that the org existed before applying limits. A caller could thereby point their key at an arbitrary org. Add `_validate_caller_can_assign_key_org` which enforces the same membership rule already applied on the `/key/list` filter path (`validate_key_list_check`); proxy admins and no-change updates skip the check. Tests cover both helpers in isolation: existing-team-admin allow, unrelated-team admin deny, proxy-admin shortcut, org-member allow, non-member deny, missing user_id deny, no-memberships deny. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR closes two authorization gaps: a confused-deputy bug in Confidence Score: 4/5Safe to merge; both security fixes are logically correct with no regressions identified. All findings are P2 (minor inefficiency and test-coverage gaps). The core logic of both fixes is correct: the project permission check uses the existing team ID, the reassignment double-check is properly gated, and the org membership validation mirrors the established pattern on the key/list path. No files require special attention beyond the minor noted observations.
|
| Filename | Overview |
|---|---|
| enterprise/litellm_enterprise/proxy/management_endpoints/project_endpoints.py | Fixes confused-deputy bug in update_project: permission check now uses existing_project.team_id instead of caller-supplied data.team_id; adds second permission check for destination team on reassignment. Minor: target_team_obj is fetched but not reused for the initial permission call in the no-reassignment branch. |
| litellm/proxy/management_endpoints/key_management_endpoints.py | Adds _validate_caller_can_assign_key_org and calls it in _validate_update_key_data to block non-admins from pointing a key at an org they don't belong to. Logic is correct; new function uses a direct uncached DB query instead of the shared user-object helper. |
| tests/test_litellm/proxy/management_endpoints/test_project_org_authz.py | New test file with 7 unit tests covering both helpers in isolation using mocks only (no real network calls). Covers the core allow/deny cases but lacks an integration-level test for the full update_project flow that proves the original confused-deputy path is blocked. |
Reviews (1): Last reviewed commit: "fix(proxy): close project hijacking and ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| target_team_id = data.team_id or existing_project.team_id | ||
| target_team_obj = None | ||
| if target_team_id is not None: | ||
| target_team_obj = await _validate_team_exists( | ||
| team_id=target_team_id, prisma_client=prisma_client | ||
| ) | ||
|
|
||
| # Check if user has permission to update this project | ||
| has_permission = await _check_user_permission_for_project( | ||
| user_api_key_dict=user_api_key_dict, | ||
| team_id=existing_project.team_id, | ||
| prisma_client=prisma_client, | ||
| team_object=( | ||
| LiteLLM_TeamTable(**team_obj_for_checks.model_dump()) | ||
| if team_obj_for_checks | ||
| else None | ||
| ), | ||
| ) |
There was a problem hiding this comment.
When data.team_id equals existing_project.team_id (the caller sends the same team, no reassignment), target_team_obj is already fetched via _validate_team_exists for exactly that team. The subsequent _check_user_permission_for_project call at line 602 omits team_object, so the helper issues another litellm_teamtable.find_unique for the same ID. Passing target_team_obj to the initial permission call when no reassignment is happening would eliminate this duplicate round-trip.
Note: when data.team_id != existing_project.team_id the extra fetch is intentional — target_team_obj is for the destination team, not the source — so the deduplication only applies to the no-reassignment branch.
| ) | ||
|
|
||
| user_row = await prisma_client.db.litellm_usertable.find_unique( | ||
| where={"user_id": user_api_key_dict.user_id}, | ||
| include={"organization_memberships": True}, | ||
| ) | ||
| memberships = ( | ||
| getattr(user_row, "organization_memberships", None) if user_row else None | ||
| ) | ||
| member_org_ids = { | ||
| membership.organization_id |
There was a problem hiding this comment.
_validate_caller_can_assign_key_org calls prisma_client.db.litellm_usertable.find_unique directly, bypassing any in-process cache. Each /key/update call that changes organization_id will pay a full round-trip for the user row. Other membership lookups in this codebase (e.g., validate_key_list_check) use get_user_object or a shared cache. Aligning with that pattern would avoid the extra latency and keep DB access consistent.
|
|
||
| # Project lives on team-A, caller is admin only of team-B. | ||
| prisma = _make_prisma_with_team(team_id="team-A", admins=["alice"]) | ||
| caller = UserAPIKeyAuth( | ||
| user_id="bob", | ||
| user_role=LitellmUserRoles.INTERNAL_USER.value, | ||
| ) | ||
|
|
||
| has_perm = await _check_user_permission_for_project( | ||
| user_api_key_dict=caller, | ||
| team_id="team-A", | ||
| prisma_client=prisma, | ||
| ) | ||
| assert has_perm is False | ||
| prisma.db.litellm_teamtable.find_unique.assert_awaited_once() | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_project_perm_check_allows_team_admin_of_existing_team(): |
There was a problem hiding this comment.
The unit tests verify _check_user_permission_for_project and _validate_caller_can_assign_key_org in isolation, which is useful. However, the core fix in update_project is that the function is now called with team_id=existing_project.team_id instead of team_id=data.team_id. A test that drives the full update_project code path — mocking the DB to return a project belonging to team-A while the attacker sends data.team_id="team-B" where they are an admin — would directly prove the confused-deputy attack is blocked rather than inferring it from the helper's behaviour.
…team-hijack fix(proxy): close project hijacking and key org IDOR (VERIA-55)
2 participants
Summary
Two related authorization gaps in management endpoints:
/project/update— cross-team project hijacking_check_user_permission_for_projectwas being called withteam_object=team_obj_for_checks, whereteam_obj_for_checkswas fetched from the caller-supplieddata.team_id. Any team admin could passdata.team_id=<team-they-admin>while addressing a project that lived under a different team, and the helper would happily check admin membership against the attacker's team — confused-deputy-style.Fix:
team_objectkwarg from the permission check so the helper falls through to its DB-fetch path against the project's existing team.data.team_id != existing_project.team_id), separately verify the caller is admin of the destination team. Without this, a team admin could shed projects into someone else's namespace./key/update— secondary IDOR onorganization_idThe org-limit branch fetched the org by id and checked limits, but never checked that the caller actually belonged to the target org. Any user could update their key's
organization_idto point at any org. Spend-tracking happens to read org from the team association, so the budget-exhaustion impact is mitigated, but the IDOR itself is real.Fix: add
_validate_caller_can_assign_key_org, called whendata.organization_idis non-None and changes from the existing key's value. It mirrors the membership rule already enforced on the/key/listfilter path (validate_key_list_check). Proxy admins skip the check.Behavior changes
data.team_id.organization_idto an org they are not a member of.Test plan
uv run pytest tests/test_litellm/proxy/management_endpoints/test_project_org_authz.py -q— 7 pass (3 project, 4 key/org)uv run black --checkon touched filesType
🐛 Bug Fix
✅ Test