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.
This commit is contained in:
Krrish Dholakia 2026-07-11 17:57:22 -07:00
parent 5ce2e3c421
commit b333f6af66
2 changed files with 93 additions and 15 deletions

View file

@ -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(

View file

@ -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