From b333f6af668d22378e9e25a4eb67c81d28e49b23 Mon Sep 17 00:00:00 2001 From: Krrish Dholakia Date: Sat, 11 Jul 2026 17:57:22 -0700 Subject: [PATCH] fix(claude-code): enforce team allowed_skills subset on key create/update validate_key_allowed_skills_against_team only blocked personal keys; a team-scoped key could still self-assign any skill name once inside a team with no per-key check against that team's own allowed_skills list. Skills have no id-resolution step downstream the way mcp_servers does, so the assignment itself is the authorization boundary - now validated as a subset of the team's grant, same as search_tools. --- .../object_permission_utils.py | 58 ++++++++++++++----- .../test_object_permission_utils.py | 50 ++++++++++++++++ 2 files changed, 93 insertions(+), 15 deletions(-) diff --git a/litellm/proxy/management_helpers/object_permission_utils.py b/litellm/proxy/management_helpers/object_permission_utils.py index d606cc2aadb..f304c338257 100644 --- a/litellm/proxy/management_helpers/object_permission_utils.py +++ b/litellm/proxy/management_helpers/object_permission_utils.py @@ -725,26 +725,54 @@ async def validate_key_allowed_skills_against_team( is_proxy_admin: bool = False, ) -> None: """ - Reject allowed_skills requested on a personal (no team) key by a non-admin - caller. Claude Code skill access is granted at use-time from the key's - object_permission.allowed_skills list, so the assignment is the - authorization boundary. Team keys and proxy admins are unaffected. + Validate key object_permission.allowed_skills is a subset of the team's + allowlist. Claude Code skill access is granted at use-time directly from + the key's (or its team/org's) object_permission.allowed_skills list with + no id-resolution step in between, so the assignment itself is the + authorization boundary - unlike mcp_servers, there's no downstream check + that could still catch an over-broad grant. + + Empty team allowlist means no restriction at team layer (skip), matching + get_allowed_skills' own intersection semantics. Non-admin callers cannot + assign allowed_skills to a personal (no team) key at all. """ requested = _extract_requested_allowed_skills(object_permission) if not requested: return - if team_obj is not None or is_proxy_admin: + + if team_obj is None and not is_proxy_admin: + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail={ + "error": ( + "Key is not in a team. Skills cannot be assigned to " + "personal keys by non-admin callers. Disallowed skills: " + f"{sorted(requested)}." + ) + }, + ) + + team_skills: List[str] = [] + if team_obj is not None and team_obj.object_permission is not None: + skills = team_obj.object_permission.allowed_skills + if skills: + team_skills = list(skills) + + if not team_skills: return - raise HTTPException( - status_code=status.HTTP_403_FORBIDDEN, - detail={ - "error": ( - "Key is not in a team. Skills cannot be assigned to " - "personal keys by non-admin callers. Disallowed skills: " - f"{sorted(requested)}." - ) - }, - ) + + disallowed = requested - set(team_skills) + if disallowed: + team_id = team_obj.team_id if team_obj is not None else "unknown" + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail={ + "error": ( + f"Key requests skills not allowed by team '{team_id}': " + f"{sorted(disallowed)}. Team allows: {sorted(team_skills)}." + ) + }, + ) def _extract_requested_search_tools( diff --git a/tests/test_litellm/proxy/management_helpers/test_object_permission_utils.py b/tests/test_litellm/proxy/management_helpers/test_object_permission_utils.py index fc395a68c1b..6dd446a5c11 100644 --- a/tests/test_litellm/proxy/management_helpers/test_object_permission_utils.py +++ b/tests/test_litellm/proxy/management_helpers/test_object_permission_utils.py @@ -1207,6 +1207,56 @@ async def test_empty_allowed_skills_passes_for_personal_non_admin(): ) +def _make_team_obj_allowed_skills(team_id="team-1", allowed_skills=None): + mock_team = MagicMock() + mock_team.team_id = team_id + if allowed_skills is not None: + mock_team.object_permission = MagicMock(spec=LiteLLM_ObjectPermissionTable) + mock_team.object_permission.allowed_skills = allowed_skills + else: + mock_team.object_permission = None + return mock_team + + +@pytest.mark.asyncio +async def test_validate_allowed_skills_subset_ok(): + await validate_key_allowed_skills_against_team( + object_permission={"allowed_skills": ["marketplace--skill-a"]}, + team_obj=_make_team_obj_allowed_skills(allowed_skills=["marketplace--skill-a", "marketplace--skill-b"]), + is_proxy_admin=False, + ) + + +@pytest.mark.asyncio +async def test_validate_allowed_skills_raises_when_not_subset(): + """ + Regression test: once a team has an explicit allowed_skills allowlist, + a member of that team must not be able to self-assign a skill outside + it - get_allowed_skills() reads allowed_skills directly with no + downstream id-resolution step to catch an over-broad grant later. + """ + with pytest.raises(HTTPException) as exc: + await validate_key_allowed_skills_against_team( + object_permission={"allowed_skills": ["marketplace--private-skill"]}, + team_obj=_make_team_obj_allowed_skills(allowed_skills=["marketplace--skill-a"]), + is_proxy_admin=False, + ) + assert exc.value.status_code == 403 + assert "marketplace--private-skill" in str(exc.value.detail) + + +@pytest.mark.asyncio +async def test_validate_allowed_skills_team_unrestricted_allows_any(): + """Empty team allowed_skills allowlist means unrestricted at the team + layer, matching get_allowed_skills' own intersection semantics - the + subset check is skipped, not treated as deny-all.""" + await validate_key_allowed_skills_against_team( + object_permission={"allowed_skills": ["marketplace--anything"]}, + team_obj=_make_team_obj_allowed_skills(allowed_skills=[]), + is_proxy_admin=False, + ) + + def test_object_permission_dict_mirrors_pydantic_model(): """ObjectPermissionDict must stay field-for-field aligned with LiteLLM_ObjectPermissionBase. If a new field is added to the Pydantic