diff --git a/api/server/routes/__tests__/mcp.spec.js b/api/server/routes/__tests__/mcp.spec.js index 96e8ff9d9e..ee316811a8 100644 --- a/api/server/routes/__tests__/mcp.spec.js +++ b/api/server/routes/__tests__/mcp.spec.js @@ -184,6 +184,9 @@ describe('MCP Routes', () => { beforeEach(() => { jest.clearAllMocks(); currentUser = undefined; + require('@librechat/api').MCPOAuthHandler.generateFlowId.mockImplementation( + (userId, serverName) => `${userId}:${serverName}`, + ); mockResolveAllMcpConfigs.mockResolvedValue({}); mockResolveMcpConfigNames.mockResolvedValue([]); mockMCPUseAllowed = true; @@ -203,11 +206,70 @@ describe('MCP Routes', () => { const { MCPOAuthHandler } = require('@librechat/api'); const { getLogStores } = require('~/cache'); - it('should initiate OAuth flow successfully', async () => { + it('should reuse stored authorization URL without starting a new OAuth flow', async () => { const mockFlowManager = { getFlowState: jest.fn().mockResolvedValue({ + status: 'PENDING', + createdAt: Date.now(), + metadata: { + serverName: 'test-server', + userId: 'test-user-id', + authorizationUrl: 'https://oauth.example.com/auth?state=stored-state', + }, + }), + }; + + getLogStores.mockReturnValue({}); + require('~/config').getFlowStateManager.mockReturnValue(mockFlowManager); + + const response = await request(app).get('/api/mcp/test-server/oauth/initiate').query({ + userId: 'test-user-id', + flowId: 'test-user-id:test-server', + }); + + expect(response.status).toBe(302); + expect(response.headers.location).toBe('https://oauth.example.com/auth?state=stored-state'); + expect(response.headers['set-cookie']?.join('')).toContain('oauth_csrf='); + expect(MCPOAuthHandler.initiateOAuthFlow).not.toHaveBeenCalled(); + expect(MCPOAuthHandler.storeStateMapping).not.toHaveBeenCalled(); + expect(mockRegistryInstance.getServerConfig).not.toHaveBeenCalled(); + }); + + it('should reject stored authorization URL when flow is no longer pending', async () => { + const mockFlowManager = { + getFlowState: jest.fn().mockResolvedValue({ + status: 'COMPLETED', + createdAt: Date.now(), + metadata: { + serverName: 'test-server', + userId: 'test-user-id', + authorizationUrl: 'https://oauth.example.com/auth?state=stored-state', + }, + }), + }; + + getLogStores.mockReturnValue({}); + require('~/config').getFlowStateManager.mockReturnValue(mockFlowManager); + + const response = await request(app).get('/api/mcp/test-server/oauth/initiate').query({ + userId: 'test-user-id', + flowId: 'test-user-id:test-server', + }); + + expect(response.status).toBe(400); + expect(response.body).toEqual({ error: 'Invalid flow state' }); + expect(MCPOAuthHandler.initiateOAuthFlow).not.toHaveBeenCalled(); + expect(MCPOAuthHandler.storeStateMapping).not.toHaveBeenCalled(); + }); + + it('should initiate OAuth flow when stored authorization URL is missing', async () => { + const mockFlowManager = { + getFlowState: jest.fn().mockResolvedValue({ + status: 'PENDING', + createdAt: Date.now(), metadata: { serverUrl: 'https://test-server.com', + state: 'old-state-value', oauth: { clientId: 'test-client-id' }, }, }), @@ -242,6 +304,23 @@ describe('MCP Routes', () => { undefined, null, ); + expect(MCPOAuthHandler.deleteStateMapping).toHaveBeenCalledWith( + 'old-state-value', + mockFlowManager, + ); + expect(mockFlowManager.initFlow).toHaveBeenCalledWith( + 'test-user-id:test-server', + 'mcp_oauth', + expect.objectContaining({ + state: 'random-state-value', + authorizationUrl: 'https://oauth.example.com/auth', + }), + ); + expect(MCPOAuthHandler.storeStateMapping).toHaveBeenCalledWith( + 'random-state-value', + 'test-user-id:test-server', + mockFlowManager, + ); }); it('should return 403 when userId does not match authenticated user', async () => { @@ -254,6 +333,27 @@ describe('MCP Routes', () => { expect(response.body).toEqual({ error: 'User mismatch' }); }); + it('should return 403 when flowId does not match authenticated user and server', async () => { + const response = await request(app).get('/api/mcp/test-server/oauth/initiate').query({ + userId: 'test-user-id', + flowId: 'other-user-id:test-server', + }); + + expect(response.status).toBe(403); + expect(response.body).toEqual({ error: 'Flow mismatch' }); + expect(getLogStores).not.toHaveBeenCalled(); + }); + + it('should return 403 when flowId query value is not a string', async () => { + const response = await request(app) + .get('/api/mcp/test-server/oauth/initiate') + .query('userId=test-user-id&flowId=test-user-id:test-server&flowId=other-flow'); + + expect(response.status).toBe(403); + expect(response.body).toEqual({ error: 'Flow mismatch' }); + expect(getLogStores).not.toHaveBeenCalled(); + }); + it('should return 404 when flow state is not found', async () => { const mockFlowManager = { getFlowState: jest.fn().mockResolvedValue(null), @@ -264,7 +364,7 @@ describe('MCP Routes', () => { const response = await request(app).get('/api/mcp/test-server/oauth/initiate').query({ userId: 'test-user-id', - flowId: 'non-existent-flow-id', + flowId: 'test-user-id:test-server', }); expect(response.status).toBe(404); diff --git a/api/server/routes/mcp.js b/api/server/routes/mcp.js index 09d6b7d49a..d3de8e29ab 100644 --- a/api/server/routes/mcp.js +++ b/api/server/routes/mcp.js @@ -83,10 +83,21 @@ router.get('/:serverName/oauth/initiate', requireJwtAuth, setOAuthSession, async const user = req.user; // Verify the userId matches the authenticated user - if (userId !== user.id) { + if (typeof userId !== 'string' || userId !== user.id) { return res.status(403).json({ error: 'User mismatch' }); } + const expectedFlowId = MCPOAuthHandler.generateFlowId(user.id, serverName); + if (typeof flowId !== 'string' || flowId !== expectedFlowId) { + logger.error('[MCP OAuth] Invalid flow ID for initiate request', { + serverName, + userId, + flowId, + expectedFlowId, + }); + return res.status(403).json({ error: 'Flow mismatch' }); + } + logger.debug('[MCP OAuth] Initiate request', { serverName, userId, flowId }); const flowsCache = getLogStores(CacheKeys.FLOWS); @@ -99,7 +110,45 @@ router.get('/:serverName/oauth/initiate', requireJwtAuth, setOAuthSession, async return res.status(404).json({ error: 'Flow not found' }); } - const { serverUrl, oauth: oauthConfig } = flowState.metadata || {}; + const { + authorizationUrl: storedAuthorizationUrl, + serverName: flowServerName, + userId: flowUserId, + serverUrl, + oauth: oauthConfig, + } = flowState.metadata || {}; + + if (flowUserId && flowUserId !== user.id) { + logger.error('[MCP OAuth] Flow user mismatch', { flowId, userId, flowUserId }); + return res.status(403).json({ error: 'User mismatch' }); + } + + if (flowServerName && flowServerName !== serverName) { + logger.error('[MCP OAuth] Flow server mismatch', { flowId, serverName, flowServerName }); + return res.status(400).json({ error: 'Invalid flow state' }); + } + + const pendingAge = flowState.createdAt ? Date.now() - flowState.createdAt : Infinity; + const isFreshPendingFlow = flowState.status === 'PENDING' && pendingAge < PENDING_STALE_MS; + if (!isFreshPendingFlow) { + logger.error('[MCP OAuth] Flow is not active for initiation', { + flowId, + status: flowState.status, + pendingAge, + }); + return res.status(400).json({ error: 'Invalid flow state' }); + } + + if (typeof storedAuthorizationUrl === 'string' && storedAuthorizationUrl.length > 0) { + logger.debug('[MCP OAuth] Reusing stored authorization URL', { + serverName, + userId, + flowId, + }); + setOAuthCsrfCookie(res, flowId, OAUTH_CSRF_COOKIE_PATH); + return res.redirect(storedAuthorizationUrl); + } + if (!serverUrl || !oauthConfig) { logger.error('[MCP OAuth] Missing server URL or OAuth config in flow state'); return res.status(400).json({ error: 'Invalid flow state' }); @@ -127,6 +176,12 @@ router.get('/:serverName/oauth/initiate', requireJwtAuth, setOAuthSession, async logger.debug('[MCP OAuth] OAuth flow initiated', { oauthFlowId, authorizationUrl }); + const oldState = flowState.metadata?.state; + if (typeof oldState === 'string') { + await MCPOAuthHandler.deleteStateMapping(oldState, flowManager); + } + const metadataWithUrl = { ...flowMetadata, authorizationUrl, tenantId: getTenantId() }; + await flowManager.initFlow(oauthFlowId, 'mcp_oauth', metadataWithUrl); await MCPOAuthHandler.storeStateMapping(flowMetadata.state, oauthFlowId, flowManager); setOAuthCsrfCookie(res, oauthFlowId, OAUTH_CSRF_COOKIE_PATH); res.redirect(authorizationUrl);