From 8e2721011e9f13ec168fcfb8e6ccbc158c0fbae5 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Thu, 26 Mar 2026 14:45:13 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=91=20fix:=20Robust=20MCP=20OAuth=20De?= =?UTF-8?q?tection=20in=20Tool-Call=20Flow=20(#12418)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(api): add buildOAuthToolCallName utility for MCP OAuth flows Extract a shared utility that builds the synthetic tool-call name used during MCP OAuth flows (oauth_mcp_{normalizedServerName}). Uses startsWith on the raw serverName (not the normalized form) to guard against double-wrapping, so names that merely normalize to start with oauth_mcp_ (e.g., oauth@mcp@server) are correctly prefixed while genuinely pre-wrapped names are left as-is. Add 8 unit tests covering normal names, pre-wrapped names, _mcp_ substrings, special characters, non-ASCII, and empty string inputs. * fix(backend): use buildOAuthToolCallName in MCP OAuth flows Replace inline tool-call name construction in both reconnectServer (MCP.js) and createOAuthEmitter (ToolService.js) with the shared buildOAuthToolCallName utility. Remove unused normalizeServerName import from ToolService.js. Fix import ordering in both files. This ensures the oauth_mcp_ prefix is consistently applied so the client correctly identifies MCP OAuth flows and binds the CSRF cookie to the right server. * fix(client): robust MCP OAuth detection and split handling in ToolCall - Fix split() destructuring to preserve tail segments for server names containing _mcp_ (e.g., foo_mcp_bar no longer truncated to foo). - Add auth URL redirect_uri fallback: when the tool-call name lacks the _mcp_ delimiter, parse redirect_uri for the MCP callback path. Set function_name to the extracted server name so progress text shows the server, not the raw tool-call ID. - Display server name instead of literal "oauth" as function_name, gated on auth presence to avoid misidentifying real tools named "oauth". - Consolidate three independent new URL(auth) parses into a single parsedAuthUrl useMemo shared across detection, actionId, and authDomain hooks. - Replace any type on ProgressText test mock with structural type. - Add 8 tests covering delimiter detection, multi-segment names, function_name display, redirect_uri fallback, normalized _mcp_ server names, and non-MCP action auth exclusion. * chore: fix import order in utils.test.ts * fix(client): drop auth gate on OAuth displayName so completed flows show server name The createOAuthEnd handler re-emits the toolCall delta without auth, so auth is cleared on the client after OAuth completes. Gating displayName on `func === 'oauth' && auth` caused completed OAuth steps to render "Completed oauth" instead of "Completed my-server". Remove the `&& auth` gate — within the MCP delimiter branch the func="oauth" check alone is sufficient. Also remove `auth` from the useMemo dep array since only `parsedAuthUrl` is referenced. Update the test to assert correct post-completion display. --- api/server/services/MCP.js | 3 +- api/server/services/ToolService.js | 3 +- .../Chat/Messages/Content/ToolCall.tsx | 66 ++++---- .../Content/__tests__/ToolCall.test.tsx | 150 +++++++++++++++++- packages/api/src/mcp/__tests__/utils.test.ts | 50 +++++- packages/api/src/mcp/utils.ts | 16 ++ 6 files changed, 255 insertions(+), 33 deletions(-) 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)