🧹 fix: Graceful MCP OAuth Revoke Cleanup When Tokens Are Missing (LibreChat-AI#12825)

* fix: graceful MCP OAuth revoke cleanup when tokens are missing (LibreChat-AI#12754)

`maybeUninstallOAuthMCP` in `api/server/controllers/UserController.js`
aborts before the DB-delete and flow-state cleanup steps whenever
`MCPTokenStorage.getTokens` throws `ReauthenticationRequiredError` —
which is exactly what happens when a user clicks "Revoke" on an MCP
server whose backend is already dead and whose refresh token is gone.
The resulting error is both surfaced to the log as a red line and, more
importantly, leaks the DB token row and OAuth flow state.

Wrap the token retrieval in try/catch following the same best-effort
pattern already used for the two `revokeOAuthToken` calls. On
`ReauthenticationRequiredError`, skip revocation silently (info log)
and continue to the cleanup steps. On any other unexpected error, log
a warning and continue — cleanup must always run.

Exported `maybeUninstallOAuthMCP` for direct unit testing and added
`api/server/controllers/__tests__/maybeUninstallOAuthMCP.spec.js` with
8 cases: early-return guards (non-MCP key, non-OAuth server, missing
client info), happy path (both tokens revoked + cleanup), both
failure-to-retrieve paths (ReauthenticationRequiredError and arbitrary
error — cleanup still runs in both), single-token path, and
revocation-call failures (cleanup still runs).

Fixes LibreChat-AI#12754.

* test: use instanceof check against real ReauthenticationRequiredError

Follow-up to the previous commit on this branch. Two changes:

1. `UserController.maybeUninstallOAuthMCP` now checks
   `error instanceof ReauthenticationRequiredError` using the real
   class imported from `@librechat/api`, instead of comparing
   `error?.name === 'ReauthenticationRequiredError'`. The name-string
   check matched any unrelated error that happened to have the same
   `.name`; the `instanceof` check is a proper identity test.

2. The accompanying spec's jest mock for `@librechat/api` now
   exposes a `ReauthenticationRequiredError` class, and the test
   imports it from that mock so the `instanceof` comparison in the
   production code holds during the test. Without this, the two
   "skips revocation ... still runs cleanup" tests threw
   `TypeError: Right-hand side of 'instanceof' is not an object`
   because the mock left the class undefined.

All 8 tests in the spec pass.

Author: gaurav0107

api/server/controllers/UserController.js

@@ -7,6 +7,7 @@ const {
   MCPTokenStorage,
   normalizeHttpError,
   extractWebSearchEnvVars,
+  ReauthenticationRequiredError,
 } = require('@librechat/api');
 const {
   Tools,
@@ -405,12 +406,32 @@ const maybeUninstallOAuthMCP = async (userId, pluginKey, appConfig) => {
   }
   const { clientInfo, clientMetadata } = clientTokenData;
 
-  // 2. get decrypted tokens before deletion
-  const tokens = await MCPTokenStorage.getTokens({
-    userId,
-    serverName,
-    findToken: db.findToken,
-  });
+  // 2. get decrypted tokens before deletion.
+  //    Token retrieval can throw ReauthenticationRequiredError (or other
+  //    errors) when the refresh token is missing/expired — exactly the
+  //    state that triggers a user-initiated revoke. Swallow it here so
+  //    the DB and flow-state cleanup below always runs. Revocation is
+  //    best-effort and the individual calls already wrap their own
+  //    try/catch.
+  let tokens = null;
+  try {
+    tokens = await MCPTokenStorage.getTokens({
+      userId,
+      serverName,
+      findToken: db.findToken,
+    });
+  } catch (error) {
+    if (error instanceof ReauthenticationRequiredError) {
+      logger.info(
+        `[maybeUninstallOAuthMCP] No usable tokens for ${serverName} — skipping revocation, continuing cleanup`,
+      );
+    } else {
+      logger.warn(
+        `[maybeUninstallOAuthMCP] Unexpected error retrieving tokens for ${serverName}:`,
+        error,
+      );
+    }
+  }
 
   // 3. revoke OAuth tokens at the provider
   const revocationEndpoint =
@@ -489,4 +510,5 @@ module.exports = {
   updateUserPluginsController,
   resendVerificationController,
   deleteUserMcpServers,
+  maybeUninstallOAuthMCP,
 };

api/server/controllers/__tests__/maybeUninstallOAuthMCP.spec.js

@@ -0,0 +1,281 @@
+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 mockDeleteFlow = 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();
+
+jest.mock('@librechat/data-schemas', () => ({
+  logger: { info: mockLoggerInfo, warn: mockLoggerWarn, error: mockLoggerError },
+  webSearchKeys: [],
+}));
+
+jest.mock('@librechat/api', () => {
+  class ReauthenticationRequiredError extends Error {
+    constructor(serverName, reason) {
+      super(`Re-authentication required for "${serverName}": ${reason}`);
+      this.name = 'ReauthenticationRequiredError';
+    }
+  }
+  return {
+    MCPOAuthHandler: {
+      revokeOAuthToken: (...args) => mockRevokeOAuthToken(...args),
+      generateFlowId: (userId, serverName) => `${userId}:${serverName}`,
+    },
+    MCPTokenStorage: {
+      getTokens: (...args) => mockGetTokens(...args),
+      getClientInfoAndMetadata: (...args) => mockGetClientInfoAndMetadata(...args),
+      deleteUserTokens: (...args) => mockDeleteUserTokens(...args),
+    },
+    ReauthenticationRequiredError,
+    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),
+  })),
+}));
+
+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(),
+}));
+
+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 { ReauthenticationRequiredError } = require('@librechat/api');
+
+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']);
+  mockGetClientInfoAndMetadata.mockResolvedValue({ clientInfo, clientMetadata });
+}
+
+describe('maybeUninstallOAuthMCP', () => {
+  beforeEach(() => {
+    jest.clearAllMocks();
+  });
+
+  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();
+  });
+
+  test('is a no-op 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).not.toHaveBeenCalled();
+  });
+
+  test('returns early when client info is missing', async () => {
+    setupOAuthServerFound();
+    mockGetClientInfoAndMetadata.mockResolvedValue(null);
+
+    await maybeUninstallOAuthMCP(userId, pluginKey, appConfig);
+
+    expect(mockGetTokens).not.toHaveBeenCalled();
+    expect(mockDeleteUserTokens).not.toHaveBeenCalled();
+    expect(mockDeleteFlow).not.toHaveBeenCalled();
+  });
+
+  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(2);
+    expect(mockDeleteFlow.mock.calls[0][1]).toBe('mcp_get_tokens');
+    expect(mockDeleteFlow.mock.calls[1][1]).toBe('mcp_oauth');
+  });
+
+  test('skips revocation but still runs cleanup when getTokens throws ReauthenticationRequiredError', async () => {
+    setupOAuthServerFound();
+    mockGetTokens.mockRejectedValue(new ReauthenticationRequiredError(serverName, '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(2);
+    expect(mockLoggerInfo).toHaveBeenCalledWith(expect.stringContaining('No usable tokens'));
+  });
+
+  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(2);
+    expect(mockLoggerWarn).toHaveBeenCalledWith(
+      expect.stringContaining('Unexpected error retrieving tokens'),
+      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(2);
+  });
+
+  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(2);
+    expect(mockLoggerError).toHaveBeenCalled();
+  });
+});