diff --git a/api/server/controllers/UserController.js b/api/server/controllers/UserController.js index ff10b96cea..6086a5b871 100644 --- a/api/server/controllers/UserController.js +++ b/api/server/controllers/UserController.js @@ -457,7 +457,11 @@ const clearStoredMCPOAuthState = async (userId, serverName) => { }) === index, ); const results = await Promise.allSettled( - flowDeletes.map(([flowId, type]) => flowManager.deleteFlow(flowId, type)), + flowDeletes.map(([flowId, type]) => + type === 'mcp_oauth' + ? MCPOAuthHandler.deleteFlowAndStateMapping(flowId, flowManager) + : flowManager.deleteFlow(flowId, type), + ), ); for (const result of results) { if (result.status === 'rejected') { diff --git a/api/server/controllers/__tests__/UserController.mcpOAuth.spec.js b/api/server/controllers/__tests__/UserController.mcpOAuth.spec.js index d8c43b5fc4..e955369a2b 100644 --- a/api/server/controllers/__tests__/UserController.mcpOAuth.spec.js +++ b/api/server/controllers/__tests__/UserController.mcpOAuth.spec.js @@ -31,6 +31,7 @@ jest.mock('@librechat/api', () => ({ const flowId = `${userId}:${serverName}`; return tenantId ? `tenant:${encodeURIComponent(tenantId)}:${flowId}` : flowId; }), + deleteFlowAndStateMapping: jest.fn().mockResolvedValue(undefined), revokeOAuthToken: jest.fn(), }, MCPTokenStorage: { @@ -182,7 +183,10 @@ describe('updateUserPluginsController MCP OAuth cleanup', () => { deleteToken: expect.any(Function), }); expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_get_tokens'); - expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_oauth'); + expect(MCPOAuthHandler.deleteFlowAndStateMapping).toHaveBeenCalledWith( + 'user-1:test-server', + flowManager, + ); expect(MCPOAuthHandler.revokeOAuthToken).not.toHaveBeenCalled(); expect(mcpManager.disconnectUserConnection).toHaveBeenCalledWith('user-1', 'test-server'); }); @@ -198,7 +202,10 @@ describe('updateUserPluginsController MCP OAuth cleanup', () => { expect(res.status).toHaveBeenCalledWith(200); expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_get_tokens'); - expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_oauth'); + expect(MCPOAuthHandler.deleteFlowAndStateMapping).toHaveBeenCalledWith( + 'user-1:test-server', + flowManager, + ); expect(logger.warn).toHaveBeenCalledWith( '[clearStoredMCPOAuthState] Failed to delete MCP OAuth tokens for test-server:', cleanupError, @@ -210,16 +217,18 @@ describe('updateUserPluginsController MCP OAuth cleanup', () => { const getTokensFlowError = new Error('get tokens flow cache down'); const oauthFlowError = new Error('oauth flow cache down'); MCPTokenStorage.getClientInfoAndMetadata.mockResolvedValue(null); - flowManager.deleteFlow - .mockRejectedValueOnce(getTokensFlowError) - .mockRejectedValueOnce(oauthFlowError); + flowManager.deleteFlow.mockRejectedValueOnce(getTokensFlowError); + MCPOAuthHandler.deleteFlowAndStateMapping.mockRejectedValueOnce(oauthFlowError); const res = createResponse(); await updateUserPluginsController(createRequest(), res); expect(res.status).toHaveBeenCalledWith(200); expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_get_tokens'); - expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_oauth'); + expect(MCPOAuthHandler.deleteFlowAndStateMapping).toHaveBeenCalledWith( + 'user-1:test-server', + flowManager, + ); expect(logger.warn).toHaveBeenCalledWith( '[clearStoredMCPOAuthState] Failed to clear MCP OAuth flow state for test-server:', getTokensFlowError, @@ -248,7 +257,10 @@ describe('updateUserPluginsController MCP OAuth cleanup', () => { deleteToken: expect.any(Function), }); expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_get_tokens'); - expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_oauth'); + expect(MCPOAuthHandler.deleteFlowAndStateMapping).toHaveBeenCalledWith( + 'user-1:test-server', + flowManager, + ); expect(MCPTokenStorage.getTokens).not.toHaveBeenCalled(); expect(MCPOAuthHandler.revokeOAuthToken).not.toHaveBeenCalled(); }); @@ -266,12 +278,15 @@ describe('updateUserPluginsController MCP OAuth cleanup', () => { 'tenant:tenant-a:user-1:test-server', 'mcp_get_tokens', ); - expect(flowManager.deleteFlow).toHaveBeenCalledWith( + expect(MCPOAuthHandler.deleteFlowAndStateMapping).toHaveBeenCalledWith( 'tenant:tenant-a:user-1:test-server', - 'mcp_oauth', + flowManager, ); expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_get_tokens'); - expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_oauth'); + expect(MCPOAuthHandler.deleteFlowAndStateMapping).toHaveBeenCalledWith( + 'user-1:test-server', + flowManager, + ); }); it('clears stored OAuth token state when server config is missing', async () => { @@ -288,7 +303,10 @@ describe('updateUserPluginsController MCP OAuth cleanup', () => { deleteToken: expect.any(Function), }); expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_get_tokens'); - expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_oauth'); + expect(MCPOAuthHandler.deleteFlowAndStateMapping).toHaveBeenCalledWith( + 'user-1:test-server', + flowManager, + ); expect(MCPTokenStorage.getClientInfoAndMetadata).not.toHaveBeenCalled(); expect(MCPOAuthHandler.revokeOAuthToken).not.toHaveBeenCalled(); }); @@ -307,7 +325,10 @@ describe('updateUserPluginsController MCP OAuth cleanup', () => { deleteToken: expect.any(Function), }); expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_get_tokens'); - expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_oauth'); + expect(MCPOAuthHandler.deleteFlowAndStateMapping).toHaveBeenCalledWith( + 'user-1:test-server', + flowManager, + ); expect(MCPTokenStorage.getClientInfoAndMetadata).not.toHaveBeenCalled(); expect(MCPOAuthHandler.revokeOAuthToken).not.toHaveBeenCalled(); }); @@ -339,7 +360,10 @@ describe('updateUserPluginsController MCP OAuth cleanup', () => { deleteToken: expect.any(Function), }); expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_get_tokens'); - expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_oauth'); + expect(MCPOAuthHandler.deleteFlowAndStateMapping).toHaveBeenCalledWith( + 'user-1:test-server', + flowManager, + ); expect(MCPOAuthHandler.revokeOAuthToken).not.toHaveBeenCalled(); }); diff --git a/api/server/controllers/__tests__/maybeUninstallOAuthMCP.spec.js b/api/server/controllers/__tests__/maybeUninstallOAuthMCP.spec.js index 1b8436233a..d09c792f22 100644 --- a/api/server/controllers/__tests__/maybeUninstallOAuthMCP.spec.js +++ b/api/server/controllers/__tests__/maybeUninstallOAuthMCP.spec.js @@ -7,6 +7,7 @@ const mockGetOAuthServers = jest.fn(); const mockGetAllowedDomains = jest.fn(); const mockGetAllowedAddresses = jest.fn(); const mockDeleteFlow = jest.fn(); +const mockDeleteFlowAndStateMapping = jest.fn(); const mockGetLogStores = jest.fn(); const mockFindToken = jest.fn(); const mockDeleteTokens = jest.fn(); @@ -25,6 +26,7 @@ jest.mock('@librechat/api', () => { return { MCPOAuthHandler: { revokeOAuthToken: (...args) => mockRevokeOAuthToken(...args), + deleteFlowAndStateMapping: (...args) => mockDeleteFlowAndStateMapping(...args), generateFlowId: (userId, serverName, tenantId) => { const flowId = `${userId}:${serverName}`; return tenantId ? `tenant:${encodeURIComponent(tenantId)}:${flowId}` : flowId; @@ -170,6 +172,7 @@ describe('maybeUninstallOAuthMCP', () => { expect(mockGetTokens).not.toHaveBeenCalled(); expect(mockDeleteUserTokens).not.toHaveBeenCalled(); expect(mockDeleteFlow).not.toHaveBeenCalled(); + expect(mockDeleteFlowAndStateMapping).not.toHaveBeenCalled(); }); test('clears stored state when the MCP server is not an OAuth server', async () => { @@ -182,7 +185,8 @@ describe('maybeUninstallOAuthMCP', () => { expect(mockGetTokens).not.toHaveBeenCalled(); expect(mockDeleteUserTokens).toHaveBeenCalledTimes(1); expect(mockDeleteUserTokens.mock.calls[0][0]).toMatchObject({ userId, serverName }); - expect(mockDeleteFlow).toHaveBeenCalledTimes(2); + expect(mockDeleteFlow).toHaveBeenCalledTimes(1); + expect(mockDeleteFlowAndStateMapping).toHaveBeenCalledTimes(1); }); test('clears stored state when client info is missing', async () => { @@ -194,7 +198,8 @@ describe('maybeUninstallOAuthMCP', () => { expect(mockGetTokens).not.toHaveBeenCalled(); expect(mockDeleteUserTokens).toHaveBeenCalledTimes(1); expect(mockDeleteUserTokens.mock.calls[0][0]).toMatchObject({ userId, serverName }); - expect(mockDeleteFlow).toHaveBeenCalledTimes(2); + expect(mockDeleteFlow).toHaveBeenCalledTimes(1); + expect(mockDeleteFlowAndStateMapping).toHaveBeenCalledTimes(1); }); test('clears stored state when client info cannot be loaded', async () => { @@ -208,7 +213,8 @@ describe('maybeUninstallOAuthMCP', () => { expect(mockGetTokens).not.toHaveBeenCalled(); expect(mockDeleteUserTokens).toHaveBeenCalledTimes(1); expect(mockDeleteUserTokens.mock.calls[0][0]).toMatchObject({ userId, serverName }); - expect(mockDeleteFlow).toHaveBeenCalledTimes(2); + expect(mockDeleteFlow).toHaveBeenCalledTimes(1); + expect(mockDeleteFlowAndStateMapping).toHaveBeenCalledTimes(1); expect(mockLoggerWarn).toHaveBeenCalledWith( `[maybeUninstallOAuthMCP] Unable to load OAuth client metadata for ${serverName}; clearing local MCP OAuth state only.`, expect.any(Error), @@ -222,11 +228,15 @@ describe('maybeUninstallOAuthMCP', () => { await maybeUninstallOAuthMCP(userId, pluginKey, appConfig); - expect(mockDeleteFlow).toHaveBeenCalledTimes(4); + expect(mockDeleteFlow).toHaveBeenCalledTimes(2); expect(mockDeleteFlow).toHaveBeenCalledWith('tenant:tenant-a:user-123:acme', 'mcp_get_tokens'); - expect(mockDeleteFlow).toHaveBeenCalledWith('tenant:tenant-a:user-123:acme', 'mcp_oauth'); expect(mockDeleteFlow).toHaveBeenCalledWith('user-123:acme', 'mcp_get_tokens'); - expect(mockDeleteFlow).toHaveBeenCalledWith('user-123:acme', 'mcp_oauth'); + expect(mockDeleteFlowAndStateMapping).toHaveBeenCalledTimes(2); + expect(mockDeleteFlowAndStateMapping).toHaveBeenCalledWith( + 'tenant:tenant-a:user-123:acme', + expect.anything(), + ); + expect(mockDeleteFlowAndStateMapping).toHaveBeenCalledWith('user-123:acme', expect.anything()); }); test('revokes both tokens and runs cleanup on happy path', async () => { @@ -250,9 +260,10 @@ describe('maybeUninstallOAuthMCP', () => { expect(mockDeleteUserTokens).toHaveBeenCalledTimes(1); expect(mockDeleteUserTokens.mock.calls[0][0]).toMatchObject({ userId, serverName }); - expect(mockDeleteFlow).toHaveBeenCalledTimes(2); + expect(mockDeleteFlow).toHaveBeenCalledTimes(1); expect(mockDeleteFlow.mock.calls[0][1]).toBe('mcp_get_tokens'); - expect(mockDeleteFlow.mock.calls[1][1]).toBe('mcp_oauth'); + expect(mockDeleteFlowAndStateMapping).toHaveBeenCalledTimes(1); + expect(mockDeleteFlowAndStateMapping).toHaveBeenCalledWith('user-123:acme', expect.anything()); }); test('skips revocation but still runs cleanup when token retrieval fails', async () => { @@ -265,7 +276,8 @@ describe('maybeUninstallOAuthMCP', () => { expect(mockRevokeOAuthToken).not.toHaveBeenCalled(); expect(mockDeleteUserTokens).toHaveBeenCalledTimes(1); - expect(mockDeleteFlow).toHaveBeenCalledTimes(2); + expect(mockDeleteFlow).toHaveBeenCalledTimes(1); + expect(mockDeleteFlowAndStateMapping).toHaveBeenCalledTimes(1); expect(mockLoggerWarn).toHaveBeenCalledWith( `[maybeUninstallOAuthMCP] Unable to load OAuth tokens for ${serverName}; clearing local token state.`, expect.any(Error), @@ -282,7 +294,8 @@ describe('maybeUninstallOAuthMCP', () => { expect(mockRevokeOAuthToken).not.toHaveBeenCalled(); expect(mockDeleteUserTokens).toHaveBeenCalledTimes(1); - expect(mockDeleteFlow).toHaveBeenCalledTimes(2); + expect(mockDeleteFlow).toHaveBeenCalledTimes(1); + expect(mockDeleteFlowAndStateMapping).toHaveBeenCalledTimes(1); expect(mockLoggerWarn).toHaveBeenCalledWith( `[maybeUninstallOAuthMCP] Unable to load OAuth tokens for ${serverName}; clearing local token state.`, expect.any(Error), @@ -301,7 +314,8 @@ describe('maybeUninstallOAuthMCP', () => { expect(mockRevokeOAuthToken).toHaveBeenCalledTimes(1); expect(mockRevokeOAuthToken.mock.calls[0][2]).toBe('access'); expect(mockDeleteUserTokens).toHaveBeenCalledTimes(1); - expect(mockDeleteFlow).toHaveBeenCalledTimes(2); + expect(mockDeleteFlow).toHaveBeenCalledTimes(1); + expect(mockDeleteFlowAndStateMapping).toHaveBeenCalledTimes(1); }); test('still runs cleanup even when both revocation calls fail', async () => { @@ -318,7 +332,8 @@ describe('maybeUninstallOAuthMCP', () => { expect(mockRevokeOAuthToken).toHaveBeenCalledTimes(2); expect(mockDeleteUserTokens).toHaveBeenCalledTimes(1); - expect(mockDeleteFlow).toHaveBeenCalledTimes(2); + expect(mockDeleteFlow).toHaveBeenCalledTimes(1); + expect(mockDeleteFlowAndStateMapping).toHaveBeenCalledTimes(1); expect(mockLoggerError).toHaveBeenCalled(); }); }); diff --git a/api/server/routes/__tests__/mcp.spec.js b/api/server/routes/__tests__/mcp.spec.js index 856b73f75c..74df9b7b59 100644 --- a/api/server/routes/__tests__/mcp.spec.js +++ b/api/server/routes/__tests__/mcp.spec.js @@ -560,6 +560,7 @@ describe('MCP Routes', () => { getLogStores.mockReturnValueOnce({}); require('~/config').getFlowStateManager.mockReturnValueOnce(mockFlowManager); MCPOAuthHandler.resolveStateToFlowId.mockResolvedValueOnce(flowId); + MCPOAuthHandler.getFlowState.mockResolvedValueOnce({ state: flowId }); const csrfToken = generateTestCsrfToken(flowId); const response = await request(app) @@ -589,6 +590,7 @@ describe('MCP Routes', () => { getLogStores.mockReturnValueOnce({}); require('~/config').getFlowStateManager.mockReturnValueOnce(mockFlowManager); MCPOAuthHandler.resolveStateToFlowId.mockResolvedValueOnce(flowId); + MCPOAuthHandler.getFlowState.mockResolvedValueOnce({ state: flowId }); const sessionToken = generateTestCsrfToken('test-user-id'); const response = await request(app) @@ -609,6 +611,33 @@ describe('MCP Routes', () => { ); }); + it('should NOT fail the flow when the error callback carries a superseded state', async () => { + const flowId = 'test-user-id:test-server'; + const mockFlowManager = { + failFlow: jest.fn(), + }; + + getLogStores.mockReturnValueOnce({}); + require('~/config').getFlowStateManager.mockReturnValueOnce(mockFlowManager); + /** Orphaned mapping resolves the superseded state to the current flow */ + MCPOAuthHandler.resolveStateToFlowId.mockResolvedValueOnce(flowId); + MCPOAuthHandler.getFlowState.mockResolvedValueOnce({ state: 'current-attempt-state' }); + + const csrfToken = generateTestCsrfToken(flowId); + const response = await request(app) + .get('/api/mcp/test-server/oauth/callback') + .set('Cookie', [`oauth_csrf=${csrfToken}`]) + .query({ + error: 'access_denied', + state: 'superseded-attempt-state', + }); + const basePath = getBasePath(); + + expect(response.status).toBe(302); + expect(response.headers.location).toBe(`${basePath}/oauth/error?error=access_denied`); + expect(mockFlowManager.failFlow).not.toHaveBeenCalled(); + }); + it('should NOT fail the flow when OAuth error is received without cookies (DoS prevention)', async () => { const flowId = 'test-user-id:test-server'; const mockFlowManager = { @@ -719,6 +748,115 @@ describe('MCP Routes', () => { expect(response.headers.location).toBe(`${basePath}/oauth/error?error=invalid_state`); }); + it('should redirect to error page when the state cannot be resolved to a flow ID', async () => { + MCPOAuthHandler.resolveStateToFlowId.mockResolvedValueOnce(null); + + const response = await request(app) + .get('/api/mcp/test-server/oauth/callback') + .query({ code: 'test-auth-code', state: 'orphaned-state-value' }); + const basePath = getBasePath(); + + expect(response.status).toBe(302); + expect(response.headers.location).toBe(`${basePath}/oauth/error?error=invalid_state`); + expect(MCPOAuthHandler.completeOAuthFlow).not.toHaveBeenCalled(); + }); + + it('should reject a stale-state callback without consuming the current flow', async () => { + const flowId = 'test-user-id:test-server'; + const mockFlowManager = { + getFlowState: jest.fn().mockResolvedValue({ + status: 'PENDING', + createdAt: Date.now(), + }), + completeFlow: jest.fn().mockResolvedValue(true), + deleteFlow: jest.fn().mockResolvedValue(true), + }; + + getLogStores.mockReturnValue({}); + require('~/config').getFlowStateManager.mockReturnValue(mockFlowManager); + /** Orphaned mapping from a superseded attempt resolves the old state to the live flow id */ + MCPOAuthHandler.resolveStateToFlowId.mockResolvedValueOnce(flowId); + MCPOAuthHandler.getFlowState.mockResolvedValue({ + state: 'current-attempt-state', + serverName: 'test-server', + userId: 'test-user-id', + metadata: {}, + clientInfo: {}, + codeVerifier: 'current-verifier', + }); + + const csrfToken = generateTestCsrfToken(flowId); + const response = await request(app) + .get('/api/mcp/test-server/oauth/callback') + .set('Cookie', [`oauth_csrf=${csrfToken}`]) + .query({ code: 'stale-auth-code', state: 'superseded-attempt-state' }); + const basePath = getBasePath(); + + expect(response.status).toBe(302); + expect(response.headers.location).toBe(`${basePath}/oauth/error?error=invalid_state`); + expect(MCPOAuthHandler.completeOAuthFlow).not.toHaveBeenCalled(); + expect(MCPTokenStorage.storeTokens).not.toHaveBeenCalled(); + expect(mockFlowManager.completeFlow).not.toHaveBeenCalled(); + }); + + it('should let the current tab complete after a stale callback burned the CSRF cookie', async () => { + const flowId = 'test-user-id:test-server'; + const mockFlowManager = { + getFlowState: jest.fn().mockResolvedValue({ + status: 'PENDING', + createdAt: Date.now(), + }), + completeFlow: jest.fn().mockResolvedValue(true), + deleteFlow: jest.fn().mockResolvedValue(true), + }; + + getLogStores.mockReturnValue({}); + require('~/config').getFlowStateManager.mockReturnValue(mockFlowManager); + MCPOAuthHandler.getFlowState.mockResolvedValue({ + state: flowId, + serverName: 'test-server', + userId: 'test-user-id', + metadata: {}, + clientInfo: {}, + codeVerifier: 'current-verifier', + }); + MCPOAuthHandler.completeOAuthFlow.mockResolvedValue({ access_token: 'test-token' }); + MCPTokenStorage.storeTokens.mockResolvedValue(); + mockRegistryInstance.getServerConfig.mockResolvedValue({}); + + const mockMcpManager = { + getUserConnection: jest.fn().mockResolvedValue({ + fetchTools: jest.fn().mockResolvedValue([]), + }), + }; + require('~/config').getMCPManager.mockReturnValue(mockMcpManager); + require('~/config').getOAuthReconnectionManager.mockReturnValue({ + clearReconnection: jest.fn(), + }); + require('~/server/services/Config/mcp').updateMCPServerTools.mockResolvedValue(); + + MCPOAuthHandler.resolveStateToFlowId.mockResolvedValueOnce(flowId); + const csrfToken = generateTestCsrfToken(flowId); + const staleResponse = await request(app) + .get('/api/mcp/test-server/oauth/callback') + .set('Cookie', [`oauth_csrf=${csrfToken}`]) + .query({ code: 'stale-auth-code', state: 'superseded-attempt-state' }); + + const basePath = getBasePath(); + expect(staleResponse.status).toBe(302); + expect(staleResponse.headers.location).toBe(`${basePath}/oauth/error?error=invalid_state`); + expect(MCPOAuthHandler.completeOAuthFlow).not.toHaveBeenCalled(); + + /** The stale callback consumed the CSRF cookie; the current tab recovers via the fresh PENDING flow */ + const legitResponse = await request(app) + .get('/api/mcp/test-server/oauth/callback') + .query({ code: 'current-auth-code', state: flowId }); + + expect(legitResponse.status).toBe(302); + expect(legitResponse.headers.location).toContain(`${basePath}/oauth/success`); + expect(MCPOAuthHandler.completeOAuthFlow).toHaveBeenCalledTimes(1); + }); + describe('CSRF fallback via active PENDING flow', () => { it('should proceed when a fresh PENDING flow exists and no cookies are present', async () => { const flowId = 'test-user-id:test-server'; @@ -731,6 +869,7 @@ describe('MCP Routes', () => { deleteFlow: jest.fn().mockResolvedValue(true), }; const mockFlowState = { + state: 'test-user-id:test-server', serverName: 'test-server', userId: 'test-user-id', metadata: {}, @@ -778,6 +917,7 @@ describe('MCP Routes', () => { deleteFlow: jest.fn().mockResolvedValue(true), }; const mockFlowState = { + state: 'test-user-id:test-server', serverName: 'test-server', userId: 'test-user-id', metadata: {}, @@ -841,6 +981,7 @@ describe('MCP Routes', () => { deleteFlow: jest.fn().mockResolvedValue(true), }; const mockFlowState = { + state: 'test-user-id:test-server', serverName: 'test-server', userId: 'test-user-id', metadata: {}, @@ -908,6 +1049,7 @@ describe('MCP Routes', () => { deleteFlow: jest.fn().mockResolvedValue(true), }; const mockFlowState = { + state: 'test-user-id:test-server', serverName: 'test-server', userId: 'test-user-id', metadata: {}, @@ -1030,6 +1172,7 @@ describe('MCP Routes', () => { deleteFlow: jest.fn().mockResolvedValue(true), }; const mockFlowState = { + state: 'test-user-id:test-server', serverName: 'test-server', userId: 'test-user-id', metadata: { toolFlowId: 'tool-flow-123' }, @@ -1120,6 +1263,7 @@ describe('MCP Routes', () => { deleteFlow: jest.fn().mockResolvedValue(true), }; const mockFlowState = { + state: 'test-user-id:test-server', serverName: 'test-server', userId: 'test-user-id', metadata: { @@ -1194,6 +1338,7 @@ describe('MCP Routes', () => { deleteFlow: jest.fn().mockResolvedValue(true), }; const mockFlowState = { + state: 'test-user-id:test-server', serverName: 'test-server', userId: 'test-user-id', metadata: {}, @@ -1254,6 +1399,7 @@ describe('MCP Routes', () => { deleteFlow: jest.fn().mockResolvedValue(true), }; const mockFlowState = { + state: 'test-user-id:test-server', serverName: 'test-server', userId: 'test-user-id', metadata: { toolFlowId: 'tool-flow-123' }, @@ -1304,6 +1450,7 @@ describe('MCP Routes', () => { deleteFlow: jest.fn().mockResolvedValue(true), }; const mockFlowState = { + state: 'test-user-id:test-server', serverName: 'test-server', userId: 'test-user-id', metadata: { toolFlowId: 'tool-flow-123' }, @@ -1379,6 +1526,7 @@ describe('MCP Routes', () => { deleteFlow: jest.fn().mockResolvedValue(true), }; const mockFlowState = { + state: 'test-user-id:test-server', serverName: 'test-server', userId: 'system', metadata: { toolFlowId: 'tool-flow-123' }, @@ -1422,6 +1570,7 @@ describe('MCP Routes', () => { deleteFlow: jest.fn().mockResolvedValue(true), }; const mockFlowState = { + state: 'test-user-id:test-server', serverName: 'test-server', userId: 'test-user-id', metadata: { toolFlowId: 'tool-flow-123' }, @@ -1474,6 +1623,7 @@ describe('MCP Routes', () => { deleteFlow: jest.fn().mockResolvedValue(true), }; const mockFlowState = { + state: 'test-user-id:test-server', serverName: 'test-server', userId: 'test-user-id', metadata: { toolFlowId: 'tool-flow-123' }, @@ -1526,6 +1676,7 @@ describe('MCP Routes', () => { client_secret: 'client_secret', }; const flowState = { + state: 'test-user-id:test-server', serverName: 'test-server', userId: 'test-user-id', metadata: { toolFlowId: 'tool-flow-123', serverUrl: 'http://example.com' }, @@ -1601,6 +1752,7 @@ describe('MCP Routes', () => { }); MCPOAuthHandler.getFlowState.mockResolvedValue({ + state: 'test-user-id:test-server', status: 'COMPLETED', serverName: 'test-server', userId: 'test-user-id', @@ -2465,6 +2617,7 @@ describe('MCP Routes', () => { }; MCPOAuthHandler.getFlowState = jest.fn().mockResolvedValue({ id: 'test-user-id:test-server', + state: 'test-user-id:test-server', userId: 'test-user-id', metadata: { serverUrl: 'https://example.com', @@ -2527,6 +2680,7 @@ describe('MCP Routes', () => { }; require('~/config').getFlowStateManager.mockReturnValue(mockFlowManager); MCPOAuthHandler.getFlowState.mockResolvedValue({ + state: 'test-user-id:test-server', serverName: 'test-server', userId: 'test-user-id', metadata: { serverUrl: 'https://example.com', oauth: {} }, @@ -2583,6 +2737,7 @@ describe('MCP Routes', () => { MCPOAuthHandler.resolveStateToFlowId.mockResolvedValue(flowId); MCPOAuthHandler.getFlowState.mockResolvedValue({ + state: flowId, serverName: 'test-server', userId: 'user123', tenantId: 'tenant-abc', diff --git a/api/server/routes/mcp.js b/api/server/routes/mcp.js index 83b5710756..71a905fe10 100644 --- a/api/server/routes/mcp.js +++ b/api/server/routes/mcp.js @@ -255,11 +255,21 @@ router.get('/:serverName/oauth/callback', async (req, res) => { const hasCsrf = validateOAuthCsrf(req, res, flowId, OAUTH_CSRF_COOKIE_PATH); const hasSession = !hasCsrf && validateOAuthSession(req, parsed.userId); if (hasCsrf || hasSession) { - await flowManager.failFlow(flowId, 'mcp_oauth', String(oauthError)); - logger.debug('[MCP OAuth] Marked flow as FAILED with OAuth error', { - flowId, - error: oauthError, - }); + /** A stale mapping can resolve a superseded attempt's state to the + * current flow (deterministic flow ids); only fail the flow this + * error callback actually belongs to */ + const flowMeta = await MCPOAuthHandler.getFlowState(flowId, flowManager); + if (flowMeta?.state === state) { + await flowManager.failFlow(flowId, 'mcp_oauth', String(oauthError)); + logger.debug('[MCP OAuth] Marked flow as FAILED with OAuth error', { + flowId, + error: oauthError, + }); + } else { + logger.warn('[MCP OAuth] Skipping failFlow for superseded OAuth error callback', { + flowId, + }); + } } } } @@ -335,6 +345,17 @@ router.get('/:serverName/oauth/callback', async (req, res) => { return res.redirect(`${basePath}/oauth/error?error=invalid_state`); } + /** + * Flow ids are deterministic (userId:serverName), so a stale state mapping + * can resolve to a newer flow for the same server. The stored state is the + * only per-attempt nonce; a mismatch means this callback belongs to a + * superseded authorization attempt and must not consume the current flow. + */ + if (flowState.state !== state) { + logger.error('[MCP OAuth] State mismatch for flow', { flowId, serverName }); + return res.redirect(`${basePath}/oauth/error?error=invalid_state`); + } + logger.debug('[MCP OAuth] Flow state details', { serverName: flowState.serverName, userId: flowState.userId, diff --git a/packages/api/src/mcp/__tests__/MCPOAuthRaceCondition.test.ts b/packages/api/src/mcp/__tests__/MCPOAuthRaceCondition.test.ts index fbb682c3cf..71e0c49f0b 100644 --- a/packages/api/src/mcp/__tests__/MCPOAuthRaceCondition.test.ts +++ b/packages/api/src/mcp/__tests__/MCPOAuthRaceCondition.test.ts @@ -542,6 +542,159 @@ describe('MCP OAuth Race Condition Fixes', () => { }); }); + describe('deleteFlowAndStateMapping (uninstall teardown)', () => { + const createFlowManager = () => { + const store = new MockKeyv(); + return new FlowStateManager(store as unknown as Keyv, { + ttl: 30000, + ci: true, + }); + }; + + it('deletes both the flow and its state mapping', async () => { + const flowManager = createFlowManager(); + const flowId = 'user1:test-server'; + const state = 'random-state-abc123'; + + await flowManager.initFlow(flowId, 'mcp_oauth', { state }); + await MCPOAuthHandler.storeStateMapping(state, flowId, flowManager); + + await MCPOAuthHandler.deleteFlowAndStateMapping(flowId, flowManager); + + expect(await MCPOAuthHandler.resolveStateToFlowId(state, flowManager)).toBeNull(); + expect(await flowManager.getFlowState(flowId, 'mcp_oauth')).toBeFalsy(); + }); + + it('handles tenant-prefixed flow ids', async () => { + const flowManager = createFlowManager(); + const flowId = 'tenant:acme:user1:test-server'; + const state = 'tenant-state-xyz789'; + + await flowManager.initFlow(flowId, 'mcp_oauth', { state }); + await MCPOAuthHandler.storeStateMapping(state, flowId, flowManager); + + await MCPOAuthHandler.deleteFlowAndStateMapping(flowId, flowManager); + + expect(await MCPOAuthHandler.resolveStateToFlowId(state, flowManager)).toBeNull(); + expect(await flowManager.getFlowState(flowId, 'mcp_oauth')).toBeFalsy(); + }); + + it('is a no-op when the flow is absent and leaves unrelated mappings intact', async () => { + const flowManager = createFlowManager(); + const otherFlowId = 'user2:other-server'; + const otherState = 'other-state-def456'; + + await flowManager.initFlow(otherFlowId, 'mcp_oauth', { state: otherState }); + await MCPOAuthHandler.storeStateMapping(otherState, otherFlowId, flowManager); + + await expect( + MCPOAuthHandler.deleteFlowAndStateMapping('user1:missing-server', flowManager), + ).resolves.toBeUndefined(); + + expect(await MCPOAuthHandler.resolveStateToFlowId(otherState, flowManager)).toBe(otherFlowId); + }); + + it('deletes the flow before the state mapping', async () => { + const flowManager = createFlowManager(); + const flowId = 'user1:test-server'; + const state = 'ordered-state-ghi789'; + + await flowManager.initFlow(flowId, 'mcp_oauth', { state }); + await MCPOAuthHandler.storeStateMapping(state, flowId, flowManager); + + const calls: string[] = []; + const deleteFlowSpy = jest.spyOn(flowManager, 'deleteFlow'); + deleteFlowSpy.mockImplementation(async (id, type) => { + calls.push(`${type}:${id}`); + return true; + }); + + await MCPOAuthHandler.deleteFlowAndStateMapping(flowId, flowManager); + + expect(calls).toEqual([`mcp_oauth:${flowId}`, `mcp_oauth_state:${state}`]); + }); + + it('still deletes the flow and rejects when the mapping delete hits a storage error', async () => { + const flowManager = createFlowManager(); + const flowId = 'user1:test-server'; + const state = 'failing-state-jkl012'; + + await flowManager.initFlow(flowId, 'mcp_oauth', { state }); + await MCPOAuthHandler.storeStateMapping(state, flowId, flowManager); + + const realDeleteFlow = flowManager.deleteFlow.bind(flowManager); + jest.spyOn(flowManager, 'deleteFlow').mockImplementation(async (id, type) => { + if (type === 'mcp_oauth_state') { + return false; + } + return realDeleteFlow(id, type); + }); + + await expect(MCPOAuthHandler.deleteFlowAndStateMapping(flowId, flowManager)).rejects.toThrow( + 'Failed to fully delete OAuth flow', + ); + + /** The callback-capable flow must not survive token deletion */ + expect(await flowManager.getFlowState(flowId, 'mcp_oauth')).toBeFalsy(); + }); + + it('still deletes the mapping and rejects when the flow delete hits a storage error', async () => { + const flowManager = createFlowManager(); + const flowId = 'user1:test-server'; + const state = 'restore-state-mno345'; + + await flowManager.initFlow(flowId, 'mcp_oauth', { state }); + await MCPOAuthHandler.storeStateMapping(state, flowId, flowManager); + + const realDeleteFlow = flowManager.deleteFlow.bind(flowManager); + jest.spyOn(flowManager, 'deleteFlow').mockImplementation(async (id, type) => { + if (type === 'mcp_oauth') { + return false; + } + return realDeleteFlow(id, type); + }); + + await expect(MCPOAuthHandler.deleteFlowAndStateMapping(flowId, flowManager)).rejects.toThrow( + 'Failed to fully delete OAuth flow', + ); + + /** The surviving flow must not stay callback-capable: its state no longer resolves */ + expect(await MCPOAuthHandler.resolveStateToFlowId(state, flowManager)).toBeNull(); + expect(await flowManager.getFlowState(flowId, 'mcp_oauth')).toBeTruthy(); + }); + + it('still deletes the flow and rejects when the metadata read hits a storage error', async () => { + const flowManager = createFlowManager(); + const flowId = 'user1:test-server'; + const state = 'unreadable-state-pqr678'; + + await flowManager.initFlow(flowId, 'mcp_oauth', { state }); + await MCPOAuthHandler.storeStateMapping(state, flowId, flowManager); + + jest + .spyOn(flowManager, 'getFlowState') + .mockRejectedValueOnce(new Error('read connection lost')); + + await expect(MCPOAuthHandler.deleteFlowAndStateMapping(flowId, flowManager)).rejects.toThrow( + 'Failed to fully delete OAuth flow', + ); + + /** The callback-capable flow must not survive token deletion */ + expect(await flowManager.getFlowState(flowId, 'mcp_oauth')).toBeFalsy(); + }); + + it('still deletes the flow when metadata carries no state', async () => { + const flowManager = createFlowManager(); + const flowId = 'user1:stateless-server'; + + await flowManager.initFlow(flowId, 'mcp_oauth', { serverName: 'stateless-server' }); + + await MCPOAuthHandler.deleteFlowAndStateMapping(flowId, flowManager); + + expect(await flowManager.getFlowState(flowId, 'mcp_oauth')).toBeFalsy(); + }); + }); + describe('Fix 4: ReauthenticationRequiredError for no-refresh-token', () => { it('should throw ReauthenticationRequiredError when access token expired and no refresh token', async () => { const expiredDate = new Date(Date.now() - 60000); diff --git a/packages/api/src/mcp/oauth/handler.ts b/packages/api/src/mcp/oauth/handler.ts index d0c55c662a..3cb7fdd861 100644 --- a/packages/api/src/mcp/oauth/handler.ts +++ b/packages/api/src/mcp/oauth/handler.ts @@ -1290,12 +1290,51 @@ export class MCPOAuthHandler { /** * Deletes an orphaned state mapping when a flow is replaced. * Prevents old authorization URLs from resolving after a flow restart. + * Returns `false` when the underlying store rejected the delete. */ static async deleteStateMapping( state: string, flowManager: FlowStateManager, + ): Promise { + return flowManager.deleteFlow(state, this.STATE_MAP_TYPE); + } + + /** + * Deletes an OAuth flow together with its state mapping, for teardown paths + * that don't already hold the flow (e.g. server uninstall). The flow is + * deleted first on purpose: it is what makes a provider callback + * completable, and teardown runs after the server's tokens were removed, so + * a surviving callback-capable flow could recreate credentials the user + * just revoked. A failure between the two deletes leaves at worst an + * orphaned mapping, which the callback's stored-state equality gates reduce + * to a clean invalid_state. Both deletes are attempted regardless of the + * other's outcome; any reported storage failure is surfaced as a rejection + * for the caller's best-effort logging. + */ + static async deleteFlowAndStateMapping( + flowId: string, + flowManager: FlowStateManager, ): Promise { - await flowManager.deleteFlow(state, this.STATE_MAP_TYPE); + /** A failed metadata read must not abort teardown: the flow is deleted + * blindly and the unidentifiable mapping is left to the callback gates */ + let state: string | null = null; + let metadataReadFailed = false; + try { + const flowState = await flowManager.getFlowState(flowId, this.FLOW_TYPE); + const metadata = flowState?.metadata as MCPOAuthFlowMetadata | undefined; + state = typeof metadata?.state === 'string' ? metadata.state : null; + } catch { + metadataReadFailed = true; + } + + const flowDeleted = await flowManager.deleteFlow(flowId, this.FLOW_TYPE); + const mappingDeleted = state ? await this.deleteStateMapping(state, flowManager) : true; + + if (metadataReadFailed || !flowDeleted || !mappingDeleted) { + throw new Error( + `Failed to fully delete OAuth flow ${flowId} (metadata read ok: ${!metadataReadFailed}, flow deleted: ${flowDeleted}, state mapping deleted: ${mappingDeleted})`, + ); + } } /**