From b97d72e51a29c08ab034b595a4f6e153a97ada27 Mon Sep 17 00:00:00 2001 From: Atef Bellaaj Date: Wed, 10 Dec 2025 02:10:56 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=92=20feat:=20Encrypt=20MCP=20server?= =?UTF-8?q?=20OAuth=20client=20secrets=20(#10846)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Atef Bellaaj --- .../__tests__/ServerConfigsDB.test.ts | 197 ++++++++++++++++-- .../src/mcp/registry/db/ServerConfigsDB.ts | 139 +++++++++--- 2 files changed, 288 insertions(+), 48 deletions(-) diff --git a/packages/api/src/mcp/registry/__tests__/ServerConfigsDB.test.ts b/packages/api/src/mcp/registry/__tests__/ServerConfigsDB.test.ts index ea2dc48285..592f0330e6 100644 --- a/packages/api/src/mcp/registry/__tests__/ServerConfigsDB.test.ts +++ b/packages/api/src/mcp/registry/__tests__/ServerConfigsDB.test.ts @@ -7,23 +7,20 @@ import { PrincipalModel, ResourceType, } from 'librechat-data-provider'; -import { createModels, createMethods, RoleBits } from '@librechat/data-schemas'; -import { ServerConfigsDB } from '../db/ServerConfigsDB'; import type { ParsedServerConfig } from '~/mcp/types'; -// Mock the logger -jest.mock('@librechat/data-schemas', () => ({ - ...jest.requireActual('@librechat/data-schemas'), - logger: { - error: jest.fn(), - warn: jest.fn(), - debug: jest.fn(), - info: jest.fn(), - }, -})); +// Types for dynamically imported modules +type ServerConfigsDBType = import('../db/ServerConfigsDB').ServerConfigsDB; +type CreateMethodsType = typeof import('@librechat/data-schemas').createMethods; +type CreateModelsType = typeof import('@librechat/data-schemas').createModels; +type RoleBitsType = typeof import('@librechat/data-schemas').RoleBits; let mongoServer: MongoMemoryServer; -let serverConfigsDB: ServerConfigsDB; +let serverConfigsDB: ServerConfigsDBType; +let ServerConfigsDB: new (mongoose: typeof import('mongoose')) => ServerConfigsDBType; +let createModels: CreateModelsType; +let createMethods: CreateMethodsType; +let RoleBits: RoleBitsType; // Test data helpers const createSSEConfig = ( @@ -38,9 +35,31 @@ const createSSEConfig = ( ...(oauth && { oauth }), }); -let dbMethods: ReturnType; +let dbMethods: ReturnType; beforeAll(async () => { + // Set encryption keys BEFORE importing modules that use crypto + process.env.CREDS_KEY = '0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef'; + process.env.CREDS_IV = '0123456789abcdef0123456789abcdef'; + + // Clear module cache so crypto module loads with new env vars + jest.resetModules(); + + // Dynamic imports after setting env vars + const dataSchemas = await import('@librechat/data-schemas'); + createModels = dataSchemas.createModels; + createMethods = dataSchemas.createMethods; + RoleBits = dataSchemas.RoleBits; + + // Mock logger after import (suppress logs during tests) + jest.spyOn(dataSchemas.logger, 'error').mockReturnValue(dataSchemas.logger); + jest.spyOn(dataSchemas.logger, 'warn').mockReturnValue(dataSchemas.logger); + jest.spyOn(dataSchemas.logger, 'debug').mockReturnValue(dataSchemas.logger); + jest.spyOn(dataSchemas.logger, 'info').mockReturnValue(dataSchemas.logger); + + const serverConfigsModule = await import('../db/ServerConfigsDB'); + ServerConfigsDB = serverConfigsModule.ServerConfigsDB; + mongoServer = await MongoMemoryServer.create(); const mongoUri = mongoServer.getUri(); await mongoose.connect(mongoUri); @@ -159,6 +178,11 @@ describe('ServerConfigsDB', () => { }); const created = await serverConfigsDB.add('temp-name', config, userId); + // Verify the secret is encrypted in DB after add (not plaintext) + const MCPServer = mongoose.models.MCPServer; + let server = await MCPServer.findOne({ serverName: created.serverName }); + expect(server?.config?.oauth?.client_secret).not.toBe('super-secret-key'); + // Update without client_secret const updatedConfig = createSSEConfig('OAuth Server', 'Updated description', { client_id: 'my-client-id', @@ -166,10 +190,13 @@ describe('ServerConfigsDB', () => { }); await serverConfigsDB.update(created.serverName, updatedConfig, userId); - // Verify the secret is preserved - const MCPServer = mongoose.models.MCPServer; - const server = await MCPServer.findOne({ serverName: created.serverName }); - expect(server?.config?.oauth?.client_secret).toBe('super-secret-key'); + // Verify the secret is still encrypted in DB (preserved, not plaintext) + server = await MCPServer.findOne({ serverName: created.serverName }); + expect(server?.config?.oauth?.client_secret).not.toBe('super-secret-key'); + + // Verify the secret is decrypted when accessed via get() + const retrieved = await serverConfigsDB.get(created.serverName, userId); + expect(retrieved?.oauth?.client_secret).toBe('super-secret-key'); }); it('should allow updating oauth.client_secret when explicitly provided', async () => { @@ -186,10 +213,57 @@ describe('ServerConfigsDB', () => { }); await serverConfigsDB.update(created.serverName, updatedConfig, userId); - // Verify the secret is updated + // Verify the secret is encrypted in DB (not plaintext) const MCPServer = mongoose.models.MCPServer; const server = await MCPServer.findOne({ serverName: created.serverName }); - expect(server?.config?.oauth?.client_secret).toBe('new-secret'); + expect(server?.config?.oauth?.client_secret).not.toBe('new-secret'); + + // Verify the secret is decrypted to the new value when accessed via get() + const retrieved = await serverConfigsDB.get(created.serverName, userId); + expect(retrieved?.oauth?.client_secret).toBe('new-secret'); + }); + + it('should encrypt oauth.client_secret when saving to database', async () => { + const config = createSSEConfig('Encryption Test', 'Test', { + client_id: 'test-client-id', + client_secret: 'plaintext-secret', + }); + const created = await serverConfigsDB.add('temp-name', config, userId); + + // Verify the secret is encrypted in DB (not plaintext) + const MCPServer = mongoose.models.MCPServer; + const server = await MCPServer.findOne({ serverName: created.serverName }); + expect(server?.config?.oauth?.client_secret).not.toBe('plaintext-secret'); + + // Verify the secret is decrypted when accessed via get() + const retrieved = await serverConfigsDB.get(created.serverName, userId); + expect(retrieved?.oauth?.client_secret).toBe('plaintext-secret'); + }); + + it('should pass through config without oauth field unchanged', async () => { + const config = createSSEConfig('No OAuth Server', 'Test without oauth'); + const created = await serverConfigsDB.add('temp-name', config, userId); + + const retrieved = await serverConfigsDB.get(created.serverName, userId); + expect(retrieved?.oauth).toBeUndefined(); + expect(retrieved?.title).toBe('No OAuth Server'); + }); + + it('should pass through config with oauth but no client_secret unchanged', async () => { + const config: ParsedServerConfig = { + type: 'sse', + url: 'https://example.com/mcp', + title: 'OAuth No Secret', + oauth: { + client_id: 'my-client-id', + // No client_secret + }, + }; + const created = await serverConfigsDB.add('temp-name', config, userId); + + const retrieved = await serverConfigsDB.get(created.serverName, userId); + expect(retrieved?.oauth?.client_id).toBe('my-client-id'); + expect(retrieved?.oauth?.client_secret).toBeUndefined(); }); }); @@ -626,6 +700,31 @@ describe('ServerConfigsDB', () => { expect(result['agent1-server']?.consumeOnly).toBe(true); expect(result['agent2-server']?.consumeOnly).toBe(true); }); + + it('should decrypt oauth.client_secret for multiple servers', async () => { + const config1 = createSSEConfig('Secret Server 1', 'First', { + client_id: 'client-1', + client_secret: 'secret-one', + }); + const config2 = createSSEConfig('Secret Server 2', 'Second', { + client_id: 'client-2', + client_secret: 'secret-two', + }); + const config3 = createSSEConfig('No Secret Server', 'Third'); + + await serverConfigsDB.add('temp1', config1, userId); + await serverConfigsDB.add('temp2', config2, userId); + await serverConfigsDB.add('temp3', config3, userId); + + const result = await serverConfigsDB.getAll(userId); + + expect(Object.keys(result)).toHaveLength(3); + // Verify secrets are decrypted + expect(result['secret-server-1']?.oauth?.client_secret).toBe('secret-one'); + expect(result['secret-server-2']?.oauth?.client_secret).toBe('secret-two'); + // Verify server without secret is unaffected + expect(result['no-secret-server']?.oauth).toBeUndefined(); + }); }); }); @@ -841,4 +940,62 @@ describe('ServerConfigsDB', () => { expect(result).toBeUndefined(); }); }); + + describe('encryption/decryption error handling', () => { + it('should return config without secret when decryption fails (graceful degradation)', async () => { + // Create a server normally first + const config = createSSEConfig('Decryption Failure Test', 'Test', { + client_id: 'test-client', + client_secret: 'test-secret', + }); + const created = await serverConfigsDB.add('temp-name', config, userId); + + // Directly corrupt the encrypted secret in the database to simulate decryption failure + const MCPServer = mongoose.models.MCPServer; + await MCPServer.updateOne( + { serverName: created.serverName }, + { $set: { 'config.oauth.client_secret': 'invalid:corrupted:encrypted:value' } }, + ); + + // Get should return config without the secret (graceful degradation) + const retrieved = await serverConfigsDB.get(created.serverName, userId); + + expect(retrieved).toBeDefined(); + expect(retrieved?.title).toBe('Decryption Failure Test'); + expect(retrieved?.oauth?.client_id).toBe('test-client'); + // Secret should be removed due to decryption failure + expect(retrieved?.oauth?.client_secret).toBeUndefined(); + }); + + it('should handle getAll with mixed valid and corrupted secrets', async () => { + // Create servers with valid secrets + const config1 = createSSEConfig('Valid Secret Server', 'Test', { + client_id: 'client-1', + client_secret: 'valid-secret', + }); + const config2 = createSSEConfig('Corrupted Secret Server', 'Test', { + client_id: 'client-2', + client_secret: 'will-be-corrupted', + }); + const created1 = await serverConfigsDB.add('temp1', config1, userId); + const created2 = await serverConfigsDB.add('temp2', config2, userId); + + // Corrupt the second server's secret + const MCPServer = mongoose.models.MCPServer; + await MCPServer.updateOne( + { serverName: created2.serverName }, + { $set: { 'config.oauth.client_secret': 'invalid:corrupted:data' } }, + ); + + // GetAll should still return both servers + const result = await serverConfigsDB.getAll(userId); + + expect(Object.keys(result)).toHaveLength(2); + // First server should have decrypted secret + expect(result[created1.serverName]?.oauth?.client_secret).toBe('valid-secret'); + // Second server should have secret removed due to decryption failure + expect(result[created2.serverName]?.oauth?.client_id).toBe('client-2'); + expect(result[created2.serverName]?.oauth?.client_secret).toBeUndefined(); + }); + }); }); diff --git a/packages/api/src/mcp/registry/db/ServerConfigsDB.ts b/packages/api/src/mcp/registry/db/ServerConfigsDB.ts index e6ef642d16..d8121557ad 100644 --- a/packages/api/src/mcp/registry/db/ServerConfigsDB.ts +++ b/packages/api/src/mcp/registry/db/ServerConfigsDB.ts @@ -5,7 +5,14 @@ import { PrincipalType, ResourceType, } from 'librechat-data-provider'; -import { AllMethods, MCPServerDocument, createMethods, logger } from '@librechat/data-schemas'; +import { + AllMethods, + MCPServerDocument, + createMethods, + logger, + encryptV2, + decryptV2, +} from '@librechat/data-schemas'; import type { IServerConfigsRepositoryInterface } from '~/mcp/registry/ServerConfigsRepositoryInterface'; import { AccessControlService } from '~/acl/accessControlService'; import type { ParsedServerConfig, AddServerResult } from '~/mcp/types'; @@ -88,7 +95,12 @@ export class ServerConfigsDB implements IServerConfigsRepositoryInterface { '[ServerConfigsDB.add] User ID is required to create a database-stored MCP server.', ); } - const createdServer = await this._dbMethods.createMCPServer({ config: config, author: userId }); + // Encrypt sensitive fields before storing in database + const encryptedConfig = await this.encryptConfig(config); + const createdServer = await this._dbMethods.createMCPServer({ + config: encryptedConfig, + author: userId, + }); await this._aclService.grantPermission({ principalType: PrincipalType.USER, principalId: userId, @@ -99,7 +111,7 @@ export class ServerConfigsDB implements IServerConfigsRepositoryInterface { }); return { serverName: createdServer.serverName, - config: this.mapDBServerToParsedConfig(createdServer), + config: await this.mapDBServerToParsedConfig(createdServer), }; } @@ -120,22 +132,29 @@ export class ServerConfigsDB implements IServerConfigsRepositoryInterface { ); } - // Preserve sensitive fields (like oauth.client_secret) that may not be sent from the client - // Create a copy to avoid mutating the input parameter - let mergedConfig = config; + // Handle secret preservation and encryption const existingServer = await this._dbMethods.findMCPServerById(serverName); - if (existingServer?.config?.oauth?.client_secret && !config.oauth?.client_secret) { - mergedConfig = { + let configToSave: ParsedServerConfig; + + if (!config.oauth?.client_secret && existingServer?.config?.oauth?.client_secret) { + // No new secret provided - preserve the existing encrypted secret from DB (don't re-encrypt) + configToSave = { ...config, oauth: { ...config.oauth, client_secret: existingServer.config.oauth.client_secret, }, }; + } else if (config.oauth?.client_secret) { + // New secret provided - encrypt it + configToSave = await this.encryptConfig(config); + } else { + // No secret in config or DB - nothing to encrypt + configToSave = config; } // specific user permissions for action permission will be handled in the controller calling the update method of the registry - await this._dbMethods.updateMCPServer(serverName, { config: mergedConfig }); + await this._dbMethods.updateMCPServer(serverName, { config: configToSave }); } /** @@ -176,7 +195,7 @@ export class ServerConfigsDB implements IServerConfigsRepositoryInterface { }) ).map((id) => id.toString()); if (directlyAccessibleMCPIds.indexOf(server._id.toString()) > -1) { - return this.mapDBServerToParsedConfig(server); + return await this.mapDBServerToParsedConfig(server); } // Check access via publicly accessible agents @@ -186,7 +205,7 @@ export class ServerConfigsDB implements IServerConfigsRepositoryInterface { `[ServerConfigsDB.get] accessing ${serverName} via public agent (consumeOnly)`, ); return { - ...this.mapDBServerToParsedConfig(server), + ...(await this.mapDBServerToParsedConfig(server)), consumeOnly: true, }; } @@ -206,7 +225,7 @@ export class ServerConfigsDB implements IServerConfigsRepositoryInterface { logger.debug( `[ServerConfigsDB.get] getting ${serverName} for user with the UserId: ${userId}`, ); - return this.mapDBServerToParsedConfig(server); + return await this.mapDBServerToParsedConfig(server); } // Check agent access (user can VIEW an agent that has this MCP server) @@ -216,7 +235,7 @@ export class ServerConfigsDB implements IServerConfigsRepositoryInterface { `[ServerConfigsDB.get] user ${userId} accessing ${serverName} via agent (consumeOnly)`, ); return { - ...this.mapDBServerToParsedConfig(server), + ...(await this.mapDBServerToParsedConfig(server)), consumeOnly: true, }; } @@ -293,14 +312,17 @@ export class ServerConfigsDB implements IServerConfigsRepositoryInterface { ids: directlyAccessibleMCPIds, }); - // 4. Build result with direct access servers + // 4. Build result with direct access servers (parallel decryption) const parsedConfigs: Record = {}; - const directServerNames = new Set(); + const directData = directResults.data || []; + const directServerNames = new Set(directData.map((s) => s.serverName)); - for (const s of directResults.data || []) { - parsedConfigs[s.serverName] = this.mapDBServerToParsedConfig(s); - directServerNames.add(s.serverName); - } + const directParsed = await Promise.all( + directData.map((s) => this.mapDBServerToParsedConfig(s)), + ); + directData.forEach((s, i) => { + parsedConfigs[s.serverName] = directParsed[i]; + }); // 5. Fetch agent-accessible servers (excluding already direct) const agentOnlyServerNames = agentMCPServerNames.filter((name) => !directServerNames.has(name)); @@ -310,12 +332,13 @@ export class ServerConfigsDB implements IServerConfigsRepositoryInterface { names: agentOnlyServerNames, }); - for (const s of agentServers.data || []) { - parsedConfigs[s.serverName] = { - ...this.mapDBServerToParsedConfig(s), - consumeOnly: true, - }; - } + const agentData = agentServers.data || []; + const agentParsed = await Promise.all( + agentData.map((s) => this.mapDBServerToParsedConfig(s)), + ); + agentData.forEach((s, i) => { + parsedConfigs[s.serverName] = { ...agentParsed[i], consumeOnly: true }; + }); } return parsedConfigs; @@ -327,12 +350,72 @@ export class ServerConfigsDB implements IServerConfigsRepositoryInterface { return; } - /** Maps a MongoDB server document to the ParsedServerConfig format. */ - private mapDBServerToParsedConfig(serverDBDoc: MCPServerDocument): ParsedServerConfig { - return { + /** + * Maps a MongoDB server document to the ParsedServerConfig format. + * Decrypts sensitive fields (oauth.client_secret) after retrieval. + */ + private async mapDBServerToParsedConfig( + serverDBDoc: MCPServerDocument, + ): Promise { + const config: ParsedServerConfig = { ...serverDBDoc.config, dbId: (serverDBDoc._id as Types.ObjectId).toString(), updatedAt: serverDBDoc.updatedAt?.getTime(), }; + // Decrypt sensitive fields after retrieval from database + return await this.decryptConfig(config); + } + + /** + * Encrypts sensitive fields in config before database storage. + * Currently encrypts only oauth.client_secret. + * Throws on failure to prevent storing plaintext secrets. + */ + private async encryptConfig(config: ParsedServerConfig): Promise { + if (!config.oauth?.client_secret) { + return config; + } + try { + return { + ...config, + oauth: { + ...config.oauth, + client_secret: await encryptV2(config.oauth.client_secret), + }, + }; + } catch (error) { + logger.error('[ServerConfigsDB.encryptConfig] Failed to encrypt client_secret', error); + throw new Error('Failed to encrypt MCP server configuration'); + } + } + + /** + * Decrypts sensitive fields in config after database retrieval. + * Returns config without secret on failure (graceful degradation). + */ + private async decryptConfig(config: ParsedServerConfig): Promise { + if (!config.oauth?.client_secret) { + return config; + } + try { + return { + ...config, + oauth: { + ...config.oauth, + client_secret: await decryptV2(config.oauth.client_secret), + }, + }; + } catch (error) { + logger.warn( + '[ServerConfigsDB.decryptConfig] Failed to decrypt client_secret, returning config without secret', + error, + ); + // eslint-disable-next-line @typescript-eslint/no-unused-vars + const { client_secret: _removed, ...oauthWithoutSecret } = config.oauth; + return { + ...config, + oauth: oauthWithoutSecret, + }; + } } }