From 30897fd1cbe78f71078f5235ea9b0e5fde8d8b88 Mon Sep 17 00:00:00 2001 From: Krrish Dholakia Date: Sat, 11 Jul 2026 17:41:08 -0700 Subject: [PATCH] fix(claude-code): close 4 skill-marketplace authz and supply-chain gaps Non-admin callers could self-assign allowed_skills on a personal key with no team/org ceiling; a disabled marketplace's skills stayed reachable through pre-existing allowed_skills grants; a marketplace re-sync could silently repoint an already-published skill's source with no re-review; and a marketplace/skill name collision could overwrite a different marketplace's (or a hand-registered) plugin row. --- .../claude_code_marketplace.py | 30 +++-- .../claude_code_marketplace_sync.py | 90 +++++++++++--- litellm/proxy/auth/route_checks.py | 5 + .../key_management_endpoints.py | 16 +++ .../object_permission_utils.py | 40 +++++++ .../test_claude_code_marketplace.py | 95 ++++++++++++++- .../test_claude_code_marketplace_sync.py | 112 ++++++++++++++++++ .../test_object_permission_utils.py | 53 +++++++++ 8 files changed, 413 insertions(+), 28 deletions(-) diff --git a/litellm/proxy/anthropic_endpoints/claude_code_endpoints/claude_code_marketplace.py b/litellm/proxy/anthropic_endpoints/claude_code_endpoints/claude_code_marketplace.py index 5341bb97b2a..9d996a415f3 100644 --- a/litellm/proxy/anthropic_endpoints/claude_code_endpoints/claude_code_marketplace.py +++ b/litellm/proxy/anthropic_endpoints/claude_code_endpoints/claude_code_marketplace.py @@ -37,7 +37,10 @@ from litellm.proxy.auth.user_api_key_auth import ( google_ai_studio_api_key_header, user_api_key_auth, ) -from litellm.repositories.table_repositories import ClaudeCodePluginRepository +from litellm.repositories.table_repositories import ( + ClaudeCodePluginRepository, + SkillMarketplaceRepository, +) from litellm.types.proxy.claude_code_endpoints import ( ListPluginsResponse, PluginListItem, @@ -118,18 +121,31 @@ async def get_marketplace( prisma_client = await _get_prisma_client() allowed_skills: FrozenSet[str] = ( - await get_allowed_skills(user_api_key_dict, prisma_client) - if user_api_key_dict is not None - else frozenset() + await get_allowed_skills(user_api_key_dict, prisma_client) if user_api_key_dict is not None else frozenset() ) where = ( - {"OR": [{"enabled": True}, {"name": {"in": list(allowed_skills)}}]} - if allowed_skills - else {"enabled": True} + {"OR": [{"enabled": True}, {"name": {"in": list(allowed_skills)}}]} if allowed_skills else {"enabled": True} ) plugins = await ClaudeCodePluginRepository(prisma_client).table.find_many(where=where) + # A per-skill grant (allowed_skills) is a standing entry on a + # key/team/org's object_permission - it isn't cleared just because an + # admin later disables the marketplace that owned the skill. Without + # this, a disabled marketplace's skills stay reachable forever by + # anyone previously granted one by name. Plugins with no + # marketplace_id (hand-registered) are unaffected. + marketplace_ids = {p.marketplace_id for p in plugins if p.marketplace_id} + if marketplace_ids: + disabled_marketplace_ids = { + m.id + for m in await SkillMarketplaceRepository(prisma_client).table.find_many( + where={"id": {"in": list(marketplace_ids)}, "enabled": False} + ) + } + if disabled_marketplace_ids: + plugins = [p for p in plugins if p.marketplace_id not in disabled_marketplace_ids] + plugin_list = [] for plugin in plugins: try: diff --git a/litellm/proxy/anthropic_endpoints/claude_code_endpoints/claude_code_marketplace_sync.py b/litellm/proxy/anthropic_endpoints/claude_code_endpoints/claude_code_marketplace_sync.py index 37e82ba230c..316c0235809 100644 --- a/litellm/proxy/anthropic_endpoints/claude_code_endpoints/claude_code_marketplace_sync.py +++ b/litellm/proxy/anthropic_endpoints/claude_code_endpoints/claude_code_marketplace_sync.py @@ -564,10 +564,7 @@ async def _fetch_entries_for_docs( ) -> tuple[tuple[ResolvedPluginEntry, ...], int]: """Returns (successfully-fetched entries, count skipped after retries).""" fetched = await asyncio.gather( - *( - _fetch_skill_entry(client, repo, branch, marketplace_name, doc, timeout=timeout) - for doc in docs - ) + *(_fetch_skill_entry(client, repo, branch, marketplace_name, doc, timeout=timeout) for doc in docs) ) entries = tuple(entry for entry in fetched if entry is not None) return entries, len(docs) - len(entries) @@ -657,11 +654,59 @@ def _build_plugin_manifest_json(entry: ResolvedPluginEntry) -> str: ) +def _existing_source_changed(existing_manifest_json: str, entry: ResolvedPluginEntry) -> bool: + try: + existing_source = json.loads(existing_manifest_json).get("source") + except json.JSONDecodeError: + return True + return existing_source != entry.source.model_dump() + + async def _upsert_single_plugin( repository: ClaudeCodePluginRepository, marketplace_id: str, entry: ResolvedPluginEntry -) -> None: +) -> bool: + """Upsert one synced entry. Returns False (and writes nothing) if it was + skipped because ``entry.stored_name`` collides with a row owned by a + different marketplace - see the collision-guard comment below.""" now = datetime.now(timezone.utc) manifest_json = _build_plugin_manifest_json(entry) + + existing = await repository.table.find_unique(where={"name": entry.stored_name}) + + # `name` is globally unique but namespacing skills as "{marketplace}--{skill}" + # is only a convention, not schema-enforced - a marketplace slug or skill + # name chosen (or supplied by a compromised upstream repo) to collide with + # another marketplace's, or a hand-registered plugin's, stored_name must + # not silently overwrite that other row. Refuse and leave it untouched. + if existing is not None and existing.marketplace_id != marketplace_id: + verbose_proxy_logger.warning( + "skill-marketplace-sync: %r already registered under a different " + "marketplace (marketplace_id=%r), refusing to overwrite from marketplace_id=%r", + entry.stored_name, + existing.marketplace_id, + marketplace_id, + ) + return False + + # A skill an admin has already reviewed and published (enabled=True) must + # not have its git source silently swapped by whoever controls the + # upstream marketplace repo on the next sync - that would let a + # compromised/malicious upstream repoint an already-trusted, publicly + # served skill without any re-review. Demote it back to unpublished so an + # admin has to look at it again before it's public with the new source. + update_data: dict[str, Any] = { + "description": entry.description, + "manifest_json": manifest_json, + "marketplace_id": marketplace_id, + "updated_at": now, + } + if existing is not None and existing.enabled and _existing_source_changed(existing.manifest_json, entry): + verbose_proxy_logger.warning( + "skill-marketplace-sync: %r changed source on re-sync, unpublishing pending admin re-review", + entry.stored_name, + ) + update_data["enabled"] = False + await repository.table.upsert( where={"name": entry.stored_name}, data={ @@ -675,14 +720,10 @@ async def _upsert_single_plugin( "created_at": now, "updated_at": now, }, - "update": { - "description": entry.description, - "manifest_json": manifest_json, - "marketplace_id": marketplace_id, - "updated_at": now, - }, + "update": update_data, }, ) + return True # prisma_client has no importable type stubs in this codebase (generated at @@ -692,9 +733,12 @@ async def _upsert_plugin_entries( prisma_client: Any, # noqa: ANN401 # see comment above marketplace_id: str, entries: tuple[ResolvedPluginEntry, ...], -) -> None: +) -> int: + """Returns the number of entries skipped due to a stored_name collision + with a row owned by a different marketplace.""" repository = ClaudeCodePluginRepository(prisma_client) - await asyncio.gather(*(_upsert_single_plugin(repository, marketplace_id, entry) for entry in entries)) + written = await asyncio.gather(*(_upsert_single_plugin(repository, marketplace_id, entry) for entry in entries)) + return sum(1 for ok in written if not ok) async def _soft_disable_stale_plugins( @@ -758,15 +802,23 @@ async def resolve_and_sync( await _record_sync_failure(prisma_client, marketplace_row, str(exc)) return SyncResult(status="error", error=str(exc), plugin_count=0) - if skipped_count: + collision_skipped_count = await _upsert_plugin_entries(prisma_client, marketplace_row.id, entries) + total_skipped_count = skipped_count + collision_skipped_count + if total_skipped_count: verbose_proxy_logger.warning( - "skill-marketplace-sync: %r imported %d skill(s), skipped %d after retries", + "skill-marketplace-sync: %r imported %d skill(s), skipped %d (%d after retries, %d name collisions)", marketplace_row.name, - len(entries), + len(entries) - collision_skipped_count, + total_skipped_count, skipped_count, + collision_skipped_count, ) - await _upsert_plugin_entries(prisma_client, marketplace_row.id, entries) await _soft_disable_stale_plugins(prisma_client, marketplace_row.id, entries) - await _record_sync_success(prisma_client, marketplace_row, source_type, skipped_count) - return SyncResult(status="success", error=None, plugin_count=len(entries), skipped_count=skipped_count) + await _record_sync_success(prisma_client, marketplace_row, source_type, total_skipped_count) + return SyncResult( + status="success", + error=None, + plugin_count=len(entries) - collision_skipped_count, + skipped_count=total_skipped_count, + ) diff --git a/litellm/proxy/auth/route_checks.py b/litellm/proxy/auth/route_checks.py index 346bed9ab0a..1a65159ba04 100644 --- a/litellm/proxy/auth/route_checks.py +++ b/litellm/proxy/auth/route_checks.py @@ -722,6 +722,11 @@ class RouteChecks: The Claude Code CLI fetches this route without custom headers, so it allows passing key=api_key in the query params (mirrors the Google generateContent carve-out above). + + Known tradeoff, shared with the Google carve-out: a key passed this + way can end up unredacted in web-server access logs. Recommend admins + use a scoped, rotatable key for marketplace URLs rather than the + master key. """ return route == "/claude-code/marketplace.json" diff --git a/litellm/proxy/management_endpoints/key_management_endpoints.py b/litellm/proxy/management_endpoints/key_management_endpoints.py index bf64f537c7f..b32c82fb8df 100644 --- a/litellm/proxy/management_endpoints/key_management_endpoints.py +++ b/litellm/proxy/management_endpoints/key_management_endpoints.py @@ -78,6 +78,7 @@ from litellm.proxy.management_helpers.object_permission_utils import ( _set_object_permission, attach_object_permission_to_dict, handle_update_object_permission_common, + validate_key_allowed_skills_against_team, validate_key_mcp_servers_against_team, validate_key_search_tools_against_team, validate_key_vector_stores_against_team, @@ -978,6 +979,11 @@ async def _common_key_generation_helper( team_obj=team_table, is_proxy_admin=_is_proxy_admin_caller, ) + await validate_key_allowed_skills_against_team( + object_permission=data_json.get("object_permission"), + team_obj=team_table, + is_proxy_admin=_is_proxy_admin_caller, + ) # Merge default_key_generate_params.object_permission in *after* the team-scope # checks above, so an admin-configured default (e.g. vector_stores, search_tools) @@ -2235,6 +2241,11 @@ async def _validate_mcp_servers_for_key_update( team_obj=effective_team_obj, is_proxy_admin=is_proxy_admin, ) + await validate_key_allowed_skills_against_team( + object_permission=object_permission_dict, + team_obj=effective_team_obj, + is_proxy_admin=is_proxy_admin, + ) return normalized_object_permission @@ -4752,6 +4763,11 @@ async def regenerate_key_fn( team_obj=regenerate_team_table, is_proxy_admin=_regen_is_proxy_admin, ) + await validate_key_allowed_skills_against_team( + object_permission=_regen_object_permission_dict, + team_obj=regenerate_team_table, + is_proxy_admin=_regen_is_proxy_admin, + ) verbose_proxy_logger.info( "Key regeneration requested: key_alias=%s", diff --git a/litellm/proxy/management_helpers/object_permission_utils.py b/litellm/proxy/management_helpers/object_permission_utils.py index 6bbd41b93ed..d606cc2aadb 100644 --- a/litellm/proxy/management_helpers/object_permission_utils.py +++ b/litellm/proxy/management_helpers/object_permission_utils.py @@ -707,6 +707,46 @@ async def validate_key_vector_stores_against_team( ) +def _extract_requested_allowed_skills( + object_permission: Optional[ObjectPermissionDict], +) -> set[str]: + """Return allowed_skills names from a key's object_permission dict.""" + if not object_permission or not isinstance(object_permission, dict): + return set() + raw = object_permission.get("allowed_skills") + if isinstance(raw, list): + return {str(x) for x in raw if x} + return set() + + +async def validate_key_allowed_skills_against_team( + object_permission: Optional[ObjectPermissionDict], + team_obj: Optional["LiteLLM_TeamTableCachedObj"], + 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. + """ + requested = _extract_requested_allowed_skills(object_permission) + if not requested: + return + if team_obj is not None or is_proxy_admin: + 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)}." + ) + }, + ) + + def _extract_requested_search_tools( object_permission: Optional[ObjectPermissionDict], ) -> list[str]: diff --git a/tests/pass_through_unit_tests/test_claude_code_marketplace.py b/tests/pass_through_unit_tests/test_claude_code_marketplace.py index 25822f21082..288b151eadf 100644 --- a/tests/pass_through_unit_tests/test_claude_code_marketplace.py +++ b/tests/pass_through_unit_tests/test_claude_code_marketplace.py @@ -20,7 +20,6 @@ sys.path.insert(0, os.path.abspath("../..")) import litellm from litellm.proxy._types import UserAPIKeyAuth from litellm.proxy.proxy_server import LitellmUserRoles -from litellm.caching.caching import DualCache from litellm.types.proxy.claude_code_endpoints import RegisterPluginRequest # Import the functions we're testing @@ -34,7 +33,14 @@ class MockPluginRecord: """Mock plugin record that mimics Prisma model behavior.""" def __init__( - self, name, version, description, manifest_json, enabled=True, created_by=None + self, + name, + version, + description, + manifest_json, + enabled=True, + created_by=None, + marketplace_id=None, ): self.id = f"plugin-{name}-{int(time.time())}" self.name = name @@ -46,6 +52,16 @@ class MockPluginRecord: self.created_at = datetime.now(timezone.utc) self.updated_at = datetime.now(timezone.utc) self.created_by = created_by + self.marketplace_id = marketplace_id + + +class MockMarketplaceRecord: + """Mock LiteLLM_SkillMarketplaceTable record.""" + + def __init__(self, id, name, enabled=True): + self.id = id + self.name = name + self.enabled = enabled @pytest.fixture @@ -97,6 +113,7 @@ def mock_prisma_client(): manifest_json=manifest, enabled=data.get("enabled", True), created_by=data.get("created_by"), + marketplace_id=data.get("marketplace_id"), ) plugins_store[plugin_name] = plugin return plugin @@ -139,6 +156,25 @@ def mock_prisma_client(): mock_table.delete = AsyncMock(side_effect=delete) mock_client.db.litellm_claudecodeplugintable = mock_table + + # In-memory storage for marketplace rows, keyed by id. + marketplaces_store = {} + + mock_marketplace_table = MagicMock() + + async def marketplace_find_many(where=None): + rows = list(marketplaces_store.values()) + if not where: + return rows + if "id" in where and "in" in where["id"]: + rows = [r for r in rows if r.id in where["id"]["in"]] + if "enabled" in where: + rows = [r for r in rows if r.enabled == where["enabled"]] + return rows + + mock_marketplace_table.find_many = AsyncMock(side_effect=marketplace_find_many) + mock_client.db.litellm_skillmarketplacetable = mock_marketplace_table + mock_client._marketplaces_store = marketplaces_store mock_client.connect = AsyncMock(side_effect=connect) # Store plugins_store on the mock for cleanup if needed @@ -365,6 +401,61 @@ async def test_get_marketplace_with_key_unlocks_allowed_imported_skill( ) +@pytest.mark.asyncio +async def test_get_marketplace_hides_skill_from_disabled_marketplace_even_with_grant( + mock_prisma_client, +): + """Regression test: allowed_skills is a standing grant on a key/team/org's + object_permission - it doesn't get cleared just because an admin later + disables the marketplace that owned the skill (DELETE + /claude-code/marketplaces/{name}, which cascades plugin.enabled=False but + leaves any pre-existing allowed_skills grants untouched). Without + filtering on the owning marketplace's own enabled state, a previously + granted key would keep seeing a skill from a marketplace an admin + explicitly shut off.""" + from litellm.models.object_permission import LiteLLM_ObjectPermissionTable + + setattr(litellm.proxy.proxy_server, "prisma_client", mock_prisma_client) + setattr(litellm.proxy.proxy_server, "master_key", "sk-1234") + await litellm.proxy.proxy_server.prisma_client.connect() + + marketplace_id = f"marketplace-{int(time.time())}" + mock_prisma_client._marketplaces_store[marketplace_id] = MockMarketplaceRecord( + id=marketplace_id, name="untrusted-marketplace", enabled=False + ) + + skill_name = f"untrusted-marketplace--skill-{int(time.time())}" + await mock_prisma_client.db.litellm_claudecodeplugintable.create( + data={ + "name": skill_name, + "version": "1.0.0", + "description": "Skill owned by a since-disabled marketplace", + "manifest_json": json.dumps( + {"source": {"source": "github", "repo": "org/untrusted-skill"}} + ), + "enabled": True, # cascade-disable races aside, this asserts the belt-and-suspenders check too + "marketplace_id": marketplace_id, + } + ) + + granted_key = UserAPIKeyAuth( + user_role=LitellmUserRoles.INTERNAL_USER, + api_key="sk-granted", + user_id="granted-user", + object_permission=LiteLLM_ObjectPermissionTable( + object_permission_id="perm-2", allowed_skills=[skill_name] + ), + ) + response = await get_marketplace(user_api_key_dict=granted_key) + body = json.loads(response.body.decode()) + + assert skill_name not in {p["name"] for p in body["plugins"]} + + # Cleanup + await mock_prisma_client.db.litellm_claudecodeplugintable.delete(where={"name": skill_name}) + del mock_prisma_client._marketplaces_store[marketplace_id] + + @pytest.mark.asyncio async def test_register_plugin_git_subdir(mock_prisma_client): """Test registering a plugin with git-subdir source type.""" diff --git a/tests/test_litellm/proxy/anthropic_endpoints/test_claude_code_marketplace_sync.py b/tests/test_litellm/proxy/anthropic_endpoints/test_claude_code_marketplace_sync.py index fd73317e50b..15ffab8012c 100644 --- a/tests/test_litellm/proxy/anthropic_endpoints/test_claude_code_marketplace_sync.py +++ b/tests/test_litellm/proxy/anthropic_endpoints/test_claude_code_marketplace_sync.py @@ -636,3 +636,115 @@ async def test_resolve_and_sync_soft_disables_stale_plugin(monkeypatch): refreshed_stale = await client.db.litellm_claudecodeplugintable.find_unique(where={"name": stale_name}) assert refreshed_stale is not None assert refreshed_stale.enabled is False + + +@pytest.mark.asyncio +async def test_resolve_and_sync_refuses_to_overwrite_row_owned_by_another_marketplace(monkeypatch): + """Regression test: `name` is only conventionally namespaced as + "{marketplace}--{skill}", not schema-enforced, so a marketplace slug or + skill name that collides with an existing row owned by a *different* + marketplace (or a hand-registered plugin with no marketplace_id) must be + skipped, never silently overwritten.""" + client = _make_fake_prisma_client() + + colliding_name = "anthropic-agent-skills--claude-api" + hand_registered = await client.db.litellm_claudecodeplugintable.create( + data={ + "name": colliding_name, + "description": "trusted, hand-registered plugin", + "manifest_json": json.dumps({"source": {"source": "github", "repo": "trusted/repo"}}), + "files_json": "{}", + "enabled": True, + "marketplace_id": None, + "created_at": datetime(2023, 1, 1), + "updated_at": datetime(2023, 1, 1), + } + ) + + marketplace = await _create_marketplace(client, name="anthropic-agent-skills", source_ref="anthropics/skills") + + async def _get(http_client, url, **kwargs): + return httpx.Response(200, json=_ANTHROPIC_SKILLS_MANIFEST) + + monkeypatch.setattr(sync_module, "async_safe_get", _get) + + result = await resolve_and_sync(client, marketplace) + + assert result.status == "success" + assert result.plugin_count == 2 # document-skills, example-skills - claude-api collided and was skipped + assert result.skipped_count == 1 + + untouched = await client.db.litellm_claudecodeplugintable.find_unique(where={"name": colliding_name}) + assert untouched.marketplace_id is None + assert untouched.enabled is True + assert json.loads(untouched.manifest_json)["source"] == {"source": "github", "repo": "trusted/repo"} + assert untouched.id == hand_registered.id + + +@pytest.mark.asyncio +async def test_resolve_and_sync_unpublishes_skill_whose_source_changed(monkeypatch): + """Regression test: an already-public (enabled=True) skill must not have + its git source silently swapped by a re-sync of the marketplace that owns + it - that would let a compromised/malicious upstream repoint an + already-trusted skill with no admin re-review. The sync should demote it + back to enabled=False instead of overwriting the source in place.""" + client = _make_fake_prisma_client() + marketplace = await _create_marketplace(client, name="anthropic-agent-skills", source_ref="anthropics/skills") + + async def _get(http_client, url, **kwargs): + return httpx.Response(200, json=_ANTHROPIC_SKILLS_MANIFEST) + + monkeypatch.setattr(sync_module, "async_safe_get", _get) + await resolve_and_sync(client, marketplace) + + published_name = "anthropic-agent-skills--claude-api" + published = await client.db.litellm_claudecodeplugintable.find_unique(where={"name": published_name}) + published.enabled = True # simulate an admin having reviewed and published it + original_source = json.loads(published.manifest_json)["source"] + + # A repointed `source` (repo root -> a subdirectory) is exactly what an + # upstream marketplace repo owner controls and could change unilaterally. + changed_manifest = { + **_ANTHROPIC_SKILLS_MANIFEST, + "plugins": [ + p if p["name"] != "claude-api" else {**p, "source": "./skills/claude-api"} + for p in _ANTHROPIC_SKILLS_MANIFEST["plugins"] + ], + } + + async def _get_changed(http_client, url, **kwargs): + return httpx.Response(200, json=changed_manifest) + + monkeypatch.setattr(sync_module, "async_safe_get", _get_changed) + result = await resolve_and_sync(client, marketplace) + + assert result.status == "success" + + refreshed = await client.db.litellm_claudecodeplugintable.find_unique(where={"name": published_name}) + assert refreshed.enabled is False + new_source = json.loads(refreshed.manifest_json)["source"] + assert new_source != original_source + assert new_source["path"] == "skills/claude-api" + + +@pytest.mark.asyncio +async def test_resolve_and_sync_leaves_published_skill_enabled_when_source_unchanged(monkeypatch): + """A re-sync that resolves to the exact same source must not touch an + already-published skill's enabled state.""" + client = _make_fake_prisma_client() + marketplace = await _create_marketplace(client, name="anthropic-agent-skills", source_ref="anthropics/skills") + + async def _get(http_client, url, **kwargs): + return httpx.Response(200, json=_ANTHROPIC_SKILLS_MANIFEST) + + monkeypatch.setattr(sync_module, "async_safe_get", _get) + await resolve_and_sync(client, marketplace) + + published_name = "anthropic-agent-skills--claude-api" + published = await client.db.litellm_claudecodeplugintable.find_unique(where={"name": published_name}) + published.enabled = True + + await resolve_and_sync(client, marketplace) + + refreshed = await client.db.litellm_claudecodeplugintable.find_unique(where={"name": published_name}) + assert refreshed.enabled is True 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 d797a27aa67..5c6d305bfa9 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 @@ -22,6 +22,7 @@ from litellm.proxy.management_helpers.object_permission_utils import ( _rewrite_object_permission_mcp_servers, _set_object_permission, enforce_all_proxy_mcp_servers_grant_is_admin_only, + validate_key_allowed_skills_against_team, validate_key_mcp_servers_against_team, validate_key_search_tools_against_team, validate_key_vector_stores_against_team, @@ -1216,6 +1217,58 @@ async def test_empty_object_permission_passes_for_personal_non_admin(): ) +@pytest.mark.asyncio +async def test_personal_non_admin_cannot_assign_allowed_skills(): + """ + Regression test: a non-admin caller with no team must not be able to + self-assign Claude Code skill access on their own key - allowed_skills + is the authorization boundary get_allowed_skills() reads from directly, + with no team/org ceiling to fall back on for a personal key. + """ + with pytest.raises(HTTPException) as exc: + await validate_key_allowed_skills_against_team( + object_permission={"allowed_skills": ["anthropic-agent-skills--private-skill"]}, + team_obj=None, + is_proxy_admin=False, + ) + assert exc.value.status_code == 403 + assert "anthropic-agent-skills--private-skill" in str(exc.value.detail) + + +@pytest.mark.asyncio +async def test_personal_admin_can_assign_allowed_skills(): + await validate_key_allowed_skills_against_team( + object_permission={"allowed_skills": ["anthropic-agent-skills--private-skill"]}, + team_obj=None, + is_proxy_admin=True, + ) + + +@pytest.mark.asyncio +async def test_team_key_allowed_skills_unrestricted_at_create(): + """Team-scoped keys retain their existing trust model at create time.""" + team_obj = _make_team_obj_search() + await validate_key_allowed_skills_against_team( + object_permission={"allowed_skills": ["anthropic-agent-skills--anything"]}, + team_obj=team_obj, + is_proxy_admin=False, + ) + + +@pytest.mark.asyncio +async def test_empty_allowed_skills_passes_for_personal_non_admin(): + await validate_key_allowed_skills_against_team( + object_permission=None, + team_obj=None, + is_proxy_admin=False, + ) + await validate_key_allowed_skills_against_team( + object_permission={"allowed_skills": []}, + team_obj=None, + 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