From 303f9a5d806126b31aff6ec9ddda2c3b9589c6fe Mon Sep 17 00:00:00 2001 From: Ishaan Jaffer Date: Wed, 6 May 2026 15:37:07 -0700 Subject: [PATCH] fix(agent_session_endpoints): block view-only admins from mutating other tenants' rows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit is_proxy_admin previously returned True for both PROXY_ADMIN and PROXY_ADMIN_VIEW_ONLY, letting view-only admins skip assert_caller_owns_agent / assert_caller_owns_session on every write endpoint and create / update / delete other tenants' agents, sessions, and runs. Split the helpers: * is_proxy_admin: now full-admin only (used for write paths via the fall-through to per-tenant ownership; view-only fails and gets 404). * is_proxy_admin_read: full + view-only, used on read paths so the support UI can still render any tenant's resources. * assert_caller_can_mutate: explicit 403 guard for view-only on every state-mutating endpoint. The mutating endpoints in agent/session/run files call assert_caller_can_mutate before any DB write — see follow-up commits. Greptile P1 SECURITY (review #PRR_kwDOKALCgc78uM7F). --- .../agent_session_endpoints/ownership.py | 61 +++++++++++++++++-- 1 file changed, 55 insertions(+), 6 deletions(-) diff --git a/litellm/proxy/agent_session_endpoints/ownership.py b/litellm/proxy/agent_session_endpoints/ownership.py index 83253099fda..af76ceddbac 100644 --- a/litellm/proxy/agent_session_endpoints/ownership.py +++ b/litellm/proxy/agent_session_endpoints/ownership.py @@ -36,7 +36,30 @@ def caller_api_key_hash(user_api_key_dict: UserAPIKeyAuth) -> str: def is_proxy_admin(user_api_key_dict: UserAPIKeyAuth) -> bool: - """Proxy admins can access any agent / session / run regardless of owner.""" + """True iff the caller is a full proxy admin (read AND write). + + ``PROXY_ADMIN_VIEW_ONLY`` is intentionally NOT included here. For + write paths, this function being False means the view-only admin + falls back to the per-tenant ownership check, which they fail (no + matching ``user_api_key_hash``) and get a 404 — closing the + privilege escalation that previously let view-only admins create / + update / delete other tenants' agents, sessions, and runs. + + For read paths, callers should use :func:`is_proxy_admin_read` so + view-only admins still get cross-tenant visibility. + """ + role = user_api_key_dict.user_role + if role is None: + return False + return role == LitellmUserRoles.PROXY_ADMIN + + +def is_proxy_admin_read(user_api_key_dict: UserAPIKeyAuth) -> bool: + """True iff the caller is any flavor of proxy admin (incl. view-only). + + Used only on read paths where the view-only admin is allowed to see + other tenants' resources. + """ role = user_api_key_dict.user_role if role is None: return False @@ -46,6 +69,22 @@ def is_proxy_admin(user_api_key_dict: UserAPIKeyAuth) -> bool: } +def assert_caller_can_mutate(user_api_key_dict: UserAPIKeyAuth) -> None: + """Reject write access for view-only admins. + + Every state-mutating endpoint (POST/PUT/PATCH/DELETE) must call this + before any DB write. View-only admins are granted read access to all + tenants via :func:`is_proxy_admin_read` but MUST NOT be allowed to + mutate state — they otherwise inherit master-admin write authority + over every tenant's agents, sessions, and runs. + """ + if user_api_key_dict.user_role == LitellmUserRoles.PROXY_ADMIN_VIEW_ONLY: + raise HTTPException( + status_code=403, + detail="View-only admins cannot perform write operations", + ) + + def assert_caller_owns_agent( user_api_key_dict: UserAPIKeyAuth, agent_row: Any, @@ -54,10 +93,14 @@ def assert_caller_owns_agent( 404 (not 403) on purpose — leaking existence of another tenant's resource is a fingerprinting risk. + + Both full and view-only admins pass this read-side check; write + endpoints must additionally call :func:`assert_caller_can_mutate` to + block view-only admins from mutating other tenants' rows. """ if agent_row is None: raise HTTPException(status_code=404, detail="Agent not found") - if is_proxy_admin(user_api_key_dict): + if is_proxy_admin_read(user_api_key_dict): return expected_hash = caller_api_key_hash(user_api_key_dict) if agent_row.user_api_key_hash != expected_hash: @@ -71,10 +114,15 @@ def assert_caller_owns_session( """Raise 404 if the caller is not the session's owner (and not admin). Sessions carry their own ``user_api_key_hash`` so we don't need to load - the parent agent to check ownership.""" + the parent agent to check ownership. + + Both full and view-only admins pass this read-side check; write + endpoints must additionally call :func:`assert_caller_can_mutate` to + block view-only admins from mutating other tenants' rows. + """ if session_row is None: raise HTTPException(status_code=404, detail="Session not found") - if is_proxy_admin(user_api_key_dict): + if is_proxy_admin_read(user_api_key_dict): return expected_hash = caller_api_key_hash(user_api_key_dict) if session_row.user_api_key_hash != expected_hash: @@ -88,8 +136,9 @@ def owner_filter_for_caller( caller's own rows. Returns ``None`` for proxy admins (no filter) so callers can spread it - into an existing where dict only when needed. + into an existing where dict only when needed. Both full and view-only + admins get the unfiltered read. """ - if is_proxy_admin(user_api_key_dict): + if is_proxy_admin_read(user_api_key_dict): return None return {"user_api_key_hash": caller_api_key_hash(user_api_key_dict)}