mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-04 14:57:42 +00:00
🔑 fix: Clear Stale Client Registration on invalid_client During OAuth Token Refresh (#12643)
* 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 <mani@muonspace.com>
This commit is contained in:
Danny Avila
GitHub
parent
7b48203906
commit
c4bb41137d
5 changed files with 302 additions and 14 deletions
|
|
@ -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;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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', () => {
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -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<MCPOAuthTokens>;
|
||||
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<MCPOAuthTokens | null> {
|
||||
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;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue