From ae6992293d0e33f8a90077918c6d3c71f2835b5f Mon Sep 17 00:00:00 2001
From: Alex Pott <alex.a.pott@googlemail.com>
Date: Tue, 28 Jan 2025 10:35:07 +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 | 25 ++++----
.../system/src/Routing/MenuLinksetRoutes.php | 1 +
.../Core/Access/RouteProcessorCsrfTest.php | 64 +++++++++++++++++--
4 files changed, 72 insertions(+), 20 deletions(-)
diff --git a/core/core.services.yml b/core/core.services.yml
index 2dff154d821b..478329f97e07 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 6b70d7c63c30..15baa36f4911 100644
--- a/core/lib/Drupal/Core/Access/RouteProcessorCsrf.php
+++ b/core/lib/Drupal/Core/Access/RouteProcessorCsrf.php
@@ -6,6 +6,7 @@
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,22 @@
*/
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) {
+ @trigger_error('Calling ' . __CLASS__ . ' constructor without the $requestStack argument is deprecated in drupal:11.2.0 and it will be required in drupal:12.0.0. See https://www.drupal.org/project/drupal/issues/3485174', E_USER_DEPRECATED);
+ $this->requestStack = \Drupal::service('request_stack');
+ }
}
/**
@@ -42,7 +44,7 @@ public function processOutbound($route_name, Route $route, array &$parameters, ?
}
// 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 +53,6 @@ public function processOutbound($route_name, Route $route, array &$parameters, ?
$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 bbafef4971ad..72d0c2910193 100644
--- a/core/modules/system/src/Routing/MenuLinksetRoutes.php
+++ b/core/modules/system/src/Routing/MenuLinksetRoutes.php
@@ -103,6 +103,7 @@ public function routes() {
],
[
'_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 621d79557c0a..c4d6d6d221c6 100644
--- a/core/tests/Drupal/Tests/Core/Access/RouteProcessorCsrfTest.php
+++ b/core/tests/Drupal/Tests/Core/Access/RouteProcessorCsrfTest.php
@@ -5,9 +5,12 @@
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 @@ protected function setUp(): void {
->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 @@ public function testProcessOutboundDynamicTwo(): void {
$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']);
+ }
+
}
--
GitLab