diff --git a/api/server/services/MCP.js b/api/server/services/MCP.js index 5d97891c55..03563a0cfc 100644 --- a/api/server/services/MCP.js +++ b/api/server/services/MCP.js @@ -14,6 +14,7 @@ const { normalizeJsonSchema, GenerationJobManager, resolveJsonSchemaRefs, + buildOAuthToolCallName, } = require('@librechat/api'); const { Time, CacheKeys, Constants, isAssistantsEndpoint } = require('librechat-data-provider'); const { @@ -271,7 +272,7 @@ async function reconnectServer({ const stepId = 'step_oauth_login_' + serverName; const toolCall = { id: flowId, - name: serverName, + name: buildOAuthToolCallName(serverName), type: 'tool_call_chunk', }; diff --git a/api/server/services/ToolService.js b/api/server/services/ToolService.js index ca75e7eb4f..838de906fe 100644 --- a/api/server/services/ToolService.js +++ b/api/server/services/ToolService.js @@ -19,6 +19,7 @@ const { buildWebSearchContext, buildImageToolContext, buildToolClassification, + buildOAuthToolCallName, } = require('@librechat/api'); const { Time, @@ -521,7 +522,7 @@ async function loadToolDefinitionsWrapper({ req, res, agent, streamId = null, to const stepId = 'step_oauth_login_' + serverName; const toolCall = { id: flowId, - name: serverName, + name: buildOAuthToolCallName(serverName), type: 'tool_call_chunk', }; diff --git a/client/src/components/Chat/Messages/Content/ToolCall.tsx b/client/src/components/Chat/Messages/Content/ToolCall.tsx index 5abdd45f98..c7dd974577 100644 --- a/client/src/components/Chat/Messages/Content/ToolCall.tsx +++ b/client/src/components/Chat/Messages/Content/ToolCall.tsx @@ -49,19 +49,47 @@ export default function ToolCall({ } }, [autoExpand, hasOutput]); + const parsedAuthUrl = useMemo(() => { + if (!auth) { + return null; + } + try { + return new URL(auth); + } catch { + return null; + } + }, [auth]); + const { function_name, domain, isMCPToolCall, mcpServerName } = useMemo(() => { if (typeof name !== 'string') { return { function_name: '', domain: null, isMCPToolCall: false, mcpServerName: '' }; } if (name.includes(Constants.mcp_delimiter)) { - const [func, server] = name.split(Constants.mcp_delimiter); + const parts = name.split(Constants.mcp_delimiter); + const func = parts[0]; + const server = parts.slice(1).join(Constants.mcp_delimiter); + const displayName = func === 'oauth' ? server : func; return { - function_name: func || '', + function_name: displayName || '', domain: server && (server.replaceAll(actionDomainSeparator, '.') || null), isMCPToolCall: true, mcpServerName: server || '', }; } + + if (parsedAuthUrl) { + const redirectUri = parsedAuthUrl.searchParams.get('redirect_uri') || ''; + const mcpMatch = redirectUri.match(/\/api\/mcp\/([^/]+)\/oauth\/callback/); + if (mcpMatch?.[1]) { + return { + function_name: mcpMatch[1], + domain: null, + isMCPToolCall: true, + mcpServerName: mcpMatch[1], + }; + } + } + const [func, _domain] = name.includes(actionDelimiter) ? name.split(actionDelimiter) : [name, '']; @@ -71,25 +99,20 @@ export default function ToolCall({ isMCPToolCall: false, mcpServerName: '', }; - }, [name]); + }, [name, parsedAuthUrl]); const toolIconType = useMemo(() => getToolIconType(name), [name]); const mcpIconMap = useMCPIconMap(); const mcpIconUrl = isMCPToolCall ? mcpIconMap.get(mcpServerName) : undefined; const actionId = useMemo(() => { - if (isMCPToolCall || !auth) { + if (isMCPToolCall || !parsedAuthUrl) { return ''; } - try { - const url = new URL(auth); - const redirectUri = url.searchParams.get('redirect_uri') || ''; - const match = redirectUri.match(/\/api\/actions\/([^/]+)\/oauth\/callback/); - return match?.[1] || ''; - } catch { - return ''; - } - }, [auth, isMCPToolCall]); + const redirectUri = parsedAuthUrl.searchParams.get('redirect_uri') || ''; + const match = redirectUri.match(/\/api\/actions\/([^/]+)\/oauth\/callback/); + return match?.[1] || ''; + }, [parsedAuthUrl, isMCPToolCall]); const handleOAuthClick = useCallback(async () => { if (!auth) { @@ -132,21 +155,8 @@ export default function ToolCall({ ); const authDomain = useMemo(() => { - const authURL = auth ?? ''; - if (!authURL) { - return ''; - } - try { - const url = new URL(authURL); - return url.hostname; - } catch (e) { - logger.error( - 'client/src/components/Chat/Messages/Content/ToolCall.tsx - Failed to parse auth URL', - e, - ); - return ''; - } - }, [auth]); + return parsedAuthUrl?.hostname ?? ''; + }, [parsedAuthUrl]); const progress = useProgress(initialProgress); const showCancelled = cancelled || (errorState && !output); diff --git a/client/src/components/Chat/Messages/Content/__tests__/ToolCall.test.tsx b/client/src/components/Chat/Messages/Content/__tests__/ToolCall.test.tsx index 41356412f6..14b4b7e07a 100644 --- a/client/src/components/Chat/Messages/Content/__tests__/ToolCall.test.tsx +++ b/client/src/components/Chat/Messages/Content/__tests__/ToolCall.test.tsx @@ -1,6 +1,6 @@ import React from 'react'; import { RecoilRoot } from 'recoil'; -import { Tools } from 'librechat-data-provider'; +import { Tools, Constants } from 'librechat-data-provider'; import { render, screen, fireEvent } from '@testing-library/react'; import ToolCall from '../ToolCall'; @@ -53,9 +53,20 @@ jest.mock('../ToolCallInfo', () => ({ jest.mock('../ProgressText', () => ({ __esModule: true, - default: ({ onClick, inProgressText, finishedText, _error, _hasInput, _isExpanded }: any) => ( + default: ({ + onClick, + inProgressText, + finishedText, + subtitle, + }: { + onClick?: () => void; + inProgressText?: string; + finishedText?: string; + subtitle?: string; + }) => (
{finishedText || inProgressText} + {subtitle && {subtitle}}
), })); @@ -346,6 +357,141 @@ describe('ToolCall', () => { }); }); + describe('MCP OAuth detection', () => { + const d = Constants.mcp_delimiter; + + it('should detect MCP OAuth from delimiter in tool-call name', () => { + renderWithRecoil( + , + ); + const subtitle = screen.getByTestId('subtitle'); + expect(subtitle.textContent).toBe('via my-server'); + }); + + it('should preserve full server name when it contains the delimiter substring', () => { + renderWithRecoil( + , + ); + const subtitle = screen.getByTestId('subtitle'); + expect(subtitle.textContent).toBe(`via foo${d}bar`); + }); + + it('should display server name (not "oauth") as function_name for OAuth tool calls', () => { + renderWithRecoil( + , + ); + const progressText = screen.getByTestId('progress-text'); + expect(progressText.textContent).toContain('Completed my-server'); + expect(progressText.textContent).not.toContain('Completed oauth'); + }); + + it('should display server name even when auth is cleared (post-completion)', () => { + // After OAuth completes, createOAuthEnd re-emits the toolCall without auth. + // The display should still show the server name, not literal "oauth". + renderWithRecoil( + , + ); + const progressText = screen.getByTestId('progress-text'); + expect(progressText.textContent).toContain('Completed my-server'); + expect(progressText.textContent).not.toContain('Completed oauth'); + }); + + it('should fallback to auth URL redirect_uri when name lacks delimiter', () => { + const authUrl = + 'https://oauth.example.com/authorize?redirect_uri=' + + encodeURIComponent('https://app.example.com/api/mcp/my-server/oauth/callback'); + renderWithRecoil( + , + ); + const subtitle = screen.getByTestId('subtitle'); + expect(subtitle.textContent).toBe('via my-server'); + }); + + it('should display server name (not raw tool-call ID) in fallback path finished text', () => { + const authUrl = + 'https://oauth.example.com/authorize?redirect_uri=' + + encodeURIComponent('https://app.example.com/api/mcp/my-server/oauth/callback'); + renderWithRecoil( + , + ); + const progressText = screen.getByTestId('progress-text'); + expect(progressText.textContent).toContain('Completed my-server'); + expect(progressText.textContent).not.toContain('bare_name'); + }); + + it('should show normalized server name when it contains _mcp_ after prefixing', () => { + // Server named oauth@mcp@server normalizes to oauth_mcp_server, + // gets prefixed to oauth_mcp_oauth_mcp_server. Client parses: + // func="oauth", server="oauth_mcp_server". Visually awkward but + // semantically correct — the normalized name IS oauth_mcp_server. + renderWithRecoil( + , + ); + const subtitle = screen.getByTestId('subtitle'); + expect(subtitle.textContent).toBe(`via oauth${d}server`); + }); + + it('should not misidentify non-MCP action auth as MCP via fallback', () => { + const authUrl = + 'https://oauth.example.com/authorize?redirect_uri=' + + encodeURIComponent('https://app.example.com/api/actions/xyz/oauth/callback'); + renderWithRecoil( + , + ); + expect(screen.queryByTestId('subtitle')).not.toBeInTheDocument(); + }); + }); + describe('A11Y-04: screen reader status announcements', () => { it('includes sr-only aria-live region for status announcements', () => { renderWithRecoil( diff --git a/packages/api/src/mcp/__tests__/utils.test.ts b/packages/api/src/mcp/__tests__/utils.test.ts index e4fb31bdad..c244205b99 100644 --- a/packages/api/src/mcp/__tests__/utils.test.ts +++ b/packages/api/src/mcp/__tests__/utils.test.ts @@ -1,4 +1,9 @@ -import { normalizeServerName, redactServerSecrets, redactAllServerSecrets } from '~/mcp/utils'; +import { + buildOAuthToolCallName, + normalizeServerName, + redactAllServerSecrets, + redactServerSecrets, +} from '~/mcp/utils'; import type { ParsedServerConfig } from '~/mcp/types'; describe('normalizeServerName', () => { @@ -28,6 +33,49 @@ describe('normalizeServerName', () => { }); }); +describe('buildOAuthToolCallName', () => { + it('should prefix a simple server name with oauth_mcp_', () => { + expect(buildOAuthToolCallName('my-server')).toBe('oauth_mcp_my-server'); + }); + + it('should not double-wrap a name that already starts with oauth_mcp_', () => { + expect(buildOAuthToolCallName('oauth_mcp_my-server')).toBe('oauth_mcp_my-server'); + }); + + it('should correctly handle server names containing _mcp_ substring', () => { + const result = buildOAuthToolCallName('my_mcp_server'); + expect(result).toBe('oauth_mcp_my_mcp_server'); + }); + + it('should normalize non-ASCII server names before prefixing', () => { + const result = buildOAuthToolCallName('我的服务'); + expect(result).toMatch(/^oauth_mcp_server_\d+$/); + }); + + it('should normalize special characters before prefixing', () => { + expect(buildOAuthToolCallName('server@name!')).toBe('oauth_mcp_server_name'); + }); + + it('should handle empty string server name gracefully', () => { + const result = buildOAuthToolCallName(''); + expect(result).toMatch(/^oauth_mcp_server_\d+$/); + }); + + it('should treat a name already starting with oauth_mcp_ as pre-wrapped', () => { + // At the function level, a name starting with the oauth prefix is + // indistinguishable from a pre-wrapped name — guard prevents double-wrapping. + // Server names with this prefix should be blocked at registration time. + expect(buildOAuthToolCallName('oauth_mcp_github')).toBe('oauth_mcp_github'); + }); + + it('should not treat special chars that normalize to oauth_mcp_* as pre-wrapped', () => { + // oauth@mcp@server does NOT start with 'oauth_mcp_' before normalization, + // so the guard correctly does not fire and the prefix is added. + const result = buildOAuthToolCallName('oauth@mcp@server'); + expect(result).toBe('oauth_mcp_oauth_mcp_server'); + }); +}); + describe('redactServerSecrets', () => { it('should strip apiKey.key from admin-sourced keys', () => { const config: ParsedServerConfig = { diff --git a/packages/api/src/mcp/utils.ts b/packages/api/src/mcp/utils.ts index ff367725fc..db89cffada 100644 --- a/packages/api/src/mcp/utils.ts +++ b/packages/api/src/mcp/utils.ts @@ -97,6 +97,22 @@ export function normalizeServerName(serverName: string): string { return normalized; } +/** + * Builds the synthetic tool-call name used during MCP OAuth flows. + * Format: `oauth` + * + * Guards against the caller passing a pre-wrapped name (one that already + * starts with the oauth prefix in its original, un-normalized form) to + * prevent double-wrapping. + */ +export function buildOAuthToolCallName(serverName: string): string { + const oauthPrefix = `oauth${Constants.mcp_delimiter}`; + if (serverName.startsWith(oauthPrefix)) { + return normalizeServerName(serverName); + } + return `${oauthPrefix}${normalizeServerName(serverName)}`; +} + /** * Sanitizes a URL by removing query parameters to prevent credential leakage in logs. * @param url - The URL to sanitize (string or URL object)