From 7a9a99d2a033b826cbbd4048e53e13f672cd615b Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sun, 14 Sep 2025 18:55:32 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=97=20refactor:=20URL=20sanitization?= =?UTF-8?q?=20for=20MCP=20logging=20(#9632)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- packages/api/src/mcp/MCPConnectionFactory.ts | 5 +- packages/api/src/mcp/MCPServersRegistry.ts | 3 +- packages/api/src/mcp/connection.ts | 23 +++--- packages/api/src/mcp/oauth/handler.ts | 78 ++++++++++++++------ packages/api/src/mcp/utils.ts | 14 ++++ 5 files changed, 88 insertions(+), 35 deletions(-) diff --git a/packages/api/src/mcp/MCPConnectionFactory.ts b/packages/api/src/mcp/MCPConnectionFactory.ts index 3481613632..6785de748f 100644 --- a/packages/api/src/mcp/MCPConnectionFactory.ts +++ b/packages/api/src/mcp/MCPConnectionFactory.ts @@ -6,6 +6,7 @@ import type { FlowStateManager } from '~/flow/manager'; import type { FlowMetadata } from '~/flow/types'; import type * as t from './types'; import { MCPTokenStorage, MCPOAuthHandler } from '~/mcp/oauth'; +import { sanitizeUrlForLogging } from './utils'; import { MCPConnection } from './connection'; import { processMCPEnv } from '~/utils'; @@ -308,7 +309,9 @@ export class MCPConnectionFactory { metadata?: OAuthMetadata; } | null> { const serverUrl = (this.serverConfig as t.SSEOptions | t.StreamableHTTPOptions).url; - logger.debug(`${this.logPrefix} \`handleOAuthRequired\` called with serverUrl: ${serverUrl}`); + logger.debug( + `${this.logPrefix} \`handleOAuthRequired\` called with serverUrl: ${serverUrl ? sanitizeUrlForLogging(serverUrl) : 'undefined'}`, + ); if (!this.flowManager || !serverUrl) { logger.error( diff --git a/packages/api/src/mcp/MCPServersRegistry.ts b/packages/api/src/mcp/MCPServersRegistry.ts index cc913e2229..d5ecd990bd 100644 --- a/packages/api/src/mcp/MCPServersRegistry.ts +++ b/packages/api/src/mcp/MCPServersRegistry.ts @@ -7,6 +7,7 @@ import type { JsonSchemaType } from '~/types'; import type * as t from '~/mcp/types'; import { ConnectionsRepository } from '~/mcp/ConnectionsRepository'; import { detectOAuthRequirement } from '~/mcp/oauth'; +import { sanitizeUrlForLogging } from '~/mcp/utils'; import { processMCPEnv } from '~/utils'; import { CONSTANTS } from '~/mcp/enum'; @@ -183,7 +184,7 @@ export class MCPServersRegistry { const prefix = this.prefix(serverName); const config = this.parsedConfigs[serverName]; logger.info(`${prefix} -------------------------------------------------┐`); - logger.info(`${prefix} URL: ${config.url}`); + logger.info(`${prefix} URL: ${config.url ? sanitizeUrlForLogging(config.url) : 'N/A'}`); logger.info(`${prefix} OAuth Required: ${config.requiresOAuth}`); logger.info(`${prefix} Capabilities: ${config.capabilities}`); logger.info(`${prefix} Tools: ${config.tools}`); diff --git a/packages/api/src/mcp/connection.ts b/packages/api/src/mcp/connection.ts index d4d56ceb83..ad1b3e32aa 100644 --- a/packages/api/src/mcp/connection.ts +++ b/packages/api/src/mcp/connection.ts @@ -1,15 +1,15 @@ import { EventEmitter } from 'events'; +import { logger } from '@librechat/data-schemas'; import { fetch as undiciFetch, Agent } from 'undici'; import { StdioClientTransport, getDefaultEnvironment, } from '@modelcontextprotocol/sdk/client/stdio.js'; -import { StreamableHTTPClientTransport } from '@modelcontextprotocol/sdk/client/streamableHttp.js'; -import { ResourceListChangedNotificationSchema } from '@modelcontextprotocol/sdk/types.js'; -import { WebSocketClientTransport } from '@modelcontextprotocol/sdk/client/websocket.js'; -import { SSEClientTransport } from '@modelcontextprotocol/sdk/client/sse.js'; import { Client } from '@modelcontextprotocol/sdk/client/index.js'; -import { logger } from '@librechat/data-schemas'; +import { SSEClientTransport } from '@modelcontextprotocol/sdk/client/sse.js'; +import { WebSocketClientTransport } from '@modelcontextprotocol/sdk/client/websocket.js'; +import { ResourceListChangedNotificationSchema } from '@modelcontextprotocol/sdk/types.js'; +import { StreamableHTTPClientTransport } from '@modelcontextprotocol/sdk/client/streamableHttp.js'; import type { Transport } from '@modelcontextprotocol/sdk/shared/transport.js'; import type { JSONRPCMessage } from '@modelcontextprotocol/sdk/types.js'; import type { @@ -18,8 +18,9 @@ import type { Response as UndiciResponse, } from 'undici'; import type { MCPOAuthTokens } from './oauth/types'; -import { mcpConfig } from './mcpConfig'; import type * as t from './types'; +import { sanitizeUrlForLogging } from './utils'; +import { mcpConfig } from './mcpConfig'; type FetchLike = (url: string | URL, init?: RequestInit) => Promise; @@ -238,7 +239,9 @@ export class MCPConnection extends EventEmitter { } this.url = options.url; const url = new URL(options.url); - logger.info(`${this.getLogPrefix()} Creating SSE transport: ${url.toString()}`); + logger.info( + `${this.getLogPrefix()} Creating SSE transport: ${sanitizeUrlForLogging(url)}`, + ); const abortController = new AbortController(); /** Add OAuth token to headers if available */ @@ -293,7 +296,7 @@ export class MCPConnection extends EventEmitter { this.url = options.url; const url = new URL(options.url); logger.info( - `${this.getLogPrefix()} Creating streamable-http transport: ${url.toString()}`, + `${this.getLogPrefix()} Creating streamable-http transport: ${sanitizeUrlForLogging(url)}`, ); const abortController = new AbortController(); @@ -473,7 +476,9 @@ export class MCPConnection extends EventEmitter { logger.warn(`${this.getLogPrefix()} OAuth authentication required`); this.oauthRequired = true; const serverUrl = this.url; - logger.debug(`${this.getLogPrefix()} Server URL for OAuth: ${serverUrl}`); + logger.debug( + `${this.getLogPrefix()} Server URL for OAuth: ${serverUrl ? sanitizeUrlForLogging(serverUrl) : 'undefined'}`, + ); const oauthTimeout = this.options.initTimeout ?? 60000 * 2; /** Promise that will resolve when OAuth is handled */ diff --git a/packages/api/src/mcp/oauth/handler.ts b/packages/api/src/mcp/oauth/handler.ts index 89ad0e2a6c..3c1718cf8e 100644 --- a/packages/api/src/mcp/oauth/handler.ts +++ b/packages/api/src/mcp/oauth/handler.ts @@ -17,6 +17,7 @@ import type { MCPOAuthTokens, OAuthMetadata, } from './types'; +import { sanitizeUrlForLogging } from '~/mcp/utils'; /** Type for the OAuth metadata from the SDK */ type SDKOAuthMetadata = Parameters[1]['metadata']; @@ -33,7 +34,9 @@ export class MCPOAuthHandler { resourceMetadata?: OAuthProtectedResourceMetadata; authServerUrl: URL; }> { - logger.debug(`[MCPOAuth] discoverMetadata called with serverUrl: ${serverUrl}`); + logger.debug( + `[MCPOAuth] discoverMetadata called with serverUrl: ${sanitizeUrlForLogging(serverUrl)}`, + ); let authServerUrl = new URL(serverUrl); let resourceMetadata: OAuthProtectedResourceMetadata | undefined; @@ -60,11 +63,15 @@ export class MCPOAuthHandler { } // Discover OAuth metadata - logger.debug(`[MCPOAuth] Discovering OAuth metadata from ${authServerUrl}`); + logger.debug( + `[MCPOAuth] Discovering OAuth metadata from ${sanitizeUrlForLogging(authServerUrl)}`, + ); const rawMetadata = await discoverAuthorizationServerMetadata(authServerUrl); if (!rawMetadata) { - logger.error(`[MCPOAuth] Failed to discover OAuth metadata from ${authServerUrl}`); + logger.error( + `[MCPOAuth] Failed to discover OAuth metadata from ${sanitizeUrlForLogging(authServerUrl)}`, + ); throw new Error('Failed to discover OAuth metadata'); } @@ -88,12 +95,15 @@ export class MCPOAuthHandler { resourceMetadata?: OAuthProtectedResourceMetadata, redirectUri?: string, ): Promise { - logger.debug(`[MCPOAuth] Starting client registration for ${serverUrl}, server metadata:`, { - grant_types_supported: metadata.grant_types_supported, - response_types_supported: metadata.response_types_supported, - token_endpoint_auth_methods_supported: metadata.token_endpoint_auth_methods_supported, - scopes_supported: metadata.scopes_supported, - }); + logger.debug( + `[MCPOAuth] Starting client registration for ${sanitizeUrlForLogging(serverUrl)}, server metadata:`, + { + grant_types_supported: metadata.grant_types_supported, + response_types_supported: metadata.response_types_supported, + token_endpoint_auth_methods_supported: metadata.token_endpoint_auth_methods_supported, + scopes_supported: metadata.scopes_supported, + }, + ); /** Client metadata based on what the server supports */ const clientMetadata = { @@ -114,7 +124,9 @@ export class MCPOAuthHandler { `[MCPOAuth] Server ${serverUrl} supports \`refresh_token\` grant type, adding to request`, ); } else { - logger.debug(`[MCPOAuth] Server ${serverUrl} does not support \`refresh_token\` grant type`); + logger.debug( + `[MCPOAuth] Server ${sanitizeUrlForLogging(serverUrl)} does not support \`refresh_token\` grant type`, + ); } clientMetadata.grant_types = requestedGrantTypes; @@ -139,19 +151,25 @@ export class MCPOAuthHandler { clientMetadata.scope = availableScopes.join(' '); } - logger.debug(`[MCPOAuth] Registering client for ${serverUrl} with metadata:`, clientMetadata); + logger.debug( + `[MCPOAuth] Registering client for ${sanitizeUrlForLogging(serverUrl)} with metadata:`, + clientMetadata, + ); const clientInfo = await registerClient(serverUrl, { metadata: metadata as unknown as SDKOAuthMetadata, clientMetadata, }); - logger.debug(`[MCPOAuth] Client registered successfully for ${serverUrl}:`, { - client_id: clientInfo.client_id, - has_client_secret: !!clientInfo.client_secret, - grant_types: clientInfo.grant_types, - scope: clientInfo.scope, - }); + logger.debug( + `[MCPOAuth] Client registered successfully for ${sanitizeUrlForLogging(serverUrl)}:`, + { + client_id: clientInfo.client_id, + has_client_secret: !!clientInfo.client_secret, + grant_types: clientInfo.grant_types, + scope: clientInfo.scope, + }, + ); return clientInfo; } @@ -165,7 +183,9 @@ export class MCPOAuthHandler { userId: string, config: MCPOptions['oauth'] | undefined, ): Promise<{ authorizationUrl: string; flowId: string; flowMetadata: MCPOAuthFlowMetadata }> { - logger.debug(`[MCPOAuth] initiateOAuthFlow called for ${serverName} with URL: ${serverUrl}`); + logger.debug( + `[MCPOAuth] initiateOAuthFlow called for ${serverName} with URL: ${sanitizeUrlForLogging(serverUrl)}`, + ); const flowId = this.generateFlowId(userId, serverName); const state = this.generateState(); @@ -226,7 +246,9 @@ export class MCPOAuthHandler { metadata, }; - logger.debug(`[MCPOAuth] Authorization URL generated: ${authorizationUrl.toString()}`); + logger.debug( + `[MCPOAuth] Authorization URL generated: ${sanitizeUrlForLogging(authorizationUrl.toString())}`, + ); return { authorizationUrl: authorizationUrl.toString(), flowId, @@ -234,10 +256,14 @@ export class MCPOAuthHandler { }; } - logger.debug(`[MCPOAuth] Starting auto-discovery of OAuth metadata from ${serverUrl}`); + logger.debug( + `[MCPOAuth] Starting auto-discovery of OAuth metadata from ${sanitizeUrlForLogging(serverUrl)}`, + ); const { metadata, resourceMetadata, authServerUrl } = await this.discoverMetadata(serverUrl); - logger.debug(`[MCPOAuth] OAuth metadata discovered, auth server URL: ${authServerUrl}`); + logger.debug( + `[MCPOAuth] OAuth metadata discovered, auth server URL: ${sanitizeUrlForLogging(authServerUrl)}`, + ); /** Dynamic client registration based on the discovered metadata */ const redirectUri = config?.redirect_uri || this.getDefaultRedirectUri(serverName); @@ -276,7 +302,9 @@ export class MCPOAuthHandler { codeVerifier = authResult.codeVerifier; logger.debug(`[MCPOAuth] startAuthorization completed successfully`); - logger.debug(`[MCPOAuth] Authorization URL: ${authorizationUrl.toString()}`); + logger.debug( + `[MCPOAuth] Authorization URL: ${sanitizeUrlForLogging(authorizationUrl.toString())}`, + ); /** Add state parameter with flowId to the authorization URL */ authorizationUrl.searchParams.set('state', flowId); @@ -515,7 +543,7 @@ export class MCPOAuthHandler { body.append('client_id', metadata.clientInfo.client_id); } - logger.debug(`[MCPOAuth] Refresh request to: ${tokenUrl}`, { + logger.debug(`[MCPOAuth] Refresh request to: ${sanitizeUrlForLogging(tokenUrl)}`, { body: body.toString(), headers, }); @@ -695,7 +723,9 @@ export class MCPOAuthHandler { } // perform the revoke request - logger.info(`[MCPOAuth] Revoking tokens for ${serverName} via ${revokeUrl.toString()}`); + logger.info( + `[MCPOAuth] Revoking tokens for ${serverName} via ${sanitizeUrlForLogging(revokeUrl.toString())}`, + ); const response = await fetch(revokeUrl, { method: 'POST', body: body.toString(), diff --git a/packages/api/src/mcp/utils.ts b/packages/api/src/mcp/utils.ts index 631ce5c210..447754031e 100644 --- a/packages/api/src/mcp/utils.ts +++ b/packages/api/src/mcp/utils.ts @@ -31,3 +31,17 @@ export function normalizeServerName(serverName: string): string { return normalized; } + +/** + * Sanitizes a URL by removing query parameters to prevent credential leakage in logs. + * @param url - The URL to sanitize (string or URL object) + * @returns The sanitized URL string without query parameters + */ +export function sanitizeUrlForLogging(url: string | URL): string { + try { + const urlObj = typeof url === 'string' ? new URL(url) : url; + return `${urlObj.protocol}//${urlObj.host}${urlObj.pathname}`; + } catch { + return '[invalid URL]'; + } +}