fix(mcp): gate apps per-request on app connections and embedded UI resources

Resolve the allowlist-derived appsEnabled value when creating app-level connections in
ConnectionsRepository so a tenant/role/user override that toggles apps is honored instead of the
boot YAML default.

Gate ui:// resources embedded in tool results on the same per-request setting so a disabled scope
renders them as plain resource text rather than a sandboxed app, resolving appsEnabled lazily only
when a result actually carries a renderable UI resource.

Fail closed in canonicalizeUri when a URI does not stabilize within the decode cap so traversal
encoded more deeply than the cap cannot satisfy a template guard a fully-decoding server resolves
as a parent-directory path.
This commit is contained in:
Dustin Healy 2026-06-29 11:07:55 -07:00
parent 1a70dce24b
commit 624a6d8f4b
5 changed files with 113 additions and 25 deletions

View file

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

View file

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

View file

@ -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', () => {

View file

@ -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: '<p>hi</p>' },
},
],
};
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<string, unknown>) =>
formatToolContent(

View file

@ -155,6 +155,32 @@ function parseAsString(result: t.MCPToolCallResponse): string {
return text;
}
/**
* MCP Apps renders only `ui://` resources whose mime type is HTML (mime omitted defaults to HTML),
* the single renderable resource type the spec defines. Used both when attaching a result resource
* and when deciding whether a result carries an app the apps toggle must gate.
*/
export function isRenderableUiResource(item: t.ToolContentPart): boolean {
if (item.type !== 'resource') {
return false;
}
const uri = item.resource?.uri;
if (typeof uri !== 'string' || !uri.startsWith('ui://')) {
return false;
}
const mimeType =
typeof item.resource.mimeType === 'string' ? item.resource.mimeType : 'text/html';
return mimeType.includes('html');
}
export function resultHasRenderableUiResource(result: t.MCPToolCallResponse): boolean {
const content = result?.content;
if (!Array.isArray(content)) {
return false;
}
return content.some((item) => isRenderableUiResource(item as t.ToolContentPart));
}
/**
* Converts MCPToolCallResponse content into a plain-text string plus optional artifacts
* (images, UI resources). All providers receive string content; images are separated into
@ -174,6 +200,7 @@ export function formatToolContent(
csp?: UIResource['csp'];
permissions?: UIResource['permissions'];
toolArgs?: Record<string, unknown>;
enableApps?: boolean;
},
): t.FormattedContentResult {
if (!RECOGNIZED_PROVIDERS.has(provider)) {
@ -216,12 +243,10 @@ export function formatToolContent(
},
resource: (item) => {
// MCP Apps defines a single renderable resource type, text/html;profile=mcp-app, and the
// host renders HTML only. ui:// resources with other mime types (json, remote-dom) have no
// renderer, so they fall through to plain resource text instead of an unrenderable marker.
const mimeType =
typeof item.resource.mimeType === 'string' ? item.resource.mimeType : 'text/html';
const isUiResource = item.resource.uri.startsWith('ui://') && mimeType.includes('html');
// ui:// resources that are not renderable apps (other mime types) or that arrive for a
// scope with apps disabled fall through to plain resource text rather than an unrenderable
// or admin-suppressed app marker.
const isUiResource = metadata?.enableApps !== false && isRenderableUiResource(item);
const resourceText: string[] = [];
if (isUiResource) {