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 316c0235809..918d3ca926d 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 @@ -688,24 +688,37 @@ async def _upsert_single_plugin( ) 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): + # A previously-synced row's approved manifest (source + description) must + # not be silently swapped by whoever controls the upstream marketplace + # repo on the next sync - that would let a compromised/malicious upstream + # repoint a skill without any re-review. This applies regardless of the + # row's current `enabled` state: get_marketplace() serves a disabled row + # to anyone with a standing allowed_skills grant, not just to public + # (enabled=True) callers, so "already published" isn't the only audience + # that could receive the swapped source. Freeze the manifest entirely and + # demote to unpublished so an admin has to look at it again; the new + # source is only recorded in this log line, never served, until the next + # sync after that review (e.g. a delete + re-register of the marketplace). + source_changed = existing is not None and _existing_source_changed(existing.manifest_json, entry) + if source_changed: verbose_proxy_logger.warning( - "skill-marketplace-sync: %r changed source on re-sync, unpublishing pending admin re-review", + "skill-marketplace-sync: %r changed source on re-sync (new source=%r), " + "keeping the previously-approved manifest and unpublishing pending admin re-review", entry.stored_name, + entry.source.model_dump(), ) - update_data["enabled"] = False + update_data: dict[str, Any] = { + "marketplace_id": marketplace_id, + "updated_at": now, + "enabled": False, + } + else: + update_data = { + "description": entry.description, + "manifest_json": manifest_json, + "marketplace_id": marketplace_id, + "updated_at": now, + } await repository.table.upsert( where={"name": entry.stored_name}, 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 15ffab8012c..4bc9b659935 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 @@ -683,11 +683,14 @@ async def test_resolve_and_sync_refuses_to_overwrite_row_owned_by_another_market @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.""" + """Regression test: a skill's git source must not be silently swapped by + a re-sync of the marketplace that owns it - that would let a + compromised/malicious upstream repoint a skill with no admin re-review. + Unlike a plain "unpublish", get_marketplace() still serves a disabled row + to anyone with a standing allowed_skills grant, so the sync must keep the + previously-approved manifest (source) in place rather than overwrite it - + demoting to enabled=False alone would still hand granted callers the new, + unreviewed source.""" client = _make_fake_prisma_client() marketplace = await _create_marketplace(client, name="anthropic-agent-skills", source_ref="anthropics/skills") @@ -700,7 +703,8 @@ async def test_resolve_and_sync_unpublishes_skill_whose_source_changed(monkeypat 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"] + original_manifest_json = published.manifest_json + original_source = json.loads(original_manifest_json)["source"] # A repointed `source` (repo root -> a subdirectory) is exactly what an # upstream marketplace repo owner controls and could change unilaterally. @@ -722,9 +726,11 @@ async def test_resolve_and_sync_unpublishes_skill_whose_source_changed(monkeypat 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" + # The served manifest (and therefore source) is untouched - not just the + # enabled flag - so a caller with a standing allowed_skills grant for + # this row still only ever sees the previously-approved source. + assert refreshed.manifest_json == original_manifest_json + assert json.loads(refreshed.manifest_json)["source"] == original_source @pytest.mark.asyncio