From 9bcc96f20792d80d102b9f3cb8e7bc954a77bdbd Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 23 Nov 2025 06:42:40 +0000 Subject: [PATCH] Fix authentication bypass vulnerability by using loadOriginalSettings for bearer auth validation Co-authored-by: samanhappy <2755122+samanhappy@users.noreply.github.com> --- src/services/sseService.test.ts | 71 ++++++++++++++++++------------ src/services/sseService.ts | 5 ++- tests/security/auth-bypass.test.ts | 11 +++-- 3 files changed, 54 insertions(+), 33 deletions(-) diff --git a/src/services/sseService.test.ts b/src/services/sseService.test.ts index 295148b..58cc996 100644 --- a/src/services/sseService.test.ts +++ b/src/services/sseService.test.ts @@ -22,21 +22,23 @@ jest.mock('../config/index.js', () => { const config = { basePath: '/test', }; + const mockSettings = { + mcpServers: {}, + systemConfig: { + routing: { + enableGlobalRoute: true, + enableGroupNameRoute: true, + enableBearerAuth: false, + bearerAuthKey: 'test-key', + }, + enableSessionRebuild: false, // Default to false for tests + }, + }; return { __esModule: true, default: config, - loadSettings: jest.fn(() => ({ - mcpServers: {}, - systemConfig: { - routing: { - enableGlobalRoute: true, - enableGroupNameRoute: true, - enableBearerAuth: false, - bearerAuthKey: 'test-key', - }, - enableSessionRebuild: false, // Default to false for tests - }, - })), + loadSettings: jest.fn(() => mockSettings), + loadOriginalSettings: jest.fn(() => mockSettings), }; }); @@ -66,7 +68,7 @@ jest.mock('@modelcontextprotocol/sdk/types.js', () => ({ // Import mocked modules import { getMcpServer } from './mcpService.js'; -import { loadSettings } from '../config/index.js'; +import { loadSettings, loadOriginalSettings } from '../config/index.js'; import { UserContextService } from './userContextService.js'; import { SSEServerTransport } from '@modelcontextprotocol/sdk/server/sse.js'; import { StreamableHTTPServerTransport } from '@modelcontextprotocol/sdk/server/streamableHttp.js'; @@ -152,12 +154,19 @@ const expectBearerUnauthorized = ( ); }; +const setMockSettings = (settings: any): void => { + (loadSettings as jest.MockedFunction).mockReturnValue(settings); + (loadOriginalSettings as jest.MockedFunction).mockReturnValue( + settings, + ); +}; + describe('sseService', () => { beforeEach(() => { jest.clearAllMocks(); // Reset settings cache - (loadSettings as jest.MockedFunction).mockReturnValue({ + const mockSettingsValue = { mcpServers: {}, systemConfig: { routing: { @@ -168,7 +177,11 @@ describe('sseService', () => { }, enableSessionRebuild: false, // Default to false for tests }, - }); + }; + setMockSettings(mockSettingsValue); + (loadOriginalSettings as jest.MockedFunction).mockReturnValue( + mockSettingsValue, + ); }); describe('bearer authentication', () => { @@ -185,7 +198,7 @@ describe('sseService', () => { }); it('should return 401 when bearer auth is enabled but no authorization header', async () => { - (loadSettings as jest.MockedFunction).mockReturnValue({ + const mockSettingsValue = { mcpServers: {}, systemConfig: { routing: { @@ -195,7 +208,11 @@ describe('sseService', () => { bearerAuthKey: 'test-key', }, }, - }); + }; + setMockSettings(mockSettingsValue); + (loadOriginalSettings as jest.MockedFunction).mockReturnValue( + mockSettingsValue, + ); const req = createMockRequest(); const res = createMockResponse(); @@ -206,7 +223,7 @@ describe('sseService', () => { }); it('should return 401 when bearer auth is enabled with invalid token', async () => { - (loadSettings as jest.MockedFunction).mockReturnValue({ + setMockSettings({ mcpServers: {}, systemConfig: { routing: { @@ -229,7 +246,7 @@ describe('sseService', () => { }); it('should pass when bearer auth is enabled with valid token', async () => { - (loadSettings as jest.MockedFunction).mockReturnValue({ + setMockSettings({ mcpServers: {}, systemConfig: { routing: { @@ -279,7 +296,7 @@ describe('sseService', () => { describe('handleSseConnection', () => { it('should reject global routes when disabled', async () => { - (loadSettings as jest.MockedFunction).mockReturnValue({ + setMockSettings({ mcpServers: {}, systemConfig: { routing: { @@ -375,7 +392,7 @@ describe('sseService', () => { }); it('should return 401 when bearer auth fails', async () => { - (loadSettings as jest.MockedFunction).mockReturnValue({ + setMockSettings({ mcpServers: {}, systemConfig: { routing: { @@ -400,7 +417,7 @@ describe('sseService', () => { describe('handleMcpPostRequest', () => { it('should reject global routes when disabled', async () => { - (loadSettings as jest.MockedFunction).mockReturnValue({ + setMockSettings({ mcpServers: {}, systemConfig: { routing: { @@ -463,7 +480,7 @@ describe('sseService', () => { it('should transparently rebuild invalid session when enabled', async () => { // Enable session rebuild for this test - (loadSettings as jest.MockedFunction).mockReturnValue({ + setMockSettings({ mcpServers: {}, systemConfig: { routing: { @@ -492,7 +509,7 @@ describe('sseService', () => { }); it('should return 401 when bearer auth fails', async () => { - (loadSettings as jest.MockedFunction).mockReturnValue({ + setMockSettings({ mcpServers: {}, systemConfig: { routing: { @@ -533,7 +550,7 @@ describe('sseService', () => { Object.keys(transports).forEach(key => delete transports[key]); // Enable bearer auth for this test - (loadSettings as jest.MockedFunction).mockReturnValue({ + setMockSettings({ mcpServers: {}, systemConfig: { routing: { @@ -570,7 +587,7 @@ describe('sseService', () => { it('should transparently rebuild invalid session in handleMcpOtherRequest when enabled', async () => { // Enable bearer auth and session rebuild for this test - (loadSettings as jest.MockedFunction).mockReturnValue({ + setMockSettings({ mcpServers: {}, systemConfig: { routing: { @@ -602,7 +619,7 @@ describe('sseService', () => { }); it('should return 401 when bearer auth fails', async () => { - (loadSettings as jest.MockedFunction).mockReturnValue({ + setMockSettings({ mcpServers: {}, systemConfig: { routing: { diff --git a/src/services/sseService.ts b/src/services/sseService.ts index ec7bf5c..8e51b1c 100644 --- a/src/services/sseService.ts +++ b/src/services/sseService.ts @@ -5,7 +5,7 @@ import { SSEServerTransport } from '@modelcontextprotocol/sdk/server/sse.js'; import { StreamableHTTPServerTransport } from '@modelcontextprotocol/sdk/server/streamableHttp.js'; import { isInitializeRequest } from '@modelcontextprotocol/sdk/types.js'; import { deleteMcpServer, getMcpServer } from './mcpService.js'; -import { loadSettings } from '../config/index.js'; +import { loadSettings, loadOriginalSettings } from '../config/index.js'; import config from '../config/index.js'; import { UserContextService } from './userContextService.js'; import { RequestContextService } from './requestContextService.js'; @@ -31,7 +31,8 @@ type BearerAuthResult = }; const validateBearerAuth = (req: Request): BearerAuthResult => { - const settings = loadSettings(); + // Use original settings to get the actual systemConfig, not filtered by user context + const settings = loadOriginalSettings(); const routingConfig = settings.systemConfig?.routing || { enableGlobalRoute: true, enableGroupNameRoute: true, diff --git a/tests/security/auth-bypass.test.ts b/tests/security/auth-bypass.test.ts index a7956c3..4f102ef 100644 --- a/tests/security/auth-bypass.test.ts +++ b/tests/security/auth-bypass.test.ts @@ -243,7 +243,8 @@ describe('Authentication Bypass Security Tests', () => { }); // With valid bearer token, should succeed (200 or 202) - expect([200, 202]).toContain(response.status); + expect(response.status).toBeGreaterThanOrEqual(200); + expect(response.status).toBeLessThan(300); }); it('should reject invalid bearer token', async () => { @@ -299,11 +300,12 @@ describe('Authentication Bypass Security Tests', () => { const response = await request(httpServer) .get('/admin/sse/alice-private') .set('Authorization', 'Bearer supersecret-value') - .set('Accept', 'text/event-stream'); + .set('Accept', 'text/event-stream') + .timeout(5000); // Add timeout to prevent hanging // Should establish SSE connection (200) expect(response.status).toBe(200); - }); + }, 10000); // Increase test timeout }); describe('Global Routes - Bearer Auth Enforcement', () => { @@ -341,7 +343,8 @@ describe('Authentication Bypass Security Tests', () => { }, }); - expect([200, 202]).toContain(response.status); + expect(response.status).toBeGreaterThanOrEqual(200); + expect(response.status).toBeLessThan(300); }); });