Skip to content

Commit 70bc6ff

Browse files
nicolas-grekasnicolas-grekas
committed
Merge branch '6.4' into 7.0
* 6.4: [WebProfilerBundle][TwigBundle] Add conflicts with 7.0 Check whether secrets are empty and mark them all as sensitive [HttpKernel] Add `ControllerResolver::allowControllers()` to define which callables are legit controllers when the `_check_controller_is_allowed` request attribute is set
2 parents f51b0b8 + e434a54 commit 70bc6ff

File tree

28 files changed

+228
-39
lines changed

28 files changed

+228
-39
lines changed

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

Lines changed: 2 additions & 0 deletions
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/HttpFoundation/UriSigner.php

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,6 @@
1212
namespace Symfony\Component\HttpFoundation;
1313

1414
/**
15-
* Signs URIs.
16-
*
1715
* @author Fabien Potencier <fabien@symfony.com>
1816
*/
1917
class UriSigner
@@ -22,11 +20,14 @@ class UriSigner
2220
private string $parameter;
2321

2422
/**
25-
* @param string $secret A secret
2623
* @param string $parameter Query string parameter to use
2724
*/
2825
public function __construct(#[\SensitiveParameter] string $secret, string $parameter = '_hash')
2926
{
27+
if (!$secret) {
28+
throw new \InvalidArgumentException('A non-empty secret is required.');
29+
}
30+
3031
$this->secret = $secret;
3132
$this->parameter = $parameter;
3233
}

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

Lines changed: 1 addition & 1 deletion
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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ CHANGELOG
3434
* Deprecate `FileLinkFormatter`, use `FileLinkFormatter` from the ErrorHandler component instead
3535
* Add argument `$buildDir` to `WarmableInterface`
3636
* Add argument `$filter` to `Profiler::find()` and `FileProfilerStorage::find()`
37+
* Add `ControllerResolver::allowControllers()` to define which callables are legit controllers when the `_check_controller_is_allowed` request attribute is set
3738

3839
6.3
3940
---

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

Lines changed: 82 additions & 5 deletions
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

Lines changed: 1 addition & 0 deletions
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

Lines changed: 68 additions & 0 deletions
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

Lines changed: 1 addition & 1 deletion
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

‎src/Symfony/Component/Mailer/Bridge/Brevo/Webhook/BrevoRequestParser.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ protected function getRequestMatcher(): RequestMatcherInterface
4141
]);
4242
}
4343

44-
protected function doParse(Request $request, string $secret): ?AbstractMailerEvent
44+
protected function doParse(Request $request, #[\SensitiveParameter] string $secret): ?AbstractMailerEvent
4545
{
4646
$content = $request->toArray();
4747
if (

‎src/Symfony/Component/Mailer/Bridge/Mailgun/Webhook/MailgunRequestParser.php

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
use Symfony\Component\HttpFoundation\RequestMatcher\MethodRequestMatcher;
1818
use Symfony\Component\HttpFoundation\RequestMatcherInterface;
1919
use Symfony\Component\Mailer\Bridge\Mailgun\RemoteEvent\MailgunPayloadConverter;
20+
use Symfony\Component\Mailer\Exception\InvalidArgumentException;
2021
use Symfony\Component\RemoteEvent\Event\Mailer\AbstractMailerEvent;
2122
use Symfony\Component\RemoteEvent\Exception\ParseException;
2223
use Symfony\Component\Webhook\Client\AbstractRequestParser;
@@ -37,8 +38,12 @@ protected function getRequestMatcher(): RequestMatcherInterface
3738
]);
3839
}
3940

40-
protected function doParse(Request $request, string $secret): ?AbstractMailerEvent
41+
protected function doParse(Request $request, #[\SensitiveParameter] string $secret): ?AbstractMailerEvent
4142
{
43+
if (!$secret) {
44+
throw new InvalidArgumentException('A non-empty secret is required.');
45+
}
46+
4247
$content = $request->toArray();
4348
if (
4449
!isset($content['signature']['timestamp'])
@@ -60,7 +65,7 @@ protected function doParse(Request $request, string $secret): ?AbstractMailerEve
6065
}
6166
}
6267

63-
private function validateSignature(array $signature, string $secret): void
68+
private function validateSignature(array $signature, #[\SensitiveParameter] string $secret): void
6469
{
6570
// see https://documentation.mailgun.com/en/latest/user_manual.html#webhooks-1
6671
if (!hash_equals($signature['signature'], hash_hmac('sha256', $signature['timestamp'].$signature['token'], $secret))) {

‎src/Symfony/Component/Mailer/Bridge/Mailjet/Webhook/MailjetRequestParser.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ protected function getRequestMatcher(): RequestMatcherInterface
3737
]);
3838
}
3939

40-
protected function doParse(Request $request, string $secret): ?AbstractMailerEvent
40+
protected function doParse(Request $request, #[\SensitiveParameter] string $secret): ?AbstractMailerEvent
4141
{
4242
try {
4343
return $this->converter->convert($request->toArray());

‎src/Symfony/Component/Mailer/Bridge/Postmark/Webhook/PostmarkRequestParser.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ protected function getRequestMatcher(): RequestMatcherInterface
4141
]);
4242
}
4343

44-
protected function doParse(Request $request, string $secret): ?AbstractMailerEvent
44+
protected function doParse(Request $request, #[\SensitiveParameter] string $secret): ?AbstractMailerEvent
4545
{
4646
$payload = $request->toArray();
4747
if (

‎src/Symfony/Component/Mailer/Bridge/Sendgrid/Webhook/SendgridRequestParser.php

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
use Symfony\Component\HttpFoundation\RequestMatcher\MethodRequestMatcher;
1818
use Symfony\Component\HttpFoundation\RequestMatcherInterface;
1919
use Symfony\Component\Mailer\Bridge\Sendgrid\RemoteEvent\SendgridPayloadConverter;
20+
use Symfony\Component\Mailer\Exception\InvalidArgumentException;
2021
use Symfony\Component\RemoteEvent\Event\Mailer\AbstractMailerEvent;
2122
use Symfony\Component\RemoteEvent\Exception\ParseException;
2223
use Symfony\Component\Webhook\Client\AbstractRequestParser;
@@ -86,12 +87,12 @@ protected function doParse(Request $request, string $secret): ?AbstractMailerEve
8687
*
8788
* @see https://docs.sendgrid.com/for-developers/tracking-events/getting-started-event-webhook-security-features
8889
*/
89-
private function validateSignature(
90-
string $signature,
91-
string $timestamp,
92-
string $payload,
93-
string $secret,
94-
): void {
90+
private function validateSignature(string $signature, string $timestamp, string $payload, #[\SensitiveParameter] string $secret): void
91+
{
92+
if (!$secret) {
93+
throw new InvalidArgumentException('A non-empty secret is required.');
94+
}
95+
9596
$timestampedPayload = $timestamp.$payload;
9697

9798
// Sendgrid provides the verification key as base64-encoded DER data. Openssl wants a PEM format, which is a multiline version of the base64 data.

0 commit comments

Comments
 (0)