From 20e4123e50ee6e0cf9dadda28a86bfbf6c403759 Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Thu, 21 May 2026 13:19:06 -0700 Subject: [PATCH] Add MCP /oauth/mcp/authorize endpoint (OAuth 2.1 + PKCE-S256) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #93. Stage 3 of MCP OAuth support (parent #86). Bridges incoming MCP authorization requests into the existing upstream-IdP flow, mints a single-use authorization code on the upstream callback, and redirects the user-agent back to the MCP client's redirect_uri. - New `mcp_auth_codes` Harper table (`@table(expiration: 300)`) — codes auto-expire after 5 min via native TTL; Stage 4's /token will read-then-delete for one-time use - New MCPAuthCodeStore with the explicit-field-access encode/decode pattern (CLAUDE.md tracked-object spread gotcha) - New handleAuthorize: two-phase validation per OAuth 2.1 §3.1.2.5 — client_id + redirect_uri exact-match before any redirect, everything else returns errors to the verified redirect_uri - RFC 8252 §7.3 loopback port flexibility — native MCP clients (Claude Desktop, mcp-remote) bind a dynamic port at runtime; we accept any port on registered loopback URIs (127.0.0.1 / [::1] / localhost) - PKCE S256 only (OAuth 2.1 §7.5.2 forbids plain); reject `plain` with invalid_request to the client URI - RFC 8707 `resource` parameter required, validated against canonical resource URI from Stage 2 - v1 single-provider constraint: mcp.providers (or the full registry) must resolve to exactly one upstream provider; 0 or >1 returns server_error. Multi-provider chooser is v1.1 - Bridge via CSRF state: the existing CSRFTokenData carries an `mcp` payload that handlers.ts handleCallback detects and detours into handleMCPCallback (callback.ts) — mints auth code, redirects to MCP client. Upstream IdP token never reaches the MCP client (spec MUST NOT, MCP authorization spec 2025-06-18) - handleCallback restructured: verify CSRF state FIRST so MCP-initiated upstream errors route back to the MCP client redirect_uri instead of the Harper app's default path. Legacy human flow behavior preserved for the no-state edge case - onLogin-mapped username (hookData.user) flows through to the issued auth code — Stage 4's JWT inherits the mapped identity, not the raw OAuth username - No Harper session is created on the MCP branch (independent lifecycle per #86 resolved decision) Gemini cross-review caught 4 of the above before push (loopback ports, mapped username, CSRF-before-IdP-error ordering, MCP-aware error redirects); all addressed in this PR before opening. 603 unit tests pass locally (+51 from this PR), including new MCP-branch integration tests in handlers.test.js. Co-Authored-By: Claude Opus 4.7 (1M context) --- schema/oauth.graphql | 17 ++ src/lib/handlers.ts | 129 ++++++---- src/lib/mcp/authCodeStore.ts | 112 +++++++++ src/lib/mcp/authorize.ts | 287 ++++++++++++++++++++++ src/lib/mcp/callback.ts | 94 ++++++++ src/lib/mcp/index.ts | 33 ++- src/lib/resource.ts | 8 +- src/types.ts | 50 ++++ test/lib/handlers.test.js | 159 ++++++++++++ test/lib/mcp/authCodeStore.test.js | 117 +++++++++ test/lib/mcp/authorize.test.js | 373 +++++++++++++++++++++++++++++ test/lib/mcp/callback.test.js | 119 +++++++++ test/lib/mcp/index.test.js | 31 ++- 13 files changed, 1481 insertions(+), 48 deletions(-) create mode 100644 src/lib/mcp/authCodeStore.ts create mode 100644 src/lib/mcp/authorize.ts create mode 100644 src/lib/mcp/callback.ts create mode 100644 test/lib/mcp/authCodeStore.test.js create mode 100644 test/lib/mcp/authorize.test.js create mode 100644 test/lib/mcp/callback.test.js diff --git a/schema/oauth.graphql b/schema/oauth.graphql index a990914..0756fab 100644 --- a/schema/oauth.graphql +++ b/schema/oauth.graphql @@ -34,6 +34,23 @@ type harper_oauth_mcp_clients @table(database: "oauth") { client_secret_expires_at: Float # 0 = never expires } +## MCP Authorization Codes (OAuth 2.1 + RFC 7636 PKCE + RFC 8707 audience) +## Short-lived codes minted by /oauth/mcp/authorize on successful upstream auth. +## Single-use: /oauth/mcp/token (Stage 4) reads-then-deletes. TTL via Harper. +## Auto-expires after 5 minutes — codes are exchanged immediately after the +## upstream redirect, so a longer window only widens the replay attack surface. +type mcp_auth_codes @table(database: "oauth", expiration: 300) { + code: ID @primaryKey + client_id: String + user: String # Harper user identifier + resource: String # RFC 8707 audience the issued token will be bound to + code_challenge: String # RFC 7636 PKCE; verified at /token + code_challenge_method: String # Always "S256" + redirect_uri: String # Bound at issuance; /token must match + scope: String # Space-separated; may be empty + created_at: Float +} + ## OAuth User Session table (optional, for future use) ## Could store OAuth-specific user data # type oauth_sessions @table(database: "oauth") { diff --git a/src/lib/handlers.ts b/src/lib/handlers.ts index f03ac15..35e4f47 100644 --- a/src/lib/handlers.ts +++ b/src/lib/handlers.ts @@ -8,7 +8,8 @@ import { readFile } from 'node:fs/promises'; import { join, dirname } from 'node:path'; import { fileURLToPath } from 'node:url'; import type { RequestTarget } from 'harper'; -import type { Request, Logger, IOAuthProvider, OAuthProviderConfig } from '../types.ts'; +import type { Request, Logger, IOAuthProvider, MCPAuthorizeState, OAuthProviderConfig } from '../types.ts'; +import { handleMCPCallback } from './mcp/index.ts'; import type { HookManager } from './hookManager.ts'; /** @@ -131,61 +132,92 @@ export async function handleCallback( const error = target.get?.('error'); const errorDescription = target.get?.('error_description'); - // Handle OAuth errors from provider - if (error) { - logger?.error?.(`OAuth error: ${error} - ${errorDescription}`); - const errorUrl = buildErrorRedirect(config.postLoginRedirect || '/', { error: 'oauth_failed', reason: error }); - return { - status: 302, - headers: { - Location: errorUrl, - }, - }; - } + // Helper: build an MCP-aware error redirect to the MCP client's redirect_uri. + // Only used in MCP branches — the human path keeps its existing + // buildErrorRedirect shape (error code only, optionally with reason) to + // avoid changing observable behavior on the human OAuth flow. + const mcpErrorRedirect = (mcp: MCPAuthorizeState, errorCode: string, description: string) => { + const url = new URL(mcp.redirectUri); + url.searchParams.set('error', errorCode); + url.searchParams.set('error_description', description); + if (mcp.clientState) url.searchParams.set('state', mcp.clientState); + return { status: 302, headers: { Location: url.toString() } }; + }; - // Validate parameters - if (!code || !state) { - logger?.warn?.('Missing required OAuth callback parameters'); + // Validate state presence — we use it both to route errors (via mcp + // payload, when present) AND to defend against CSRF. + if (!state) { + // Without state, we can't be in an MCP flow (MCP always sets state). + // Preserve the legacy human-OAuth error UX: if the IdP sent an error, + // echo it to postLoginRedirect with the original reason. Otherwise, + // generic invalid_request. + if (error) { + logger?.error?.(`OAuth error (no state): ${error} - ${errorDescription}`); + const errorUrl = buildErrorRedirect(config.postLoginRedirect || '/', { + error: 'oauth_failed', + reason: error, + }); + return { status: 302, headers: { Location: errorUrl } }; + } + logger?.warn?.('Missing state parameter in OAuth callback'); const errorUrl = buildErrorRedirect(config.postLoginRedirect || '/', { error: 'invalid_request' }); - return { - status: 302, - headers: { - Location: errorUrl, - }, - }; + return { status: 302, headers: { Location: errorUrl } }; } - // Verify CSRF token + // Verify CSRF token FIRST — before checking the upstream error param. + // This is what lets MCP-initiated errors route back to the MCP client's + // redirect_uri instead of the Harper app's default path. The token is + // single-use; consuming it on error is fine because OAuth callbacks + // aren't retried with the same state. const tokenData = await provider.verifyCSRFToken(state); if (!tokenData) { logger?.warn?.('Invalid or expired CSRF token'); - // Redirect back to login with error parameter + // We can't know if this was an MCP flow (state didn't decode), so + // fall back to a generic redirect — same behavior as pre-fix. const loginUrl = `/oauth/${providerName}/login?error=session_expired`; - return { - status: 302, - headers: { - Location: loginUrl, - }, - }; + return { status: 302, headers: { Location: loginUrl } }; } - // Verify state token was issued for THIS provider (prevents cross-provider attacks) + const mcpState = tokenData.mcp as MCPAuthorizeState | undefined; + + // Verify state token was issued for THIS provider (prevents cross-provider attacks). if (tokenData.providerName !== providerName) { logger?.warn?.( `State token provider mismatch: token issued for '${tokenData.providerName}', callback for '${providerName}'` ); - // This could be an attack - redirect back to original URL with error - // Do NOT redirect to login endpoint as that would restart OAuth flow + if (mcpState) { + return mcpErrorRedirect(mcpState, 'invalid_request', 'cross-provider state mismatch'); + } const errorUrl = buildErrorRedirect(tokenData.originalUrl || config.postLoginRedirect || '/', { error: 'auth_failed', reason: 'csrf', }); - return { - status: 302, - headers: { - Location: errorUrl, - }, - }; + return { status: 302, headers: { Location: errorUrl } }; + } + + // Now that we know the flow context, handle upstream IdP errors. + if (error) { + logger?.error?.(`OAuth error: ${error} - ${errorDescription}`); + if (mcpState) { + return mcpErrorRedirect(mcpState, error, errorDescription || error); + } + const errorUrl = buildErrorRedirect(tokenData.originalUrl || config.postLoginRedirect || '/', { + error: 'oauth_failed', + reason: error, + }); + return { status: 302, headers: { Location: errorUrl } }; + } + + // Validate code presence — needed regardless of flow. + if (!code) { + logger?.warn?.('Missing authorization code in OAuth callback'); + if (mcpState) { + return mcpErrorRedirect(mcpState, 'invalid_request', 'Missing authorization code'); + } + const errorUrl = buildErrorRedirect(tokenData.originalUrl || config.postLoginRedirect || '/', { + error: 'invalid_request', + }); + return { status: 302, headers: { Location: errorUrl } }; } try { @@ -215,6 +247,17 @@ export async function handleCallback( // Pass providerName (registry key) not config.provider (provider type) for multi-tenant support const hookData = await hookManager.callOnLogin(user, tokenResponse, request.session, request, providerName); + // MCP branch: if the CSRF state was minted by /oauth/mcp/authorize, the + // upstream callback's job is to mint an MCP authorization code, NOT to + // create a Harper session. Independent lifecycle per #86 resolved + // decision. The upstream IdP token never reaches the MCP client. + // Pass the onLogin-mapped username so the issued auth code (and the + // JWT exchanged for it in Stage 4) is bound to the correct identity. + if (mcpState) { + const userIdentifier = hookData?.user ?? user.username; + return handleMCPCallback(request, mcpState, userIdentifier, logger); + } + // Store in session if available if (request.session) { // Calculate token expiration and refresh thresholds @@ -288,16 +331,14 @@ export async function handleCallback( else if (message.includes('claim')) reason = 'user_mapping'; else if (message.includes('user info') || message.includes('userinfo')) reason = 'user_info'; else if (message.includes('hook') || message.includes('onLogin')) reason = 'login_hook'; + if (mcpState) { + return mcpErrorRedirect(mcpState, 'server_error', reason); + } const errorUrl = buildErrorRedirect(tokenData.originalUrl || config.postLoginRedirect || '/', { error: 'auth_failed', reason, }); - return { - status: 302, - headers: { - Location: errorUrl, - }, - }; + return { status: 302, headers: { Location: errorUrl } }; } } diff --git a/src/lib/mcp/authCodeStore.ts b/src/lib/mcp/authCodeStore.ts new file mode 100644 index 0000000..605a5eb --- /dev/null +++ b/src/lib/mcp/authCodeStore.ts @@ -0,0 +1,112 @@ +/** + * MCP Authorization Code Store + * + * Persists short-lived authorization codes in the `mcp_auth_codes` Harper + * table. The table is declared with `@table(expiration: 300)`, so codes + * auto-expire after 5 minutes — Stage 4's /token endpoint must still + * delete on successful exchange (single-use), but the TTL is a safety + * net against codes that are never redeemed. + * + * Explicit field access on encode/decode (no `{ ...raw }`) — Harper + * tracked-object Proxies return empty own-keys, so spread would drop + * scalar fields. See CLAUDE.md "GenericTrackedObject + spread" gotcha. + */ + +import type { Logger, MCPAuthCodeRecord, Table } from '../../types.ts'; + +declare const databases: any; + +let authCodesTable: Table | undefined; + +function getAuthCodesTable(): Table { + if (!authCodesTable) { + if (!databases?.oauth?.mcp_auth_codes) { + throw new Error( + 'OAuth MCP auth codes table (oauth.mcp_auth_codes) not found. ' + + 'Please ensure the OAuth plugin is properly installed with its schema.' + ); + } + authCodesTable = databases.oauth.mcp_auth_codes; + } + return authCodesTable as Table; +} + +/** + * Reset the cached table reference (for testing only) + * @internal + */ +export function resetMCPAuthCodesTableCache(): void { + authCodesTable = undefined; +} + +function encodeRecord(record: MCPAuthCodeRecord): Record { + return { + code: record.code, + client_id: record.client_id, + user: record.user, + resource: record.resource, + code_challenge: record.code_challenge, + code_challenge_method: record.code_challenge_method, + redirect_uri: record.redirect_uri, + scope: record.scope, + created_at: record.created_at, + }; +} + +function decodeRecord(raw: Record): MCPAuthCodeRecord { + return { + code: raw.code, + client_id: raw.client_id, + user: raw.user, + resource: raw.resource, + code_challenge: raw.code_challenge, + code_challenge_method: raw.code_challenge_method, + redirect_uri: raw.redirect_uri, + scope: raw.scope ?? undefined, + created_at: raw.created_at, + }; +} + +export class MCPAuthCodeStore { + private logger?: Logger; + + constructor(logger?: Logger) { + this.logger = logger; + } + + async set(record: MCPAuthCodeRecord): Promise { + const table = getAuthCodesTable(); + try { + await table.put(encodeRecord(record)); + this.logger?.debug?.(`Stored MCP auth code for client ${record.client_id}`); + } catch (error) { + this.logger?.error?.('Failed to store MCP auth code:', error); + throw error; + } + } + + async get(code: string): Promise { + const table = getAuthCodesTable(); + try { + const raw = await table.get(code); + if (!raw || !raw.code) { + return null; + } + return decodeRecord(raw); + } catch (error) { + this.logger?.error?.('Failed to retrieve MCP auth code:', error); + return null; + } + } + + async delete(code: string): Promise { + const table = getAuthCodesTable(); + try { + await table.delete(code); + this.logger?.debug?.('Deleted MCP auth code'); + } catch (error) { + // Non-critical: TTL will eventually evict. + this.logger?.warn?.('Failed to delete MCP auth code:', error); + } + } +} diff --git a/src/lib/mcp/authorize.ts b/src/lib/mcp/authorize.ts new file mode 100644 index 0000000..72db2b2 --- /dev/null +++ b/src/lib/mcp/authorize.ts @@ -0,0 +1,287 @@ +/** + * MCP Authorization Endpoint (OAuth 2.1 + RFC 7636 PKCE + RFC 8707 audience) + * + * Handles GET /oauth/mcp/authorize. The MCP client navigates the user's + * browser here with PKCE + resource params; this handler validates the + * request, picks the configured upstream IdP, and 302-redirects the user + * to the upstream provider's authorize URL. + * + * The PKCE challenge and the MCP client's redirect_uri / state are + * embedded in the CSRF token that the upstream provider echoes back. + * `handleMCPCallback` (callback.ts) reads them out on the way back and + * mints the actual authorization code. + * + * Two-phase validation per OAuth 2.1 §3.1.2.5: + * - Pre-redirect: client_id + redirect_uri must match a registered + * client. Failure → 400 JSON (can't safely redirect to unverified URI). + * - Post-redirect: every other validation error → 302 to client's + * verified redirect_uri with `?error=...&error_description=...`. + */ + +import type { RequestTarget } from 'harper'; +import type { + Logger, + MCPAuthorizeState, + MCPClientRecord, + MCPConfig, + OAuthProviderConfig, + ProviderRegistry, + Request, +} from '../../types.ts'; +import { MCPClientStore } from './clientStore.ts'; +import { resolveResource } from './wellKnown.ts'; + +type ErrorJSON = { + status: 400 | 500; + body: { error: string; error_description?: string }; +}; + +type Redirect = { + status: 302; + headers: { Location: string }; +}; + +/** + * Resolve which upstream provider an MCP authorize request flows through. + * v1 requires the resolved set to be exactly one entry (multi-provider + * chooser UI is v1.1). + */ +export function selectMCPProvider( + mcpConfig: MCPConfig, + providers: ProviderRegistry +): { providerName: string } | { error: string; description: string } { + const allowed = mcpConfig.providers && mcpConfig.providers.length > 0 ? mcpConfig.providers : Object.keys(providers); + const candidates = allowed.filter((name) => name in providers); + + if (candidates.length === 0) { + return { + error: 'server_error', + description: 'No upstream OAuth provider configured for MCP authorization', + }; + } + if (candidates.length > 1) { + return { + error: 'server_error', + description: 'Multiple upstream providers configured; v1 requires `mcp.providers` to resolve to exactly one', + }; + } + return { providerName: candidates[0] }; +} + +/** + * Match a requested redirect_uri against a list of registered URIs. + * + * Exact string match in the general case, but RFC 8252 §7.3 requires + * authorization servers to accept ANY port on loopback redirect URIs + * (127.0.0.1 / [::1]) — native MCP clients like Claude Desktop and + * mcp-remote bind to a dynamic port at runtime and can't pre-register + * it. We treat `localhost` the same way; the RFC notes its use is "not + * recommended" but is widespread in practice, and our DCR validation + * already accepts it as a loopback equivalent (Stage 1). + * + * Two URIs match if they're identical, OR they both parse as URLs whose + * host is in the loopback set AND scheme + pathname + search match. + */ +export function redirectUriMatches(requested: string, registered: string[]): boolean { + if (registered.includes(requested)) return true; + let requestedUrl: URL; + try { + requestedUrl = new URL(requested); + } catch { + return false; + } + const LOOPBACK_HOSTS = new Set(['127.0.0.1', '[::1]', 'localhost']); + if (!LOOPBACK_HOSTS.has(requestedUrl.hostname)) return false; + for (const candidate of registered) { + let candidateUrl: URL; + try { + candidateUrl = new URL(candidate); + } catch { + continue; + } + if ( + LOOPBACK_HOSTS.has(candidateUrl.hostname) && + requestedUrl.hostname === candidateUrl.hostname && + requestedUrl.protocol === candidateUrl.protocol && + requestedUrl.pathname === candidateUrl.pathname && + requestedUrl.search === candidateUrl.search + ) { + return true; + } + } + return false; +} + +function validateCanonicalResource(resource: string, mcpConfig: MCPConfig, request: Request): string | null { + try { + const url = new URL(resource); + if (url.hash) return 'resource must not contain a fragment'; + } catch { + return 'resource is not a valid URI'; + } + // Must match the configured canonical URI exactly. resolveResource derives + // `/mcp` when unset — handlers using request-derived defaults + // inherit the same Host-header caveat documented in Stage 2. + const expected = resolveResource(request, mcpConfig); + if (resource !== expected) { + return `resource does not match this server (expected ${expected})`; + } + return null; +} + +function buildClientErrorRedirect( + redirectUri: string, + error: string, + description: string, + clientState: string | undefined +): Redirect { + const url = new URL(redirectUri); + url.searchParams.set('error', error); + url.searchParams.set('error_description', description); + if (clientState) url.searchParams.set('state', clientState); + return { status: 302, headers: { Location: url.toString() } }; +} + +interface AuthorizeQuery { + client_id?: string; + redirect_uri?: string; + response_type?: string; + code_challenge?: string; + code_challenge_method?: string; + resource?: string; + scope?: string; + state?: string; +} + +function readQuery(target: RequestTarget): AuthorizeQuery { + return { + client_id: target.get?.('client_id') ?? undefined, + redirect_uri: target.get?.('redirect_uri') ?? undefined, + response_type: target.get?.('response_type') ?? undefined, + code_challenge: target.get?.('code_challenge') ?? undefined, + code_challenge_method: target.get?.('code_challenge_method') ?? undefined, + resource: target.get?.('resource') ?? undefined, + scope: target.get?.('scope') ?? undefined, + state: target.get?.('state') ?? undefined, + }; +} + +/** + * Handle GET /oauth/mcp/authorize. + * + * Returns either a 302 redirect (success: to upstream IdP; or post-validation + * error: to client's redirect_uri with error params) or a 400 JSON response + * (pre-validation error: client_id / redirect_uri mismatch). + */ +export async function handleAuthorize( + request: Request, + target: RequestTarget, + mcpConfig: MCPConfig, + providers: ProviderRegistry, + logger?: Logger +): Promise { + const query = readQuery(target); + + // Phase 1: client_id and redirect_uri MUST validate before any redirect. + if (!query.client_id) { + return { + status: 400, + body: { error: 'invalid_request', error_description: 'client_id is required' }, + }; + } + + const clientStore = new MCPClientStore(logger); + let client: MCPClientRecord | null; + try { + client = await clientStore.get(query.client_id); + } catch (error) { + logger?.error?.('MCP authorize: client lookup failed:', (error as Error).message); + return { + status: 500, + body: { error: 'server_error', error_description: 'Client lookup failed' }, + }; + } + + if (!client) { + return { + status: 400, + body: { error: 'invalid_client', error_description: 'Unknown client_id' }, + }; + } + + if (!query.redirect_uri || !redirectUriMatches(query.redirect_uri, client.redirect_uris)) { + return { + status: 400, + body: { + error: 'invalid_request', + error_description: 'redirect_uri does not match a registered URI for this client', + }, + }; + } + + const clientState = query.state; + // Phase 2: everything else redirects to the verified client redirect_uri. + const redirect = (error: string, description: string) => + buildClientErrorRedirect(query.redirect_uri as string, error, description, clientState); + + if (query.response_type !== 'code') { + return redirect('unsupported_response_type', 'response_type must be "code"'); + } + + if (!query.code_challenge) { + return redirect('invalid_request', 'code_challenge is required (PKCE)'); + } + if (query.code_challenge_method !== 'S256') { + return redirect('invalid_request', 'code_challenge_method must be "S256" (OAuth 2.1 forbids the plain method)'); + } + + if (!query.resource) { + return redirect('invalid_target', 'resource is required (RFC 8707)'); + } + const resourceErr = validateCanonicalResource(query.resource, mcpConfig, request); + if (resourceErr) { + return redirect('invalid_target', resourceErr); + } + + // Resolve the upstream provider that will handle the user-facing OAuth. + const selection = selectMCPProvider(mcpConfig, providers); + if ('error' in selection) { + return redirect(selection.error, selection.description); + } + const providerEntry = providers[selection.providerName]; + const providerConfig: OAuthProviderConfig = providerEntry.config; + + // Build the state object the callback handler will pick up to mint the + // MCP auth code (instead of running the human-session flow). + const mcpState: MCPAuthorizeState = { + clientId: query.client_id, + resource: query.resource, + codeChallenge: query.code_challenge, + codeChallengeMethod: 'S256', + redirectUri: query.redirect_uri, + scope: query.scope, + clientState, + }; + + let csrfToken: string; + try { + csrfToken = await providerEntry.provider.generateCSRFToken({ + // providerName binds the state to THIS upstream provider so a callback + // arriving on a different provider's URL is rejected (existing CSRF + // invariant from handleCallback). + providerName: selection.providerName, + mcp: mcpState, + }); + } catch (error) { + logger?.error?.('MCP authorize: failed to generate CSRF token:', (error as Error).message); + return redirect('server_error', 'Failed to initialize upstream OAuth flow'); + } + + const upstreamAuthUrl = providerEntry.provider.getAuthorizationUrl(csrfToken, providerConfig.redirectUri || ''); + + logger?.info?.( + `MCP authorize: client=${query.client_id} -> upstream=${selection.providerName}; bound resource=${query.resource}` + ); + + return { status: 302, headers: { Location: upstreamAuthUrl } }; +} diff --git a/src/lib/mcp/callback.ts b/src/lib/mcp/callback.ts new file mode 100644 index 0000000..63e7ee8 --- /dev/null +++ b/src/lib/mcp/callback.ts @@ -0,0 +1,94 @@ +/** + * MCP Callback Branch + * + * Invoked from the main `handleCallback` (handlers.ts) when the verified + * CSRF state carries an `mcp` payload — i.e., the flow was kicked off by + * /oauth/mcp/authorize, not /oauth//login. We've already + * completed the upstream OAuth dance and run the `onLogin` hook; this + * function mints the MCP authorization code and redirects the user-agent + * back to the MCP client's `redirect_uri`. + * + * Importantly: this branch does NOT create a Harper session. MCP and + * human-session lifecycles are kept independent in v1 (matches the + * "independent lifecycle" resolved decision in #86). Future stages may + * add an opt-in for joint sessions. + * + * Never include the upstream IdP token in the response to the MCP client + * — that's the spec-mandated "no token passthrough" rule. + */ + +import { randomBytes } from 'node:crypto'; +import type { Logger, MCPAuthCodeRecord, MCPAuthorizeState, Request } from '../../types.ts'; +import { MCPAuthCodeStore } from './authCodeStore.ts'; + +type Redirect = { + status: 302; + headers: { Location: string }; +}; + +function buildSuccessRedirect(redirectUri: string, code: string, clientState: string | undefined): Redirect { + const url = new URL(redirectUri); + url.searchParams.set('code', code); + if (clientState) url.searchParams.set('state', clientState); + return { status: 302, headers: { Location: url.toString() } }; +} + +function buildErrorRedirect( + redirectUri: string, + error: string, + description: string, + clientState: string | undefined +): Redirect { + const url = new URL(redirectUri); + url.searchParams.set('error', error); + url.searchParams.set('error_description', description); + if (clientState) url.searchParams.set('state', clientState); + return { status: 302, headers: { Location: url.toString() } }; +} + +/** + * Mint an MCP authorization code and redirect to the MCP client. + * + * `userIdentifier` is the resolved Harper identity — the caller passes + * `hookData?.user ?? oauthUser.username`, so any onLogin mapping that the + * human-session branch would apply is also reflected on the auth code + * (and downstream JWT in Stage 4). + * + * `request` is unused today but accepted for signature parity with the + * human-session branch — Stage 6 audit hooks will read from it. + */ +export async function handleMCPCallback( + _request: Request, + mcpState: MCPAuthorizeState, + userIdentifier: string, + logger?: Logger +): Promise { + const code = randomBytes(32).toString('base64url'); + const record: MCPAuthCodeRecord = { + code, + client_id: mcpState.clientId, + user: userIdentifier, + resource: mcpState.resource, + code_challenge: mcpState.codeChallenge, + code_challenge_method: mcpState.codeChallengeMethod, + redirect_uri: mcpState.redirectUri, + scope: mcpState.scope, + created_at: Date.now(), + }; + + const store = new MCPAuthCodeStore(logger); + try { + await store.set(record); + } catch (error) { + logger?.error?.('MCP callback: failed to persist auth code:', (error as Error).message); + return buildErrorRedirect( + mcpState.redirectUri, + 'server_error', + 'Failed to persist authorization code', + mcpState.clientState + ); + } + + logger?.info?.(`MCP callback: minted auth code for client=${mcpState.clientId} user=${userIdentifier}`); + return buildSuccessRedirect(mcpState.redirectUri, code, mcpState.clientState); +} diff --git a/src/lib/mcp/index.ts b/src/lib/mcp/index.ts index 87df12a..1777629 100644 --- a/src/lib/mcp/index.ts +++ b/src/lib/mcp/index.ts @@ -3,12 +3,17 @@ * * Routes /oauth/mcp/* sub-paths to the appropriate handlers. Kept as a thin * dispatcher so OAuthResource doesn't grow MCP-specific logic; Stage 4 will - * add /token, Stage 3 will add /authorize. + * add /token. */ -import type { Logger, MCPConfig, Request } from '../../types.ts'; +import type { RequestTarget } from 'harper'; +import type { Logger, MCPConfig, ProviderRegistry, Request } from '../../types.ts'; +import { handleAuthorize } from './authorize.ts'; import { handleRegister } from './dcr.ts'; +export { MCPAuthCodeStore, resetMCPAuthCodesTableCache } from './authCodeStore.ts'; +export { handleAuthorize, selectMCPProvider } from './authorize.ts'; +export { handleMCPCallback } from './callback.ts'; export { MCPClientStore, resetMCPClientsTableCache } from './clientStore.ts'; export { handleRegister } from './dcr.ts'; @@ -35,3 +40,27 @@ export async function handleMCPPost( return { status: 404, body: { error: 'Not found' } }; } + +/** + * Dispatch GET /oauth/mcp/. + * + * Returns 404 when MCP is disabled. + */ +export async function handleMCPGet( + action: string, + request: Request, + target: RequestTarget, + mcpConfig: MCPConfig | undefined, + providers: ProviderRegistry, + logger?: Logger +): Promise { + if (!mcpConfig?.enabled) { + return { status: 404, body: { error: 'Not found' } }; + } + + if (action === 'authorize') { + return handleAuthorize(request, target, mcpConfig, providers, logger); + } + + return { status: 404, body: { error: 'Not found' } }; +} diff --git a/src/lib/resource.ts b/src/lib/resource.ts index be3537e..5af0b34 100644 --- a/src/lib/resource.ts +++ b/src/lib/resource.ts @@ -8,7 +8,7 @@ import { Resource } from 'harper'; import type { RequestTarget } from 'harper'; import type { Request, Logger, MCPConfig, ProviderRegistry, OAuthProviderConfig } from '../types.ts'; import { handleLogin, handleCallback, handleLogout, handleUserInfo, handleTestPage } from './handlers.ts'; -import { handleMCPPost } from './mcp/index.ts'; +import { handleMCPGet, handleMCPPost } from './mcp/index.ts'; import type { HookManager } from './hookManager.ts'; import type { DynamicProviderCache } from './dynamicProviderCache.ts'; @@ -309,6 +309,12 @@ export class OAuthResource extends Resource { }; } + // Handle MCP endpoints (/oauth/mcp/) — `mcp` is not a regular + // provider so it bypasses the provider lookup below. + if (providerName === 'mcp') { + return handleMCPGet(action, request, target, OAuthResource.mcpConfig, providers, logger); + } + // Check if provider exists in static registry or dynamic cache let providerData = providers[providerName] ?? OAuthResource.dynamicProviderCache?.get(providerName); diff --git a/src/types.ts b/src/types.ts index c34a86f..31f1400 100644 --- a/src/types.ts +++ b/src/types.ts @@ -59,6 +59,14 @@ export interface MCPConfig { * the request origin (scheme + host) when unset. */ issuer?: string; + /** + * Subset of upstream providers eligible to complete the MCP auth flow. + * v1 requires exactly one effective provider — if the list (or, when + * unset, the full provider registry) resolves to 0 or >1 candidates, + * /oauth/mcp/authorize returns a configuration error. Multi-provider + * chooser UI is v1.1. + */ + providers?: string[]; /** Dynamic Client Registration settings (RFC 7591) */ dynamicClientRegistration?: MCPDynamicClientRegistrationConfig; } @@ -126,6 +134,48 @@ export interface MCPClientRecord extends MCPClientMetadata { client_secret_expires_at?: number; } +/** + * MCP authorization-request state carried through the upstream IdP round-trip. + * + * Stored inside the CSRF token (which the upstream provider echoes back as + * `state`); the callback handler reads this back to determine whether to mint + * an MCP authorization code (this object present) or fall through to the + * standard human-session flow (this object absent). + */ +export interface MCPAuthorizeState { + /** DCR client_id from /oauth/mcp/authorize */ + clientId: string; + /** RFC 8707 canonical resource URI */ + resource: string; + /** PKCE challenge (RFC 7636); verified at /oauth/mcp/token */ + codeChallenge: string; + /** PKCE method — only "S256" supported (OAuth 2.1 §7.5.2) */ + codeChallengeMethod: 'S256'; + /** Exact redirect_uri the MCP client wants the code delivered to */ + redirectUri: string; + /** Requested scope, space-separated (may be empty) */ + scope?: string; + /** Original `state` parameter from the MCP client; echoed verbatim on redirect */ + clientState?: string; +} + +/** + * MCP authorization code record (table `mcp_auth_codes`, `expiration: 300`). + * + * One-time use: /oauth/mcp/token (Stage 4) reads-then-deletes. + */ +export interface MCPAuthCodeRecord { + code: string; + client_id: string; + user: string; + resource: string; + code_challenge: string; + code_challenge_method: string; + redirect_uri: string; + scope?: string; + created_at: number; +} + /** * OAuth Lifecycle Hooks * Callbacks invoked at key points in the OAuth flow diff --git a/test/lib/handlers.test.js b/test/lib/handlers.test.js index 102f78b..8b73adf 100644 --- a/test/lib/handlers.test.js +++ b/test/lib/handlers.test.js @@ -683,6 +683,165 @@ describe('OAuth Handlers', () => { }); }); + describe('handleCallback — MCP branch', () => { + let originalDatabases; + let storedAuthCodes; + // Import lazily so the dist symbol load doesn't happen at file parse. + let resetMCPAuthCodesTableCache; + + const MCP_STATE = { + clientId: 'mcp-client-1', + resource: 'https://app.example.com/mcp', + codeChallenge: 'fake-challenge-32-chars-or-longer', + codeChallengeMethod: 'S256', + redirectUri: 'https://mcp-client.example.com/cb', + scope: 'mcp:read', + clientState: 'mcp-state-xyz', + }; + + beforeEach(async () => { + ({ resetMCPAuthCodesTableCache } = await import('../../dist/lib/mcp/authCodeStore.js')); + resetMCPAuthCodesTableCache(); + originalDatabases = global.databases; + storedAuthCodes = new Map(); + global.databases = { + oauth: { + mcp_auth_codes: { + get: async (id) => storedAuthCodes.get(id) ?? null, + put: async (record) => { + storedAuthCodes.set(record.code, record); + }, + delete: async (id) => storedAuthCodes.delete(id), + }, + }, + }; + // Replace verifyCSRFToken to return MCP state by default for this block. + mockProvider.verifyCSRFToken = createMockFn(async () => ({ + timestamp: Date.now(), + providerName: 'test-provider', + mcp: { ...MCP_STATE }, + })); + }); + + // Restore after each test block via the outer afterEach (no-op if global.databases survives — we use a fresh object each time). + + it('on success, mints auth code and redirects to MCP client redirect_uri', async () => { + const result = await handleCallback( + mockRequest, + mockTarget, + mockProvider, + mockConfig, + mockHookManager, + 'test-provider', + mockLogger + ); + assert.equal(result.status, 302); + const url = new URL(result.headers.Location); + assert.equal(url.origin + url.pathname, MCP_STATE.redirectUri); + assert.ok(url.searchParams.get('code')); + assert.equal(url.searchParams.get('state'), MCP_STATE.clientState); + assert.equal(storedAuthCodes.size, 1); + global.databases = originalDatabases; + }); + + it('binds the auth code to onLogin-mapped user (hookData.user wins over OAuth username)', async () => { + mockHookManager.callOnLogin = createMockFn(async () => ({ user: 'internal-user-id-42' })); + const result = await handleCallback( + mockRequest, + mockTarget, + mockProvider, + mockConfig, + mockHookManager, + 'test-provider', + mockLogger + ); + assert.equal(result.status, 302); + const [record] = storedAuthCodes.values(); + assert.equal(record.user, 'internal-user-id-42'); + global.databases = originalDatabases; + }); + + it('routes upstream IdP error to MCP client redirect_uri (not Harper postLoginRedirect)', async () => { + mockTarget.get = createMockFn((key) => { + const params = { + state: 'csrf-token-123', + error: 'access_denied', + error_description: 'User denied authorization', + }; + return params[key]; + }); + const result = await handleCallback( + mockRequest, + mockTarget, + mockProvider, + mockConfig, + mockHookManager, + 'test-provider', + mockLogger + ); + assert.equal(result.status, 302); + const url = new URL(result.headers.Location); + assert.equal(url.origin + url.pathname, MCP_STATE.redirectUri); + assert.equal(url.searchParams.get('error'), 'access_denied'); + assert.equal(url.searchParams.get('state'), MCP_STATE.clientState); + global.databases = originalDatabases; + }); + + it('routes cross-provider state mismatch to MCP redirect_uri', async () => { + mockProvider.verifyCSRFToken = createMockFn(async () => ({ + timestamp: Date.now(), + providerName: 'other-provider', // mismatch + mcp: { ...MCP_STATE }, + })); + const result = await handleCallback( + mockRequest, + mockTarget, + mockProvider, + mockConfig, + mockHookManager, + 'test-provider', + mockLogger + ); + assert.equal(result.status, 302); + const url = new URL(result.headers.Location); + assert.equal(url.origin + url.pathname, MCP_STATE.redirectUri); + assert.equal(url.searchParams.get('error'), 'invalid_request'); + global.databases = originalDatabases; + }); + + it('does NOT include upstream IdP token in MCP redirect URL', async () => { + const result = await handleCallback( + mockRequest, + mockTarget, + mockProvider, + mockConfig, + mockHookManager, + 'test-provider', + mockLogger + ); + const location = result.headers.Location; + for (const banned of ['access_token', 'refresh_token', 'id_token', 'token_type', 'access-token-123']) { + assert.ok(!location.includes(banned), `${banned} must not appear in MCP redirect URL`); + } + global.databases = originalDatabases; + }); + + it('does NOT create a Harper session on the MCP branch (independent lifecycle)', async () => { + await handleCallback( + mockRequest, + mockTarget, + mockProvider, + mockConfig, + mockHookManager, + 'test-provider', + mockLogger + ); + // session.update must not have been called for the MCP branch + assert.equal(mockRequest.session.update.mock.calls.length, 0); + global.databases = originalDatabases; + }); + }); + describe('handleLogout', () => { it('should clear session data', async () => { // Add delete method mock to session diff --git a/test/lib/mcp/authCodeStore.test.js b/test/lib/mcp/authCodeStore.test.js new file mode 100644 index 0000000..249a6d9 --- /dev/null +++ b/test/lib/mcp/authCodeStore.test.js @@ -0,0 +1,117 @@ +/** + * Tests for MCPAuthCodeStore + */ + +import { describe, it, before, after, beforeEach } from 'node:test'; +import assert from 'node:assert/strict'; +import { MCPAuthCodeStore, resetMCPAuthCodesTableCache } from '../../../dist/lib/mcp/authCodeStore.js'; + +function asTrackedObject(plain) { + return new Proxy(plain, { + ownKeys() { + return []; + }, + getOwnPropertyDescriptor() { + return undefined; + }, + }); +} + +describe('MCPAuthCodeStore', () => { + let store; + let originalDatabases; + let storedRecords; + let mockTable; + + before(() => { + originalDatabases = global.databases; + }); + + after(() => { + global.databases = originalDatabases; + }); + + beforeEach(() => { + resetMCPAuthCodesTableCache(); + store = new MCPAuthCodeStore(); + storedRecords = new Map(); + mockTable = { + get: async (id) => { + const raw = storedRecords.get(id); + return raw ? asTrackedObject(raw) : null; + }, + put: async (record) => { + storedRecords.set(record.code, record); + }, + delete: async (id) => { + storedRecords.delete(id); + }, + }; + global.databases = { + oauth: { + mcp_auth_codes: mockTable, + }, + }; + }); + + const sampleRecord = { + code: 'sample-code-abc', + client_id: 'client-123', + user: 'alice@example.com', + resource: 'https://app.example.com/mcp', + code_challenge: 'fake-challenge', + code_challenge_method: 'S256', + redirect_uri: 'https://mcp-client.example.com/cb', + scope: 'mcp:read', + created_at: 1700000000, + }; + + it('persists and retrieves a code, preserving scalar fields through tracked-object Proxy', async () => { + await store.set(sampleRecord); + + const retrieved = await store.get(sampleRecord.code); + assert.ok(retrieved, 'record retrieved'); + assert.equal(retrieved.code, sampleRecord.code); + assert.equal(retrieved.client_id, sampleRecord.client_id); + assert.equal(retrieved.user, sampleRecord.user); + assert.equal(retrieved.resource, sampleRecord.resource); + assert.equal(retrieved.code_challenge, sampleRecord.code_challenge); + assert.equal(retrieved.code_challenge_method, sampleRecord.code_challenge_method); + assert.equal(retrieved.redirect_uri, sampleRecord.redirect_uri); + assert.equal(retrieved.scope, sampleRecord.scope); + assert.equal(retrieved.created_at, sampleRecord.created_at); + }); + + it('returns null for an unknown code', async () => { + assert.equal(await store.get('not-real'), null); + }); + + it('deletes a code (single-use semantics enforced by /token in Stage 4)', async () => { + await store.set(sampleRecord); + assert.equal(storedRecords.size, 1); + await store.delete(sampleRecord.code); + assert.equal(storedRecords.size, 0); + }); + + it('returns null when the underlying get throws', async () => { + mockTable.get = async () => { + throw new Error('db failure'); + }; + assert.equal(await store.get('whatever'), null); + }); + + it('propagates set() errors so callers can fail the request', async () => { + mockTable.put = async () => { + throw new Error('db write failure'); + }; + await assert.rejects(() => store.set(sampleRecord)); + }); + + it('records without a scope decode as scope=undefined', async () => { + const { scope, ...withoutScope } = sampleRecord; + void scope; + await store.set(withoutScope); + const retrieved = await store.get(sampleRecord.code); + assert.equal(retrieved.scope, undefined); + }); +}); diff --git a/test/lib/mcp/authorize.test.js b/test/lib/mcp/authorize.test.js new file mode 100644 index 0000000..3c184b8 --- /dev/null +++ b/test/lib/mcp/authorize.test.js @@ -0,0 +1,373 @@ +/** + * Tests for /oauth/mcp/authorize handler. + */ + +import { describe, it, before, after, beforeEach } from 'node:test'; +import assert from 'node:assert/strict'; +import { handleAuthorize, redirectUriMatches, selectMCPProvider } from '../../../dist/lib/mcp/authorize.js'; +import { resetMCPClientsTableCache } from '../../../dist/lib/mcp/clientStore.js'; + +/** + * Minimal RequestTarget stub for the existing target.get?.() pattern. + */ +function makeTarget(params) { + return { + get(name) { + return Object.prototype.hasOwnProperty.call(params, name) ? params[name] : undefined; + }, + }; +} + +function makeRequest(overrides = {}) { + return { + protocol: 'https', + host: 'app.example.com', + headers: { host: 'app.example.com' }, + ...overrides, + }; +} + +const VALID_CLIENT = { + client_id: 'client-1', + client_id_issued_at: 1700000000, + redirect_uris: ['https://mcp-client.example.com/cb', 'http://localhost:6274/cb'], + grant_types: ['authorization_code', 'refresh_token'], + response_types: ['code'], + token_endpoint_auth_method: 'none', + application_type: 'web', +}; + +function makeProvider(name = 'github') { + const generatedTokens = []; + const builtUrls = []; + const provider = { + generateCSRFToken: async (metadata) => { + generatedTokens.push(metadata); + return `csrf-${generatedTokens.length}`; + }, + getAuthorizationUrl: (state, redirectUri) => { + builtUrls.push({ state, redirectUri }); + return `https://upstream.example.com/authorize?state=${encodeURIComponent(state)}&redirect_uri=${encodeURIComponent(redirectUri)}`; + }, + }; + const config = { + provider: name, + clientId: 'upstream-client', + clientSecret: 'upstream-secret', + authorizationUrl: 'https://upstream.example.com/authorize', + tokenUrl: 'https://upstream.example.com/token', + userInfoUrl: 'https://upstream.example.com/userinfo', + redirectUri: 'https://app.example.com/oauth/' + name + '/callback', + }; + return { provider, config, generatedTokens, builtUrls }; +} + +function makeProviderRegistry(...names) { + const entries = {}; + const harnesses = {}; + for (const name of names) { + const h = makeProvider(name); + entries[name] = { provider: h.provider, config: h.config }; + harnesses[name] = h; + } + return { entries, harnesses }; +} + +const BASE_QUERY = { + client_id: VALID_CLIENT.client_id, + redirect_uri: VALID_CLIENT.redirect_uris[0], + response_type: 'code', + code_challenge: 'fake-challenge-value-32-chars-min', + code_challenge_method: 'S256', + resource: 'https://app.example.com/mcp', + state: 'mcp-client-state', + scope: 'mcp:read', +}; + +describe('redirectUriMatches', () => { + it('matches by exact string for non-loopback hosts', () => { + assert.equal(redirectUriMatches('https://app.example.com/cb', ['https://app.example.com/cb']), true); + assert.equal(redirectUriMatches('https://app.example.com/cb', ['https://app.example.com/other']), false); + assert.equal(redirectUriMatches('https://app.example.com/cb', []), false); + }); + + it('requires exact port match for non-loopback hosts', () => { + assert.equal( + redirectUriMatches('https://app.example.com:8443/cb', ['https://app.example.com/cb']), + false, + 'different ports should not match for non-loopback' + ); + }); + + it('accepts any port for 127.0.0.1 (RFC 8252 §7.3)', () => { + assert.equal(redirectUriMatches('http://127.0.0.1:54321/cb', ['http://127.0.0.1:6274/cb']), true); + assert.equal(redirectUriMatches('http://127.0.0.1/cb', ['http://127.0.0.1:6274/cb']), true); + }); + + it('accepts any port for [::1]', () => { + assert.equal(redirectUriMatches('http://[::1]:54321/cb', ['http://[::1]:6274/cb']), true); + }); + + it('accepts any port for localhost', () => { + assert.equal(redirectUriMatches('http://localhost:54321/cb', ['http://localhost:6274/cb']), true); + }); + + it('still requires matching path on loopback', () => { + assert.equal(redirectUriMatches('http://localhost:5000/different', ['http://localhost:6274/cb']), false); + }); + + it('still requires matching scheme on loopback (no http vs https)', () => { + assert.equal(redirectUriMatches('https://localhost:5000/cb', ['http://localhost:6274/cb']), false); + }); + + it('does not cross-match loopback variants (127.0.0.1 != localhost != [::1])', () => { + assert.equal(redirectUriMatches('http://127.0.0.1:5000/cb', ['http://localhost:6274/cb']), false); + assert.equal(redirectUriMatches('http://localhost:5000/cb', ['http://[::1]:6274/cb']), false); + }); + + it('rejects malformed URIs', () => { + assert.equal(redirectUriMatches('not-a-url', ['http://localhost:6274/cb']), false); + }); +}); + +describe('selectMCPProvider', () => { + it('returns the only provider when none are filtered', () => { + const result = selectMCPProvider({}, { github: {} }); + assert.deepEqual(result, { providerName: 'github' }); + }); + + it('uses mcp.providers to filter when multiple are configured', () => { + const result = selectMCPProvider({ providers: ['github'] }, { github: {}, google: {} }); + assert.deepEqual(result, { providerName: 'github' }); + }); + + it('returns an error when no providers are available', () => { + const result = selectMCPProvider({}, {}); + assert.ok('error' in result); + assert.equal(result.error, 'server_error'); + }); + + it('returns an error when mcp.providers references nothing valid', () => { + const result = selectMCPProvider({ providers: ['nonexistent'] }, { github: {} }); + assert.ok('error' in result); + assert.equal(result.error, 'server_error'); + }); + + it('returns an error when multiple providers resolve and the chooser would be needed', () => { + const result = selectMCPProvider({}, { github: {}, google: {} }); + assert.ok('error' in result); + assert.match(result.description, /exactly one/); + }); +}); + +describe('handleAuthorize', () => { + let originalDatabases; + let storedClients; + + before(() => { + originalDatabases = global.databases; + }); + + after(() => { + global.databases = originalDatabases; + }); + + function encodeClientForStorage(client) { + // MCPClientStore.decodeRecord expects JSON-encoded array fields — that's + // what the real Harper table stores. Mirror that representation here so + // the round-trip matches production. + const encoded = { ...client }; + for (const field of ['redirect_uris', 'contacts', 'grant_types', 'response_types']) { + if (Array.isArray(encoded[field])) encoded[field] = JSON.stringify(encoded[field]); + } + return encoded; + } + + beforeEach(() => { + resetMCPClientsTableCache(); + storedClients = new Map([[VALID_CLIENT.client_id, encodeClientForStorage(VALID_CLIENT)]]); + global.databases = { + oauth: { + harper_oauth_mcp_clients: { + get: async (id) => storedClients.get(id) ?? null, + put: async () => {}, + delete: async (id) => storedClients.delete(id), + }, + }, + }; + }); + + const validConfig = { enabled: true }; + const newRegistry = () => makeProviderRegistry('github'); + + describe('phase 1 — pre-redirect validation (cannot safely redirect)', () => { + it('rejects missing client_id with 400 JSON', async () => { + const { entries } = newRegistry(); + const target = makeTarget({ ...BASE_QUERY, client_id: undefined }); + const response = await handleAuthorize(makeRequest(), target, validConfig, entries); + assert.equal(response.status, 400); + assert.equal(response.body.error, 'invalid_request'); + }); + + it('rejects unknown client_id with 400 invalid_client', async () => { + const { entries } = newRegistry(); + const target = makeTarget({ ...BASE_QUERY, client_id: 'unknown-client' }); + const response = await handleAuthorize(makeRequest(), target, validConfig, entries); + assert.equal(response.status, 400); + assert.equal(response.body.error, 'invalid_client'); + }); + + it('rejects unregistered redirect_uri with 400 (no redirect to unverified URI)', async () => { + const { entries } = newRegistry(); + const target = makeTarget({ ...BASE_QUERY, redirect_uri: 'https://attacker.example.com/steal' }); + const response = await handleAuthorize(makeRequest(), target, validConfig, entries); + assert.equal(response.status, 400); + assert.equal(response.body.error, 'invalid_request'); + assert.match(response.body.error_description, /redirect_uri/); + }); + + it('rejects missing redirect_uri with 400', async () => { + const { entries } = newRegistry(); + const target = makeTarget({ ...BASE_QUERY, redirect_uri: undefined }); + const response = await handleAuthorize(makeRequest(), target, validConfig, entries); + assert.equal(response.status, 400); + }); + }); + + describe('phase 2 — post-validation redirect to client redirect_uri with error', () => { + function parseRedirect(response) { + assert.equal(response.status, 302); + const url = new URL(response.headers.Location); + return { + host: url.host, + params: Object.fromEntries(url.searchParams), + }; + } + + it('redirects with unsupported_response_type when response_type is not "code"', async () => { + const { entries } = newRegistry(); + const target = makeTarget({ ...BASE_QUERY, response_type: 'token' }); + const response = await handleAuthorize(makeRequest(), target, validConfig, entries); + const { host, params } = parseRedirect(response); + assert.equal(host, 'mcp-client.example.com'); + assert.equal(params.error, 'unsupported_response_type'); + assert.equal(params.state, BASE_QUERY.state, 'client state echoed back'); + }); + + it('redirects with invalid_request when PKCE code_challenge missing', async () => { + const { entries } = newRegistry(); + const target = makeTarget({ ...BASE_QUERY, code_challenge: undefined }); + const response = await handleAuthorize(makeRequest(), target, validConfig, entries); + const { params } = parseRedirect(response); + assert.equal(params.error, 'invalid_request'); + assert.match(params.error_description, /code_challenge/); + }); + + it('redirects with invalid_request when code_challenge_method is "plain" (OAuth 2.1 forbids)', async () => { + const { entries } = newRegistry(); + const target = makeTarget({ ...BASE_QUERY, code_challenge_method: 'plain' }); + const response = await handleAuthorize(makeRequest(), target, validConfig, entries); + const { params } = parseRedirect(response); + assert.equal(params.error, 'invalid_request'); + assert.match(params.error_description, /S256/); + }); + + it('redirects with invalid_target when resource is missing', async () => { + const { entries } = newRegistry(); + const target = makeTarget({ ...BASE_QUERY, resource: undefined }); + const response = await handleAuthorize(makeRequest(), target, validConfig, entries); + const { params } = parseRedirect(response); + assert.equal(params.error, 'invalid_target'); + }); + + it('redirects with invalid_target when resource has a fragment', async () => { + const { entries } = newRegistry(); + const target = makeTarget({ ...BASE_QUERY, resource: 'https://app.example.com/mcp#frag' }); + const response = await handleAuthorize(makeRequest(), target, validConfig, entries); + const { params } = parseRedirect(response); + assert.equal(params.error, 'invalid_target'); + }); + + it('redirects with invalid_target when resource does not match this server', async () => { + const { entries } = newRegistry(); + const target = makeTarget({ ...BASE_QUERY, resource: 'https://other-server.example.com/mcp' }); + const response = await handleAuthorize(makeRequest(), target, validConfig, entries); + const { params } = parseRedirect(response); + assert.equal(params.error, 'invalid_target'); + }); + + it('redirects with server_error when no upstream provider is configured', async () => { + const target = makeTarget(BASE_QUERY); + const response = await handleAuthorize(makeRequest(), target, validConfig, {}); + const { params } = parseRedirect(response); + assert.equal(params.error, 'server_error'); + }); + + it('redirects with server_error when multiple upstream providers resolve (v1 single-provider requirement)', async () => { + const { entries } = makeProviderRegistry('github', 'google'); + const target = makeTarget(BASE_QUERY); + const response = await handleAuthorize(makeRequest(), target, validConfig, entries); + const { params } = parseRedirect(response); + assert.equal(params.error, 'server_error'); + }); + }); + + describe('happy path', () => { + it('redirects to upstream provider with CSRF token carrying MCP state', async () => { + const { entries, harnesses } = newRegistry(); + const target = makeTarget(BASE_QUERY); + const response = await handleAuthorize(makeRequest(), target, validConfig, entries); + + assert.equal(response.status, 302); + const upstreamUrl = new URL(response.headers.Location); + assert.equal(upstreamUrl.host, 'upstream.example.com'); + + // The CSRF generation got the MCP state + assert.equal(harnesses.github.generatedTokens.length, 1); + const metadata = harnesses.github.generatedTokens[0]; + assert.equal(metadata.providerName, 'github'); + assert.ok(metadata.mcp); + assert.equal(metadata.mcp.clientId, VALID_CLIENT.client_id); + assert.equal(metadata.mcp.resource, BASE_QUERY.resource); + assert.equal(metadata.mcp.codeChallenge, BASE_QUERY.code_challenge); + assert.equal(metadata.mcp.codeChallengeMethod, 'S256'); + assert.equal(metadata.mcp.redirectUri, BASE_QUERY.redirect_uri); + assert.equal(metadata.mcp.scope, BASE_QUERY.scope); + assert.equal(metadata.mcp.clientState, BASE_QUERY.state); + }); + + it('accepts a redirect_uri that matches the registered loopback URI', async () => { + const { entries } = newRegistry(); + const target = makeTarget({ ...BASE_QUERY, redirect_uri: 'http://localhost:6274/cb' }); + const response = await handleAuthorize(makeRequest(), target, validConfig, entries); + assert.equal(response.status, 302); + const url = new URL(response.headers.Location); + assert.equal(url.host, 'upstream.example.com'); + }); + + it('accepts a different port on a registered loopback URI (RFC 8252 §7.3)', async () => { + const { entries } = newRegistry(); + // Native client registered :6274 but bound to :54321 at runtime + const target = makeTarget({ ...BASE_QUERY, redirect_uri: 'http://localhost:54321/cb' }); + const response = await handleAuthorize(makeRequest(), target, validConfig, entries); + assert.equal(response.status, 302, 'loopback port flexibility per RFC 8252 §7.3'); + const url = new URL(response.headers.Location); + assert.equal(url.host, 'upstream.example.com'); + }); + + it('respects an explicitly configured mcp.resource', async () => { + const { entries } = newRegistry(); + const customResource = 'https://app.example.com/custom-mcp'; + const target = makeTarget({ ...BASE_QUERY, resource: customResource }); + const response = await handleAuthorize( + makeRequest(), + target, + { enabled: true, resource: customResource }, + entries + ); + assert.equal(response.status, 302); + const url = new URL(response.headers.Location); + assert.equal(url.host, 'upstream.example.com'); + }); + }); +}); diff --git a/test/lib/mcp/callback.test.js b/test/lib/mcp/callback.test.js new file mode 100644 index 0000000..86a9cd4 --- /dev/null +++ b/test/lib/mcp/callback.test.js @@ -0,0 +1,119 @@ +/** + * Tests for the MCP callback branch (handleMCPCallback). + * + * Verifies it mints an authorization code, persists the binding fields, + * redirects to the client's redirect_uri with `code` + echoed `state`, + * and never leaks upstream-provider token material in the response. + */ + +import { describe, it, before, after, beforeEach } from 'node:test'; +import assert from 'node:assert/strict'; +import { handleMCPCallback } from '../../../dist/lib/mcp/callback.js'; +import { resetMCPAuthCodesTableCache } from '../../../dist/lib/mcp/authCodeStore.js'; + +const SAMPLE_MCP_STATE = { + clientId: 'client-abc', + resource: 'https://app.example.com/mcp', + codeChallenge: 'fake-challenge-32-chars-or-longer', + codeChallengeMethod: 'S256', + redirectUri: 'https://mcp-client.example.com/cb', + scope: 'mcp:read', + clientState: 'mcp-client-state', +}; + +const SAMPLE_USER_ID = 'alice@example.com'; + +describe('handleMCPCallback', () => { + let originalDatabases; + let storedRecords; + let mockTable; + + before(() => { + originalDatabases = global.databases; + }); + + after(() => { + global.databases = originalDatabases; + }); + + beforeEach(() => { + resetMCPAuthCodesTableCache(); + storedRecords = new Map(); + mockTable = { + get: async (id) => storedRecords.get(id) ?? null, + put: async (record) => { + storedRecords.set(record.code, record); + }, + delete: async (id) => storedRecords.delete(id), + }; + global.databases = { + oauth: { + mcp_auth_codes: mockTable, + }, + }; + }); + + it('mints an auth code and persists the binding fields', async () => { + const response = await handleMCPCallback({}, SAMPLE_MCP_STATE, SAMPLE_USER_ID); + assert.equal(response.status, 302); + assert.equal(storedRecords.size, 1); + const [record] = storedRecords.values(); + assert.ok(record.code); + assert.equal(record.client_id, SAMPLE_MCP_STATE.clientId); + assert.equal(record.user, SAMPLE_USER_ID); + assert.equal(record.resource, SAMPLE_MCP_STATE.resource); + assert.equal(record.code_challenge, SAMPLE_MCP_STATE.codeChallenge); + assert.equal(record.code_challenge_method, SAMPLE_MCP_STATE.codeChallengeMethod); + assert.equal(record.redirect_uri, SAMPLE_MCP_STATE.redirectUri); + assert.equal(record.scope, SAMPLE_MCP_STATE.scope); + assert.equal(typeof record.created_at, 'number'); + }); + + it('redirects to the client redirect_uri with code and echoed state', async () => { + const response = await handleMCPCallback({}, SAMPLE_MCP_STATE, SAMPLE_USER_ID); + const url = new URL(response.headers.Location); + assert.equal(url.origin + url.pathname, SAMPLE_MCP_STATE.redirectUri); + assert.ok(url.searchParams.get('code')); + assert.equal(url.searchParams.get('state'), SAMPLE_MCP_STATE.clientState); + }); + + it('omits state when the MCP client did not send one', async () => { + const { clientState, ...stateWithoutClientState } = SAMPLE_MCP_STATE; + void clientState; + const response = await handleMCPCallback({}, stateWithoutClientState, SAMPLE_USER_ID); + const url = new URL(response.headers.Location); + assert.equal(url.searchParams.has('state'), false); + assert.ok(url.searchParams.get('code')); + }); + + it('generates a fresh code per invocation (no reuse across requests)', async () => { + const a = await handleMCPCallback({}, SAMPLE_MCP_STATE, SAMPLE_USER_ID); + const b = await handleMCPCallback({}, SAMPLE_MCP_STATE, SAMPLE_USER_ID); + const codeA = new URL(a.headers.Location).searchParams.get('code'); + const codeB = new URL(b.headers.Location).searchParams.get('code'); + assert.notEqual(codeA, codeB); + assert.equal(storedRecords.size, 2); + }); + + it('redirects with error when persistence fails (does not throw)', async () => { + mockTable.put = async () => { + throw new Error('db write failure'); + }; + const response = await handleMCPCallback({}, SAMPLE_MCP_STATE, SAMPLE_USER_ID); + assert.equal(response.status, 302); + const url = new URL(response.headers.Location); + assert.equal(url.origin + url.pathname, SAMPLE_MCP_STATE.redirectUri); + assert.equal(url.searchParams.get('error'), 'server_error'); + assert.equal(url.searchParams.get('state'), SAMPLE_MCP_STATE.clientState); + }); + + it('never includes upstream provider token in the redirect URL', async () => { + // `handleMCPCallback` signature doesn't take a token — that's the guard. + // But assert no field on the response references known token-ish names. + const response = await handleMCPCallback({}, SAMPLE_MCP_STATE, SAMPLE_USER_ID); + const location = response.headers.Location; + for (const banned of ['access_token', 'refresh_token', 'id_token', 'token_type']) { + assert.ok(!location.includes(banned), `${banned} must not appear in MCP redirect URL`); + } + }); +}); diff --git a/test/lib/mcp/index.test.js b/test/lib/mcp/index.test.js index 5572f3b..c76d15d 100644 --- a/test/lib/mcp/index.test.js +++ b/test/lib/mcp/index.test.js @@ -8,7 +8,7 @@ import { describe, it, before, after, beforeEach } from 'node:test'; import assert from 'node:assert/strict'; -import { handleMCPPost } from '../../../dist/lib/mcp/index.js'; +import { handleMCPGet, handleMCPPost } from '../../../dist/lib/mcp/index.js'; import { resetMCPClientsTableCache } from '../../../dist/lib/mcp/clientStore.js'; const VALID_BODY = { @@ -110,3 +110,32 @@ describe('handleMCPPost (dispatcher)', () => { }); }); }); + +describe('handleMCPGet (dispatcher)', () => { + function makeTarget(params = {}) { + return { + get(name) { + return Object.prototype.hasOwnProperty.call(params, name) ? params[name] : undefined; + }, + }; + } + + it('returns 404 when MCP is disabled', async () => { + const response = await handleMCPGet('authorize', makeRequest(), makeTarget(), undefined, {}); + assert.equal(response.status, 404); + }); + + it('returns 404 for an unknown action', async () => { + const response = await handleMCPGet('not-a-thing', makeRequest(), makeTarget(), { enabled: true }, {}); + assert.equal(response.status, 404); + }); + + it('dispatches authorize through handleAuthorize (verified via 400 invalid_request on empty query)', async () => { + // handleAuthorize will hit phase-1 client_id validation; getting that 400 + // confirms the dispatcher routed us into authorize rather than the + // catch-all 404. + const response = await handleMCPGet('authorize', makeRequest(), makeTarget(), { enabled: true }, {}); + assert.equal(response.status, 400); + assert.equal(response.body.error, 'invalid_request'); + }); +});