[Fix] RBAC: Drop management_routes Write Fallback for Admin Viewer

Greptile P1: the unsafe-method branch of `_check_proxy_admin_viewer_access`
ended with a blanket `if route in management_routes: return`. That set is a
mix of reads (info/list — handled via the safe-method GET branch above) and
writes. The fallback let Admin Viewer POST to write endpoints not enumerated
in `_ADMIN_VIEWER_BLOCKED_WRITE_ROUTES`, including:
  - /team/block, /team/unblock, /team/permissions_update
  - /jwt/key/mapping/{new,update,delete}
  - /key/bulk_update
  - /key/{key_id}/reset_spend

Remove the fallback. The two remaining allow sets (admin_viewer_routes and
global_spend_tracking_routes) are both read-only, so removal does not affect
the legitimate POST-as-read cases (e.g. /spend/calculate, which is in
spend_tracking_routes ⊂ admin_viewer_routes).

Tests:
  - 8 new parametrized cases pinning each previously-leaking management write
    endpoint to 403 on POST for PROXY_ADMIN_VIEW_ONLY.
This commit is contained in:
Yuneng Jiang
2026-05-01 16:15:21 -07:00
parent c78144ccf0
commit 6499fa76de
2 changed files with 66 additions and 7 deletions
+9 -7
View File
@@ -699,7 +699,8 @@ class RouteChecks:
)
# Legacy explicit-allow sets (kept for routes that are POST but
# semantically read-only, e.g. /spend/calculate).
# semantically read-only, e.g. /spend/calculate). Both admin_viewer_routes
# and global_spend_tracking_routes are reads/listings.
if RouteChecks.check_route_access(
route=route, allowed_routes=LiteLLMRoutes.admin_viewer_routes.value
):
@@ -708,13 +709,14 @@ class RouteChecks:
route=route, allowed_routes=LiteLLMRoutes.global_spend_tracking_routes.value
):
return
if RouteChecks.check_route_access(
route=route, allowed_routes=LiteLLMRoutes.management_routes.value
):
# On management routes, allow non-blocked writes (e.g. read-only
# info/list endpoints implemented as POST).
return
# NOTE: We intentionally do NOT fall back to allowing all
# `management_routes`. That set is a mix of reads (info/list — handled
# via the safe-method branch above) and writes (`/team/block`,
# `/team/permissions_update`, `/jwt/key/mapping/{new,update,delete}`,
# `/key/bulk_update`, `/key/{id}/reset_spend`). A blanket allow would
# let Admin Viewer POST these write endpoints — violating the
# "no writes, ever" rule. Default-deny instead.
raise HTTPException(
status_code=status.HTTP_403_FORBIDDEN,
detail=f"user not allowed to access this route, role= {_user_role}. Trying to access: {route}",
@@ -1484,6 +1484,63 @@ def test_proxy_admin_viewer_post_blocked_outside_allowlists(route):
assert exc_info.value.status_code == 403
# ── Admin Viewer: management_routes write endpoints stay blocked ─────────────
#
# `management_routes` is a mix of reads (info/list, handled via the safe-method
# branch — GET) and writes. The route_checks layer must NOT blanket-allow the
# whole set on POST — that would let Admin Viewer mutate teams, JWT mappings,
# and bulk-update keys, violating the "no writes, ever" rule.
#
# These cases pin the gap closed (Greptile P1 review, 2026-04-30).
ADMIN_VIEWER_MANAGEMENT_ROUTE_WRITES = [
# Team writes
"/team/block",
"/team/unblock",
"/team/permissions_update",
# JWT key mapping writes
"/jwt/key/mapping/new",
"/jwt/key/mapping/update",
"/jwt/key/mapping/delete",
# Key writes (existing _ADMIN_VIEWER_BLOCKED_WRITE_ROUTES doesn't list bulk
# update or per-key reset-spend, so the management_routes fallback was the
# only thing keeping them out — and it was permissive, not restrictive).
"/key/bulk_update",
"/key/some-key-id/reset_spend",
]
@pytest.mark.parametrize("route", ADMIN_VIEWER_MANAGEMENT_ROUTE_WRITES)
def test_proxy_admin_viewer_post_blocked_for_management_route_writes(route):
"""
Admin Viewer must be blocked on POST to write endpoints in
`management_routes`, even when the specific route is not in
`_ADMIN_VIEWER_BLOCKED_WRITE_ROUTES`.
"""
user_obj = LiteLLM_UserTable(
user_id="viewer_user",
user_email="viewer@example.com",
user_role=LitellmUserRoles.PROXY_ADMIN_VIEW_ONLY.value,
)
valid_token = UserAPIKeyAuth(
user_id="viewer_user",
user_role=LitellmUserRoles.PROXY_ADMIN_VIEW_ONLY.value,
)
request = MagicMock(spec=Request)
request.method = "POST"
request.query_params = {}
with pytest.raises(HTTPException) as exc_info:
RouteChecks.non_proxy_admin_allowed_routes_check(
user_obj=user_obj,
_user_role=LitellmUserRoles.PROXY_ADMIN_VIEW_ONLY.value,
route=route,
request=request,
valid_token=valid_token,
request_data={},
)
assert exc_info.value.status_code == 403
class TestModelsRouteExemptFromDisableLLMEndpoints:
"""
Test that /models and /v1/models are exempt from DISABLE_LLM_API_ENDPOINTS.