From 2fcadecbf9fc165c1cf398c25c36b49ef0b657ad Mon Sep 17 00:00:00 2001 From: Pascal CESCON - Amoifr Date: Fri, 28 Aug 2026 14:22:17 +0200 Subject: [PATCH] fix(mcp): evaluate security when listing tools and resources --- src/Mcp/Server/ListHandler.php | 64 +++++++++- src/Mcp/Tests/Server/ListHandlerTest.php | 120 ++++++++++++++++++ .../Bundle/Resources/config/mcp/mcp.php | 3 + tests/Functional/McpSecurityTest.php | 54 ++++++++ 4 files changed, 239 insertions(+), 2 deletions(-) diff --git a/src/Mcp/Server/ListHandler.php b/src/Mcp/Server/ListHandler.php index 3ac710dc8d..eaf48b2138 100644 --- a/src/Mcp/Server/ListHandler.php +++ b/src/Mcp/Server/ListHandler.php @@ -13,16 +13,22 @@ namespace ApiPlatform\Mcp\Server; +use ApiPlatform\Metadata\Operation\Factory\OperationMetadataFactoryInterface; +use ApiPlatform\Metadata\ResourceAccessCheckerInterface; use Mcp\Capability\Registry\Loader\LoaderInterface; use Mcp\Capability\RegistryInterface; use Mcp\Schema\JsonRpc\Request; use Mcp\Schema\JsonRpc\Response; use Mcp\Schema\Request\ListResourcesRequest; use Mcp\Schema\Request\ListToolsRequest; +use Mcp\Schema\ResourceDefinition; use Mcp\Schema\Result\ListResourcesResult; use Mcp\Schema\Result\ListToolsResult; +use Mcp\Schema\Tool; use Mcp\Server\Handler\Request\RequestHandlerInterface; use Mcp\Server\Session\SessionInterface; +use Symfony\Component\ExpressionLanguage\SyntaxError; +use Symfony\Component\HttpFoundation\RequestStack; /** * Serves tools/list and resources/list from the MCP registry, loading API Platform elements @@ -37,6 +43,10 @@ * * Tagged mcp.request_handler, it takes precedence over the SDK's registry-backed list handlers. * + * Elements whose operation-level "security" expression denies the current caller are omitted from + * the listings, so a caller cannot discover the name, description and input schema of a tool it is + * not allowed to invoke. + * * @experimental * TODO: remove once php-sdk:^0.7 has https://github.com/modelcontextprotocol/php-sdk/pull/389/changes * @@ -50,6 +60,9 @@ public function __construct( private readonly RegistryInterface $registry, private readonly LoaderInterface $loader, private readonly int $pageSize = 20, + private readonly ?OperationMetadataFactoryInterface $operationMetadataFactory = null, + private readonly ?ResourceAccessCheckerInterface $resourceAccessChecker = null, + private readonly ?RequestStack $requestStack = null, ) { } @@ -70,13 +83,60 @@ public function handle(Request $request, SessionInterface $session): Response if ($request instanceof ListResourcesRequest) { $page = $this->registry->getResources($this->pageSize, $request->cursor); - $result = new ListResourcesResult($page->references, $page->nextCursor); + $references = $this->filterGranted($page->references, static fn (ResourceDefinition $resource): string => $resource->uri); + $result = new ListResourcesResult($references, $page->nextCursor); } else { \assert($request instanceof ListToolsRequest); $page = $this->registry->getTools($this->pageSize, $request->cursor); - $result = new ListToolsResult($page->references, $page->nextCursor); + $references = $this->filterGranted($page->references, static fn (Tool $tool): string => $tool->name); + $result = new ListToolsResult($references, $page->nextCursor); } return new Response($request->getId(), $result); } + + /** + * Filtering happens after paging, so a page may hold fewer elements than the page size. The + * cursor still walks the whole registry, so no element is skipped. + * + * @template T of Tool|ResourceDefinition + * + * @param list $references + * @param callable(T): string $identify returns the operation name the reference maps to + * + * @return list + */ + private function filterGranted(array $references, callable $identify): array + { + if (null === $this->operationMetadataFactory || null === $this->resourceAccessChecker) { + return $references; + } + + return array_values(array_filter($references, fn (Tool|ResourceDefinition $reference): bool => $this->isGranted($identify($reference)))); + } + + /** + * Only the operation-level "security" expression can be evaluated here: securityPostDenormalize + * and securityPostValidation need arguments and an object that do not exist yet. + */ + private function isGranted(string $operationName): bool + { + \assert(null !== $this->operationMetadataFactory && null !== $this->resourceAccessChecker); + + $operation = $this->operationMetadataFactory->create($operationName); + + if (null === $operation || null === ($security = $operation->getSecurity())) { + return true; + } + + try { + return $this->resourceAccessChecker->isGranted($operation->getClass() ?? '', $security, ['request' => $this->requestStack?->getCurrentRequest()]); + } catch (SyntaxError) { + // The expression reads variables that only exist once the element is called (object, + // previous_object, uri variables). Listing cannot decide, so the element stays visible + // and the expression is enforced on tools/call and resources/read, as + // AccessCheckerProvider already defers the pre_read stage in that case. + return true; + } + } } diff --git a/src/Mcp/Tests/Server/ListHandlerTest.php b/src/Mcp/Tests/Server/ListHandlerTest.php index a8bfd4bdf4..9aadcd7a5c 100644 --- a/src/Mcp/Tests/Server/ListHandlerTest.php +++ b/src/Mcp/Tests/Server/ListHandlerTest.php @@ -20,10 +20,12 @@ use ApiPlatform\Metadata\ApiResource; use ApiPlatform\Metadata\McpResource; use ApiPlatform\Metadata\McpTool; +use ApiPlatform\Metadata\Operation\Factory\OperationMetadataFactoryInterface; use ApiPlatform\Metadata\Resource\Factory\ResourceMetadataCollectionFactoryInterface; use ApiPlatform\Metadata\Resource\Factory\ResourceNameCollectionFactoryInterface; use ApiPlatform\Metadata\Resource\ResourceMetadataCollection; use ApiPlatform\Metadata\Resource\ResourceNameCollection; +use ApiPlatform\Metadata\ResourceAccessCheckerInterface; use Mcp\Capability\Registry; use Mcp\Capability\Registry\Loader\LoaderInterface; use Mcp\Capability\RegistryInterface; @@ -34,6 +36,7 @@ use Mcp\Schema\Tool; use Mcp\Server\Session\SessionInterface; use PHPUnit\Framework\TestCase; +use Symfony\Component\ExpressionLanguage\SyntaxError; class ListHandlerTest extends TestCase { @@ -127,6 +130,73 @@ public function testSupportsListRequests(): void $this->assertTrue($handler->supports(new ListResourcesRequest())); } + public function testListToolsOmitsToolsTheCallerCannotInvoke(): void + { + $secured = new McpTool(name: 'secured', description: 'Secured', structuredContent: false, class: \stdClass::class, security: "is_granted('ROLE_ADMIN')"); + $public = new McpTool(name: 'public', description: 'Public', structuredContent: false, class: \stdClass::class); + + $accessChecker = $this->createMock(ResourceAccessCheckerInterface::class); + $accessChecker->method('isGranted')->willReturn(false); + + $result = $this->handleListTools([$secured, $public], $accessChecker); + + $this->assertInstanceOf(ListToolsResult::class, $result); + $this->assertSame(['public'], array_map(static fn (Tool $t): string => $t->name, $result->tools)); + } + + public function testListToolsKeepsToolsTheCallerCanInvoke(): void + { + $secured = new McpTool(name: 'secured', description: 'Secured', structuredContent: false, class: \stdClass::class, security: "is_granted('ROLE_ADMIN')"); + + $accessChecker = $this->createMock(ResourceAccessCheckerInterface::class); + $accessChecker->expects($this->once())->method('isGranted')->with(\stdClass::class, "is_granted('ROLE_ADMIN')")->willReturn(true); + + $result = $this->handleListTools([$secured], $accessChecker); + + $this->assertInstanceOf(ListToolsResult::class, $result); + $this->assertSame(['secured'], array_map(static fn (Tool $t): string => $t->name, $result->tools)); + } + + /** + * An expression reading the object (or a uri variable) cannot be evaluated before the tool is + * called: the tool stays listed and tools/call still enforces the expression. + */ + public function testListToolsKeepsToolsWhoseExpressionNeedsCallTimeVariables(): void + { + $secured = new McpTool(name: 'secured', description: 'Secured', structuredContent: false, class: \stdClass::class, security: 'object.owner == user'); + + $accessChecker = $this->createMock(ResourceAccessCheckerInterface::class); + $accessChecker->method('isGranted')->willThrowException(new SyntaxError('Variable "object" is not valid')); + + $result = $this->handleListTools([$secured], $accessChecker); + + $this->assertInstanceOf(ListToolsResult::class, $result); + $this->assertSame(['secured'], array_map(static fn (Tool $t): string => $t->name, $result->tools)); + } + + public function testListResourcesOmitsResourcesTheCallerCannotRead(): void + { + $secured = new McpResource(uri: 'dummy://secured', name: 'secured', description: 'Secured', mimeType: 'text/plain', class: \stdClass::class, security: "is_granted('ROLE_ADMIN')"); + $public = new McpResource(uri: 'dummy://public', name: 'public', description: 'Public', mimeType: 'text/plain', class: \stdClass::class); + + $accessChecker = $this->createMock(ResourceAccessCheckerInterface::class); + $accessChecker->method('isGranted')->willReturn(false); + + $apiResource = (new ApiResource(class: \stdClass::class))->withMcp(['secured' => $secured, 'public' => $public]); + $handler = new ListHandler( + new Registry(), + $this->createLoader($apiResource, $this->createMock(SchemaFactoryInterface::class)), + 20, + $this->createOperationMetadataFactory([$secured, $public]), + $accessChecker, + ); + + $result = $handler->handle((new ListResourcesRequest())->withId(1), $this->createMock(SessionInterface::class))->result; + + $this->assertInstanceOf(ListResourcesResult::class, $result); + $this->assertSame(['dummy://public'], array_map(static fn ($r): string => $r->uri, $result->resources)); + } + private function createLoader(ApiResource $resource, SchemaFactoryInterface $schemaFactory): Loader { $nameCollectionFactory = $this->createMock(ResourceNameCollectionFactoryInterface::class); @@ -137,4 +207,54 @@ private function createLoader(ApiResource $resource, SchemaFactoryInterface $sch return new Loader($nameCollectionFactory, $metadataCollectionFactory, $schemaFactory); } + + /** + * @param list $tools + */ + private function handleListTools(array $tools, ResourceAccessCheckerInterface $accessChecker): mixed + { + $inputSchema = new Schema(Schema::VERSION_JSON_SCHEMA); + unset($inputSchema['$schema']); + $inputSchema['type'] = 'object'; + $inputSchema['properties'] = []; + + $schemaFactory = $this->createMock(SchemaFactoryInterface::class); + $schemaFactory->method('buildSchema')->willReturn($inputSchema); + + $mcp = []; + foreach ($tools as $tool) { + $mcp[$tool->getName()] = $tool; + } + + $resource = (new ApiResource(class: \stdClass::class))->withMcp($mcp); + + $handler = new ListHandler( + new Registry(), + $this->createLoader($resource, $schemaFactory), + 20, + $this->createOperationMetadataFactory($tools), + $accessChecker, + ); + + return $handler->handle((new ListToolsRequest())->withId(1), $this->createMock(SessionInterface::class))->result; + } + + /** + * @param list $operations + */ + private function createOperationMetadataFactory(array $operations): OperationMetadataFactoryInterface + { + $factory = $this->createMock(OperationMetadataFactoryInterface::class); + $factory->method('create')->willReturnCallback(static function (string $name) use ($operations) { + foreach ($operations as $operation) { + if ($operation->getName() === $name || ($operation instanceof McpResource && $operation->getUri() === $name)) { + return $operation; + } + } + + return null; + }); + + return $factory; + } } diff --git a/src/Symfony/Bundle/Resources/config/mcp/mcp.php b/src/Symfony/Bundle/Resources/config/mcp/mcp.php index 4740dde1f3..8e0e2426f4 100644 --- a/src/Symfony/Bundle/Resources/config/mcp/mcp.php +++ b/src/Symfony/Bundle/Resources/config/mcp/mcp.php @@ -46,6 +46,9 @@ service('mcp.registry'), service('api_platform.mcp.loader'), ]) + ->arg('$operationMetadataFactory', service('api_platform.mcp.metadata.operation.mcp_factory')) + ->arg('$resourceAccessChecker', service('api_platform.security.resource_access_checker')->ignoreOnInvalid()) + ->arg('$requestStack', service('request_stack')) ->tag('mcp.request_handler'); $services->set('api_platform.mcp.iri_converter', IriConverter::class) diff --git a/tests/Functional/McpSecurityTest.php b/tests/Functional/McpSecurityTest.php index cbeaa26ad5..8316a869f8 100644 --- a/tests/Functional/McpSecurityTest.php +++ b/tests/Functional/McpSecurityTest.php @@ -85,6 +85,34 @@ public function testAdminCanCallSecuredTool(string $tool, array $arguments): voi self::assertStringContainsString('Secured: hello', $result['result']['content'][0]['text'] ?? ''); } + public function testAnonymousCannotDiscoverToolsItCannotCall(): void + { + $this->skipUnlessMcpIsAvailable(); + + $client = self::createClient(); + $names = $this->listToolNames($client, $this->initializeMcpSession($client)); + + self::assertNotContains('secured_tool', $names, 'An anonymous caller discovered a tool it cannot invoke.'); + + // Only the operation level "security" can be evaluated before the tool runs: the other + // expressions need arguments, an object or uri variables, so those tools stay listed and + // are enforced on tools/call. + self::assertContains('secured_post_denormalize_tool', $names); + self::assertContains('secured_post_validation_tool', $names); + self::assertContains('secured_uri_variable_tool', $names); + } + + public function testAdminDiscoversSecuredTool(): void + { + $this->skipUnlessMcpIsAvailable(); + + $client = self::createClient(); + $sessionId = $this->initializeMcpSession($client); + $names = $this->listToolNames($client, $sessionId, ['Authorization' => self::ADMIN_AUTH]); + + self::assertContains('secured_tool', $names); + } + private function skipUnlessMcpIsAvailable(): void { if (!class_exists(McpBundle::class)) { @@ -152,4 +180,30 @@ private function callTool($client, string $sessionId, string $toolName, array $a ], ]); } + + /** + * @param array $headers + * + * @return list + */ + private function listToolNames($client, string $sessionId, array $headers = []): array + { + $result = $client->request('POST', '/mcp', [ + 'headers' => $headers + [ + 'Accept' => 'application/json, text/event-stream', + 'Content-Type' => 'application/json', + 'mcp-session-id' => $sessionId, + ], + 'json' => [ + 'jsonrpc' => '2.0', + 'id' => 2, + 'method' => 'tools/list', + 'params' => [], + ], + ])->toArray(false); + + self::assertArrayNotHasKey('error', $result, 'MCP error: '.json_encode($result['error'] ?? null)); + + return array_column($result['result']['tools'] ?? [], 'name'); + } }