From a3f9909962b5284a10e159bef9b97cd26103b43a Mon Sep 17 00:00:00 2001
From: Alex Pott <alex.a.pott@googlemail.com>
Date: Wed, 10 Jul 2024 11:22:54 +0100
Subject: [PATCH] Issue #3454346 by kristiaanvandeneynde, bbrala, mxr576:
 JsonApiRequestValidator does not set cacheable metadata when the filter
 allows the request

---
 .../JsonApiRequestValidator.php               | 40 ++++++++++++++++---
 .../tests/src/Functional/BlockTest.php        |  2 +-
 .../EntityTestComputedFieldTest.php           |  2 +-
 .../tests/src/Functional/EntryPointTest.php   |  1 +
 .../tests/src/Functional/FileUploadTest.php   |  2 +-
 .../jsonapi/tests/src/Functional/NodeTest.php |  2 +-
 .../Functional/ResourceResponseTestTrait.php  |  5 +--
 .../tests/src/Functional/ResourceTestBase.php | 33 ++++++---------
 .../src/Functional/RestJsonApiUnsupported.php |  2 +-
 .../jsonapi/tests/src/Functional/UserTest.php |  2 +-
 ...ollectionFilterAccessTestPatternsTrait.php |  3 +-
 11 files changed, 56 insertions(+), 38 deletions(-)

diff --git a/core/modules/jsonapi/src/EventSubscriber/JsonApiRequestValidator.php b/core/modules/jsonapi/src/EventSubscriber/JsonApiRequestValidator.php
index 43b16a105529..44723dd7f7f6 100644
--- a/core/modules/jsonapi/src/EventSubscriber/JsonApiRequestValidator.php
+++ b/core/modules/jsonapi/src/EventSubscriber/JsonApiRequestValidator.php
@@ -3,15 +3,17 @@
 namespace Drupal\jsonapi\EventSubscriber;
 
 use Drupal\Core\Cache\CacheableMetadata;
+use Drupal\Core\Cache\CacheableResponseInterface;
 use Drupal\jsonapi\JsonApiSpec;
 use Symfony\Component\EventDispatcher\EventSubscriberInterface;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpKernel\Event\RequestEvent;
 use Drupal\Core\Http\Exception\CacheableBadRequestHttpException;
+use Symfony\Component\HttpKernel\Event\ResponseEvent;
 use Symfony\Component\HttpKernel\KernelEvents;
 
 /**
- * Request subscriber that validates a JSON:API request.
+ * Subscriber that validates the query parameter names on a JSON:API request.
  *
  * @internal JSON:API maintains no PHP API. The API is the HTTP API. This class
  *   may change at any time and could break any dependencies on it.
@@ -36,6 +38,28 @@ public function onRequest(RequestEvent $event) {
     $this->validateQueryParams($request);
   }
 
+  /**
+   * Validates JSON:API requests.
+   *
+   * @param \Symfony\Component\HttpKernel\Event\ResponseEvent $event
+   *   The event to process.
+   */
+  public function onResponse(ResponseEvent $event) {
+    $request = $event->getRequest();
+    if ($request->getRequestFormat() !== 'api_json') {
+      return;
+    }
+
+    // At this point, we've already run validation on the request by checking
+    // the query arguments. This means that if the query arguments change, we
+    // may need to run this validation again. Therefore, all responses need to
+    // vary by url.query_args.
+    $response = $event->getResponse();
+    if ($response instanceof CacheableResponseInterface) {
+      $response->addCacheableDependency((new CacheableMetadata())->addCacheContexts(['url.query_args']));
+    }
+  }
+
   /**
    * Validates custom (implementation-specific) query parameter names.
    *
@@ -60,20 +84,20 @@ protected function validateQueryParams(Request $request) {
       }
     }
 
+    if (empty($invalid_query_params)) {
+      return NULL;
+    }
+
     // Drupal uses the `_format` query parameter for Content-Type negotiation.
     // Using it violates the JSON:API spec. Nudge people nicely in the correct
     // direction. (This is special cased because using it is pretty common.)
     if (in_array('_format', $invalid_query_params, TRUE)) {
       $uri_without_query_string = $request->getSchemeAndHttpHost() . $request->getBaseUrl() . $request->getPathInfo();
-      $exception = new CacheableBadRequestHttpException((new CacheableMetadata())->addCacheContexts(['url.query_args:_format']), 'JSON:API does not need that ugly \'_format\' query string! 🤘 Use the URL provided in \'links\' 🙏');
+      $exception = new CacheableBadRequestHttpException((new CacheableMetadata())->addCacheContexts(['url.query_args']), 'JSON:API does not need that ugly \'_format\' query string! 🤘 Use the URL provided in \'links\' 🙏');
       $exception->setHeaders(['Link' => $uri_without_query_string]);
       throw $exception;
     }
 
-    if (empty($invalid_query_params)) {
-      return NULL;
-    }
-
     $message = sprintf('The following query parameters violate the JSON:API spec: \'%s\'.', implode("', '", $invalid_query_params));
     $exception = new CacheableBadRequestHttpException((new CacheableMetadata())->addCacheContexts(['url.query_args']), $message);
     $exception->setHeaders(['Link' => 'http://jsonapi.org/format/#query-parameters']);
@@ -85,6 +109,10 @@ protected function validateQueryParams(Request $request) {
    */
   public static function getSubscribedEvents(): array {
     $events[KernelEvents::REQUEST][] = ['onRequest'];
+
+    // Run before the resource response subscriber (priority 128), so that said
+    // subscriber gets the cacheable metadata from this one.
+    $events[KernelEvents::RESPONSE][] = ['onResponse', 129];
     return $events;
   }
 
diff --git a/core/modules/jsonapi/tests/src/Functional/BlockTest.php b/core/modules/jsonapi/tests/src/Functional/BlockTest.php
index 0adc2dadcdbc..5efb77ef42e6 100644
--- a/core/modules/jsonapi/tests/src/Functional/BlockTest.php
+++ b/core/modules/jsonapi/tests/src/Functional/BlockTest.php
@@ -184,7 +184,7 @@ protected function getExpectedUnauthorizedAccessCacheability() {
         'http_response',
         'user:2',
       ])
-      ->setCacheContexts(['url.site', 'user.roles']);
+      ->setCacheContexts(['url.query_args', 'url.site', 'user.roles']);
   }
 
   /**
diff --git a/core/modules/jsonapi/tests/src/Functional/EntityTestComputedFieldTest.php b/core/modules/jsonapi/tests/src/Functional/EntityTestComputedFieldTest.php
index 534eb3b83efe..23e6de945168 100644
--- a/core/modules/jsonapi/tests/src/Functional/EntityTestComputedFieldTest.php
+++ b/core/modules/jsonapi/tests/src/Functional/EntityTestComputedFieldTest.php
@@ -173,7 +173,7 @@ protected function getSparseFieldSets() {
   protected function getExpectedCacheContexts(?array $sparse_fieldset = NULL) {
     $cache_contexts = parent::getExpectedCacheContexts($sparse_fieldset);
     if ($sparse_fieldset === NULL || in_array('computed_test_cacheable_string_field', $sparse_fieldset)) {
-      $cache_contexts = Cache::mergeContexts($cache_contexts, ['url.query_args:computed_test_cacheable_string_field']);
+      $cache_contexts = Cache::mergeContexts($cache_contexts, ['url.query_args']);
     }
 
     return $cache_contexts;
diff --git a/core/modules/jsonapi/tests/src/Functional/EntryPointTest.php b/core/modules/jsonapi/tests/src/Functional/EntryPointTest.php
index d6fc9884175a..8e6aa6d09b2f 100644
--- a/core/modules/jsonapi/tests/src/Functional/EntryPointTest.php
+++ b/core/modules/jsonapi/tests/src/Functional/EntryPointTest.php
@@ -46,6 +46,7 @@ public function testEntryPoint(): void {
     $response = $this->request('GET', Url::fromUri('base://jsonapi'), $request_options);
     $document = $this->getDocumentFromResponse($response);
     $expected_cache_contexts = [
+      'url.query_args',
       'url.site',
       'user.roles:authenticated',
     ];
diff --git a/core/modules/jsonapi/tests/src/Functional/FileUploadTest.php b/core/modules/jsonapi/tests/src/Functional/FileUploadTest.php
index 354904df3898..16e2de4a2fa4 100644
--- a/core/modules/jsonapi/tests/src/Functional/FileUploadTest.php
+++ b/core/modules/jsonapi/tests/src/Functional/FileUploadTest.php
@@ -319,7 +319,7 @@ public function testPostFileUploadAndUseInSingleRequest(): void {
     // This request fails despite the upload succeeding, because we're not
     // allowed to view the entity we're uploading to.
     $response = $this->fileRequest($uri, $this->testFileData);
-    $this->assertResourceErrorResponse(403, $this->getExpectedUnauthorizedAccessMessage('GET'), $uri, $response, FALSE, ['4xx-response', 'http_response'], ['url.site', 'user.permissions']);
+    $this->assertResourceErrorResponse(403, $this->getExpectedUnauthorizedAccessMessage('GET'), $uri, $response, FALSE, ['4xx-response', 'http_response'], ['url.query_args', 'url.site', 'user.permissions']);
 
     $this->setUpAuthorization('GET');
 
diff --git a/core/modules/jsonapi/tests/src/Functional/NodeTest.php b/core/modules/jsonapi/tests/src/Functional/NodeTest.php
index 710fa6577f44..fa1d5828cddc 100644
--- a/core/modules/jsonapi/tests/src/Functional/NodeTest.php
+++ b/core/modules/jsonapi/tests/src/Functional/NodeTest.php
@@ -345,7 +345,7 @@ public function testGetIndividual(): void {
       $response,
       '/data',
       ['4xx-response', 'http_response', 'node:1'],
-      ['url.query_args:resourceVersion', 'url.site', 'user.permissions'],
+      ['url.query_args', 'url.site', 'user.permissions'],
       FALSE,
       'MISS'
     );
diff --git a/core/modules/jsonapi/tests/src/Functional/ResourceResponseTestTrait.php b/core/modules/jsonapi/tests/src/Functional/ResourceResponseTestTrait.php
index d5d8774e6f44..2f73786a5e7b 100644
--- a/core/modules/jsonapi/tests/src/Functional/ResourceResponseTestTrait.php
+++ b/core/modules/jsonapi/tests/src/Functional/ResourceResponseTestTrait.php
@@ -525,7 +525,7 @@ protected static function getAccessDeniedResponse(EntityInterface $entity, Acces
       'jsonapi' => static::$jsonApiMember,
       'errors' => [$error],
     ], 403))
-      ->addCacheableDependency((new CacheableMetadata())->addCacheTags(['4xx-response', 'http_response'])->addCacheContexts(['url.site']))
+      ->addCacheableDependency((new CacheableMetadata())->addCacheTags(['4xx-response', 'http_response'])->addCacheContexts(['url.query_args', 'url.site']))
       ->addCacheableDependency($access);
   }
 
@@ -545,8 +545,7 @@ protected function getEmptyCollectionResponse($cardinality, $self_link) {
     // If the entity type is revisionable, add a resource version cache context.
     $cache_contexts = Cache::mergeContexts([
       // Cache contexts for JSON:API URL query parameters.
-      'url.query_args:fields',
-      'url.query_args:include',
+      'url.query_args',
       // Drupal defaults.
       'url.site',
     ], $this->entity->getEntityType()->isRevisionable() ? ['url.query_args:resourceVersion'] : []);
diff --git a/core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php b/core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php
index f95f4a4c1c39..5960ca8a8954 100644
--- a/core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php
+++ b/core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php
@@ -495,7 +495,7 @@ protected function getPatchDocument() {
   protected function getExpectedUnauthorizedAccessCacheability() {
     return (new CacheableMetadata())
       ->setCacheTags(['4xx-response', 'http_response'])
-      ->setCacheContexts(['url.site', 'user.permissions'])
+      ->setCacheContexts(['url.query_args', 'url.site', 'user.permissions'])
       ->addCacheContexts($this->entity->getEntityType()->isRevisionable()
         ? ['url.query_args:resourceVersion']
         : []
@@ -546,8 +546,7 @@ protected function getExtraRevisionCacheTags() {
   protected function getExpectedCacheContexts(?array $sparse_fieldset = NULL) {
     $cache_contexts = [
       // Cache contexts for JSON:API URL query parameters.
-      'url.query_args:fields',
-      'url.query_args:include',
+      'url.query_args',
       // Drupal defaults.
       'url.site',
       'user.permissions',
@@ -608,11 +607,7 @@ protected static function getExpectedCollectionCacheability(AccountInterface $ac
     $cacheability->addCacheTags($entity_type->getListCacheTags());
     $cache_contexts = [
       // Cache contexts for JSON:API URL query parameters.
-      'url.query_args:fields',
-      'url.query_args:filter',
-      'url.query_args:include',
-      'url.query_args:page',
-      'url.query_args:sort',
+      'url.query_args',
       // Drupal defaults.
       'url.site',
     ];
@@ -1077,12 +1072,12 @@ public function testGetIndividual(): void {
     $message_url = clone $url;
     $path = str_replace($random_uuid, '{entity}', $message_url->setAbsolute()->setOptions(['base_url' => '', 'query' => []])->toString());
     $message = 'The "entity" parameter was not converted for the path "' . $path . '" (route name: "jsonapi.' . static::$resourceTypeName . '.individual")';
-    $this->assertResourceErrorResponse(404, $message, $url, $response, FALSE, ['4xx-response', 'http_response'], ['url.site'], FALSE, 'UNCACHEABLE');
+    $this->assertResourceErrorResponse(404, $message, $url, $response, FALSE, ['4xx-response', 'http_response'], ['url.query_args', 'url.site'], FALSE, 'UNCACHEABLE');
 
     // DX: when Accept request header is missing, still 404, same response.
     unset($request_options[RequestOptions::HEADERS]['Accept']);
     $response = $this->request('GET', $url, $request_options);
-    $this->assertResourceErrorResponse(404, $message, $url, $response, FALSE, ['4xx-response', 'http_response'], ['url.site'], FALSE, 'UNCACHEABLE');
+    $this->assertResourceErrorResponse(404, $message, $url, $response, FALSE, ['4xx-response', 'http_response'], ['url.query_args', 'url.site'], FALSE, 'UNCACHEABLE');
   }
 
   /**
@@ -1157,8 +1152,7 @@ public function testCollection(): void {
       $expected_error_message = "The current user is not authorized to filter by the `field_jsonapi_test_entity_ref` field, given in the path `field_jsonapi_test_entity_ref`. The 'field_jsonapi_test_entity_ref view access' permission is required.";
       $expected_cache_tags = ['4xx-response', 'http_response'];
       $expected_cache_contexts = [
-        'url.query_args:filter',
-        'url.query_args:sort',
+        'url.query_args',
         'url.site',
         'user.permissions',
       ];
@@ -1710,7 +1704,7 @@ protected function doTestRelationshipMutation(array $request_options) {
    */
   protected function getExpectedGetRelationshipResponse($relationship_field_name, ?EntityInterface $entity = NULL) {
     $entity = $entity ?: $this->entity;
-    $access = AccessResult::neutral()->addCacheContexts($entity->getEntityType()->isRevisionable() ? ['url.query_args:resourceVersion'] : []);
+    $access = AccessResult::neutral()->addCacheContexts($entity->getEntityType()->isRevisionable() ? ['url.query_args'] : []);
     $access = $access->orIf(static::entityFieldAccess($entity, $this->resourceType->getInternalName($relationship_field_name), 'view', $this->account));
     if (!$access->isAllowed()) {
       $via_link = Url::fromRoute(
@@ -1724,8 +1718,7 @@ protected function getExpectedGetRelationshipResponse($relationship_field_name,
       ->addCacheTags(['http_response'])
       ->addCacheContexts([
         'url.site',
-        'url.query_args:include',
-        'url.query_args:fields',
+        'url.query_args',
       ])
       ->addCacheableDependency($entity)
       ->addCacheableDependency($access);
@@ -1922,7 +1915,7 @@ protected function getExpectedRelatedResponse($relationship_field_name, array $r
     // every related resource.
     $base_resource_identifier = static::toResourceIdentifier($entity);
     $internal_name = $this->resourceType->getInternalName($relationship_field_name);
-    $access = AccessResult::neutral()->addCacheContexts($entity->getEntityType()->isRevisionable() ? ['url.query_args:resourceVersion'] : []);
+    $access = AccessResult::neutral()->addCacheContexts($entity->getEntityType()->isRevisionable() ? ['url.query_args'] : []);
     $access = $access->orIf(static::entityFieldAccess($entity, $internal_name, 'view', $this->account));
     if (!$access->isAllowed()) {
       $detail = 'The current user is not allowed to view this relationship.';
@@ -1944,8 +1937,7 @@ protected function getExpectedRelatedResponse($relationship_field_name, array $r
       if (empty($relationship_document['data'])) {
         $cache_contexts = Cache::mergeContexts([
           // Cache contexts for JSON:API URL query parameters.
-          'url.query_args:fields',
-          'url.query_args:include',
+          'url.query_args',
           // Drupal defaults.
           'url.site',
         ], $this->entity->getEntityType()->isRevisionable() ? ['url.query_args:resourceVersion'] : []);
@@ -3041,8 +3033,7 @@ public function testRevisions(): void {
     $actual_response = $this->request('GET', $rel_working_copy_collection_url_filtered, $request_options);
     $filtered_collection_expected_cache_contexts = [
       'url.path',
-      'url.query_args:filter',
-      'url.query_args:resourceVersion',
+      'url.query_args',
       'url.site',
     ];
     $this->assertResourceErrorResponse(501, 'JSON:API does not support filtering on revisions other than the latest version because a secure Drupal core API does not yet exist to do so.', $rel_working_copy_collection_url_filtered, $actual_response, FALSE, ['http_response'], $filtered_collection_expected_cache_contexts);
@@ -3050,7 +3041,7 @@ public function testRevisions(): void {
     $actual_response = $this->request('GET', $rel_invalid_collection_url, $request_options);
     $invalid_version_expected_cache_contexts = [
       'url.path',
-      'url.query_args:resourceVersion',
+      'url.query_args',
       'url.site',
     ];
     $this->assertResourceErrorResponse(400, 'Collection resources only support the following resource version identifiers: rel:latest-version, rel:working-copy', $rel_invalid_collection_url, $actual_response, FALSE, ['4xx-response', 'http_response'], $invalid_version_expected_cache_contexts);
diff --git a/core/modules/jsonapi/tests/src/Functional/RestJsonApiUnsupported.php b/core/modules/jsonapi/tests/src/Functional/RestJsonApiUnsupported.php
index ade4cf3b6903..c07992ca8013 100644
--- a/core/modules/jsonapi/tests/src/Functional/RestJsonApiUnsupported.php
+++ b/core/modules/jsonapi/tests/src/Functional/RestJsonApiUnsupported.php
@@ -104,7 +104,7 @@ public function testApiJsonNotSupportedInRest(): void {
       FALSE,
       $response,
       ['4xx-response', 'config:system.logging', 'config:user.role.anonymous', 'http_response', 'node:1'],
-      ['url.query_args:_format', 'url.site', 'user.permissions'],
+      ['url.query_args', 'url.site', 'user.permissions'],
       'MISS',
       'MISS'
     );
diff --git a/core/modules/jsonapi/tests/src/Functional/UserTest.php b/core/modules/jsonapi/tests/src/Functional/UserTest.php
index a7db8bd9aa5f..7231798f6f98 100644
--- a/core/modules/jsonapi/tests/src/Functional/UserTest.php
+++ b/core/modules/jsonapi/tests/src/Functional/UserTest.php
@@ -458,7 +458,7 @@ public function testQueryInvolvingRoles(): void {
     $this->grantPermissionsToTestedRole(['administer users']);
 
     $response = $this->request('GET', $collection_url, $request_options);
-    $expected_cache_contexts = ['url.path', 'url.query_args:filter', 'url.site'];
+    $expected_cache_contexts = ['url.path', 'url.query_args', 'url.site'];
     $this->assertResourceErrorResponse(400, "Filtering on config entities is not supported by Drupal's entity API. You tried to filter on a Role config entity.", $collection_url, $response, FALSE, ['4xx-response', 'http_response'], $expected_cache_contexts, FALSE, 'MISS');
   }
 
diff --git a/core/modules/jsonapi/tests/src/Traits/CommonCollectionFilterAccessTestPatternsTrait.php b/core/modules/jsonapi/tests/src/Traits/CommonCollectionFilterAccessTestPatternsTrait.php
index 35f84c346cd7..a5a919d1ef03 100644
--- a/core/modules/jsonapi/tests/src/Traits/CommonCollectionFilterAccessTestPatternsTrait.php
+++ b/core/modules/jsonapi/tests/src/Traits/CommonCollectionFilterAccessTestPatternsTrait.php
@@ -105,8 +105,7 @@ public function doTestCollectionFilterAccessBasedOnPermissions($label_field_name
     $message = "The current user is not authorized to filter by the `spotlight` field, given in the path `spotlight`.";
     $expected_cache_tags = ['4xx-response', 'http_response'];
     $expected_cache_contexts = [
-      'url.query_args:filter',
-      'url.query_args:sort',
+      'url.query_args',
       'url.site',
       'user.permissions',
     ];
-- 
GitLab