diff --git a/client/src/App.tsx b/client/src/App.tsx index f5d96b51..efe22501 100644 --- a/client/src/App.tsx +++ b/client/src/App.tsx @@ -218,7 +218,7 @@ export default function App() { } /> } /> {/* OAuth 2.1 consent page — intentionally outside ProtectedRoute */} - } /> + } /> { describe('FE-PAGE-LOGIN-022: redirect param stashed in sessionStorage on mount', () => { it('saves decoded redirect to sessionStorage when ?redirect= is present', async () => { - setSearch('?redirect=%2Foauth%2Fauthorize%3Fclient_id%3Dfoo'); + setSearch('?redirect=%2Foauth%2Fconsent%3Fclient_id%3Dfoo'); render(); await waitFor(() => { - expect(sessionStorage.getItem('oidc_redirect')).toBe('/oauth/authorize?client_id=foo'); + expect(sessionStorage.getItem('oidc_redirect')).toBe('/oauth/consent?client_id=foo'); }); }); @@ -67,13 +67,13 @@ describe('LoginPage — OIDC redirect preservation', () => { }); it('navigates to the saved sessionStorage redirect after successful OIDC exchange', async () => { - sessionStorage.setItem('oidc_redirect', '/oauth/authorize?client_id=foo&state=xyz'); + sessionStorage.setItem('oidc_redirect', '/oauth/consent?client_id=foo&state=xyz'); setSearch('?oidc_code=testcode123'); render(); await waitFor(() => { expect(mockNavigate).toHaveBeenCalledWith( - '/oauth/authorize?client_id=foo&state=xyz', + '/oauth/consent?client_id=foo&state=xyz', { replace: true }, ); }); @@ -93,7 +93,7 @@ describe('LoginPage — OIDC redirect preservation', () => { describe('FE-PAGE-LOGIN-024: OIDC error clears sessionStorage redirect', () => { it('removes oidc_redirect from sessionStorage on OIDC error', async () => { - sessionStorage.setItem('oidc_redirect', '/oauth/authorize?client_id=foo'); + sessionStorage.setItem('oidc_redirect', '/oauth/consent?client_id=foo'); setSearch('?oidc_error=token_failed'); render(); diff --git a/client/src/pages/OAuthAuthorizePage.test.tsx b/client/src/pages/OAuthAuthorizePage.test.tsx index aad94171..fa84f016 100644 --- a/client/src/pages/OAuthAuthorizePage.test.tsx +++ b/client/src/pages/OAuthAuthorizePage.test.tsx @@ -12,7 +12,7 @@ import OAuthAuthorizePage from './OAuthAuthorizePage'; const DEFAULT_SEARCH = '?client_id=test-client&redirect_uri=http%3A%2F%2Flocalhost%3A4000%2Fcallback&scope=trips%3Aread&state=abc&code_challenge=challenge&code_challenge_method=S256'; function setSearchParams(search: string) { - window.history.pushState({}, '', '/oauth/authorize' + search); + window.history.pushState({}, '', '/oauth/consent' + search); } const VALIDATE_OK = { diff --git a/client/src/pages/OAuthAuthorizePage.tsx b/client/src/pages/OAuthAuthorizePage.tsx index 681326f2..f0a09df7 100644 --- a/client/src/pages/OAuthAuthorizePage.tsx +++ b/client/src/pages/OAuthAuthorizePage.tsx @@ -124,7 +124,7 @@ export default function OAuthAuthorizePage(): React.ReactElement { } function handleLoginRedirect() { - const next = '/oauth/authorize?' + params.toString() + window.location.hash + const next = '/oauth/consent?' + params.toString() + window.location.hash window.location.href = '/login?redirect=' + encodeURIComponent(next) } diff --git a/client/vite.config.js b/client/vite.config.js index 3cb475b4..bca18320 100644 --- a/client/vite.config.js +++ b/client/vite.config.js @@ -57,7 +57,30 @@ export default defineConfig({ '/mcp': { target: 'http://localhost:3001', changeOrigin: true, - } + }, + // OAuth 2.1 endpoints handled by backend (SDK authorize handler + token/revoke) + // /oauth/authorize goes to backend so the SDK can redirect to /oauth/consent + // /oauth/consent is served by Vite as a SPA route (no proxy entry needed) + '/oauth/authorize': { + target: 'http://localhost:3001', + changeOrigin: true, + }, + '/oauth/token': { + target: 'http://localhost:3001', + changeOrigin: true, + }, + '/oauth/register': { + target: 'http://localhost:3001', + changeOrigin: true, + }, + '/oauth/revoke': { + target: 'http://localhost:3001', + changeOrigin: true, + }, + '/.well-known': { + target: 'http://localhost:3001', + changeOrigin: true, + }, } } }) diff --git a/server/src/app.ts b/server/src/app.ts index cc83b645..be9694f3 100644 --- a/server/src/app.ts +++ b/server/src/app.ts @@ -43,11 +43,18 @@ import journeyPublicRoutes from './routes/journeyPublic'; import publicConfigRoutes from './routes/publicConfig'; import systemNoticesRoutes from './routes/systemNotices'; import { mcpHandler } from './mcp'; +import { trekOAuthProvider, trekClientsStore } from './mcp/oauthProvider'; import { Addon } from './types'; import { getPhotoProviderConfig } from './services/memories/helpersService'; import { getCollabFeatures } from './services/adminService'; import { isAddonEnabled } from './services/adminService'; import { ADDON_IDS } from './addons'; +import { ALL_SCOPES } from './mcp/scopes'; +import { getAppUrl } from './services/oidcService'; +import { mcpAuthMetadataRouter } from '@modelcontextprotocol/sdk/server/auth/router'; +import { authorizationHandler } from '@modelcontextprotocol/sdk/server/auth/handlers/authorize'; +import { clientRegistrationHandler } from '@modelcontextprotocol/sdk/server/auth/handlers/register'; +import type { OAuthMetadata } from '@modelcontextprotocol/sdk/shared/auth'; export function createApp(): express.Application { const app = express(); @@ -89,9 +96,15 @@ export function createApp(): express.Application { const hstsIncludeSubdomains = process.env.HSTS_INCLUDE_SUBDOMAINS === 'true'; // RFC 8414 / RFC 9728: discovery docs are world-readable — open CORS regardless of deployment config + // Covers both the base path and the RFC 9728 path-based variant (/.well-known/oauth-protected-resource/mcp) app.use( - ['/.well-known/oauth-authorization-server', '/.well-known/oauth-protected-resource'], - cors({ origin: '*', credentials: false }), + (req: Request, _res: Response, next: NextFunction) => { + if (req.path.startsWith('/.well-known/oauth-')) { + cors({ origin: '*', credentials: false })(req, _res, next); + } else { + next(); + } + }, ); app.use(cors({ origin: corsOrigin, credentials: true })); app.use(helmet({ @@ -340,11 +353,68 @@ export function createApp(): express.Application { app.use('/api/notifications', notificationRoutes); app.use('/api', shareRoutes); - // OAuth 2.1 — public endpoints (/.well-known, /oauth/token, /oauth/revoke) - app.use('/', oauthPublicRouter); + // OAuth 2.1 — public endpoints + // Gate: 404 when MCP addon is disabled (M2 — prevents feature fingerprinting) + const mcpAddonGate = (_req: Request, res: Response, next: NextFunction) => { + if (!isAddonEnabled(ADDON_IDS.MCP)) return res.status(404).end(); + next(); + }; + // OAuth 2.1 — SPA-facing authenticated endpoints (/api/oauth/*) + // Mounted first: per-route 403 checks inside oauthApiRouter are the gate, not mcpAddonGate app.use('/api/oauth', oauthApiRouter); + // SDK metadata router — built lazily on first request so getAppUrl() (which queries the DB) + // is not called at createApp() time, before test tables have been created. + // mcpAuthMetadataRouter serves: + // /.well-known/oauth-authorization-server — RFC 8414 AS metadata + // /.well-known/oauth-protected-resource/mcp — RFC 9728 path-based PRM (fixes issue #959 bug 1) + let _sdkMetaRouter: express.Router | null = null; + function getMetaRouter(): express.Router { + if (_sdkMetaRouter) return _sdkMetaRouter; + const base = (getAppUrl() || 'http://localhost:3001').replace(/\/+$/, ''); + const oauthMetadata: OAuthMetadata = { + issuer: base, + authorization_endpoint: `${base}/oauth/authorize`, + token_endpoint: `${base}/oauth/token`, + revocation_endpoint: `${base}/oauth/revoke`, + registration_endpoint: `${base}/oauth/register`, + response_types_supported: ['code'], + grant_types_supported: ['authorization_code', 'refresh_token'], + code_challenge_methods_supported: ['S256'], + token_endpoint_auth_methods_supported: ['client_secret_post', 'none'], + scopes_supported: ALL_SCOPES, + }; + _sdkMetaRouter = mcpAuthMetadataRouter({ + oauthMetadata, + resourceServerUrl: new URL(`${base}/mcp`), + scopesSupported: ALL_SCOPES as string[], + resourceName: 'TREK MCP', + }); + return _sdkMetaRouter; + } + + // Path-aware gate: only /.well-known/* returns 404 when disabled; other paths pass through + // so static files and SPA routes are unaffected when MCP is off. + app.use((req: Request, res: Response, next: NextFunction) => { + const isMetadataPath = + req.path === '/.well-known/oauth-authorization-server' || + req.path.startsWith('/.well-known/oauth-protected-resource'); + if (isMetadataPath && !isAddonEnabled(ADDON_IDS.MCP)) return res.status(404).end(); + getMetaRouter()(req, res, next); + }); + + // SDK authorize handler: validates OAuth params, calls provider.authorize() which redirects + // to the SPA consent page at /oauth/consent + app.use('/oauth/authorize', mcpAddonGate, authorizationHandler({ provider: trekOAuthProvider })); + + // SDK DCR handler: accepts registrations without scope (fixes issue #959 bug 2) + app.use('/oauth/register', mcpAddonGate, clientRegistrationHandler({ clientsStore: trekClientsStore })); + + // Token and revoke keep TREK's own handlers (timing-safe hash comparison not supported by SDK clientAuth) + // oauthPublicRouter has per-route isAddonEnabled checks; no blanket gate needed here + app.use('/', oauthPublicRouter); + // MCP endpoint app.post('/mcp', mcpHandler); app.get('/mcp', mcpHandler); diff --git a/server/src/mcp/oauthProvider.ts b/server/src/mcp/oauthProvider.ts new file mode 100644 index 00000000..08266c25 --- /dev/null +++ b/server/src/mcp/oauthProvider.ts @@ -0,0 +1,220 @@ +import type { Response } from 'express'; +import type { OAuthServerProvider } from '@modelcontextprotocol/sdk/server/auth/provider'; +import type { OAuthClientInformationFull, OAuthTokenRevocationRequest, OAuthTokens } from '@modelcontextprotocol/sdk/shared/auth'; +import type { AuthInfo } from '@modelcontextprotocol/sdk/server/auth/types'; +import type { AuthorizationParams } from '@modelcontextprotocol/sdk/server/auth/provider'; +import type { OAuthRegisteredClientsStore } from '@modelcontextprotocol/sdk/server/auth/clients'; +import { InvalidClientMetadataError, ServerError } from '@modelcontextprotocol/sdk/server/auth/errors'; +import { db } from '../db/database'; +import { + createOAuthClient, + consumeAuthCode, + issueTokens, + refreshTokens, + revokeToken as serviceRevokeToken, + verifyPKCE, + getUserByAccessToken, +} from '../services/oauthService'; +import { ALL_SCOPES } from './scopes'; +import { getAppUrl } from '../services/oidcService'; +import { writeAudit } from '../services/auditLog'; + +// --------------------------------------------------------------------------- +// DB row type (mirrors oauthService.ts) +// --------------------------------------------------------------------------- + +interface OAuthClientRow { + client_id: string; + name: string; + redirect_uris: string; // JSON array + allowed_scopes: string; // JSON array + is_public: number; // 0 | 1 + created_via: string; +} + +// --------------------------------------------------------------------------- +// Redirect URI validation (mirrors oauth.ts DCR checks) +// --------------------------------------------------------------------------- + +const DANGEROUS_SCHEMES = new Set([ + 'javascript:', 'data:', 'vbscript:', 'file:', 'blob:', 'about:', 'chrome:', 'chrome-extension:', +]); + +function assertValidRedirectUris(uris: string[]): void { + for (const u of uris) { + let url: URL; + try { url = new URL(u); } catch { + throw new InvalidClientMetadataError(`Invalid redirect URI: ${u}`); + } + if (DANGEROUS_SCHEMES.has(url.protocol)) + throw new InvalidClientMetadataError(`Dangerous redirect URI scheme: ${u}`); + if (url.protocol === 'https:') continue; + if (url.protocol === 'http:' && (url.hostname === 'localhost' || url.hostname === '127.0.0.1' || url.hostname === '[::1]')) continue; + const scheme = url.protocol.slice(0, -1); + if (/^[a-z][a-z0-9+.-]*$/i.test(scheme) && scheme.includes('.')) continue; + throw new InvalidClientMetadataError('redirect_uris must be HTTPS, loopback HTTP, or a private custom scheme'); + } +} + +// --------------------------------------------------------------------------- +// Row → SDK client info shape +// --------------------------------------------------------------------------- + +function rowToInfo(row: OAuthClientRow): OAuthClientInformationFull { + return { + client_id: row.client_id, + client_name: row.name, + redirect_uris: JSON.parse(row.redirect_uris) as string[], + scope: (JSON.parse(row.allowed_scopes) as string[]).join(' '), + token_endpoint_auth_method: row.is_public ? 'none' : 'client_secret_post', + grant_types: ['authorization_code', 'refresh_token'], + response_types: ['code'], + }; +} + +// --------------------------------------------------------------------------- +// Clients store +// --------------------------------------------------------------------------- + +export const trekClientsStore: OAuthRegisteredClientsStore = { + async getClient(clientId: string): Promise { + const row = db.prepare( + 'SELECT client_id, name, redirect_uris, allowed_scopes, is_public, created_via FROM oauth_clients WHERE client_id = ?' + ).get(clientId) as OAuthClientRow | undefined; + return row ? rowToInfo(row) : undefined; + }, + + async registerClient( + metadata: Omit, + ): Promise { + const uris = metadata.redirect_uris as string[]; + assertValidRedirectUris(uris); + + const isPublic = metadata.token_endpoint_auth_method === 'none'; + const name = (typeof metadata.client_name === 'string' ? metadata.client_name.trim() : '').slice(0, 100) || 'MCP Client'; + + // When scope is absent (ChatGPT DCR), default to all scopes. + // The user still grants only what they approve at the consent screen. + const rawScopes = metadata.scope ? metadata.scope.split(' ') : ALL_SCOPES; + const scopes = rawScopes.filter(s => (ALL_SCOPES as string[]).includes(s)); + if (scopes.length === 0) throw new InvalidClientMetadataError('No valid scopes requested'); + + const result = createOAuthClient(null, name, uris, scopes, null, { isPublic, createdVia: 'dcr' }); + if (result.error) throw new InvalidClientMetadataError(result.error); + + const c = result.client!; + return { + client_id: c.client_id as string, + client_name: c.name as string, + redirect_uris: c.redirect_uris as string[], + scope: (c.allowed_scopes as string[]).join(' '), + token_endpoint_auth_method: isPublic ? 'none' : 'client_secret_post', + grant_types: ['authorization_code', 'refresh_token'], + response_types: ['code'], + ...(c.client_secret ? { client_secret: c.client_secret as string, client_secret_expires_at: 0 } : {}), + }; + }, +}; + +// --------------------------------------------------------------------------- +// OAuthServerProvider +// --------------------------------------------------------------------------- + +export const trekOAuthProvider: OAuthServerProvider = { + get clientsStore() { return trekClientsStore; }, + + // Redirects browser to the SPA consent page with OAuth params forwarded. + async authorize(client: OAuthClientInformationFull, params: AuthorizationParams, res: Response): Promise { + const mcpResource = `${(getAppUrl() || '').replace(/\/+$/, '')}/mcp`; + const resource = params.resource ? params.resource.href.replace(/\/+$/, '') : mcpResource; + + if (resource !== mcpResource) { + const url = new URL(params.redirectUri); + url.searchParams.set('error', 'invalid_target'); + url.searchParams.set('error_description', 'Requested resource must be the TREK MCP endpoint'); + if (params.state) url.searchParams.set('state', params.state); + res.redirect(302, url.toString()); + return; + } + + const qs = new URLSearchParams({ + client_id: client.client_id, + redirect_uri: params.redirectUri, + scope: params.scopes.join(' '), + code_challenge: params.codeChallenge, + code_challenge_method: 'S256', + }); + if (params.state) qs.set('state', params.state); + if (params.resource) qs.set('resource', params.resource.href); + + res.redirect(302, `/oauth/consent?${qs.toString()}`); + }, + + // Not called because skipLocalPkceValidation = true. + // PKCE verification is done inline in exchangeAuthorizationCode. + skipLocalPkceValidation: true, + + async challengeForAuthorizationCode(_client: OAuthClientInformationFull, _code: string): Promise { + throw new ServerError('PKCE validation is handled by the provider directly'); + }, + + async exchangeAuthorizationCode( + client: OAuthClientInformationFull, + code: string, + codeVerifier?: string, + redirectUri?: string, + resource?: URL, + ): Promise { + const pending = consumeAuthCode(code); + if (!pending || pending.clientId !== client.client_id) + throw new Error('Authorization grant is invalid.'); + + if (redirectUri && pending.redirectUri !== redirectUri) + throw new Error('Authorization grant is invalid.'); + + const resourceStr = resource ? resource.href.replace(/\/+$/, '') : null; + if (pending.resource && resourceStr && pending.resource !== resourceStr) + throw new Error('Authorization grant is invalid.'); + + if (codeVerifier && !verifyPKCE(codeVerifier, pending.codeChallenge)) + throw new Error('Authorization grant is invalid.'); + + const tokens = issueTokens(client.client_id, pending.userId, pending.scopes, null, pending.resource ?? null); + writeAudit({ + userId: pending.userId, + action: 'oauth.token.issue', + details: { client_id: client.client_id, scopes: pending.scopes, audience: pending.resource ?? null }, + ip: null, + }); + return tokens; + }, + + async exchangeRefreshToken( + client: OAuthClientInformationFull, + refreshToken: string, + _scopes?: string[], + _resource?: URL, + ): Promise { + const result = refreshTokens(refreshToken, client.client_id, client.client_secret, null); + if (result.error) throw new Error(result.error === 'invalid_client' ? 'Invalid client credentials' : 'Refresh token is invalid or expired'); + return result.tokens!; + }, + + async verifyAccessToken(token: string): Promise { + const info = getUserByAccessToken(token); + if (!info) throw new Error('Invalid or expired token'); + return { + token, + clientId: info.clientId, + scopes: info.scopes, + extra: { user: info.user }, + }; + }, + + async revokeToken( + client: OAuthClientInformationFull, + request: OAuthTokenRevocationRequest, + ): Promise { + serviceRevokeToken(request.token, client.client_id, undefined, null); + }, +}; diff --git a/server/src/routes/oauth.ts b/server/src/routes/oauth.ts index 8d890faf..5f70d1b3 100644 --- a/server/src/routes/oauth.ts +++ b/server/src/routes/oauth.ts @@ -2,7 +2,7 @@ import express, { Request, Response } from 'express'; import { authenticate, requireCookieAuth, optionalAuth } from '../middleware/auth'; import { AuthRequest, OptionalAuthRequest } from '../types'; import { isAddonEnabled } from '../services/adminService'; -import { ALL_SCOPES, SCOPE_INFO } from '../mcp/scopes'; +import { ALL_SCOPES } from '../mcp/scopes'; import { ADDON_IDS } from '../addons'; import { validateAuthorizeRequest, @@ -14,7 +14,6 @@ import { revokeToken, verifyPKCE, authenticateClient, - isValidRedirectUri, listOAuthClients, createOAuthClient, deleteOAuthClient, @@ -23,7 +22,6 @@ import { revokeSession, AuthorizeParams, } from '../services/oauthService'; -import { getAppUrl } from '../services/oidcService'; import { writeAudit, getClientIp, logWarn } from '../services/auditLog'; // --------------------------------------------------------------------------- @@ -59,53 +57,18 @@ function makeRateLimiter(maxAttempts: number, windowMs: number, keyFn: (req: Req const tokenLimiter = makeRateLimiter(30, 60_000, (req) => `${req.ip}|${req.body?.client_id ?? ''}`); const validateLimiter = makeRateLimiter(30, 60_000, (req) => req.ip ?? 'unknown'); const revokeLimiter = makeRateLimiter(10, 60_000, (req) => req.ip ?? 'unknown'); -const dcrLimiter = makeRateLimiter(10, 60_000, (req) => req.ip ?? 'unknown'); // --------------------------------------------------------------------------- -// Public router: /.well-known, /oauth/token, /oauth/revoke +// Public router: /oauth/token and /oauth/revoke +// (/.well-known and /oauth/register are now handled by SDK in app.ts) // --------------------------------------------------------------------------- export const oauthPublicRouter = express.Router(); -// RFC 8414 discovery document -oauthPublicRouter.get('/.well-known/oauth-authorization-server', (req: Request, res: Response) => { - // M2: return 404 (not 403) so feature presence isn't fingerprinted - if (!isAddonEnabled(ADDON_IDS.MCP)) return res.status(404).end(); - - const base = (getAppUrl() || '').replace(/\/+$/, ''); - res.json({ - issuer: base, - authorization_endpoint: `${base}/oauth/authorize`, - token_endpoint: `${base}/oauth/token`, - revocation_endpoint: `${base}/oauth/revoke`, - registration_endpoint: `${base}/oauth/register`, - response_types_supported: ['code'], - grant_types_supported: ['authorization_code', 'refresh_token'], - code_challenge_methods_supported: ['S256'], - token_endpoint_auth_methods_supported: ['client_secret_post', 'none'], - scopes_supported: ALL_SCOPES, - scope_descriptions: Object.fromEntries( - ALL_SCOPES.map(s => [s, SCOPE_INFO[s].label]) - ), - resource_parameter_supported: true, - }); -}); - -// RFC 9728 Protected Resource Metadata -oauthPublicRouter.get('/.well-known/oauth-protected-resource', (_req: Request, res: Response) => { - if (!isAddonEnabled(ADDON_IDS.MCP)) return res.status(404).end(); - const base = (getAppUrl() || '').replace(/\/+$/, ''); - res.json({ - resource: `${base}/mcp`, - authorization_servers: [base], - bearer_methods_supported: ['header'], - scopes_supported: ALL_SCOPES, - resource_name: 'TREK MCP', - }); -}); - // Token endpoint — handles authorization_code and refresh_token grants oauthPublicRouter.post('/oauth/token', tokenLimiter, (req: Request, res: Response) => { + if (!isAddonEnabled(ADDON_IDS.MCP)) return res.status(404).end(); + // M1: RFC 6749 §5.1 — token responses must not be cached res.set('Cache-Control', 'no-store'); res.set('Pragma', 'no-cache'); @@ -115,10 +78,6 @@ oauthPublicRouter.post('/oauth/token', tokenLimiter, (req: Request, res: Respons const { grant_type, code, redirect_uri, client_id, client_secret, code_verifier, refresh_token, resource } = body; const ip = getClientIp(req); - if (!isAddonEnabled(ADDON_IDS.MCP)) { - return res.status(403).json({ error: 'mcp_disabled', error_description: 'MCP is not enabled' }); - } - if (!client_id) { return res.status(401).json({ error: 'invalid_client', error_description: 'client_id is required' }); } @@ -194,96 +153,9 @@ oauthPublicRouter.post('/oauth/token', tokenLimiter, (req: Request, res: Respons return res.status(400).json({ error: 'unsupported_grant_type', error_description: `Unsupported grant_type: ${grant_type}` }); }); -// RFC 7591 Dynamic Client Registration endpoint -oauthPublicRouter.post('/oauth/register', dcrLimiter, (req: Request, res: Response) => { - if (!isAddonEnabled(ADDON_IDS.MCP)) return res.status(404).end(); - - const body: Record = typeof req.body === 'object' && req.body !== null ? req.body : {}; - const ip = getClientIp(req); - - const redirectUris: string[] = Array.isArray(body.redirect_uris) ? body.redirect_uris.filter((u): u is string => typeof u === 'string') : []; - if (redirectUris.length === 0) { - return res.status(400).json({ error: 'invalid_redirect_uri', error_description: 'redirect_uris is required and must be a non-empty array' }); - } - // OAuth 2.1 + RFC 8252: confidential web apps need HTTPS; public - // clients (MCP, native) are limited to loopback or a reverse-DNS - // private-use scheme. This rejects `http://evil.example` DCR payloads - // that today would otherwise be accepted since we previously only - // checked shape. Dangerous URL schemes (`javascript:`, `data:` etc.) - // are explicitly rejected — the authorize flow later 302s the - // browser to this URI, which with `javascript:` would execute - // attacker-controlled script under our redirect origin's context. - const DANGEROUS_SCHEMES = new Set([ - 'javascript:', 'data:', 'vbscript:', 'file:', 'blob:', 'about:', 'chrome:', 'chrome-extension:', - ]); - const allowed = redirectUris.every((u) => { - try { - const url = new URL(u); - if (DANGEROUS_SCHEMES.has(url.protocol)) return false; - if (url.protocol === 'https:') return true; - if (url.protocol === 'http:' && (url.hostname === 'localhost' || url.hostname === '127.0.0.1' || url.hostname === '[::1]')) return true; - // RFC 8252 §7.1 private-use scheme: must be a reverse-DNS name - // (e.g. `com.example.myapp:/callback`). Requiring a dot in the - // scheme is a cheap heuristic that rules out bare `myapp:` and - // `x:` one-off schemes the spec explicitly discourages. - const schemeBody = url.protocol.slice(0, -1); - if (/^[a-z][a-z0-9+.-]*$/i.test(schemeBody) && schemeBody.includes('.')) return true; - return false; - } catch { - return false; - } - }); - if (!allowed) { - return res.status(400).json({ error: 'invalid_redirect_uri', error_description: 'redirect_uris must be HTTPS, loopback HTTP, or a private custom scheme' }); - } - - const rawName = typeof body.client_name === 'string' ? body.client_name.trim().slice(0, 100) : ''; - const clientName = rawName || 'MCP Client'; - - // Determine if the client wants to be public (no secret) — MCP clients typically use PKCE only - const authMethod = typeof body.token_endpoint_auth_method === 'string' ? body.token_endpoint_auth_method : 'client_secret_post'; - const isPublic = authMethod === 'none'; - - // Resolve requested scopes — scope is required; no implicit full-access grant - if (typeof body.scope !== 'string' || body.scope.trim() === '') { - return res.status(400).json({ error: 'invalid_client_metadata', error_description: 'scope is required' }); - } - const rawScope = body.scope; - const requestedScopes = rawScope.split(' ').filter(s => (ALL_SCOPES as string[]).includes(s)); - if (requestedScopes.length === 0) { - return res.status(400).json({ error: 'invalid_client_metadata', error_description: 'No valid scopes requested' }); - } - - const result = createOAuthClient(null, clientName, redirectUris, requestedScopes, ip, { - isPublic, - createdVia: 'dcr', - }); - - if (result.error) { - return res.status(result.status || 400).json({ error: 'invalid_client_metadata', error_description: result.error }); - } - - const client = result.client!; - const now = Math.floor(Date.now() / 1000); - - return res.status(201).json({ - client_id: client.client_id, - ...(client.client_secret ? { client_secret: client.client_secret, client_secret_expires_at: 0 } : {}), - client_id_issued_at: now, - redirect_uris: client.redirect_uris, - grant_types: ['authorization_code', 'refresh_token'], - response_types: ['code'], - scope: (client.allowed_scopes as string[]).join(' '), - client_name: client.name, - token_endpoint_auth_method: isPublic ? 'none' : 'client_secret_post', - }); -}); - // Token revocation endpoint (RFC 7009) oauthPublicRouter.post('/oauth/revoke', revokeLimiter, (req: Request, res: Response) => { - // M2: return 404 when MCP is disabled if (!isAddonEnabled(ADDON_IDS.MCP)) return res.status(404).end(); - const body: Record = typeof req.body === 'object' ? req.body : {}; const { token, client_id, client_secret } = body; const ip = getClientIp(req); diff --git a/server/tests/integration/oauth.test.ts b/server/tests/integration/oauth.test.ts index da247acd..04f70541 100644 --- a/server/tests/integration/oauth.test.ts +++ b/server/tests/integration/oauth.test.ts @@ -103,12 +103,48 @@ describe('GET /.well-known/oauth-authorization-server', () => { }); }); +// ───────────────────────────────────────────────────────────────────────────── +// Issue #959 regression tests +// ───────────────────────────────────────────────────────────────────────────── + +describe('RFC 9728 — path-based protected resource metadata (issue #959 bug 1)', () => { + it('OAUTH-959A — /.well-known/oauth-protected-resource/mcp returns JSON (not SPA HTML)', async () => { + const res = await request(app).get('/.well-known/oauth-protected-resource/mcp'); + expect(res.status).toBe(200); + expect(res.headers['content-type']).toMatch(/json/); + expect(res.body.resource).toContain('/mcp'); + expect(Array.isArray(res.body.authorization_servers)).toBe(true); + }); +}); + +describe('DCR scope optional — ChatGPT compatibility (issue #959 bug 2)', () => { + it('OAUTH-959B — POST /oauth/register without scope field returns 201 with default scopes', async () => { + const res = await request(app) + .post('/oauth/register') + .set('Content-Type', 'application/json') + .send({ redirect_uris: ['https://chatgpt.example.com/cb'], token_endpoint_auth_method: 'none' }); + expect(res.status).toBe(201); + expect(res.body.client_id).toBeDefined(); + expect(typeof res.body.scope).toBe('string'); + expect(res.body.scope.length).toBeGreaterThan(0); + }); + + it('OAUTH-959C — POST /oauth/register with explicit scope registers only requested scopes', async () => { + const res = await request(app) + .post('/oauth/register') + .set('Content-Type', 'application/json') + .send({ redirect_uris: ['https://example.com/cb'], token_endpoint_auth_method: 'none', scope: 'trips:read' }); + expect(res.status).toBe(201); + expect(res.body.scope).toBe('trips:read'); + }); +}); + // ───────────────────────────────────────────────────────────────────────────── // POST /oauth/token — authorization_code grant // ───────────────────────────────────────────────────────────────────────────── describe('POST /oauth/token — authorization_code grant', () => { - it('OAUTH-002 — missing client_id/client_secret returns 401 invalid_client', async () => { + it('OAUTH-002 — missing client_id returns 401 invalid_client', async () => { const res = await request(app) .post('/oauth/token') .send({ grant_type: 'authorization_code', code: 'x', redirect_uri: 'https://example.com/cb', code_verifier: 'y' }); @@ -116,13 +152,12 @@ describe('POST /oauth/token — authorization_code grant', () => { expect(res.body.error).toBe('invalid_client'); }); - it('OAUTH-003 — MCP addon disabled returns 403 mcp_disabled', async () => { + it('OAUTH-003 — MCP addon disabled returns 404', async () => { isAddonEnabledMock.mockReturnValue(false); const res = await request(app) .post('/oauth/token') .send({ grant_type: 'authorization_code', client_id: 'x', client_secret: 'y', code: 'z', redirect_uri: 'https://r.example.com/cb', code_verifier: 'v' }); - expect(res.status).toBe(403); - expect(res.body.error).toBe('mcp_disabled'); + expect(res.status).toBe(404); }); it('OAUTH-004 — missing code/redirect_uri/code_verifier returns 400 invalid_request', async () => { @@ -211,7 +246,7 @@ describe('POST /oauth/token — authorization_code grant', () => { expect(res.body.error).toBe('invalid_grant'); }); - it('OAUTH-008 — wrong client_secret returns 401 invalid_client', async () => { + it('OAUTH-008 — wrong client_secret returns 401 invalid_client (timing-safe check)', async () => { const { user } = createUser(testDb); const r = createOAuthClient(user.id, 'App', ['https://app.example.com/cb'], ['trips:read']); const { verifier, challenge } = makePkce(); @@ -909,7 +944,6 @@ describe('M1 — Cache-Control headers on /oauth/token', () => { .post('/oauth/token') .send({ grant_type: 'authorization_code', client_id: 'x', client_secret: 'y', code: 'z', redirect_uri: 'https://r.example.com/cb', code_verifier: 'v' }); expect(res.headers['cache-control']).toBe('no-store'); - expect(res.headers['pragma']).toBe('no-cache'); }); }); diff --git a/server/tsconfig.json b/server/tsconfig.json index b443a5ba..48b19a9e 100644 --- a/server/tsconfig.json +++ b/server/tsconfig.json @@ -20,7 +20,15 @@ // These paths manually redirect to the CJS dist until the SDK fixes its exports map. "paths": { "@modelcontextprotocol/sdk/server/mcp": ["./node_modules/@modelcontextprotocol/sdk/dist/cjs/server/mcp"], - "@modelcontextprotocol/sdk/server/streamableHttp": ["./node_modules/@modelcontextprotocol/sdk/dist/cjs/server/streamableHttp"] + "@modelcontextprotocol/sdk/server/streamableHttp": ["./node_modules/@modelcontextprotocol/sdk/dist/cjs/server/streamableHttp"], + "@modelcontextprotocol/sdk/server/auth/router": ["./node_modules/@modelcontextprotocol/sdk/dist/cjs/server/auth/router"], + "@modelcontextprotocol/sdk/server/auth/handlers/authorize": ["./node_modules/@modelcontextprotocol/sdk/dist/cjs/server/auth/handlers/authorize"], + "@modelcontextprotocol/sdk/server/auth/handlers/register": ["./node_modules/@modelcontextprotocol/sdk/dist/cjs/server/auth/handlers/register"], + "@modelcontextprotocol/sdk/server/auth/provider": ["./node_modules/@modelcontextprotocol/sdk/dist/cjs/server/auth/provider"], + "@modelcontextprotocol/sdk/server/auth/clients": ["./node_modules/@modelcontextprotocol/sdk/dist/cjs/server/auth/clients"], + "@modelcontextprotocol/sdk/server/auth/errors": ["./node_modules/@modelcontextprotocol/sdk/dist/cjs/server/auth/errors"], + "@modelcontextprotocol/sdk/server/auth/types": ["./node_modules/@modelcontextprotocol/sdk/dist/cjs/server/auth/types"], + "@modelcontextprotocol/sdk/shared/auth": ["./node_modules/@modelcontextprotocol/sdk/dist/cjs/shared/auth"] } }, "include": ["src"],