mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-04 06:52:47 +00:00
🧹 fix: Clean Up MCP OAuth State Mappings on Uninstall + Reject Superseded Callbacks (#14549)
* 🧹 fix: Clean Up MCP OAuth State Mappings on Uninstall + Reject Superseded Callbacks Disconnecting an OAuth MCP server deleted its mcp_oauth flows but left the mcp_oauth_state:{state} mappings behind for the full TTL. Because flow ids are deterministic (userId:serverName) and the CSRF token is HMAC(flowId), a stale browser tab's callback could resolve its orphaned state to the NEXT flow for the same server, pass CSRF, burn the fresh flow's one-shot CSRF cookie, and fail the PKCE exchange, sabotaging the legitimate retry. - Add MCPOAuthHandler.deleteFlowAndStateMapping: reads the flow's stored state and deletes the mapping before the flow (mapping-first so a crash between deletes fails closed instead of recreating the orphan) - Route mcp_oauth deletions in clearStoredMCPOAuthState through the helper for both tenant-scoped and legacy flow ids - Reject callbacks whose state does not match the resolved flow's stored state: the only control distinguishing a superseded attempt from the current one on a deterministic flow id Fixes #14534 * fix: gate failFlow on state match in the OAuth error branch (Codex P1) The provider-error branch failed the resolved flow on CSRF/session alone, so a superseded error callback resolved through an orphaned mapping could mark the current flow FAILED. Apply the same stored-state equality gate before failFlow. * fix: leave the flow in place when the state-mapping delete fails (Codex P2) deleteFlow swallows storage errors and returns false, and deleteStateMapping discarded that result, so a failed mapping delete followed by a successful flow delete would silently recreate the orphan. Surface the boolean from deleteStateMapping and throw from deleteFlowAndStateMapping before touching the flow, so the caller's allSettled warn branch fires and the next replacement retries both. * fix: restore the state mapping when the flow delete fails (Codex P2) The inverse partial failure of the round-3 fix: a successful mapping delete followed by a silently failed flow delete left a PENDING flow whose reused authorization URL could never resolve, dead-ending every callback in invalid_state until the flow went stale. Check deleteFlow's result, re-store the mapping on failure, and throw so the caller's allSettled warn branch fires. * fix: never leave a callback-capable flow behind on uninstall (Codex round 6) Teardown runs after the server's tokens are deleted, so a preserved flow+mapping pair (the round-3 early-throw path) let a lingering consent tab complete the callback and recreate credentials post-uninstall. Now that both callback branches gate on stored-state equality, an orphaned mapping is the benign failure mode, so invert the order: delete the flow first, attempt the mapping delete regardless, and reject when either reports a storage failure. This supersedes the round-4 mapping restore, which also preserved a callback-capable pair. * fix: delete the flow even when its metadata read fails (Codex round 7) A storage error on the initial getFlowState aborted teardown before any delete ran, preserving the callback-capable flow after token deletion. Tolerate the read failure, delete the flow blindly, skip the mapping it could not identify (the callback gates neutralize the possible orphan), and reject so the caller's warn branch fires.
This commit is contained in:
Danny Avila
GitHub
parent
a67b0c1da8
commit
78ec1940a2
7 changed files with 443 additions and 32 deletions
|
|
@ -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') {
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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',
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -542,6 +542,159 @@ describe('MCP OAuth Race Condition Fixes', () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe('deleteFlowAndStateMapping (uninstall teardown)', () => {
|
||||
const createFlowManager = () => {
|
||||
const store = new MockKeyv();
|
||||
return new FlowStateManager<MCPOAuthTokens | null>(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);
|
||||
|
|
|
|||
|
|
@ -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<MCPOAuthTokens | null>,
|
||||
): Promise<boolean> {
|
||||
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<MCPOAuthTokens | null>,
|
||||
): Promise<void> {
|
||||
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})`,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue