mirror of
https://github.com/danny-avila/LibreChat.git
synced 2025-12-17 00:40:14 +01:00
🔒 feat: Encrypt MCP server OAuth client secrets (#10846)
Co-authored-by: Atef Bellaaj <slalom.bellaaj@external.daimlertruck.com>
This commit is contained in:
Atef Bellaaj
Danny Avila
parent
b4b5a2cd69
commit
b97d72e51a
2 changed files with 288 additions and 48 deletions
|
|
@ -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<typeof createMethods>;
|
||||
let dbMethods: ReturnType<CreateMethodsType>;
|
||||
|
||||
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();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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<string, ParsedServerConfig> = {};
|
||||
const directServerNames = new Set<string>();
|
||||
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<ParsedServerConfig> {
|
||||
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<ParsedServerConfig> {
|
||||
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<ParsedServerConfig> {
|
||||
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,
|
||||
};
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue