From 7c58232a218e6b5cfcc7e1df087746e4582aa602 Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Thu, 21 May 2026 04:58:42 -0700 Subject: [PATCH 1/3] Add MCP Dynamic Client Registration (RFC 7591) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stage 1 of MCP OAuth support (issue #86). Adds the persistent client registry and POST /oauth/mcp/register endpoint so MCP clients (Claude Desktop, Cursor, mcp-remote) that cache their issued client_id continue working across Harper restarts. - New harper_oauth_mcp_clients table declared in schema/oauth.graphql - MCPClientStore mirrors the existing CSRFTokenManager pattern - handleRegister implements RFC 7591: redirect URI validation (HTTPS-only except loopback per RFC 8252 §8.3, no fragments, optional host allowlist), public-client default (deviation from RFC default client_secret_basic, documented), supported grant_types and response_types whitelisted - Sensitive config leaves under `mcp` (e.g. initialAccessToken) support ${ENV_VAR} expansion via expandEnvVarsDeep - Endpoint dispatched through OAuthResource.post when providerName=mcp; /authorize and /token follow in subsequent stages Co-Authored-By: Claude Opus 4.7 (1M context) --- schema/oauth.graphql | 25 +++ src/index.ts | 17 +- src/lib/config.ts | 30 ++- src/lib/mcp/clientStore.ts | 128 +++++++++++ src/lib/mcp/dcr.ts | 283 ++++++++++++++++++++++++ src/lib/mcp/index.ts | 37 ++++ src/lib/resource.ts | 18 +- src/types.ts | 82 +++++++ test/lib/config.test.js | 37 ++++ test/lib/mcp/clientStore.test.js | 139 ++++++++++++ test/lib/mcp/dcr.test.js | 362 +++++++++++++++++++++++++++++++ 11 files changed, 1149 insertions(+), 9 deletions(-) create mode 100644 src/lib/mcp/clientStore.ts create mode 100644 src/lib/mcp/dcr.ts create mode 100644 src/lib/mcp/index.ts create mode 100644 test/lib/mcp/clientStore.test.js create mode 100644 test/lib/mcp/dcr.test.js diff --git a/schema/oauth.graphql b/schema/oauth.graphql index e9b7fa3..a990914 100644 --- a/schema/oauth.graphql +++ b/schema/oauth.graphql @@ -9,6 +9,31 @@ type csrf_tokens @table(database: "oauth", expiration: 600) { created_at: Float } +## MCP Dynamic Client Registration table (RFC 7591) +## Stores OAuth client registrations for MCP clients (Claude Desktop, Cursor, +## mcp-remote, etc.). No expiration: clients like Claude Desktop cache their +## issued client_id across launches, so registrations must survive restarts. +## Array-valued fields (redirect_uris, contacts, grant_types, response_types) +## are stored as JSON-encoded strings to match the existing csrf_tokens pattern. +type harper_oauth_mcp_clients @table(database: "oauth") { + client_id: ID @primaryKey + client_secret: String + redirect_uris: String # JSON array of allowed redirect URIs + client_name: String + client_uri: String + logo_uri: String + scope: String + contacts: String # JSON array + grant_types: String # JSON array; default ["authorization_code", "refresh_token"] + response_types: String # JSON array; default ["code"] + token_endpoint_auth_method: String # "none" for public clients (default), "client_secret_basic", "client_secret_post" + application_type: String # "web" (default) or "native" + software_id: String + software_version: String + client_id_issued_at: Float + client_secret_expires_at: Float # 0 = never expires +} + ## OAuth User Session table (optional, for future use) ## Could store OAuth-specific user data # type oauth_sessions @table(database: "oauth") { diff --git a/src/index.ts b/src/index.ts index 88e1fde..8e67d23 100644 --- a/src/index.ts +++ b/src/index.ts @@ -5,7 +5,7 @@ * Supports any standard OAuth 2.0 provider through configuration. */ -import { initializeProviders, expandEnvVar, extractPluginDefaults } from './lib/config.ts'; +import { initializeProviders, expandEnvVar, expandEnvVarsDeep, extractPluginDefaults } from './lib/config.ts'; import { OAuthResource } from './lib/resource.ts'; import { validateAndRefreshSession } from './lib/sessionValidator.ts'; import { clearOAuthSession } from './lib/handlers.ts'; @@ -175,8 +175,19 @@ export async function handleApplication(scope: Scope): Promise { }, }); } else { - // Configure the OAuth resource with providers and settings - OAuthResource.configure(providers, debugMode, hookManager, pluginDefaults, logger, dynamicProviderCache); + // Configure the OAuth resource with providers and settings. + // expandEnvVarsDeep on the mcp block so sensitive leaves like + // mcp.dynamicClientRegistration.initialAccessToken support ${ENV_VAR}. + const mcpConfig = options.mcp ? expandEnvVarsDeep(options.mcp) : undefined; + OAuthResource.configure( + providers, + debugMode, + hookManager, + pluginDefaults, + logger, + dynamicProviderCache, + mcpConfig + ); // Register the OAuth resource class resources.set('oauth', OAuthResource); diff --git a/src/lib/config.ts b/src/lib/config.ts index a78051a..fa60df2 100644 --- a/src/lib/config.ts +++ b/src/lib/config.ts @@ -31,6 +31,30 @@ export function expandEnvVar(value: any): any { return value; } +/** + * Recursively expand `${ENV_VAR}` placeholders on every string leaf of a value. + * + * Used for structured config blocks (like `mcp`) where sensitive leaves + * (e.g., `mcp.dynamicClientRegistration.initialAccessToken`) still need + * env-var expansion but the block itself isn't a flat property bag. + */ +export function expandEnvVarsDeep(value: T): T { + if (typeof value === 'string') { + return expandEnvVar(value); + } + if (Array.isArray(value)) { + return value.map(expandEnvVarsDeep) as unknown as T; + } + if (value !== null && typeof value === 'object') { + const expanded: Record = {}; + for (const [key, item] of Object.entries(value as Record)) { + expanded[key] = expandEnvVarsDeep(item); + } + return expanded as T; + } + return value; +} + /** * Build configuration for a specific provider */ @@ -117,9 +141,11 @@ export function buildProviderConfig( export function extractPluginDefaults(options: OAuthPluginConfig): Partial { const pluginDefaults: Partial = {}; - // Copy all non-provider config to defaults, expanding environment variables + // Copy all non-provider config to defaults, expanding environment variables. + // `mcp` is a structured top-level config block (not a provider-level default) + // so it's excluded; it's threaded through OAuthResource.configure separately. for (const [key, value] of Object.entries(options)) { - if (key !== 'providers' && key !== 'debug') { + if (key !== 'providers' && key !== 'debug' && key !== 'mcp') { pluginDefaults[key as keyof OAuthProviderConfig] = expandEnvVar(value); } } diff --git a/src/lib/mcp/clientStore.ts b/src/lib/mcp/clientStore.ts new file mode 100644 index 0000000..4dab929 --- /dev/null +++ b/src/lib/mcp/clientStore.ts @@ -0,0 +1,128 @@ +/** + * MCP Client Store + * + * Persists Dynamic Client Registration (RFC 7591) records in the Harper + * `harper_oauth_mcp_clients` table. Clients survive Harper restarts so MCP + * clients (Claude Desktop, Cursor, mcp-remote) that cache their issued + * client_id continue to authenticate after a deploy. + * + * Array-valued fields (redirect_uris, contacts, grant_types, response_types) + * are JSON-encoded on write and decoded on read, matching the + * csrf_tokens.data pattern already used in this plugin. + */ + +import type { Logger, MCPClientRecord, Table } from '../../types.ts'; + +// Harper's databases global contains all databases +declare const databases: any; + +let clientsTable: Table | undefined; + +function getMCPClientsTable(): Table { + if (!clientsTable) { + if (!databases?.oauth?.harper_oauth_mcp_clients) { + throw new Error( + 'OAuth MCP clients table (oauth.harper_oauth_mcp_clients) not found. ' + + 'Please ensure the OAuth plugin is properly installed with its schema.' + ); + } + clientsTable = databases.oauth.harper_oauth_mcp_clients; + } + return clientsTable as Table; +} + +/** + * Reset the cached table reference (for testing only) + * @internal + */ +export function resetMCPClientsTableCache(): void { + clientsTable = undefined; +} + +const ARRAY_FIELDS = ['redirect_uris', 'contacts', 'grant_types', 'response_types'] as const; + +function encodeRecord(record: MCPClientRecord): Record { + const encoded: Record = { ...record }; + for (const field of ARRAY_FIELDS) { + const value = record[field]; + encoded[field] = value === undefined ? undefined : JSON.stringify(value); + } + return encoded; +} + +function decodeRecord(raw: Record): MCPClientRecord { + const decoded: Record = { ...raw }; + for (const field of ARRAY_FIELDS) { + const value = raw[field]; + if (typeof value === 'string') { + try { + const parsed = JSON.parse(value); + decoded[field] = Array.isArray(parsed) ? parsed : undefined; + } catch { + // Malformed stored data — treat as absent to avoid crashing callers. + decoded[field] = undefined; + } + } else if (value === null) { + // Normalize null (e.g. from a DB column default) to undefined + // so the decoded record matches the `string[] | undefined` type. + decoded[field] = undefined; + } + } + return decoded as MCPClientRecord; +} + +export class MCPClientStore { + private logger?: Logger; + + constructor(logger?: Logger) { + this.logger = logger; + } + + /** + * Persist a client registration. Overwrites any existing record with the + * same client_id (RFC 7591 registration is idempotent at the storage layer; + * the registration endpoint allocates a fresh client_id per request). + */ + async set(record: MCPClientRecord): Promise { + const table = getMCPClientsTable(); + try { + await table.put(encodeRecord(record)); + this.logger?.debug?.(`Stored MCP client: ${record.client_id}`); + } catch (error) { + this.logger?.error?.('Failed to store MCP client:', error); + throw error; + } + } + + /** + * Look up a client by client_id. Returns null if not found or on read error + * (errors logged; we don't surface storage failures to OAuth clients). + */ + async get(clientId: string): Promise { + const table = getMCPClientsTable(); + try { + const raw = await table.get(clientId); + if (!raw || !raw.client_id) { + return null; + } + return decodeRecord(raw); + } catch (error) { + this.logger?.error?.('Failed to retrieve MCP client:', error); + return null; + } + } + + /** + * Remove a client registration. + */ + async delete(clientId: string): Promise { + const table = getMCPClientsTable(); + try { + await table.delete(clientId); + this.logger?.debug?.(`Deleted MCP client: ${clientId}`); + } catch (error) { + // Not critical if delete fails — admin can retry. + this.logger?.warn?.('Failed to delete MCP client:', error); + } + } +} diff --git a/src/lib/mcp/dcr.ts b/src/lib/mcp/dcr.ts new file mode 100644 index 0000000..ead42f2 --- /dev/null +++ b/src/lib/mcp/dcr.ts @@ -0,0 +1,283 @@ +/** + * MCP Dynamic Client Registration (RFC 7591) + * + * Implements POST /oauth/mcp/register. MCP clients (Claude Desktop, Cursor, + * mcp-remote) register at runtime with no pre-baked client_id; the registry + * persists their issued client_id so it survives Harper restarts. + * + * Defaults applied here reflect the MCP authorization spec 2025-06-18: public + * clients (token_endpoint_auth_method=none), authorization_code + refresh_token + * grants, response_type=code. Confidential clients can opt in explicitly. + */ + +import { randomUUID, randomBytes } from 'node:crypto'; +import type { Logger, MCPClientMetadata, MCPClientRecord, MCPConfig } from '../../types.ts'; +import { MCPClientStore } from './clientStore.ts'; + +type ErrorResponse = { + status: number; + body: { error: string; error_description?: string }; +}; + +const SUPPORTED_GRANT_TYPES = new Set(['authorization_code', 'refresh_token']); +const SUPPORTED_RESPONSE_TYPES = new Set(['code']); +const SUPPORTED_AUTH_METHODS = new Set(['none', 'client_secret_basic', 'client_secret_post']); +const LOCAL_HOSTS = new Set(['localhost', '127.0.0.1', '[::1]', '::1']); + +/** + * Validate the Authorization header against a configured initial access token. + * Returns null when no token is configured (open registration per RFC 7591). + * Node's HTTP parser lowercases incoming headers, so we read the lowercase form. + */ +function checkInitialAccessToken(authHeader: string | undefined, configured: string | undefined): ErrorResponse | null { + if (!configured) { + return null; + } + if (!authHeader || !authHeader.startsWith('Bearer ')) { + return { + status: 401, + body: { error: 'invalid_token', error_description: 'Missing initial access token' }, + }; + } + const presented = authHeader.slice('Bearer '.length).trim(); + if (presented !== configured) { + return { + status: 401, + body: { error: 'invalid_token', error_description: 'Invalid initial access token' }, + }; + } + return null; +} + +function validateOptionalString(value: unknown, fieldName: string): string | null { + if (value === undefined) return null; + if (typeof value !== 'string') return `${fieldName} must be a string`; + return null; +} + +/** + * Validate a single redirect URI against RFC 7591 + RFC 8252 rules. + * Returns an error message on failure, null on success. + */ +function validateRedirectUri(uri: unknown, allowedHosts: string[] | undefined): string | null { + if (typeof uri !== 'string' || uri.length === 0) { + return 'redirect_uris must contain non-empty strings'; + } + let parsed: URL; + try { + parsed = new URL(uri); + } catch { + return `redirect_uri is not a valid URL: ${uri}`; + } + if (parsed.hash) { + return `redirect_uri must not contain a fragment: ${uri}`; + } + const isLocal = LOCAL_HOSTS.has(parsed.hostname); + // HTTPS is required except for loopback addresses (RFC 8252 §8.3). + if (parsed.protocol !== 'https:' && !(parsed.protocol === 'http:' && isLocal)) { + return `redirect_uri must use https (or http to a loopback address): ${uri}`; + } + if (allowedHosts && allowedHosts.length > 0 && !isLocal && !allowedHosts.includes(parsed.hostname)) { + return `redirect_uri host not in allowlist: ${parsed.hostname}`; + } + return null; +} + +function validateStringArray(value: unknown, fieldName: string): string | null { + if (value === undefined) return null; + if (!Array.isArray(value)) return `${fieldName} must be an array of strings`; + for (const item of value) { + if (typeof item !== 'string') return `${fieldName} must be an array of strings`; + } + return null; +} + +/** + * Validate the request body and produce a normalized record (with defaults + * applied). Returns either a record-shape (no client_id yet) or an error. + */ +function buildClientFromRequest( + body: any, + allowedHosts: string[] | undefined +): { record: Omit } | ErrorResponse { + if (!body || typeof body !== 'object') { + return { + status: 400, + body: { error: 'invalid_client_metadata', error_description: 'Request body must be a JSON object' }, + }; + } + + // redirect_uris is required for clients using the authorization code flow. + if (!Array.isArray(body.redirect_uris) || body.redirect_uris.length === 0) { + return { + status: 400, + body: { error: 'invalid_redirect_uri', error_description: 'redirect_uris is required and must be non-empty' }, + }; + } + for (const uri of body.redirect_uris) { + const err = validateRedirectUri(uri, allowedHosts); + if (err) { + return { status: 400, body: { error: 'invalid_redirect_uri', error_description: err } }; + } + } + + for (const [field, value] of Object.entries({ + contacts: body.contacts, + grant_types: body.grant_types, + response_types: body.response_types, + })) { + const err = validateStringArray(value, field); + if (err) { + return { status: 400, body: { error: 'invalid_client_metadata', error_description: err } }; + } + } + + // Validate optional scalar string fields — without this an attacker could + // POST `{client_name: {evil: ...}}` and we'd persist the object verbatim. + for (const [field, value] of Object.entries({ + client_name: body.client_name, + client_uri: body.client_uri, + logo_uri: body.logo_uri, + scope: body.scope, + software_id: body.software_id, + software_version: body.software_version, + })) { + const err = validateOptionalString(value, field); + if (err) { + return { status: 400, body: { error: 'invalid_client_metadata', error_description: err } }; + } + } + + const grantTypes: string[] = body.grant_types ?? ['authorization_code', 'refresh_token']; + for (const grant of grantTypes) { + if (!SUPPORTED_GRANT_TYPES.has(grant)) { + return { + status: 400, + body: { error: 'invalid_client_metadata', error_description: `Unsupported grant_type: ${grant}` }, + }; + } + } + + const responseTypes: string[] = body.response_types ?? ['code']; + for (const responseType of responseTypes) { + if (!SUPPORTED_RESPONSE_TYPES.has(responseType)) { + return { + status: 400, + body: { + error: 'invalid_client_metadata', + error_description: `Unsupported response_type: ${responseType}`, + }, + }; + } + } + + // MCP default: public clients. RFC 7591's default is "client_secret_basic" + // but the MCP authorization spec 2025-06-18 expects public clients (Claude + // Desktop, Cursor, mcp-remote run on user machines without secure secret + // storage). Confidential clients must opt in explicitly. + const tokenEndpointAuthMethod: string = body.token_endpoint_auth_method ?? 'none'; + if (!SUPPORTED_AUTH_METHODS.has(tokenEndpointAuthMethod)) { + return { + status: 400, + body: { + error: 'invalid_client_metadata', + error_description: `Unsupported token_endpoint_auth_method: ${tokenEndpointAuthMethod}`, + }, + }; + } + + const applicationType: string = body.application_type ?? 'web'; + if (applicationType !== 'web' && applicationType !== 'native') { + return { + status: 400, + body: { error: 'invalid_client_metadata', error_description: `Unsupported application_type: ${applicationType}` }, + }; + } + + const metadata: MCPClientMetadata = { + redirect_uris: body.redirect_uris, + client_name: body.client_name, + client_uri: body.client_uri, + logo_uri: body.logo_uri, + scope: body.scope, + contacts: body.contacts, + grant_types: grantTypes, + response_types: responseTypes, + token_endpoint_auth_method: tokenEndpointAuthMethod, + application_type: applicationType, + software_id: body.software_id, + software_version: body.software_version, + }; + + return { record: metadata }; +} + +/** + * Handle POST /oauth/mcp/register. RFC 7591 §3 returns 201 with the issued + * client_id (and client_secret for confidential clients) plus echoed metadata. + * + * @param request - HTTP request (only used for the Authorization header today) + * @param body - Parsed JSON request body + * @param mcpConfig - Plugin MCP configuration + * @param logger - Optional logger + */ +export async function handleRegister( + request: { headers?: { authorization?: string } } | undefined, + body: any, + mcpConfig: MCPConfig | undefined, + logger?: Logger +): Promise { + const dcrConfig = mcpConfig?.dynamicClientRegistration; + + // DCR defaults to enabled when MCP is enabled. Explicit `enabled: false` + // turns it off and the endpoint returns 404 (existence-hiding). + if (dcrConfig?.enabled === false) { + return { status: 404, body: { error: 'Not found' } }; + } + + const authHeader = request?.headers?.authorization; + const authError = checkInitialAccessToken(authHeader, dcrConfig?.initialAccessToken); + if (authError) { + return authError; + } + + const built = buildClientFromRequest(body, dcrConfig?.allowedRedirectUriHosts); + if ('status' in built) { + return built; + } + + const isConfidential = built.record.token_endpoint_auth_method !== 'none'; + const clientId = randomUUID(); + const clientIdIssuedAt = Math.floor(Date.now() / 1000); + + const record: MCPClientRecord = { + ...built.record, + client_id: clientId, + client_id_issued_at: clientIdIssuedAt, + }; + if (isConfidential) { + record.client_secret = randomBytes(32).toString('base64url'); + // 0 = never expires; we don't currently rotate client_secrets. + record.client_secret_expires_at = 0; + } + + const store = new MCPClientStore(logger); + try { + await store.set(record); + } catch (error) { + logger?.error?.('MCP client registration storage failed:', (error as Error).message); + return { + status: 500, + body: { error: 'server_error', error_description: 'Failed to persist client registration' }, + }; + } + + logger?.info?.( + `MCP client registered: ${clientId} (${isConfidential ? 'confidential' : 'public'}, ${record.redirect_uris.length} redirect URI(s))` + ); + + return { + status: 201, + body: record, + }; +} diff --git a/src/lib/mcp/index.ts b/src/lib/mcp/index.ts new file mode 100644 index 0000000..87df12a --- /dev/null +++ b/src/lib/mcp/index.ts @@ -0,0 +1,37 @@ +/** + * MCP OAuth Endpoint Dispatcher + * + * 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. + */ + +import type { Logger, MCPConfig, Request } from '../../types.ts'; +import { handleRegister } from './dcr.ts'; + +export { MCPClientStore, resetMCPClientsTableCache } from './clientStore.ts'; +export { handleRegister } from './dcr.ts'; + +/** + * Dispatch POST /oauth/mcp/. + * + * Returns 404 when MCP is disabled (existence-hiding — clients shouldn't + * be able to probe whether MCP support is configured). + */ +export async function handleMCPPost( + action: string, + request: Request, + body: any, + mcpConfig: MCPConfig | undefined, + logger?: Logger +): Promise { + if (!mcpConfig?.enabled) { + return { status: 404, body: { error: 'Not found' } }; + } + + if (action === 'register') { + return handleRegister(request, body, mcpConfig, logger); + } + + return { status: 404, body: { error: 'Not found' } }; +} diff --git a/src/lib/resource.ts b/src/lib/resource.ts index e0f5dbc..be3537e 100644 --- a/src/lib/resource.ts +++ b/src/lib/resource.ts @@ -6,8 +6,9 @@ import { Resource } from 'harper'; import type { RequestTarget } from 'harper'; -import type { Request, Logger, ProviderRegistry, OAuthProviderConfig } from '../types.ts'; +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 type { HookManager } from './hookManager.ts'; import type { DynamicProviderCache } from './dynamicProviderCache.ts'; @@ -34,6 +35,7 @@ export class OAuthResource extends Resource { static pluginDefaults: Partial = {}; static dynamicProviderCache: DynamicProviderCache | null = null; static logger: Logger | undefined = undefined; + static mcpConfig: MCPConfig | undefined = undefined; /** * Configure the OAuth resource with providers and settings @@ -45,7 +47,8 @@ export class OAuthResource extends Resource { hookManager: HookManager, pluginDefaults: Partial, logger?: Logger, - dynamicProviderCache?: DynamicProviderCache + dynamicProviderCache?: DynamicProviderCache, + mcpConfig?: MCPConfig ): void { OAuthResource.providers = providers; OAuthResource.debugMode = debugMode; @@ -53,6 +56,7 @@ export class OAuthResource extends Resource { OAuthResource.pluginDefaults = pluginDefaults; OAuthResource.logger = logger; OAuthResource.dynamicProviderCache = dynamicProviderCache ?? null; + OAuthResource.mcpConfig = mcpConfig; } /** @@ -375,13 +379,13 @@ export class OAuthResource extends Resource { * Handle POST requests to OAuth endpoints * Resource API v2 signature: post(target, data) */ - async post(target: RequestTarget, _data: any): Promise { + async post(target: RequestTarget, data: any): Promise { const logger = OAuthResource.logger; const hookManager = OAuthResource.hookManager!; // Parse the route const route = OAuthResource.parseRoute(target); - const { providerName } = route; + const { providerName, action } = route; // Get request from context (HarperDB provides the HTTP request here) const context = this.getContext(); @@ -399,6 +403,11 @@ export class OAuthResource extends Resource { return handleLogout(request, hookManager, logger); } + // Handle MCP endpoints (/oauth/mcp/) + if (providerName === 'mcp') { + return handleMCPPost(action, request, data, OAuthResource.mcpConfig, logger); + } + // All other POST endpoints are not supported return OAuthResource.notFoundResponse(); } @@ -429,5 +438,6 @@ export class OAuthResource extends Resource { OAuthResource.pluginDefaults = {}; OAuthResource.dynamicProviderCache = null; OAuthResource.logger = undefined; + OAuthResource.mcpConfig = undefined; } } diff --git a/src/types.ts b/src/types.ts index 9c07bb2..37f1d6d 100644 --- a/src/types.ts +++ b/src/types.ts @@ -32,6 +32,88 @@ export interface OAuthPluginConfig { hooks?: OAuthHooks; /** Cache providers resolved via onResolveProvider hook. true = forever (default), false = never, number = TTL in seconds */ cacheDynamicProviders?: boolean | number; + /** MCP OAuth flow configuration (RFC 9728 PRM, RFC 7591 DCR, RFC 8707 audience binding) */ + mcp?: MCPConfig; +} + +// ============================================================================ +// MCP OAuth Types (RFCs 7591, 8707, 9728; MCP authorization spec 2025-06-18) +// ============================================================================ + +/** + * MCP OAuth Configuration + * + * Opt-in configuration for serving the MCP authorization-server flow alongside + * the existing human-OAuth (relying party) flow. + */ +export interface MCPConfig { + /** Master switch for MCP OAuth endpoints */ + enabled?: boolean; + /** Dynamic Client Registration settings (RFC 7591) */ + dynamicClientRegistration?: MCPDynamicClientRegistrationConfig; +} + +/** + * Dynamic Client Registration configuration (RFC 7591) + * + * Defaults to enabled because Claude Desktop, Cursor, and mcp-remote all + * register at runtime with no pre-baked client_id. Restricting registration + * is opt-in via initialAccessToken or allowedRedirectUriHosts. + */ +export interface MCPDynamicClientRegistrationConfig { + /** Enable the /register endpoint. Default: true. */ + enabled?: boolean; + /** + * If set, registration requests must present `Authorization: Bearer ` + * matching this value. Default: open registration per RFC 7591. + */ + initialAccessToken?: string; + /** + * If set, redirect_uris hosts must match an entry in this list. localhost + * is always allowed for native clients per RFC 8252. Default: unrestricted. + */ + allowedRedirectUriHosts?: string[]; +} + +/** + * MCP client metadata (RFC 7591 §2) + * + * Request body shape for POST /oauth/mcp/register. Fields with defaults are + * optional in the request and populated by the registration handler. + */ +export interface MCPClientMetadata { + /** Required: array of allowed redirect URIs (exact-match validated on /authorize) */ + redirect_uris: string[]; + client_name?: string; + client_uri?: string; + logo_uri?: string; + scope?: string; + contacts?: string[]; + /** Default: ["authorization_code", "refresh_token"] */ + grant_types?: string[]; + /** Default: ["code"] */ + response_types?: string[]; + /** Default: "none" (public clients). Other values: "client_secret_basic", "client_secret_post". */ + token_endpoint_auth_method?: string; + /** "web" (default) or "native" */ + application_type?: string; + software_id?: string; + software_version?: string; +} + +/** + * MCP client record as returned from /register and stored in the + * harper_oauth_mcp_clients table. + */ +export interface MCPClientRecord extends MCPClientMetadata { + /** Server-issued client identifier */ + client_id: string; + /** Server-issued secret (only for confidential clients) */ + client_secret?: string; + /** Unix timestamp (seconds) when the client was registered */ + client_id_issued_at: number; + /** Unix timestamp (seconds) when client_secret expires; 0 = never */ + client_secret_expires_at?: number; } /** diff --git a/test/lib/config.test.js b/test/lib/config.test.js index c3374b4..4e3429c 100644 --- a/test/lib/config.test.js +++ b/test/lib/config.test.js @@ -9,6 +9,7 @@ import { extractPluginDefaults, initializeProviders, expandEnvVar, + expandEnvVarsDeep, } from '../../dist/lib/config.js'; describe('OAuth Configuration', () => { @@ -81,6 +82,42 @@ describe('OAuth Configuration', () => { }); }); + describe('expandEnvVarsDeep', () => { + it('expands string leaves on nested objects', () => { + process.env.TEST_TOKEN = 'secret-123'; + const input = { + enabled: true, + dynamicClientRegistration: { + initialAccessToken: '${TEST_TOKEN}', + allowedRedirectUriHosts: ['app.example.com', '${TEST_NOT_SET}'], + }, + }; + const result = expandEnvVarsDeep(input); + assert.equal(result.dynamicClientRegistration.initialAccessToken, 'secret-123'); + assert.equal(result.dynamicClientRegistration.allowedRedirectUriHosts[0], 'app.example.com'); + // Unset env vars retain their placeholder + assert.equal(result.dynamicClientRegistration.allowedRedirectUriHosts[1], '${TEST_NOT_SET}'); + }); + + it('passes non-string, non-object scalars through', () => { + const input = { a: 1, b: true, c: null }; + assert.deepEqual(expandEnvVarsDeep(input), input); + }); + + it('returns the input unchanged when there are no placeholders', () => { + const input = { foo: 'bar', baz: { qux: 'quux' } }; + assert.deepEqual(expandEnvVarsDeep(input), input); + }); + + it('does not mutate the input object', () => { + process.env.TEST_X = 'expanded'; + const input = { nested: { value: '${TEST_X}' } }; + const result = expandEnvVarsDeep(input); + assert.equal(input.nested.value, '${TEST_X}', 'input should be untouched'); + assert.equal(result.nested.value, 'expanded'); + }); + }); + describe('buildProviderConfig', () => { it('should build basic provider config', () => { const providerConfig = { diff --git a/test/lib/mcp/clientStore.test.js b/test/lib/mcp/clientStore.test.js new file mode 100644 index 0000000..36dd28e --- /dev/null +++ b/test/lib/mcp/clientStore.test.js @@ -0,0 +1,139 @@ +/** + * Tests for MCPClientStore + */ + +import { describe, it, before, after, beforeEach } from 'node:test'; +import assert from 'node:assert/strict'; +import { MCPClientStore, resetMCPClientsTableCache } from '../../../dist/lib/mcp/clientStore.js'; + +describe('MCPClientStore', () => { + let store; + let originalDatabases; + let storedRecords; + let mockTable; + + before(() => { + originalDatabases = global.databases; + }); + + after(() => { + global.databases = originalDatabases; + }); + + beforeEach(() => { + resetMCPClientsTableCache(); + store = new MCPClientStore(); + storedRecords = new Map(); + mockTable = { + get: async (id) => storedRecords.get(id) || null, + put: async (record) => { + storedRecords.set(record.client_id, record); + }, + delete: async (id) => { + storedRecords.delete(id); + }, + }; + global.databases = { + oauth: { + harper_oauth_mcp_clients: mockTable, + }, + }; + }); + + describe('CRUD', () => { + it('stores and retrieves a client, JSON-encoding array fields on write', async () => { + const record = { + client_id: 'abc-123', + client_id_issued_at: 1700000000, + redirect_uris: ['https://example.com/cb', 'https://example.com/cb2'], + client_name: 'Test Client', + grant_types: ['authorization_code', 'refresh_token'], + response_types: ['code'], + token_endpoint_auth_method: 'none', + application_type: 'web', + }; + + await store.set(record); + + const stored = storedRecords.get('abc-123'); + assert.ok(stored, 'record was persisted'); + // Array fields stored as JSON strings + assert.equal(typeof stored.redirect_uris, 'string'); + assert.deepEqual(JSON.parse(stored.redirect_uris), record.redirect_uris); + assert.equal(typeof stored.grant_types, 'string'); + // Scalar fields stored as-is + assert.equal(stored.client_name, 'Test Client'); + + const retrieved = await store.get('abc-123'); + assert.ok(retrieved); + assert.equal(retrieved.client_id, 'abc-123'); + assert.deepEqual(retrieved.redirect_uris, record.redirect_uris); + assert.deepEqual(retrieved.grant_types, record.grant_types); + }); + + it('returns null for unknown client_id', async () => { + const result = await store.get('does-not-exist'); + assert.equal(result, null); + }); + + it('deletes a client', async () => { + await store.set({ + client_id: 'to-delete', + client_id_issued_at: 1700000000, + redirect_uris: ['https://example.com/cb'], + }); + assert.equal(storedRecords.size, 1); + + await store.delete('to-delete'); + assert.equal(storedRecords.size, 0); + }); + + it('handles malformed array JSON gracefully (returns undefined for that field)', async () => { + // Simulate corrupted record stored outside our encoder + storedRecords.set('corrupted', { + client_id: 'corrupted', + client_id_issued_at: 1700000000, + redirect_uris: 'not-json-{', + }); + + const retrieved = await store.get('corrupted'); + assert.ok(retrieved); + assert.equal(retrieved.client_id, 'corrupted'); + assert.equal(retrieved.redirect_uris, undefined); + }); + + it('omits array fields that were undefined on the record', async () => { + await store.set({ + client_id: 'minimal', + client_id_issued_at: 1700000000, + redirect_uris: ['https://example.com/cb'], + // contacts, grant_types, response_types intentionally omitted + }); + + const stored = storedRecords.get('minimal'); + assert.equal(stored.contacts, undefined); + assert.equal(stored.grant_types, undefined); + }); + + it('returns null when the underlying get call throws', async () => { + mockTable.get = async () => { + throw new Error('db read failure'); + }; + const result = await store.get('whatever'); + assert.equal(result, null); + }); + + it('propagates errors on set() so callers can fail registration with 500', async () => { + mockTable.put = async () => { + throw new Error('db write failure'); + }; + await assert.rejects(() => + store.set({ + client_id: 'will-fail', + client_id_issued_at: 1700000000, + redirect_uris: ['https://example.com/cb'], + }) + ); + }); + }); +}); diff --git a/test/lib/mcp/dcr.test.js b/test/lib/mcp/dcr.test.js new file mode 100644 index 0000000..4a00221 --- /dev/null +++ b/test/lib/mcp/dcr.test.js @@ -0,0 +1,362 @@ +/** + * Tests for MCP Dynamic Client Registration handler (RFC 7591) + */ + +import { describe, it, before, after, beforeEach } from 'node:test'; +import assert from 'node:assert/strict'; +import { handleRegister } from '../../../dist/lib/mcp/dcr.js'; +import { resetMCPClientsTableCache } from '../../../dist/lib/mcp/clientStore.js'; + +const VALID_BODY = { + redirect_uris: ['https://app.example.com/cb'], + client_name: 'Test MCP Client', +}; + +function makeRequest(headers = {}) { + return { headers }; +} + +describe('handleRegister (RFC 7591 DCR)', () => { + let originalDatabases; + let storedRecords; + + before(() => { + originalDatabases = global.databases; + }); + + after(() => { + global.databases = originalDatabases; + }); + + beforeEach(() => { + resetMCPClientsTableCache(); + storedRecords = new Map(); + global.databases = { + oauth: { + harper_oauth_mcp_clients: { + get: async (id) => storedRecords.get(id) || null, + put: async (record) => { + storedRecords.set(record.client_id, record); + }, + delete: async (id) => { + storedRecords.delete(id); + }, + }, + }, + }; + }); + + describe('DCR enable/disable gating', () => { + it('returns 404 when DCR is explicitly disabled', async () => { + const response = await handleRegister(makeRequest(), VALID_BODY, { + enabled: true, + dynamicClientRegistration: { enabled: false }, + }); + assert.equal(response.status, 404); + assert.equal(storedRecords.size, 0); + }); + + it('proceeds when DCR config is absent (defaults to enabled)', async () => { + const response = await handleRegister(makeRequest(), VALID_BODY, { enabled: true }); + assert.equal(response.status, 201); + }); + + it('proceeds when DCR is explicitly enabled', async () => { + const response = await handleRegister(makeRequest(), VALID_BODY, { + enabled: true, + dynamicClientRegistration: { enabled: true }, + }); + assert.equal(response.status, 201); + }); + }); + + describe('initial access token gate', () => { + const config = { + enabled: true, + dynamicClientRegistration: { initialAccessToken: 'secret-token' }, + }; + + it('returns 401 when initial access token is configured and Authorization header is missing', async () => { + const response = await handleRegister(makeRequest(), VALID_BODY, config); + assert.equal(response.status, 401); + assert.equal(response.body.error, 'invalid_token'); + }); + + it('returns 401 when Authorization header does not start with Bearer', async () => { + const response = await handleRegister(makeRequest({ authorization: 'Basic dXNlcjpwYXNz' }), VALID_BODY, config); + assert.equal(response.status, 401); + }); + + it('returns 401 when Bearer token does not match', async () => { + const response = await handleRegister(makeRequest({ authorization: 'Bearer wrong-token' }), VALID_BODY, config); + assert.equal(response.status, 401); + assert.equal(response.body.error, 'invalid_token'); + }); + + it('accepts a matching Bearer token', async () => { + const response = await handleRegister(makeRequest({ authorization: 'Bearer secret-token' }), VALID_BODY, config); + assert.equal(response.status, 201); + }); + + it('rejects the capitalized Authorization header (Node HTTP parser lowercases)', async () => { + // Production: incoming headers are lowercased before reaching us. + // If a caller hands us a literal { Authorization: ... } object, we + // treat it as "no token presented" — matching the production contract. + const response = await handleRegister(makeRequest({ Authorization: 'Bearer secret-token' }), VALID_BODY, config); + assert.equal(response.status, 401); + }); + }); + + describe('scalar metadata validation', () => { + const config = { enabled: true }; + + it('rejects non-string client_name', async () => { + const response = await handleRegister( + makeRequest(), + { redirect_uris: ['https://app.example.com/cb'], client_name: { evil: 'object' } }, + config + ); + assert.equal(response.status, 400); + assert.equal(response.body.error, 'invalid_client_metadata'); + }); + + it('rejects non-string logo_uri', async () => { + const response = await handleRegister( + makeRequest(), + { redirect_uris: ['https://app.example.com/cb'], logo_uri: ['/icons/a.png'] }, + config + ); + assert.equal(response.status, 400); + }); + + it('rejects non-string software_version', async () => { + const response = await handleRegister( + makeRequest(), + { redirect_uris: ['https://app.example.com/cb'], software_version: 1.2 }, + config + ); + assert.equal(response.status, 400); + }); + }); + + describe('redirect_uris validation', () => { + const config = { enabled: true }; + + it('rejects missing redirect_uris', async () => { + const response = await handleRegister(makeRequest(), { client_name: 'X' }, config); + assert.equal(response.status, 400); + assert.equal(response.body.error, 'invalid_redirect_uri'); + }); + + it('rejects empty redirect_uris array', async () => { + const response = await handleRegister(makeRequest(), { redirect_uris: [] }, config); + assert.equal(response.status, 400); + assert.equal(response.body.error, 'invalid_redirect_uri'); + }); + + it('rejects non-array redirect_uris', async () => { + const response = await handleRegister(makeRequest(), { redirect_uris: 'https://example.com/cb' }, config); + assert.equal(response.status, 400); + assert.equal(response.body.error, 'invalid_redirect_uri'); + }); + + it('rejects http URIs to non-loopback hosts', async () => { + const response = await handleRegister( + makeRequest(), + { redirect_uris: ['http://attacker.example.com/cb'] }, + config + ); + assert.equal(response.status, 400); + assert.equal(response.body.error, 'invalid_redirect_uri'); + }); + + it('accepts http URIs to localhost (RFC 8252 §8.3)', async () => { + const response = await handleRegister( + makeRequest(), + { redirect_uris: ['http://localhost:6274/oauth/callback'] }, + config + ); + assert.equal(response.status, 201); + }); + + it('accepts http URIs to 127.0.0.1', async () => { + const response = await handleRegister( + makeRequest(), + { redirect_uris: ['http://127.0.0.1:6274/oauth/callback'] }, + config + ); + assert.equal(response.status, 201); + }); + + it('rejects URIs with a fragment', async () => { + const response = await handleRegister( + makeRequest(), + { redirect_uris: ['https://app.example.com/cb#foo'] }, + config + ); + assert.equal(response.status, 400); + assert.equal(response.body.error, 'invalid_redirect_uri'); + }); + + it('rejects malformed URIs', async () => { + const response = await handleRegister(makeRequest(), { redirect_uris: ['not-a-url'] }, config); + assert.equal(response.status, 400); + }); + + it('enforces allowedRedirectUriHosts when configured', async () => { + const allowlistConfig = { + enabled: true, + dynamicClientRegistration: { allowedRedirectUriHosts: ['app.example.com'] }, + }; + const allowed = await handleRegister( + makeRequest(), + { redirect_uris: ['https://app.example.com/cb'] }, + allowlistConfig + ); + assert.equal(allowed.status, 201); + + const denied = await handleRegister( + makeRequest(), + { redirect_uris: ['https://other.example.com/cb'] }, + allowlistConfig + ); + assert.equal(denied.status, 400); + assert.equal(denied.body.error, 'invalid_redirect_uri'); + }); + + it('always allows localhost even with allowedRedirectUriHosts set', async () => { + const response = await handleRegister( + makeRequest(), + { redirect_uris: ['http://localhost:6274/cb'] }, + { + enabled: true, + dynamicClientRegistration: { allowedRedirectUriHosts: ['app.example.com'] }, + } + ); + assert.equal(response.status, 201); + }); + }); + + describe('defaults and metadata', () => { + it('applies MCP-context defaults (public client, code flow)', async () => { + const response = await handleRegister(makeRequest(), VALID_BODY, { enabled: true }); + assert.equal(response.status, 201); + assert.equal(response.body.token_endpoint_auth_method, 'none'); + assert.deepEqual(response.body.grant_types, ['authorization_code', 'refresh_token']); + assert.deepEqual(response.body.response_types, ['code']); + assert.equal(response.body.application_type, 'web'); + }); + + it('preserves client-specified values over defaults', async () => { + const response = await handleRegister( + makeRequest(), + { + redirect_uris: ['https://app.example.com/cb'], + grant_types: ['authorization_code'], + application_type: 'native', + }, + { enabled: true } + ); + assert.equal(response.status, 201); + assert.deepEqual(response.body.grant_types, ['authorization_code']); + assert.equal(response.body.application_type, 'native'); + }); + + it('rejects unsupported grant_types', async () => { + const response = await handleRegister( + makeRequest(), + { redirect_uris: ['https://app.example.com/cb'], grant_types: ['client_credentials'] }, + { enabled: true } + ); + assert.equal(response.status, 400); + assert.equal(response.body.error, 'invalid_client_metadata'); + }); + + it('rejects unsupported response_types', async () => { + const response = await handleRegister( + makeRequest(), + { redirect_uris: ['https://app.example.com/cb'], response_types: ['token'] }, + { enabled: true } + ); + assert.equal(response.status, 400); + }); + + it('rejects unsupported token_endpoint_auth_method', async () => { + const response = await handleRegister( + makeRequest(), + { redirect_uris: ['https://app.example.com/cb'], token_endpoint_auth_method: 'private_key_jwt' }, + { enabled: true } + ); + assert.equal(response.status, 400); + }); + + it('rejects unsupported application_type', async () => { + const response = await handleRegister( + makeRequest(), + { redirect_uris: ['https://app.example.com/cb'], application_type: 'service' }, + { enabled: true } + ); + assert.equal(response.status, 400); + }); + + it('rejects non-string entries in string-array fields', async () => { + const response = await handleRegister( + makeRequest(), + { redirect_uris: ['https://app.example.com/cb'], contacts: ['nathan@example.com', 42] }, + { enabled: true } + ); + assert.equal(response.status, 400); + }); + }); + + describe('issued credentials', () => { + it('issues a client_id and persists the record (public client, no secret)', async () => { + const response = await handleRegister(makeRequest(), VALID_BODY, { enabled: true }); + assert.equal(response.status, 201); + assert.ok(response.body.client_id, 'client_id was issued'); + assert.equal(response.body.client_secret, undefined, 'public clients receive no secret'); + assert.equal(typeof response.body.client_id_issued_at, 'number'); + + const stored = storedRecords.get(response.body.client_id); + assert.ok(stored, 'client was persisted'); + }); + + it('issues a client_secret for confidential clients (client_secret_basic)', async () => { + const response = await handleRegister( + makeRequest(), + { + redirect_uris: ['https://app.example.com/cb'], + token_endpoint_auth_method: 'client_secret_basic', + }, + { enabled: true } + ); + assert.equal(response.status, 201); + assert.ok(response.body.client_secret, 'confidential client received a secret'); + assert.equal(response.body.client_secret_expires_at, 0, '0 == never expires'); + }); + + it('issues unique client_ids for repeated registrations of the same metadata', async () => { + const a = await handleRegister(makeRequest(), VALID_BODY, { enabled: true }); + const b = await handleRegister(makeRequest(), VALID_BODY, { enabled: true }); + assert.notEqual(a.body.client_id, b.body.client_id); + assert.equal(storedRecords.size, 2); + }); + }); + + describe('error handling', () => { + it('rejects non-object bodies', async () => { + const response = await handleRegister(makeRequest(), null, { enabled: true }); + assert.equal(response.status, 400); + assert.equal(response.body.error, 'invalid_client_metadata'); + }); + + it('returns 500 when storage fails', async () => { + global.databases.oauth.harper_oauth_mcp_clients.put = async () => { + throw new Error('storage failure'); + }; + const response = await handleRegister(makeRequest(), VALID_BODY, { enabled: true }); + assert.equal(response.status, 500); + assert.equal(response.body.error, 'server_error'); + }); + }); +}); From acc51471b48e72ccf49e8bb67f7d125d509ef800 Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Thu, 21 May 2026 05:30:45 -0700 Subject: [PATCH 2/3] Address bot review feedback on PR #89 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three blockers from Claude and Gemini reviewers, all real: 1. **Constant-time initialAccessToken compare** (Claude). Replaced `presented !== configured` with `timingSafeEqual` (length-checked first, since timingSafeEqual requires equal-length buffers). A precise-latency attacker could otherwise progressively recover the token character-by-character. 2. **Spread on Harper tracked object dropped scalar fields** (Gemini — the exact gotcha called out in CLAUDE.md). decodeRecord used `{ ...raw }`, which silently produces `{}` against a GenericTrackedObject Proxy (own-keys is empty), so client_id and every other scalar field would have been lost on read in production. Rewrote encodeRecord and decodeRecord to use explicit field access for the known MCPClientRecord shape. Mock in clientStore.test.js now wraps stored rows in a tracked-object Proxy so this can't regress; added an explicit "preserves scalar fields" assertion. 3. **handleMCPPost dispatcher untested** (Claude). The master enable gate, action routing, and unknown-action 404 all bypassed the handleRegister-direct tests. Added test/lib/mcp/index.test.js covering all three (and the layered deny when MCP is enabled but DCR is explicitly off). 525 unit tests pass locally (+9 net from these fixes). Co-Authored-By: Claude Opus 4.7 (1M context) --- src/lib/mcp/clientStore.ts | 89 +++++++++++++++++------- src/lib/mcp/dcr.ts | 9 ++- test/lib/mcp/clientStore.test.js | 47 ++++++++++++- test/lib/mcp/index.test.js | 112 +++++++++++++++++++++++++++++++ 4 files changed, 229 insertions(+), 28 deletions(-) create mode 100644 test/lib/mcp/index.test.js diff --git a/src/lib/mcp/clientStore.ts b/src/lib/mcp/clientStore.ts index 4dab929..d6dfeba 100644 --- a/src/lib/mcp/clientStore.ts +++ b/src/lib/mcp/clientStore.ts @@ -39,36 +39,75 @@ export function resetMCPClientsTableCache(): void { clientsTable = undefined; } -const ARRAY_FIELDS = ['redirect_uris', 'contacts', 'grant_types', 'response_types'] as const; +function serializeArrayField(value: unknown): string | undefined { + return value === undefined ? undefined : JSON.stringify(value); +} -function encodeRecord(record: MCPClientRecord): Record { - const encoded: Record = { ...record }; - for (const field of ARRAY_FIELDS) { - const value = record[field]; - encoded[field] = value === undefined ? undefined : JSON.stringify(value); +function parseArrayField(value: unknown): string[] | undefined { + if (typeof value !== 'string') { + // null, undefined, or anything else from the DB collapses to undefined. + return undefined; + } + try { + const parsed = JSON.parse(value); + return Array.isArray(parsed) ? parsed : undefined; + } catch { + // Malformed stored data — treat as absent rather than crashing callers. + return undefined; } - return encoded; } +/** + * Encode the record for storage. Explicit field access (no spread) so we + * write a well-typed record even if a caller hands us a tracked object — + * per CLAUDE.md's "GenericTrackedObject + spread" gotcha. + */ +function encodeRecord(record: MCPClientRecord): Record { + return { + client_id: record.client_id, + client_secret: record.client_secret, + client_name: record.client_name, + client_uri: record.client_uri, + logo_uri: record.logo_uri, + scope: record.scope, + token_endpoint_auth_method: record.token_endpoint_auth_method, + application_type: record.application_type, + software_id: record.software_id, + software_version: record.software_version, + client_id_issued_at: record.client_id_issued_at, + client_secret_expires_at: record.client_secret_expires_at, + redirect_uris: serializeArrayField(record.redirect_uris), + contacts: serializeArrayField(record.contacts), + grant_types: serializeArrayField(record.grant_types), + response_types: serializeArrayField(record.response_types), + }; +} + +/** + * Decode a stored row. Must use explicit property access — Harper returns + * GenericTrackedObject Proxies whose own-keys are empty, so { ...raw } drops + * every scalar field (client_id, client_secret, …) and breaks retrieval. + * Caught by Gemini review on PR #89; documented in CLAUDE.md. + */ function decodeRecord(raw: Record): MCPClientRecord { - const decoded: Record = { ...raw }; - for (const field of ARRAY_FIELDS) { - const value = raw[field]; - if (typeof value === 'string') { - try { - const parsed = JSON.parse(value); - decoded[field] = Array.isArray(parsed) ? parsed : undefined; - } catch { - // Malformed stored data — treat as absent to avoid crashing callers. - decoded[field] = undefined; - } - } else if (value === null) { - // Normalize null (e.g. from a DB column default) to undefined - // so the decoded record matches the `string[] | undefined` type. - decoded[field] = undefined; - } - } - return decoded as MCPClientRecord; + return { + client_id: raw.client_id, + client_secret: raw.client_secret, + client_name: raw.client_name, + client_uri: raw.client_uri, + logo_uri: raw.logo_uri, + scope: raw.scope, + token_endpoint_auth_method: raw.token_endpoint_auth_method, + application_type: raw.application_type, + software_id: raw.software_id, + software_version: raw.software_version, + client_id_issued_at: raw.client_id_issued_at, + client_secret_expires_at: raw.client_secret_expires_at, + redirect_uris: parseArrayField(raw.redirect_uris) as string[], + contacts: parseArrayField(raw.contacts), + grant_types: parseArrayField(raw.grant_types), + response_types: parseArrayField(raw.response_types), + }; } export class MCPClientStore { diff --git a/src/lib/mcp/dcr.ts b/src/lib/mcp/dcr.ts index ead42f2..c5a3b68 100644 --- a/src/lib/mcp/dcr.ts +++ b/src/lib/mcp/dcr.ts @@ -10,7 +10,7 @@ * grants, response_type=code. Confidential clients can opt in explicitly. */ -import { randomUUID, randomBytes } from 'node:crypto'; +import { randomUUID, randomBytes, timingSafeEqual } from 'node:crypto'; import type { Logger, MCPClientMetadata, MCPClientRecord, MCPConfig } from '../../types.ts'; import { MCPClientStore } from './clientStore.ts'; @@ -40,7 +40,12 @@ function checkInitialAccessToken(authHeader: string | undefined, configured: str }; } const presented = authHeader.slice('Bearer '.length).trim(); - if (presented !== configured) { + // Constant-time comparison — `!==` leaks per-character timing and lets a + // precise-latency attacker progressively recover initialAccessToken. + // timingSafeEqual requires equal-length buffers, so length-check first. + const presentedBuf = Buffer.from(presented); + const configuredBuf = Buffer.from(configured); + if (presentedBuf.length !== configuredBuf.length || !timingSafeEqual(presentedBuf, configuredBuf)) { return { status: 401, body: { error: 'invalid_token', error_description: 'Invalid initial access token' }, diff --git a/test/lib/mcp/clientStore.test.js b/test/lib/mcp/clientStore.test.js index 36dd28e..c20a8eb 100644 --- a/test/lib/mcp/clientStore.test.js +++ b/test/lib/mcp/clientStore.test.js @@ -6,6 +6,23 @@ import { describe, it, before, after, beforeEach } from 'node:test'; import assert from 'node:assert/strict'; import { MCPClientStore, resetMCPClientsTableCache } from '../../../dist/lib/mcp/clientStore.js'; +/** + * Simulate Harper's GenericTrackedObject: property access works, but + * own-keys / spread / Object.keys see the object as empty. Wrapping + * stored records in this Proxy in tests ensures decodeRecord can never + * regress to relying on { ...raw } and silently dropping scalar fields. + */ +function asTrackedObject(plain) { + return new Proxy(plain, { + ownKeys() { + return []; + }, + getOwnPropertyDescriptor() { + return undefined; + }, + }); +} + describe('MCPClientStore', () => { let store; let originalDatabases; @@ -25,7 +42,12 @@ describe('MCPClientStore', () => { store = new MCPClientStore(); storedRecords = new Map(); mockTable = { - get: async (id) => storedRecords.get(id) || null, + // Wrap returned rows in a tracked-object Proxy so spread / Object.keys + // see them as empty (matches production GenericTrackedObject behavior). + get: async (id) => { + const raw = storedRecords.get(id); + return raw ? asTrackedObject(raw) : null; + }, put: async (record) => { storedRecords.set(record.client_id, record); }, @@ -115,6 +137,29 @@ describe('MCPClientStore', () => { assert.equal(stored.grant_types, undefined); }); + it('preserves scalar fields when the table returns a tracked-object Proxy', async () => { + // Regression guard for the spread-on-tracked-object bug: + // production rows return as Proxies whose own-keys are [], so + // { ...raw } would drop client_id and every other scalar. + await store.set({ + client_id: 'proxy-test', + client_secret: 'secret-abc', + client_id_issued_at: 1700000000, + client_secret_expires_at: 0, + redirect_uris: ['https://example.com/cb'], + client_name: 'Proxy Test', + token_endpoint_auth_method: 'client_secret_basic', + }); + + const retrieved = await store.get('proxy-test'); + assert.equal(retrieved.client_id, 'proxy-test'); + assert.equal(retrieved.client_secret, 'secret-abc'); + assert.equal(retrieved.client_name, 'Proxy Test'); + assert.equal(retrieved.token_endpoint_auth_method, 'client_secret_basic'); + assert.equal(retrieved.client_id_issued_at, 1700000000); + assert.deepEqual(retrieved.redirect_uris, ['https://example.com/cb']); + }); + it('returns null when the underlying get call throws', async () => { mockTable.get = async () => { throw new Error('db read failure'); diff --git a/test/lib/mcp/index.test.js b/test/lib/mcp/index.test.js new file mode 100644 index 0000000..5572f3b --- /dev/null +++ b/test/lib/mcp/index.test.js @@ -0,0 +1,112 @@ +/** + * Tests for the MCP POST dispatcher (src/lib/mcp/index.ts:handleMCPPost). + * + * Covers the deny paths and action routing that handleRegister-direct tests + * intentionally skip: the master mcpConfig.enabled gate, the action map, and + * the unknown-action fallthrough. + */ + +import { describe, it, before, after, beforeEach } from 'node:test'; +import assert from 'node:assert/strict'; +import { handleMCPPost } from '../../../dist/lib/mcp/index.js'; +import { resetMCPClientsTableCache } from '../../../dist/lib/mcp/clientStore.js'; + +const VALID_BODY = { + redirect_uris: ['https://app.example.com/cb'], + client_name: 'Test MCP Client', +}; + +function makeRequest(headers = {}) { + return { headers }; +} + +describe('handleMCPPost (dispatcher)', () => { + let originalDatabases; + let storedRecords; + + before(() => { + originalDatabases = global.databases; + }); + + after(() => { + global.databases = originalDatabases; + }); + + beforeEach(() => { + resetMCPClientsTableCache(); + storedRecords = new Map(); + global.databases = { + oauth: { + harper_oauth_mcp_clients: { + get: async (id) => storedRecords.get(id) || null, + put: async (record) => { + storedRecords.set(record.client_id, record); + }, + delete: async (id) => { + storedRecords.delete(id); + }, + }, + }, + }; + }); + + describe('master enable gate', () => { + it('returns 404 when mcpConfig is undefined and never persists', async () => { + const response = await handleMCPPost('register', makeRequest(), VALID_BODY, undefined); + assert.equal(response.status, 404); + assert.equal(storedRecords.size, 0, 'protected action must not run on deny path'); + }); + + it('returns 404 when mcpConfig.enabled is false', async () => { + const response = await handleMCPPost('register', makeRequest(), VALID_BODY, { enabled: false }); + assert.equal(response.status, 404); + assert.equal(storedRecords.size, 0); + }); + + it('returns 404 when mcpConfig.enabled is undefined', async () => { + const response = await handleMCPPost('register', makeRequest(), VALID_BODY, { dynamicClientRegistration: {} }); + assert.equal(response.status, 404); + assert.equal(storedRecords.size, 0); + }); + + it('proceeds when mcpConfig.enabled is true', async () => { + const response = await handleMCPPost('register', makeRequest(), VALID_BODY, { enabled: true }); + assert.equal(response.status, 201); + assert.equal(storedRecords.size, 1); + }); + }); + + describe('action routing', () => { + it('dispatches `register` to handleRegister', async () => { + const response = await handleMCPPost('register', makeRequest(), VALID_BODY, { enabled: true }); + assert.equal(response.status, 201); + assert.ok(response.body.client_id, 'register response carries an issued client_id'); + }); + + it('returns 404 for an unknown action even when MCP is enabled', async () => { + const response = await handleMCPPost('not-a-thing', makeRequest(), VALID_BODY, { enabled: true }); + assert.equal(response.status, 404); + assert.equal(storedRecords.size, 0); + }); + + it('returns 404 for an empty action', async () => { + const response = await handleMCPPost('', makeRequest(), VALID_BODY, { enabled: true }); + assert.equal(response.status, 404); + assert.equal(storedRecords.size, 0); + }); + }); + + describe('layered deny: DCR sub-gate', () => { + it('returns 404 when MCP is enabled but DCR is explicitly disabled', async () => { + // The master gate passes (mcp.enabled=true) but handleRegister's own + // gate (dynamicClientRegistration.enabled=false) trips. Either deny + // path is acceptable — the contract is no registration happens. + const response = await handleMCPPost('register', makeRequest(), VALID_BODY, { + enabled: true, + dynamicClientRegistration: { enabled: false }, + }); + assert.equal(response.status, 404); + assert.equal(storedRecords.size, 0); + }); + }); +}); From f1185e85ce2cbc75cc64e3f6409030517bb1435b Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Thu, 21 May 2026 07:00:41 -0700 Subject: [PATCH 3/3] Docs: surface MCP DCR schema and initialAccessToken security gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Gemini's second-round review on PR #89 flagged that admins reading the README post-merge have no way to discover that the new /oauth/mcp/register endpoint defaults to OPEN registration per RFC 7591, nor how to lock it down. Real security-visibility gap. Adding the security-relevant surface only: - README "Database Schema" section: mention harper_oauth_mcp_clients - README "Security Considerations": call out open-by-default DCR and the two ways to gate it (initialAccessToken, allowedRedirectUriHosts) - docs/configuration.md: new "MCP OAuth (work in progress)" subsection with the mcp config block, sub-fields, env expansion note, and a prominent in-line warning about open registration Full MCP docs (quickstart, /authorize, /token, withMCPAuth wrapper) stay deferred to Stage 8 of issue #86 — meaningful only once those endpoints land in subsequent PRs. Co-Authored-By: Claude Opus 4.7 (1M context) --- README.md | 3 +++ docs/configuration.md | 27 +++++++++++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/README.md b/README.md index 903a8af..890ea72 100644 --- a/README.md +++ b/README.md @@ -186,6 +186,8 @@ type CSRFToken @table { Tokens automatically expire after 10 minutes. +When MCP OAuth is enabled (see [issue #86](https://github.com/HarperFast/oauth/issues/86)), the plugin also creates a `harper_oauth_mcp_clients` table for RFC 7591 Dynamic Client Registration. Registrations persist indefinitely so `client_id`s cached by MCP clients (Claude Desktop, Cursor, `mcp-remote`) survive Harper restarts. + ## Security Considerations - **HTTPS required** - OAuth requires HTTPS in production @@ -193,6 +195,7 @@ Tokens automatically expire after 10 minutes. - **ID token verification** - OIDC providers verify token signatures - **Secure sessions** - Use Harper's secure session configuration - **Token storage** - Tokens stored in session (configure secure cookies) +- **MCP client registration (when `mcp.enabled` is true)** - The `/oauth/mcp/register` endpoint defaults to **open registration** per RFC 7591. Set `mcp.dynamicClientRegistration.initialAccessToken` to require a bearer token on registration, or `mcp.dynamicClientRegistration.allowedRedirectUriHosts` to restrict which hosts may register `redirect_uri`s. See [`docs/configuration.md`](./docs/configuration.md). ## Debug Mode diff --git a/docs/configuration.md b/docs/configuration.md index 6070cc2..df37f79 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -26,6 +26,7 @@ Complete configuration options for the `@harperfast/oauth` plugin. | `redirectUri` | string | (auto-gen) | OAuth callback URL where providers redirect back to | | `postLoginRedirect` | string | `/` | Default URL to redirect users after successful OAuth login | | `cacheDynamicProviders` | boolean \| number | `true` | Cache providers resolved via `onResolveProvider` hook. `true` = forever, `false` = never, number = TTL in seconds | +| `mcp` | object | (off) | MCP OAuth flow configuration. See [MCP OAuth](#mcp-oauth-work-in-progress) below | ### Provider Configuration @@ -54,6 +55,32 @@ Each provider requires: - `userInfoUrl` - User info endpoint URL (required) - `jwksUrl` - JWKS endpoint URL (required for ID token verification) +### MCP OAuth (work in progress) + +Opt-in support for the Model Context Protocol authorization flow ([issue #86](https://github.com/HarperFast/oauth/issues/86)). The first stage adds Dynamic Client Registration at `POST /oauth/mcp/register` (RFC 7591) so MCP clients (Claude Desktop, Cursor, `mcp-remote`) can register themselves at runtime. `/authorize`, `/token`, and the `withMCPAuth` wrapper land in subsequent releases. + +```yaml +'@harperfast/oauth': + mcp: + enabled: true + dynamicClientRegistration: + # Optional: require Authorization: Bearer on /register. + # Without this, registration is OPEN per RFC 7591 — anyone can register. + initialAccessToken: ${OAUTH_MCP_REGISTRATION_TOKEN} + # Optional: restrict redirect_uri hosts (localhost always allowed). + allowedRedirectUriHosts: + - app.example.com +``` + +| Option | Type | Default | Description | +| ------------------------------------------------------- | -------- | ------- | ------------------------------------------------------------------------------------------ | +| `mcp.enabled` | boolean | `false` | Master switch for the MCP OAuth endpoints | +| `mcp.dynamicClientRegistration.enabled` | boolean | `true` | Enable the `/register` endpoint when `mcp.enabled` is true | +| `mcp.dynamicClientRegistration.initialAccessToken` | string | (none) | If set, registration requires `Authorization: Bearer `. Otherwise open per RFC 7591 | +| `mcp.dynamicClientRegistration.allowedRedirectUriHosts` | string[] | (none) | Allowlist for redirect_uri hosts. Localhost always allowed per RFC 8252 | + +Sensitive leaves inside `mcp` support `${ENV_VAR}` expansion (e.g., `initialAccessToken: ${OAUTH_MCP_REGISTRATION_TOKEN}`), the same way provider credentials do. + ## Environment Variables All configuration options can be set via environment variables: