fix: validate auth server identity and target cleanup to reused clients

- Gate client reuse on authorization server identity: compare stored
  issuer against freshly discovered metadata before reusing, preventing
  wrong-client reuse when the MCP server switches auth providers
- Add reusedStoredClient flag to MCPOAuthFlowMetadata so cleanup only
  runs when the failed flow actually reused a stored registration,
  not on unrelated failures (timeouts, user-denied consent, etc.)
- Add cleanup in returnOnOAuth path: when a prior flow that reused a
  stored client is detected as failed, clear the stale registration
  before re-initiating
- Add tests for issuer mismatch and reusedStoredClient flag assertions
This commit is contained in:
Danny Avila 2026-04-03 15:47:54 -04:00
parent d355be7dd0
commit 978ce2b4eb
4 changed files with 83 additions and 14 deletions

View file

@ -355,7 +355,17 @@ export class MCPConnectionFactory {
); );
if (existingFlow) { if (existingFlow) {
const oldState = (existingFlow.metadata as MCPOAuthFlowMetadata)?.state; const oldMeta = existingFlow.metadata as MCPOAuthFlowMetadata | undefined;
if (oldMeta?.reusedStoredClient && this.tokenMethods?.deleteTokens) {
await MCPTokenStorage.deleteClientRegistration({
userId: this.userId!,
serverName: this.serverName,
deleteTokens: this.tokenMethods.deleteTokens,
}).catch((err) => {
logger.debug(`${this.logPrefix} Failed to clear stale client registration`, err);
});
}
const oldState = oldMeta?.state;
await this.flowManager!.deleteFlow(newFlowId, 'mcp_oauth'); await this.flowManager!.deleteFlow(newFlowId, 'mcp_oauth');
if (oldState) { if (oldState) {
await MCPOAuthHandler.deleteStateMapping(oldState, this.flowManager!); await MCPOAuthHandler.deleteStateMapping(oldState, this.flowManager!);
@ -413,16 +423,21 @@ export class MCPConnectionFactory {
if (result?.tokens) { if (result?.tokens) {
connection.emit('oauthHandled'); connection.emit('oauthHandled');
} else { } else {
// OAuth failed — clear stored client registration so the next attempt // OAuth failed — if we reused a stored client registration, clear it
// does a fresh DCR instead of reusing a potentially stale client_id // so the next attempt falls through to fresh DCR
if (this.tokenMethods?.deleteTokens) { if (result?.clientInfo && this.tokenMethods?.deleteTokens) {
await MCPTokenStorage.deleteClientRegistration({ const flowId = MCPOAuthHandler.generateFlowId(this.userId!, this.serverName);
userId: this.userId!, const failedFlow = await this.flowManager?.getFlowState(flowId, 'mcp_oauth');
serverName: this.serverName, const failedMeta = failedFlow?.metadata as MCPOAuthFlowMetadata | undefined;
deleteTokens: this.tokenMethods.deleteTokens, if (failedMeta?.reusedStoredClient) {
}).catch((err) => { await MCPTokenStorage.deleteClientRegistration({
logger.debug(`${this.logPrefix} Failed to clear stale client registration`, err); userId: this.userId!,
}); serverName: this.serverName,
deleteTokens: this.tokenMethods.deleteTokens,
}).catch((err) => {
logger.debug(`${this.logPrefix} Failed to clear stale client registration`, err);
});
}
} }
logger.warn(`${this.logPrefix} OAuth failed, emitting oauthFailed event`); logger.warn(`${this.logPrefix} OAuth failed, emitting oauthFailed event`);
connection.emit('oauthFailed', new Error('OAuth authentication failed')); connection.emit('oauthFailed', new Error('OAuth authentication failed'));

View file

@ -145,6 +145,7 @@ describe('MCPOAuthHandler - client registration reuse on reconnection', () => {
expect(server.registeredClients.size).toBe(1); expect(server.registeredClients.size).toBe(1);
expect(secondResult.flowMetadata.clientInfo?.client_id).toBe(firstClientId); expect(secondResult.flowMetadata.clientInfo?.client_id).toBe(firstClientId);
expect(secondResult.flowMetadata.reusedStoredClient).toBe(true);
}); });
it('should reuse the same client when two reconnections fire concurrently with pre-seeded token', async () => { it('should reuse the same client when two reconnections fire concurrently with pre-seeded token', async () => {
@ -315,10 +316,11 @@ describe('MCPOAuthHandler - client registration reuse on reconnection', () => {
expect(firstResult.flowMetadata.clientInfo?.client_id).toBe( expect(firstResult.flowMetadata.clientInfo?.client_id).toBe(
'stale-client-that-oauth-server-deleted', 'stale-client-that-oauth-server-deleted',
); );
expect(firstResult.flowMetadata.reusedStoredClient).toBe(true);
expect(server.registeredClients.size).toBe(0); expect(server.registeredClients.size).toBe(0);
// Simulate flow failure: the OAuth server rejected the stale client_id, // Simulate what MCPConnectionFactory does on failure when reusedStoredClient is set:
// so the operator clears the stored client registration // clear the stored client registration so the next attempt does a fresh DCR
await MCPTokenStorage.deleteClientRegistration({ await MCPTokenStorage.deleteClientRegistration({
userId: 'user-1', userId: 'user-1',
serverName: 'test-server', serverName: 'test-server',
@ -336,11 +338,51 @@ describe('MCPOAuthHandler - client registration reuse on reconnection', () => {
tokenStore.findToken, tokenStore.findToken,
); );
// Now it registered a new client
expect(server.registeredClients.size).toBe(1); expect(server.registeredClients.size).toBe(1);
expect(secondResult.flowMetadata.clientInfo?.client_id).not.toBe( expect(secondResult.flowMetadata.clientInfo?.client_id).not.toBe(
'stale-client-that-oauth-server-deleted', 'stale-client-that-oauth-server-deleted',
); );
expect(secondResult.flowMetadata.reusedStoredClient).toBeUndefined();
});
it('should re-register when stored client was issued by a different authorization server', async () => {
server = await createOAuthMCPServer({ tokenTTLMs: 60000 });
const tokenStore = new InMemoryTokenStore();
// Seed a stored client that was registered with a different issuer
await MCPTokenStorage.storeTokens({
userId: 'user-1',
serverName: 'test-server',
tokens: { access_token: 'old-token', token_type: 'Bearer' },
createToken: tokenStore.createToken,
updateToken: tokenStore.updateToken,
findToken: tokenStore.findToken,
clientInfo: {
client_id: 'old-issuer-client',
client_secret: 'secret',
redirect_uris: ['http://localhost:3080/api/mcp/test-server/oauth/callback'],
} as OAuthClientInformation & { redirect_uris: string[] },
metadata: {
issuer: 'https://old-auth-server.example.com',
authorization_endpoint: 'https://old-auth-server.example.com/authorize',
token_endpoint: 'https://old-auth-server.example.com/token',
},
});
const result = await MCPOAuthHandler.initiateOAuthFlow(
'test-server',
server.url,
'user-1',
{},
undefined,
undefined,
tokenStore.findToken,
);
// Should have registered a NEW client because the issuer changed
expect(server.registeredClients.size).toBe(1);
expect(result.flowMetadata.clientInfo?.client_id).not.toBe('old-issuer-client');
expect(result.flowMetadata.reusedStoredClient).toBeUndefined();
}); });
it('should not call getClientInfoAndMetadata when findToken is not provided', async () => { it('should not call getClientInfoAndMetadata when findToken is not provided', async () => {

View file

@ -500,6 +500,7 @@ export class MCPOAuthHandler {
logger.debug(`[MCPOAuth] Resolving OAuth client with redirect URI: ${redirectUri}`); logger.debug(`[MCPOAuth] Resolving OAuth client with redirect URI: ${redirectUri}`);
let clientInfo: OAuthClientInformation | undefined; let clientInfo: OAuthClientInformation | undefined;
let reusedStoredClient = false;
if (findToken) { if (findToken) {
try { try {
@ -511,15 +512,23 @@ export class MCPOAuthHandler {
if (existing?.clientInfo?.client_id) { if (existing?.clientInfo?.client_id) {
const storedRedirectUri = (existing.clientInfo as OAuthClientInformation) const storedRedirectUri = (existing.clientInfo as OAuthClientInformation)
.redirect_uris?.[0]; .redirect_uris?.[0];
const storedIssuer = (existing.clientMetadata as Record<string, unknown>)?.issuer;
const currentIssuer = metadata.issuer ?? authServerUrl.toString();
if (!storedRedirectUri || storedRedirectUri !== redirectUri) { if (!storedRedirectUri || storedRedirectUri !== redirectUri) {
logger.debug( logger.debug(
`[MCPOAuth] Stored redirect_uri "${storedRedirectUri}" differs from current "${redirectUri}", will re-register`, `[MCPOAuth] Stored redirect_uri "${storedRedirectUri}" differs from current "${redirectUri}", will re-register`,
); );
} else if (storedIssuer && storedIssuer !== currentIssuer) {
logger.debug(
`[MCPOAuth] Stored issuer "${storedIssuer}" differs from current "${currentIssuer}", will re-register`,
);
} else { } else {
logger.debug( logger.debug(
`[MCPOAuth] Reusing existing client registration: ${existing.clientInfo.client_id}`, `[MCPOAuth] Reusing existing client registration: ${existing.clientInfo.client_id}`,
); );
clientInfo = existing.clientInfo; clientInfo = existing.clientInfo;
reusedStoredClient = true;
} }
} }
} catch (error) { } catch (error) {
@ -610,6 +619,7 @@ export class MCPOAuthHandler {
metadata, metadata,
resourceMetadata, resourceMetadata,
...(Object.keys(oauthHeaders).length > 0 && { oauthHeaders }), ...(Object.keys(oauthHeaders).length > 0 && { oauthHeaders }),
...(reusedStoredClient && { reusedStoredClient }),
}; };
logger.debug( logger.debug(

View file

@ -91,6 +91,8 @@ export interface MCPOAuthFlowMetadata extends FlowMetadata {
authorizationUrl?: string; authorizationUrl?: string;
/** Custom headers for OAuth token exchange, persisted at flow initiation for the callback. */ /** Custom headers for OAuth token exchange, persisted at flow initiation for the callback. */
oauthHeaders?: Record<string, string>; oauthHeaders?: Record<string, string>;
/** True when the flow reused a stored client registration from a prior successful OAuth flow */
reusedStoredClient?: boolean;
} }
export interface MCPOAuthTokens extends OAuthTokens { export interface MCPOAuthTokens extends OAuthTokens {