mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-01-13 14:08:51 +01:00
🪪 fix: Misleading MCP Server Lookup Method Name (#11315)
* 🔧 fix: MCP server ID resolver in access permissions (#11315) - Replaced `findMCPServerById` with `findMCPServerByObjectId` in access permissions route and corresponding tests for improved clarity and consistency in resource identification. * 🔧 refactor: Update MCP server resource access methods to use server name - Replaced instances of `findMCPServerById` with `findMCPServerByServerName` across middleware, database, and test files for improved clarity and consistency in resource identification. - Updated related comments and test cases to reflect the change in method usage. * chore: Increase timeout for Redis update in GenerationJobManager integration tests - Updated the timeout duration from 50ms to 200ms in the GenerationJobManager integration tests to ensure reliable verification of final event data in Redis after emitting the done event.
This commit is contained in:
parent
a8fa85b8e2
commit
f8774983a0
8 changed files with 26 additions and 27 deletions
|
|
@ -1,16 +1,16 @@
|
|||
const { ResourceType } = require('librechat-data-provider');
|
||||
const { canAccessResource } = require('./canAccessResource');
|
||||
const { findMCPServerById } = require('~/models');
|
||||
const { findMCPServerByServerName } = require('~/models');
|
||||
|
||||
/**
|
||||
* MCP Server ID resolver function
|
||||
* Resolves custom MCP server ID (e.g., "mcp_abc123") to MongoDB ObjectId
|
||||
* MCP Server name resolver function
|
||||
* Resolves MCP server name (e.g., "my-mcp-server") to MongoDB ObjectId
|
||||
*
|
||||
* @param {string} mcpServerCustomId - Custom MCP server ID from route parameter
|
||||
* @param {string} serverName - Server name from route parameter
|
||||
* @returns {Promise<Object|null>} MCP server document with _id field, or null if not found
|
||||
*/
|
||||
const resolveMCPServerId = async (mcpServerCustomId) => {
|
||||
return await findMCPServerById(mcpServerCustomId);
|
||||
const resolveMCPServerName = async (serverName) => {
|
||||
return await findMCPServerByServerName(serverName);
|
||||
};
|
||||
|
||||
/**
|
||||
|
|
@ -52,7 +52,7 @@ const canAccessMCPServerResource = (options) => {
|
|||
resourceType: ResourceType.MCPSERVER,
|
||||
requiredPermission,
|
||||
resourceIdParam,
|
||||
idResolver: resolveMCPServerId,
|
||||
idResolver: resolveMCPServerName,
|
||||
});
|
||||
};
|
||||
|
||||
|
|
|
|||
|
|
@ -545,7 +545,7 @@ describe('canAccessMCPServerResource middleware', () => {
|
|||
|
||||
describe('error handling', () => {
|
||||
test('should handle server returning null gracefully (treated as not found)', async () => {
|
||||
// When an MCP server is not found, findMCPServerById returns null
|
||||
// When an MCP server is not found, findMCPServerByServerName returns null
|
||||
// which the middleware correctly handles as a 404
|
||||
req.params.serverName = 'definitely-non-existent-server';
|
||||
|
||||
|
|
|
|||
|
|
@ -11,7 +11,7 @@ const {
|
|||
const { requireJwtAuth, checkBan, uaParser, canAccessResource } = require('~/server/middleware');
|
||||
const { checkPeoplePickerAccess } = require('~/server/middleware/checkPeoplePickerAccess');
|
||||
const { checkSharePublicAccess } = require('~/server/middleware/checkSharePublicAccess');
|
||||
const { findMCPServerById } = require('~/models');
|
||||
const { findMCPServerByObjectId } = require('~/models');
|
||||
|
||||
const router = express.Router();
|
||||
|
||||
|
|
@ -64,7 +64,7 @@ const checkResourcePermissionAccess = (requiredPermission) => (req, res, next) =
|
|||
resourceType: ResourceType.MCPSERVER,
|
||||
requiredPermission,
|
||||
resourceIdParam: 'resourceId',
|
||||
idResolver: findMCPServerById,
|
||||
idResolver: findMCPServerByObjectId,
|
||||
});
|
||||
} else {
|
||||
return res.status(400).json({
|
||||
|
|
|
|||
|
|
@ -32,7 +32,7 @@ jest.mock('~/server/middleware/checkPeoplePickerAccess', () => ({
|
|||
|
||||
// Import actual middleware to get canAccessResource
|
||||
const { canAccessResource } = require('~/server/middleware');
|
||||
const { findMCPServerById } = require('~/models');
|
||||
const { findMCPServerByObjectId } = require('~/models');
|
||||
|
||||
/**
|
||||
* Security Tests for SBA-ADV-20251203-02
|
||||
|
|
@ -151,7 +151,7 @@ describe('Access Permissions Routes - Security Tests (SBA-ADV-20251203-02)', ()
|
|||
resourceType: ResourceType.MCPSERVER,
|
||||
requiredPermission,
|
||||
resourceIdParam: 'resourceId',
|
||||
idResolver: findMCPServerById,
|
||||
idResolver: findMCPServerByObjectId,
|
||||
});
|
||||
} else {
|
||||
return res.status(400).json({
|
||||
|
|
|
|||
|
|
@ -134,7 +134,7 @@ export class ServerConfigsDB implements IServerConfigsRepositoryInterface {
|
|||
);
|
||||
}
|
||||
|
||||
const existingServer = await this._dbMethods.findMCPServerById(serverName);
|
||||
const existingServer = await this._dbMethods.findMCPServerByServerName(serverName);
|
||||
let configToSave: ParsedServerConfig = { ...config };
|
||||
|
||||
// Transform user-provided API key config (adds customUserVars and headers)
|
||||
|
|
@ -204,7 +204,7 @@ export class ServerConfigsDB implements IServerConfigsRepositoryInterface {
|
|||
* @returns The parsed server config or undefined if not found. If accessed via agent, consumeOnly will be true.
|
||||
*/
|
||||
public async get(serverName: string, userId?: string): Promise<ParsedServerConfig | undefined> {
|
||||
const server = await this._dbMethods.findMCPServerById(serverName);
|
||||
const server = await this._dbMethods.findMCPServerByServerName(serverName);
|
||||
if (!server) return undefined;
|
||||
|
||||
// Check public access if no userId
|
||||
|
|
|
|||
|
|
@ -584,8 +584,7 @@ describe('GenerationJobManager Integration Tests', () => {
|
|||
};
|
||||
GenerationJobManager.emitDone(streamId, finalEventData as never);
|
||||
|
||||
// Wait for async Redis update
|
||||
await new Promise((resolve) => setTimeout(resolve, 50));
|
||||
await new Promise((resolve) => setTimeout(resolve, 200));
|
||||
|
||||
// Verify finalEvent is in Redis
|
||||
const jobStore = services.jobStore;
|
||||
|
|
|
|||
|
|
@ -228,22 +228,22 @@ describe('MCPServer Model Tests', () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe('findMCPServerById', () => {
|
||||
describe('findMCPServerByServerName', () => {
|
||||
test('should find server by serverName', async () => {
|
||||
const created = await methods.createMCPServer({
|
||||
config: createSSEConfig('Find By Id Test'),
|
||||
config: createSSEConfig('Find By Name Test'),
|
||||
author: authorId,
|
||||
});
|
||||
|
||||
const found = await methods.findMCPServerById(created.serverName);
|
||||
const found = await methods.findMCPServerByServerName(created.serverName);
|
||||
|
||||
expect(found).toBeDefined();
|
||||
expect(found?.serverName).toBe('find-by-id-test');
|
||||
expect(found?.config.title).toBe('Find By Id Test');
|
||||
expect(found?.serverName).toBe('find-by-name-test');
|
||||
expect(found?.config.title).toBe('Find By Name Test');
|
||||
});
|
||||
|
||||
test('should return null when server not found', async () => {
|
||||
const found = await methods.findMCPServerById('non-existent-server');
|
||||
const found = await methods.findMCPServerByServerName('non-existent-server');
|
||||
|
||||
expect(found).toBeNull();
|
||||
});
|
||||
|
|
@ -254,7 +254,7 @@ describe('MCPServer Model Tests', () => {
|
|||
author: authorId,
|
||||
});
|
||||
|
||||
const found = await methods.findMCPServerById('lean-test');
|
||||
const found = await methods.findMCPServerByServerName('lean-test');
|
||||
|
||||
// Lean documents don't have mongoose methods
|
||||
expect(found).toBeDefined();
|
||||
|
|
@ -621,7 +621,7 @@ describe('MCPServer Model Tests', () => {
|
|||
expect(deleted?.serverName).toBe('delete-test');
|
||||
|
||||
// Verify it's actually deleted
|
||||
const found = await methods.findMCPServerById('delete-test');
|
||||
const found = await methods.findMCPServerByServerName('delete-test');
|
||||
expect(found).toBeNull();
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -134,10 +134,10 @@ export function createMCPServerMethods(mongoose: typeof import('mongoose')) {
|
|||
|
||||
/**
|
||||
* Find an MCP server by serverName
|
||||
* @param serverName - The MCP server ID
|
||||
* @param serverName - The unique server name identifier
|
||||
* @returns The MCP server document or null
|
||||
*/
|
||||
async function findMCPServerById(serverName: string): Promise<MCPServerDocument | null> {
|
||||
async function findMCPServerByServerName(serverName: string): Promise<MCPServerDocument | null> {
|
||||
const MCPServer = mongoose.models.MCPServer as Model<MCPServerDocument>;
|
||||
return await MCPServer.findOne({ serverName }).lean();
|
||||
}
|
||||
|
|
@ -311,7 +311,7 @@ export function createMCPServerMethods(mongoose: typeof import('mongoose')) {
|
|||
|
||||
return {
|
||||
createMCPServer,
|
||||
findMCPServerById,
|
||||
findMCPServerByServerName,
|
||||
findMCPServerByObjectId,
|
||||
findMCPServersByAuthor,
|
||||
getListMCPServersByIds,
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue