From 68ea22813c2cd0078cc32e83eaa88da6f83a956b Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Fri, 3 Apr 2026 18:48:06 -0400 Subject: [PATCH] fix: issuer validation, callback error propagation, and cleanup DRY MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Issuer check: re-register when storedIssuer is absent or non-string instead of silently reusing. Narrows unknown type with typeof guard and inverts condition so missing issuer → fresh DCR (safer default). - OAuth callback route: call failFlow with the OAuth error when the authorization server redirects back with error= parameter, so the waiting flow receives the actual rejection instead of timing out. This lets isClientRejection match stale-client errors correctly. - Extract duplicated cleanup block to clearStaleClientIfRejected() private method, called from both returnOnOAuth and blocking paths. - Test fixes: add issuer to stored metadata in reuse tests, reset server to undefined in afterEach to prevent double-close. --- api/server/routes/mcp.js | 16 ++++++ packages/api/src/mcp/MCPConnectionFactory.ts | 50 ++++++++----------- .../MCPOAuthClientRegistrationReuse.test.ts | 12 ++++- .../api/src/mcp/__tests__/handler.test.ts | 2 +- packages/api/src/mcp/oauth/handler.ts | 9 ++-- 5 files changed, 55 insertions(+), 34 deletions(-) diff --git a/api/server/routes/mcp.js b/api/server/routes/mcp.js index c6496ad4b4..6fd5a6634d 100644 --- a/api/server/routes/mcp.js +++ b/api/server/routes/mcp.js @@ -149,6 +149,22 @@ router.get('/:serverName/oauth/callback', async (req, res) => { if (oauthError) { logger.error('[MCP OAuth] OAuth error received', { error: oauthError }); + if (state && typeof state === 'string') { + try { + const flowsCache = getLogStores(CacheKeys.FLOWS); + const flowManager = getFlowStateManager(flowsCache); + const flowId = await MCPOAuthHandler.resolveStateToFlowId(state, flowManager); + if (flowId) { + await flowManager.failFlow(flowId, 'mcp_oauth', String(oauthError)); + logger.debug('[MCP OAuth] Marked flow as FAILED with OAuth error', { + flowId, + error: oauthError, + }); + } + } catch (err) { + logger.debug('[MCP OAuth] Could not mark flow as failed', err); + } + } return res.redirect( `${basePath}/oauth/error?error=${encodeURIComponent(String(oauthError))}`, ); diff --git a/packages/api/src/mcp/MCPConnectionFactory.ts b/packages/api/src/mcp/MCPConnectionFactory.ts index 26d83e5330..cb3cda15d0 100644 --- a/packages/api/src/mcp/MCPConnectionFactory.ts +++ b/packages/api/src/mcp/MCPConnectionFactory.ts @@ -373,19 +373,7 @@ export class MCPConnectionFactory { this.flowManager!.createFlow(newFlowId, 'mcp_oauth', {}, this.signal).catch( async (error) => { logger.debug(`${this.logPrefix} OAuth flow monitor ended`, error); - if ( - flowMetadata.reusedStoredClient && - this.tokenMethods?.deleteTokens && - MCPConnectionFactory.isClientRejection(error) - ) { - await MCPTokenStorage.deleteClientRegistration({ - userId: this.userId!, - serverName: this.serverName, - deleteTokens: this.tokenMethods.deleteTokens, - }).catch((err) => { - logger.warn(`${this.logPrefix} Failed to clear stale client registration`, err); - }); - } + await this.clearStaleClientIfRejected(flowMetadata.reusedStoredClient, error); }, ); @@ -429,21 +417,7 @@ export class MCPConnectionFactory { if (result?.tokens) { connection.emit('oauthHandled'); } else { - // OAuth failed — if we reused a stored client registration and the failure - // indicates client rejection, clear it so the next attempt does fresh DCR - if ( - result?.reusedStoredClient && - this.tokenMethods?.deleteTokens && - MCPConnectionFactory.isClientRejection(result.error) - ) { - await MCPTokenStorage.deleteClientRegistration({ - userId: this.userId!, - serverName: this.serverName, - deleteTokens: this.tokenMethods.deleteTokens, - }).catch((err) => { - logger.warn(`${this.logPrefix} Failed to clear stale client registration`, err); - }); - } + await this.clearStaleClientIfRejected(result?.reusedStoredClient, result?.error); logger.warn(`${this.logPrefix} OAuth failed, emitting oauthFailed event`); connection.emit('oauthFailed', new Error('OAuth authentication failed')); } @@ -497,6 +471,26 @@ export class MCPConnectionFactory { } } + /** Clears stored client registration if the error indicates client rejection */ + private async clearStaleClientIfRejected( + reusedStoredClient: boolean | undefined, + error: unknown, + ): Promise { + if (!reusedStoredClient || !this.tokenMethods?.deleteTokens) { + return; + } + if (!MCPConnectionFactory.isClientRejection(error)) { + return; + } + await MCPTokenStorage.deleteClientRegistration({ + userId: this.userId!, + serverName: this.serverName, + deleteTokens: this.tokenMethods.deleteTokens, + }).catch((err) => { + logger.warn(`${this.logPrefix} Failed to clear stale client registration`, err); + }); + } + /** Determines if an error indicates the OAuth client registration was rejected */ static isClientRejection(error: unknown): boolean { if (!error || typeof error !== 'object') { diff --git a/packages/api/src/mcp/__tests__/MCPOAuthClientRegistrationReuse.test.ts b/packages/api/src/mcp/__tests__/MCPOAuthClientRegistrationReuse.test.ts index e9667cf335..38b2d13fb7 100644 --- a/packages/api/src/mcp/__tests__/MCPOAuthClientRegistrationReuse.test.ts +++ b/packages/api/src/mcp/__tests__/MCPOAuthClientRegistrationReuse.test.ts @@ -54,7 +54,7 @@ jest.mock('~/mcp/mcpConfig', () => ({ })); describe('MCPOAuthHandler - client registration reuse on reconnection', () => { - let server: OAuthTestServer; + let server: OAuthTestServer | undefined; let originalDomainServer: string | undefined; beforeEach(() => { @@ -70,6 +70,7 @@ describe('MCPOAuthHandler - client registration reuse on reconnection', () => { } if (server) { await server.close(); + server = undefined; } jest.clearAllMocks(); }); @@ -289,7 +290,9 @@ describe('MCPOAuthHandler - client registration reuse on reconnection', () => { server = await createOAuthMCPServer({ tokenTTLMs: 60000 }); const tokenStore = new InMemoryTokenStore(); - // Seed a stored client with a client_id that the OAuth server doesn't recognize + // Seed a stored client with a client_id that the OAuth server doesn't recognize, + // but with matching issuer and redirect_uri so reuse logic accepts it + const serverIssuer = `http://127.0.0.1:${server.port}`; await MCPTokenStorage.storeTokens({ userId: 'user-1', serverName: 'test-server', @@ -302,6 +305,11 @@ describe('MCPOAuthHandler - client registration reuse on reconnection', () => { client_secret: 'stale-secret', redirect_uris: ['http://localhost:3080/api/mcp/test-server/oauth/callback'], } as OAuthClientInformation & { redirect_uris: string[] }, + metadata: { + issuer: serverIssuer, + authorization_endpoint: `${serverIssuer}/authorize`, + token_endpoint: `${serverIssuer}/token`, + }, }); // First attempt: reuses the stale client (this is expected — we don't know it's stale yet) diff --git a/packages/api/src/mcp/__tests__/handler.test.ts b/packages/api/src/mcp/__tests__/handler.test.ts index 26116d65f4..87de316d17 100644 --- a/packages/api/src/mcp/__tests__/handler.test.ts +++ b/packages/api/src/mcp/__tests__/handler.test.ts @@ -1429,7 +1429,7 @@ describe('MCPOAuthHandler - Configurable OAuth Metadata', () => { mockGetClientInfoAndMetadata.mockResolvedValueOnce({ clientInfo: existingClientInfo, - clientMetadata: {}, + clientMetadata: { issuer: 'https://example.com' }, }); // Mock resource metadata discovery to fail diff --git a/packages/api/src/mcp/oauth/handler.ts b/packages/api/src/mcp/oauth/handler.ts index d2823c065f..883ebb123f 100644 --- a/packages/api/src/mcp/oauth/handler.ts +++ b/packages/api/src/mcp/oauth/handler.ts @@ -512,16 +512,19 @@ export class MCPOAuthHandler { if (existing?.clientInfo?.client_id) { const storedRedirectUri = (existing.clientInfo as OAuthClientInformation) .redirect_uris?.[0]; - const storedIssuer = existing.clientMetadata?.issuer; + const storedIssuer = + typeof existing.clientMetadata?.issuer === 'string' + ? existing.clientMetadata.issuer + : null; const currentIssuer = metadata.issuer ?? authServerUrl.toString(); if (!storedRedirectUri || storedRedirectUri !== redirectUri) { logger.debug( `[MCPOAuth] Stored redirect_uri "${storedRedirectUri}" differs from current "${redirectUri}", will re-register`, ); - } else if (storedIssuer && storedIssuer !== currentIssuer) { + } else if (!storedIssuer || storedIssuer !== currentIssuer) { logger.debug( - `[MCPOAuth] Stored issuer "${storedIssuer}" differs from current "${currentIssuer}", will re-register`, + `[MCPOAuth] Issuer mismatch (stored: ${storedIssuer ?? 'none'}, current: ${currentIssuer}), will re-register`, ); } else { logger.debug(