From 2905c1563b92d45b767b2fecbdb9d5dc7e62b404 Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Mon, 29 Jun 2026 12:09:16 -0700 Subject: [PATCH] fix(mcp): honor per-request apps flag in discovery and tighten app resource routing Use the appsEnabled value resolved by resolveAllowlists in the tool-discovery connection path so an OAuth/reinitialize fallback discovery does not advertise the UI extension for a user whose effective config disabled apps. Constrain simple URI-template expansions to exclude query delimiters so a value like q={q} cannot match q=foo&admin=true and authorize an undeclared parameter on app resources/read. Route app requests that carry a config-tier override through a request-scoped connection so iframe reads and tool calls reach the overridden server instead of the cached base app connection. --- packages/api/src/mcp/MCPManager.ts | 40 +++++++++---- .../api/src/mcp/__tests__/MCPManager.test.ts | 60 +++++++++++++++++-- 2 files changed, 83 insertions(+), 17 deletions(-) diff --git a/packages/api/src/mcp/MCPManager.ts b/packages/api/src/mcp/MCPManager.ts index 178b161a4f..f259f9af90 100644 --- a/packages/api/src/mcp/MCPManager.ts +++ b/packages/api/src/mcp/MCPManager.ts @@ -195,7 +195,7 @@ export class MCPManager extends UserConnectionManager { } const registry = MCPServersRegistry.getInstance(); - const { allowedDomains, allowedAddresses, useSSRFProtection } = + const { allowedDomains, allowedAddresses, useSSRFProtection, appsEnabled } = 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: registry.getAppsEnabled(), + enableApps: appsEnabled, }; const finalizeDiscoveryResult = async ( @@ -837,14 +837,28 @@ Please follow these instructions when using tools from the respective MCP server } } - const connection = await this.getConnection({ - serverName, - user, - serverConfig: rawConfig ?? undefined, - customUserVars, - flowManager, - tokenMethods, - }); + // 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, + }); // 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 @@ -1124,8 +1138,10 @@ Please follow these instructions when using tools from the respective MCP server case '&': // query continuation: only the declared parameter names pattern += keys ? `(?:&(?:${keys})=[^#&]*)+` : '&[^#]*'; break; - default: // simple expansion: a single value, no reserved chars - pattern += '[^/?#]+'; + default: // simple expansion: a single value. RFC 6570 percent-encodes reserved chars, + // so a real value never contains a raw `&` or `=`; excluding them stops a query value + // like `q={q}` from matching `q=foo&admin=true` and authorizing an undeclared param. + pattern += '[^/?#&=]+'; } i = end + 1; } diff --git a/packages/api/src/mcp/__tests__/MCPManager.test.ts b/packages/api/src/mcp/__tests__/MCPManager.test.ts index 99453d316b..1a6e045554 100644 --- a/packages/api/src/mcp/__tests__/MCPManager.test.ts +++ b/packages/api/src/mcp/__tests__/MCPManager.test.ts @@ -1279,7 +1279,7 @@ describe('MCPManager', () => { }); }); - it('forwards configServers, flowManager, and tokenMethods to getConnection', async () => { + it('routes config-override app requests through a request-scoped connection honoring the override', 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 getConnectionSpy = jest - .spyOn(manager, 'getConnection') + const getUserConnectionSpy = jest + .spyOn(manager, 'getUserConnection') .mockResolvedValue(mockConnection); const flowManager = {} as never; @@ -1319,8 +1319,8 @@ describe('MCPManager', () => { 'user-123', configServers, ); - expect(getConnectionSpy).toHaveBeenCalledWith( - expect.objectContaining({ flowManager, tokenMethods }), + expect(getUserConnectionSpy).toHaveBeenCalledWith( + expect.objectContaining({ serverName: 'cfg-server', flowManager, tokenMethods }), ); }); @@ -1511,6 +1511,56 @@ describe('MCPManager', () => { const methods = request.mock.calls.map((c) => (c[0] as { method: string }).method); expect(methods).not.toContain('resources/read'); }); + + it('rejects a query value that smuggles an undeclared parameter past a simple-variable template', async () => { + 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: 'search://items?q={q}' }] }); + } + 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: 'search://items?q=foo&admin=true', + 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'); + }); + + it('allows a single query value that matches a simple-variable template', async () => { + 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: 'search://items?q={q}' }] }); + } + return Promise.resolve({ contents: [] }); + }); + const manager = await MCPManager.createInstance(newMCPServersConfig()); + jest.spyOn(manager, 'getConnection').mockResolvedValue(buildConnection(request)); + + await manager.readResource({ + userId: 'user-123', + serverName: 'srv', + uri: 'search://items?q=foo', + user: mockUser as IUser, + }); + + const methods = request.mock.calls.map((c) => (c[0] as { method: string }).method); + expect(methods).toContain('resources/read'); + }); }); describe('getConnection', () => {