diff --git a/packages/api/src/mcp/__tests__/handler.test.ts b/packages/api/src/mcp/__tests__/handler.test.ts index 87de316d17..aa19eec9b0 100644 --- a/packages/api/src/mcp/__tests__/handler.test.ts +++ b/packages/api/src/mcp/__tests__/handler.test.ts @@ -2153,4 +2153,259 @@ describe('MCPOAuthHandler - Configurable OAuth Metadata', () => { }); }); }); + + describe('Protected Resource Metadata validation (RFC 9728 / GHSA-gvpj-vm2f-2m23)', () => { + const originalFetch = global.fetch; + const mockFetch = jest.fn(); + + beforeEach(() => { + jest.clearAllMocks(); + global.fetch = mockFetch as unknown as typeof fetch; + mockFetch.mockResolvedValue({ ok: true, json: async () => ({}) } as Response); + }); + + afterAll(() => { + global.fetch = originalFetch; + }); + + it('rejects metadata whose resource points at a different origin than the configured server', async () => { + mockDiscoverOAuthProtectedResourceMetadata.mockResolvedValueOnce({ + // attacker's server pretends to be real-mcp.com so tokens minted by real-mcp's + // auth server get sent to the attacker + resource: 'https://real-mcp.com/mcp', + authorization_servers: ['https://auth.real-mcp.com'], + }); + + await expect( + MCPOAuthHandler.initiateOAuthFlow( + 'evil-server', + 'https://fake-mcp.com/mcp', + 'user-123', + {}, + undefined, + ), + ).rejects.toThrow(/does not match server URL/); + + // authorization_servers from the tainted document must never be consulted + expect(mockDiscoverAuthorizationServerMetadata).not.toHaveBeenCalled(); + expect(mockStartAuthorization).not.toHaveBeenCalled(); + expect(mockRegisterClient).not.toHaveBeenCalled(); + }); + + it('rejects metadata whose resource is not a parseable URL (error-wrapping path)', async () => { + // A malicious or broken server could return a `resource` that passes the + // zod schema but is not a valid URL. `resourceUrlFromServerUrl` / + // `checkResourceAllowed` call `new URL()` internally and will throw; + // `assertResourceBoundToServer` wraps that into a descriptive error rather + // than letting a raw `TypeError: Invalid URL` leak out. + mockDiscoverOAuthProtectedResourceMetadata.mockResolvedValueOnce({ + resource: 'not-a-url', + authorization_servers: ['https://auth.example.com'], + }); + + await expect( + MCPOAuthHandler.initiateOAuthFlow( + 'test-server', + 'https://example.com/mcp', + 'user-123', + {}, + undefined, + ), + ).rejects.toThrow(/Unable to validate Protected Resource Metadata 'resource'/); + + expect(mockDiscoverAuthorizationServerMetadata).not.toHaveBeenCalled(); + expect(mockStartAuthorization).not.toHaveBeenCalled(); + }); + + it('rejects metadata that is missing the required resource identifier', async () => { + mockDiscoverOAuthProtectedResourceMetadata.mockResolvedValueOnce({ + // RFC 9728 §2: `resource` is REQUIRED + authorization_servers: ['https://auth.example.com'], + } as unknown as Awaited>); + + await expect( + MCPOAuthHandler.initiateOAuthFlow( + 'test-server', + 'https://example.com/mcp', + 'user-123', + {}, + undefined, + ), + ).rejects.toThrow(/missing the required 'resource' identifier/); + + expect(mockDiscoverAuthorizationServerMetadata).not.toHaveBeenCalled(); + }); + + it('rejects metadata whose resource points at the same origin but a sibling path', async () => { + // Same-origin path-confusion: checkResourceAllowed enforces path-prefix match, so + // a server at /api can't claim tokens for /admin on the same origin. + mockDiscoverOAuthProtectedResourceMetadata.mockResolvedValueOnce({ + resource: 'https://example.com/admin', + authorization_servers: ['https://auth.example.com'], + }); + + await expect( + MCPOAuthHandler.initiateOAuthFlow( + 'test-server', + 'https://example.com/api', + 'user-123', + {}, + undefined, + ), + ).rejects.toThrow(/does not match server URL/); + }); + + it('accepts metadata whose resource exactly matches the server URL', async () => { + mockDiscoverOAuthProtectedResourceMetadata.mockResolvedValueOnce({ + resource: 'https://example.com/mcp', + authorization_servers: ['https://auth.example.com'], + }); + + mockDiscoverAuthorizationServerMetadata.mockResolvedValueOnce({ + issuer: 'https://auth.example.com', + authorization_endpoint: 'https://auth.example.com/authorize', + token_endpoint: 'https://auth.example.com/token', + registration_endpoint: 'https://auth.example.com/register', + response_types_supported: ['code'], + } as AuthorizationServerMetadata); + + mockRegisterClient.mockResolvedValueOnce({ + client_id: 'new-client-id', + redirect_uris: ['http://localhost:3080/api/mcp/test-server/oauth/callback'], + logo_uri: undefined, + tos_uri: undefined, + }); + + mockStartAuthorization.mockResolvedValueOnce({ + authorizationUrl: new URL('https://auth.example.com/authorize?client_id=new-client-id'), + codeVerifier: 'test-code-verifier', + }); + + const result = await MCPOAuthHandler.initiateOAuthFlow( + 'test-server', + 'https://example.com/mcp', + 'user-123', + {}, + undefined, + ); + + expect(result.authorizationUrl).toContain('resource=https%3A%2F%2Fexample.com%2Fmcp'); + }); + + it('accepts metadata whose resource is an origin-level prefix of the server URL', async () => { + // Some RFC 9728 implementations advertise the origin as `resource` for a + // sub-path MCP server; checkResourceAllowed permits this (path-prefix match). + mockDiscoverOAuthProtectedResourceMetadata.mockResolvedValueOnce({ + resource: 'https://example.com', + authorization_servers: ['https://auth.example.com'], + }); + + mockDiscoverAuthorizationServerMetadata.mockResolvedValueOnce({ + issuer: 'https://auth.example.com', + authorization_endpoint: 'https://auth.example.com/authorize', + token_endpoint: 'https://auth.example.com/token', + registration_endpoint: 'https://auth.example.com/register', + response_types_supported: ['code'], + } as AuthorizationServerMetadata); + + mockRegisterClient.mockResolvedValueOnce({ + client_id: 'client-id', + redirect_uris: ['http://localhost:3080/api/mcp/test-server/oauth/callback'], + logo_uri: undefined, + tos_uri: undefined, + }); + + mockStartAuthorization.mockResolvedValueOnce({ + authorizationUrl: new URL('https://auth.example.com/authorize?client_id=client-id'), + codeVerifier: 'test-code-verifier', + }); + + await expect( + MCPOAuthHandler.initiateOAuthFlow( + 'test-server', + 'https://example.com/mcp', + 'user-123', + {}, + undefined, + ), + ).resolves.toBeDefined(); + }); + + it('re-validates resource binding at token exchange for flows initiated before the fix', async () => { + // Defense-in-depth: flow state has a 10-min TTL, so a flow created under older + // (vulnerable) code could still be in-flight at upgrade time with unvalidated + // resourceMetadata stored. completeOAuthFlow must re-assert the binding rather + // than blindly trusting stored state — and must still run the normal failure + // bookkeeping (failFlow) so the flow manager doesn't leak a stuck PENDING entry. + const mockFailFlow = jest.fn(); + const mockFlowManager = { + getFlowState: jest.fn().mockResolvedValue({ + status: 'PENDING', + metadata: { + serverName: 'evil-server', + userId: 'user-123', + serverUrl: 'https://fake-mcp.com/mcp', + state: 'abc', + codeVerifier: 'verifier', + clientInfo: { client_id: 'cid' }, + metadata: { authorization_endpoint: 'x', token_endpoint: 'y' }, + resourceMetadata: { + // tainted: stored during a pre-fix initiateOAuthFlow + resource: 'https://real-mcp.com/mcp', + authorization_servers: ['https://auth.real-mcp.com'], + }, + } as MCPOAuthFlowMetadata, + }), + failFlow: mockFailFlow, + } as unknown as FlowStateManager; + + await expect( + MCPOAuthHandler.completeOAuthFlow('flow-id', 'auth-code', mockFlowManager, {}), + ).rejects.toThrow(/does not match server URL/); + + expect(mockExchangeAuthorization).not.toHaveBeenCalled(); + expect(mockFailFlow).toHaveBeenCalledWith('flow-id', expect.any(String), expect.any(Error)); + }); + + it('falls back to origin-based discovery when the well-known endpoint returns no metadata', async () => { + // A missing/404 PRM doc is different from a spoofed one: the SDK throws, we + // catch it, and proceed to discover the auth server from the MCP server URL. + // This path must NOT trip the new validation. + mockDiscoverOAuthProtectedResourceMetadata.mockRejectedValueOnce( + new Error('Resource server does not implement OAuth 2.0 Protected 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'], + } as AuthorizationServerMetadata); + + mockRegisterClient.mockResolvedValueOnce({ + client_id: 'client-id', + 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=client-id'), + codeVerifier: 'test-code-verifier', + }); + + const result = await MCPOAuthHandler.initiateOAuthFlow( + 'test-server', + 'https://example.com/mcp', + 'user-123', + {}, + undefined, + ); + + expect(result.authorizationUrl).toBeDefined(); + // No PRM, so the authorization URL must not carry a `resource` parameter + expect(result.authorizationUrl).not.toContain('resource='); + }); + }); }); diff --git a/packages/api/src/mcp/oauth/handler.ts b/packages/api/src/mcp/oauth/handler.ts index ccca5b1945..b7e98fbce5 100644 --- a/packages/api/src/mcp/oauth/handler.ts +++ b/packages/api/src/mcp/oauth/handler.ts @@ -2,6 +2,10 @@ import { randomBytes } from 'crypto'; import { logger } from '@librechat/data-schemas'; import { FetchLike } from '@modelcontextprotocol/sdk/shared/transport'; import { OAuthMetadataSchema } from '@modelcontextprotocol/sdk/shared/auth.js'; +import { + checkResourceAllowed, + resourceUrlFromServerUrl, +} from '@modelcontextprotocol/sdk/shared/auth-utils.js'; import { registerClient, startAuthorization, @@ -146,8 +150,25 @@ export class MCPOAuthHandler { `[MCPOAuth] Attempting to discover protected resource metadata from ${serverUrl}`, ); resourceMetadata = await discoverOAuthProtectedResourceMetadata(serverUrl, {}, fetchFn); + } catch (error) { + logger.debug('[MCPOAuth] Resource metadata discovery failed, continuing with server URL', { + error, + }); + } - if (resourceMetadata?.authorization_servers?.length) { + if (resourceMetadata) { + /** + * RFC 9728 §3.3 / §7.3: the `resource` identifier in a Protected Resource Metadata + * document MUST match the URL the client used to fetch it. Without this check a + * malicious MCP server can impersonate a legitimate one by advertising the real + * server's resource URL plus the real server's authorization server, causing tokens + * minted for the real server to be sent to the attacker (GHSA-gvpj-vm2f-2m23). + * On mismatch, discard the entire document: `authorization_servers` and any other + * field on it are equally untrustworthy. + */ + this.assertResourceBoundToServer(serverUrl, resourceMetadata); + + if (resourceMetadata.authorization_servers?.length) { const discoveredAuthServer = resourceMetadata.authorization_servers[0]; await this.validateOAuthUrl(discoveredAuthServer, 'authorization_server', allowedDomains); authServerUrl = new URL(discoveredAuthServer); @@ -157,10 +178,6 @@ export class MCPOAuthHandler { } else { logger.debug(`[MCPOAuth] No authorization servers found in resource metadata`); } - } catch (error) { - logger.debug('[MCPOAuth] Resource metadata discovery failed, continuing with server URL', { - error, - }); } // Discover OAuth metadata @@ -586,25 +603,28 @@ export class MCPOAuthHandler { authorizationUrl.searchParams.set('state', state); logger.debug(`[MCPOAuth] Added state parameter to authorization URL`); - if (resourceMetadata?.resource != null && 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, - ); - } + if (resourceMetadata?.resource) { + /** + * `resource` was already canonicalized and bound to `serverUrl` inside + * {@link discoverMetadata} via {@link assertResourceBoundToServer}, so `new URL` + * here cannot throw and the value is safe to echo back to the authorization server. + */ + const canonicalResource = new URL(resourceMetadata.resource).href; + authorizationUrl.searchParams.set('resource', canonicalResource); + logger.debug( + `[MCPOAuth] Added resource parameter to authorization URL: ${canonicalResource}`, + ); } else { + /** + * Reachable only when `discoverOAuthProtectedResourceMetadata` did not return a + * document (404 / network error / server does not implement RFC 9728). If a PRM + * document exists but is missing `resource`, {@link assertResourceBoundToServer} + * rejects it before this code runs, so this branch does not warn about a + * malformed document — it warns about the absence of one. + */ logger.warn( - `[MCPOAuth] Resource metadata missing 'resource' property for ${serverName}. ` + - 'This can cause issues with some Authorization Servers who expect a "resource" parameter.', + `[MCPOAuth] No protected resource metadata available for ${serverName}. ` + + 'This can cause issues with some Authorization Servers that expect a "resource" parameter.', ); } } catch (error) { @@ -677,17 +697,19 @@ export class MCPOAuthHandler { } let resource: URL | undefined; - try { - if (metadata.resourceMetadata?.resource != null && metadata.resourceMetadata.resource) { + if (metadata.resourceMetadata) { + /** + * Defense-in-depth: re-assert the RFC 9728 §3.3 binding against the flow's stored + * server URL. Flow state has a 10-minute TTL, so a flow initiated under older + * (pre-fix) code could still be in-flight at upgrade time carrying unvalidated + * resource metadata. Re-validating here closes that window without requiring ops + * teams to flush flow state on deploy (GHSA-gvpj-vm2f-2m23). + */ + this.assertResourceBoundToServer(metadata.serverUrl, metadata.resourceMetadata); + if (metadata.resourceMetadata.resource) { resource = new URL(metadata.resourceMetadata.resource); logger.debug(`[MCPOAuth] Resource URL for flow ${flowId}: ${resource.toString()}`); } - } catch (error) { - logger.warn( - `[MCPOAuth] Invalid resource URL format for flow ${flowId}: '${metadata.resourceMetadata!.resource}'. ` + - `Error: ${error instanceof Error ? error.message : 'Unknown error'}. Proceeding without resource parameter.`, - ); - resource = undefined; } const tokens = await exchangeAuthorization(metadata.serverUrl, { @@ -755,6 +777,48 @@ export class MCPOAuthHandler { return randomBytes(32).toString('base64url'); } + /** + * Enforces RFC 9728 §3.3 / §7.3: the `resource` identifier advertised by an OAuth + * Protected Resource Metadata document MUST match the URL the client used to fetch + * the document. A mismatch means the metadata is attacker-controlled (or the server + * is badly misconfigured); per the RFC the whole document MUST be discarded, and in + * practice we must fail the OAuth flow because `authorization_servers` on the same + * document is also untrustworthy and was the primary theft vector in + * GHSA-gvpj-vm2f-2m23. + * + * Uses the MCP SDK's own {@link checkResourceAllowed} so the semantics (same origin + * plus configured-path-prefix) match what the SDK enforces internally via + * {@link selectResourceURL}, a code path LibreChat does not go through. + */ + private static assertResourceBoundToServer( + serverUrl: string, + resourceMetadata: OAuthProtectedResourceMetadata, + ): void { + if (!resourceMetadata.resource) { + throw new Error( + `[MCPOAuth] Protected Resource Metadata from ${sanitizeUrlForLogging(serverUrl)} is missing the required 'resource' identifier (RFC 9728 §2). Refusing OAuth flow.`, + ); + } + + let allowed = false; + try { + allowed = checkResourceAllowed({ + requestedResource: resourceUrlFromServerUrl(serverUrl), + configuredResource: resourceMetadata.resource, + }); + } catch (error) { + throw new Error( + `[MCPOAuth] Unable to validate Protected Resource Metadata 'resource' for ${sanitizeUrlForLogging(serverUrl)}: ${error instanceof Error ? error.message : String(error)}.`, + ); + } + + if (!allowed) { + throw new Error( + `[MCPOAuth] Protected Resource Metadata 'resource' (${sanitizeUrlForLogging(resourceMetadata.resource)}) does not match server URL (${sanitizeUrlForLogging(serverUrl)}). Refusing OAuth flow (RFC 9728 §3.3).`, + ); + } + } + /** * Validates an OAuth URL is not targeting a private/internal address. * Skipped when the full URL (hostname + protocol + port) matches an admin-trusted