Skip to content

docs: add plugin-acceptance security review and repo security policy#33

Merged
kyle-sexton merged 1 commit into
mainfrom
docs/plugin-security-gate
Jul 9, 2026
Merged

docs: add plugin-acceptance security review and repo security policy#33
kyle-sexton merged 1 commit into
mainfrom
docs/plugin-security-gate

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Summary

Closes the process gap a third-party fork surfaced: it forked this marketplace and added a plugin bundling a remote MCP server (.mcp.json → external endpoint + Bearer ${API_KEY}). Our technical controls stop a malicious PR from silently merging, but there was no written security gate for the judgment call of accepting a plugin that executes code or delegates trust to a third party.

  • docs/MIGRATION-PLAYBOOK.md — new Plugin-acceptance security review section next to the per-plugin migration gate. Six surfaces, deny-by-default: code-execution hooks, remote MCP servers (net-new — no current plugin ships one), userConfig secrets (sensitive → keychain), cache isolation, data egress/telemetry, and provenance/third-party trust. Grounded in the plugins + MCP reference (verified 2026-07-09).
  • CLAUDE.md — a "Security-reviewed" design rule + pointer.
  • SECURITY.md (new) — repo-specific policy mirroring the org channel (GitHub private vulnerability reporting + security@melodicsoftware.com) with plugin-supply-chain reporting guidance.

Private vulnerability reporting was enabled on this repo out-of-band this session (recorded in github-iac).

Test plan

  • markdownlint-cli2 clean on all three files
  • No plugin manifest / marketplace.json change → claude plugin validate unaffected
  • CI ci-status green

🤖 Generated with Claude Code


Note

Low Risk
Documentation-only changes with no plugin manifests, hooks, or runtime behavior modified.

Overview
Adds a written security gate for accepting marketplace plugins, motivated by third-party forks that could add remote MCP servers and trust delegation without an explicit review bar.

docs/MIGRATION-PLAYBOOK.md gains a Plugin-acceptance security review section (deny-by-default) covering hooks/code execution, remote MCP, userConfig secrets, cache isolation, telemetry/egress, and provenance—plus a requirement to record accept/deny rationale when surfaces 2, 5, or 6 apply.

CLAUDE.md adds a Security-reviewed design rule and points the process section at that playbook section.

SECURITY.md (new) defines how to report vulnerabilities privately (GitHub advisory flow or security@melodicsoftware.com) and what to include for plugin supply-chain issues (version, hooks, MCP endpoints).

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

Closes the process gap surfaced by a third-party fork that added a plugin
bundling a remote MCP server: the marketplace had no security gate for
accepting plugins that execute code or delegate trust to a third party.

- MIGRATION-PLAYBOOK.md: new "Plugin-acceptance security review" section
  adjacent to the migration gate — six surfaces (code execution, remote MCP
  servers, userConfig secrets, cache isolation, data egress, provenance),
  deny-by-default, grounded in the plugins/MCP reference (verified 2026-07-09).
- CLAUDE.md: a "Security-reviewed" design rule + pointer.
- SECURITY.md (new): repo-specific policy mirroring the org channel (GitHub
  private vulnerability reporting + security@melodicsoftware.com), with
  plugin-supply-chain reporting guidance. Private vulnerability reporting was
  enabled on this repo this session.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J1pUFM3DckCZ8iW1JaviZj
@claude

claude Bot commented Jul 9, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 2m 22s —— View job


PR Review: docs/plugin-security-gate

  • Gather context and read changed files
  • Review CLAUDE.md changes
  • Review SECURITY.md (new file)
  • Review docs/MIGRATION-PLAYBOOK.md changes
  • Verify alignment with CLAUDE.md guidelines and fresh-docs mandate
  • Post final review

This PR adds well-structured security process documentation that fills a real gap. The six-surface checklist in the playbook is technically grounded and the deny-by-default stance is correct for a public marketplace. A few findings below, ranging from a potential correctness issue to minor improvements.


Findings

1. Potential: Email domain mismatch in SECURITY.md

SECURITY.md L16

email **<security@melodicsoftware.com>**

The GitHub org slug is melodic-software (hyphenated). Many orgs drop the hyphen for email domains, but melodicsoftware.com (no hyphen) vs melodic-software.com is worth a double-check. A broken security contact is a bad look — reporters give up. Fix this →


2. CLAUDE.md design rule scope says "before publish" but version bumps are in scope too

CLAUDE.md (new bullet)

Every plugin clears the playbook's plugin-acceptance security review before publish

The playbook says "new, or a version bump that adds a trust surface" and "a later version bump that introduces a new surface re-triggers this review." The summary in CLAUDE.md only says "before publish," which an AI agent could reasonably interpret as first-publish only and skip re-review on bumps. Suggest:

"…before publish and on any version bump that introduces a new trust surface."

Fix this →


3. "Record accept/deny" has no stated location

docs/MIGRATION-PLAYBOOK.md L124

Record accept/deny + rationale for any plugin touching surfaces 2, 5, or 6

The instruction doesn't say where to record the decision (PR description? a dedicated log file? a comment in plugin.json?). Without a home, this will be inconsistently applied — some reviewers will drop a note in the PR, others will skip it entirely. Recommend naming the artifact, e.g.:

"Record accept/deny + rationale in the PR description under a Security review heading…"

Fix this →


4. Surface 1 kill switch phrasing is ambiguous (MUST vs check)

docs/MIGRATION-PLAYBOOK.md L97-98

a kill switch (`HOOK_<PLUGIN>_ENABLED`) exists.

The other items in the surface-1 checklist are stated as requirements ("no eval", "never flows unquoted"), but the kill switch reads as a check question ("does one exist?"). For consistency with the rest of the review's deny-by-default stance this should be stated as a requirement: "must expose a kill switch." Minor, but AI agents parsing this as a checklist will apply it inconsistently.


5. "Subject to per-server approval" in Surface 2 is under-explained

docs/MIGRATION-PLAYBOOK.md L100

starts automatically when the plugin is enabled (subject to per-server approval)

A reader unfamiliar with the Claude Code trust dialog won't know what "per-server approval" means here or whether it provides meaningful protection. A brief parenthetical ("Claude Code prompts the consumer to trust each server on first use") would make the downstream risk clearer.


What's well done

  • The deny-by-default framing (not opt-in) is correct for a public supply chain.
  • Surface 2 on remote MCP is the sharpest section: explicitly flags prompt-injection risk, requires naming vendor + data + token scope, and states "do not accept" without recorded decision. This directly addresses the motivating fork.
  • Surface 3 correctly distinguishes sensitive (keychain) vs non-sensitive (settings.json) — a subtle but important distinction.
  • Fresh-docs mandate compliance: the section header cites 2026-07-09 verification date, matching the mandate's requirement.
  • SECURITY.md scope statement is accurate ("skills, hooks, and agents") and the plugin-specific guidance for reporters (include version from plugin.json, name the command/endpoint) is actionable and good.

Overall: approve with the email domain verified and finding #2 (version bump scope) addressed. The others are improvements but not blockers.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Risk: low. Cursor Bugbot completed successfully with no findings. Documentation-only changes adding security policy and migration playbook guidance; approving without additional reviewers.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Router and Approver

@kyle-sexton
kyle-sexton merged commit bff46e1 into main Jul 9, 2026
22 checks passed
@kyle-sexton
kyle-sexton deleted the docs/plugin-security-gate branch July 9, 2026 19:43
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.

1 participant