🔑 fix: Robust MCP OAuth Detection in Tool-Call Flow (#12418)

* 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.
This commit is contained in:
Danny Avila 2026-03-26 14:45:13 -04:00 committed by GitHub
parent 359cc63b41
commit 8e2721011e
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
6 changed files with 255 additions and 33 deletions

View file

@ -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);

View file

@ -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;
}) => (
<div data-testid="progress-text" onClick={onClick}>
{finishedText || inProgressText}
{subtitle && <span data-testid="subtitle">{subtitle}</span>}
</div>
),
}));
@ -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(
<ToolCall
{...mockProps}
name={`oauth${d}my-server`}
initialProgress={0.5}
isSubmitting={true}
auth="https://auth.example.com"
/>,
);
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(
<ToolCall
{...mockProps}
name={`oauth${d}foo${d}bar`}
initialProgress={0.5}
isSubmitting={true}
auth="https://auth.example.com"
/>,
);
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(
<ToolCall
{...mockProps}
name={`oauth${d}my-server`}
initialProgress={1}
isSubmitting={false}
output="done"
auth="https://auth.example.com"
/>,
);
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(
<ToolCall
{...mockProps}
name={`oauth${d}my-server`}
initialProgress={1}
isSubmitting={false}
output="done"
/>,
);
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(
<ToolCall
{...mockProps}
name="bare_name"
initialProgress={0.5}
isSubmitting={true}
auth={authUrl}
/>,
);
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(
<ToolCall
{...mockProps}
name="bare_name"
initialProgress={1}
isSubmitting={false}
output="done"
auth={authUrl}
/>,
);
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(
<ToolCall
{...mockProps}
name={`oauth${d}oauth${d}server`}
initialProgress={0.5}
isSubmitting={true}
auth="https://auth.example.com"
/>,
);
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(
<ToolCall
{...mockProps}
name="action_name"
initialProgress={0.5}
isSubmitting={true}
auth={authUrl}
/>,
);
expect(screen.queryByTestId('subtitle')).not.toBeInTheDocument();
});
});
describe('A11Y-04: screen reader status announcements', () => {
it('includes sr-only aria-live region for status announcements', () => {
renderWithRecoil(