Skip to content

Security/plugin hardening#393

Draft
pontusab wants to merge 10 commits into
mainfrom
security/plugin-hardening
Draft

Security/plugin hardening#393
pontusab wants to merge 10 commits into
mainfrom
security/plugin-hardening

Conversation

@pontusab
Copy link
Copy Markdown
Collaborator

@pontusab pontusab commented May 23, 2026

Note

High Risk
Changes affect plugin install payloads, repository pinning, security rescans, and star-count integrity via a new SECURITY DEFINER RPC—areas that directly impact user trust and marketplace data correctness.

Overview
Hardens plugin create, update, star, and detail flows against trust and install-safety issues.

Create resolves github_repo_id server-side from the repository URL (GitHub API) instead of trusting the client, then passes it into insertPlugin for idempotent repo identity.

Update locks the repository URL when github_repo_id is set, fingerprints only install-relevant component fields (content/metadata/repository), deactivates and resets scan flags when those change, and enqueues rescans only when needed. The edit form mirrors the lock in the UI.

Stars replace the multi-step admin/user split with a single toggle_plugin_star RPC on the user-scoped Supabase client; a migration adds a unique (plugin_id, user_id) constraint, serializes toggles, recomputes star_count from plugin_stars, and drops the old increment/decrement helpers.

Plugin detail URL-encodes MCP install deeplink base64, hides one-click install when the plugin is inactive (content still visible), and expands MCP rows with a config preview before install.

GitHub parse exports resolveGithubRepoIdFromRepository for the create path.

Reviewed by Cursor Bugbot for commit 1ccb658. Bugbot is set up for automated code reviews on this repo. Configure here.

pontusab and others added 5 commits May 23, 2026 13:06
`buildMCPInstallDeepLink` base64-encoded the MCP config and dropped the
result straight into the URL query string. Standard base64 contains `+`,
`/`, and `=`; in `application/x-www-form-urlencoded` parsing — which
Cursor's deeplink handler uses — `+` decodes to a space. The receiving
end then base64-decodes a string with spaces in place of `+`, producing
garbage bytes, and `JSON.parse` throws "Not valid JSON".

Wrap the base64 value in `encodeURIComponent` so the bytes survive the
URL trip end-to-end. Other deeplink builders (rule, command) already
encode their payloads; MCP was the outlier.

Co-authored-by: Cursor <cursoragent@cursor.com>
`starPluginAction` did three independent round-trips per click — a
`select` against `plugin_stars`, an `insert`/`delete`, and a separate
`increment_star_count` / `decrement_star_count` RPC — with no
serialization. N concurrent toggles from the same authenticated user
all saw the same "not yet starred" state, all ran the increment, and
the join-table unique constraint silently swallowed the duplicate
inserts without rolling back the count update. Net effect: a single
account could inflate or drive negative any plugin's `star_count` at
will by firing concurrent requests.

Replace the three-step dance with a single SECURITY DEFINER RPC
`toggle_plugin_star(uuid)` that runs in one transaction:

  * takes a row lock on the plugin row
  * upserts/deletes the join-table row idempotently
  * recomputes `star_count` from `count(*) from plugin_stars` so the
    cached count is *derived*, not deltaed — even pathological inputs
    can't make it drift from the source of truth

Also adds the (plugin_id, user_id) unique constraint that the old code
implicitly assumed but never enforced, drops the now-unused
`increment_star_count` / `decrement_star_count` helpers so no future
code can accidentally re-introduce the drift, and back-fills any
already-skewed counts.

The action now calls the new RPC via the user-scoped supabase client;
`auth.uid()` inside the SECURITY DEFINER body provides the authn
check, so the function can't be used to star on behalf of another
user.

Co-authored-by: Cursor <cursoragent@cursor.com>
`updatePluginAction` enqueued the security scan after PR #381 but never
touched `active` or `scan_status`, so an owner of an approved plugin
could swap the components in-place and have the new install commands
live the moment the request returned — the scan would then run against
content that was already being installed by anyone who hit the page.
This is the missing symmetric branch of the create flow's "publish
gates on a passing scan".

When the install-relevant content changes — components or the
repository URL — the action now:

  * Sets `active = false`, `scan_status = 'pending'` and clears the
    stale flag fields, so the plugin is hidden from listings and the
    detail page renders the "Plugin is being verified" banner that
    already exists for fresh submissions.
  * Enqueues the scan as before; `applyVerdict` re-enables the plugin
    on a clean verdict.

Cosmetic-only edits (name, description, logo, keywords, homepage)
still skip the scan and stay live, so a typo fix doesn't burn a scan
slot or briefly de-list the plugin.

`PluginDetailView` also gates the per-component install buttons on
`plugin.active`. Component content (rule body, MCP config, command
text) stays visible so owners can still review the new payload while
the scan is in flight; we just don't hand visitors a one-click
install for an unvetted payload.

Co-authored-by: Cursor <cursoragent@cursor.com>
Every other component type (rules, commands, hooks, skills, agents,
LSPs) renders its content on the page so a visitor can read it before
clicking Add to Cursor. MCPs are the only type whose install actually
spawns a subprocess on the host (`command` + `args` + `env`), and they
were also the only type that didn't show the user what they were
about to run — the card was just a name and an install button.

Rewrite `McpSection` to mirror the `RulesSection` disclosure pattern:

  * Each MCP row is now an expandable button. The pretty-printed
    config (the same one base64'd into the deeplink) is rendered in a
    bordered `<code>` block, with a short note telling the user to
    inspect `command`, `args`, and `env` before continuing.
  * The first MCP is auto-expanded by default, mirroring how the
    first rule is auto-expanded, so the payload is visible without a
    click on the common single-server case.

No deeplink shape change — the build still calls
`buildMCPInstallDeepLink(name, JSON.stringify(resolved.config))` and
the `installable`-gated install controls stay where they were.

Co-authored-by: Cursor <cursoragent@cursor.com>
… one

The "Source" link on the plugin detail page was just `plugin.repository`,
a submitter-controlled URL with no platform-side binding to what the
plugin actually installs. A submitter could submit a plugin via the
GitHub parser (getting the verified-looking "Source: github.com/x/y"
badge for free), then edit the field to point anywhere — or keep the
parsed URL while swapping out the component payload that the URL is
supposed to represent.

When a plugin was created via the GitHub parser we already know the
authoritative GitHub repo id (`plugins.github_repo_id`, deduped by the
`plugins_github_repo_id_unique` index). Use that as the binding:

  * `createPluginAction` now accepts and persists `githubRepoId`, and
    the auto-submission flow in `plugin-form.tsx` passes the value
    `parseGitHubPluginAction` returned. Without this, user-submitted
    plugins parsed from GitHub never recorded a `github_repo_id` and
    the lock below silently never engaged.

  * `updatePluginAction` ignores any submitted `repository` when the
    row carries a `github_repo_id` and re-asserts the existing value,
    so the binding can't be unstuck after the fact.

  * `EditPluginForm` renders the Repository URL field as `readOnly`
    with a "locked to GitHub source" hint when the binding exists, so
    the UI tells the truth about what the server will accept rather
    than silently dropping the user's edit.

Manually-submitted plugins (no parser, no `github_repo_id`) are
unaffected — their Source field stays editable.

Co-authored-by: Cursor <cursoragent@cursor.com>
@vercel
Copy link
Copy Markdown

vercel Bot commented May 23, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
cursor-directory Ready Ready Preview, Comment May 24, 2026 6:14pm

Request Review

Comment thread apps/cursor/src/actions/update-plugin.ts
Comment thread apps/cursor/src/actions/create-plugin.ts
Remove client-supplied githubRepoId from createPluginAction. Derive the
numeric repo id from the repository URL via GitHub API so attackers
cannot squat arbitrary github_repo_id values and block idempotent imports.

Co-authored-by: Lee Robinson <lee@leerob.com>
The edit form does not edit metadata, so comparing it caused false
rescans for GitHub-imported MCP plugins. Preserve stored metadata on
save when the client omits it.

Co-authored-by: Lee Robinson <lee@leerob.com>
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Autofix Details

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: Metadata changes skip rescan
    • Component fingerprints now include the effective saved metadata so install-relevant MCP metadata updates trigger a rescan.

Create PR

Or push these changes by commenting:

@cursor push 8f1c1185c3
Preview (8f1c1185c3)
diff --git a/apps/cursor/src/actions/update-plugin.ts b/apps/cursor/src/actions/update-plugin.ts
--- a/apps/cursor/src/actions/update-plugin.ts
+++ b/apps/cursor/src/actions/update-plugin.ts
@@ -48,19 +48,21 @@
 }
 
 // Only fields that affect the install payload — cosmetic edits to name,
-// description, sort_order, or metadata must not trigger a rescan. The edit
-// form does not surface metadata (GitHub imports store MCP config there).
+// description, or sort_order must not trigger a rescan. MCP install links and
+// configs can live in metadata, so compare the metadata that will be saved.
 function fingerprintComponent(c: {
   type: string;
   slug?: string | null;
   name: string;
   content?: string | null;
+  metadata?: Record<string, unknown> | null;
 }): string {
   const slug = c.slug || slugify(c.name);
   return JSON.stringify({
     type: c.type,
     slug,
     content: c.content ?? "",
+    metadata: c.metadata ?? {},
   });
 }
 
@@ -95,7 +97,20 @@
     .sort((a, b) => a.sort_order - b.sort_order)
     .map(fingerprintComponent)
     .sort();
-  const nextSorted = nextComponents.map(fingerprintComponent).sort();
+  const prevByKey = new Map(
+    prevComponents.map((c) => [
+      componentKey(c.type, c.slug || slugify(c.name)),
+      c,
+    ]),
+  );
+  const nextSorted = nextComponents
+    .map((component) =>
+      fingerprintComponent({
+        ...component,
+        metadata: resolveComponentMetadata(component, prevByKey),
+      }),
+    )
+    .sort();
 
   for (let i = 0; i < prevSorted.length; i++) {
     if (prevSorted[i] !== nextSorted[i]) return true;

You can send follow-ups to the cloud agent here.

Comment thread apps/cursor/src/actions/update-plugin.ts
@leerob
Copy link
Copy Markdown
Collaborator

leerob commented May 24, 2026

@cursor push 8f1c118

Applied via @cursor push command
@leerob
Copy link
Copy Markdown
Collaborator

leerob commented May 24, 2026

@cursor fix this too

./src/actions/update-plugin.ts:262:58
Type error: Cannot find name 'queueError'.
  260 |           kickDrainAfterResponse();
  261 |         } catch (workflowError) {
> 262 |           console.error("Failed to enqueue plugin scan", queueError);
      |                                                          ^
  263 |         }
  264 |       }

@cursor
Copy link
Copy Markdown

cursor Bot commented May 24, 2026

Fixed the type error in update-plugin.ts: the catch block bound workflowError but logged queueError. The handler now uses queueError in both places, matching insert.ts.

Change is committed and pushed to security/plugin-hardening on PR #393.

Open in Web Open in Cursor 

Co-authored-by: Lee Robinson <lee@leerob.com>
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.

Fix All in Cursor

Bugbot Autofix prepared fixes for both issues found in the latest run.

  • ✅ Fixed: Delist before components saved
    • Plugin updates and component rewrites now run through an atomic database function so delist/reset changes roll back if component replacement fails.
  • ✅ Fixed: Scan quota on cosmetic edits
    • The plugin scan rate limit now runs only when an install-relevant change will actually enqueue a rescan.

Create PR

Or push these changes by commenting:

@cursor push 7fc3405725
Preview (7fc3405725)
diff --git a/apps/cursor/src/actions/update-plugin.ts b/apps/cursor/src/actions/update-plugin.ts
--- a/apps/cursor/src/actions/update-plugin.ts
+++ b/apps/cursor/src/actions/update-plugin.ts
@@ -172,13 +172,6 @@
         );
       }
 
-      const { success } = await pluginScanLimit(userId);
-      if (!success) {
-        throw new ActionError(
-          "Too many plugin updates in the last hour. Please try again later.",
-        );
-      }
-
       // Source URL is pinned to the parsed GitHub repo so an owner can't keep
       // the verified-looking badge while swapping the install payload.
       const repositoryLocked = existing.github_repo_id != null;
@@ -197,51 +190,15 @@
       );
 
       const shouldRescan = installChanged;
-      const updatePayload: Record<string, unknown> = {
-        name,
-        description,
-        logo: logo || null,
-        repository: effectiveRepository,
-        homepage: homepage || null,
-        keywords: keywords || [],
-      };
-
-      if (shouldRescan && existing.active) {
-        updatePayload.active = false;
-        updatePayload.scan_status = "pending";
-        updatePayload.flag_summary = null;
-        updatePayload.flag_reasons = [];
-        updatePayload.flag_severity = null;
-        updatePayload.flagged_at = null;
-      }
-
-      const { error: updateError } = await supabase
-        .from("plugins")
-        .update(updatePayload)
-        .eq("id", id);
-
-      if (updateError) {
-        if (updateError.code === "23505") {
+      if (shouldRescan) {
+        const { success } = await pluginScanLimit(userId);
+        if (!success) {
           throw new ActionError(
-            "A plugin with this name already exists. Please choose a different name.",
+            "Too many plugin updates in the last hour. Please try again later.",
           );
         }
-        throw new ActionError(
-          `Failed to update plugin: ${updateError.message}`,
-        );
       }
 
-      const { error: deleteError } = await supabase
-        .from("plugin_components")
-        .delete()
-        .eq("plugin_id", id);
-
-      if (deleteError) {
-        throw new ActionError(
-          `Failed to update components: ${deleteError.message}`,
-        );
-      }
-
       const prevByKey = new Map(
         prevComponents.map((c) => [
           componentKey(c.type, c.slug || slugify(c.name)),
@@ -260,15 +217,32 @@
         sort_order: i,
       }));
 
-      const { error: compError } = await supabase
-        .from("plugin_components")
-        .insert(componentRows);
+      const { error: updateError } = await supabase.rpc(
+        "update_plugin_with_components",
+        {
+          p_plugin_id: id,
+          p_name: name,
+          p_description: description,
+          p_logo: logo || null,
+          p_repository: effectiveRepository,
+          p_homepage: homepage || null,
+          p_keywords: keywords || [],
+          p_components: componentRows,
+          p_deactivate_for_scan: shouldRescan && existing.active,
+        },
+      );
 
-      if (compError) {
+      if (updateError) {
+        if (updateError.code === "23505") {
+          throw new ActionError(
+            "A plugin with this name already exists. Please choose a different name.",
+          );
+        }
         throw new ActionError(
-          `Failed to save plugin components: ${compError.message}`,
+          `Failed to update plugin: ${updateError.message}`,
         );
       }
+
       if (shouldRescan) {
         try {
           await enqueuePluginScan(id);

diff --git a/supabase/migrations/20260524_atomic_plugin_update.sql b/supabase/migrations/20260524_atomic_plugin_update.sql
new file mode 100644
--- /dev/null
+++ b/supabase/migrations/20260524_atomic_plugin_update.sql
@@ -1,0 +1,85 @@
+-- Keep plugin metadata updates and component rewrites in a single transaction.
+-- If a component row fails validation, Postgres rolls back the delist/reset too.
+create or replace function public.update_plugin_with_components(
+  p_plugin_id uuid,
+  p_name text,
+  p_description text,
+  p_logo text,
+  p_repository text,
+  p_homepage text,
+  p_keywords text[],
+  p_components jsonb,
+  p_deactivate_for_scan boolean
+)
+returns void
+language plpgsql
+set search_path = public
+as $$
+begin
+  update public.plugins
+  set
+    name = p_name,
+    description = p_description,
+    logo = p_logo,
+    repository = p_repository,
+    homepage = p_homepage,
+    keywords = coalesce(p_keywords, '{}'::text[]),
+    active = case when p_deactivate_for_scan then false else active end,
+    scan_status = case
+      when p_deactivate_for_scan then 'pending'
+      else scan_status
+    end,
+    flag_summary = case when p_deactivate_for_scan then null else flag_summary end,
+    flag_reasons = case
+      when p_deactivate_for_scan then '{}'::text[]
+      else flag_reasons
+    end,
+    flag_severity = case when p_deactivate_for_scan then null else flag_severity end,
+    flagged_at = case when p_deactivate_for_scan then null else flagged_at end
+  where id = p_plugin_id;
+
+  if not found then
+    raise exception 'plugin not found'
+      using errcode = 'P0002';
+  end if;
+
+  delete from public.plugin_components
+  where plugin_id = p_plugin_id;
+
+  insert into public.plugin_components (
+    plugin_id,
+    type,
+    name,
+    slug,
+    description,
+    content,
+    metadata,
+    sort_order
+  )
+  select
+    p_plugin_id,
+    component.type,
+    component.name,
+    component.slug,
+    component.description,
+    component.content,
+    coalesce(component.metadata, '{}'::jsonb),
+    component.sort_order
+  from jsonb_to_recordset(p_components) as component(
+    type text,
+    name text,
+    slug text,
+    description text,
+    content text,
+    metadata jsonb,
+    sort_order integer
+  );
+end;
+$$;
+
+revoke execute on function public.update_plugin_with_components(
+  uuid, text, text, text, text, text, text[], jsonb, boolean
+) from public, anon, authenticated;
+grant execute on function public.update_plugin_with_components(
+  uuid, text, text, text, text, text, text[], jsonb, boolean
+) to service_role;

You can send follow-ups to the cloud agent here.

Reviewed by Cursor Bugbot for commit 1ccb658. Configure here.

homepage: homepage || null,
keywords: keywords || [],
})
.update(updatePayload)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delist before components saved

High Severity · Logic Bug

When an install-relevant edit runs on an active plugin, the row is set active=false and scan fields reset before components are deleted and re-inserted. If the component insert fails after the delete succeeds, the plugin stays delisted with no components, even though the action returns an error.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 1ccb658. Configure here.

effectiveRepository,
);

const shouldRescan = installChanged;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scan quota on cosmetic edits

Medium Severity · Logic Bug

pluginScanLimit (5/hour per user per firewall config) still runs on every update before shouldRescan is computed. Cosmetic-only saves no longer enqueue scans, but they still consume the scan rate limit and can block further edits with “Too many plugin updates in the last hour.”

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 1ccb658. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants