Harden plugin MCP tool registration#988
Conversation
🦋 Changeset detectedLatest commit: e7f2a51 The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Scope checkThis PR changes 683 lines across 26 files. Large PRs are harder to review and more likely to be closed without review. If this scope is intentional, no action needed. A maintainer will review it. If not, please consider splitting this into smaller PRs. See CONTRIBUTING.md for contribution guidelines. |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-verification
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
Overlapping PRsThis PR modifies files that are also changed by other open PRs:
This may cause merge conflicts or duplicated work. A maintainer will coordinate. |
|
/review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid implementation overall — capability gating, hex-encoded tool names to avoid plugin-id collisions, admin scope + ADMIN role enforced before dispatch, and good test coverage (capability filter, sandbox-not-loaded guard, scope check, ID collision). The delegation to handlePluginApiRoute is a clean reuse strategy.
Findings are mostly DX/footgun concerns at the definePlugin boundary plus a couple of manifest cross-validation gaps. No correctness or auth bugs that block merge.
Notes:
- Trusted (
definePlugin) plugins skip several validations that sandboxed plugins get frompluginManifestSchema— tool-name shape, route existence, and capability declaration. Silent filtering vs. early errors. getPluginRouteMetafalls back to{ public: false }for any route on an existing sandboxed plugin, so MCP tools pointing at nonexistent sandboxed routes register fine and fail at call time. The catch-all route has the same behavior, so this matches existing semantics — flagging only as DX context.- One coverage gap: no test for
requireRole(ADMIN)rejecting a non-admin user holding anadmin-scoped token (the defense-in-depth branch). - Sandboxed plugin MCP tools have no input schema in
ManifestMcpToolEntry, so they all fall back toz.record(z.string(), z.unknown())— MCP clients lose introspection. Future work, but worth tracking.
|
Review submitted successfully (review ID 4270626524) with 6 line-anchored comments and a summary body. Key findings:
No blocking issues — auth/correctness paths check out. |
ascorbic
left a comment
ascorbic
left a comment
There was a problem hiding this comment.
Excellent work. One thing that would be good would be if there was a way we could provide a schema for tool inputs so we can validate and describe them. While we can't serialise a Zod schema directly, maybe a JSON schema would work?
Can you include docs in the PR too, as it will also make it easier to review.
Agreed on both. I’ll add docs in this PR, and I’ll look at adding a constrained I’ll keep the JSON Schema subset conservative: object root, No |
|
Done. I added docs for plugin MCP tools and added manifest-side |
|
/review |
There was a problem hiding this comment.
Reviewing on top of the existing rounds (ask-bonk + ascorbic). Most of the substantive concerns from the earlier passes — capability cross-check, route cross-check, tool-name shape validation, JSON Schema introspection, dual-check coverage, admin-only intent comment, runtime dedup — have been addressed. A few remaining things worth a look, all DX/robustness rather than auth/correctness:
- One bad
inputSchemafrom a trusted plugin can take down the whole MCP endpoint becausejsonSchemaObjectToZodis called inline duringcreateMcpServerand has no isolation. Trusted plugins skip the Zod manifest validation that sandboxed plugins get, so the JSON Schema is unvalidated untilz.fromJSONSchemathrows. pluginToolNamecollision detection looks for__within a single segment, but two distinct valid scoped IDs can still collapse to the same generated name when a dash sits on either side of the slash boundary (@a-/bvs@a/-bboth →a___b__foo). Narrow but real.- No dedicated unit test file for
pluginToolName— the integration tests cover the happy paths but the ambiguous-segment / hash-fallback branches aren't directly exercised.
No blocking issues from me.
|
This PR has been inactive for 14 days. It will be closed automatically in 7 days if there is no further activity. If you're still working on this, please push an update or leave a comment. |
|
This PR has been inactive for 14 days. It will be closed automatically in 7 days if there is no further activity. If you're still working on this, please push an update or leave a comment. |
# Conflicts: # packages/core/src/astro/integration/runtime.ts # packages/core/src/astro/integration/virtual-modules.ts # packages/core/src/astro/middleware.ts # packages/core/src/astro/routes/api/mcp.ts # packages/core/src/cli/commands/bundle-utils.ts # packages/core/src/emdash-runtime.ts # packages/core/src/mcp/server.ts # packages/core/src/plugin-types.ts # packages/core/src/plugins/adapt-sandbox-entry.ts # packages/core/src/plugins/define-plugin.ts # packages/core/src/plugins/manifest-schema.ts # packages/core/src/plugins/types.ts # packages/core/tests/unit/astro/middleware-prerender.test.ts # packages/core/tests/unit/cli/bundle-utils.test.ts # packages/core/tests/unit/plugins/manifest-schema.test.ts # packages/plugin-cli/src/bundle/types.ts # packages/plugin-cli/src/bundle/utils.ts # packages/plugin-cli/tests/bundle-utils.test.ts # packages/plugin-types/src/index.ts # pnpm-lock.yaml
masonjames
changed the title
masonjames
added
review/ready-for-maintainer
and removed
review/needs-rereview
github-actions
Bot
added
review/needs-rereview
2 participants
What does this PR do?
Resolves the branch against current
mainafter #2002 landed the approved plugin MCP tool contract. The conflict resolution keeps that newer contract authoritative and retains two hardening improvements from this branch:This PR no longer introduces the earlier parallel
mcpToolsmanifest surface.Follow-up to #2002 and Discussion #850.
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain.AI-generated code disclosure
Screenshots / test output
pnpm typecheckpnpm lint:json— 0 diagnosticspnpm formatpnpm changeset status --since=origin/mainpnpm --filter emdash exec vitest run tests/unit/mcp/authorization.test.ts tests/unit/mcp/json-schema.test.ts tests/unit/mcp/plugin-tool-name.test.ts— 52 tests passedd6ff36871aeee3a12b2e36010e2b6db9d44a0d35, including unit, integration, smoke, browser, Node E2E, Cloudflare E2E, lint, typecheck, changeset, and security checksLocal full-core Vitest completed with 4,976 passing tests and one unchanged macOS temp-path normalization failure in
virtual-modules.test.ts(/var/foldersversus/private/var/folders). The focused tests and Linux CI do not reproduce it.