diff --git a/packages/api/src/mcp/ConnectionsRepository.ts b/packages/api/src/mcp/ConnectionsRepository.ts index 5296f4fc82..76a315501f 100644 --- a/packages/api/src/mcp/ConnectionsRepository.ts +++ b/packages/api/src/mcp/ConnectionsRepository.ts @@ -78,7 +78,7 @@ export class ConnectionsRepository { } } const registry = MCPServersRegistry.getInstance(); - const { allowedDomains, allowedAddresses, useSSRFProtection } = + const { allowedDomains, allowedAddresses, useSSRFProtection, appsEnabled } = await registry.resolveAllowlists({ userId: this.ownerId }); const connection = await MCPConnectionFactory.create( { @@ -88,7 +88,7 @@ export class ConnectionsRepository { useSSRFProtection, allowedDomains, allowedAddresses, - enableApps: registry.getAppsEnabled(), + enableApps: appsEnabled, }, this.oauthOpts, ); diff --git a/packages/api/src/mcp/MCPManager.ts b/packages/api/src/mcp/MCPManager.ts index e364f126c3..178b161a4f 100644 --- a/packages/api/src/mcp/MCPManager.ts +++ b/packages/api/src/mcp/MCPManager.ts @@ -32,6 +32,7 @@ import { requiresUserScopedConnection, } from './utils'; import { mcpOptionsContainGraphTokenPlaceholder, preProcessGraphTokens } from '~/utils/graph'; +import { formatToolContent, resultHasRenderableUiResource } from './parsers'; import { getToolUiResourceUri, isToolVisibilityModelOnly } from './apps'; import { MCPServersInitializer } from './registry/MCPServersInitializer'; import { OboTokenResolutionError, resolveOboToken } from '~/mcp/oauth'; @@ -40,7 +41,6 @@ import { MCPServersRegistry } from './registry/MCPServersRegistry'; import { UserConnectionManager } from './UserConnectionManager'; import { ConnectionsRepository } from './ConnectionsRepository'; import { MCPConnectionFactory } from './MCPConnectionFactory'; -import { formatToolContent } from './parsers'; import { MCPConnection } from './connection'; import { processMCPEnv } from '~/utils/env'; @@ -726,24 +726,28 @@ Please follow these instructions when using tools from the respective MCP server userId, requiresEphemeralUserConnection(rawConfig), ); - if (resourceMeta) { - // Honor the per-request apps setting so a tenant that disabled apps gets no UI resource - // (it would otherwise render as a broken iframe once the gated app endpoints reject the - // follow-up calls). Resolved lazily so non-app tools skip the per-request lookup. - const { appsEnabled } = await registry.resolveAllowlists({ userId, role: user?.role }); - if (!appsEnabled) { - resourceMeta = undefined; - } else { - logger.debug( - `[MCP][${serverName}][${toolName}] Found resourceUri: ${resourceMeta.uri}`, - ); - } - } } catch { /* empty */ } } + // The apps toggle must gate both the tool-declared app and any ui:// resource embedded in the + // result (either renders an iframe that breaks once the gated app endpoints reject follow-up + // calls). Resolved lazily, only when a UI resource is in play, so plain tool calls skip the + // per-request lookup. + let enableApps = true; + if (resourceMeta || resultHasRenderableUiResource(result as t.MCPToolCallResponse)) { + ({ appsEnabled: enableApps } = await registry.resolveAllowlists({ + userId, + role: user?.role, + })); + if (!enableApps) { + resourceMeta = undefined; + } else if (resourceMeta) { + logger.debug(`[MCP][${serverName}][${toolName}] Found resourceUri: ${resourceMeta.uri}`); + } + } + return formatToolContent( result as t.MCPToolCallResponse, provider, @@ -755,8 +759,9 @@ Please follow these instructions when using tools from the respective MCP server csp: resourceMeta?.csp, permissions: resourceMeta?.permissions, toolArgs: toolArguments, + enableApps, } - : undefined, + : { enableApps }, ); } catch (error) { // Log with context and re-throw or handle as needed @@ -1033,11 +1038,14 @@ Please follow these instructions when using tools from the respective MCP server /** * Fully percent-decodes a URI to the canonical form a server resolves. Returns null when it - * cannot be decoded or contains a relative (`.`/`..`) segment, so neither encoded traversal nor - * relative segments can satisfy a template guard. + * cannot be decoded, does not stabilize within the decode cap, or contains a relative (`.`/`..`) + * segment, so neither deeply encoded traversal nor relative segments can satisfy a template + * guard. Failing closed on the cap matters because a server that decodes until stable would + * otherwise receive a traversal this guard never saw in decoded form. */ private static canonicalizeUri(uri: string): string | null { let current = uri; + let stabilized = false; for (let depth = 0; depth < 5; depth++) { let decoded: string; try { @@ -1046,10 +1054,14 @@ Please follow these instructions when using tools from the respective MCP server return null; } if (decoded === current) { + stabilized = true; break; } current = decoded; } + if (!stabilized) { + return null; + } if (current.split(/[/\\]/).some((segment) => segment === '.' || segment === '..')) { return null; } diff --git a/packages/api/src/mcp/__tests__/MCPManager.test.ts b/packages/api/src/mcp/__tests__/MCPManager.test.ts index 20dc87954f..99453d316b 100644 --- a/packages/api/src/mcp/__tests__/MCPManager.test.ts +++ b/packages/api/src/mcp/__tests__/MCPManager.test.ts @@ -1481,6 +1481,36 @@ describe('MCPManager', () => { const methods = request.mock.calls.map((c) => (c[0] as { method: string }).method); expect(methods).not.toContain('resources/read'); }); + + it('rejects traversal encoded more deeply than the decode cap rather than treating it as canonical', async () => { + let traversal = '%2e%2e%2f'; + for (let i = 0; i < 6; i++) { + traversal = traversal.replace(/%/g, '%25'); + } + const request = jest.fn().mockImplementation((req: { method: string }) => { + if (req.method === 'resources/list') { + return Promise.resolve({ resources: [] }); + } + if (req.method === 'resources/templates/list') { + return Promise.resolve({ resourceTemplates: [{ uriTemplate: 'file://docs{+path}' }] }); + } + return Promise.resolve({ contents: [] }); + }); + const manager = await MCPManager.createInstance(newMCPServersConfig()); + jest.spyOn(manager, 'getConnection').mockResolvedValue(buildConnection(request)); + + await expect( + manager.readResource({ + userId: 'user-123', + serverName: 'srv', + uri: `file://docs/${traversal}secret`, + user: mockUser as IUser, + }), + ).rejects.toThrow(/not advertised/); + + const methods = request.mock.calls.map((c) => (c[0] as { method: string }).method); + expect(methods).not.toContain('resources/read'); + }); }); describe('getConnection', () => { diff --git a/packages/api/src/mcp/__tests__/parsers.test.ts b/packages/api/src/mcp/__tests__/parsers.test.ts index dec533d4a2..93e788e14e 100644 --- a/packages/api/src/mcp/__tests__/parsers.test.ts +++ b/packages/api/src/mcp/__tests__/parsers.test.ts @@ -328,6 +328,27 @@ describe('formatToolContent', () => { expect(uiResourceArtifact?.content).toEqual(result.content); }); + it('suppresses embedded ui:// resources when apps are disabled for the scope', () => { + const result: t.MCPToolCallResponse = { + content: [ + { + type: 'resource', + resource: { uri: 'ui://app', mimeType: 'text/html', text: '
hi
' }, + }, + ], + }; + + const [content, artifacts] = formatToolContent(result, 'openai', { + serverName: 'srv', + toolName: 'do_thing', + enableApps: false, + }); + + expect(artifacts?.ui_resources).toBeUndefined(); + expect(content).toContain('Resource URI: ui://app'); + expect(content).not.toContain('UI Resource Marker:'); + }); + it('gives embedded ui:// resources distinct ids per tool result payload', () => { const resourceIdFor = (sc: Record