From a0a17491510b1f6c407997e4c5e06f5414ebe67c Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 2 Mar 2026 15:52:29 -0500 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=97=20fix:=20Normalize=20MCP=20OAuth?= =?UTF-8?q?=20`resource`=20parameter=20to=20match=20token=20exchange=20(#1?= =?UTF-8?q?2018)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 🔗 fix: Normalize MCP OAuth `resource` parameter to match token exchange The authorization request used the raw resource string from metadata while the token exchange normalized it through `new URL().href`, causing a trailing-slash mismatch that Cloudflare's auth server rejected. Canonicalize the resource URL in both paths so they match. * 🔧 test: Simplify LeaderElection integration tests for Redis Refactored the integration tests for LeaderElection with Redis by reducing the number of instances from 100 to 1, streamlining the leadership election process. Updated assertions to verify leadership status and UUID after resignation, improving test clarity and performance. Adjusted timeout to 15 seconds for the single instance scenario. * 🔧 test: Update LeaderElection test case description for clarity Modified the description of the test case for leader resignation in the LeaderElection integration tests to better reflect the expected behavior, enhancing clarity and understanding of the test's purpose. * refactor: `resource` parameter in MCP OAuth authorization URL Updated the `MCPOAuthHandler` to ensure the `resource` parameter is added to the authorization URL even when an error occurs while retrieving it from metadata. This change improves the handling of invalid resource URLs by using the raw value as a fallback, enhancing the robustness of the authorization process. --- .../LeaderElection.cache_integration.spec.ts | 39 ++++++------------- packages/api/src/mcp/oauth/handler.ts | 18 +++++++-- 2 files changed, 26 insertions(+), 31 deletions(-) diff --git a/packages/api/src/cluster/__tests__/LeaderElection.cache_integration.spec.ts b/packages/api/src/cluster/__tests__/LeaderElection.cache_integration.spec.ts index b37c291880..9bad4dcfac 100644 --- a/packages/api/src/cluster/__tests__/LeaderElection.cache_integration.spec.ts +++ b/packages/api/src/cluster/__tests__/LeaderElection.cache_integration.spec.ts @@ -49,39 +49,24 @@ describe('LeaderElection with Redis', () => { }); describe('Test Case 1: Simulate shutdown of the leader', () => { - it('should elect a new leader after the current leader resigns', async () => { - // Create 100 instances - instances = Array.from({ length: 100 }, () => new LeaderElection()); + it('should allow an instance to re-elect itself after resignation', async () => { + const instance = new LeaderElection(); + instances.push(instance); - // Call isLeader on all instances and get leadership status - const resultsWithInstances = await Promise.all( - instances.map(async (instance) => ({ - instance, - isLeader: await instance.isLeader(), - })), - ); - - // Find leader and followers - const leaders = resultsWithInstances.filter((r) => r.isLeader); - const followers = resultsWithInstances.filter((r) => !r.isLeader); - const leader = leaders[0].instance; - const nextLeader = followers[0].instance; - - // Verify only one is leader - expect(leaders.length).toBe(1); - - // Verify getLeaderUUID matches the leader's UUID - expect(await LeaderElection.getLeaderUUID()).toBe(leader.UUID); + // Instance becomes leader + expect(await instance.isLeader()).toBe(true); + expect(await LeaderElection.getLeaderUUID()).toBe(instance.UUID); // Leader resigns - await leader.resign(); + await instance.resign(); - // Verify getLeaderUUID returns null after resignation + // Verify leadership key is cleared after resignation expect(await LeaderElection.getLeaderUUID()).toBeNull(); - // Next instance to call isLeader should become the new leader - expect(await nextLeader.isLeader()).toBe(true); - }, 30000); // 30 second timeout for 100 instances + // Instance can re-elect itself after resignation + expect(await instance.isLeader()).toBe(true); + expect(await LeaderElection.getLeaderUUID()).toBe(instance.UUID); + }, 15000); }); describe('Test Case 2: Simulate crash of the leader', () => { diff --git a/packages/api/src/mcp/oauth/handler.ts b/packages/api/src/mcp/oauth/handler.ts index c07918c591..976db8e68b 100644 --- a/packages/api/src/mcp/oauth/handler.ts +++ b/packages/api/src/mcp/oauth/handler.ts @@ -529,10 +529,20 @@ export class MCPOAuthHandler { logger.debug(`[MCPOAuth] Added state parameter to authorization URL`); if (resourceMetadata?.resource != null && resourceMetadata.resource) { - authorizationUrl.searchParams.set('resource', resourceMetadata.resource); - logger.debug( - `[MCPOAuth] Added resource parameter to authorization URL: ${resourceMetadata.resource}`, - ); + try { + const canonicalResource = new URL(resourceMetadata.resource).href; + authorizationUrl.searchParams.set('resource', canonicalResource); + logger.debug( + `[MCPOAuth] Added resource parameter to authorization URL: ${canonicalResource}`, + ); + } catch (error) { + authorizationUrl.searchParams.set('resource', resourceMetadata.resource); + logger.error( + `[MCPOAuth] Invalid resource URL from metadata for ${serverName}: ` + + `'${resourceMetadata.resource}'. Using raw value as fallback.`, + error, + ); + } } else { logger.warn( `[MCPOAuth] Resource metadata missing 'resource' property for ${serverName}. ` +