From 6499fa76debe1377dd95c81e0e2ef487e8829926 Mon Sep 17 00:00:00 2001 From: Yuneng Jiang Date: Fri, 1 May 2026 16:15:21 -0700 Subject: [PATCH] [Fix] RBAC: Drop management_routes Write Fallback for Admin Viewer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- litellm/proxy/auth/route_checks.py | 16 +++--- .../proxy/auth/test_route_checks.py | 57 +++++++++++++++++++ 2 files changed, 66 insertions(+), 7 deletions(-) diff --git a/litellm/proxy/auth/route_checks.py b/litellm/proxy/auth/route_checks.py index c30c5459be..dba29f8413 100644 --- a/litellm/proxy/auth/route_checks.py +++ b/litellm/proxy/auth/route_checks.py @@ -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}", diff --git a/tests/test_litellm/proxy/auth/test_route_checks.py b/tests/test_litellm/proxy/auth/test_route_checks.py index ad71e765fa..39f832256d 100644 --- a/tests/test_litellm/proxy/auth/test_route_checks.py +++ b/tests/test_litellm/proxy/auth/test_route_checks.py @@ -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.