fix(core): return OAuth discovery 401 for unauthenticated MCP POSTs#371
Conversation
🦋 Changeset detectedLatest commit: 5f83a8d The changes in this PR will be included in the next version bump. This PR includes changesets to release 15 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 |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/blocks
@emdash-cms/cloudflare
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@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-forms
@emdash-cms/plugin-webhook-notifier
commit: |
ascorbic
left a comment
There was a problem hiding this comment.
Thanks for the PR. The fix is in the right area, but I think there's a simpler approach. No MCP client will ever authenticate via session cookies, so we don't need to preserve CSRF for session-backed MCP requests: we can just make MCP bearer-only. I've made a couple of inline suggesitons, but it might need a few more as I've not tested. Basically for MCP requests we want to exempt from CSRF, but only allow token auth.
| const isOAuthConsent = url.pathname.startsWith("/_emdash/oauth/authorize"); | ||
| if ( | ||
| isApiRoute && | ||
| !isTokenAuth && | ||
| !isOAuthConsent && |
There was a problem hiding this comment.
| const isOAuthConsent = url.pathname.startsWith("/_emdash/oauth/authorize"); | |
| const isMcpEndpoint = url.pathname === "/_emdash/api/mcp"; | |
| if ( | |
| isApiRoute && | |
| !isTokenAuth && | |
| !isOAuthConsent && | |
| !isMcpEndpoint && |
There was a problem hiding this comment.
@ascorbic Updated this to match the broader bearer-only direction. MCP now short-circuits before session or external auth: requests without a valid bearer token return the discovery-style 401 with WWW-Authenticate, and session-backed MCP requests are no longer accepted even if the CSRF header is present.
I also updated the regression test to assert that MCP POST discovery no longer reads session state at all. Verified with pnpm --filter emdash test tests/unit/auth/mcp-discovery-post.test.ts, pnpm test tests/unit/auth/*.test.ts, pnpm typecheck, and pnpm --silent lint:json | jq .diagnostics | length.
| { | ||
| status: 401, | ||
| headers: { | ||
| "WWW-Authenticate": `Bearer resource_metadata="${url.origin}/.well-known/oauth-protected-resource"`, |
There was a problem hiding this comment.
This will need to use getPublicOrigin instead of url.origin, meaning mcpUnauthorizedResponse needs to be passed the config.
There was a problem hiding this comment.
@ascorbic Updated this to route the anonymous MCP discovery 401 through getPublicOrigin(...), so the WWW-Authenticate resource metadata URL now respects siteUrl / reverse-proxy deployments instead of using the raw request origin.
I also added a regression covering the configured public-origin case on the anonymous MCP POST path.
Verified with:
pnpm --filter emdash test tests/unit/auth/mcp-discovery-post.test.tspnpm test tests/unit/auth/*.test.tspnpm typecheckpnpm --silent lint:json | jq .diagnostics | length
0d0526d to
5f83a8d
Compare
|
@ascorbic rebased this branch onto current
The PR was already ready for review, so it remains ready. |
There was a problem hiding this comment.
Pull request overview
This PR adjusts the core Astro auth middleware so unauthenticated MCP POST /_emdash/api/mcp requests can reach the MCP auth path and return the OAuth discovery-style 401 (with WWW-Authenticate) instead of being blocked earlier by CSRF enforcement.
Changes:
- Adds an MCP-specific early
401 + WWW-Authenticateresponse path for non-token requests. - Refactors CSRF rejection response and unsafe-method detection into small helpers.
- Adds unit tests covering unauthenticated MCP
POSTdiscovery responses and invalid bearer token handling, plus a CSRF non-regression test for non-MCP API routes.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/core/src/astro/middleware/auth.ts | Adds MCP-specific unauthorized response path before CSRF enforcement; refactors CSRF response building. |
| packages/core/tests/unit/auth/mcp-discovery-post.test.ts | Adds regression tests for MCP discovery via POST and invalid bearer token behavior. |
| .changeset/fresh-mice-battle.md | Publishes a patch changeset describing the MCP discovery fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // MCP discovery/tooling is bearer-only. Session/external auth should never | ||
| // be consulted for this endpoint, and unauthenticated requests must return | ||
| // the OAuth discovery-style 401 response. | ||
| const method = context.request.method.toUpperCase(); | ||
| const isMcpEndpoint = url.pathname === MCP_ENDPOINT_PATH; | ||
| if (isMcpEndpoint && !isTokenAuth) { | ||
| return mcpUnauthorizedResponse(url, context.locals.emdash?.config); | ||
| } |
There was a problem hiding this comment.
The new MCP short-circuit runs before the CSRF check, so any non-token request to /_emdash/api/mcp (including browser requests carrying session cookies but missing X-EmDash-Request: 1) will now return the OAuth-discovery 401 instead of 403 CSRF_REJECTED. This doesn’t match the PR description’s goal of preserving CSRF rejection for session/cookie-backed MCP POSTs. If that behavior is still desired, gate this early-return to only apply to truly anonymous discovery (e.g., no Cookie/session cookie present), and add a regression test covering the cookie-present + missing-CSRF-header case.
| const MCP_ENDPOINT_PATH = "/_emdash/api/mcp"; | ||
|
|
There was a problem hiding this comment.
MCP_ENDPOINT_PATH is introduced, but the file still contains hard-coded "/_emdash/api/mcp" comparisons elsewhere (e.g., the invalid-token WWW-Authenticate branch and the scope rule). To avoid drift, prefer using the constant consistently in this module.
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| vi.clearAllMocks(); | ||
| }); |
There was a problem hiding this comment.
vi.clearAllMocks() is called in both beforeEach and afterEach. One of these is redundant and removing the duplicate will reduce noise in the test setup while keeping the same isolation behavior.
What does this PR do?
Fixes OAuth discovery for unauthenticated MCP
POSTrequests on/_emdash/api/mcp.Some MCP clients, including Codex, probe the MCP endpoint with
POSTwhen starting auth discovery. Before this change, unauthenticatedPOST /_emdash/api/mcprequests withoutX-EmDash-Request: 1were rejected by the generic CSRF middleware with403 CSRF_REJECTEDbefore the MCP auth path could return401 Unauthorizedwith theWWW-Authenticateheader.That prevented OAuth-capable MCP clients from starting discovery even though EmDash already exposed valid OAuth metadata endpoints.
This change:
POSTrequests with no authenticated session to reach the existing401 + WWW-AuthenticateresponsePOSTrequests withoutX-EmDash-Request: 1POST, invalid bearer token handling, and the non-regression CSRF casesType of change
Checklist
pnpm typecheckpassespnpm --silent lint:json | jq '.diagnostics | length'returns 0pnpm testpasses (or targeted tests for my change)pnpm formathas been runAI-generated code disclosure
Screenshots / test output
Ran:
pnpm buildpnpm typecheckpnpm typecheck:demospnpm --filter emdash exec vitest run tests/unit/auth/mcp-discovery-post.test.ts tests/unit/auth/discovery-endpoints.test.ts tests/unit/api/csrf.test.ts tests/unit/mcp/authorization.test.tsNotes:
pnpm --silent lint:json | jq '.diagnostics | length'currently reports 3 existing warnings in unrelated files.