mirror of
https://github.com/BerriAI/litellm.git
synced 2026-10-06 02:48:13 +00:00
fix(claude-code): preserve approved manifest on source change, not just enabled
Demoting to enabled=False on a source change wasn't enough: get_marketplace() serves a disabled row to anyone with a standing allowed_skills grant, so those callers still received the new, unreviewed source on the next fetch. Freeze the manifest (source + description) entirely when the resolved source changes on an existing row, regardless of its enabled state, so a compromised or malicious upstream can't repoint a skill for granted callers either.
This commit is contained in:
parent
3d24e41121
commit
d79899455f
2 changed files with 43 additions and 24 deletions
|
|
@ -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},
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue