fix(mcp): default to disabled, matching docs and least privilege#1377
fix(mcp): default to disabled, matching docs and least privilege#1377sam-fakhreddine wants to merge 4 commits into
Conversation
The MCP guard checked `resolvedConfig.mcp !== false`, injecting the endpoint on every site that didn't explicitly disable it. Changed to `resolvedConfig.mcp === true` so the endpoint is opt-in only. Closes emdash-cms#1228
🦋 Changeset detectedLatest commit: f3d3d85 The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 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 |
|
All contributors have signed the CLA ✍️ ✅ |
github-actions
Bot
added
review/needs-review
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Makes MCP route injection opt-in to match documentation and the principle of least privilege.
Changes:
- Change MCP route injection from default-on (
mcp !== false) to explicit opt-in (mcp === true) - Update
EmDashConfig.mcpdocs to reflect default disabled and@default false - Add a regression test and a changeset documenting the behavior change
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/core/src/astro/integration/index.ts | Switches MCP route injection guard to explicit opt-in |
| packages/core/src/astro/integration/runtime.ts | Updates config documentation to “disabled by default” (@default false) |
| packages/core/tests/unit/mcp/mcp-default-disabled.test.ts | Adds regression test ensuring opt-in behavior |
| .changeset/mcp-default-false.md | Adds release note describing the behavior correction |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the correct security fix: switching MCP from opt-out to opt-in aligns with the documented behavior and the principle of least privilege. The core logic change (!== false → === true) is correct, the JSDoc update is consistent, and the regression test covers the specific bug.
However, the PR's stated scope includes eliminating the public OAuth discovery metadata that fingerprints EmDash instances, and that part is incomplete. The /.well-known/oauth-protected-resource endpoint is injected unconditionally in injectBuiltinAuthRoutes (not behind the new mcp === true guard). It returns resource: ${origin}/_emdash/api/mcp, so it still advertises MCP capability even when the MCP route itself is disabled. The changeset description is therefore overstated. The /.well-known/oauth-authorization-server/_emdash endpoint can remain unconditional because it serves the general OAuth infrastructure (CLI, device code, etc.), not just MCP.
|
I have read the CLA Document and I hereby sign the CLA. |
The /.well-known/oauth-protected-resource endpoint advertises the MCP resource URL, fingerprinting EmDash instances even when MCP is disabled. Move it into injectMcpRoute so both the API and its discovery metadata are conditional on mcp: true. Addresses review feedback from emdashbot and Copilot. Also fixes import.meta.dirname portability and improves JSDoc clarity.
github-actions
Bot
added
review/needs-rereview
|
@copilot recheck please |
Addresses Copilot review feedback — the test now exercises injectMcpRoute with a stubbed injectRoute and asserts on collected route patterns instead of scanning source code strings. Also verifies oauth-protected-resource is co-located with the MCP route.
sam-fakhreddine
force-pushed
the
fix/mcp-default-false
branch
from
de80e49 to
f3d3d85
Compare
What does this PR do?
Changes the MCP route injection guard from
resolvedConfig.mcp !== false(opt-out) toresolvedConfig.mcp === true(opt-in), so the MCP endpoint at/_emdash/api/mcpand its OAuth discovery metadata are only injected when explicitly enabled.This matches the AI Tools documentation which states "The MCP server is disabled by default. Enable it in your Astro configuration:
emdash({ mcp: true })."Without this fix, every EmDash deployment that omits the
mcpconfig key gets a live MCP endpoint plus public OAuth discovery at/.well-known/oauth-protected-resourceand/.well-known/oauth-authorization-server/_emdash. The endpoint requires auth (401 without credentials), but it exposes unnecessary attack surface and allows fingerprinting the site as an EmDash instance. Confirmed independently by @Vallhalen on a production instance.Closes #1228
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
Changes
packages/core/src/astro/integration/index.tsresolvedConfig.mcp !== false→resolvedConfig.mcp === truepackages/core/src/astro/integration/runtime.tsmcpconfig field:@default true→@default false, "Enabled by default" → "Disabled by default"packages/core/tests/unit/mcp/mcp-default-disabled.test.ts=== truenot!== false.changeset/mcp-default-false.md