mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-03-24 08:36:33 +01:00
🔑 fix: Type-Safe User Context Forwarding for Non-OAuth Tool Discovery (#12348)
Some checks failed
Docker Dev Images Build / build (Dockerfile, librechat-dev, node) (push) Has been cancelled
Docker Dev Images Build / build (Dockerfile.multi, librechat-dev-api, api-build) (push) Has been cancelled
Sync Locize Translations & Create Translation PR / Sync Translation Keys with Locize (push) Has been cancelled
Sync Locize Translations & Create Translation PR / Create Translation PR on Version Published (push) Has been cancelled
Some checks failed
Docker Dev Images Build / build (Dockerfile, librechat-dev, node) (push) Has been cancelled
Docker Dev Images Build / build (Dockerfile.multi, librechat-dev-api, api-build) (push) Has been cancelled
Sync Locize Translations & Create Translation PR / Sync Translation Keys with Locize (push) Has been cancelled
Sync Locize Translations & Create Translation PR / Create Translation PR on Version Published (push) Has been cancelled
* fix(mcp): pass missing customUserVars and user during unauthenticated tool discovery * fix(mcp): type-safe user context forwarding for non-OAuth tool discovery Extract UserConnectionContext from OAuthConnectionOptions to properly model the non-OAuth case where user/customUserVars/requestBody need placeholder resolution without requiring OAuth-specific fields. - Remove prohibited `as unknown as` double-cast - Forward requestBody and connectionTimeout (previously omitted) - Add unit tests for argument forwarding at Manager and Factory layers - Add integration test exercising real processMCPEnv substitution --------- Co-authored-by: Danny Avila <danny@librechat.ai>
This commit is contained in:
parent
0736ff2668
commit
290984c514
6 changed files with 188 additions and 23 deletions
|
|
@ -58,9 +58,13 @@ export class MCPConnectionFactory {
|
|||
*/
|
||||
static async discoverTools(
|
||||
basic: t.BasicConnectionOptions,
|
||||
oauth?: Omit<t.OAuthConnectionOptions, 'returnOnOAuth'>,
|
||||
options?: Omit<t.OAuthConnectionOptions, 'returnOnOAuth'> | t.UserConnectionContext,
|
||||
): Promise<ToolDiscoveryResult> {
|
||||
const factory = new this(basic, oauth ? { ...oauth, returnOnOAuth: true } : undefined);
|
||||
if (options != null && 'useOAuth' in options) {
|
||||
const factory = new this(basic, { ...options, returnOnOAuth: true });
|
||||
return factory.discoverToolsInternal();
|
||||
}
|
||||
const factory = new this(basic, options);
|
||||
return factory.discoverToolsInternal();
|
||||
}
|
||||
|
||||
|
|
@ -187,31 +191,36 @@ export class MCPConnectionFactory {
|
|||
return null;
|
||||
}
|
||||
|
||||
protected constructor(basic: t.BasicConnectionOptions, oauth?: t.OAuthConnectionOptions) {
|
||||
protected constructor(
|
||||
basic: t.BasicConnectionOptions,
|
||||
options?: t.OAuthConnectionOptions | t.UserConnectionContext,
|
||||
) {
|
||||
this.serverConfig = processMCPEnv({
|
||||
user: oauth?.user,
|
||||
body: oauth?.requestBody,
|
||||
user: options?.user,
|
||||
body: options?.requestBody,
|
||||
dbSourced: basic.dbSourced,
|
||||
options: basic.serverConfig,
|
||||
customUserVars: oauth?.customUserVars,
|
||||
customUserVars: options?.customUserVars,
|
||||
});
|
||||
this.serverName = basic.serverName;
|
||||
this.useOAuth = !!oauth?.useOAuth;
|
||||
this.useSSRFProtection = basic.useSSRFProtection === true;
|
||||
this.allowedDomains = basic.allowedDomains;
|
||||
this.connectionTimeout = oauth?.connectionTimeout;
|
||||
this.logPrefix = oauth?.user
|
||||
? `[MCP][${basic.serverName}][${oauth.user.id}]`
|
||||
this.connectionTimeout = options?.connectionTimeout;
|
||||
this.logPrefix = options?.user
|
||||
? `[MCP][${basic.serverName}][${options.user.id}]`
|
||||
: `[MCP][${basic.serverName}]`;
|
||||
|
||||
if (oauth?.useOAuth) {
|
||||
this.userId = oauth.user?.id;
|
||||
this.flowManager = oauth.flowManager;
|
||||
this.tokenMethods = oauth.tokenMethods;
|
||||
this.signal = oauth.signal;
|
||||
this.oauthStart = oauth.oauthStart;
|
||||
this.oauthEnd = oauth.oauthEnd;
|
||||
this.returnOnOAuth = oauth.returnOnOAuth;
|
||||
if (options != null && 'useOAuth' in options) {
|
||||
this.useOAuth = true;
|
||||
this.userId = options.user?.id;
|
||||
this.flowManager = options.flowManager;
|
||||
this.tokenMethods = options.tokenMethods;
|
||||
this.signal = options.signal;
|
||||
this.oauthStart = options.oauthStart;
|
||||
this.oauthEnd = options.oauthEnd;
|
||||
this.returnOnOAuth = options.returnOnOAuth;
|
||||
} else {
|
||||
this.useOAuth = false;
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -113,7 +113,12 @@ export class MCPManager extends UserConnectionManager {
|
|||
};
|
||||
|
||||
if (!useOAuth) {
|
||||
const result = await MCPConnectionFactory.discoverTools(basic);
|
||||
const result = await MCPConnectionFactory.discoverTools(basic, {
|
||||
user: args.user,
|
||||
customUserVars: args.customUserVars,
|
||||
requestBody: args.requestBody,
|
||||
connectionTimeout: args.connectionTimeout,
|
||||
});
|
||||
return {
|
||||
tools: result.tools,
|
||||
oauthRequired: result.oauthRequired,
|
||||
|
|
|
|||
|
|
@ -764,6 +764,39 @@ describe('MCPConnectionFactory', () => {
|
|||
expect(result.connection).toBe(mockConnectionInstance);
|
||||
});
|
||||
|
||||
it('should forward user context to processMCPEnv for non-OAuth discovery', async () => {
|
||||
const serverConfig: t.MCPOptions = {
|
||||
type: 'streamable-http',
|
||||
url: 'https://my-mcp.server.com?key={{MY_CUSTOM_KEY}}',
|
||||
} as t.MCPOptions;
|
||||
|
||||
const basicOptions = {
|
||||
serverName: 'test-server',
|
||||
serverConfig,
|
||||
};
|
||||
|
||||
const userContext = {
|
||||
user: mockUser,
|
||||
customUserVars: { MY_CUSTOM_KEY: 'c527bd0abc123' },
|
||||
connectionTimeout: 10000,
|
||||
};
|
||||
|
||||
mockConnectionInstance.connect.mockResolvedValue(undefined);
|
||||
mockConnectionInstance.isConnected.mockResolvedValue(true);
|
||||
mockConnectionInstance.fetchTools = jest.fn().mockResolvedValue(mockTools);
|
||||
|
||||
const result = await MCPConnectionFactory.discoverTools(basicOptions, userContext);
|
||||
|
||||
expect(result.tools).toEqual(mockTools);
|
||||
expect(mockProcessMCPEnv).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
user: mockUser,
|
||||
options: serverConfig,
|
||||
customUserVars: { MY_CUSTOM_KEY: 'c527bd0abc123' },
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it('should detect OAuth required without generating URL in discovery mode', async () => {
|
||||
const basicOptions = {
|
||||
serverName: 'test-server',
|
||||
|
|
|
|||
|
|
@ -847,6 +847,46 @@ describe('MCPManager', () => {
|
|||
expect(MCPConnectionFactory.discoverTools).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('should forward user, customUserVars, requestBody, and connectionTimeout to discoverTools in the non-OAuth path', async () => {
|
||||
const mockUser = { id: 'user123', email: 'test@example.com' } as unknown as IUser;
|
||||
const customUserVars = { MY_CUSTOM_KEY: 'c527bd0abc123' };
|
||||
|
||||
mockAppConnections({
|
||||
get: jest.fn().mockResolvedValue(null),
|
||||
});
|
||||
|
||||
(mockRegistryInstance.getServerConfig as jest.Mock).mockResolvedValue({
|
||||
type: 'streamable-http',
|
||||
url: 'https://my-mcp.server.com?key={{MY_CUSTOM_KEY}}',
|
||||
});
|
||||
|
||||
(MCPConnectionFactory.discoverTools as jest.Mock).mockResolvedValue({
|
||||
tools: mockTools,
|
||||
connection: null,
|
||||
oauthRequired: false,
|
||||
oauthUrl: null,
|
||||
});
|
||||
|
||||
const manager = await MCPManager.createInstance(newMCPServersConfig());
|
||||
await manager.discoverServerTools({
|
||||
serverName,
|
||||
user: mockUser,
|
||||
customUserVars,
|
||||
requestBody: { conversationId: 'conv-123' } as t.ToolDiscoveryOptions['requestBody'],
|
||||
connectionTimeout: 10000,
|
||||
});
|
||||
|
||||
expect(MCPConnectionFactory.discoverTools).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ serverName }),
|
||||
expect.objectContaining({
|
||||
user: mockUser,
|
||||
customUserVars,
|
||||
requestBody: { conversationId: 'conv-123' },
|
||||
connectionTimeout: 10000,
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it('should return null tools when server config not found', async () => {
|
||||
mockAppConnections({
|
||||
get: jest.fn().mockResolvedValue(null),
|
||||
|
|
|
|||
|
|
@ -0,0 +1,74 @@
|
|||
/**
|
||||
* Integration test exercising real processMCPEnv for the non-OAuth
|
||||
* customUserVars scenario: a streamable-http server whose URL contains
|
||||
* a {{PLACEHOLDER}} that must be resolved from per-user custom variables.
|
||||
*
|
||||
* This is the exact bug scenario from PR #12348 — without the fix,
|
||||
* the literal string `{{MY_CUSTOM_KEY}}` would be sent to the MCP
|
||||
* server endpoint instead of the substituted value.
|
||||
*/
|
||||
import type { IUser } from '@librechat/data-schemas';
|
||||
import type * as t from '~/mcp/types';
|
||||
import { processMCPEnv } from '~/utils/env';
|
||||
|
||||
describe('processMCPEnv — customUserVars placeholder resolution', () => {
|
||||
const mockUser = { id: 'user-abc', email: 'test@example.com' } as IUser;
|
||||
|
||||
it('should resolve {{CUSTOM_VAR}} in a streamable-http URL', () => {
|
||||
const serverConfig: t.MCPOptions = {
|
||||
type: 'streamable-http',
|
||||
url: 'https://my-mcp.server.com/server?key={{MY_CUSTOM_KEY}}',
|
||||
} as t.MCPOptions;
|
||||
|
||||
const result = processMCPEnv({
|
||||
options: serverConfig,
|
||||
user: mockUser,
|
||||
customUserVars: { MY_CUSTOM_KEY: 'c527bd0abc123' },
|
||||
});
|
||||
|
||||
expect((result as t.StreamableHTTPOptions).url).toBe(
|
||||
'https://my-mcp.server.com/server?key=c527bd0abc123',
|
||||
);
|
||||
});
|
||||
|
||||
it('should resolve multiple placeholders in URL and headers simultaneously', () => {
|
||||
const serverConfig: t.MCPOptions = {
|
||||
type: 'streamable-http',
|
||||
url: 'https://my-mcp.server.com/server?key={{API_KEY}}&project={{PROJECT_ID}}',
|
||||
headers: {
|
||||
Authorization: 'Bearer {{AUTH_TOKEN}}',
|
||||
'X-Project': '{{PROJECT_ID}}',
|
||||
},
|
||||
} as t.MCPOptions;
|
||||
|
||||
const result = processMCPEnv({
|
||||
options: serverConfig,
|
||||
user: mockUser,
|
||||
customUserVars: {
|
||||
API_KEY: 'key-123',
|
||||
PROJECT_ID: 'proj-456',
|
||||
AUTH_TOKEN: 'tok-789',
|
||||
},
|
||||
});
|
||||
|
||||
const typed = result as t.StreamableHTTPOptions;
|
||||
expect(typed.url).toBe('https://my-mcp.server.com/server?key=key-123&project=proj-456');
|
||||
expect(typed.headers).toEqual({
|
||||
Authorization: 'Bearer tok-789',
|
||||
'X-Project': 'proj-456',
|
||||
});
|
||||
});
|
||||
|
||||
it('should leave unmatched placeholders as literal strings when customUserVars is undefined', () => {
|
||||
const serverConfig: t.MCPOptions = {
|
||||
type: 'streamable-http',
|
||||
url: 'https://my-mcp.server.com/server?key={{MY_CUSTOM_KEY}}',
|
||||
} as t.MCPOptions;
|
||||
|
||||
const result = processMCPEnv({
|
||||
options: serverConfig,
|
||||
});
|
||||
|
||||
expect((result as t.StreamableHTTPOptions).url).toContain('{{MY_CUSTOM_KEY}}');
|
||||
});
|
||||
});
|
||||
|
|
@ -174,18 +174,22 @@ export interface BasicConnectionOptions {
|
|||
dbSourced?: boolean;
|
||||
}
|
||||
|
||||
export interface OAuthConnectionOptions {
|
||||
/** User context for placeholder resolution in MCP connections (non-OAuth and OAuth alike) */
|
||||
export interface UserConnectionContext {
|
||||
user?: IUser;
|
||||
useOAuth: true;
|
||||
requestBody?: RequestBody;
|
||||
customUserVars?: Record<string, string>;
|
||||
requestBody?: RequestBody;
|
||||
connectionTimeout?: number;
|
||||
}
|
||||
|
||||
export interface OAuthConnectionOptions extends UserConnectionContext {
|
||||
useOAuth: true;
|
||||
flowManager: FlowStateManager<o.MCPOAuthTokens | null>;
|
||||
tokenMethods?: TokenMethods;
|
||||
signal?: AbortSignal;
|
||||
oauthStart?: (authURL: string) => Promise<void>;
|
||||
oauthEnd?: () => Promise<void>;
|
||||
returnOnOAuth?: boolean;
|
||||
connectionTimeout?: number;
|
||||
}
|
||||
|
||||
export interface ToolDiscoveryOptions {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue