From ec91b0d0cd0adfbd45803764b51dec6efc3dece7 Mon Sep 17 00:00:00 2001 From: Alex Pott Date: Tue, 28 Jan 2025 10:36:50 +0000 Subject: [PATCH] Issue #3485174 by fago, arthur_lorenz, alexpott, smustgrave: Menu APIs provide invalid CSRF tokens --- core/core.services.yml | 2 +- .../Drupal/Core/Access/RouteProcessorCsrf.php | 24 +++---- .../system/src/Routing/MenuLinksetRoutes.php | 1 + .../Core/Access/RouteProcessorCsrfTest.php | 64 +++++++++++++++++-- 4 files changed, 71 insertions(+), 20 deletions(-) diff --git a/core/core.services.yml b/core/core.services.yml index 5ffa0d96f4d..bc44aa75ccf 100644 --- a/core/core.services.yml +++ b/core/core.services.yml @@ -1457,7 +1457,7 @@ services: class: Drupal\Core\Access\RouteProcessorCsrf tags: - { name: route_processor_outbound } - arguments: ['@csrf_token'] + arguments: ['@csrf_token', '@request_stack'] transliteration: class: Drupal\Core\Transliteration\PhpTransliteration arguments: [null, '@module_handler'] diff --git a/core/lib/Drupal/Core/Access/RouteProcessorCsrf.php b/core/lib/Drupal/Core/Access/RouteProcessorCsrf.php index 6b70d7c63c3..79dea87a612 100644 --- a/core/lib/Drupal/Core/Access/RouteProcessorCsrf.php +++ b/core/lib/Drupal/Core/Access/RouteProcessorCsrf.php @@ -6,6 +6,7 @@ use Drupal\Component\Utility\Crypt; use Drupal\Core\Render\BubbleableMetadata; use Drupal\Core\Security\TrustedCallbackInterface; use Drupal\Core\RouteProcessor\OutboundRouteProcessorInterface; +use Symfony\Component\HttpFoundation\RequestStack; use Symfony\Component\Routing\Route; /** @@ -13,21 +14,21 @@ use Symfony\Component\Routing\Route; */ class RouteProcessorCsrf implements OutboundRouteProcessorInterface, TrustedCallbackInterface { - /** - * The CSRF token generator. - * - * @var \Drupal\Core\Access\CsrfTokenGenerator - */ - protected $csrfToken; - /** * Constructs a RouteProcessorCsrf object. * - * @param \Drupal\Core\Access\CsrfTokenGenerator $csrf_token + * @param \Drupal\Core\Access\CsrfTokenGenerator $csrfToken * The CSRF token generator. + * @param \Symfony\Component\HttpFoundation\RequestStack|null $requestStack + * The request stack. */ - public function __construct(CsrfTokenGenerator $csrf_token) { - $this->csrfToken = $csrf_token; + public function __construct( + protected CsrfTokenGenerator $csrfToken, + protected ?RequestStack $requestStack = NULL, + ) { + if ($requestStack === NULL) { + $this->requestStack = \Drupal::service('request_stack'); + } } /** @@ -42,7 +43,7 @@ class RouteProcessorCsrf implements OutboundRouteProcessorInterface, TrustedCall } // Adding this to the parameters means it will get merged into the query // string when the route is compiled. - if (!$bubbleable_metadata) { + if (!$bubbleable_metadata || $this->requestStack->getCurrentRequest()->getRequestFormat() !== 'html') { $parameters['token'] = $this->csrfToken->get($path); } else { @@ -51,7 +52,6 @@ class RouteProcessorCsrf implements OutboundRouteProcessorInterface, TrustedCall $placeholder_render_array = [ '#lazy_builder' => ['route_processor_csrf:renderPlaceholderCsrfToken', [$path]], ]; - // Instead of setting an actual CSRF token as the query string, we set // the placeholder, which will be replaced at the very last moment. This // ensures links with CSRF tokens don't break cacheability. diff --git a/core/modules/system/src/Routing/MenuLinksetRoutes.php b/core/modules/system/src/Routing/MenuLinksetRoutes.php index bbafef4971a..72d0c291019 100644 --- a/core/modules/system/src/Routing/MenuLinksetRoutes.php +++ b/core/modules/system/src/Routing/MenuLinksetRoutes.php @@ -103,6 +103,7 @@ class MenuLinksetRoutes extends RouteSubscriberBase implements ContainerInjectio ], [ '_access' => 'TRUE', + '_format' => 'json', ], [ 'parameters' => [ diff --git a/core/tests/Drupal/Tests/Core/Access/RouteProcessorCsrfTest.php b/core/tests/Drupal/Tests/Core/Access/RouteProcessorCsrfTest.php index 621d79557c0..c4d6d6d221c 100644 --- a/core/tests/Drupal/Tests/Core/Access/RouteProcessorCsrfTest.php +++ b/core/tests/Drupal/Tests/Core/Access/RouteProcessorCsrfTest.php @@ -5,9 +5,12 @@ declare(strict_types=1); namespace Drupal\Tests\Core\Access; use Drupal\Component\Utility\Crypt; +use Drupal\Core\Access\CsrfTokenGenerator; use Drupal\Core\Render\BubbleableMetadata; use Drupal\Tests\UnitTestCase; use Drupal\Core\Access\RouteProcessorCsrf; +use PHPUnit\Framework\MockObject\MockObject; +use Symfony\Component\HttpFoundation\RequestStack; use Symfony\Component\Routing\Route; /** @@ -18,17 +21,18 @@ class RouteProcessorCsrfTest extends UnitTestCase { /** * The mock CSRF token generator. - * - * @var \Drupal\Core\Access\CsrfTokenGenerator|\PHPUnit\Framework\MockObject\MockObject */ - protected $csrfToken; + protected CsrfTokenGenerator&MockObject $csrfToken; + + /** + * The mock request stack. + */ + protected RequestStack&MockObject $requestStack; /** * The route processor. - * - * @var \Drupal\Core\Access\RouteProcessorCsrf */ - protected $processor; + protected RouteProcessorCsrf $processor; /** * {@inheritdoc} @@ -40,7 +44,20 @@ class RouteProcessorCsrfTest extends UnitTestCase { ->disableOriginalConstructor() ->getMock(); - $this->processor = new RouteProcessorCsrf($this->csrfToken); + $this->requestStack = $this->getMockBuilder('Symfony\Component\HttpFoundation\RequestStack') + ->disableOriginalConstructor() + ->getMock(); + + $request = $this->createMock('Symfony\Component\HttpFoundation\Request'); + $request->expects($this->any()) + ->method('getRequestFormat') + ->willReturn('html'); + + $this->requestStack->expects($this->any()) + ->method('getCurrentRequest') + ->willReturn($request); + + $this->processor = new RouteProcessorCsrf($this->csrfToken, $this->requestStack); } /** @@ -122,4 +139,37 @@ class RouteProcessorCsrfTest extends UnitTestCase { $this->assertEquals((new BubbleableMetadata())->setAttachments(['placeholders' => [$placeholder => $placeholder_render_array]]), $bubbleable_metadata); } + /** + * Tests JSON requests to get no placeholders, but real tokens. + */ + public function testProcessOutboundJsonFormat(): void { + // Create a new request mock that returns 'json' format. + $request = $this->createMock('Symfony\Component\HttpFoundation\Request'); + $request->expects($this->once()) + ->method('getRequestFormat') + ->willReturn('json'); + $this->requestStack = $this->createMock('Symfony\Component\HttpFoundation\RequestStack'); + $this->requestStack->expects($this->once()) + ->method('getCurrentRequest') + ->willReturn($request); + + // Mock that the CSRF token service should be called once with 'test-path' + // and return a test token. + $this->csrfToken->expects($this->any()) + ->method('get') + ->with('test-path') + ->willReturn('real_token_value'); + + $this->processor = new RouteProcessorCsrf($this->csrfToken, $this->requestStack); + + $route = new Route('/test-path', [], ['_csrf_token' => 'TRUE']); + $parameters = []; + // For JSON requests, the actual CSRF token should be in parameters, + // regardless of whether cache metadata is present. + $this->processor->processOutbound('test', $route, $parameters); + $this->assertEquals('real_token_value', $parameters['token']); + $this->processor->processOutbound('test', $route, $parameters, new BubbleableMetadata()); + $this->assertEquals('real_token_value', $parameters['token']); + } + }