Skip to content

Navigation Menu

Sign in

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit f0fcc9f

Browse filesBrowse files
nicolas-grekasnicolas-grekas
committed
feature #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
2 parents 2196b67 + 893aba9 commit f0fcc9f
Copy full SHA for f0fcc9f

File tree

7 files changed

+156
-7
lines changed
Filter options

7 files changed

+156
-7
lines changed

‎src/Symfony/Bundle/FrameworkBundle/Resources/config/web.php

Copy file name to clipboardExpand all lines: src/Symfony/Bundle/FrameworkBundle/Resources/config/web.php
+2
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111

1212
namespace Symfony\Component\DependencyInjection\Loader\Configurator;
1313

14+
use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
1415
use Symfony\Bundle\FrameworkBundle\Controller\ControllerResolver;
1516
use Symfony\Component\HttpKernel\Controller\ArgumentResolver;
1617
use Symfony\Component\HttpKernel\Controller\ArgumentResolver\BackedEnumValueResolver;
@@ -40,6 +41,7 @@
4041
service('service_container'),
4142
service('logger')->ignoreOnInvalid(),
4243
])
44+
->call('allowControllers', [[AbstractController::class]])
4345
->tag('monolog.logger', ['channel' => 'request'])
4446

4547
->set('argument_metadata_factory', ArgumentMetadataFactory::class)

‎src/Symfony/Component/HttpKernel/Attribute/AsController.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/HttpKernel/Attribute/AsController.php
+1-1
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818
* This enables injecting services as method arguments in addition
1919
* to other conventional dependency injection strategies.
2020
*/
21-
#[\Attribute(\Attribute::TARGET_CLASS)]
21+
#[\Attribute(\Attribute::TARGET_CLASS | \Attribute::TARGET_FUNCTION)]
2222
class AsController
2323
{
2424
public function __construct()

‎src/Symfony/Component/HttpKernel/CHANGELOG.md

Copy file name to clipboardExpand all lines: src/Symfony/Component/HttpKernel/CHANGELOG.md
+1
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ CHANGELOG
1818
* Deprecate `FileLinkFormatter`, use `FileLinkFormatter` from the ErrorHandler component instead
1919
* Add argument `$buildDir` to `WarmableInterface`
2020
* Add argument `$filter` to `Profiler::find()` and `FileProfilerStorage::find()`
21+
* Add `ControllerResolver::allowControllers()` to define which callables are legit controllers when the `_check_controller_is_allowed` request attribute is set
2122

2223
6.3
2324
---

‎src/Symfony/Component/HttpKernel/Controller/ControllerResolver.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/HttpKernel/Controller/ControllerResolver.php
+82-5
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,9 @@
1212
namespace Symfony\Component\HttpKernel\Controller;
1313

1414
use Psr\Log\LoggerInterface;
15+
use Symfony\Component\HttpFoundation\Exception\BadRequestException;
1516
use Symfony\Component\HttpFoundation\Request;
17+
use Symfony\Component\HttpKernel\Attribute\AsController;
1618

1719
/**
1820
* This implementation uses the '_controller' request attribute to determine
@@ -24,12 +26,32 @@
2426
class ControllerResolver implements ControllerResolverInterface
2527
{
2628
private ?LoggerInterface $logger;
29+
private array $allowedControllerTypes = [];
30+
private array $allowedControllerAttributes = [AsController::class => AsController::class];
2731

2832
public function __construct(LoggerInterface $logger = null)
2933
{
3034
$this->logger = $logger;
3135
}
3236

37+
/**
38+
* @param array<class-string> $types
39+
* @param array<class-string> $attributes
40+
*/
41+
public function allowControllers(array $types = [], array $attributes = []): void
42+
{
43+
foreach ($types as $type) {
44+
$this->allowedControllerTypes[$type] = $type;
45+
}
46+
47+
foreach ($attributes as $attribute) {
48+
$this->allowedControllerAttributes[$attribute] = $attribute;
49+
}
50+
}
51+
52+
/**
53+
* @throws BadRequestException when the request has attribute "_check_controller_is_allowed" set to true and the controller is not allowed
54+
*/
3355
public function getController(Request $request): callable|false
3456
{
3557
if (!$controller = $request->attributes->get('_controller')) {
@@ -44,7 +66,7 @@ public function getController(Request $request): callable|false
4466
$controller[0] = $this->instantiateController($controller[0]);
4567
} catch (\Error|\LogicException $e) {
4668
if (\is_callable($controller)) {
47-
return $controller;
69+
return $this->checkController($request, $controller);
4870
}
4971

5072
throw $e;
@@ -55,19 +77,19 @@ public function getController(Request $request): callable|false
5577
throw new \InvalidArgumentException(sprintf('The controller for URI "%s" is not callable: ', $request->getPathInfo()).$this->getControllerError($controller));
5678
}
5779

58-
return $controller;
80+
return $this->checkController($request, $controller);
5981
}
6082

6183
if (\is_object($controller)) {
6284
if (!\is_callable($controller)) {
6385
throw new \InvalidArgumentException(sprintf('The controller for URI "%s" is not callable: ', $request->getPathInfo()).$this->getControllerError($controller));
6486
}
6587

66-
return $controller;
88+
return $this->checkController($request, $controller);
6789
}
6890

6991
if (\function_exists($controller)) {
70-
return $controller;
92+
return $this->checkController($request, $controller);
7193
}
7294

7395
try {
@@ -80,7 +102,7 @@ public function getController(Request $request): callable|false
80102
throw new \InvalidArgumentException(sprintf('The controller for URI "%s" is not callable: ', $request->getPathInfo()).$this->getControllerError($callable));
81103
}
82104

83-
return $callable;
105+
return $this->checkController($request, $callable);
84106
}
85107

86108
/**
@@ -199,4 +221,59 @@ private function getClassMethodsWithoutMagicMethods($classOrObject): array
199221

200222
return array_filter($methods, fn (string $method) => 0 !== strncmp($method, '__', 2));
201223
}
224+
225+
private function checkController(Request $request, callable $controller): callable
226+
{
227+
if (!$request->attributes->get('_check_controller_is_allowed', false)) {
228+
return $controller;
229+
}
230+
231+
$r = null;
232+
233+
if (\is_array($controller)) {
234+
[$class, $name] = $controller;
235+
$name = (\is_string($class) ? $class : $class::class).'::'.$name;
236+
} elseif (\is_object($controller) && !$controller instanceof \Closure) {
237+
$class = $controller;
238+
$name = $class::class.'::__invoke';
239+
} else {
240+
$r = new \ReflectionFunction($controller);
241+
$name = $r->name;
242+
243+
if (str_contains($name, '{closure}')) {
244+
$name = $class = \Closure::class;
245+
} elseif ($class = \PHP_VERSION_ID >= 80111 ? $r->getClosureCalledClass() : $r->getClosureScopeClass()) {
246+
$class = $class->name;
247+
$name = $class.'::'.$name;
248+
}
249+
}
250+
251+
if ($class) {
252+
foreach ($this->allowedControllerTypes as $type) {
253+
if (is_a($class, $type, true)) {
254+
return $controller;
255+
}
256+
}
257+
}
258+
259+
$r ??= new \ReflectionClass($class);
260+
261+
foreach ($r->getAttributes() as $attribute) {
262+
if (isset($this->allowedControllerAttributes[$attribute->getName()])) {
263+
return $controller;
264+
}
265+
}
266+
267+
if (str_contains($name, '@anonymous')) {
268+
$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);
269+
}
270+
271+
if (-1 === $request->attributes->get('_check_controller_is_allowed')) {
272+
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);
273+
274+
return $controller;
275+
}
276+
277+
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));
278+
}
202279
}

‎src/Symfony/Component/HttpKernel/EventListener/FragmentListener.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/HttpKernel/EventListener/FragmentListener.php
+1
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@ public function onKernelRequest(RequestEvent $event): void
7070
}
7171

7272
parse_str($request->query->get('_path', ''), $attributes);
73+
$attributes['_check_controller_is_allowed'] = -1; // @deprecated, switch to true in Symfony 7
7374
$request->attributes->add($attributes);
7475
$request->attributes->set('_route_params', array_replace($request->attributes->get('_route_params', []), $attributes));
7576
$request->query->remove('_path');

‎src/Symfony/Component/HttpKernel/Tests/Controller/ControllerResolverTest.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/HttpKernel/Tests/Controller/ControllerResolverTest.php
+68
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,9 @@
1313

1414
use PHPUnit\Framework\TestCase;
1515
use Psr\Log\LoggerInterface;
16+
use Symfony\Component\HttpFoundation\Exception\BadRequestException;
1617
use Symfony\Component\HttpFoundation\Request;
18+
use Symfony\Component\HttpKernel\Attribute\AsController;
1719
use Symfony\Component\HttpKernel\Controller\ControllerResolver;
1820

1921
class ControllerResolverTest extends TestCase
@@ -185,12 +187,77 @@ public static function getUndefinedControllers()
185187
];
186188
}
187189

190+
public function testAllowedControllerTypes()
191+
{
192+
$resolver = $this->createControllerResolver();
193+
194+
$request = Request::create('/');
195+
$controller = new ControllerTest();
196+
$request->attributes->set('_controller', [$controller, 'publicAction']);
197+
$request->attributes->set('_check_controller_is_allowed', true);
198+
199+
try {
200+
$resolver->getController($request);
201+
$this->expectException(BadRequestException::class);
202+
} catch (BadRequestException) {
203+
// expected
204+
}
205+
206+
$resolver->allowControllers(types: [ControllerTest::class]);
207+
208+
$this->assertSame([$controller, 'publicAction'], $resolver->getController($request));
209+
210+
$request->attributes->set('_controller', $action = $controller->publicAction(...));
211+
$this->assertSame($action, $resolver->getController($request));
212+
}
213+
214+
public function testAllowedControllerAttributes()
215+
{
216+
$resolver = $this->createControllerResolver();
217+
218+
$request = Request::create('/');
219+
$controller = some_controller_function(...);
220+
$request->attributes->set('_controller', $controller);
221+
$request->attributes->set('_check_controller_is_allowed', true);
222+
223+
try {
224+
$resolver->getController($request);
225+
$this->expectException(BadRequestException::class);
226+
} catch (BadRequestException) {
227+
// expected
228+
}
229+
230+
$resolver->allowControllers(attributes: [DummyController::class]);
231+
232+
$this->assertSame($controller, $resolver->getController($request));
233+
234+
$controller = some_controller_function::class;
235+
$request->attributes->set('_controller', $controller);
236+
$this->assertSame($controller, $resolver->getController($request));
237+
}
238+
239+
public function testAllowedAsControllerAttribute()
240+
{
241+
$resolver = $this->createControllerResolver();
242+
243+
$request = Request::create('/');
244+
$controller = new InvokableController();
245+
$request->attributes->set('_controller', [$controller, '__invoke']);
246+
$request->attributes->set('_check_controller_is_allowed', true);
247+
248+
$this->assertSame([$controller, '__invoke'], $resolver->getController($request));
249+
250+
$request->attributes->set('_controller', $controller);
251+
$this->assertSame($controller, $resolver->getController($request));
252+
}
253+
188254
protected function createControllerResolver(LoggerInterface $logger = null)
189255
{
190256
return new ControllerResolver($logger);
191257
}
192258
}
193259

260+
#[DummyController]
194261
function some_controller_function($foo, $foobar)
195262
{
196263
}
@@ -223,6 +290,7 @@ public static function staticAction()
223290
}
224291
}
225292

293+
#[AsController]
226294
class InvokableController
227295
{
228296
public function __invoke($foo, $bar = null)

‎src/Symfony/Component/HttpKernel/Tests/EventListener/FragmentListenerTest.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/HttpKernel/Tests/EventListener/FragmentListenerTest.php
+1-1
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ public function testWithSignature()
8383

8484
$listener->onKernelRequest($event);
8585

86-
$this->assertEquals(['foo' => 'bar', '_controller' => 'foo'], $request->attributes->get('_route_params'));
86+
$this->assertEquals(['foo' => 'bar', '_controller' => 'foo', '_check_controller_is_allowed' => -1], $request->attributes->get('_route_params'));
8787
$this->assertFalse($request->query->has('_path'));
8888
}
8989

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.