From c4bb41137d92ef6e3846c83ea626a7779e30db42 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 13 Apr 2026 10:12:54 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=91=20fix:=20Clear=20Stale=20Client=20?= =?UTF-8?q?Registration=20on=20`invalid=5Fclient`=20During=20OAuth=20Token?= =?UTF-8?q?=20Refresh=20(#12643)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: clear stale client registration on invalid_client during token refresh When a token refresh fails with `invalid_client`, the stored DCR client registration is no longer valid on the authorization server. The existing error handler only checked for `unauthorized_client` and returned null, leaving the stale client_id cached in the database permanently. Every subsequent token refresh attempt would fail with the same error. Now when `invalid_client` is detected during refresh: 1. The stale client registration is deleted from the database 2. A `ReauthenticationRequiredError` is thrown to trigger a fresh OAuth flow with new dynamic client registration Also passes `deleteTokens` from MCPConnectionFactory to getTokens() so the cleanup has access to the token deletion method. * fix: address review findings for stale client cleanup on token refresh - Delete stale refresh token alongside client registration on invalid_client (Finding 1) - Add tests for all new code paths: cleanup, warning, case-insensitivity, cleanup failure (Finding 2) - Detect all vendor-specific client rejection patterns (client_id mismatch, client not found, unknown client) with case-insensitive matching (Finding 3) - Use else-if for mutually exclusive error branches (Finding 4) - Log warning when deleteTokens is not available on client rejection (Finding 6) - Fix log message to say "attempting to clear" before async cleanup (Finding 7) - Extract isClientRejectionMessage to shared utility, refactor MCPConnectionFactory.isClientRejection to use it * fix: address followup review findings - Extract isInvalidClientMessage (4 stale-client patterns) from isClientRejectionMessage to eliminate pattern duplication between utils.ts and tokens.ts (Finding 1) - Remove redundant staleIdentifier variable, reuse identifier already in scope (Finding 2) - Separate await from .then() on Promise.allSettled for readability (Finding 3) - Add dedicated unit tests for isInvalidClientMessage and isClientRejectionMessage (Finding 4) - Assert error message content in primary invalid_client test (Finding 5) - Add JSDoc on deleteTokens in GetTokensParams (Finding 6) --------- Co-authored-by: Mani Japra --- packages/api/src/mcp/MCPConnectionFactory.ts | 12 +- .../__tests__/MCPOAuthTokenStorage.test.ts | 203 ++++++++++++++++++ packages/api/src/mcp/__tests__/utils.test.ts | 40 ++++ packages/api/src/mcp/oauth/tokens.ts | 39 +++- packages/api/src/mcp/utils.ts | 22 ++ 5 files changed, 302 insertions(+), 14 deletions(-) diff --git a/packages/api/src/mcp/MCPConnectionFactory.ts b/packages/api/src/mcp/MCPConnectionFactory.ts index c517140b8a..c308fee72e 100644 --- a/packages/api/src/mcp/MCPConnectionFactory.ts +++ b/packages/api/src/mcp/MCPConnectionFactory.ts @@ -7,7 +7,7 @@ import type { FlowStateManager } from '~/flow/manager'; import type * as t from './types'; import { MCPTokenStorage, MCPOAuthHandler, ReauthenticationRequiredError } from '~/mcp/oauth'; import { PENDING_STALE_MS, normalizeExpiresAt } from '~/flow/manager'; -import { sanitizeUrlForLogging } from './utils'; +import { sanitizeUrlForLogging, isClientRejectionMessage } from './utils'; import { withTimeout } from '~/utils/promise'; import { MCPConnection } from './connection'; import { processMCPEnv } from '~/utils'; @@ -264,6 +264,7 @@ export class MCPConnectionFactory { findToken: this.tokenMethods!.findToken!, createToken: this.tokenMethods!.createToken, updateToken: this.tokenMethods!.updateToken, + deleteTokens: this.tokenMethods!.deleteTokens, refreshTokens: this.createRefreshTokensFunction(), }); }, @@ -503,14 +504,7 @@ export class MCPConnectionFactory { return false; } if ('message' in error && typeof error.message === 'string') { - const msg = error.message.toLowerCase(); - return ( - msg.includes('invalid_client') || - msg.includes('unauthorized_client') || - msg.includes('client_id mismatch') || - msg.includes('client not found') || - msg.includes('unknown client') - ); + return isClientRejectionMessage(error.message); } return false; } diff --git a/packages/api/src/mcp/__tests__/MCPOAuthTokenStorage.test.ts b/packages/api/src/mcp/__tests__/MCPOAuthTokenStorage.test.ts index 2d3905d2fb..a1db56f95d 100644 --- a/packages/api/src/mcp/__tests__/MCPOAuthTokenStorage.test.ts +++ b/packages/api/src/mcp/__tests__/MCPOAuthTokenStorage.test.ts @@ -511,6 +511,209 @@ describe('MCPTokenStorage', () => { expect.stringContaining('does not support refresh tokens'), ); }); + + it('should delete client registration and refresh token on invalid_client when deleteTokens provided', async () => { + await store.createToken({ + userId: 'u1', + type: 'mcp_oauth', + identifier: 'mcp:srv1', + token: 'enc:expired-token', + expiresIn: -1, + }); + await store.createToken({ + userId: 'u1', + type: 'mcp_oauth_refresh', + identifier: 'mcp:srv1:refresh', + token: 'enc:rt', + expiresIn: 86400, + }); + await store.createToken({ + userId: 'u1', + type: 'mcp_oauth_client', + identifier: 'mcp:srv1:client', + token: 'enc:{"client_id":"cid"}', + expiresIn: 86400, + }); + + const refreshTokens = jest.fn().mockRejectedValue(new Error('invalid_client')); + + await expect( + MCPTokenStorage.getTokens({ + userId: 'u1', + serverName: 'srv1', + findToken: store.findToken, + createToken: store.createToken, + deleteTokens: store.deleteTokens, + refreshTokens, + }), + ).rejects.toThrow( + expect.objectContaining({ + name: 'ReauthenticationRequiredError', + message: expect.stringContaining('stored client registration is no longer valid'), + }), + ); + + const clientReg = await store.findToken({ + userId: 'u1', + type: 'mcp_oauth_client', + identifier: 'mcp:srv1:client', + }); + expect(clientReg).toBeNull(); + + const refreshToken = await store.findToken({ + userId: 'u1', + type: 'mcp_oauth_refresh', + identifier: 'mcp:srv1:refresh', + }); + expect(refreshToken).toBeNull(); + }); + + it('should return null and log warning on invalid_client when deleteTokens not provided', async () => { + const { logger } = await import('@librechat/data-schemas'); + + await store.createToken({ + userId: 'u1', + type: 'mcp_oauth', + identifier: 'mcp:srv1', + token: 'enc:expired-token', + expiresIn: -1, + }); + await store.createToken({ + userId: 'u1', + type: 'mcp_oauth_refresh', + identifier: 'mcp:srv1:refresh', + token: 'enc:rt', + expiresIn: 86400, + }); + + const refreshTokens = jest.fn().mockRejectedValue(new Error('invalid_client')); + + const result = await MCPTokenStorage.getTokens({ + userId: 'u1', + serverName: 'srv1', + findToken: store.findToken, + createToken: store.createToken, + refreshTokens, + }); + + expect(result).toBeNull(); + expect(logger.warn).toHaveBeenCalledWith( + expect.stringContaining('deleteTokens not available'), + ); + }); + + it('should handle client_not_found and other vendor-specific rejection patterns', async () => { + await store.createToken({ + userId: 'u1', + type: 'mcp_oauth', + identifier: 'mcp:srv1', + token: 'enc:expired-token', + expiresIn: -1, + }); + await store.createToken({ + userId: 'u1', + type: 'mcp_oauth_refresh', + identifier: 'mcp:srv1:refresh', + token: 'enc:rt', + expiresIn: 86400, + }); + await store.createToken({ + userId: 'u1', + type: 'mcp_oauth_client', + identifier: 'mcp:srv1:client', + token: 'enc:{"client_id":"cid"}', + expiresIn: 86400, + }); + + const refreshTokens = jest.fn().mockRejectedValue(new Error('client not found')); + + await expect( + MCPTokenStorage.getTokens({ + userId: 'u1', + serverName: 'srv1', + findToken: store.findToken, + createToken: store.createToken, + deleteTokens: store.deleteTokens, + refreshTokens, + }), + ).rejects.toThrow(ReauthenticationRequiredError); + + expect( + await store.findToken({ + userId: 'u1', + type: 'mcp_oauth_client', + identifier: 'mcp:srv1:client', + }), + ).toBeNull(); + expect( + await store.findToken({ + userId: 'u1', + type: 'mcp_oauth_refresh', + identifier: 'mcp:srv1:refresh', + }), + ).toBeNull(); + }); + + it('should handle case-insensitive error messages for client rejection', async () => { + await store.createToken({ + userId: 'u1', + type: 'mcp_oauth', + identifier: 'mcp:srv1', + token: 'enc:expired-token', + expiresIn: -1, + }); + await store.createToken({ + userId: 'u1', + type: 'mcp_oauth_refresh', + identifier: 'mcp:srv1:refresh', + token: 'enc:rt', + expiresIn: 86400, + }); + + const refreshTokens = jest.fn().mockRejectedValue(new Error('INVALID_CLIENT')); + + await expect( + MCPTokenStorage.getTokens({ + userId: 'u1', + serverName: 'srv1', + findToken: store.findToken, + createToken: store.createToken, + deleteTokens: store.deleteTokens, + refreshTokens, + }), + ).rejects.toThrow(ReauthenticationRequiredError); + }); + + it('should still throw ReauthenticationRequiredError when deleteClientRegistration fails', async () => { + await store.createToken({ + userId: 'u1', + type: 'mcp_oauth', + identifier: 'mcp:srv1', + token: 'enc:expired-token', + expiresIn: -1, + }); + await store.createToken({ + userId: 'u1', + type: 'mcp_oauth_refresh', + identifier: 'mcp:srv1:refresh', + token: 'enc:rt', + expiresIn: 86400, + }); + + const refreshTokens = jest.fn().mockRejectedValue(new Error('invalid_client')); + const failingDeleteTokens = jest.fn().mockRejectedValue(new Error('DB connection lost')); + + await expect( + MCPTokenStorage.getTokens({ + userId: 'u1', + serverName: 'srv1', + findToken: store.findToken, + createToken: store.createToken, + deleteTokens: failingDeleteTokens, + refreshTokens, + }), + ).rejects.toThrow(ReauthenticationRequiredError); + }); }); describe('storeTokens + getTokens round-trip', () => { diff --git a/packages/api/src/mcp/__tests__/utils.test.ts b/packages/api/src/mcp/__tests__/utils.test.ts index b9c2a31fa5..19f9c1eaf7 100644 --- a/packages/api/src/mcp/__tests__/utils.test.ts +++ b/packages/api/src/mcp/__tests__/utils.test.ts @@ -3,6 +3,8 @@ import { normalizeServerName, redactAllServerSecrets, redactServerSecrets, + isInvalidClientMessage, + isClientRejectionMessage, isUserSourced, } from '~/mcp/utils'; import type { ParsedServerConfig } from '~/mcp/types'; @@ -275,6 +277,44 @@ describe('redactAllServerSecrets', () => { }); }); +describe('isInvalidClientMessage', () => { + it.each(['invalid_client', 'client_id mismatch', 'client not found', 'unknown client'])( + 'should detect "%s"', + (pattern) => { + expect(isInvalidClientMessage(`OAuth error: ${pattern}`)).toBe(true); + }, + ); + + it('should be case-insensitive', () => { + expect(isInvalidClientMessage('INVALID_CLIENT')).toBe(true); + expect(isInvalidClientMessage('Client Not Found')).toBe(true); + }); + + it('should not match unauthorized_client', () => { + expect(isInvalidClientMessage('unauthorized_client')).toBe(false); + }); + + it('should return false for unrelated messages', () => { + expect(isInvalidClientMessage('connection timeout')).toBe(false); + expect(isInvalidClientMessage('')).toBe(false); + }); +}); + +describe('isClientRejectionMessage', () => { + it('should match all isInvalidClientMessage patterns', () => { + expect(isClientRejectionMessage('invalid_client')).toBe(true); + expect(isClientRejectionMessage('client not found')).toBe(true); + }); + + it('should also match unauthorized_client', () => { + expect(isClientRejectionMessage('unauthorized_client')).toBe(true); + }); + + it('should return false for unrelated messages', () => { + expect(isClientRejectionMessage('server error')).toBe(false); + }); +}); + describe('isUserSourced', () => { it('returns false when source is yaml', () => { expect(isUserSourced({ source: 'yaml' })).toBe(false); diff --git a/packages/api/src/mcp/oauth/tokens.ts b/packages/api/src/mcp/oauth/tokens.ts index 61b442ca8c..5476cc302b 100644 --- a/packages/api/src/mcp/oauth/tokens.ts +++ b/packages/api/src/mcp/oauth/tokens.ts @@ -2,13 +2,16 @@ import { logger, encryptV2, decryptV2 } from '@librechat/data-schemas'; import type { OAuthTokens, OAuthClientInformation } from '@modelcontextprotocol/sdk/shared/auth.js'; import type { TokenMethods, IToken } from '@librechat/data-schemas'; import type { MCPOAuthTokens, ExtendedOAuthTokens, OAuthMetadata } from './types'; +import { isInvalidClientMessage } from '~/mcp/utils'; import { isSystemUserId } from '~/mcp/enum'; export class ReauthenticationRequiredError extends Error { - constructor(serverName: string, reason: 'expired' | 'missing') { - super( - `Re-authentication required for "${serverName}": access token ${reason} and no refresh token available`, - ); + constructor(serverName: string, reason: 'expired' | 'missing' | 'invalid_client') { + const detail = + reason === 'invalid_client' + ? 'stored client registration is no longer valid' + : `access token ${reason} and no refresh token available`; + super(`Re-authentication required for "${serverName}": ${detail}`); this.name = 'ReauthenticationRequiredError'; } } @@ -47,6 +50,8 @@ interface GetTokensParams { ) => Promise; createToken?: TokenMethods['createToken']; updateToken?: TokenMethods['updateToken']; + /** Enables cleanup of stale client registration and refresh token on invalid_client errors during refresh. */ + deleteTokens?: TokenMethods['deleteTokens']; } export class MCPTokenStorage { @@ -254,6 +259,7 @@ export class MCPTokenStorage { findToken, createToken, updateToken, + deleteTokens, refreshTokens, }: GetTokensParams): Promise { const logPrefix = this.getLogPrefix(userId, serverName); @@ -385,10 +391,33 @@ export class MCPTokenStorage { // Check if it's an unauthorized_client error (refresh not supported) const errorMessage = refreshError instanceof Error ? refreshError.message : String(refreshError); - if (errorMessage.includes('unauthorized_client')) { + if (errorMessage.toLowerCase().includes('unauthorized_client')) { logger.info( `${logPrefix} Server does not support refresh tokens for this client. New authentication required.`, ); + } else if (isInvalidClientMessage(errorMessage)) { + if (deleteTokens) { + logger.info( + `${logPrefix} Client registration rejected during token refresh, attempting to clear stale registration and refresh token`, + ); + const results = await Promise.allSettled([ + MCPTokenStorage.deleteClientRegistration({ userId, serverName, deleteTokens }), + deleteTokens({ + userId, + type: 'mcp_oauth_refresh', + identifier: `${identifier}:refresh`, + }), + ]); + for (const r of results) { + if (r.status === 'rejected') { + logger.warn(`${logPrefix} Failed to clear stale token data`, r.reason); + } + } + throw new ReauthenticationRequiredError(serverName, 'invalid_client'); + } + logger.warn( + `${logPrefix} Client registration rejected during token refresh but deleteTokens not available — stale registration cannot be cleared`, + ); } return null; } diff --git a/packages/api/src/mcp/utils.ts b/packages/api/src/mcp/utils.ts index 653a96d5bd..9befc3c549 100644 --- a/packages/api/src/mcp/utils.ts +++ b/packages/api/src/mcp/utils.ts @@ -124,6 +124,28 @@ export function buildOAuthToolCallName(serverName: string): string { return `${oauthPrefix}${normalizeServerName(serverName)}`; } +const INVALID_CLIENT_PATTERNS = [ + 'invalid_client', + 'client_id mismatch', + 'client not found', + 'unknown client', +] as const; + +/** Checks whether a message indicates the stored client registration is invalid/stale. */ +export function isInvalidClientMessage(message: string): boolean { + const msg = message.toLowerCase(); + return INVALID_CLIENT_PATTERNS.some((p) => msg.includes(p)); +} + +/** + * Checks whether a message indicates the OAuth client registration was rejected. + * Superset of `isInvalidClientMessage`: also matches `unauthorized_client` + * (grant-type refusal), which has different recovery semantics. + */ +export function isClientRejectionMessage(message: string): boolean { + return isInvalidClientMessage(message) || message.toLowerCase().includes('unauthorized_client'); +} + /** * Sanitizes a URL by removing query parameters to prevent credential leakage in logs. * @param url - The URL to sanitize (string or URL object)