mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-07-03 04:42:11 +00:00
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.
This commit is contained in:
parent
624a6d8f4b
commit
2905c1563b
2 changed files with 83 additions and 17 deletions
|
|
@ -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;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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', () => {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue