feat(skills): self-service skill submission with admin review - #36677
feat(skills): self-service skill submission with admin review#36677yassin-berriai wants to merge 1 commit into
Conversation
Greptile SummaryThis PR adds self-service skill submission with administrator review and content-bound approval
Confidence Score: 4/5The concurrent reject transition must be made atomic before merging because it can hide a newly approved skill Rejection validates the previously read state but updates only by name, so a concurrent approval between the read and write can be overwritten with rejected and disabled state Files Needing Attention: litellm/proxy/anthropic_endpoints/claude_code_endpoints/claude_code_marketplace.py
|
| Filename | Overview |
|---|---|
| litellm/proxy/anthropic_endpoints/claude_code_endpoints/claude_code_marketplace.py | Implements ownership-aware submission and review transitions, but rejection is not atomically restricted to pending rows |
| litellm-proxy-extras/litellm_proxy_extras/migrations/20260812000000_add_claudecodeplugin_approval_status/migration.sql | Adds approval and review metadata with active defaults for existing rows |
| litellm/proxy/public_endpoints/public_endpoints.py | Filters the public skill hub to active, enabled skills |
| ui/litellm-dashboard/src/app/(dashboard)/skills/_components/ClaudeCodePluginsPanel.tsx | Adds self-service submission and administrator review controls to the skills dashboard |
Reviews (5): Last reviewed commit: "feat(skills): self-service skill submiss..." | Re-trigger Greptile
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
7f02a85 to
a8dd69d
Compare
|
|
|
@greptileai please review the current head a8dd69d. It rebases the reviewed commit onto staging and fixes one unrelated dashboard test |
| "/search_tools/ui/available_providers", | ||
| # Self-service skill submission. Approve/reject stay admin-only. | ||
| "/claude-code/marketplace.json", | ||
| "/claude-code/plugins", |
There was a problem hiding this comment.
Low: Unbounded persistent skill submissions
An internal user can create an arbitrary number of uniquely named pending skills, while the request fields have no length limits, submissions have no per-owner quota, and the list endpoint has no pagination. An attacker can continuously grow this table and force plugin-list requests to load and serialize all of their records; add bounded field lengths, a per-owner pending-submission limit, and pagination before exposing the create route.
| enabled=p.enabled, | ||
| approval_status=_as_approval_status(p.approval_status), | ||
| manifest_fingerprint=_manifest_fingerprint(p.manifest_json), | ||
| review_notes=p.review_notes, |
There was a problem hiding this comment.
Low: Review and owner metadata disclosed across users
The non-admin filter includes every active skill, and these fields are then returned unconditionally. An unrelated internal user can enumerate submitter and reviewer IDs, along with any approval notes, for all approved skills; only include review metadata for proxy admins and callers matching the resource owner.
PR overviewThis pull request adds self-service skill submission, admin review, and marketplace listing for approved skills. The submission flow currently allows internal users to create unlimited, potentially oversized pending records and trigger unpaginated listings, creating persistent storage and response-amplification risk. Approved-skill listings also disclose submitter, reviewer, and approval metadata to unrelated internal users. Both issues remain open, so access controls and abuse limits should be tightened before rollout. Open issues (2)
Fixed/addressed: 0 · PR risk: 6/10 |
|
bugbot run |
| }, | ||
| ) | ||
| if reviewed_rows == 0: | ||
| raise _stale_review_error(plugin_name) |
There was a problem hiding this comment.
CAS check ignores BatchPayload count
High Severity
update_many from prisma-client-py returns a BatchPayload with a .count field, but the compare-and-set path treats the return value as an integer and compares it to 0. When a submitter edit lands between the read and the write, the update matches no rows, yet the == 0 check never fires, so approve still returns success while the skill stays unpublished.
Reviewed by Cursor Bugbot for commit a8dd69d. Configure here.
There was a problem hiding this comment.
Checked against real Postgres: prisma-client-py update_many returns a plain int (0 on miss), no BatchPayload. Evidence in the PR description
a8dd69d to
ca1a4e5
Compare
|
@greptileai please review the current head ca1a4e5. It fixes two bugbot findings; reasoning and the third finding's rebuttal are in the PR description |
ca1a4e5 to
5cb1e40
Compare
|
@greptileai please review the current head 5cb1e40. It rebases onto staging, absorbing a typing refactor in public_endpoints. The feature diff is otherwise unchanged |
|
bugbot run |
| "created_at": datetime.now(timezone.utc), | ||
| "updated_at": datetime.now(timezone.utc), | ||
| "created_by": user_api_key_dict.user_id, | ||
| "created_by": owner_scope, |
There was a problem hiding this comment.
Register leaks hidden skill names
Medium Severity
register_plugin returns 409 whenever a name is taken, including for pending or rejected skills the caller cannot see. get_plugin, update_plugin, and delete_plugin intentionally answer 404 for those rows so names cannot be enumerated, but POST still confirms a hidden submission exists under that name.
Reviewed by Cursor Bugbot for commit 5cb1e40. Configure here.
There was a problem hiding this comment.
Correct, and my earlier 'complete set' claim was wrong. Skill names are globally unique, so a create cannot hide a collision. Reasoning in the description
Resolves LIT-5465
5cb1e40 to
55a80ad
Compare
|
@greptileai please review the current head 55a80ad. It restricts reject to unapproved skills and answers the register name-collision finding in the description |
|
bugbot run |
| where={ # mutable-ok: prisma query arguments must be plain dicts | ||
| "name": plugin_name, | ||
| **({"manifest_json": existing.manifest_json} if publishes else {}), | ||
| }, |
There was a problem hiding this comment.
Concurrent rejection overwrites approval
If approval and rejection overlap, rejection updates by name only and can overwrite the active state, hiding the newly approved skill
| where={ # mutable-ok: prisma query arguments must be plain dicts | |
| "name": plugin_name, | |
| **({"manifest_json": existing.manifest_json} if publishes else {}), | |
| }, | |
| where={ # mutable-ok: prisma query arguments must be plain dicts | |
| "name": plugin_name, | |
| **( | |
| {"manifest_json": existing.manifest_json} | |
| if publishes | |
| else {"approval_status": SKILL_PENDING_REVIEW} | |
| ), | |
| }, |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 55a80ad. Configure here.
| "enabled": publishes, | ||
| "updated_at": reviewed_at, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Reject race demotes approved skills
High Severity
_record_review blocks rejecting an already-approved skill on the pre-read, but the reject update_many only matches on name. If an approve lands after that read, the reject write still succeeds and sets approval_status to rejected with enabled false, unpublishing the skill and hiding it from non-owners—the same takedown the sequential guard was meant to prevent.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 55a80ad. Configure here.


TLDR
Problem this solves:
How it solves it:
User Flow
Before: a developer who wants their team's skill on the gateway cannot add it themselves, so an admin has to do every submission by hand
After: the same developer submits it themselves, and it stays private until an admin approves the exact content they read
"approval_status": "pending_review"and"enabled": falseclaude plugin installcannot pick it up yetAnother user who has nothing to do with the submission cannot see a pending or rejected skill at all: GET http://localhost:4000/claude-code/plugins omits it and GET http://localhost:4000/claude-code/plugins/{name} returns 404 for them, while the submitter and admins can read it
Relevant issues
Linear ticket
Resolves LIT-5465
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Screenshots / Proof of Fix
Captured against a live proxy on a real Postgres 16, driving the real HTTP routes. Both legs use the same database and the same requests, and each leg mints its own
internal_userkey and refuses to run if that key comes back empty, so a refusal below can never be an artifact of a missing keyBefore, at
b0626cad8c(unmodifiedlitellm_internal_staging), proxy on :4465After, at
7f02a85d29, proxy on :4466Steps 5 to 7 are the case the fingerprint exists for. Without it the approve in step 6 succeeds and publishes
acme/swapped-in-after-review, a source no administrator ever looked atBoth legs were captured at the commits named above. The head is now
5cb1e405e2, which is7f02a85d29carried forward onto160548d40bwith the review fixes described below. The rebase onto160548d40bchanged the feature diff in exactly two lines, both of them absorbing a typing refactor staging landed in6b5249bcce:public_skill_hubnow reaches the table through staging's_plugin_tablehelper rather than the repository directly, and that helper'sfind_manyprotocol widened fromMapping[str, bool]toMapping[str, object], since the published-skill filter carries anapproval_statusstring alongsideenabled. Nothing else moved. The review fixes do change three refusal paths, so the numbers above still describe steps 1 to 10, and the new behaviour is covered by the tests named below rather than by this captureSettling the compare-and-set question raised in review, against a real Postgres 16 and a prisma client generated from this branch's schema
The miss is the exact state a submitter edit landing between the admin's read and the approve write produces: the
whereno longer matches, nothing is written, and the skill stays unpublishedUI screenshots for the submit form, the pending badge, the review queue, and the approve and reject dialogs are below
Type
🆕 New Feature
Caveats (if any)
ui-unit-testswas red on 12 failures inmemory,workflowsandguardrails-monitor, none of which this PR's feature touches. They failed identically on unmodifiedb0626cad8c, and this PR only ran those files at all becausevitest relatedpulls in most of the dashboard whenever the generatedschema.d.tsmoves. Staging has since fixed all three, so the one-lineworkflowsfix this PR was carrying dropped out of the rebase and the diff is now the feature aloneReview notes
Cursor Bugbot has filed five findings across two rounds. Four were real and are fixed in the current head
55a80add53; one rests on a premise that does not hold for this client, and the run above is why. The first round landed ona8dd69d9db, the second on5cb1e405e2after the first round's fixesUpdate and delete leak hidden skills (Medium), fixed.
get_plugin404s a skill the caller cannot see, butupdate_pluginanddelete_plugin403'd once the row existed, so the status code told any internal user that a name was taken by someone else's pending submission. Both now refuse through the same_caller_can_seecheck thatget_pluginuses, so a hidden skill answers exactly as an absent one does. A published skill is visible to everyone, so a non-owner touching one still gets 403 and that boundary is pinned by its own test.enable,disable,approveandrejectgate onis_proxy_adminbefore any read, so a non-admin never reaches their lookupThe repo does both elsewhere, so worth naming the precedent rather than asserting a convention: the container ownership checks return a bare 403 (
container_endpoints/ownership.py:228and:341), while the nearest neighbour by concept, the skills handler, folds the two cases together on purpose and says why in a comment, "Same 'not found' message for both 'missing' and 'cross-tenant' so callers can't enumerate skill IDs they don't own" (llms/litellm_proxy/skills/handler.py:156). Sameresource_ownershipmodule, same feature area, and the same shape asget_plugin. The decisive argument is internal though: without this change a caller 404s on read and 403s on update for one rowOrphaned submissions lack owner stamp (Medium), fixed.
register_pluginstampedcreated_byfromuser_id or get_primary_resource_owner_scope(...)and never rejected when both were absent, leaving a pending row its own submitter could not list, read, update or withdraw.resource_ownership.pyalready documents that contract: callers that depend on a primary scope "must surface that as a hard error rather than fall back to a shared sentinel". Submissions with no attributable owner are now refused, socreated_byis non-null for every row in the review queueThe reachable path is narrower than the finding implies, and worth stating so the severity is judged on evidence. Master-key auth populates both
api_keyanduser_idand is proxy admin anyway, and a virtual key always carriestoken(user_api_key_auth.py:2070-2072), so neither can produce this. What does is no-auth dev mode: withmaster_keyunset,user_api_key_auth.py:1476-1487returns aUserAPIKeyAuthcarrying onlyuser_role=INTERNAL_USER, and a request with no Authorization header leavesapi_keyas"". That caller is identity-less, is not an admin, and/claude-code/pluginsadmits internal users, so it lands on the orphan branch. So it needs a proxy with no authentication configured at all, which is exactly the deployment where an unwithdrawable pending row is least likely to be noticedCAS check ignores BatchPayload count (High), not reproducible. The finding says
update_manyreturns aBatchPayloadwhose.countfield makesreviewed_rows == 0dead code. That is the TypeScript client's shape. prisma-client-py 0.11.0 defines noBatchPayloadat all:grep -rn BatchPayloadover the installed package returns nothing, and all 70 generatedupdate_manymethods inprisma/actions.pyannotate-> int, with no other return annotation among them, so this is the generator's contract rather than a per-table accident. The one this code calls isLiteLLM_ClaudeCodePluginTableActions.update_manyatprisma/actions.py:64454, whose body returnsint(resp["data"]["result"]["count"]). Nor does the config-sync wrapper reshape it, and by a stronger route than I first wrote here:litellm_claudecodeplugintableis absent from_CONFIG_SYNCED_TABLE_NAMES, sowrap_table_actions_for_config_syncreturns the actions object unwrapped and no wrapper is in the path at all. The capture above drives it end to end rather than arguing from the source: a non-matchingwherereturns0, anintwith no.count,== 0isTrue, and the row stayspending_reviewwithenabled=False. To be precise about what the suite does and does not show: the compare-and-set branch is covered, bytest_approval_write_does_not_publish_an_edit_that_lands_after_the_fingerprint_check, which lands a submitter edit inside the window so the pre-check passes and only the write can catch it. What that test cannot settle is the return type, because its fake table returnslen(matched), an int by construction, so it would stay green under a client that returned an object. That is the gap the run above closes, and it is why this was worth answering against a real database rather than from the sourceRegister leaks hidden skill names (Medium), not fixable in scope, and the paragraph above used to overclaim. The second round pointed out that
register_pluginstill answers 409 for a name held by someone else's hidden submission, so POST remains a probe the read and mutate routes no longer are. That is correct, and an earlier version of this description called get, update and delete "the complete set", which was wrong. Register is a fourth probeIt cannot be closed without changing what a skill name is.
nameisString @uniqueinschema.prisma, globally, so a create against a taken name cannot both refuse and stay silent: any response distinguishable from success is an oracle, and the only indistinguishable one would be to claim the submission was accepted and drop it. Scoping submissions per owner instead,@@unique([name, created_by]), would genuinely close it, but the published namespace has to stay unique becausemarketplace.jsonkeys on the name andclaude plugin install <name>resolves by it, so that is a schema change reaching every by-name route rather than something to slip into this PRWhat remains is narrower than the read-route leak was.
_name_conflict_errornames neither the owner nor the review state, only that the name is taken, and a non-adminGET /claude-code/pluginsstill lists active plus their own, so there is no way to enumerate submissions. It is a guess-confirm oracle on a name the caller already has in hand, which is the same property any globally unique namespace has at signup. Happy to take the composite-key design as its own ticket if that trade is worth makingReject revokes published skills (Medium), fixed.
_record_reviewaccepted a reject from any state other than rejected, includingactive. Because_caller_can_seekeys onapproval_status, rejecting an approved skill did more than unpublish it: it hid the skill from every non-owner, wheredisableleaves it visible and merely takes it out of the public lists. That is a takedown wearing a review step's name, and both docstrings describe reject as applying to a submitted skill. An approved skill now refuses the reject with a 400 that points atdisable, andtest_rejecting_an_approved_skill_is_refused_and_leaves_it_publishedpins it by asserting the skill is still active, still enabled, still carries no review note and still appears inmarketplace.json. Removing the guard fails that testNo privilege boundary moved here, since an admin can still delete the skill outright. The reason to fix it is that reject and disable had silently different blast radii, and only one of them was documented
Five new tests cover this, and each was checked by mutation: restoring the pre-fix shape in
update_plugin, indelete_plugin, dropping the identity guard, and dropping the approved-skill reject guard each fail their own test, and hiding published skills too fails the 403 boundary test. The three carried over from the first round were re-run at the current head rather than assumed to survive the rebaseTwo older
veriafindings are deliberately not addressed here, since both are about the shape of the feature rather than a defect in it, and both want an API change this PR's scope does not carry. Submissions are unbounded (no per-owner quota, no field length caps, no pagination on the list route), and review metadata (created_by,reviewed_by,review_notes) is returned to any internal user for an active skill. Happy to take either in this PR or a follow-upFinal Attestation