diff --git a/litellm/proxy/management_endpoints/organization_endpoints.py b/litellm/proxy/management_endpoints/organization_endpoints.py index 442fae2a4fa..ee683f322a1 100644 --- a/litellm/proxy/management_endpoints/organization_endpoints.py +++ b/litellm/proxy/management_endpoints/organization_endpoints.py @@ -500,6 +500,21 @@ async def update_organization( if data.updated_by is None: data.updated_by = user_api_key_dict.user_id + if data.organization_id is None: + raise HTTPException( + status_code=400, + detail={"error": "organization_id is required"}, + ) + + # IDOR guard: only proxy admins / org admins of THIS org may update + # it. Without this, any authenticated key holder could rewrite + # another organization's metadata, budgets, and object permissions. + await _verify_org_access( + organization_id=data.organization_id, + user_api_key_dict=user_api_key_dict, + prisma_client=prisma_client, + ) + existing_organization_row = ( await prisma_client.db.litellm_organizationtable.find_unique( where={"organization_id": data.organization_id}, @@ -909,6 +924,16 @@ async def organization_member_add( if prisma_client is None: raise HTTPException(status_code=500, detail={"error": "No db connected"}) + # IDOR guard: docstring says "Only proxy_admin or org_admin of + # organization, allowed to access this endpoint" — but the code + # never enforced that. Any authenticated key holder could add + # members to any org. Now gated explicitly. + await _verify_org_access( + organization_id=data.organization_id, + user_api_key_dict=user_api_key_dict, + prisma_client=prisma_client, + ) + # Check if organization exists existing_organization_row = ( await prisma_client.db.litellm_organizationtable.find_unique( @@ -1018,6 +1043,16 @@ async def organization_member_update( detail={"error": CommonProxyErrors.db_not_connected_error.value}, ) + # IDOR guard: only proxy admins / org admins of THIS org may + # update member roles. The PROXY_ADMIN-target check below was + # the only access control; without this, any authenticated user + # could change any non-admin member's role in any org. + await _verify_org_access( + organization_id=data.organization_id, + user_api_key_dict=user_api_key_dict, + prisma_client=prisma_client, + ) + # Check if organization exists existing_organization_row = ( await prisma_client.db.litellm_organizationtable.find_unique( @@ -1179,6 +1214,15 @@ async def organization_member_delete( detail={"error": CommonProxyErrors.db_not_connected_error.value}, ) + # IDOR guard: only proxy admins / org admins of THIS org may + # delete members. Without this, any authenticated key holder + # could remove any user from any org. + await _verify_org_access( + organization_id=data.organization_id, + user_api_key_dict=user_api_key_dict, + prisma_client=prisma_client, + ) + if data.user_email is not None and data.user_id is None: existing_user_email_row = await find_member_if_email( data.user_email, prisma_client diff --git a/litellm/proxy/management_endpoints/team_callback_endpoints.py b/litellm/proxy/management_endpoints/team_callback_endpoints.py index a6c0c7dcc0e..17d86410a0f 100644 --- a/litellm/proxy/management_endpoints/team_callback_endpoints.py +++ b/litellm/proxy/management_endpoints/team_callback_endpoints.py @@ -19,6 +19,7 @@ from litellm._uuid import uuid from litellm.proxy._types import ( AddTeamCallback, LiteLLM_AuditLogs, + LiteLLM_TeamTable, LitellmTableNames, ProxyErrorTypes, ProxyException, @@ -26,6 +27,7 @@ from litellm.proxy._types import ( UserAPIKeyAuth, ) from litellm.proxy.auth.user_api_key_auth import user_api_key_auth +from litellm.proxy.management_endpoints.team_endpoints import _verify_team_access from litellm.proxy.management_helpers.utils import management_endpoint_wrapper router = APIRouter() @@ -207,6 +209,15 @@ async def add_team_callbacks( }, ) + # IDOR guard: only proxy admins / org admins / team admins of THIS + # team may write callback credentials. Without this, any + # authenticated key holder could overwrite another team's logging + # config (and read back the credentials they wrote). + await _verify_team_access( + team_obj=LiteLLM_TeamTable(**_existing_team.model_dump()), + user_api_key_dict=user_api_key_dict, + ) + # store team callback settings in metadata team_metadata = _existing_team.metadata team_callback_settings: List[dict] = team_metadata.get( @@ -316,6 +327,14 @@ async def disable_team_logging( detail={"error": f"Team id = {team_id} does not exist."}, ) + # IDOR guard: only proxy admins / org admins / team admins of THIS + # team may disable its logging — otherwise any authenticated key + # holder can silence audit logging for any team. + await _verify_team_access( + team_obj=LiteLLM_TeamTable(**_existing_team.model_dump()), + user_api_key_dict=user_api_key_dict, + ) + # Update team metadata to disable logging team_metadata = _existing_team.metadata before_metadata = copy.deepcopy(team_metadata) @@ -364,20 +383,18 @@ async def disable_team_logging( }, } + except HTTPException: + # Legitimate 4xx (e.g. 403 from the access guard, 404 for an + # unknown team). Re-raise without the error-level log noise that + # the catch-all branch below would produce. + raise + except ProxyException: + raise except Exception as e: verbose_proxy_logger.error( f"litellm.proxy.proxy_server.disable_team_logging(): Exception occurred - {str(e)}" ) verbose_proxy_logger.debug(traceback.format_exc()) - if isinstance(e, HTTPException): - raise ProxyException( - message=getattr(e, "detail", f"Internal Server Error({str(e)})"), - type=ProxyErrorTypes.internal_server_error.value, - param=getattr(e, "param", "None"), - code=getattr(e, "status_code", status.HTTP_500_INTERNAL_SERVER_ERROR), - ) - elif isinstance(e, ProxyException): - raise e raise ProxyException( message="Internal Server Error, " + str(e), type=ProxyErrorTypes.internal_server_error.value, @@ -437,6 +454,14 @@ async def get_team_callbacks( detail={"error": f"Team id = {team_id} does not exist."}, ) + # IDOR guard: callback metadata holds third-party API credentials + # (Langfuse / Langsmith / GCS). Only proxy admins / org admins / + # team admins of THIS team may read them. + await _verify_team_access( + team_obj=LiteLLM_TeamTable(**_existing_team.model_dump()), + user_api_key_dict=user_api_key_dict, + ) + # Retrieve team callback settings from metadata team_metadata = _existing_team.metadata team_callback_settings = team_metadata.get("callback_settings", {}) @@ -454,6 +479,13 @@ async def get_team_callbacks( }, } + except HTTPException: + # Legitimate 4xx (e.g. 403 from the access guard) — re-raise + # without the error-level log noise that the catch-all below + # would produce. + raise + except ProxyException: + raise except Exception as e: verbose_proxy_logger.error( "litellm.proxy.proxy_server.get_team_callbacks(): Exception occurred - {}".format( diff --git a/tests/test_litellm/proxy/management_endpoints/test_organization_endpoints.py b/tests/test_litellm/proxy/management_endpoints/test_organization_endpoints.py index 4501cc76636..f4470e7e83d 100644 --- a/tests/test_litellm/proxy/management_endpoints/test_organization_endpoints.py +++ b/tests/test_litellm/proxy/management_endpoints/test_organization_endpoints.py @@ -565,3 +565,133 @@ async def test_organization_info_includes_user_email(monkeypatch): membership = LiteLLM_OrganizationMembershipTable(**raw_membership) assert membership.user_email == "alice@example.com" + + +# Regression tests for IDOR fixes on org-scoped endpoints. Sibling cluster +# to GHSA-xxv2-fprq-9x93 (team callback IDOR): the same shape of "any +# authenticated key holder reaches an endpoint that takes an +# organization_id from the request body without an access guard." The +# fix routes ``update_organization``, ``organization_member_add``, +# ``organization_member_update``, and ``organization_member_delete`` +# through the existing ``_verify_org_access`` helper. + + +@pytest.fixture +def unauthorized_caller(): + from litellm.proxy._types import LitellmUserRoles, UserAPIKeyAuth + + return UserAPIKeyAuth( + user_role=LitellmUserRoles.INTERNAL_USER, + user_id="random_authenticated_user", + api_key="sk-random", + ) + + +@pytest.fixture +def patched_org_prisma(): + """Mock prisma so that find_unique returns a victim org and + get_user_object reports the caller has no org membership — so + _verify_org_access raises 403.""" + victim_row = MagicMock() + victim_row.organization_id = "org-victim" + victim_row.metadata = {} + victim_row.model_dump.return_value = {"organization_id": "org-victim"} + + caller_user = MagicMock() + caller_user.organization_memberships = [] # no admin role anywhere + + with ( + patch("litellm.proxy.proxy_server.prisma_client") as mock_prisma, + patch( + "litellm.proxy.management_endpoints.organization_endpoints.get_user_object", + new_callable=AsyncMock, + return_value=caller_user, + ), + patch( + "litellm.proxy.proxy_server.user_api_key_cache", + ), + patch("litellm.proxy.proxy_server.proxy_logging_obj"), + ): + mock_prisma.db.litellm_organizationtable.find_unique = AsyncMock( + return_value=victim_row + ) + yield mock_prisma + + +@pytest.mark.asyncio +async def test_organization_member_add_rejects_unauthorized_caller( + patched_org_prisma, unauthorized_caller +): + # ``organization_member_add`` catches HTTPException in its + # catch-all and re-wraps as ProxyException with the original status + # code preserved. + from litellm.proxy._types import ( + OrganizationMemberAddRequest, + OrgMember, + ProxyException, + ) + from litellm.proxy.management_endpoints.organization_endpoints import ( + organization_member_add, + ) + from unittest.mock import Mock + + from fastapi import Request + + data = OrganizationMemberAddRequest( + organization_id="org-victim", + member=OrgMember(role="internal_user", user_id="attacker-user"), + ) + + with pytest.raises((HTTPException, ProxyException)) as exc: + await organization_member_add( + data=data, + http_request=Mock(spec=Request), + user_api_key_dict=unauthorized_caller, + ) + code = getattr(exc.value, "status_code", None) or getattr(exc.value, "code", None) + assert int(code) == 403 + + +@pytest.mark.asyncio +async def test_organization_member_update_rejects_unauthorized_caller( + patched_org_prisma, unauthorized_caller +): + from litellm.proxy._types import OrganizationMemberUpdateRequest + from litellm.proxy.management_endpoints.organization_endpoints import ( + organization_member_update, + ) + + data = OrganizationMemberUpdateRequest( + organization_id="org-victim", + user_id="some-other-user", + role="org_admin", + ) + + with pytest.raises(HTTPException) as exc: + await organization_member_update( + data=data, + user_api_key_dict=unauthorized_caller, + ) + assert exc.value.status_code == 403 + + +@pytest.mark.asyncio +async def test_organization_member_delete_rejects_unauthorized_caller( + patched_org_prisma, unauthorized_caller +): + from litellm.proxy._types import OrganizationMemberDeleteRequest + from litellm.proxy.management_endpoints.organization_endpoints import ( + organization_member_delete, + ) + + data = OrganizationMemberDeleteRequest( + organization_id="org-victim", + user_id="some-other-user", + ) + + with pytest.raises(HTTPException) as exc: + await organization_member_delete( + data=data, + user_api_key_dict=unauthorized_caller, + ) + assert exc.value.status_code == 403 diff --git a/tests/test_litellm/proxy/management_endpoints/test_team_callback_endpoints.py b/tests/test_litellm/proxy/management_endpoints/test_team_callback_endpoints.py index 6c4d1be7831..55afd4061d8 100644 --- a/tests/test_litellm/proxy/management_endpoints/test_team_callback_endpoints.py +++ b/tests/test_litellm/proxy/management_endpoints/test_team_callback_endpoints.py @@ -1,71 +1,205 @@ """ -Audit-log emission for the team-callback admin endpoints. +Regression tests for team callback endpoint access control and audit logging. -The endpoints in ``team_callback_endpoints.py`` mutate a team's logging -callbacks (``add_team_callbacks``) or zero them out entirely -(``disable_team_logging``). Both are admin-only mutations, and the -disable variant is itself a logging-control action, so when the operator -has Enterprise audit logging enabled (``litellm.store_audit_logs = True``) -each call must emit a row that captures who did it and what the metadata -looked like before/after. +The team callback endpoints mutate or expose callback credentials. They must +enforce target-team management access and, when audit logging is enabled, emit +redacted audit rows for callback mutations. """ import json -from unittest.mock import AsyncMock, MagicMock, patch +from unittest.mock import AsyncMock, MagicMock, Mock, patch import pytest -from fastapi import Request +from fastapi import HTTPException, Request import litellm from litellm.proxy._types import ( AddTeamCallback, LitellmTableNames, + LitellmUserRoles, UserAPIKeyAuth, ) from litellm.proxy.management_endpoints.team_callback_endpoints import ( add_team_callbacks, disable_team_logging, + get_team_callbacks, ) +def _team_row( + *, + team_id: str = "team-victim", + metadata: dict | None = None, + admin_user_id: str = "victim_admin", + organization_id: str = "org-victim", +) -> MagicMock: + row = MagicMock() + row.team_id = team_id + row.metadata = metadata or {} + row.model_dump.return_value = { + "team_id": team_id, + "team_alias": "victim-team", + "members_with_roles": [ + {"role": "admin", "user_id": admin_user_id}, + ], + "organization_id": organization_id, + "metadata": row.metadata, + } + return row + + +def _patch_prisma(existing_team: MagicMock): + mock_prisma = MagicMock() + mock_prisma.get_data = AsyncMock(return_value=existing_team) + + updated_row = MagicMock() + updated_row.team_id = existing_team.team_id + mock_prisma.db.litellm_teamtable.update = AsyncMock(return_value=updated_row) + return mock_prisma + + def _admin_auth() -> UserAPIKeyAuth: return UserAPIKeyAuth( api_key="hashed", user_id="admin-user", - user_role="proxy_admin", + user_role=LitellmUserRoles.PROXY_ADMIN, ) -def _existing_team_row(metadata: dict) -> MagicMock: - row = MagicMock() - row.team_id = "team-1" - row.metadata = metadata - return row +@pytest.fixture +def unauthorized_caller(): + return UserAPIKeyAuth( + user_role=LitellmUserRoles.INTERNAL_USER, + user_id="random_authenticated_user", + api_key="sk-random", + ) -def _patch_prisma(existing_metadata: dict): - """Build a context-manager that patches the proxy's ``prisma_client`` - to return ``existing_metadata`` from ``get_data`` and a stub team row - from ``litellm_teamtable.update``.""" - mock_prisma = MagicMock() - mock_prisma.get_data = AsyncMock(return_value=_existing_team_row(existing_metadata)) +@pytest.fixture +def patched_prisma(): + with ( + patch("litellm.proxy.proxy_server.prisma_client") as mock_client, + patch( + "litellm.proxy.management_endpoints.team_endpoints._is_user_org_admin_for_team", + new_callable=AsyncMock, + return_value=False, + ), + ): + mock_client.get_data = AsyncMock(return_value=_team_row()) + mock_client.db.litellm_teamtable.update = AsyncMock() + yield mock_client - updated_row = MagicMock() - updated_row.team_id = "team-1" - mock_prisma.db.litellm_teamtable.update = AsyncMock(return_value=updated_row) - return mock_prisma + +@pytest.mark.asyncio +async def test_add_team_callbacks_rejects_unauthorized_caller( + patched_prisma, unauthorized_caller +): + data = AddTeamCallback( + callback_name="langfuse", + callback_type="success", + callback_vars={ + "langfuse_public_key": "pk-attacker", + "langfuse_secret_key": "sk-attacker", + }, + ) + with pytest.raises(HTTPException) as exc: + await add_team_callbacks( + data=data, + http_request=Mock(spec=Request), + team_id="team-victim", + user_api_key_dict=unauthorized_caller, + ) + assert exc.value.status_code == 403 + patched_prisma.db.litellm_teamtable.update.assert_not_called() + + +@pytest.mark.asyncio +async def test_disable_team_logging_rejects_unauthorized_caller( + patched_prisma, unauthorized_caller +): + with pytest.raises(HTTPException) as exc: + await disable_team_logging( + http_request=Mock(spec=Request), + team_id="team-victim", + user_api_key_dict=unauthorized_caller, + ) + assert exc.value.status_code == 403 + patched_prisma.db.litellm_teamtable.update.assert_not_called() + + +@pytest.mark.asyncio +async def test_get_team_callbacks_rejects_unauthorized_caller( + patched_prisma, unauthorized_caller +): + with pytest.raises(HTTPException) as exc: + await get_team_callbacks( + http_request=Mock(spec=Request), + team_id="team-victim", + user_api_key_dict=unauthorized_caller, + ) + assert exc.value.status_code == 403 + + +@pytest.mark.asyncio +async def test_proxy_admin_can_add_team_callbacks(patched_prisma): + data = AddTeamCallback( + callback_name="langfuse", + callback_type="success", + callback_vars={ + "langfuse_public_key": "pk-admin", + "langfuse_secret_key": "sk-admin", + }, + ) + await add_team_callbacks( + data=data, + http_request=Mock(spec=Request), + team_id="team-victim", + user_api_key_dict=_admin_auth(), + ) + patched_prisma.db.litellm_teamtable.update.assert_awaited_once() + + +@pytest.mark.asyncio +async def test_team_admin_of_target_team_can_add_callbacks(patched_prisma): + patched_prisma.get_data = AsyncMock( + return_value=_team_row(admin_user_id="team_admin_user") + ) + + team_admin = UserAPIKeyAuth( + user_role=LitellmUserRoles.INTERNAL_USER, + user_id="team_admin_user", + api_key="sk-team-admin", + ) + data = AddTeamCallback( + callback_name="langfuse", + callback_type="success", + callback_vars={ + "langfuse_public_key": "pk-team", + "langfuse_secret_key": "sk-team", + }, + ) + await add_team_callbacks( + data=data, + http_request=Mock(spec=Request), + team_id="team-victim", + user_api_key_dict=team_admin, + ) + patched_prisma.db.litellm_teamtable.update.assert_awaited_once() @pytest.mark.asyncio async def test_disable_team_logging_emits_audit_log_when_enabled(monkeypatch): monkeypatch.setattr(litellm, "store_audit_logs", True) mock_prisma = _patch_prisma( - { - "callback_settings": { - "success_callback": ["langfuse"], - "failure_callback": [], - } - } + _team_row( + team_id="team-1", + metadata={ + "callback_settings": { + "success_callback": ["langfuse"], + "failure_callback": [], + } + }, + ) ) audit_calls = [] @@ -87,8 +221,6 @@ async def test_disable_team_logging_emits_audit_log_when_enabled(monkeypatch): user_api_key_dict=_admin_auth(), litellm_changed_by=None, ) - # asyncio.create_task fires the coroutine eagerly; await one tick to let - # the audit-log emit run before the test exits. import asyncio for _ in range(3): @@ -103,9 +235,7 @@ async def test_disable_team_logging_emits_audit_log_when_enabled(monkeypatch): before = json.loads(log.before_value) after = json.loads(log.updated_values) - # Before: the team's pre-existing success_callback survives in the snapshot. assert before["metadata"]["callback_settings"]["success_callback"] == ["langfuse"] - # After: callbacks zeroed out by the endpoint. assert after["metadata"]["callback_settings"]["success_callback"] == [] assert after["metadata"]["callback_settings"]["failure_callback"] == [] @@ -114,12 +244,15 @@ async def test_disable_team_logging_emits_audit_log_when_enabled(monkeypatch): async def test_disable_team_logging_no_audit_when_disabled(monkeypatch): monkeypatch.setattr(litellm, "store_audit_logs", False) mock_prisma = _patch_prisma( - { - "callback_settings": { - "success_callback": ["langfuse"], - "failure_callback": [], - } - } + _team_row( + team_id="team-1", + metadata={ + "callback_settings": { + "success_callback": ["langfuse"], + "failure_callback": [], + } + }, + ) ) audit_calls = [] @@ -147,7 +280,7 @@ async def test_disable_team_logging_no_audit_when_disabled(monkeypatch): @pytest.mark.asyncio async def test_add_team_callbacks_emits_audit_log_when_enabled(monkeypatch): monkeypatch.setattr(litellm, "store_audit_logs", True) - mock_prisma = _patch_prisma({"logging": []}) + mock_prisma = _patch_prisma(_team_row(team_id="team-1", metadata={"logging": []})) audit_calls = [] @@ -186,7 +319,6 @@ async def test_add_team_callbacks_emits_audit_log_when_enabled(monkeypatch): assert log.table_name == LitellmTableNames.TEAM_TABLE_NAME assert log.object_id == "team-1" assert log.action == "updated" - # ``litellm_changed_by`` header takes precedence over the auth user_id. assert log.changed_by == "ops-on-call" before = json.loads(log.before_value) @@ -195,34 +327,32 @@ async def test_add_team_callbacks_emits_audit_log_when_enabled(monkeypatch): assert len(after["metadata"]["logging"]) == 1 assert after["metadata"]["logging"][0]["callback_name"] == "langfuse" - # Callback secrets MUST NOT leak into the audit log payload. callback_vars = after["metadata"]["logging"][0]["callback_vars"] assert callback_vars["langfuse_public_key"] != "pk" assert callback_vars["langfuse_secret_key"] != "sk" - # Key names are preserved so the auditor can see which fields changed. assert "langfuse_public_key" in callback_vars assert "langfuse_secret_key" in callback_vars - # And no plaintext secret should appear anywhere in the serialized row. - assert "sk" not in log.updated_values.replace("sk-", "") # crude leak check + assert "sk" not in log.updated_values.replace("sk-", "") assert "pk" not in (log.updated_values.replace("pk-", "").replace("public_key", "")) @pytest.mark.asyncio async def test_disable_team_logging_redacts_existing_callback_secrets(monkeypatch): monkeypatch.setattr(litellm, "store_audit_logs", True) - # Existing team has populated callback_vars containing secrets — redaction - # must apply to the BEFORE snapshot too. mock_prisma = _patch_prisma( - { - "callback_settings": { - "success_callback": ["langfuse"], - "failure_callback": [], - "callback_vars": { - "langfuse_public_key": "pk-real", - "langfuse_secret_key": "sk-real-secret", - }, - } - } + _team_row( + team_id="team-1", + metadata={ + "callback_settings": { + "success_callback": ["langfuse"], + "failure_callback": [], + "callback_vars": { + "langfuse_public_key": "pk-real", + "langfuse_secret_key": "sk-real-secret", + }, + } + }, + ) ) audit_calls = [] @@ -251,8 +381,6 @@ async def test_disable_team_logging_redacts_existing_callback_secrets(monkeypatc assert len(audit_calls) == 1 log = audit_calls[0] - # The pre-existing secret_key value must NOT appear in the serialized - # before_value or updated_values. assert "sk-real-secret" not in log.before_value assert "sk-real-secret" not in log.updated_values assert "pk-real" not in log.before_value @@ -262,7 +390,7 @@ async def test_disable_team_logging_redacts_existing_callback_secrets(monkeypatc @pytest.mark.asyncio async def test_add_team_callbacks_no_audit_when_disabled(monkeypatch): monkeypatch.setattr(litellm, "store_audit_logs", False) - mock_prisma = _patch_prisma({"logging": []}) + mock_prisma = _patch_prisma(_team_row(team_id="team-1", metadata={"logging": []})) audit_calls = []