Issue #3485174 by fago, arthur_lorenz, alexpott, smustgrave: Menu APIs provide invalid CSRF tokens

(cherry picked from commit ec91b0d0)

core/core.services.yml

@@ -1481,7 +1481,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']

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,21 @@
  */
 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 @@ 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 +52,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.

core/modules/system/src/Routing/MenuLinksetRoutes.php

@@ -103,6 +103,7 @@ public function routes() {
         ],
         [
           '_access' => 'TRUE',
+          '_format' => 'json',
         ],
         [
           'parameters' => [

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