From 4358643f309c110cce95495b68239ba74e6961d9 Mon Sep 17 00:00:00 2001 From: Joel Wurtz Date: Thu, 30 Jul 2026 16:44:13 +0200 Subject: [PATCH] fix(mcp): fix no security / validation in mcp tools when using symfony listeners --- .../ApiPlatformExtension.php | 26 ++- .../Bundle/Resources/config/mcp/events.php | 45 ++++- .../Bundle/Resources/config/mcp/security.php | 51 ++++++ .../config/mcp/security_validator.php | 28 ++++ .../Bundle/Resources/config/mcp/state.php | 6 +- .../Bundle/Resources/config/mcp/validator.php | 35 ++++ .../ApiResource/McpSecuredReference.php | 48 ++++++ .../ApiResource/McpSecuredTools.php | 79 +++++++++ tests/Functional/McpSecurityTest.php | 155 ++++++++++++++++++ 9 files changed, 470 insertions(+), 3 deletions(-) create mode 100644 src/Symfony/Bundle/Resources/config/mcp/security.php create mode 100644 src/Symfony/Bundle/Resources/config/mcp/security_validator.php create mode 100644 src/Symfony/Bundle/Resources/config/mcp/validator.php create mode 100644 tests/Fixtures/TestBundle/ApiResource/McpSecuredReference.php create mode 100644 tests/Fixtures/TestBundle/ApiResource/McpSecuredTools.php create mode 100644 tests/Functional/McpSecurityTest.php diff --git a/src/Symfony/Bundle/DependencyInjection/ApiPlatformExtension.php b/src/Symfony/Bundle/DependencyInjection/ApiPlatformExtension.php index 9b9019b368c..914485ba50d 100644 --- a/src/Symfony/Bundle/DependencyInjection/ApiPlatformExtension.php +++ b/src/Symfony/Bundle/DependencyInjection/ApiPlatformExtension.php @@ -217,7 +217,31 @@ public function load(array $configs, ContainerBuilder $container): void // McpToolProvider requires Symfony's object_mapper service; mirror FrameworkBundle's gate so we don't try to wire it when object-mapper is dev-only. if (($config['mcp']['enabled'] ?? false) && class_exists(McpBundle::class) && ContainerBuilder::willBeAvailable('symfony/object-mapper', ObjectMapperInterface::class, ['symfony/framework-bundle'])) { $loader->load('mcp/mcp.php'); - $loader->load($config['use_symfony_listeners'] ? 'mcp/events.php' : 'mcp/state.php'); + + if ($config['use_symfony_listeners']) { + // In this mode the state pipeline is driven by kernel listeners, which never run for + // a JSON-RPC tool call, so MCP needs its own provider chain to keep enforcing + // security, parameters and validation. + $loader->load('mcp/events.php'); + + /** @var string[] $bundles */ + $bundles = $container->getParameter('kernel.bundles'); + $hasValidator = interface_exists(ValidatorInterface::class); + + if ($hasValidator) { + $loader->load('mcp/validator.php'); + } + + if (isset($bundles['SecurityBundle'])) { + $loader->load('mcp/security.php'); + + if ($hasValidator) { + $loader->load('mcp/security_validator.php'); + } + } + } else { + $loader->load('mcp/state.php'); + } } $container->registerForAutoconfiguration(FilterInterface::class) diff --git a/src/Symfony/Bundle/Resources/config/mcp/events.php b/src/Symfony/Bundle/Resources/config/mcp/events.php index 3bb7db5372b..2c5c8489084 100644 --- a/src/Symfony/Bundle/Resources/config/mcp/events.php +++ b/src/Symfony/Bundle/Resources/config/mcp/events.php @@ -16,10 +16,53 @@ use ApiPlatform\Mcp\Server\Handler; use ApiPlatform\Mcp\State\StructuredContentProcessor; use ApiPlatform\State\Processor\WriteProcessor; +use ApiPlatform\State\Provider\ContentNegotiationProvider; +use ApiPlatform\State\Provider\DeserializeProvider; +use ApiPlatform\State\Provider\ParameterProvider; +use ApiPlatform\State\Provider\ReadProvider; return static function (ContainerConfigurator $container) { $services = $container->services(); + // A tool call is dispatched over JSON-RPC from within the handler, never as an HTTP request + // cycle, so the kernel listeners that build the state pipeline in this mode never fire. MCP + // therefore gets its own provider chain, mirroring "api_platform.state_provider.main" so that + // security, parameters and validation behave the same whichever mode is configured. + // See mcp/security.php, mcp/security_validator.php and mcp/validator.php for the decorators. + $services->alias('api_platform.mcp.state_provider', 'api_platform.state_provider.locator'); + + $services->set('api_platform.mcp.state_provider.read', ReadProvider::class) + ->decorate('api_platform.mcp.state_provider', null, 500) + ->args([ + service('api_platform.mcp.state_provider.read.inner'), + service('api_platform.serializer.context_builder'), + ]); + + $services->set('api_platform.mcp.state_provider.deserialize', DeserializeProvider::class) + ->decorate('api_platform.mcp.state_provider', null, 300) + ->args([ + service('api_platform.mcp.state_provider.deserialize.inner'), + service('api_platform.serializer'), + service('api_platform.serializer.context_builder'), + service('translator')->nullOnInvalid(), + ]); + + $services->set('api_platform.mcp.state_provider.parameter', ParameterProvider::class) + ->decorate('api_platform.mcp.state_provider', null, 180) + ->args([ + service('api_platform.mcp.state_provider.parameter.inner'), + tagged_locator('api_platform.parameter_provider', 'key'), + ]); + + $services->set('api_platform.mcp.state_provider.content_negotiation', ContentNegotiationProvider::class) + ->decorate('api_platform.mcp.state_provider', null, 100) + ->args([ + service('api_platform.mcp.state_provider.content_negotiation.inner'), + service('api_platform.negotiator'), + '%api_platform.formats%', + '%api_platform.error_formats%', + ]); + $services->set('api_platform.mcp.state_processor.write', WriteProcessor::class) ->args([ null, @@ -36,7 +79,7 @@ $services->set('api_platform.mcp.handler', Handler::class) ->args([ service('api_platform.mcp.metadata.operation.mcp_factory'), - service('api_platform.state_provider.locator'), + service('api_platform.mcp.state_provider'), service('api_platform.mcp.state_processor'), service('request_stack'), service('logger')->ignoreOnInvalid(), diff --git a/src/Symfony/Bundle/Resources/config/mcp/security.php b/src/Symfony/Bundle/Resources/config/mcp/security.php new file mode 100644 index 00000000000..4a1cd43fb78 --- /dev/null +++ b/src/Symfony/Bundle/Resources/config/mcp/security.php @@ -0,0 +1,51 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace Symfony\Component\DependencyInjection\Loader\Configurator; + +use ApiPlatform\State\Provider\SecurityParameterProvider; +use ApiPlatform\Symfony\Security\State\AccessCheckerProvider; + +return static function (ContainerConfigurator $container) { + $services = $container->services(); + + $services->set('api_platform.mcp.state_provider.access_checker', AccessCheckerProvider::class) + ->decorate('api_platform.mcp.state_provider.read', null, 0) + ->args([ + service('api_platform.mcp.state_provider.access_checker.inner'), + service('api_platform.security.resource_access_checker'), + ]); + + $services->set('api_platform.mcp.state_provider.access_checker.post_deserialize', AccessCheckerProvider::class) + ->decorate('api_platform.mcp.state_provider.deserialize', null, 0) + ->args([ + service('api_platform.mcp.state_provider.access_checker.post_deserialize.inner'), + service('api_platform.security.resource_access_checker'), + 'post_denormalize', + ]); + + $services->set('api_platform.mcp.state_provider.security_parameter', SecurityParameterProvider::class) + ->decorate('api_platform.mcp.state_provider.access_checker', null, 0) + ->args([ + service('api_platform.mcp.state_provider.security_parameter.inner'), + service('api_platform.security.resource_access_checker'), + ]); + + $services->set('api_platform.mcp.state_provider.access_checker.pre_read', AccessCheckerProvider::class) + ->decorate('api_platform.mcp.state_provider.read', null, 10) + ->args([ + service('api_platform.mcp.state_provider.access_checker.pre_read.inner'), + service('api_platform.security.resource_access_checker'), + 'pre_read', + ]); +}; diff --git a/src/Symfony/Bundle/Resources/config/mcp/security_validator.php b/src/Symfony/Bundle/Resources/config/mcp/security_validator.php new file mode 100644 index 00000000000..0007ffda137 --- /dev/null +++ b/src/Symfony/Bundle/Resources/config/mcp/security_validator.php @@ -0,0 +1,28 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace Symfony\Component\DependencyInjection\Loader\Configurator; + +use ApiPlatform\Symfony\Security\State\AccessCheckerProvider; + +return static function (ContainerConfigurator $container) { + $services = $container->services(); + + $services->set('api_platform.mcp.state_provider.access_checker.post_validate', AccessCheckerProvider::class) + ->decorate('api_platform.mcp.state_provider.validate', null, 0) + ->args([ + service('api_platform.mcp.state_provider.access_checker.post_validate.inner'), + service('api_platform.security.resource_access_checker'), + 'post_validate', + ]); +}; diff --git a/src/Symfony/Bundle/Resources/config/mcp/state.php b/src/Symfony/Bundle/Resources/config/mcp/state.php index 85525d4b779..5b4cc526c12 100644 --- a/src/Symfony/Bundle/Resources/config/mcp/state.php +++ b/src/Symfony/Bundle/Resources/config/mcp/state.php @@ -20,6 +20,10 @@ return static function (ContainerConfigurator $container) { $services = $container->services(); + // The main provider chain already carries read, deserialize, validate, parameters and the + // access checkers, so MCP reuses it as-is. + $services->alias('api_platform.mcp.state_provider', 'api_platform.state_provider.main'); + $services->set('api_platform.mcp.state_processor.write', WriteProcessor::class) ->args([ null, @@ -36,7 +40,7 @@ $services->set('api_platform.mcp.handler', Handler::class) ->args([ service('api_platform.mcp.metadata.operation.mcp_factory'), - service('api_platform.state_provider.main'), + service('api_platform.mcp.state_provider'), service('api_platform.mcp.state_processor'), service('request_stack'), service('logger')->ignoreOnInvalid(), diff --git a/src/Symfony/Bundle/Resources/config/mcp/validator.php b/src/Symfony/Bundle/Resources/config/mcp/validator.php new file mode 100644 index 00000000000..01075763eab --- /dev/null +++ b/src/Symfony/Bundle/Resources/config/mcp/validator.php @@ -0,0 +1,35 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace Symfony\Component\DependencyInjection\Loader\Configurator; + +use ApiPlatform\Symfony\Validator\State\ParameterValidatorProvider; +use ApiPlatform\Symfony\Validator\State\ValidateProvider; + +return static function (ContainerConfigurator $container) { + $services = $container->services(); + + $services->set('api_platform.mcp.state_provider.validate', ValidateProvider::class) + ->decorate('api_platform.mcp.state_provider', null, 200) + ->args([ + service('api_platform.mcp.state_provider.validate.inner'), + service('api_platform.validator'), + ]); + + $services->set('api_platform.mcp.state_provider.parameter_validator', ParameterValidatorProvider::class) + ->decorate('api_platform.mcp.state_provider', null, 191) + ->args([ + service('validator'), + service('api_platform.mcp.state_provider.parameter_validator.inner'), + ]); +}; diff --git a/tests/Fixtures/TestBundle/ApiResource/McpSecuredReference.php b/tests/Fixtures/TestBundle/ApiResource/McpSecuredReference.php new file mode 100644 index 00000000000..b028c1aaf7c --- /dev/null +++ b/tests/Fixtures/TestBundle/ApiResource/McpSecuredReference.php @@ -0,0 +1,48 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace ApiPlatform\Tests\Fixtures\TestBundle\ApiResource; + +use ApiPlatform\Metadata\ApiResource; +use ApiPlatform\Metadata\Get; +use ApiPlatform\Metadata\Operation; + +/** + * Link target of the "secured_uri_variable_tool" of {@see McpSecuredTools}: link security first + * reads the related resource, then evaluates the expression carried by the Link. + */ +#[ApiResource( + shortName: 'McpSecuredReference', + operations: [ + new Get( + uriTemplate: '/mcp_secured_references/{reference}', + uriVariables: ['reference'], + provider: [self::class, 'provide'] + ), + ] +)] +class McpSecuredReference +{ + public function __construct( + public ?string $reference = null, + ) { + } + + /** + * @param array $uriVariables + */ + public static function provide(Operation $operation, array $uriVariables = []): self + { + return new self($uriVariables['reference'] ?? null); + } +} diff --git a/tests/Fixtures/TestBundle/ApiResource/McpSecuredTools.php b/tests/Fixtures/TestBundle/ApiResource/McpSecuredTools.php new file mode 100644 index 00000000000..f6d41dbf2c1 --- /dev/null +++ b/tests/Fixtures/TestBundle/ApiResource/McpSecuredTools.php @@ -0,0 +1,79 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace ApiPlatform\Tests\Fixtures\TestBundle\ApiResource; + +use ApiPlatform\Metadata\ApiResource; +use ApiPlatform\Metadata\Link; +use ApiPlatform\Metadata\McpTool; + +#[ApiResource( + shortName: 'McpSecuredTools', + operations: [], + mcp: [ + 'secured_tool' => new McpTool( + security: "is_granted('ROLE_ADMIN')", + processor: [self::class, 'process'] + ), + 'secured_post_denormalize_tool' => new McpTool( + securityPostDenormalize: "is_granted('ROLE_ADMIN')", + processor: [self::class, 'process'] + ), + 'secured_post_validation_tool' => new McpTool( + validate: true, + securityPostValidation: "is_granted('ROLE_ADMIN')", + processor: [self::class, 'process'] + ), + 'secured_uri_variable_tool' => new McpTool( + uriVariables: [ + 'reference' => new Link(fromClass: McpSecuredReference::class, security: "is_granted('ROLE_ADMIN')"), + ], + processor: [self::class, 'process'] + ), + ] +)] +class McpSecuredTools +{ + public function __construct( + private ?string $text = null, + private ?string $reference = null, + ) { + } + + public function getText(): ?string + { + return $this->text; + } + + public function setText(?string $text): void + { + $this->text = $text; + } + + public function getReference(): ?string + { + return $this->reference; + } + + public function setReference(?string $reference): void + { + $this->reference = $reference; + } + + public static function process($data): mixed + { + $data->setText('Secured: '.$data->getText()); + + return $data; + } +} diff --git a/tests/Functional/McpSecurityTest.php b/tests/Functional/McpSecurityTest.php new file mode 100644 index 00000000000..cbeaa26ad58 --- /dev/null +++ b/tests/Functional/McpSecurityTest.php @@ -0,0 +1,155 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace ApiPlatform\Tests\Functional; + +use ApiPlatform\Symfony\Bundle\Test\ApiTestCase; +use ApiPlatform\Tests\Fixtures\TestBundle\ApiResource\McpSecuredReference; +use ApiPlatform\Tests\Fixtures\TestBundle\ApiResource\McpSecuredTools; +use ApiPlatform\Tests\RecreateSchemaTrait; +use ApiPlatform\Tests\SetupClassResourcesTrait; +use PHPUnit\Framework\Attributes\DataProvider; +use Symfony\AI\McpBundle\McpBundle; + +/** + * The security attributes of an McpTool must be enforced whatever the value of + * "use_symfony_listeners": the kernel listeners never run for a JSON-RPC tool call, so the MCP + * handler drives the state pipeline itself and has to carry the access checkers. + */ +final class McpSecurityTest extends ApiTestCase +{ + use RecreateSchemaTrait; + use SetupClassResourcesTrait; + + protected static ?bool $alwaysBootKernel = false; + + private const ADMIN_AUTH = 'Basic YWRtaW46a2l0dGVu'; + + /** + * @return class-string[] + */ + public static function getResources(): array + { + return [McpSecuredTools::class, McpSecuredReference::class]; + } + + /** + * @return iterable}> + */ + public static function securedToolProvider(): iterable + { + yield 'security' => ['secured_tool', ['text' => 'hello', 'reference' => null]]; + yield 'securityPostDenormalize' => ['secured_post_denormalize_tool', ['text' => 'hello', 'reference' => null]]; + yield 'securityPostValidation' => ['secured_post_validation_tool', ['text' => 'hello', 'reference' => null]]; + yield 'uriVariable security' => ['secured_uri_variable_tool', ['text' => 'hello', 'reference' => 'abc']]; + } + + /** + * @param array $arguments + */ + #[DataProvider('securedToolProvider')] + public function testAnonymousCannotCallSecuredTool(string $tool, array $arguments): void + { + $this->skipUnlessMcpIsAvailable(); + + $client = self::createClient(); + $result = $this->callTool($client, $this->initializeMcpSession($client), $tool, $arguments)->toArray(false); + + self::assertArrayNotHasKey('result', $result, \sprintf('Tool "%s" ran for an anonymous caller.', $tool)); + self::assertSame('Access Denied.', $result['error']['message'] ?? null); + } + + /** + * @param array $arguments + */ + #[DataProvider('securedToolProvider')] + public function testAdminCanCallSecuredTool(string $tool, array $arguments): void + { + $this->skipUnlessMcpIsAvailable(); + + $client = self::createClient(); + $sessionId = $this->initializeMcpSession($client); + $result = $this->callTool($client, $sessionId, $tool, $arguments, ['Authorization' => self::ADMIN_AUTH])->toArray(false); + + self::assertArrayNotHasKey('error', $result, 'MCP error: '.json_encode($result['error'] ?? null)); + self::assertStringContainsString('Secured: hello', $result['result']['content'][0]['text'] ?? ''); + } + + private function skipUnlessMcpIsAvailable(): void + { + if (!class_exists(McpBundle::class)) { + $this->markTestSkipped('MCP bundle is not installed'); + } + + if ($this->isMongoDB()) { + $this->markTestSkipped('MCP is not supported with MongoDB'); + } + + try { + if (!class_exists('Http\Discovery\Psr17FactoryDiscovery')) { + $this->markTestSkipped('PSR-17 HTTP factory implementation not available (required for MCP)'); + } + + \Http\Discovery\Psr17FactoryDiscovery::findServerRequestFactory(); + } catch (\Throwable) { + $this->markTestSkipped('PSR-17 HTTP factory implementation not available (required for MCP)'); + } + } + + private function initializeMcpSession($client): string + { + $res = $client->request('POST', '/mcp', [ + 'headers' => [ + 'Accept' => 'application/json, text/event-stream', + 'Content-Type' => 'application/json', + ], + 'json' => [ + 'jsonrpc' => '2.0', + 'id' => 1, + 'method' => 'initialize', + 'params' => [ + 'protocolVersion' => '2024-11-05', + 'clientInfo' => ['name' => 'ApiPlatform Test Suite', 'version' => '1.0'], + 'capabilities' => [], + ], + ], + ]); + self::assertResponseIsSuccessful(); + + return $res->getHeaders()['mcp-session-id'][0]; + } + + /** + * @param array $arguments + * @param array $headers + */ + private function callTool($client, string $sessionId, string $toolName, array $arguments = [], array $headers = []) + { + return $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/call', + 'params' => [ + 'name' => $toolName, + 'arguments' => $arguments, + ], + ], + ]); + } +}