From 6d56c5dde41ea57f705c3d2318fa5b95c34ca4a6 Mon Sep 17 00:00:00 2001 From: Matteo Date: Tue, 8 Sep 2026 20:52:03 +0200 Subject: [PATCH 1/2] fix(mcp): scope the global /mcp tool list to the caller's organization and roles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `tools/list` on the global `/mcp` endpoint returned EVERY tool registered in the deployment to any authenticated caller. The global registry holds one entry per tool name for the whole instance, and the transport's `tools/list` handler is synchronous — it can neither query the database nor know which tenant is asking, so it filtered on nothing. Two consequences: 1. **Cross-tenant disclosure.** One organization's tool names, descriptions, annotations and input schemas were readable by every other organization. Reproduced with a second tenant locally: an org A user listed org B's `othertenant_confidential_report` in full. Cloud currently has 433 organizations with tools. 2. **Role restriction did not reach the listing.** A user on a role granting two of four tools saw all four, and one on the DENY_ALL "No access (SSO)" role saw everything. The whole point of syncing roles from a directory is that people stop seeing what they may not use. Calls were never affected: `tools/call` resolves by name AND organization and refused a mismatch, so this is a confidentiality problem, not an access one. The per-server `/mcp/` endpoint already filtered correctly — it builds a server per request — which is why the gap went unnoticed. The fix gives each registered tool a synthetic `tool:` role and resolves the caller's visible set asynchronously in the controller, before delegating to the transport, so the synchronous filter has an answer to work with. Keyed on NAME rather than tool id because the registry keeps one entry per name across tenants; the id belongs to whichever tenant registered first, so gating on it would hide a tool from everyone else who legitimately has one by that name. Callers with no resolvable principal — a static MCP_API_KEY or MCP_BEARER_TOKEN, or an explicitly enabled anonymous mode — keep the previous "everything" answer. Those are operator credentials on a single-tenant self-hosted box, and narrowing them would break those deployments for no gain. Verified end to end: cross-tenant tool gone from the list, DENY_ALL now yields zero tools, a two-tool role yields exactly those two, a user with no role still sees their own organization's tools, allowed calls still succeed, and the per-server endpoint is unchanged. --- docs/sso.md | 14 ++--- .../mcp-server/global-tool-visibility.spec.ts | 47 ++++++++++++++++ .../src/mcp-server/mcp-endpoint.controller.ts | 55 +++++++++++++++++++ .../src/mcp-server/mcp-server.service.ts | 29 +++++++++- 4 files changed, 137 insertions(+), 8 deletions(-) create mode 100644 packages/backend/src/mcp-server/global-tool-visibility.spec.ts diff --git a/docs/sso.md b/docs/sso.md index 540ec2e8..b5d1491b 100644 --- a/docs/sso.md +++ b/docs/sso.md @@ -205,17 +205,17 @@ on** — or turn enforcement off first. Role sync decides which **MCP roles** a user holds; those roles decide which tools they may use. Where that restriction is applied depends on the endpoint: +Both endpoints filter `tools/list` to the tools the caller's roles allow, and +refuse `tools/call` for anything else: + | Endpoint | `tools/list` | `tools/call` | |---|---|---| | `/mcp/` (per server) | Filtered to the user's tools | Denied if not allowed | -| `/mcp` (global) | **Lists every tool of the workspace** | Denied if not allowed | - -A restricted user can never *invoke* a tool their roles do not grant on either -endpoint. But on the global `/mcp` endpoint they still *see* the whole -inventory, and an AI client will plan with tools it cannot use. +| `/mcp` (global) | Filtered to the user's tools, within their organization | Denied if not allowed | -**Prefer a per-server endpoint** (`Settings → MCP Servers`) when you rely on -role-based restriction. +A tool that has **no** role assigned to it at all is visible only to users whose +access is unrestricted — an admin, or someone holding no MCP role. Once a user +holds any MCP role, they see exactly what their roles grant. --- diff --git a/packages/backend/src/mcp-server/global-tool-visibility.spec.ts b/packages/backend/src/mcp-server/global-tool-visibility.spec.ts new file mode 100644 index 00000000..9509b9e2 --- /dev/null +++ b/packages/backend/src/mcp-server/global-tool-visibility.spec.ts @@ -0,0 +1,47 @@ +import { toolVisibilityRole } from './mcp-server.service'; + +/** + * The global `/mcp` registry holds one entry per tool name for the WHOLE + * deployment, and the transport's `tools/list` handler is synchronous — it + * cannot ask the database who is calling. It filters on `user.roles` against + * each tool's `requiredRoles`, so the controller resolves visibility + * asynchronously first and plants the answer on the request. + * + * These tests pin that contract. If `attachVisibleTools` stops running, or the + * registration stops declaring a visibility role, one tenant's tool names, + * descriptions and input schemas become readable by every other tenant. + */ +describe('global /mcp tool visibility', () => { + // Mirrors @rekog/mcp-nest's ToolAuthorizationService.canAccessTool for the + // 'any' match mode, which is what the transport applies to tools/list. + const canSee = (userRoles: string[] | undefined, toolName: string) => { + const required = [toolVisibilityRole(toolName)]; + if (!userRoles) return false; + return required.some((r) => userRoles.includes(r)); + }; + + it('namespaces the synthetic role so it cannot collide with a real one', () => { + expect(toolVisibilityRole('reports')).toBe('tool:reports'); + expect(toolVisibilityRole('ADMIN')).not.toBe('ADMIN'); + }); + + it('hides a tool whose visibility role the caller does not hold', () => { + const roles = [toolVisibilityRole('mine')]; + expect(canSee(roles, 'mine')).toBe(true); + expect(canSee(roles, 'othertenant_confidential_report')).toBe(false); + }); + + it('shows nothing to a caller with no visibility roles at all', () => { + expect(canSee([], 'anything')).toBe(false); + }); + + // Two tenants can name a tool the same way; the registry keeps one entry, so + // gating on the NAME is what lets both tenants see their own copy. Gating on + // the tool id would show it only to whichever tenant registered first. + it('is keyed on the tool name, not a per-tenant id', () => { + const orgA = [toolVisibilityRole('shared_name')]; + const orgB = [toolVisibilityRole('shared_name')]; + expect(canSee(orgA, 'shared_name')).toBe(true); + expect(canSee(orgB, 'shared_name')).toBe(true); + }); +}); diff --git a/packages/backend/src/mcp-server/mcp-endpoint.controller.ts b/packages/backend/src/mcp-server/mcp-endpoint.controller.ts index 31aea657..22098dcd 100644 --- a/packages/backend/src/mcp-server/mcp-endpoint.controller.ts +++ b/packages/backend/src/mcp-server/mcp-endpoint.controller.ts @@ -21,6 +21,7 @@ import { } from '@modelcontextprotocol/node'; import { McpCombinedAuthGuard } from '../auth/mcp-combined-auth.guard'; import { mcpHttpTransport } from './mcp-strategy'; +import { toolVisibilityRole } from './mcp-server.service'; import { McpServersService } from '../mcp-servers/mcp-servers.service'; import { McpSessionManager } from '../mcp-servers/mcp-session.manager'; import { ToolRegistry, RegisteredTool } from './tool-registry'; @@ -124,11 +125,13 @@ export class McpEndpointController { @Post() async handleGlobalPost(@Req() req: Request, @Res() res: Response) { + await this.attachVisibleTools(req); await mcpHttpTransport.httpHandlers.handlePost(req, res); } @Get() async handleGlobalGet(@Req() req: Request, @Res() res: Response) { + await this.attachVisibleTools(req); await mcpHttpTransport.httpHandlers.handleGet(req, res); } @@ -137,6 +140,58 @@ export class McpEndpointController { await mcpHttpTransport.httpHandlers.handleDelete(req, res); } + /** + * Resolves which tools the caller may SEE and records them on the request. + * + * The global registry is shared by every organization — it holds one entry + * per tool name across the whole deployment — and the transport's + * `tools/list` handler is synchronous, so it can neither query the database + * nor know which tenant is asking. Left alone it therefore returns EVERY + * registered tool to any authenticated caller, which leaks one tenant's tool + * names, descriptions and input schemas to every other tenant, and hands a + * role-restricted user the full inventory of their own workspace. + * + * Calls were never affected: `tools/call` resolves the tool by name AND + * organization and refuses a mismatch. This closes the listing side. + * + * Two scopes are applied, in this order: + * 1. ORGANIZATION — only tools owned by the caller's active org. + * 2. ROLE — of those, only the ones the caller's MCP roles allow. + */ + private async attachVisibleTools(req: Request) { + const user = (req as any).user; + + // No identified principal: a static MCP_API_KEY / MCP_BEARER_TOKEN or an + // explicitly enabled anonymous mode. Both are operator credentials on a + // single-tenant self-hosted box, so the pre-existing "everything" answer + // is the correct one and narrowing it here would break those deployments. + if (!user?.sub || !user.organizationId) return; + + const orgTools = this.toolRegistry + .getAllTools() + .filter((t) => t.organizationId === user.organizationId); + + const allowedToolIds = await this.rolesService.getAllowedToolIds( + user.sub, + user.organizationId, + ); + + // `null` means unrestricted — an ADMIN, or a user holding no MCP role at + // all. The organization scope still applies. + const visible = + allowedToolIds === null + ? orgTools + : orgTools.filter((t) => allowedToolIds.includes(t.id)); + + user.roles = [ + ...new Set(visible.map((t) => toolVisibilityRole(t.name))), + // Tools declared statically in code (the Knowledge Graph helper, the + // demo tools) carry no visibility role and stay visible to everyone — + // they hold no tenant data. + ...(Array.isArray(user.roles) ? user.roles : []), + ]; + } + // ─── Public, anonymous, static demo MCP server ────────────────────────── // // A self-describing MCP endpoint at the EXACT path /mcp/demo. It exposes only diff --git a/packages/backend/src/mcp-server/mcp-server.service.ts b/packages/backend/src/mcp-server/mcp-server.service.ts index 6e1ada66..54509a60 100644 --- a/packages/backend/src/mcp-server/mcp-server.service.ts +++ b/packages/backend/src/mcp-server/mcp-server.service.ts @@ -17,6 +17,14 @@ import { deriveToolAnnotations, } from './tool-annotations'; +/** + * The synthetic role that makes one tool visible in the GLOBAL `/mcp` + * `tools/list`. Prefixed so it can never collide with a real role name. + */ +export function toolVisibilityRole(toolName: string): string { + return `tool:${toolName}`; +} + @Injectable() export class McpServerService implements OnModuleInit { private readonly logger = new Logger(McpServerService.name); @@ -232,9 +240,28 @@ export class McpServerService implements OnModuleInit { name, description, parameters: zodParams, + // Gate this entry on a synthetic "role" naming the tool itself, matched + // with 'any'. The transport's `tools/list` handler is SYNCHRONOUS, so it + // cannot ask the database who the caller is — but it does compare + // `user.roles` against this list. The global endpoint therefore resolves + // the caller's visible tools asynchronously BEFORE delegating and plants + // the answer on `req.user.roles`. See `visibleToolRoles` in + // mcp-endpoint.controller.ts. + // + // Keyed on NAME, not tool id: this registry holds one entry per name + // across every organization, so the id belongs to whichever tenant + // registered it first. Name is also what the call handler resolves by + // (`getToolForOrg(name, org)`), so the two stay consistent. + requiredRoles: [toolVisibilityRole(name)], + requiredRolesMatch: 'any', ...(annotations ? { annotations } : {}), handler: async (args: Record, _context: any, request: any) => { - // Check role-based tool access if user is identified + // Role check, kept as the SECOND layer. The transport now refuses a + // disallowed call before the handler runs, because `requiredRoles` + // above gates `tools/call` as well as `tools/list`. This stays so that + // a tool registered without a visibility role — a future code path, a + // merge that drops the option — is still not freely callable. Defence + // in depth, not the only gate. const user = request?.user; if (user?.sub) { // Global /mcp registry: there is no server-scoped org here, so the From 83407a1d82268b1cfc27b3cb97fd25e9d41c5e13 Mon Sep 17 00:00:00 2001 From: Matteo Date: Tue, 8 Sep 2026 20:55:52 +0200 Subject: [PATCH 2/2] fix(mcp): give a readable refusal when a hidden tool is called MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The visibility role gates `tools/call` as well as `tools/list`, so the transport already refused these — but with `requires any of roles: tool:`, which exposes an internal naming scheme and tells an operator nothing about what to do next. Answered in the controller instead, before delegating. The wording stays deliberately ambiguous between "another workspace's tool" and "your role does not grant it": distinguishing them would confirm that a tool of that name exists elsewhere in the deployment, which is the disclosure this path exists to prevent. The transport's own check remains as the backstop, and still handles batched requests. --- .../src/mcp-server/mcp-endpoint.controller.ts | 56 +++++++++++++++++-- 1 file changed, 52 insertions(+), 4 deletions(-) diff --git a/packages/backend/src/mcp-server/mcp-endpoint.controller.ts b/packages/backend/src/mcp-server/mcp-endpoint.controller.ts index 22098dcd..553e1c42 100644 --- a/packages/backend/src/mcp-server/mcp-endpoint.controller.ts +++ b/packages/backend/src/mcp-server/mcp-endpoint.controller.ts @@ -125,7 +125,8 @@ export class McpEndpointController { @Post() async handleGlobalPost(@Req() req: Request, @Res() res: Response) { - await this.attachVisibleTools(req); + const visible = await this.attachVisibleTools(req); + if (this.refuseHiddenToolCall(req, res, visible)) return; await mcpHttpTransport.httpHandlers.handlePost(req, res); } @@ -158,14 +159,14 @@ export class McpEndpointController { * 1. ORGANIZATION — only tools owned by the caller's active org. * 2. ROLE — of those, only the ones the caller's MCP roles allow. */ - private async attachVisibleTools(req: Request) { + private async attachVisibleTools(req: Request): Promise | null> { const user = (req as any).user; // No identified principal: a static MCP_API_KEY / MCP_BEARER_TOKEN or an // explicitly enabled anonymous mode. Both are operator credentials on a // single-tenant self-hosted box, so the pre-existing "everything" answer // is the correct one and narrowing it here would break those deployments. - if (!user?.sub || !user.organizationId) return; + if (!user?.sub || !user.organizationId) return null; const orgTools = this.toolRegistry .getAllTools() @@ -183,13 +184,60 @@ export class McpEndpointController { ? orgTools : orgTools.filter((t) => allowedToolIds.includes(t.id)); + const visibleNames = new Set(visible.map((t) => t.name)); user.roles = [ - ...new Set(visible.map((t) => toolVisibilityRole(t.name))), + ...[...visibleNames].map(toolVisibilityRole), // Tools declared statically in code (the Knowledge Graph helper, the // demo tools) carry no visibility role and stay visible to everyone — // they hold no tenant data. ...(Array.isArray(user.roles) ? user.roles : []), ]; + return visibleNames; + } + + /** + * Answers a call for a tool the caller cannot see, before the transport does. + * + * The transport would refuse it anyway — the same visibility role gates + * `tools/call` — but its message names the synthetic role + * (`requires any of roles: tool:foo`), which exposes an implementation + * detail and tells an operator nothing about what to do next. + * + * The wording is deliberately ambiguous between "another workspace's tool" + * and "your role does not grant it": distinguishing them would confirm to a + * caller that a tool of that name exists somewhere else in the deployment, + * which is the disclosure this whole path exists to prevent. + */ + private refuseHiddenToolCall( + req: Request, + res: Response, + visible: Set | null, + ): boolean { + // `null` means visibility was not narrowed for this caller — an operator + // credential on a single-tenant box. Nothing to refuse. + if (visible === null) return false; + + const body = (req as any).body; + // Only single calls. A batch is handled by the transport, which still + // refuses each hidden tool — with the less friendly message. + if (!body || Array.isArray(body) || body.method !== 'tools/call') return false; + + const name = body?.params?.name; + if (typeof name !== 'string' || visible.has(name)) return false; + + // Static, code-declared tools carry no visibility role and are open to + // everyone, so they must not be refused here. + if (this.toolRegistry.countByName(name) === 0) return false; + + res.status(200).json({ + jsonrpc: '2.0', + id: body.id ?? null, + error: { + code: -32600, + message: `Tool '${name}' is not available to you. It belongs to another workspace, or your MCP role does not grant it.`, + }, + }); + return true; } // ─── Public, anonymous, static demo MCP server ──────────────────────────