mirror of
https://github.com/BerriAI/litellm.git
synced 2026-10-03 02:22:24 +00:00
fix(agent_session_endpoints): block view-only admins from mutating other tenants' rows
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).
This commit is contained in:
parent
5037026d75
commit
303f9a5d80
1 changed files with 55 additions and 6 deletions
|
|
@ -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)}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue