feature symfony#52471 [HttpKernel] Add ControllerResolver::allowControllers() to define which callables are legit controllers when the _check_controller_is_allowed request attribute is set (nicolas-grekas)

This PR was merged into the 6.4 branch.

Discussion
----------

[HttpKernel] Add `ControllerResolver::allowControllers()` to define which callables are legit controllers when the `_check_controller_is_allowed` request attribute is set

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Issues        | -
| License       | MIT

Right now, when one doesn't configure properly their APP_SECRET, this can too easily lead to an RCE.

This PR proposes to harden security by rejecting any not-allowed controllers when the `_check_controller_is_allowed` request attribute is set. We leverage this in FragmentListener to close the RCE gap.

In order to allow a controller, one should call `ControllerResolver::allowControllers()` during instantiation to tell which types or attributes should be accepted. #[AsController] is always allowed, and FrameworkBundle also allows instances of `AbstractController`.

Third-party bundles that provide controllers meant to be used as fragments should ensure their controllers are allowed by adding the method call to the `controller_resolver` service definition.

I propose this as a late 6.4 feature so that we can provide this hardening right away in 7.0. In 6.4, this would be only a deprecation.

Commits
-------

893aba9 [HttpKernel] Add `ControllerResolver::allowControllers()` to define which callables are legit controllers when the `_check_controller_is_allowed` request attribute is set

Author: nicolas-grekas

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)

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()

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
 ---

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<class-string> $types
+     * @param array<class-string> $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,19 +77,19 @@ 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)) {
             if (!\is_callable($controller)) {
                 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));
+    }
 }

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');

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)

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'));
     }