From 4e57fb905e92b5bd17defdc812f310c72dc11df7 Mon Sep 17 00:00:00 2001 From: soyuka Date: Sun, 16 Aug 2026 11:11:23 +0200 Subject: [PATCH] refactor(mcp): dedupe security and validator gates PR #8435 re-derived isset($bundles['SecurityBundle']) and interface_exists(ValidatorInterface) inside load() to decide whether to wire mcp/security.php and mcp/validator.php. Both gates already exist in registerSecurityConfiguration() and registerValidatorConfiguration(). They agree today, but changing a canonical gate would silently strip MCP of security again -- the exact bug #8435 fixed. Load the MCP files from inside those two methods instead, guarded by a single $mcpProviderChain flag derived once from the existing MCP-enabled condition. One place now decides "security available", one decides "validator available". --- .../ApiPlatformExtension.php | 63 ++++++++--------- .../ApiPlatformExtensionTest.php | 69 +++++++++++++++++-- 2 files changed, 93 insertions(+), 39 deletions(-) diff --git a/src/Symfony/Bundle/DependencyInjection/ApiPlatformExtension.php b/src/Symfony/Bundle/DependencyInjection/ApiPlatformExtension.php index 914485ba50..05e66fe8b4 100644 --- a/src/Symfony/Bundle/DependencyInjection/ApiPlatformExtension.php +++ b/src/Symfony/Bundle/DependencyInjection/ApiPlatformExtension.php @@ -179,6 +179,11 @@ public function load(array $configs, ContainerBuilder $container): void $patchFormats['jsonapi'] = ['application/vnd.api+json']; } + // 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. + $mcpEnabled = ($config['mcp']['enabled'] ?? false) && class_exists(McpBundle::class) && ContainerBuilder::willBeAvailable('symfony/object-mapper', ObjectMapperInterface::class, ['symfony/framework-bundle']); + // kernel listeners never run for a JSON-RPC tool call, so MCP needs its own provider chain + $mcpProviderChain = $mcpEnabled && $config['use_symfony_listeners']; + $this->registerCommonConfiguration($container, $config, $loader, $formats, $patchFormats, $errorFormats, $docsFormats); $this->registerMetadataConfiguration($container, $config, $loader); $this->registerOAuthConfiguration($container, $config); @@ -193,12 +198,12 @@ public function load(array $configs, ContainerBuilder $container): void $this->registerDoctrineOrmConfiguration($container, $config, $loader); $this->registerDoctrineMongoDbOdmConfiguration($container, $config, $loader); $this->registerHttpCacheConfiguration($container, $config, $loader); - $this->registerValidatorConfiguration($container, $config, $loader); + $this->registerValidatorConfiguration($container, $config, $loader, $mcpProviderChain); $this->registerDataCollectorConfiguration($container, $config, $loader); $this->registerMercureConfiguration($container, $config, $loader); $this->registerMessengerConfiguration($container, $config, $loader); $this->registerElasticsearchConfiguration($container, $config, $loader); - $this->registerSecurityConfiguration($container, $config, $loader); + $this->registerSecurityConfiguration($container, $config, $loader, $mcpProviderChain); $this->registerMakerConfiguration($container, $config, $loader); $this->registerArgumentResolverConfiguration($loader); $this->registerLinkSecurityConfiguration($loader, $config); @@ -214,34 +219,9 @@ public function load(array $configs, ContainerBuilder $container): void $container->setParameter('api_platform.mcp.format', $config['mcp']['format'] ?? null); - // 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'])) { + if ($mcpEnabled) { $loader->load('mcp/mcp.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'); - } + $loader->load($mcpProviderChain ? 'mcp/events.php' : 'mcp/state.php'); } $container->registerForAutoconfiguration(FilterInterface::class) @@ -956,9 +936,9 @@ private function getFormats(array $configFormats): array return $formats; } - private function registerValidatorConfiguration(ContainerBuilder $container, array $config, PhpFileLoader $loader): void + private function registerValidatorConfiguration(ContainerBuilder $container, array $config, PhpFileLoader $loader, bool $mcpProviderChain): void { - if (interface_exists(ValidatorInterface::class)) { + if ($this->isValidatorAvailable()) { $loader->load('metadata/validator.php'); $loader->load('validator/validator.php'); @@ -968,6 +948,10 @@ private function registerValidatorConfiguration(ContainerBuilder $container, arr $loader->load($config['use_symfony_listeners'] ? 'validator/events.php' : 'validator/state.php'); + if ($mcpProviderChain) { + $loader->load('mcp/validator.php'); + } + $container->registerForAutoconfiguration(ValidationGroupsGeneratorInterface::class) ->addTag('api_platform.validation_groups_generator'); $container->registerForAutoconfiguration(PropertySchemaRestrictionMetadataInterface::class) @@ -1067,7 +1051,7 @@ private function registerElasticsearchConfiguration(ContainerBuilder $container, $loader->load('elasticsearch.php'); } - private function registerSecurityConfiguration(ContainerBuilder $container, array $config, PhpFileLoader $loader): void + private function registerSecurityConfiguration(ContainerBuilder $container, array $config, PhpFileLoader $loader, bool $mcpProviderChain): void { /** @var string[] $bundles */ $bundles = $container->getParameter('kernel.bundles'); @@ -1080,8 +1064,16 @@ private function registerSecurityConfiguration(ContainerBuilder $container, arra $loader->load('state/security.php'); - if (interface_exists(ValidatorInterface::class)) { + if ($mcpProviderChain) { + $loader->load('mcp/security.php'); + } + + if ($this->isValidatorAvailable()) { $loader->load('state/security_validator.php'); + + if ($mcpProviderChain) { + $loader->load('mcp/security_validator.php'); + } } if ($this->isConfigEnabled($container, $config['graphql'])) { @@ -1089,6 +1081,11 @@ private function registerSecurityConfiguration(ContainerBuilder $container, arra } } + private function isValidatorAvailable(): bool + { + return interface_exists(ValidatorInterface::class); + } + private function registerOpenApiConfiguration(ContainerBuilder $container, array $config, PhpFileLoader $loader): void { $container->setParameter('api_platform.openapi.termsOfService', $config['openapi']['termsOfService']); diff --git a/src/Symfony/Tests/Bundle/DependencyInjection/ApiPlatformExtensionTest.php b/src/Symfony/Tests/Bundle/DependencyInjection/ApiPlatformExtensionTest.php index 854a06f251..e4ab0febdb 100644 --- a/src/Symfony/Tests/Bundle/DependencyInjection/ApiPlatformExtensionTest.php +++ b/src/Symfony/Tests/Bundle/DependencyInjection/ApiPlatformExtensionTest.php @@ -118,13 +118,21 @@ class ApiPlatformExtensionTest extends TestCase private ContainerBuilder $container; protected function setUp(): void + { + $this->container = $this->createContainer([ + 'DoctrineBundle' => DoctrineBundle::class, + 'SecurityBundle' => SecurityBundle::class, + 'TwigBundle' => TwigBundle::class, + ]); + } + + /** + * @param array $bundles + */ + private function createContainer(array $bundles): ContainerBuilder { $containerParameterBag = new ParameterBag([ - 'kernel.bundles' => [ - 'DoctrineBundle' => DoctrineBundle::class, - 'SecurityBundle' => SecurityBundle::class, - 'TwigBundle' => TwigBundle::class, - ], + 'kernel.bundles' => $bundles, 'kernel.bundles_metadata' => [ 'TestBundle' => [ 'parent' => null, @@ -137,7 +145,7 @@ protected function setUp(): void 'kernel.environment' => 'test', ]); - $this->container = new ContainerBuilder($containerParameterBag); + return new ContainerBuilder($containerParameterBag); } private function assertContainerHas(array $services, array $aliases = []): void @@ -370,6 +378,55 @@ public function testEventListenersConfiguration(): void $this->container->hasParameter('api_platform.swagger.http_auth'); } + public function testMcpProviderChainIsSecuredAndValidatedWithSecurityBundle(): void + { + $config = self::DEFAULT_CONFIG; + $config['api_platform']['use_symfony_listeners'] = true; + (new ApiPlatformExtension())->load($config, $this->container); + + $this->assertContainerHasService('api_platform.mcp.handler'); + + foreach ([ + 'api_platform.mcp.state_provider.access_checker', + 'api_platform.mcp.state_provider.access_checker.pre_read', + 'api_platform.mcp.state_provider.access_checker.post_deserialize', + 'api_platform.mcp.state_provider.access_checker.post_validate', + 'api_platform.mcp.state_provider.security_parameter', + 'api_platform.mcp.state_provider.validate', + 'api_platform.mcp.state_provider.parameter_validator', + ] as $service) { + $this->assertContainerHasService($service); + } + } + + public function testMcpProviderChainIsNotSecuredWithoutSecurityBundle(): void + { + $this->container = $this->createContainer([ + 'DoctrineBundle' => DoctrineBundle::class, + 'TwigBundle' => TwigBundle::class, + ]); + + $config = self::DEFAULT_CONFIG; + $config['api_platform']['use_symfony_listeners'] = true; + (new ApiPlatformExtension())->load($config, $this->container); + + $this->assertContainerHasService('api_platform.mcp.handler'); + $this->assertNotContainerHasService('api_platform.state_provider.access_checker'); + + foreach ([ + 'api_platform.mcp.state_provider.access_checker', + 'api_platform.mcp.state_provider.access_checker.pre_read', + 'api_platform.mcp.state_provider.access_checker.post_deserialize', + 'api_platform.mcp.state_provider.access_checker.post_validate', + 'api_platform.mcp.state_provider.security_parameter', + ] as $service) { + $this->assertNotContainerHasService($service); + } + + $this->assertContainerHasService('api_platform.mcp.state_provider.validate'); + $this->assertContainerHasService('api_platform.mcp.state_provider.parameter_validator'); + } + public function testItRegistersMetadataConfiguration(): void { $config = self::DEFAULT_CONFIG;