mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-07-01 11:53:55 +00:00
fix(mcp): advertise UI capability at host level and align app-resource handling
Per the MCP Apps spec (SEP-1865) the io.modelcontextprotocol/ui capability is negotiated once per client-server session and declares the host's rendering ability, not a per-user preference; per-tenant apps policy belongs in the host's downstream layer. Advertise the capability from the global apps setting at every connection-building path instead of the per-request resolved value, so a shared serverName-keyed connection no longer has its capability fixed by whichever scope opened it first. Per-tenant apps-enabled stays enforced downstream in callTool resource attachment and the app endpoints. Classify only text/html;profile=mcp-app resources as app-backed so the bridge metadata matches the client isMcpAppResource check; a plain text/html ui:// resource renders as a static srcDoc on both sides instead of carrying dead tool-result metadata. Redact the tool-result _meta (resultMeta) from ui_resources during share serialization so model-hidden metadata cannot ride into a public shared transcript. Resolve app follow-up requests through the originating tool call's connection path rather than forcing a user-scoped connection, which the app-level guard rejects for shared servers.
This commit is contained in:
parent
2905c1563b
commit
d87c12b7ec
8 changed files with 127 additions and 46 deletions
|
|
@ -78,7 +78,7 @@ export class ConnectionsRepository {
|
|||
}
|
||||
}
|
||||
const registry = MCPServersRegistry.getInstance();
|
||||
const { allowedDomains, allowedAddresses, useSSRFProtection, appsEnabled } =
|
||||
const { allowedDomains, allowedAddresses, useSSRFProtection } =
|
||||
await registry.resolveAllowlists({ userId: this.ownerId });
|
||||
const connection = await MCPConnectionFactory.create(
|
||||
{
|
||||
|
|
@ -88,7 +88,7 @@ export class ConnectionsRepository {
|
|||
useSSRFProtection,
|
||||
allowedDomains,
|
||||
allowedAddresses,
|
||||
enableApps: appsEnabled,
|
||||
enableApps: registry.getAppsEnabled(),
|
||||
},
|
||||
this.oauthOpts,
|
||||
);
|
||||
|
|
|
|||
|
|
@ -195,7 +195,7 @@ export class MCPManager extends UserConnectionManager {
|
|||
}
|
||||
|
||||
const registry = MCPServersRegistry.getInstance();
|
||||
const { allowedDomains, allowedAddresses, useSSRFProtection, appsEnabled } =
|
||||
const { allowedDomains, allowedAddresses, useSSRFProtection } =
|
||||
await registry.resolveAllowlists({ userId: user?.id, role: user?.role });
|
||||
await this.assertResolvedRuntimeConfigAllowed({
|
||||
config: serverConfig,
|
||||
|
|
@ -217,7 +217,7 @@ export class MCPManager extends UserConnectionManager {
|
|||
useSSRFProtection,
|
||||
allowedDomains,
|
||||
allowedAddresses,
|
||||
enableApps: appsEnabled,
|
||||
enableApps: registry.getAppsEnabled(),
|
||||
};
|
||||
|
||||
const finalizeDiscoveryResult = async (
|
||||
|
|
@ -656,7 +656,7 @@ Please follow these instructions when using tools from the respective MCP server
|
|||
resolvedHeaders['Authorization'] = `Bearer ${oboTokens.access_token}`;
|
||||
}
|
||||
if (userId && user && oauthStart && flowManager && isOAuthServer(currentOptions)) {
|
||||
const { allowedDomains, allowedAddresses, useSSRFProtection, appsEnabled } =
|
||||
const { allowedDomains, allowedAddresses, useSSRFProtection } =
|
||||
await registry.resolveAllowlists({ userId, role: user?.role });
|
||||
cleanupRequestOAuthHandler = MCPConnectionFactory.attachRequestOAuthHandler(
|
||||
{
|
||||
|
|
@ -667,7 +667,7 @@ Please follow these instructions when using tools from the respective MCP server
|
|||
useSSRFProtection,
|
||||
allowedDomains,
|
||||
allowedAddresses,
|
||||
enableApps: appsEnabled,
|
||||
enableApps: registry.getAppsEnabled(),
|
||||
},
|
||||
{
|
||||
useOAuth: true,
|
||||
|
|
@ -837,28 +837,14 @@ Please follow these instructions when using tools from the respective MCP server
|
|||
}
|
||||
}
|
||||
|
||||
// A config-tier override for this name resolves above via configServers, but the shared
|
||||
// app-connection pool resolves configs without it and would return the base server's
|
||||
// connection. Route overrides through a request-scoped connection so iframe reads/calls reach
|
||||
// the overridden server rather than the cached base one.
|
||||
const hasConfigOverride = !!user && configServers?.[serverName] != null;
|
||||
const connection = hasConfigOverride
|
||||
? await this.getUserConnection({
|
||||
serverName,
|
||||
user,
|
||||
serverConfig: rawConfig ?? undefined,
|
||||
customUserVars,
|
||||
flowManager,
|
||||
tokenMethods,
|
||||
})
|
||||
: await this.getConnection({
|
||||
serverName,
|
||||
user,
|
||||
serverConfig: rawConfig ?? undefined,
|
||||
customUserVars,
|
||||
flowManager,
|
||||
tokenMethods,
|
||||
});
|
||||
const connection = await this.getConnection({
|
||||
serverName,
|
||||
user,
|
||||
serverConfig: rawConfig ?? undefined,
|
||||
customUserVars,
|
||||
flowManager,
|
||||
tokenMethods,
|
||||
});
|
||||
|
||||
// Refresh headers when the config can be fully resolved: env-var-only configs always, and
|
||||
// customUserVar configs only when the route supplied those vars. Without them, re-processing
|
||||
|
|
|
|||
|
|
@ -453,7 +453,7 @@ export abstract class UserConnectionManager {
|
|||
graphTokenResolver,
|
||||
});
|
||||
const registry = MCPServersRegistry.getInstance();
|
||||
const { allowedDomains, allowedAddresses, useSSRFProtection, appsEnabled } =
|
||||
const { allowedDomains, allowedAddresses, useSSRFProtection } =
|
||||
await registry.resolveAllowlists({ userId: user?.id, role: user?.role });
|
||||
await this.assertResolvedRuntimeConfigAllowed({
|
||||
config: runtimeConfig,
|
||||
|
|
@ -472,7 +472,7 @@ export abstract class UserConnectionManager {
|
|||
useSSRFProtection,
|
||||
allowedDomains,
|
||||
allowedAddresses,
|
||||
enableApps: appsEnabled,
|
||||
enableApps: registry.getAppsEnabled(),
|
||||
ephemeralConnection,
|
||||
};
|
||||
|
||||
|
|
|
|||
|
|
@ -1279,7 +1279,7 @@ describe('MCPManager', () => {
|
|||
});
|
||||
});
|
||||
|
||||
it('routes config-override app requests through a request-scoped connection honoring the override', async () => {
|
||||
it('forwards configServers, flowManager, and tokenMethods to getConnection', async () => {
|
||||
(mockRegistryInstance.getServerConfig as jest.Mock).mockResolvedValue({
|
||||
source: 'yaml',
|
||||
type: 'sse',
|
||||
|
|
@ -1295,8 +1295,8 @@ describe('MCPManager', () => {
|
|||
} as unknown as MCPConnection;
|
||||
|
||||
const manager = await MCPManager.createInstance(newMCPServersConfig());
|
||||
const getUserConnectionSpy = jest
|
||||
.spyOn(manager, 'getUserConnection')
|
||||
const getConnectionSpy = jest
|
||||
.spyOn(manager, 'getConnection')
|
||||
.mockResolvedValue(mockConnection);
|
||||
|
||||
const flowManager = {} as never;
|
||||
|
|
@ -1319,8 +1319,8 @@ describe('MCPManager', () => {
|
|||
'user-123',
|
||||
configServers,
|
||||
);
|
||||
expect(getUserConnectionSpy).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ serverName: 'cfg-server', flowManager, tokenMethods }),
|
||||
expect(getConnectionSpy).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ flowManager, tokenMethods }),
|
||||
);
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -301,12 +301,16 @@ describe('formatToolContent', () => {
|
|||
expect(artifacts).toBeUndefined();
|
||||
});
|
||||
|
||||
it('attaches the tool result to embedded ui:// resources for the app bridge', () => {
|
||||
it('attaches the tool result to embedded mcp-app resources for the app bridge', () => {
|
||||
const result: t.MCPToolCallResponse = {
|
||||
content: [
|
||||
{
|
||||
type: 'resource',
|
||||
resource: { uri: 'ui://app', mimeType: 'text/html', text: '<p>hi</p>' },
|
||||
resource: {
|
||||
uri: 'ui://app',
|
||||
mimeType: 'text/html;profile=mcp-app',
|
||||
text: '<p>hi</p>',
|
||||
},
|
||||
},
|
||||
],
|
||||
structuredContent: { count: 3 },
|
||||
|
|
@ -328,6 +332,31 @@ describe('formatToolContent', () => {
|
|||
expect(uiResourceArtifact?.content).toEqual(result.content);
|
||||
});
|
||||
|
||||
it('renders a plain text/html ui:// resource statically without app-bridge metadata', () => {
|
||||
const result: t.MCPToolCallResponse = {
|
||||
content: [
|
||||
{
|
||||
type: 'resource',
|
||||
resource: { uri: 'ui://static', mimeType: 'text/html', text: '<p>hi</p>' },
|
||||
},
|
||||
],
|
||||
structuredContent: { count: 3 },
|
||||
};
|
||||
|
||||
const [content, artifacts] = formatToolContent(result, 'openai', {
|
||||
serverName: 'srv',
|
||||
toolName: 'do_thing',
|
||||
});
|
||||
|
||||
const uiResourceArtifact = artifacts?.ui_resources?.data?.[0];
|
||||
expect(content).toContain('UI Resource Marker:');
|
||||
expect(uiResourceArtifact).toMatchObject({ uri: 'ui://static' });
|
||||
expect(uiResourceArtifact?.serverName).toBeUndefined();
|
||||
expect(uiResourceArtifact?.toolName).toBeUndefined();
|
||||
expect(uiResourceArtifact?.structuredContent).toBeUndefined();
|
||||
expect(uiResourceArtifact?.resultMeta).toBeUndefined();
|
||||
});
|
||||
|
||||
it('suppresses embedded ui:// resources when apps are disabled for the scope', () => {
|
||||
const result: t.MCPToolCallResponse = {
|
||||
content: [
|
||||
|
|
|
|||
|
|
@ -264,18 +264,27 @@ export function formatToolContent(
|
|||
const itemUi = (item.resource._meta as { ui?: Record<string, unknown> } | undefined)?.ui as
|
||||
| { csp?: UIResource['csp']; permissions?: UIResource['permissions'] }
|
||||
| undefined;
|
||||
// Only the text/html;profile=mcp-app profile runs the App Bridge on the client
|
||||
// (isMcpAppResource); a plain text/html ui:// resource renders as a static srcDoc, so the
|
||||
// tool result/context fields would be dead, misleading metadata on it. Classify the same
|
||||
// way on both sides and attach the bridge payload only for the app profile.
|
||||
const isAppBacked = (item.resource.mimeType ?? '').includes('profile=mcp-app');
|
||||
const uiResource: UIResource = {
|
||||
...item.resource,
|
||||
resourceId,
|
||||
serverName: metadata?.serverName,
|
||||
toolName: metadata?.toolName,
|
||||
structuredContent: result?.structuredContent,
|
||||
content: result?.content,
|
||||
isError: result?.isError,
|
||||
resultMeta: (result as { _meta?: Record<string, unknown> })?._meta,
|
||||
csp: itemUi?.csp ?? metadata?.csp,
|
||||
permissions: itemUi?.permissions ?? metadata?.permissions,
|
||||
toolArgs: metadata?.toolArgs,
|
||||
...(isAppBacked
|
||||
? {
|
||||
serverName: metadata?.serverName,
|
||||
toolName: metadata?.toolName,
|
||||
structuredContent: result?.structuredContent,
|
||||
content: result?.content,
|
||||
isError: result?.isError,
|
||||
resultMeta: (result as { _meta?: Record<string, unknown> })?._meta,
|
||||
toolArgs: metadata?.toolArgs,
|
||||
}
|
||||
: {}),
|
||||
};
|
||||
uiResources.push(uiResource);
|
||||
resourceText.push(`UI Resource ID: ${resourceId}`);
|
||||
|
|
|
|||
|
|
@ -543,6 +543,46 @@ describe('Share Methods', () => {
|
|||
expect(attachment).not.toHaveProperty('storageKey');
|
||||
expect(attachment).not.toHaveProperty('metadata');
|
||||
});
|
||||
|
||||
test('strips resultMeta from shared ui_resources while keeping render fields', async () => {
|
||||
const userId = new mongoose.Types.ObjectId().toString();
|
||||
const conversationId = `conv_${nanoid()}`;
|
||||
const shareId = `share_${nanoid()}`;
|
||||
|
||||
const message = await Message.create({
|
||||
messageId: `msg_${nanoid()}`,
|
||||
conversationId,
|
||||
user: userId,
|
||||
text: 'has app',
|
||||
isCreatedByUser: false,
|
||||
attachments: [
|
||||
{
|
||||
type: 'ui_resources',
|
||||
ui_resources: [
|
||||
{
|
||||
resourceId: 'res1',
|
||||
uri: 'ui://app',
|
||||
mimeType: 'text/html;profile=mcp-app',
|
||||
text: '<p>hi</p>',
|
||||
serverName: 'srv',
|
||||
toolName: 'do_thing',
|
||||
resultMeta: { secret: 'hidden-from-model' },
|
||||
},
|
||||
],
|
||||
},
|
||||
],
|
||||
});
|
||||
|
||||
await SharedLink.create({ shareId, conversationId, user: userId, messages: [message._id] });
|
||||
|
||||
const result = await shareMethods.getSharedMessages(shareId);
|
||||
const attachment = result?.messages[0]?.attachments?.[0] as unknown as {
|
||||
ui_resources?: Array<Record<string, unknown>>;
|
||||
};
|
||||
const resource = attachment?.ui_resources?.[0];
|
||||
expect(resource).toMatchObject({ uri: 'ui://app', resourceId: 'res1', text: '<p>hi</p>' });
|
||||
expect(resource).not.toHaveProperty('resultMeta');
|
||||
});
|
||||
});
|
||||
|
||||
describe('getSharedLinks', () => {
|
||||
|
|
|
|||
|
|
@ -67,6 +67,19 @@ const SENSITIVE_SHARED_FILE_FIELDS = new Set([
|
|||
'metadata',
|
||||
]);
|
||||
|
||||
/**
|
||||
* The MCP tool result `_meta` is carried on a UI resource as `resultMeta` for the App Bridge to
|
||||
* hydrate from, but it is intentionally kept out of the model-visible result and must not become
|
||||
* part of a public shared transcript. MCP apps never render in a shared view anyway, so drop it.
|
||||
*/
|
||||
function sanitizeSharedUIResource(resource: unknown): unknown {
|
||||
if (!resource || typeof resource !== 'object' || Array.isArray(resource)) {
|
||||
return resource;
|
||||
}
|
||||
const { resultMeta: _resultMeta, ...rest } = resource as Record<string, unknown>;
|
||||
return rest;
|
||||
}
|
||||
|
||||
/**
|
||||
* Strip storage/identity-internal fields from a file or attachment while keeping
|
||||
* render-relevant data (including tool-call payloads keyed by tool name).
|
||||
|
|
@ -78,9 +91,13 @@ function sanitizeSharedFile(value: unknown): t.SharedFile | null {
|
|||
|
||||
const result: t.SharedFile = {};
|
||||
for (const [key, fieldValue] of Object.entries(value as Record<string, unknown>)) {
|
||||
if (!SENSITIVE_SHARED_FILE_FIELDS.has(key)) {
|
||||
result[key] = fieldValue;
|
||||
if (SENSITIVE_SHARED_FILE_FIELDS.has(key)) {
|
||||
continue;
|
||||
}
|
||||
result[key] =
|
||||
key === 'ui_resources' && Array.isArray(fieldValue)
|
||||
? fieldValue.map(sanitizeSharedUIResource)
|
||||
: fieldValue;
|
||||
}
|
||||
|
||||
return Object.keys(result).length > 0 ? result : null;
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue