Issue #3085360 by bradjones1, josephdpurcell, Giuseppe87, ravi.shankar,...

Issue #3085360 by bradjones1, josephdpurcell, Giuseppe87, ravi.shankar, rajandro, ridhimaabrol24, bbrala, andregp, jhedstrom: RouteProvider::getRouteCollectionForRequest() can poison query string of next request

core/lib/Drupal/Core/Routing/RouteProvider.php

@@ -453,6 +453,8 @@ protected function getRouteCollectionCacheId(Request $request) {
     // based on the domain.
     $this->addExtraCacheKeyPart('language', $this->getCurrentLanguageCacheIdPart());
 
+    $this->addExtraCacheKeyPart('query_parameters', $this->getQueryParametersCacheIdPart($request));
+
     // Sort the cache key parts by their provider in order to have predictable
     // cache keys.
     ksort($this->extraCacheKeyParts);
@@ -461,7 +463,52 @@ protected function getRouteCollectionCacheId(Request $request) {
       $key_parts[] = '[' . $provider . ']=' . $key_part;
     }
 
-    return 'route:' . implode(':', $key_parts) . ':' . $request->getPathInfo() . ':' . $request->getQueryString();
+    return 'route:' . implode(':', $key_parts) . ':' . $request->getPathInfo();
+  }
+
+  /**
+   * Returns the query parameters identifier for the route collection cache.
+   *
+   * The query parameters on the request may be altered programmatically, e.g.
+   * while serving private files or in subrequests. As such, we must vary on
+   * both the query string from the client and the parameter bag after incoming
+   * route processors have modified the request object.
+   *
+   * @param \Symfony\Component\HttpFoundation\Request $request
+   *   Request.
+   *
+   * @return string
+   */
+  protected function getQueryParametersCacheIdPart(Request $request) {
+    // @todo Use \Symfony\Component\HttpFoundation\Request::normalizeQueryString
+    //   for recursive key ordering if support is added in the future.
+    $recursive_sort = function (&$array) use (&$recursive_sort) {
+      foreach ($array as &$v) {
+        if (is_array($v)) {
+          $recursive_sort($v);
+        }
+      }
+      ksort($array);
+    };
+    // Recursively normalize the query parameters to ensure maximal cache hits.
+    // If we did not normalize the order, functionally identical query string
+    // sets could be sent in differing order creating a potential DoS vector
+    // and decreasing cache hit rates.
+    $sorted_resolved_parameters = $request->query->all();
+    $recursive_sort($sorted_resolved_parameters);
+    $sorted_original_parameters = Request::create('/?' . $request->getQueryString())->query->all();
+    $recursive_sort($sorted_original_parameters);
+    // Hash this portion to help shorten the total key length.
+    $resolved_hash = $sorted_resolved_parameters
+      ? sha1(http_build_query($sorted_resolved_parameters))
+      : NULL;
+    return implode(
+      ',',
+      array_filter([
+        http_build_query($sorted_original_parameters),
+        $resolved_hash,
+      ])
+    );
   }
 
   /**

core/modules/system/tests/modules/router_test_directory/router_test.module

+<?php
+
+/**
+ * @file
+ * Test module.
+ */
+
+use Drupal\Core\Url;
+
+/**
+ * Implements hook_preprocess_HOOK().
+ *
+ * Performs an operation that calls the RouteProvider's collection method
+ * during an exception page view. (which is rendered during a subrequest.)
+ *
+ * @see \Drupal\FunctionalTests\Routing\RouteCachingQueryAlteredTest
+ */
+function router_test_preprocess_page(&$variables) {
+  $request = \Drupal::request();
+  if ($request->getPathInfo() === '/router-test/rejects-query-strings') {
+    // Create a URL from the request, e.g. for a breadcrumb or other contextual
+    // information.
+    Url::createFromRequest($request);
+  }
+}

core/modules/system/tests/modules/router_test_directory/router_test.routing.yml

@@ -247,3 +247,10 @@ router_test.case_sensitive_duplicate3:
     _controller: '\Drupal\router_test\TestControllers::testRouteName'
   requirements:
     _access: 'TRUE'
+
+router_test.rejects_query_strings:
+  path: '/router-test/rejects-query-strings'
+  defaults:
+    _controller: '\Drupal\router_test\TestControllers::rejectsQueryStrings'
+  requirements:
+    _access: 'TRUE'

core/modules/system/tests/modules/router_test_directory/src/RouterTestEarlyExceptionSubscriber.php

+<?php
+
+declare(strict_types=1);
+
+namespace Drupal\router_test;
+
+use Symfony\Component\EventDispatcher\EventSubscriberInterface;
+use Symfony\Component\HttpFoundation\Response;
+use Symfony\Component\HttpKernel\Event\RequestEvent;
+use Symfony\Component\HttpKernel\Exception\HttpException;
+use Symfony\Component\HttpKernel\KernelEvents;
+
+/**
+ * Event subscribers for exceptions thrown in early kernel middleware.
+ */
+class RouterTestEarlyExceptionSubscriber implements EventSubscriberInterface {
+
+  /**
+   * Throw an exception, which will trigger exception-handling subscribers.
+   *
+   * See DefaultExceptionHtmlSubscriber.
+   */
+  public function onKernelRequest(RequestEvent $event): void {
+    if ($event->isMainRequest() && $event->getRequest()->headers->get('Authorization') === 'Bearer invalid') {
+      throw new HttpException(
+        Response::HTTP_UNAUTHORIZED,
+        'This is a common exception during authentication.'
+      );
+    }
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public static function getSubscribedEvents(): array {
+    // This is the same priority as AuthenticationSubscriber, however
+    // exceptions are not restricted to authentication; this is a common,
+    // early point to emulate an exception, e.g. when an OAuth token is
+    // rejected.
+    $events[KernelEvents::REQUEST][] = ['onKernelRequest', 300];
+    return $events;
+  }
+
+}

core/modules/system/tests/modules/router_test_directory/src/RouterTestServiceProvider.php

@@ -14,9 +14,12 @@ class RouterTestServiceProvider implements ServiceProviderInterface {
    * {@inheritdoc}
    */
   public function register(ContainerBuilder $container) {
-    $container->register('router_test.subscriber', 'Drupal\router_test\RouteTestSubscriber')->addTag('event_subscriber');
+    $container->register('router_test.subscriber', 'Drupal\router_test\RouteTestSubscriber')
+      ->addTag('event_subscriber');
     $container->register('access_check.router_test', 'Drupal\router_test\Access\TestAccessCheck')
       ->addTag('access_check', ['applies_to' => '_access_router_test']);
+    $container->register('router_test.early_exception.subscriber', 'Drupal\router_test\RouterTestEarlyExceptionSubscriber')
+      ->addTag('event_subscriber');
   }
 
 }

core/modules/system/tests/modules/router_test_directory/src/TestControllers.php

@@ -118,6 +118,19 @@ public function testRouteName(Request $request) {
     ];
   }
 
+  /**
+   * Rejects requests with query keys.
+   *
+   * @param \Symfony\Component\HttpFoundation\Request $request
+   *   The given request.
+   *
+   * @return \Symfony\Component\HttpFoundation\Response
+   *   The response.
+   */
+  public function rejectsQueryStrings(Request $request) {
+    return new Response('', $request->query->keys() ? Response::HTTP_BAD_REQUEST : Response::HTTP_OK);
+  }
+
   /**
    * Throws an exception.
    *

core/tests/Drupal/FunctionalTests/Routing/RouteCachingQueryAlteredTest.php

+<?php
+
+declare(strict_types=1);
+
+namespace Drupal\FunctionalTests\Routing;
+
+use Drupal\Tests\BrowserTestBase;
+use Symfony\Component\HttpFoundation\Response;
+
+/**
+ * Tests the route cache when the request's query parameters are altered.
+ *
+ * This happens either in the normal course of operations or due to an
+ * exception.
+ *
+ * @group routing
+ */
+class RouteCachingQueryAlteredTest extends BrowserTestBase {
+
+  /**
+   * {@inheritdoc}
+   */
+  protected static $modules = ['router_test'];
+
+  /**
+   * {@inheritdoc}
+   */
+  protected $defaultTheme = 'stark';
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function setUp(): void {
+    parent::setUp();
+    // page_cache module is enabled in the testing profile, however by default
+    // exceptions which create 4xx responses are cached for 1 hour. This is
+    // undesirable for certain response types (e.g., 401) which vary on other
+    // elements of the request than the URL. For this reason, do not cache 4xx
+    // responses for the purposes of this test.
+    $settings['settings']['cache_ttl_4xx'] = (object) [
+      'value' => 0,
+      'required' => TRUE,
+    ];
+    $this->writeSettings($settings);
+  }
+
+  /**
+   * Tests route collection cache after an exception.
+   */
+  public function testRouteCollectionCacheAfterException() {
+    // Force an exception early in the Kernel middleware on a cold cache by
+    // simulating bad Bearer authentication.
+    $this->drupalGet('/router-test/rejects-query-strings', [], [
+      'Authorization' => 'Bearer invalid',
+    ]);
+    $this->assertSession()->statusCodeEquals(Response::HTTP_UNAUTHORIZED);
+    // Check that the route collection cache does not recover any unexpected
+    // query strings from the earlier request that involved an exception.
+    // The requested controller returns 400 if there are any query parameters
+    // present, similar to JSON:API paths that strictly filter requests.
+    $this->drupalGet('/router-test/rejects-query-strings', [], [
+      'Authorization' => 'Bearer valid',
+    ]);
+    $this->assertSession()->statusCodeEquals(Response::HTTP_OK);
+  }
+
+}

core/tests/Drupal/KernelTests/Core/Routing/RouteProviderTest.php

@@ -584,7 +584,7 @@ public function testRouteCaching() {
     $request = Request::create($path, 'GET');
     $provider->getRouteCollectionForRequest($request);
 
-    $cache = $this->cache->get('route:[language]=en:/path/add/one:');
+    $cache = $this->cache->get('route:[language]=en:[query_parameters]=:/path/add/one');
     $this->assertEquals('/path/add/one', $cache->data['path']);
     $this->assertEquals([], $cache->data['query']);
     $this->assertCount(3, $cache->data['routes']);
@@ -594,17 +594,33 @@ public function testRouteCaching() {
     $request = Request::create($path, 'GET');
     $provider->getRouteCollectionForRequest($request);
 
-    $cache = $this->cache->get('route:[language]=en:/path/add/one:foo=bar');
+    $cache = $this->cache->get('route:[language]=en:[query_parameters]=foo=bar,2fb8f40115dd1e695cbe23d4f97ce5b1fb697eee:/path/add/one');
     $this->assertEquals('/path/add/one', $cache->data['path']);
     $this->assertEquals(['foo' => 'bar'], $cache->data['query']);
     $this->assertCount(3, $cache->data['routes']);
 
+    // A path with multivalued query parameters.
+    $path = '/path/add/one?foo=bar&foo2[]=bar2&foo2[]=bar3';
+    $request = Request::create($path, 'GET');
+    $provider->getRouteCollectionForRequest($request);
+
+    $cache = $this->cache->get('route:[language]=en:[query_parameters]=foo=bar&foo2%5B0%5D=bar2&foo2%5B1%5D=bar3,2d3a0851c4970a16be1c851a0e9946e6d10a3fe2:/path/add/one');
+    $this->assertEquals('/path/add/one', $cache->data['path']);
+    $this->assertEquals(
+      [
+        'foo' => 'bar',
+        'foo2' => ['bar2', 'bar3'],
+      ],
+      $cache->data['query']
+    );
+    $this->assertCount(3, $cache->data['routes']);
+
     // A path with placeholders.
     $path = '/path/1/one';
     $request = Request::create($path, 'GET');
     $provider->getRouteCollectionForRequest($request);
 
-    $cache = $this->cache->get('route:[language]=en:/path/1/one:');
+    $cache = $this->cache->get('route:[language]=en:[query_parameters]=:/path/1/one');
     $this->assertEquals('/path/1/one', $cache->data['path']);
     $this->assertEquals([], $cache->data['query']);
     $this->assertCount(2, $cache->data['routes']);
@@ -619,7 +635,7 @@ public function testRouteCaching() {
     $request = Request::create($path, 'GET');
     $provider->getRouteCollectionForRequest($request);
 
-    $cache = $this->cache->get('route:[language]=en:/path/add-one:');
+    $cache = $this->cache->get('route:[language]=en:[query_parameters]=:/path/add-one');
     $this->assertEquals('/path/add/one', $cache->data['path']);
     $this->assertEquals([], $cache->data['query']);
     $this->assertCount(3, $cache->data['routes']);
@@ -634,7 +650,7 @@ public function testRouteCaching() {
     $request = Request::create($path, 'GET');
     $provider->getRouteCollectionForRequest($request);
 
-    $cache = $this->cache->get('route:[language]=gsw-berne:/path/add-one:');
+    $cache = $this->cache->get('route:[language]=gsw-berne:[query_parameters]=:/path/add-one');
     $this->assertEquals('/path/add/one', $cache->data['path']);
     $this->assertEquals([], $cache->data['query']);
     $this->assertCount(3, $cache->data['routes']);