fix: address follow-up review findings R1, R2, R3

- R1: Move `import type { TokenMethods }` to the type-imports section,
  before local types, per CLAUDE.md import order rules
- R2: Add unit test for empty redirect_uris in handler.test.ts to
  verify the inverted condition triggers re-registration
- R3: Use delete for process.env.DOMAIN_SERVER restoration when the
  original value was undefined to avoid coercion to string "undefined"
This commit is contained in:
Danny Avila 2026-04-03 15:20:10 -04:00
parent 83ba37853b
commit 20a08e1904
3 changed files with 67 additions and 2 deletions

View file

@ -62,7 +62,11 @@ describe('MCPOAuthHandler - client registration reuse on reconnection', () => {
});
afterEach(async () => {
process.env.DOMAIN_SERVER = originalDomainServer;
if (originalDomainServer !== undefined) {
process.env.DOMAIN_SERVER = originalDomainServer;
} else {
delete process.env.DOMAIN_SERVER;
}
if (server) {
await server.close();
}

View file

@ -1681,6 +1681,67 @@ describe('MCPOAuthHandler - Configurable OAuth Metadata', () => {
}),
);
});
it('should re-register when stored client has empty redirect_uris', async () => {
const existingClientInfo = {
client_id: 'empty-redirect-client',
client_secret: 'secret',
redirect_uris: [],
};
mockGetClientInfoAndMetadata.mockResolvedValueOnce({
clientInfo: existingClientInfo,
clientMetadata: {},
});
mockDiscoverOAuthProtectedResourceMetadata.mockRejectedValueOnce(
new Error('No resource metadata'),
);
mockDiscoverAuthorizationServerMetadata.mockResolvedValueOnce({
issuer: 'https://example.com',
authorization_endpoint: 'https://example.com/authorize',
token_endpoint: 'https://example.com/token',
registration_endpoint: 'https://example.com/register',
response_types_supported: ['code'],
jwks_uri: 'https://example.com/.well-known/jwks.json',
subject_types_supported: ['public'],
id_token_signing_alg_values_supported: ['RS256'],
} as AuthorizationServerMetadata);
mockRegisterClient.mockResolvedValueOnce({
client_id: 'new-client-id',
client_secret: 'new-client-secret',
redirect_uris: ['http://localhost:3080/api/mcp/test-server/oauth/callback'],
logo_uri: undefined,
tos_uri: undefined,
});
mockStartAuthorization.mockResolvedValueOnce({
authorizationUrl: new URL('https://example.com/authorize?client_id=new-client-id'),
codeVerifier: 'test-code-verifier',
});
await MCPOAuthHandler.initiateOAuthFlow(
'test-server',
'https://example.com/mcp',
'user-123',
{},
undefined,
undefined,
mockFindToken,
);
expect(mockRegisterClient).toHaveBeenCalled();
expect(mockStartAuthorization).toHaveBeenCalledWith(
'https://example.com/mcp',
expect.objectContaining({
clientInformation: expect.objectContaining({
client_id: 'new-client-id',
}),
}),
);
});
});
describe('Fallback OAuth Metadata (Legacy Server Support)', () => {

View file

@ -10,6 +10,7 @@ import {
discoverOAuthProtectedResourceMetadata,
} from '@modelcontextprotocol/sdk/client/auth.js';
import { TokenExchangeMethodEnum, type MCPOptions } from 'librechat-data-provider';
import type { TokenMethods } from '@librechat/data-schemas';
import type { FlowStateManager } from '~/flow/manager';
import type {
OAuthClientInformation,
@ -24,7 +25,6 @@ import {
selectRegistrationAuthMethod,
inferClientAuthMethod,
} from './methods';
import type { TokenMethods } from '@librechat/data-schemas';
import { isSSRFTarget, resolveHostnameSSRF, isOAuthUrlAllowed } from '~/auth';
import { MCPTokenStorage } from './tokens';
import { sanitizeUrlForLogging } from '~/mcp/utils';