LibreChat/api/server/controllers/__tests__/maybeUninstallOAuthMCP.spec.js
Danny Avila 78ec1940a2
🧹 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.
2026-07-31 12:11:36 -04:00

339 lines
13 KiB
JavaScript

const mockGetTokens = jest.fn();
const mockDeleteUserTokens = jest.fn();
const mockGetClientInfoAndMetadata = jest.fn();
const mockRevokeOAuthToken = jest.fn();
const mockGetServerConfig = jest.fn();
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();
const mockLoggerInfo = jest.fn();
const mockLoggerWarn = jest.fn();
const mockLoggerError = jest.fn();
const mockGetTenantId = jest.fn();
jest.mock('@librechat/data-schemas', () => ({
logger: { info: mockLoggerInfo, warn: mockLoggerWarn, error: mockLoggerError },
getTenantId: (...args) => mockGetTenantId(...args),
webSearchKeys: [],
}));
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;
},
generateTokenFlowId: (userId, serverName, tenantId) => {
const flowId = `${userId}:${serverName}`;
return tenantId ? `tenant:${encodeURIComponent(tenantId)}:${flowId}` : flowId;
},
},
MCPTokenStorage: {
getTokens: (...args) => mockGetTokens(...args),
getClientInfoAndMetadata: (...args) => mockGetClientInfoAndMetadata(...args),
deleteUserTokens: (...args) => mockDeleteUserTokens(...args),
},
normalizeHttpError: jest.fn(),
extractWebSearchEnvVars: jest.fn(),
needsRefresh: jest.fn(),
getNewS3URL: jest.fn(),
};
});
jest.mock('librechat-data-provider', () => ({
Tools: {},
CacheKeys: { FLOWS: 'flows' },
Constants: { mcp_delimiter: '::', mcp_prefix: 'mcp_' },
FileSources: {},
ResourceType: {},
}));
jest.mock('~/config', () => ({
getMCPManager: jest.fn(),
getFlowStateManager: jest.fn(() => ({
deleteFlow: (...args) => mockDeleteFlow(...args),
})),
getMCPServersRegistry: jest.fn(() => ({
getServerConfig: (...args) => mockGetServerConfig(...args),
getOAuthServers: (...args) => mockGetOAuthServers(...args),
getAllowedDomains: (...args) => mockGetAllowedDomains(...args),
getAllowedAddresses: (...args) => mockGetAllowedAddresses(...args),
})),
}));
jest.mock('~/cache', () => ({
getLogStores: (...args) => mockGetLogStores(...args),
}));
jest.mock('~/server/services/PluginService', () => ({
updateUserPluginAuth: jest.fn(),
deleteUserPluginAuth: jest.fn(),
}));
jest.mock('~/server/services/twoFactorService', () => ({
verifyOTPOrBackupCode: jest.fn(),
}));
jest.mock('~/server/services/AuthService', () => ({
verifyEmail: jest.fn(),
resendVerificationEmail: jest.fn(),
}));
jest.mock('~/server/services/Config/getCachedTools', () => ({
invalidateCachedTools: jest.fn(),
}));
jest.mock('~/server/services/Files/process', () => ({
processDeleteRequest: jest.fn().mockResolvedValue({ deletedFileIds: [], failedFileIds: [] }),
}));
jest.mock('~/server/services/Config', () => ({
getAppConfig: jest.fn(),
}));
jest.mock('~/models', () => ({
findToken: (...args) => mockFindToken(...args),
deleteTokens: (...args) => mockDeleteTokens(...args),
updateUser: jest.fn(),
deleteAllUserSessions: jest.fn(),
deleteAllSharedLinks: jest.fn(),
updateUserPlugins: jest.fn(),
deleteUserById: jest.fn(),
deleteMessages: jest.fn(),
deletePresets: jest.fn(),
deleteUserKey: jest.fn(),
getUserById: jest.fn(),
deleteConvos: jest.fn(),
deleteFiles: jest.fn(),
getFiles: jest.fn(),
deleteToolCalls: jest.fn(),
deleteUserAgents: jest.fn(),
deleteUserPrompts: jest.fn(),
deleteTransactions: jest.fn(),
deleteBalances: jest.fn(),
deleteAllAgentApiKeys: jest.fn(),
deleteAssistants: jest.fn(),
deleteConversationTags: jest.fn(),
deleteAllUserMemories: jest.fn(),
deleteActions: jest.fn(),
removeUserFromAllGroups: jest.fn(),
deleteAclEntries: jest.fn(),
getSoleOwnedResourceIds: jest.fn().mockResolvedValue([]),
}));
const { maybeUninstallOAuthMCP } = require('~/server/controllers/UserController');
const userId = 'user-123';
const pluginKey = 'mcp_acme';
const serverName = 'acme';
const serverConfig = {
url: 'https://acme.example.com',
oauth: {
revocation_endpoint: 'https://acme.example.com/revoke',
revocation_endpoint_auth_methods_supported: ['client_secret_basic'],
},
oauth_headers: { 'X-Tenant': 'acme' },
};
const appConfig = {
mcpServers: { acme: serverConfig },
};
const clientInfo = { client_id: 'cid', client_secret: 'csec' };
const clientMetadata = {};
function setupOAuthServerFound() {
mockGetServerConfig.mockResolvedValue(serverConfig);
mockGetOAuthServers.mockResolvedValue(new Set([serverName]));
mockGetAllowedDomains.mockReturnValue(['https://acme.example.com']);
mockGetAllowedAddresses.mockReturnValue(null);
mockGetClientInfoAndMetadata.mockResolvedValue({ clientInfo, clientMetadata });
}
describe('maybeUninstallOAuthMCP', () => {
beforeEach(() => {
jest.clearAllMocks();
mockGetTenantId.mockReturnValue(undefined);
});
test('is a no-op when pluginKey is not an MCP key', async () => {
await maybeUninstallOAuthMCP(userId, 'plugin_google_calendar', appConfig);
expect(mockGetServerConfig).not.toHaveBeenCalled();
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 () => {
mockGetServerConfig.mockResolvedValue(serverConfig);
mockGetOAuthServers.mockResolvedValue(new Set(['other']));
await maybeUninstallOAuthMCP(userId, pluginKey, appConfig);
expect(mockGetClientInfoAndMetadata).not.toHaveBeenCalled();
expect(mockGetTokens).not.toHaveBeenCalled();
expect(mockDeleteUserTokens).toHaveBeenCalledTimes(1);
expect(mockDeleteUserTokens.mock.calls[0][0]).toMatchObject({ userId, serverName });
expect(mockDeleteFlow).toHaveBeenCalledTimes(1);
expect(mockDeleteFlowAndStateMapping).toHaveBeenCalledTimes(1);
});
test('clears stored state when client info is missing', async () => {
setupOAuthServerFound();
mockGetClientInfoAndMetadata.mockResolvedValue(null);
await maybeUninstallOAuthMCP(userId, pluginKey, appConfig);
expect(mockGetTokens).not.toHaveBeenCalled();
expect(mockDeleteUserTokens).toHaveBeenCalledTimes(1);
expect(mockDeleteUserTokens.mock.calls[0][0]).toMatchObject({ userId, serverName });
expect(mockDeleteFlow).toHaveBeenCalledTimes(1);
expect(mockDeleteFlowAndStateMapping).toHaveBeenCalledTimes(1);
});
test('clears stored state when client info cannot be loaded', async () => {
setupOAuthServerFound();
mockGetClientInfoAndMetadata.mockRejectedValue(new Error('bad client data'));
mockDeleteUserTokens.mockResolvedValue(undefined);
mockDeleteFlow.mockResolvedValue(undefined);
await maybeUninstallOAuthMCP(userId, pluginKey, appConfig);
expect(mockGetTokens).not.toHaveBeenCalled();
expect(mockDeleteUserTokens).toHaveBeenCalledTimes(1);
expect(mockDeleteUserTokens.mock.calls[0][0]).toMatchObject({ userId, serverName });
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),
);
});
test('clears tenant-scoped and legacy flow state when tenant context exists', async () => {
setupOAuthServerFound();
mockGetTenantId.mockReturnValue('tenant-a');
mockGetClientInfoAndMetadata.mockResolvedValue(null);
await maybeUninstallOAuthMCP(userId, pluginKey, appConfig);
expect(mockDeleteFlow).toHaveBeenCalledTimes(2);
expect(mockDeleteFlow).toHaveBeenCalledWith('tenant:tenant-a:user-123:acme', 'mcp_get_tokens');
expect(mockDeleteFlow).toHaveBeenCalledWith('user-123:acme', 'mcp_get_tokens');
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 () => {
setupOAuthServerFound();
mockGetTokens.mockResolvedValue({
access_token: 'access-abc',
refresh_token: 'refresh-xyz',
});
mockRevokeOAuthToken.mockResolvedValue(undefined);
mockDeleteUserTokens.mockResolvedValue(undefined);
mockDeleteFlow.mockResolvedValue(undefined);
await maybeUninstallOAuthMCP(userId, pluginKey, appConfig);
expect(mockRevokeOAuthToken).toHaveBeenCalledTimes(2);
expect(mockRevokeOAuthToken.mock.calls[0][1]).toBe('access-abc');
expect(mockRevokeOAuthToken.mock.calls[0][2]).toBe('access');
expect(mockRevokeOAuthToken.mock.calls[1][1]).toBe('refresh-xyz');
expect(mockRevokeOAuthToken.mock.calls[1][2]).toBe('refresh');
expect(mockDeleteUserTokens).toHaveBeenCalledTimes(1);
expect(mockDeleteUserTokens.mock.calls[0][0]).toMatchObject({ userId, serverName });
expect(mockDeleteFlow).toHaveBeenCalledTimes(1);
expect(mockDeleteFlow.mock.calls[0][1]).toBe('mcp_get_tokens');
expect(mockDeleteFlowAndStateMapping).toHaveBeenCalledTimes(1);
expect(mockDeleteFlowAndStateMapping).toHaveBeenCalledWith('user-123:acme', expect.anything());
});
test('skips revocation but still runs cleanup when token retrieval fails', async () => {
setupOAuthServerFound();
mockGetTokens.mockRejectedValue(new Error('missing'));
mockDeleteUserTokens.mockResolvedValue(undefined);
mockDeleteFlow.mockResolvedValue(undefined);
await expect(maybeUninstallOAuthMCP(userId, pluginKey, appConfig)).resolves.toBeUndefined();
expect(mockRevokeOAuthToken).not.toHaveBeenCalled();
expect(mockDeleteUserTokens).toHaveBeenCalledTimes(1);
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),
);
});
test('skips revocation, logs warn, and still runs cleanup on unexpected token-retrieval error', async () => {
setupOAuthServerFound();
mockGetTokens.mockRejectedValue(new Error('boom: unreachable'));
mockDeleteUserTokens.mockResolvedValue(undefined);
mockDeleteFlow.mockResolvedValue(undefined);
await expect(maybeUninstallOAuthMCP(userId, pluginKey, appConfig)).resolves.toBeUndefined();
expect(mockRevokeOAuthToken).not.toHaveBeenCalled();
expect(mockDeleteUserTokens).toHaveBeenCalledTimes(1);
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),
);
});
test('continues cleanup when only one token type is present', async () => {
setupOAuthServerFound();
mockGetTokens.mockResolvedValue({ access_token: 'only-access' });
mockRevokeOAuthToken.mockResolvedValue(undefined);
mockDeleteUserTokens.mockResolvedValue(undefined);
mockDeleteFlow.mockResolvedValue(undefined);
await maybeUninstallOAuthMCP(userId, pluginKey, appConfig);
expect(mockRevokeOAuthToken).toHaveBeenCalledTimes(1);
expect(mockRevokeOAuthToken.mock.calls[0][2]).toBe('access');
expect(mockDeleteUserTokens).toHaveBeenCalledTimes(1);
expect(mockDeleteFlow).toHaveBeenCalledTimes(1);
expect(mockDeleteFlowAndStateMapping).toHaveBeenCalledTimes(1);
});
test('still runs cleanup even when both revocation calls fail', async () => {
setupOAuthServerFound();
mockGetTokens.mockResolvedValue({
access_token: 'a',
refresh_token: 'r',
});
mockRevokeOAuthToken.mockRejectedValue(new Error('network down'));
mockDeleteUserTokens.mockResolvedValue(undefined);
mockDeleteFlow.mockResolvedValue(undefined);
await expect(maybeUninstallOAuthMCP(userId, pluginKey, appConfig)).resolves.toBeUndefined();
expect(mockRevokeOAuthToken).toHaveBeenCalledTimes(2);
expect(mockDeleteUserTokens).toHaveBeenCalledTimes(1);
expect(mockDeleteFlow).toHaveBeenCalledTimes(1);
expect(mockDeleteFlowAndStateMapping).toHaveBeenCalledTimes(1);
expect(mockLoggerError).toHaveBeenCalled();
});
});