Skip to content

feat(mcp): add OAuth DCR support - #407

Open
cedricziel wants to merge 1 commit into
orangecoding:masterfrom
cedricziel:feat/mcp-oauth-dcr
Open

feat(mcp): add OAuth DCR support#407
cedricziel wants to merge 1 commit into
orangecoding:masterfrom
cedricziel:feat/mcp-oauth-dcr

Conversation

@cedricziel

Copy link
Copy Markdown

Summary

  • add OAuth authorization-code PKCE flow and dynamic client registration for HTTP MCP clients
  • advertise MCP protected-resource metadata and enforce OAuth token audience, scope, expiry, and session ownership
  • document Claude.ai and ChatGPT setup, including login return handling

Validation

  • eslint .
  • TEST_MODE=offline vitest run test/api/mcpOAuthRoute.test.js

The full offline suite completed 1,035 tests but the runner exited nonzero because this environment exhausted Vitest fork workers.

@orangecoding

Copy link
Copy Markdown
Owner

Hey @cedricziel nice seeing you around ;)

I've reviewed your pr, thanks, this is well built. PKCE S256 mandatory with no plain fallback, exact-match redirect URIs, single-use codes with timingSafeEqual on the verifier, refresh rotation with family revocation, all tokens hashed at rest. And it unlocks the one thing a static mcp_token can't: claude.ai and ChatGPT remote connectors. :) I was planning to build it myself at some point.

However, I found some things to fix first.

Blocking: the resource audience is derived from two different sources.

Authorization stores it from settings.baseUrl (${baseUrl}/api/mcp). Validation recomputes it from request headers in mcpAuthentication.js:

const protocol = forwardedProto || 'http';
return `${protocol}://${req.headers.host}/api/mcp`;

Two problems. Behind a proxy that doesn't set X-Forwarded-Proto, this yields http:// against a stored https:// and every MCP call 401s after a successful OAuth flow. And comparing a stored audience against a client-supplied Host header defeats the point of RFC 8707 binding. Please derive it from settings.baseUrl, same source as oauthUrls(). mcpHttpRoute.js already imports getSettings.

Also needed:

  • Renumber the migration to 35. 32.configured-adapters.js is already on master.
  • Rename the tables to mcp_oauth_*. Single mcp:read scope, hardcoded in validateAccessToken, resource always /api/mcp. Generic oauth_* names imply Fredy is a general authorization server, which it won't become. Cheap to change now while the migration is unapplied.
  • Move addContentTypeParser into an encapsulated plugin. registerMcpOAuthRoutes(fastify) gets the root instance, so the urlencoded parser applies app-wide, contrary to the comment above it. sameSite: 'lax' covers us today, but it removes a CSRF layer from every route.
  • On refresh-token reuse you revoke the family but not the live access tokens. oauth_access_tokens.revoked_at exists and is never set, so a stolen access token stays valid for up to an hour after detection.
  • no way to view or revoke a granted client, no cleanup of expired codes and tokens (session-cleanup-cron is the pattern), and unbounded unauthenticated DCR.

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.

2 participants