diff --git a/CHANGELOG.md b/CHANGELOG.md index bd292ce1c..35b0ad5de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Added progress bar when navigating between pages. [#1204](https://github.com/sourcebot-dev/sourcebot/pull/1204) - Added a integrated changelog into the sidebar. [#1227](https://github.com/sourcebot-dev/sourcebot/pull/1227) +### Fixed +- [EE] Fixed MCP connector validation to fail closed when server reachability cannot be verified, preventing unreachable or invalid URLs from being added as connectors. [#1252](https://github.com/sourcebot-dev/sourcebot/pull/1252) + ### Changed - Redesigned the app layout with a new collapsible sidebar navigation, replacing the previous top navigation bar. [#1097](https://github.com/sourcebot-dev/sourcebot/pull/1097) - Expired offline license keys no longer crash the process. An expired key now degrades to the unlicensed state. [#1109](https://github.com/sourcebot-dev/sourcebot/pull/1109) diff --git a/packages/web/src/app/(app)/settings/workspaceAskAgent/workspaceAskAgentPage.tsx b/packages/web/src/app/(app)/settings/workspaceAskAgent/workspaceAskAgentPage.tsx index 18fbc4411..1e9325f89 100644 --- a/packages/web/src/app/(app)/settings/workspaceAskAgent/workspaceAskAgentPage.tsx +++ b/packages/web/src/app/(app)/settings/workspaceAskAgent/workspaceAskAgentPage.tsx @@ -327,6 +327,15 @@ export function WorkspaceAskAgentPage({ callbackStatus, callbackServer, callback setIsClientCredentialsDialogOpen(true); return; } + + if (!dcrSupport.isKnown) { + toast({ + title: "Error", + description: "Could not verify this connector. The URL may be unreachable or not a valid MCP server.", + variant: "destructive" + }); + return; + } } const result = await createMcpServer(displayName, normalizedServerUrl); diff --git a/packages/web/src/ee/features/chat/mcp/actions.test.ts b/packages/web/src/ee/features/chat/mcp/actions.test.ts index 1009c3a4f..b12630bf5 100644 --- a/packages/web/src/ee/features/chat/mcp/actions.test.ts +++ b/packages/web/src/ee/features/chat/mcp/actions.test.ts @@ -24,6 +24,7 @@ const mocks = vi.hoisted(() => ({ deleteMany: vi.fn(), }, }, + probeMcpServerCompatibility: vi.fn(), })); vi.mock('server-only', () => ({})); @@ -44,6 +45,10 @@ vi.mock('@sourcebot/shared', () => ({ vi.mock('@/lib/posthog', () => ({ captureEvent: mocks.captureEvent, })); +vi.mock('./dcrDiscovery', () => ({ + probeMcpServerCompatibility: mocks.probeMcpServerCompatibility, + checkMcpServerDcrSupport: vi.fn(), +})); const { createMcpServer, createStaticOAuthMcpServer, deleteMcpServer, disconnectMcpServer } = await import('./actions'); @@ -94,6 +99,15 @@ beforeEach(() => { mocks.env.NODE_ENV = 'production'; mocks.env.SOURCEBOT_MCP_TOOL_CALL_TIMEOUT_MS = 5000; mocks.captureEvent.mockResolvedValue(undefined); + mocks.probeMcpServerCompatibility.mockResolvedValue({ + success: true, + dcrSupport: { + supportsDcr: true, + isKnown: true, + authorizationServerUrl: 'https://mcp.linear.app', + registrationEndpoint: 'https://mcp.linear.app/register', + }, + }); }); describe('createMcpServer', () => { @@ -108,6 +122,10 @@ describe('createMcpServer', () => { sanitizedName: 'linear', serverUrl: 'https://mcp.linear.app/mcp', }); + expect(mocks.probeMcpServerCompatibility).toHaveBeenCalledWith( + 'https://mcp.linear.app/mcp', + expect.any(Function), + ); expect(prisma.mcpServer.create).toHaveBeenCalledWith({ data: { name: 'Linear', @@ -152,6 +170,42 @@ describe('createMcpServer', () => { }); expect(prisma.mcpServer.create).not.toHaveBeenCalled(); }); + + test('rejects MCP servers that are unreachable', async () => { + const prisma = setAuthContext(OrgRole.OWNER); + mocks.probeMcpServerCompatibility.mockResolvedValue({ + success: false, + reason: 'unreachable', + }); + + const result = await createMcpServer('Bogus', 'https://mcp.nonexistent.invalid/mcp'); + + expect(result).toMatchObject({ + statusCode: 502, + errorCode: ErrorCode.MCP_SERVER_UNREACHABLE, + message: 'Could not reach this connector URL. Please verify the URL is correct and the server is accessible.', + }); + expect(prisma.mcpServer.create).not.toHaveBeenCalled(); + expect(mocks.captureEvent).not.toHaveBeenCalled(); + }); + + test('rejects URLs that are not MCP-compatible', async () => { + const prisma = setAuthContext(OrgRole.OWNER); + mocks.probeMcpServerCompatibility.mockResolvedValue({ + success: false, + reason: 'not_compatible', + }); + + const result = await createMcpServer('NotMcp', 'https://example.com/not-mcp'); + + expect(result).toMatchObject({ + statusCode: 400, + errorCode: ErrorCode.MCP_SERVER_NOT_COMPATIBLE, + message: 'This URL does not appear to be a valid MCP connector. The server did not provide the expected OAuth metadata.', + }); + expect(prisma.mcpServer.create).not.toHaveBeenCalled(); + expect(mocks.captureEvent).not.toHaveBeenCalled(); + }); }); describe('createStaticOAuthMcpServer', () => { @@ -165,6 +219,10 @@ describe('createStaticOAuthMcpServer', () => { clientSecret: ' client-secret ', }); + expect(mocks.probeMcpServerCompatibility).toHaveBeenCalledWith( + 'https://mcp.slack.com/mcp', + expect.any(Function), + ); expect(mocks.encryptOAuthToken).toHaveBeenCalledWith(JSON.stringify({ client_id: 'client-id', client_secret: 'client-secret', @@ -197,6 +255,50 @@ describe('createStaticOAuthMcpServer', () => { }); }); + test('rejects static OAuth MCP servers that are unreachable', async () => { + const prisma = setAuthContext(OrgRole.OWNER); + mocks.probeMcpServerCompatibility.mockResolvedValue({ + success: false, + reason: 'unreachable', + }); + + const result = await createStaticOAuthMcpServer(createStaticOAuthRequest({ + serverUrl: 'https://mcp.nonexistent.invalid/mcp', + })); + + expect(result).toMatchObject({ + statusCode: 502, + errorCode: ErrorCode.MCP_SERVER_UNREACHABLE, + message: 'Could not reach this connector URL. Please verify the URL is correct and the server is accessible.', + }); + expect(prisma.mcpServer.create).not.toHaveBeenCalled(); + expect(mocks.encryptOAuthToken).not.toHaveBeenCalled(); + expect(mocks.captureEvent).not.toHaveBeenCalled(); + expect(JSON.stringify(result)).not.toContain('client-secret'); + }); + + test('rejects static OAuth MCP servers that are not MCP-compatible', async () => { + const prisma = setAuthContext(OrgRole.OWNER); + mocks.probeMcpServerCompatibility.mockResolvedValue({ + success: false, + reason: 'not_compatible', + }); + + const result = await createStaticOAuthMcpServer(createStaticOAuthRequest({ + serverUrl: 'https://example.com/not-mcp', + })); + + expect(result).toMatchObject({ + statusCode: 400, + errorCode: ErrorCode.MCP_SERVER_NOT_COMPATIBLE, + message: 'This URL does not appear to be a valid MCP connector. The server did not provide the expected OAuth metadata.', + }); + expect(prisma.mcpServer.create).not.toHaveBeenCalled(); + expect(mocks.encryptOAuthToken).not.toHaveBeenCalled(); + expect(mocks.captureEvent).not.toHaveBeenCalled(); + expect(JSON.stringify(result)).not.toContain('client-secret'); + }); + test('members cannot add static OAuth MCP servers', async () => { const prisma = setAuthContext(OrgRole.MEMBER); diff --git a/packages/web/src/ee/features/chat/mcp/actions.ts b/packages/web/src/ee/features/chat/mcp/actions.ts index 7ce05ede4..20e1abbca 100644 --- a/packages/web/src/ee/features/chat/mcp/actions.ts +++ b/packages/web/src/ee/features/chat/mcp/actions.ts @@ -13,12 +13,14 @@ import { z } from 'zod'; import { sanitizeMcpServerName } from './utils'; import { hasEntitlement } from '@/lib/entitlements'; import { oauthNotSupported } from './errors'; -import { checkMcpServerDcrSupport } from './dcrDiscovery'; +import { checkMcpServerDcrSupport, probeMcpServerCompatibility } from './dcrDiscovery'; import { encryptOAuthToken, env } from '@sourcebot/shared'; import { captureEvent } from '@/lib/posthog'; import { getMcpAuthMode } from './analytics'; import type { McpConnectorEntryPoint } from '@/lib/posthogEvents'; +const MCP_PROBE_TIMEOUT_MS = Math.min(env.SOURCEBOT_MCP_TOOL_CALL_TIMEOUT_MS, 10000); + const MCP_DCR_DISCOVERY_TIMEOUT_MS = Math.min(env.SOURCEBOT_MCP_TOOL_CALL_TIMEOUT_MS, 10000); const createStaticOAuthMcpServerSchema = z.object({ name: z.string().trim().min(1), @@ -66,6 +68,22 @@ function invalidRequest(message: string): ServiceError { }; } +function mcpServerUnreachable(): ServiceError { + return { + statusCode: StatusCodes.BAD_GATEWAY, + errorCode: ErrorCode.MCP_SERVER_UNREACHABLE, + message: 'Could not reach this connector URL. Please verify the URL is correct and the server is accessible.', + }; +} + +function mcpServerNotCompatible(): ServiceError { + return { + statusCode: StatusCodes.BAD_REQUEST, + errorCode: ErrorCode.MCP_SERVER_NOT_COMPATIBLE, + message: 'This URL does not appear to be a valid MCP connector. The server did not provide the expected OAuth metadata.', + }; +} + function assertHttpsAuthUrlInProduction(): ServiceError | undefined { if (env.NODE_ENV !== 'production') { return undefined; @@ -204,6 +222,17 @@ export const createStaticOAuthMcpServer = async ( return preparedServer; } + const probeResult = await probeMcpServerCompatibility( + preparedServer.normalizedServerUrl, + createTimeoutFetch(MCP_PROBE_TIMEOUT_MS), + ); + if (!probeResult.success) { + if (probeResult.reason === 'unreachable') { + return mcpServerUnreachable(); + } + return mcpServerNotCompatible(); + } + const clientInfo = encryptOAuthToken(JSON.stringify({ client_id: parsed.data.clientId, client_secret: parsed.data.clientSecret, @@ -263,6 +292,17 @@ export const createMcpServer = async (name: string, serverUrl: string) => sew(() return preparedServer; } + const probeResult = await probeMcpServerCompatibility( + preparedServer.normalizedServerUrl, + createTimeoutFetch(MCP_PROBE_TIMEOUT_MS), + ); + if (!probeResult.success) { + if (probeResult.reason === 'unreachable') { + return mcpServerUnreachable(); + } + return mcpServerNotCompatible(); + } + const mcpServer = await prisma.mcpServer.create({ data: { name: preparedServer.displayName, diff --git a/packages/web/src/ee/features/chat/mcp/dcrDiscovery.test.ts b/packages/web/src/ee/features/chat/mcp/dcrDiscovery.test.ts index 194a2a815..1043c2cde 100644 --- a/packages/web/src/ee/features/chat/mcp/dcrDiscovery.test.ts +++ b/packages/web/src/ee/features/chat/mcp/dcrDiscovery.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test, vi } from 'vitest'; -import { checkMcpServerDcrSupport } from './dcrDiscovery'; +import { checkMcpServerDcrSupport, probeMcpServerCompatibility } from './dcrDiscovery'; function jsonResponse(body: unknown) { return new Response(JSON.stringify(body), { @@ -215,3 +215,131 @@ describe('checkMcpServerDcrSupport', () => { }); }); }); + +describe('probeMcpServerCompatibility', () => { + test('returns success when authorization server metadata is found', async () => { + const fetchMock = vi.fn(async (input: string | URL | Request) => { + const url = input.toString(); + if (url === 'https://mcp.example.com/.well-known/oauth-protected-resource/mcp') { + return jsonResponse({ authorization_servers: ['https://auth.example.com'] }); + } + if (url === 'https://auth.example.com/.well-known/oauth-authorization-server') { + return jsonResponse({ registration_endpoint: 'https://auth.example.com/register' }); + } + return notFoundResponse(); + }) as unknown as typeof fetch; + + const result = await probeMcpServerCompatibility('https://mcp.example.com/mcp', fetchMock); + + expect(result).toEqual({ + success: true, + dcrSupport: { + supportsDcr: true, + isKnown: true, + authorizationServerUrl: 'https://auth.example.com', + registrationEndpoint: 'https://auth.example.com/register', + }, + }); + }); + + test('returns success for static OAuth servers without DCR support', async () => { + const fetchMock = vi.fn(async (input: string | URL | Request) => { + const url = input.toString(); + if (url === 'https://mcp.slack.com/.well-known/oauth-protected-resource') { + return jsonResponse({ authorization_servers: ['https://mcp.slack.com'] }); + } + if (url === 'https://mcp.slack.com/.well-known/oauth-authorization-server') { + return jsonResponse({ + authorization_endpoint: 'https://slack.com/oauth/v2_user/authorize', + token_endpoint: 'https://slack.com/api/oauth.v2.user.access', + }); + } + return notFoundResponse(); + }) as unknown as typeof fetch; + + const result = await probeMcpServerCompatibility('https://mcp.slack.com/mcp', fetchMock); + + expect(result).toEqual({ + success: true, + dcrSupport: { + supportsDcr: false, + isKnown: true, + authorizationServerUrl: 'https://mcp.slack.com', + }, + }); + }); + + test('returns unreachable when network errors occur', async () => { + const fetchMock = vi.fn(async () => { + const error = new Error('Network error'); + (error as Error & { cause?: { code?: string } }).cause = { code: 'ENOTFOUND' }; + throw error; + }) as unknown as typeof fetch; + + const result = await probeMcpServerCompatibility('https://mcp.nonexistent.invalid/mcp', fetchMock); + + expect(result).toEqual({ + success: false, + reason: 'unreachable', + }); + }); + + test('returns unreachable when connection is refused', async () => { + const fetchMock = vi.fn(async () => { + const error = new Error('Connection refused'); + (error as Error & { cause?: { code?: string } }).cause = { code: 'ECONNREFUSED' }; + throw error; + }) as unknown as typeof fetch; + + const result = await probeMcpServerCompatibility('https://localhost:9999/mcp', fetchMock); + + expect(result).toEqual({ + success: false, + reason: 'unreachable', + }); + }); + + test('returns unreachable when request times out', async () => { + const fetchMock = vi.fn(async () => { + const error = new Error('The operation was aborted.'); + error.name = 'AbortError'; + throw error; + }) as unknown as typeof fetch; + + const result = await probeMcpServerCompatibility('https://slow.example.com/mcp', fetchMock); + + expect(result).toEqual({ + success: false, + reason: 'unreachable', + }); + }); + + test('returns not_compatible when server is reachable but has no OAuth metadata', async () => { + const fetchMock = vi.fn(async () => { + return notFoundResponse(); + }) as unknown as typeof fetch; + + const result = await probeMcpServerCompatibility('https://not-mcp.example.com/mcp', fetchMock); + + expect(result).toEqual({ + success: false, + reason: 'not_compatible', + }); + }); + + test('returns not_compatible when server returns HTML instead of OAuth metadata', async () => { + const fetchMock = vi.fn(async () => { + return new Response('
Not Found', { + status: 200, + headers: { 'content-type': 'text/html' }, + }); + }) as unknown as typeof fetch; + + const result = await probeMcpServerCompatibility('https://web-server.example.com/mcp', fetchMock); + + expect(result).toEqual({ + success: false, + reason: 'not_compatible', + }); + }); +}); diff --git a/packages/web/src/ee/features/chat/mcp/dcrDiscovery.ts b/packages/web/src/ee/features/chat/mcp/dcrDiscovery.ts index 286883d50..7d69a11d8 100644 --- a/packages/web/src/ee/features/chat/mcp/dcrDiscovery.ts +++ b/packages/web/src/ee/features/chat/mcp/dcrDiscovery.ts @@ -17,6 +17,22 @@ export interface McpServerDcrSupport { registrationEndpoint?: string; } +export type McpProbeFailureReason = + | 'unreachable' + | 'not_compatible'; + +export interface McpProbeSuccess { + success: true; + dcrSupport: McpServerDcrSupport; +} + +export interface McpProbeFailure { + success: false; + reason: McpProbeFailureReason; +} + +export type McpProbeResult = McpProbeSuccess | McpProbeFailure; + function getMetadataHeaders() { return { Accept: 'application/json', @@ -86,71 +102,138 @@ function extractResourceMetadataUrl(response: Response): URL | undefined { } } -async function fetchJson(url: URL, fetchFn: typeof fetch): Promise