From 3269432502d269202627f211144cfb22186a8242 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Thu, 16 Jul 2026 20:49:49 +0200 Subject: [PATCH] fix(mcp): match toolWhitelist per function, not per registered tool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ToolRegistryFacade::listTools() intersected the whitelist against tool REGISTRY ids while invokeTool() has always resolved a call against FUNCTION ids (name or dotted mcpId). Consumers store function-level ids — an ADR-035 toolWhitelist, Hermiq's per-agent grants — so the two id spaces disagreed in both directions: - An entry naming a real function ("list_schemas" — exactly what Hermiq's agent tool-catalog offers, and the id space of the 30 built-in descriptors that carry no mcpId) matched NOTHING and silently resolved to zero tools, indistinguishable from a tool-less agent. - An entry naming a tool ("openregister.schema" — one tool, five functions) handed over every function it owns, delete_schema included, defeating a caller's read-only intent. listTools() now applies the whitelist per function, accepting the same forms invokeTool() does plus the owning tool's registry id (which still admits all of that tool's functions, so existing coarse grants are unchanged). Tests: the function-name case fails against the old facade; sibling exclusion, registry-id compatibility, mcpId resolution and the unknown-entry case are locked in. 16/16 green, phpcs + psalm clean. --- lib/Service/Mcp/ToolRegistryFacade.php | 70 +++++++++- .../Service/Mcp/ToolRegistryFacadeTest.php | 132 ++++++++++++++++++ 2 files changed, 200 insertions(+), 2 deletions(-) diff --git a/lib/Service/Mcp/ToolRegistryFacade.php b/lib/Service/Mcp/ToolRegistryFacade.php index e32c4705c..cd6749a79 100644 --- a/lib/Service/Mcp/ToolRegistryFacade.php +++ b/lib/Service/Mcp/ToolRegistryFacade.php @@ -99,7 +99,27 @@ public function __construct( * LLM provider. Mirrors what ToolManagementHandler does internally for * the in-app chat path, as a public stable read. * - * @param array $toolWhitelist Optional whitelist of registry ids + * The whitelist is matched PER FUNCTION, the same dual-form way invokeTool() + * resolves a call: an entry may name a function's LLPhant-safe `name` + * (`list_schemas`), its dotted `mcpId` (`openbuild.upsertSchema`), or the + * registry id of the owning tool (`openregister.schema`, which admits all of + * that tool's functions). + * + * Matching per FUNCTION rather than per TOOL is deliberate and load-bearing. + * A registry id such as `openregister.schema` is ONE tool exposing five + * functions (list/get/create/update/delete_schema), and most built-in + * descriptors carry no `mcpId` at all — so intersecting the whitelist against + * registry ids made the two id spaces disagree in BOTH directions: an entry + * naming a real function (`list_schemas`) matched nothing and silently + * resolved to zero tools, while an entry naming a tool handed over every + * function it owns — `delete_schema` included — defeating a caller's + * read-only intent. Consumers store function-level ids (an ADR-035 + * toolWhitelist, Hermiq's per-agent grants) and invokeTool() has always + * resolved them function-level; this is listTools() keeping the contract the + * rest of the class already documents. + * + * @param array $toolWhitelist Optional whitelist of function names, + * dotted mcpIds and/or registry ids * ({appId}.{toolName}). Empty = all * discovered tools allowed (ADR-035 * Decision 4 default semantics). @@ -112,8 +132,18 @@ public function listTools(array $toolWhitelist=[]): array { $descriptors = []; - foreach ($this->resolveRegisteredTools(toolWhitelist: $toolWhitelist) as $tool) { + foreach ($this->resolveRegisteredTools(toolWhitelist: []) as $registryId => $tool) { foreach ($tool->getFunctions() as $function) { + $allowed = $this->functionIsWhitelisted( + function: $function, + registryId: (string) $registryId, + toolWhitelist: $toolWhitelist + ); + + if ($allowed === false) { + continue; + } + $descriptors[] = $function; } } @@ -121,6 +151,42 @@ public function listTools(array $toolWhitelist=[]): array return $descriptors; }//end listTools() + /** + * Whether one function descriptor is admitted by a whitelist. + * + * An empty whitelist admits everything (ADR-035 Decision 4). Otherwise the + * entry must name the function's `name`, its dotted `mcpId`, or the registry + * id of the tool that owns it. + * + * @param array $function The LLPhant function descriptor. + * @param string $registryId Registry id of the owning tool. + * @param array $toolWhitelist The whitelist to apply. + * + * @return bool True when the descriptor should be listed. + * + * @spec openspec/specs/ai-mcp/spec.md + */ + private function functionIsWhitelisted(array $function, string $registryId, array $toolWhitelist): bool + { + if ($toolWhitelist === []) { + return true; + } + + $candidates = [$registryId]; + + $name = ($function['name'] ?? null); + if (is_string($name) === true && $name !== '') { + $candidates[] = $name; + } + + $mcpId = ($function['mcpId'] ?? null); + if (is_string($mcpId) === true && $mcpId !== '') { + $candidates[] = $mcpId; + } + + return (array_intersect($candidates, $toolWhitelist) !== []); + }//end functionIsWhitelisted() + /** * Invoke a tool function by its descriptor name or dotted mcpId. * diff --git a/tests/Unit/Service/Mcp/ToolRegistryFacadeTest.php b/tests/Unit/Service/Mcp/ToolRegistryFacadeTest.php index 1d02b6d33..b5f992738 100644 --- a/tests/Unit/Service/Mcp/ToolRegistryFacadeTest.php +++ b/tests/Unit/Service/Mcp/ToolRegistryFacadeTest.php @@ -193,6 +193,138 @@ public function testListToolsNarrowsByWhitelist(): void $this->assertSame('decidesk_listMeetings', $result[0]['name']); }//end testListToolsNarrowsByWhitelist() + /** + * A whitelist naming a FUNCTION resolves it — the id space consumers + * actually store. + * + * Regression: the whitelist used to be intersected against registry ids, so + * an entry naming a real function of a multi-function tool (`list_schemas`, + * exactly what the agent tool-catalog offers) matched nothing and silently + * resolved to ZERO tools — indistinguishable from a tool-less agent. + * + * @return void + */ + public function testListToolsResolvesAWhitelistedFunctionName(): void + { + $this->registry->registerTool( + 'openregister.schema', + $this->createToolMock( + [ + ['name' => 'list_schemas', 'description' => 'List', 'parameters' => []], + ['name' => 'delete_schema', 'description' => 'Delete', 'parameters' => []], + ] + ), + $this->metadata('openregister') + ); + + $result = $this->facade->listTools(['list_schemas']); + + $this->assertCount(1, $result); + $this->assertSame('list_schemas', $result[0]['name']); + }//end testListToolsResolvesAWhitelistedFunctionName() + + /** + * A whitelist naming one function does NOT drag in its tool's other + * functions. + * + * Regression (the other direction of the same defect): matching per TOOL + * meant a read-only intent silently handed over every function the tool + * owns, `delete_schema` included. + * + * @return void + */ + public function testListToolsDoesNotAdmitSiblingFunctionsOfAWhitelistedFunction(): void + { + $this->registry->registerTool( + 'openregister.schema', + $this->createToolMock( + [ + ['name' => 'list_schemas', 'description' => 'List', 'parameters' => []], + ['name' => 'delete_schema', 'description' => 'Delete', 'parameters' => []], + ] + ), + $this->metadata('openregister') + ); + + $names = array_column($this->facade->listTools(['list_schemas']), 'name'); + + $this->assertNotContains('delete_schema', $names); + }//end testListToolsDoesNotAdmitSiblingFunctionsOfAWhitelistedFunction() + + /** + * A whitelist naming a tool's REGISTRY id still admits all of its functions + * (the pre-existing coarse-grained grant stays valid). + * + * @return void + */ + public function testListToolsRegistryIdStillAdmitsEveryFunctionOfThatTool(): void + { + $this->registry->registerTool( + 'openregister.schema', + $this->createToolMock( + [ + ['name' => 'list_schemas', 'description' => 'List', 'parameters' => []], + ['name' => 'delete_schema', 'description' => 'Delete', 'parameters' => []], + ] + ), + $this->metadata('openregister') + ); + + $names = array_column($this->facade->listTools(['openregister.schema']), 'name'); + + $this->assertSame(['list_schemas', 'delete_schema'], $names); + }//end testListToolsRegistryIdStillAdmitsEveryFunctionOfThatTool() + + /** + * A whitelist naming a function's dotted `mcpId` resolves it — the third + * accepted form, and the one an ADR-035 toolWhitelist stores for bridged + * tools. + * + * @return void + */ + public function testListToolsResolvesAWhitelistedMcpId(): void + { + $this->registry->registerTool( + 'openbuild.upsertSchema', + $this->createToolMock( + [ + [ + 'name' => 'openbuild_upsertSchema', + 'mcpId' => 'openbuild.upsertSchema', + 'description' => 'Upsert', + 'parameters' => [], + ], + ] + ), + $this->metadata('openbuild') + ); + + $result = $this->facade->listTools(['openbuild.upsertSchema']); + + $this->assertCount(1, $result); + $this->assertSame('openbuild_upsertSchema', $result[0]['name']); + }//end testListToolsResolvesAWhitelistedMcpId() + + /** + * An unknown whitelist entry resolves to nothing — it must NOT fuzzy-match. + * + * `openregister.schemas` (plural) is not a real id; the tool is + * `openregister.schema`. Resolving to zero is correct here; making that zero + * VISIBLE is the consumer's job (Hermiq raises on it). + * + * @return void + */ + public function testListToolsUnknownWhitelistEntryResolvesToNothing(): void + { + $this->registry->registerTool( + 'openregister.schema', + $this->createToolMock([['name' => 'list_schemas', 'description' => 'List', 'parameters' => []]]), + $this->metadata('openregister') + ); + + $this->assertSame([], $this->facade->listTools(['openregister.schemas'])); + }//end testListToolsUnknownWhitelistEntryResolvesToNothing() + /** * An explicit empty whitelist means "all discovered tools allowed" * (hydra ADR-035 Decision 4 default semantics).