From 964d7a3d2018057b97dea168943abd3e59408ca2 Mon Sep 17 00:00:00 2001 From: Christopher Hertel Date: Fri, 7 Nov 2025 19:25:06 +0100 Subject: [PATCH] Add tool name validation for SEP-986 --- phpstan.dist.neon | 4 ++ src/Capability/Registry.php | 16 ++++-- src/Capability/Tool/NameValidator.php | 20 +++++++ tests/Inspector/InspectorSnapshotTestCase.php | 1 - .../Unit/Capability/Registry/RegistryTest.php | 8 +-- .../Capability/Tool/NameValidatorTest.php | 56 +++++++++++++++++++ 6 files changed, 96 insertions(+), 9 deletions(-) create mode 100644 src/Capability/Tool/NameValidator.php create mode 100644 tests/Unit/Capability/Tool/NameValidatorTest.php diff --git a/phpstan.dist.neon b/phpstan.dist.neon index 17f4c3b0..900f12fd 100644 --- a/phpstan.dist.neon +++ b/phpstan.dist.neon @@ -15,6 +15,10 @@ parameters: ignoreErrors: - message: "#^Method .*::test.*\\(\\) has no return type specified\\.$#" + - + identifier: missingType.iterableValue + path: tests/ + # These errors should actually be fixed, but are ignored for now - identifier: missingType.iterableValue diff --git a/src/Capability/Registry.php b/src/Capability/Registry.php index 3a13c323..381af919 100644 --- a/src/Capability/Registry.php +++ b/src/Capability/Registry.php @@ -18,6 +18,7 @@ use Mcp\Capability\Registry\ResourceReference; use Mcp\Capability\Registry\ResourceTemplateReference; use Mcp\Capability\Registry\ToolReference; +use Mcp\Capability\Tool\NameValidator; use Mcp\Event\PromptListChangedEvent; use Mcp\Event\ResourceListChangedEvent; use Mcp\Event\ResourceTemplateListChangedEvent; @@ -71,6 +72,7 @@ final class Registry implements ReferenceProviderInterface, ReferenceRegistryInt public function __construct( private readonly ?EventDispatcherInterface $eventDispatcher = null, private readonly LoggerInterface $logger = new NullLogger(), + private readonly NameValidator $nameValidator = new NameValidator(), ) { } @@ -100,12 +102,18 @@ public function registerTool(Tool $tool, callable|array|string $handler, bool $i if ($existing && !$isManual && $existing->isManual) { $this->logger->debug( - "Ignoring discovered tool '{$toolName}' as it conflicts with a manually registered one.", + \sprintf('Ignoring discovered tool "%s" as it conflicts with a manually registered one.', $toolName), ); return; } + if (!$this->nameValidator->isValid($toolName)) { + $this->logger->warning( + \sprintf('Tool name "%s" is invalid. Tool names should only contain letters (a-z, A-Z), numbers, dots, hyphens, underscores, and forward slashes.', $toolName), + ); + } + $this->tools[$toolName] = new ToolReference($tool, $handler, $isManual); $this->eventDispatcher?->dispatch(new ToolListChangedEvent()); @@ -118,7 +126,7 @@ public function registerResource(Resource $resource, callable|array|string $hand if ($existing && !$isManual && $existing->isManual) { $this->logger->debug( - "Ignoring discovered resource '{$uri}' as it conflicts with a manually registered one.", + \sprintf('Ignoring discovered resource "%s" as it conflicts with a manually registered one.', $uri), ); return; @@ -140,7 +148,7 @@ public function registerResourceTemplate( if ($existing && !$isManual && $existing->isManual) { $this->logger->debug( - "Ignoring discovered template '{$uriTemplate}' as it conflicts with a manually registered one.", + \sprintf('Ignoring discovered template "%s" as it conflicts with a manually registered one.', $uriTemplate), ); return; @@ -167,7 +175,7 @@ public function registerPrompt( if ($existing && !$isManual && $existing->isManual) { $this->logger->debug( - "Ignoring discovered prompt '{$promptName}' as it conflicts with a manually registered one.", + \sprintf('Ignoring discovered prompt "%s" as it conflicts with a manually registered one.', $promptName), ); return; diff --git a/src/Capability/Tool/NameValidator.php b/src/Capability/Tool/NameValidator.php new file mode 100644 index 00000000..fe9bb084 --- /dev/null +++ b/src/Capability/Tool/NameValidator.php @@ -0,0 +1,20 @@ +> */ public static function provideMethods(): array { return [ diff --git a/tests/Unit/Capability/Registry/RegistryTest.php b/tests/Unit/Capability/Registry/RegistryTest.php index 33cb967e..4f3c4ddd 100644 --- a/tests/Unit/Capability/Registry/RegistryTest.php +++ b/tests/Unit/Capability/Registry/RegistryTest.php @@ -140,7 +140,7 @@ public function testRegisterToolIgnoresDiscoveredWhenManualExists(): void $this->logger ->expects($this->once()) ->method('debug') - ->with("Ignoring discovered tool 'test_tool' as it conflicts with a manually registered one."); + ->with('Ignoring discovered tool "test_tool" as it conflicts with a manually registered one.'); $this->registry->registerTool($discoveredTool, fn () => 'discovered', false); @@ -181,7 +181,7 @@ public function testRegisterResourceIgnoresDiscoveredWhenManualExists(): void $this->logger ->expects($this->once()) ->method('debug') - ->with("Ignoring discovered resource 'test://resource' as it conflicts with a manually registered one."); + ->with('Ignoring discovered resource "test://resource" as it conflicts with a manually registered one.'); $this->registry->registerResource($discoveredResource, fn () => 'discovered', false); @@ -210,7 +210,7 @@ public function testRegisterResourceTemplateIgnoresDiscoveredWhenManualExists(): $this->logger ->expects($this->once()) ->method('debug') - ->with("Ignoring discovered template 'test://{id}' as it conflicts with a manually registered one."); + ->with('Ignoring discovered template "test://{id}" as it conflicts with a manually registered one.'); $this->registry->registerResourceTemplate($discoveredTemplate, fn () => 'discovered', [], false); @@ -239,7 +239,7 @@ public function testRegisterPromptIgnoresDiscoveredWhenManualExists(): void $this->logger ->expects($this->once()) ->method('debug') - ->with("Ignoring discovered prompt 'test_prompt' as it conflicts with a manually registered one."); + ->with('Ignoring discovered prompt "test_prompt" as it conflicts with a manually registered one.'); $this->registry->registerPrompt($discoveredPrompt, fn () => 'discovered', [], false); diff --git a/tests/Unit/Capability/Tool/NameValidatorTest.php b/tests/Unit/Capability/Tool/NameValidatorTest.php new file mode 100644 index 00000000..269be890 --- /dev/null +++ b/tests/Unit/Capability/Tool/NameValidatorTest.php @@ -0,0 +1,56 @@ +assertTrue((new NameValidator())->isValid($name)); + } + + public static function provideValidNames(): array + { + return [ + ['my_tool'], + ['MyTool123'], + ['my.tool'], + ['my-tool'], + ['my/tool'], + ['my_tool-01.02'], + ['my_long_toolname_that_is_exactly_sixty_four_characters_long_1234'], + ]; + } + + #[DataProvider('provideInvalidNames')] + public function testInvalidNames(string $name): void + { + $this->assertFalse((new NameValidator())->isValid($name)); + } + + public static function provideInvalidNames(): array + { + return [ + [''], + ['my tool'], + ['my@tool'], + ['my!tool'], + ['my_tool#1'], + ['this_tool_name_is_way_too_long_because_it_exceeds_the_sixty_four_character_limit_set_by_the_validator'], + ]; + } +}