-
Notifications
You must be signed in to change notification settings - Fork 773
fix(core): return OAuth discovery 401 for unauthenticated MCP POSTs #371
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
5948993
e3eb9ca
116e6a9
7f57fc7
4fa8c41
0ed9652
5f83a8d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "emdash": patch | ||
| --- | ||
|
|
||
| Fix MCP OAuth discovery for unauthenticated POST requests. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,6 +51,38 @@ declare global { | |
|
|
||
| // Role level constants (matching @emdash-cms/auth) | ||
| const ROLE_ADMIN = 50; | ||
| const MCP_ENDPOINT_PATH = "/_emdash/api/mcp"; | ||
|
|
||
| function isUnsafeMethod(method: string): boolean { | ||
| return method !== "GET" && method !== "HEAD" && method !== "OPTIONS"; | ||
| } | ||
|
|
||
| function csrfRejectedResponse(): Response { | ||
| return new Response( | ||
| JSON.stringify({ error: { code: "CSRF_REJECTED", message: "Missing required header" } }), | ||
| { | ||
| status: 403, | ||
| headers: { "Content-Type": "application/json", ...MW_CACHE_HEADERS }, | ||
| }, | ||
| ); | ||
| } | ||
|
|
||
| function mcpUnauthorizedResponse( | ||
| url: URL, | ||
| config?: Parameters<typeof getPublicOrigin>[1], | ||
| ): Response { | ||
| const origin = getPublicOrigin(url, config); | ||
| return Response.json( | ||
| { error: { code: "NOT_AUTHENTICATED", message: "Not authenticated" } }, | ||
| { | ||
| status: 401, | ||
| headers: { | ||
| "WWW-Authenticate": `Bearer resource_metadata="${origin}/.well-known/oauth-protected-resource"`, | ||
| ...MW_CACHE_HEADERS, | ||
| }, | ||
| }, | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * API routes that skip auth — each handles its own access control. | ||
|
|
@@ -183,31 +215,31 @@ export const onRequest = defineMiddleware(async (context, next) => { | |
|
|
||
| const isTokenAuth = bearerResult === "authenticated"; | ||
|
|
||
| // 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); | ||
| } | ||
|
Comment on lines
+218
to
+225
|
||
|
|
||
| // CSRF protection: require X-EmDash-Request header on state-changing requests. | ||
| // Skip for token-authenticated requests (tokens aren't ambient credentials). | ||
| // Browsers block cross-origin custom headers, so this prevents CSRF without tokens. | ||
| // OAuth authorize consent is exempt: it's a standard HTML form POST that can't | ||
| // include custom headers. The consent flow is protected by session + single-use codes. | ||
| const method = context.request.method.toUpperCase(); | ||
| const isOAuthConsent = url.pathname.startsWith("/_emdash/oauth/authorize"); | ||
| if ( | ||
| isApiRoute && | ||
| !isTokenAuth && | ||
| !isOAuthConsent && | ||
| method !== "GET" && | ||
| method !== "HEAD" && | ||
| method !== "OPTIONS" && | ||
| isUnsafeMethod(method) && | ||
| !isPublicApiRoute | ||
| ) { | ||
| const csrfHeader = context.request.headers.get("X-EmDash-Request"); | ||
| if (csrfHeader !== "1") { | ||
| return new Response( | ||
| JSON.stringify({ error: { code: "CSRF_REJECTED", message: "Missing required header" } }), | ||
| { | ||
| status: 403, | ||
| headers: { "Content-Type": "application/json", ...MW_CACHE_HEADERS }, | ||
| }, | ||
| ); | ||
| return csrfRejectedResponse(); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -562,18 +594,10 @@ async function handlePasskeyAuth( | |
| const sessionUser = await session?.get("user"); | ||
|
|
||
| if (!sessionUser?.id) { | ||
| // Not authenticated | ||
| if (isApiRoute) { | ||
| const headers: Record<string, string> = { ...MW_CACHE_HEADERS }; | ||
| // Add WWW-Authenticate on MCP endpoint 401s to trigger OAuth discovery | ||
| if (url.pathname === "/_emdash/api/mcp") { | ||
| const origin = getPublicOrigin(url, emdash?.config); | ||
| headers["WWW-Authenticate"] = | ||
| `Bearer resource_metadata="${origin}/.well-known/oauth-protected-resource"`; | ||
| } | ||
| return Response.json( | ||
| { error: { code: "NOT_AUTHENTICATED", message: "Not authenticated" } }, | ||
| { status: 401, headers }, | ||
| { status: 401, headers: MW_CACHE_HEADERS }, | ||
| ); | ||
| } | ||
| const loginUrl = new URL("/_emdash/admin/login", getPublicOrigin(url, emdash?.config)); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,187 @@ | ||
| import { afterEach, beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; | ||
|
|
||
| vi.mock("virtual:emdash/auth", () => ({ authenticate: vi.fn() })); | ||
| vi.mock("astro:middleware", () => ({ | ||
| defineMiddleware: (handler: unknown) => handler, | ||
| })); | ||
| vi.mock("@emdash-cms/auth", () => ({ | ||
| TOKEN_PREFIXES: {}, | ||
| generatePrefixedToken: vi.fn(), | ||
| hashPrefixedToken: vi.fn(), | ||
| VALID_SCOPES: [], | ||
| validateScopes: vi.fn(), | ||
| hasScope: vi.fn(() => false), | ||
| computeS256Challenge: vi.fn(), | ||
| Role: { ADMIN: 50 }, | ||
| })); | ||
| vi.mock("@emdash-cms/auth/adapters/kysely", () => ({ | ||
| createKyselyAdapter: vi.fn(() => ({ | ||
| getUserById: vi.fn(async (id: string) => ({ | ||
| id, | ||
| email: "admin@test.com", | ||
| name: "Admin", | ||
| role: 50, | ||
| disabled: 0, | ||
| })), | ||
| getUserByEmail: vi.fn(), | ||
| })), | ||
| })); | ||
|
|
||
| type AuthMiddlewareModule = typeof import("../../../src/astro/middleware/auth.js"); | ||
|
|
||
| let onRequest: AuthMiddlewareModule["onRequest"]; | ||
|
|
||
| beforeAll(async () => { | ||
| ({ onRequest } = await import("../../../src/astro/middleware/auth.js")); | ||
| }); | ||
|
|
||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| vi.clearAllMocks(); | ||
| }); | ||
|
Comment on lines
+38
to
+44
|
||
|
|
||
| async function runAuthMiddleware(opts: { | ||
| pathname: string; | ||
| method?: string; | ||
| headers?: HeadersInit; | ||
| sessionUserId?: string | null; | ||
| siteUrl?: string; | ||
| }) { | ||
| const url = new URL(opts.pathname, "https://example.com"); | ||
| const session = { | ||
| get: vi.fn().mockResolvedValue(opts.sessionUserId ? { id: opts.sessionUserId } : null), | ||
| set: vi.fn(), | ||
| destroy: vi.fn(), | ||
| }; | ||
| const next = vi.fn(async () => new Response("ok")); | ||
| const response = await onRequest( | ||
| { | ||
| url, | ||
| request: new Request(url, { | ||
| method: opts.method ?? "POST", | ||
| headers: opts.headers, | ||
| body: JSON.stringify({ | ||
| jsonrpc: "2.0", | ||
| id: 1, | ||
| method: "initialize", | ||
| params: { | ||
| protocolVersion: "2025-03-26", | ||
| capabilities: {}, | ||
| clientInfo: { name: "debug", version: "1.0" }, | ||
| }, | ||
| }), | ||
| }), | ||
| locals: { | ||
| emdash: { | ||
| db: {}, | ||
| config: opts.siteUrl ? { siteUrl: opts.siteUrl } : {}, | ||
| }, | ||
| }, | ||
| session, | ||
| redirect: (location: string) => | ||
| new Response(null, { | ||
| status: 302, | ||
| headers: { Location: location }, | ||
| }), | ||
| } as Parameters<AuthMiddlewareModule["onRequest"]>[0], | ||
| next, | ||
| ); | ||
|
|
||
| return { response, next, session }; | ||
| } | ||
|
|
||
| describe("MCP discovery auth middleware", () => { | ||
| it("returns 401 with discovery metadata for unauthenticated MCP POST requests", async () => { | ||
| const { response, next } = await runAuthMiddleware({ | ||
| pathname: "/_emdash/api/mcp", | ||
| headers: { "Content-Type": "application/json" }, | ||
| }); | ||
|
|
||
| expect(next).not.toHaveBeenCalled(); | ||
| expect(response.status).toBe(401); | ||
| expect(response.headers.get("WWW-Authenticate")).toBe( | ||
| 'Bearer resource_metadata="https://example.com/.well-known/oauth-protected-resource"', | ||
| ); | ||
| await expect(response.json()).resolves.toEqual({ | ||
| error: { code: "NOT_AUTHENTICATED", message: "Not authenticated" }, | ||
| }); | ||
| }); | ||
|
|
||
| it("does not read the session for anonymous MCP POST discovery requests", async () => { | ||
| const { response, next, session } = await runAuthMiddleware({ | ||
| pathname: "/_emdash/api/mcp", | ||
| headers: { "Content-Type": "application/json" }, | ||
| }); | ||
|
|
||
| expect(next).not.toHaveBeenCalled(); | ||
| expect(response.status).toBe(401); | ||
| expect(session.get).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("uses the configured public origin for anonymous MCP POST discovery responses", async () => { | ||
| const { response, next } = await runAuthMiddleware({ | ||
| pathname: "/_emdash/api/mcp", | ||
| headers: { "Content-Type": "application/json" }, | ||
| siteUrl: "https://public.example.com", | ||
| }); | ||
|
|
||
| expect(next).not.toHaveBeenCalled(); | ||
| expect(response.status).toBe(401); | ||
| expect(response.headers.get("WWW-Authenticate")).toBe( | ||
| 'Bearer resource_metadata="https://public.example.com/.well-known/oauth-protected-resource"', | ||
| ); | ||
| }); | ||
|
|
||
| it("returns 401 with discovery metadata for invalid bearer tokens on MCP POST", async () => { | ||
| const { response, next } = await runAuthMiddleware({ | ||
| pathname: "/_emdash/api/mcp", | ||
| headers: { | ||
| Authorization: "Bearer invalid", | ||
| "Content-Type": "application/json", | ||
| }, | ||
| }); | ||
|
|
||
| expect(next).not.toHaveBeenCalled(); | ||
| expect(response.status).toBe(401); | ||
| expect(response.headers.get("WWW-Authenticate")).toBe( | ||
| 'Bearer resource_metadata="https://example.com/.well-known/oauth-protected-resource"', | ||
| ); | ||
| await expect(response.json()).resolves.toEqual({ | ||
| error: { code: "INVALID_TOKEN", message: "Invalid or expired token" }, | ||
| }); | ||
| }); | ||
|
|
||
| it("rejects MCP POST requests that only have session auth", async () => { | ||
| const { response, next, session } = await runAuthMiddleware({ | ||
| pathname: "/_emdash/api/mcp", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| "X-EmDash-Request": "1", | ||
| }, | ||
| sessionUserId: "user_1", | ||
| }); | ||
|
|
||
| expect(next).not.toHaveBeenCalled(); | ||
| expect(response.status).toBe(401); | ||
| expect(session.get).not.toHaveBeenCalled(); | ||
| await expect(response.json()).resolves.toEqual({ | ||
| error: { code: "NOT_AUTHENTICATED", message: "Not authenticated" }, | ||
| }); | ||
| }); | ||
|
|
||
| it("still rejects non-MCP API POST requests without the CSRF header", async () => { | ||
| const { response, next } = await runAuthMiddleware({ | ||
| pathname: "/_emdash/api/content/posts", | ||
| headers: { "Content-Type": "application/json" }, | ||
| }); | ||
|
|
||
| expect(next).not.toHaveBeenCalled(); | ||
| expect(response.status).toBe(403); | ||
| await expect(response.json()).resolves.toEqual({ | ||
| error: { code: "CSRF_REJECTED", message: "Missing required header" }, | ||
| }); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MCP_ENDPOINT_PATHis introduced, but the file still contains hard-coded"/_emdash/api/mcp"comparisons elsewhere (e.g., the invalid-tokenWWW-Authenticatebranch and the scope rule). To avoid drift, prefer using the constant consistently in this module.