From 893aba903216e3288070a6ee4e297aa862b60455 Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Fri, 3 Nov 2023 13:33:17 +0100 Subject: [PATCH] [HttpKernel] Add `ControllerResolver::allowControllers()` to define which callables are legit controllers when the `_check_controller_is_allowed` request attribute is set --- .../FrameworkBundle/Resources/config/web.php | 2 + .../HttpKernel/Attribute/AsController.php | 2 +- src/Symfony/Component/HttpKernel/CHANGELOG.md | 1 + .../Controller/ControllerResolver.php | 87 +++++++++++++++++-- .../EventListener/FragmentListener.php | 1 + .../Controller/ControllerResolverTest.php | 68 +++++++++++++++ .../EventListener/FragmentListenerTest.php | 2 +- 7 files changed, 156 insertions(+), 7 deletions(-) diff --git a/src/Symfony/Bundle/FrameworkBundle/Resources/config/web.php b/src/Symfony/Bundle/FrameworkBundle/Resources/config/web.php index a3a6ef773561..817ed07c18a0 100644 --- a/src/Symfony/Bundle/FrameworkBundle/Resources/config/web.php +++ b/src/Symfony/Bundle/FrameworkBundle/Resources/config/web.php @@ -11,6 +11,7 @@ namespace Symfony\Component\DependencyInjection\Loader\Configurator; +use Symfony\Bundle\FrameworkBundle\Controller\AbstractController; use Symfony\Bundle\FrameworkBundle\Controller\ControllerResolver; use Symfony\Component\HttpKernel\Controller\ArgumentResolver; use Symfony\Component\HttpKernel\Controller\ArgumentResolver\BackedEnumValueResolver; @@ -40,6 +41,7 @@ service('service_container'), service('logger')->ignoreOnInvalid(), ]) + ->call('allowControllers', [[AbstractController::class]]) ->tag('monolog.logger', ['channel' => 'request']) ->set('argument_metadata_factory', ArgumentMetadataFactory::class) diff --git a/src/Symfony/Component/HttpKernel/Attribute/AsController.php b/src/Symfony/Component/HttpKernel/Attribute/AsController.php index 48f8e577fddb..0f2c91d45b5b 100644 --- a/src/Symfony/Component/HttpKernel/Attribute/AsController.php +++ b/src/Symfony/Component/HttpKernel/Attribute/AsController.php @@ -18,7 +18,7 @@ * This enables injecting services as method arguments in addition * to other conventional dependency injection strategies. */ -#[\Attribute(\Attribute::TARGET_CLASS)] +#[\Attribute(\Attribute::TARGET_CLASS | \Attribute::TARGET_FUNCTION)] class AsController { public function __construct() diff --git a/src/Symfony/Component/HttpKernel/CHANGELOG.md b/src/Symfony/Component/HttpKernel/CHANGELOG.md index 65b4a6aadd7e..c1743b1d141b 100644 --- a/src/Symfony/Component/HttpKernel/CHANGELOG.md +++ b/src/Symfony/Component/HttpKernel/CHANGELOG.md @@ -18,6 +18,7 @@ CHANGELOG * Deprecate `FileLinkFormatter`, use `FileLinkFormatter` from the ErrorHandler component instead * Add argument `$buildDir` to `WarmableInterface` * Add argument `$filter` to `Profiler::find()` and `FileProfilerStorage::find()` + * Add `ControllerResolver::allowControllers()` to define which callables are legit controllers when the `_check_controller_is_allowed` request attribute is set 6.3 --- diff --git a/src/Symfony/Component/HttpKernel/Controller/ControllerResolver.php b/src/Symfony/Component/HttpKernel/Controller/ControllerResolver.php index b12ce8d35ffd..d39508949215 100644 --- a/src/Symfony/Component/HttpKernel/Controller/ControllerResolver.php +++ b/src/Symfony/Component/HttpKernel/Controller/ControllerResolver.php @@ -12,7 +12,9 @@ namespace Symfony\Component\HttpKernel\Controller; use Psr\Log\LoggerInterface; +use Symfony\Component\HttpFoundation\Exception\BadRequestException; use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\HttpKernel\Attribute\AsController; /** * This implementation uses the '_controller' request attribute to determine @@ -24,12 +26,32 @@ class ControllerResolver implements ControllerResolverInterface { private ?LoggerInterface $logger; + private array $allowedControllerTypes = []; + private array $allowedControllerAttributes = [AsController::class => AsController::class]; public function __construct(LoggerInterface $logger = null) { $this->logger = $logger; } + /** + * @param array $types + * @param array $attributes + */ + public function allowControllers(array $types = [], array $attributes = []): void + { + foreach ($types as $type) { + $this->allowedControllerTypes[$type] = $type; + } + + foreach ($attributes as $attribute) { + $this->allowedControllerAttributes[$attribute] = $attribute; + } + } + + /** + * @throws BadRequestException when the request has attribute "_check_controller_is_allowed" set to true and the controller is not allowed + */ public function getController(Request $request): callable|false { if (!$controller = $request->attributes->get('_controller')) { @@ -44,7 +66,7 @@ public function getController(Request $request): callable|false $controller[0] = $this->instantiateController($controller[0]); } catch (\Error|\LogicException $e) { if (\is_callable($controller)) { - return $controller; + return $this->checkController($request, $controller); } throw $e; @@ -55,7 +77,7 @@ public function getController(Request $request): callable|false throw new \InvalidArgumentException(sprintf('The controller for URI "%s" is not callable: ', $request->getPathInfo()).$this->getControllerError($controller)); } - return $controller; + return $this->checkController($request, $controller); } if (\is_object($controller)) { @@ -63,11 +85,11 @@ public function getController(Request $request): callable|false throw new \InvalidArgumentException(sprintf('The controller for URI "%s" is not callable: ', $request->getPathInfo()).$this->getControllerError($controller)); } - return $controller; + return $this->checkController($request, $controller); } if (\function_exists($controller)) { - return $controller; + return $this->checkController($request, $controller); } try { @@ -80,7 +102,7 @@ public function getController(Request $request): callable|false throw new \InvalidArgumentException(sprintf('The controller for URI "%s" is not callable: ', $request->getPathInfo()).$this->getControllerError($callable)); } - return $callable; + return $this->checkController($request, $callable); } /** @@ -199,4 +221,59 @@ private function getClassMethodsWithoutMagicMethods($classOrObject): array return array_filter($methods, fn (string $method) => 0 !== strncmp($method, '__', 2)); } + + private function checkController(Request $request, callable $controller): callable + { + if (!$request->attributes->get('_check_controller_is_allowed', false)) { + return $controller; + } + + $r = null; + + if (\is_array($controller)) { + [$class, $name] = $controller; + $name = (\is_string($class) ? $class : $class::class).'::'.$name; + } elseif (\is_object($controller) && !$controller instanceof \Closure) { + $class = $controller; + $name = $class::class.'::__invoke'; + } else { + $r = new \ReflectionFunction($controller); + $name = $r->name; + + if (str_contains($name, '{closure}')) { + $name = $class = \Closure::class; + } elseif ($class = \PHP_VERSION_ID >= 80111 ? $r->getClosureCalledClass() : $r->getClosureScopeClass()) { + $class = $class->name; + $name = $class.'::'.$name; + } + } + + if ($class) { + foreach ($this->allowedControllerTypes as $type) { + if (is_a($class, $type, true)) { + return $controller; + } + } + } + + $r ??= new \ReflectionClass($class); + + foreach ($r->getAttributes() as $attribute) { + if (isset($this->allowedControllerAttributes[$attribute->getName()])) { + return $controller; + } + } + + if (str_contains($name, '@anonymous')) { + $name = preg_replace_callback('/[a-zA-Z_\x7f-\xff][\\\\a-zA-Z0-9_\x7f-\xff]*+@anonymous\x00.*?\.php(?:0x?|:[0-9]++\$)[0-9a-fA-F]++/', fn ($m) => class_exists($m[0], false) ? (get_parent_class($m[0]) ?: key(class_implements($m[0])) ?: 'class').'@anonymous' : $m[0], $name); + } + + if (-1 === $request->attributes->get('_check_controller_is_allowed')) { + trigger_deprecation('symfony/http-kernel', '6.4', 'Callable "%s()" is not allowed as a controller. Did you miss tagging it with "#[AsController]" or registering its type with "%s::allowControllers()"?', $name, self::class); + + return $controller; + } + + throw new BadRequestException(sprintf('Callable "%s()" is not allowed as a controller. Did you miss tagging it with "#[AsController]" or registering its type with "%s::allowControllers()"?', $name, self::class)); + } } diff --git a/src/Symfony/Component/HttpKernel/EventListener/FragmentListener.php b/src/Symfony/Component/HttpKernel/EventListener/FragmentListener.php index f267ba581714..562244b338b5 100644 --- a/src/Symfony/Component/HttpKernel/EventListener/FragmentListener.php +++ b/src/Symfony/Component/HttpKernel/EventListener/FragmentListener.php @@ -70,6 +70,7 @@ public function onKernelRequest(RequestEvent $event): void } parse_str($request->query->get('_path', ''), $attributes); + $attributes['_check_controller_is_allowed'] = -1; // @deprecated, switch to true in Symfony 7 $request->attributes->add($attributes); $request->attributes->set('_route_params', array_replace($request->attributes->get('_route_params', []), $attributes)); $request->query->remove('_path'); diff --git a/src/Symfony/Component/HttpKernel/Tests/Controller/ControllerResolverTest.php b/src/Symfony/Component/HttpKernel/Tests/Controller/ControllerResolverTest.php index 222d806931ff..25ff02f380a5 100644 --- a/src/Symfony/Component/HttpKernel/Tests/Controller/ControllerResolverTest.php +++ b/src/Symfony/Component/HttpKernel/Tests/Controller/ControllerResolverTest.php @@ -13,7 +13,9 @@ use PHPUnit\Framework\TestCase; use Psr\Log\LoggerInterface; +use Symfony\Component\HttpFoundation\Exception\BadRequestException; use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\HttpKernel\Attribute\AsController; use Symfony\Component\HttpKernel\Controller\ControllerResolver; class ControllerResolverTest extends TestCase @@ -185,12 +187,77 @@ public static function getUndefinedControllers() ]; } + public function testAllowedControllerTypes() + { + $resolver = $this->createControllerResolver(); + + $request = Request::create('/'); + $controller = new ControllerTest(); + $request->attributes->set('_controller', [$controller, 'publicAction']); + $request->attributes->set('_check_controller_is_allowed', true); + + try { + $resolver->getController($request); + $this->expectException(BadRequestException::class); + } catch (BadRequestException) { + // expected + } + + $resolver->allowControllers(types: [ControllerTest::class]); + + $this->assertSame([$controller, 'publicAction'], $resolver->getController($request)); + + $request->attributes->set('_controller', $action = $controller->publicAction(...)); + $this->assertSame($action, $resolver->getController($request)); + } + + public function testAllowedControllerAttributes() + { + $resolver = $this->createControllerResolver(); + + $request = Request::create('/'); + $controller = some_controller_function(...); + $request->attributes->set('_controller', $controller); + $request->attributes->set('_check_controller_is_allowed', true); + + try { + $resolver->getController($request); + $this->expectException(BadRequestException::class); + } catch (BadRequestException) { + // expected + } + + $resolver->allowControllers(attributes: [DummyController::class]); + + $this->assertSame($controller, $resolver->getController($request)); + + $controller = some_controller_function::class; + $request->attributes->set('_controller', $controller); + $this->assertSame($controller, $resolver->getController($request)); + } + + public function testAllowedAsControllerAttribute() + { + $resolver = $this->createControllerResolver(); + + $request = Request::create('/'); + $controller = new InvokableController(); + $request->attributes->set('_controller', [$controller, '__invoke']); + $request->attributes->set('_check_controller_is_allowed', true); + + $this->assertSame([$controller, '__invoke'], $resolver->getController($request)); + + $request->attributes->set('_controller', $controller); + $this->assertSame($controller, $resolver->getController($request)); + } + protected function createControllerResolver(LoggerInterface $logger = null) { return new ControllerResolver($logger); } } +#[DummyController] function some_controller_function($foo, $foobar) { } @@ -223,6 +290,7 @@ public static function staticAction() } } +#[AsController] class InvokableController { public function __invoke($foo, $bar = null) diff --git a/src/Symfony/Component/HttpKernel/Tests/EventListener/FragmentListenerTest.php b/src/Symfony/Component/HttpKernel/Tests/EventListener/FragmentListenerTest.php index 185267ba527f..55d222cbe428 100644 --- a/src/Symfony/Component/HttpKernel/Tests/EventListener/FragmentListenerTest.php +++ b/src/Symfony/Component/HttpKernel/Tests/EventListener/FragmentListenerTest.php @@ -83,7 +83,7 @@ public function testWithSignature() $listener->onKernelRequest($event); - $this->assertEquals(['foo' => 'bar', '_controller' => 'foo'], $request->attributes->get('_route_params')); + $this->assertEquals(['foo' => 'bar', '_controller' => 'foo', '_check_controller_is_allowed' => -1], $request->attributes->get('_route_params')); $this->assertFalse($request->query->has('_path')); }