mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-02-23 19:04:10 +01:00
🛡️ fix: Secure MCP/Actions OAuth Flows, Resolve Race Condition & Tool Cache Cleanup (#11756)
* 🔧 fix: Update OAuth error message for clarity - Changed the default error message in the OAuth error route from 'Unknown error' to 'Unknown OAuth error' to provide clearer context during authentication failures. * 🔒 feat: Enhance OAuth flow with CSRF protection and session management - Implemented CSRF protection for OAuth flows by introducing `generateOAuthCsrfToken`, `setOAuthCsrfCookie`, and `validateOAuthCsrf` functions. - Added session management for OAuth with `setOAuthSession` and `validateOAuthSession` middleware. - Updated routes to bind CSRF tokens for MCP and action OAuth flows, ensuring secure authentication. - Enhanced tests to validate CSRF handling and session management in OAuth processes. * 🔧 refactor: Invalidate cached tools after user plugin disconnection - Added a call to `invalidateCachedTools` in the `updateUserPluginsController` to ensure that cached tools are refreshed when a user disconnects from an MCP server after a plugin authentication update. This change improves the accuracy of tool data for users. * chore: imports order * fix: domain separator regex usage in ToolService - Moved the declaration of `domainSeparatorRegex` to avoid redundancy in the `loadActionToolsForExecution` function, improving code clarity and performance. * chore: OAuth flow error handling and CSRF token generation - Enhanced the OAuth callback route to validate the flow ID format, ensuring proper error handling for invalid states. - Updated the CSRF token generation function to require a JWT secret, throwing an error if not provided, which improves security and clarity in token generation. - Adjusted tests to reflect changes in flow ID handling and ensure robust validation across various scenarios.
This commit is contained in:
Danny Avila
GitHub
parent
72a30cd9c4
commit
599f4a11f1
14 changed files with 523 additions and 141 deletions
|
|
@ -1,8 +1,18 @@
|
|||
const crypto = require('crypto');
|
||||
const express = require('express');
|
||||
const request = require('supertest');
|
||||
const mongoose = require('mongoose');
|
||||
const { MongoMemoryServer } = require('mongodb-memory-server');
|
||||
const cookieParser = require('cookie-parser');
|
||||
const { getBasePath } = require('@librechat/api');
|
||||
const { MongoMemoryServer } = require('mongodb-memory-server');
|
||||
|
||||
function generateTestCsrfToken(flowId) {
|
||||
return crypto
|
||||
.createHmac('sha256', process.env.JWT_SECRET)
|
||||
.update(flowId)
|
||||
.digest('hex')
|
||||
.slice(0, 32);
|
||||
}
|
||||
|
||||
const mockRegistryInstance = {
|
||||
getServerConfig: jest.fn(),
|
||||
|
|
@ -130,6 +140,7 @@ describe('MCP Routes', () => {
|
|||
|
||||
app = express();
|
||||
app.use(express.json());
|
||||
app.use(cookieParser());
|
||||
|
||||
app.use((req, res, next) => {
|
||||
req.user = { id: 'test-user-id' };
|
||||
|
|
@ -168,12 +179,12 @@ describe('MCP Routes', () => {
|
|||
|
||||
MCPOAuthHandler.initiateOAuthFlow.mockResolvedValue({
|
||||
authorizationUrl: 'https://oauth.example.com/auth',
|
||||
flowId: 'test-flow-id',
|
||||
flowId: 'test-user-id:test-server',
|
||||
});
|
||||
|
||||
const response = await request(app).get('/api/mcp/test-server/oauth/initiate').query({
|
||||
userId: 'test-user-id',
|
||||
flowId: 'test-flow-id',
|
||||
flowId: 'test-user-id:test-server',
|
||||
});
|
||||
|
||||
expect(response.status).toBe(302);
|
||||
|
|
@ -190,7 +201,7 @@ describe('MCP Routes', () => {
|
|||
it('should return 403 when userId does not match authenticated user', async () => {
|
||||
const response = await request(app).get('/api/mcp/test-server/oauth/initiate').query({
|
||||
userId: 'different-user-id',
|
||||
flowId: 'test-flow-id',
|
||||
flowId: 'test-user-id:test-server',
|
||||
});
|
||||
|
||||
expect(response.status).toBe(403);
|
||||
|
|
@ -228,7 +239,7 @@ describe('MCP Routes', () => {
|
|||
|
||||
const response = await request(app).get('/api/mcp/test-server/oauth/initiate').query({
|
||||
userId: 'test-user-id',
|
||||
flowId: 'test-flow-id',
|
||||
flowId: 'test-user-id:test-server',
|
||||
});
|
||||
|
||||
expect(response.status).toBe(400);
|
||||
|
|
@ -245,7 +256,7 @@ describe('MCP Routes', () => {
|
|||
|
||||
const response = await request(app).get('/api/mcp/test-server/oauth/initiate').query({
|
||||
userId: 'test-user-id',
|
||||
flowId: 'test-flow-id',
|
||||
flowId: 'test-user-id:test-server',
|
||||
});
|
||||
|
||||
expect(response.status).toBe(500);
|
||||
|
|
@ -255,7 +266,7 @@ describe('MCP Routes', () => {
|
|||
it('should return 400 when flow state metadata is null', async () => {
|
||||
const mockFlowManager = {
|
||||
getFlowState: jest.fn().mockResolvedValue({
|
||||
id: 'test-flow-id',
|
||||
id: 'test-user-id:test-server',
|
||||
metadata: null,
|
||||
}),
|
||||
};
|
||||
|
|
@ -265,7 +276,7 @@ describe('MCP Routes', () => {
|
|||
|
||||
const response = await request(app).get('/api/mcp/test-server/oauth/initiate').query({
|
||||
userId: 'test-user-id',
|
||||
flowId: 'test-flow-id',
|
||||
flowId: 'test-user-id:test-server',
|
||||
});
|
||||
|
||||
expect(response.status).toBe(400);
|
||||
|
|
@ -280,7 +291,7 @@ describe('MCP Routes', () => {
|
|||
it('should redirect to error page when OAuth error is received', async () => {
|
||||
const response = await request(app).get('/api/mcp/test-server/oauth/callback').query({
|
||||
error: 'access_denied',
|
||||
state: 'test-flow-id',
|
||||
state: 'test-user-id:test-server',
|
||||
});
|
||||
const basePath = getBasePath();
|
||||
|
||||
|
|
@ -290,7 +301,7 @@ describe('MCP Routes', () => {
|
|||
|
||||
it('should redirect to error page when code is missing', async () => {
|
||||
const response = await request(app).get('/api/mcp/test-server/oauth/callback').query({
|
||||
state: 'test-flow-id',
|
||||
state: 'test-user-id:test-server',
|
||||
});
|
||||
const basePath = getBasePath();
|
||||
|
||||
|
|
@ -308,15 +319,50 @@ describe('MCP Routes', () => {
|
|||
expect(response.headers.location).toBe(`${basePath}/oauth/error?error=missing_state`);
|
||||
});
|
||||
|
||||
it('should redirect to error page when flow state is not found', async () => {
|
||||
MCPOAuthHandler.getFlowState.mockResolvedValue(null);
|
||||
|
||||
it('should redirect to error page when CSRF cookie is missing', async () => {
|
||||
const response = await request(app).get('/api/mcp/test-server/oauth/callback').query({
|
||||
code: 'test-auth-code',
|
||||
state: 'invalid-flow-id',
|
||||
state: 'test-user-id:test-server',
|
||||
});
|
||||
const basePath = getBasePath();
|
||||
|
||||
expect(response.status).toBe(302);
|
||||
expect(response.headers.location).toBe(
|
||||
`${basePath}/oauth/error?error=csrf_validation_failed`,
|
||||
);
|
||||
});
|
||||
|
||||
it('should redirect to error page when CSRF cookie does not match state', async () => {
|
||||
const csrfToken = generateTestCsrfToken('different-flow-id');
|
||||
const response = await request(app)
|
||||
.get('/api/mcp/test-server/oauth/callback')
|
||||
.set('Cookie', [`oauth_csrf=${csrfToken}`])
|
||||
.query({
|
||||
code: 'test-auth-code',
|
||||
state: 'test-user-id:test-server',
|
||||
});
|
||||
const basePath = getBasePath();
|
||||
|
||||
expect(response.status).toBe(302);
|
||||
expect(response.headers.location).toBe(
|
||||
`${basePath}/oauth/error?error=csrf_validation_failed`,
|
||||
);
|
||||
});
|
||||
|
||||
it('should redirect to error page when flow state is not found', async () => {
|
||||
MCPOAuthHandler.getFlowState.mockResolvedValue(null);
|
||||
const flowId = 'invalid-flow:id';
|
||||
const csrfToken = generateTestCsrfToken(flowId);
|
||||
|
||||
const response = await request(app)
|
||||
.get('/api/mcp/test-server/oauth/callback')
|
||||
.set('Cookie', [`oauth_csrf=${csrfToken}`])
|
||||
.query({
|
||||
code: 'test-auth-code',
|
||||
state: flowId,
|
||||
});
|
||||
const basePath = getBasePath();
|
||||
|
||||
expect(response.status).toBe(302);
|
||||
expect(response.headers.location).toBe(`${basePath}/oauth/error?error=invalid_state`);
|
||||
});
|
||||
|
|
@ -369,16 +415,22 @@ describe('MCP Routes', () => {
|
|||
});
|
||||
setCachedTools.mockResolvedValue();
|
||||
|
||||
const response = await request(app).get('/api/mcp/test-server/oauth/callback').query({
|
||||
code: 'test-auth-code',
|
||||
state: 'test-flow-id',
|
||||
});
|
||||
const flowId = 'test-user-id:test-server';
|
||||
const csrfToken = generateTestCsrfToken(flowId);
|
||||
|
||||
const response = await request(app)
|
||||
.get('/api/mcp/test-server/oauth/callback')
|
||||
.set('Cookie', [`oauth_csrf=${csrfToken}`])
|
||||
.query({
|
||||
code: 'test-auth-code',
|
||||
state: flowId,
|
||||
});
|
||||
const basePath = getBasePath();
|
||||
|
||||
expect(response.status).toBe(302);
|
||||
expect(response.headers.location).toBe(`${basePath}/oauth/success?serverName=test-server`);
|
||||
expect(MCPOAuthHandler.completeOAuthFlow).toHaveBeenCalledWith(
|
||||
'test-flow-id',
|
||||
flowId,
|
||||
'test-auth-code',
|
||||
mockFlowManager,
|
||||
{},
|
||||
|
|
@ -400,16 +452,24 @@ describe('MCP Routes', () => {
|
|||
'mcp_oauth',
|
||||
mockTokens,
|
||||
);
|
||||
expect(mockFlowManager.deleteFlow).toHaveBeenCalledWith('test-flow-id', 'mcp_get_tokens');
|
||||
expect(mockFlowManager.deleteFlow).toHaveBeenCalledWith(
|
||||
'test-user-id:test-server',
|
||||
'mcp_get_tokens',
|
||||
);
|
||||
});
|
||||
|
||||
it('should redirect to error page when callback processing fails', async () => {
|
||||
MCPOAuthHandler.getFlowState.mockRejectedValue(new Error('Callback error'));
|
||||
const flowId = 'test-user-id:test-server';
|
||||
const csrfToken = generateTestCsrfToken(flowId);
|
||||
|
||||
const response = await request(app).get('/api/mcp/test-server/oauth/callback').query({
|
||||
code: 'test-auth-code',
|
||||
state: 'test-flow-id',
|
||||
});
|
||||
const response = await request(app)
|
||||
.get('/api/mcp/test-server/oauth/callback')
|
||||
.set('Cookie', [`oauth_csrf=${csrfToken}`])
|
||||
.query({
|
||||
code: 'test-auth-code',
|
||||
state: flowId,
|
||||
});
|
||||
const basePath = getBasePath();
|
||||
|
||||
expect(response.status).toBe(302);
|
||||
|
|
@ -442,15 +502,21 @@ describe('MCP Routes', () => {
|
|||
getLogStores.mockReturnValue({});
|
||||
require('~/config').getFlowStateManager.mockReturnValue(mockFlowManager);
|
||||
|
||||
const response = await request(app).get('/api/mcp/test-server/oauth/callback').query({
|
||||
code: 'test-auth-code',
|
||||
state: 'test-flow-id',
|
||||
});
|
||||
const flowId = 'test-user-id:test-server';
|
||||
const csrfToken = generateTestCsrfToken(flowId);
|
||||
|
||||
const response = await request(app)
|
||||
.get('/api/mcp/test-server/oauth/callback')
|
||||
.set('Cookie', [`oauth_csrf=${csrfToken}`])
|
||||
.query({
|
||||
code: 'test-auth-code',
|
||||
state: flowId,
|
||||
});
|
||||
const basePath = getBasePath();
|
||||
|
||||
expect(response.status).toBe(302);
|
||||
expect(response.headers.location).toBe(`${basePath}/oauth/success?serverName=test-server`);
|
||||
expect(mockFlowManager.deleteFlow).toHaveBeenCalledWith('test-flow-id', 'mcp_get_tokens');
|
||||
expect(mockFlowManager.deleteFlow).toHaveBeenCalledWith(flowId, 'mcp_get_tokens');
|
||||
});
|
||||
|
||||
it('should handle reconnection failure after OAuth', async () => {
|
||||
|
|
@ -488,16 +554,22 @@ describe('MCP Routes', () => {
|
|||
getCachedTools.mockResolvedValue({});
|
||||
setCachedTools.mockResolvedValue();
|
||||
|
||||
const response = await request(app).get('/api/mcp/test-server/oauth/callback').query({
|
||||
code: 'test-auth-code',
|
||||
state: 'test-flow-id',
|
||||
});
|
||||
const flowId = 'test-user-id:test-server';
|
||||
const csrfToken = generateTestCsrfToken(flowId);
|
||||
|
||||
const response = await request(app)
|
||||
.get('/api/mcp/test-server/oauth/callback')
|
||||
.set('Cookie', [`oauth_csrf=${csrfToken}`])
|
||||
.query({
|
||||
code: 'test-auth-code',
|
||||
state: flowId,
|
||||
});
|
||||
const basePath = getBasePath();
|
||||
|
||||
expect(response.status).toBe(302);
|
||||
expect(response.headers.location).toBe(`${basePath}/oauth/success?serverName=test-server`);
|
||||
expect(MCPTokenStorage.storeTokens).toHaveBeenCalled();
|
||||
expect(mockFlowManager.deleteFlow).toHaveBeenCalledWith('test-flow-id', 'mcp_get_tokens');
|
||||
expect(mockFlowManager.deleteFlow).toHaveBeenCalledWith(flowId, 'mcp_get_tokens');
|
||||
});
|
||||
|
||||
it('should redirect to error page if token storage fails', async () => {
|
||||
|
|
@ -530,10 +602,16 @@ describe('MCP Routes', () => {
|
|||
};
|
||||
require('~/config').getMCPManager.mockReturnValue(mockMcpManager);
|
||||
|
||||
const response = await request(app).get('/api/mcp/test-server/oauth/callback').query({
|
||||
code: 'test-auth-code',
|
||||
state: 'test-flow-id',
|
||||
});
|
||||
const flowId = 'test-user-id:test-server';
|
||||
const csrfToken = generateTestCsrfToken(flowId);
|
||||
|
||||
const response = await request(app)
|
||||
.get('/api/mcp/test-server/oauth/callback')
|
||||
.set('Cookie', [`oauth_csrf=${csrfToken}`])
|
||||
.query({
|
||||
code: 'test-auth-code',
|
||||
state: flowId,
|
||||
});
|
||||
const basePath = getBasePath();
|
||||
|
||||
expect(response.status).toBe(302);
|
||||
|
|
@ -589,22 +667,27 @@ describe('MCP Routes', () => {
|
|||
clearReconnection: jest.fn(),
|
||||
});
|
||||
|
||||
const response = await request(app).get('/api/mcp/test-server/oauth/callback').query({
|
||||
code: 'test-auth-code',
|
||||
state: 'test-flow-id',
|
||||
});
|
||||
const flowId = 'test-user-id:test-server';
|
||||
const csrfToken = generateTestCsrfToken(flowId);
|
||||
|
||||
const response = await request(app)
|
||||
.get('/api/mcp/test-server/oauth/callback')
|
||||
.set('Cookie', [`oauth_csrf=${csrfToken}`])
|
||||
.query({
|
||||
code: 'test-auth-code',
|
||||
state: flowId,
|
||||
});
|
||||
const basePath = getBasePath();
|
||||
|
||||
expect(response.status).toBe(302);
|
||||
expect(response.headers.location).toBe(`${basePath}/oauth/success?serverName=test-server`);
|
||||
|
||||
// Verify storeTokens was called with ORIGINAL flow state credentials
|
||||
expect(MCPTokenStorage.storeTokens).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
userId: 'test-user-id',
|
||||
serverName: 'test-server',
|
||||
tokens: mockTokens,
|
||||
clientInfo: clientInfo, // Uses original flow state, not any "updated" credentials
|
||||
clientInfo: clientInfo,
|
||||
metadata: flowState.metadata,
|
||||
}),
|
||||
);
|
||||
|
|
@ -631,16 +714,21 @@ describe('MCP Routes', () => {
|
|||
getLogStores.mockReturnValue({});
|
||||
require('~/config').getFlowStateManager.mockReturnValue(mockFlowManager);
|
||||
|
||||
const response = await request(app).get('/api/mcp/test-server/oauth/callback').query({
|
||||
code: 'test-auth-code',
|
||||
state: 'test-flow-id',
|
||||
});
|
||||
const flowId = 'test-user-id:test-server';
|
||||
const csrfToken = generateTestCsrfToken(flowId);
|
||||
|
||||
const response = await request(app)
|
||||
.get('/api/mcp/test-server/oauth/callback')
|
||||
.set('Cookie', [`oauth_csrf=${csrfToken}`])
|
||||
.query({
|
||||
code: 'test-auth-code',
|
||||
state: flowId,
|
||||
});
|
||||
const basePath = getBasePath();
|
||||
|
||||
expect(response.status).toBe(302);
|
||||
expect(response.headers.location).toBe(`${basePath}/oauth/success?serverName=test-server`);
|
||||
|
||||
// Verify completeOAuthFlow was NOT called (prevented duplicate)
|
||||
expect(MCPOAuthHandler.completeOAuthFlow).not.toHaveBeenCalled();
|
||||
expect(MCPTokenStorage.storeTokens).not.toHaveBeenCalled();
|
||||
});
|
||||
|
|
@ -755,7 +843,7 @@ describe('MCP Routes', () => {
|
|||
getLogStores.mockReturnValue({});
|
||||
require('~/config').getFlowStateManager.mockReturnValue(mockFlowManager);
|
||||
|
||||
const response = await request(app).get('/api/mcp/oauth/status/test-flow-id');
|
||||
const response = await request(app).get('/api/mcp/oauth/status/test-user-id:test-server');
|
||||
|
||||
expect(response.status).toBe(200);
|
||||
expect(response.body).toEqual({
|
||||
|
|
@ -766,6 +854,13 @@ describe('MCP Routes', () => {
|
|||
});
|
||||
});
|
||||
|
||||
it('should return 403 when flowId does not match authenticated user', async () => {
|
||||
const response = await request(app).get('/api/mcp/oauth/status/other-user-id:test-server');
|
||||
|
||||
expect(response.status).toBe(403);
|
||||
expect(response.body).toEqual({ error: 'Access denied' });
|
||||
});
|
||||
|
||||
it('should return 404 when flow is not found', async () => {
|
||||
const mockFlowManager = {
|
||||
getFlowState: jest.fn().mockResolvedValue(null),
|
||||
|
|
@ -774,7 +869,7 @@ describe('MCP Routes', () => {
|
|||
getLogStores.mockReturnValue({});
|
||||
require('~/config').getFlowStateManager.mockReturnValue(mockFlowManager);
|
||||
|
||||
const response = await request(app).get('/api/mcp/oauth/status/non-existent-flow');
|
||||
const response = await request(app).get('/api/mcp/oauth/status/test-user-id:non-existent');
|
||||
|
||||
expect(response.status).toBe(404);
|
||||
expect(response.body).toEqual({ error: 'Flow not found' });
|
||||
|
|
@ -788,7 +883,7 @@ describe('MCP Routes', () => {
|
|||
getLogStores.mockReturnValue({});
|
||||
require('~/config').getFlowStateManager.mockReturnValue(mockFlowManager);
|
||||
|
||||
const response = await request(app).get('/api/mcp/oauth/status/error-flow-id');
|
||||
const response = await request(app).get('/api/mcp/oauth/status/test-user-id:error-server');
|
||||
|
||||
expect(response.status).toBe(500);
|
||||
expect(response.body).toEqual({ error: 'Failed to get flow status' });
|
||||
|
|
@ -1375,7 +1470,7 @@ describe('MCP Routes', () => {
|
|||
refresh_token: 'edge-refresh-token',
|
||||
};
|
||||
MCPOAuthHandler.getFlowState = jest.fn().mockResolvedValue({
|
||||
id: 'test-flow-id',
|
||||
id: 'test-user-id:test-server',
|
||||
userId: 'test-user-id',
|
||||
metadata: {
|
||||
serverUrl: 'https://example.com',
|
||||
|
|
@ -1403,8 +1498,12 @@ describe('MCP Routes', () => {
|
|||
};
|
||||
require('~/config').getMCPManager.mockReturnValue(mockMcpManager);
|
||||
|
||||
const flowId = 'test-user-id:test-server';
|
||||
const csrfToken = generateTestCsrfToken(flowId);
|
||||
|
||||
const response = await request(app)
|
||||
.get('/api/mcp/test-server/oauth/callback?code=test-code&state=test-flow-id')
|
||||
.get(`/api/mcp/test-server/oauth/callback?code=test-code&state=${flowId}`)
|
||||
.set('Cookie', [`oauth_csrf=${csrfToken}`])
|
||||
.expect(302);
|
||||
|
||||
const basePath = getBasePath();
|
||||
|
|
@ -1424,7 +1523,7 @@ describe('MCP Routes', () => {
|
|||
|
||||
const mockFlowManager = {
|
||||
getFlowState: jest.fn().mockResolvedValue({
|
||||
id: 'test-flow-id',
|
||||
id: 'test-user-id:test-server',
|
||||
userId: 'test-user-id',
|
||||
metadata: { serverUrl: 'https://example.com', oauth: {} },
|
||||
clientInfo: {},
|
||||
|
|
@ -1453,8 +1552,12 @@ describe('MCP Routes', () => {
|
|||
};
|
||||
require('~/config').getMCPManager.mockReturnValue(mockMcpManager);
|
||||
|
||||
const flowId = 'test-user-id:test-server';
|
||||
const csrfToken = generateTestCsrfToken(flowId);
|
||||
|
||||
const response = await request(app)
|
||||
.get('/api/mcp/test-server/oauth/callback?code=test-code&state=test-flow-id')
|
||||
.get(`/api/mcp/test-server/oauth/callback?code=test-code&state=${flowId}`)
|
||||
.set('Cookie', [`oauth_csrf=${csrfToken}`])
|
||||
.expect(302);
|
||||
|
||||
const basePath = getBasePath();
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue