Make 4xx REST responses cacheable by (Dynamic) Page Cache + comprehensive cacheability test coverage

Thanks so much! Especially for your crystal-clear bug report, but also the patch! :) Much appreciated!

+++ b/core/modules/rest/src/RequestHandler.php
@@ -128,7 +129,14 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
-      return new Response($content, $e->getStatusCode(), $headers);
+      if ($e->getStatusCode() >= 400 && $e->getStatusCode() < 500) {
+        $response = new CacheableResponse($content, $e->getStatusCode(), $headers);
+        $response->addCacheableDependency($this->container->get('config.factory')->get('rest.settings'));
+        return $response;
+      }
+      else {
+        return new Response($content, $e->getStatusCode(), $headers);
+      }

This is the wrong place to be making this change. The problem is that this is lacking cacheability metadata for the resource itself.

AFAICT this is repurposing the 4xx-response cache tag for a different purpose, that just happens to work in the special case you have, but it won't work in every case.

The root cause here is that the REST module was architected in a time where the cache system in D8 wasn't matured yet. So it uses the Symfony convention of

if (!$entity_access->isAllowed()) {
      throw new AccessDeniedHttpException();
}

… which then also throws away cacheability metadata in the access check result.

The proper solution would be to not throw that exception in EntityResource::get() anymore, but instead send a CacheableResponse() with a 403 status, and the cacheability metadata stored in $entity_access:
return (new CacheableResponse('', 403))->addCacheableDependency($entity_access);

Well, yes, every REST resource whose access depends on modifiable data (like an entity) will have to return a 403/404 response with the necessary cacheability metadata, they should not use the Symfony exceptions. That is annoying (but is only because Symfony's HTTP kernel/response objects/etc design is frankly quite short-sighted), but necessary. It won't be necessary for many REST resources though: most don't have this advanced access checking logic. And for those that do, this would mean core would be providing the right example.

Well, it feels like optimizing for 400 errors is kind of a weird one. If people want to, they can always bypass caching easily for those.

This patch looks great to me. It's only missing explicit tests for entity REST resources first resulting in a 404 or 403 and then after modifying the entity resulting in a 200.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/ExceptionJsonSubscriber.php
   @@ -34,7 +34,7 @@ protected static function getPriority() {
   -    $response = new JsonResponse(array('message' => $event->getException()->getMessage()), Response::HTTP_FORBIDDEN);
   +    $response = new CacheableJsonResponse(array('message' => $event->getException()->getMessage()), Response::HTTP_FORBIDDEN);
   
   @@ -45,7 +45,7 @@ public function on403(GetResponseForExceptionEvent $event) {
   -    $response = new JsonResponse(array('message' => $event->getException()->getMessage()), Response::HTTP_NOT_FOUND);
   +    $response = new CacheableJsonResponse(array('message' => $event->getException()->getMessage()), Response::HTTP_NOT_FOUND);
   
   @@ -56,7 +56,7 @@ public function on404(GetResponseForExceptionEvent $event) {
   -    $response = new JsonResponse(array('message' => $event->getException()->getMessage()), Response::HTTP_METHOD_NOT_ALLOWED);
   +    $response = new CacheableJsonResponse(array('message' => $event->getException()->getMessage()), Response::HTTP_METHOD_NOT_ALLOWED);
   
   @@ -67,7 +67,7 @@ public function on405(GetResponseForExceptionEvent $event) {
   -    $response = new JsonResponse(['message' => $event->getException()->getMessage()], Response::HTTP_NOT_ACCEPTABLE);
   +    $response = new CacheableJsonResponse(['message' => $event->getException()->getMessage()], Response::HTTP_NOT_ACCEPTABLE);
   

   Why are we changing these?

   There's no cacheability metadata for exceptions.

2. +++ b/core/modules/rest/src/RequestHandler.php
   @@ -95,7 +96,9 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
   -          return new Response($content, 400, array('Content-Type' => $request->getMimeType($format)));
   +          $response = new CacheableResponse($content, 400, ['Content-Type' => $request->getMimeType($format)]);
   +          $response->addCacheableDependency($this->container->get('config.factory')->get('rest.settings'));
   
   @@ -125,7 +128,9 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
   -      return new Response($content, $e->getStatusCode(), $headers);
   +      $response = new CacheableResponse($content, $e->getStatusCode(), $headers);
   +      $response->addCacheableDependency($this->container->get('config.factory')->get('rest.settings'));
   

   These seem out of scope for the same reason as the first point.

3. +++ b/core/modules/serialization/src/EventSubscriber/DefaultExceptionSubscriber.php
   @@ -127,7 +128,7 @@ protected function setEventResponse(GetResponseForExceptionEvent $event, $status
   -    $response = new Response($encoded_content, $status);
   +    $response = new CacheableResponse($encoded_content, $status);
   

   This one also seems out of scope for the same reason as the first point.

i.e. the scope of this issue should be to explicitly support 403/404 responses for EntityResource responses having the appropriate cache tags. It should not change "responses for exceptions", because those don't carry cacheability metadata anyway.

They DO NOT need to do

return new ResourceResponse(NULL, 404);

They need to do

$response = new ResourceResponse(NULL, 404);
$response->addCacheableDependency($acces_result_object);
return $response;

Because you need a response object that implements \Drupal\Core\Cache\CacheableResponseInterface to be able to add cacheability metadata.

throw new NotFoundException();

can never do the same thing, because an exception does not carry cacheability metadata. It could, but it doesn't. And doing that would require Drupal developers to never use Symfony's HttpException classes, but only Drupal's "cacheability metadata-compatible exceptions". Which is not realistic.

So, let's please focus only on the concrete problem, and not try to solve the generic problem. Because solving the generic problem would require 10 times as many changes. Feel free to create a new issue for that though, if you feel passionate about this.

I still would like to question this issue with my comment #16

Where does $acces_result_object come from on a 404? There is no entity, what are we checking access against?

True, you can't have that in case of a 404. So I guess I'm saying I so far am only convinced by the 403 handling. Although even in that case you have authentication to deal with, so … I wonder to what extent it's even cacheable.

Is there any cachable metadata in there?

No, there isn't. And that's why that response cannot be cacheable safely(in the sense that it can never be guaranteed to be up to date if it were made cacheable, because there are no cache tags for DB log entries). Entities do have cache tags, so there we can do that.

I still would like to question this issue with my comment #16

Hm good point. @davidwbarratt said: We are just making REST resources behave just like HTML resources. — except one of the big reasons we make 4xx HTML responses cacheable by PageCache is because search engine spiders/robots tend to cause a lot of requests. Having many requests to REST routes is less likely. Even less so because usually you need to be authenticated. And if you're authenticated, the Page Cache is no use anyway.

@davidwbarratt: I'm guessing that in your use case, you're doing REST requests for anonymous users? (i.e. anonymous users can access those routes?)

@dawehner: I think that in general it'd be interesting to have explicit support for REST+PageCache for anonymous user use cases, and for REST+Dynamic Page Cache for authenticated user use cases.

Thank you for explaining a bit what is going on. Just to be clear, if someone is attacking your site though, you have a hard time in Drupal anyway as its trivial to bypass caching layers.

IMHO we should rename the issue title and focus the issue a bit more on GET requests, see some reasons below.

1. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -82,8 +85,11 @@ public function post(EntityInterface $entity = NULL) {
   +    $entity_access = $entity->access('create', NULL, TRUE);
   +    if (!$entity_access->isAllowed()) {
   +      $response = new ResourceResponse(NULL, 403);
   +      $response->addCacheableDependency($entity_access);
   +      return $response;
   
   @@ -101,8 +107,14 @@ public function post(EntityInterface $entity = NULL) {
   +      $field_access = $entity->get($field_name)->access('edit', NULL, TRUE);
   +      if (!$field_access->isAllowed()) {
   +        $response = new ResourceResponse([
   +          'error' => "Access denied on creating field '$field_name'"
   +        ], 403);
   +        $response->addCacheableDependency($entity_access);
   +        $response->addCacheableDependency($field_access);
   +        return $response;
   
   @@ -145,8 +157,12 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
   +    $original_entity_access = $original_entity->access('update', NULL, TRUE);
   +    if (!$original_entity_access->isAllowed()) {
   +      $response = new ResourceResponse(NULL, 403);
   +      $response->addCacheableDependency($original_entity);
   +      $response->addCacheableDependency($original_entity_access);
   +      return $response;
        }
   
   @@ -159,8 +175,15 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
   +      $field_access = $original_entity->get($field_name)->access('edit', NULL, TRUE);
   +      if (!$field_access->isAllowed()) {
   +        $response = new ResourceResponse([
   +          'error' => "Access denied on updating field '$field_name'."
   +        ], 403);
   +        $response->addCacheableDependency($original_entity);
   +        $response->addCacheableDependency($original_entity_access);
   +        $response->addCacheableDependency($field_access);
   +        return $response;
   
   @@ -191,8 +214,12 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
   +    $entity_access = $entity->access('delete', NULL, TRUE);
   +    if (!$entity_access->isAllowed()) {
   +      $response = new ResourceResponse(NULL, 403);
   +      $response->addCacheableDependency($entity);
   +      $response->addCacheableDependency($entity_access);
   +      return $response;
   

   Its honestly a bit weird that we add cacheabiliy metadata for non GET/HEAD requests ... These access denied conditions vary by the submitted fields for example, so do we vary somewhere by the HTTP body? I don't see that. Yes, DELETE requests are theoretically cacheable, given they are idempotent, but that's a weird optimization strategy

2. +++ b/core/modules/rest/src/RequestHandler.php
   @@ -7,6 +7,7 @@
   @@ -66,7 +67,9 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
   
   @@ -66,7 +67,9 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
            catch (UnexpectedValueException $e) {
              $error['error'] = $e->getMessage();
              $content = $serializer->serialize($error, $format);
   -          return new Response($content, 400, array('Content-Type' => $request->getMimeType($format)));
   +          $response = new CacheableResponse($content, 400, ['Content-Type' => $request->getMimeType($format)]);
   +          $response->addCacheableDependency($this->container->get('config.factory')->get('rest.settings'));
   

   Just reading that code ... we are throwing an exception and then assuming we know how to cache that. Do we know what varied in order to throw the exception? To do this right one would need for example some kind of cache context for the serialized value at least. I don't see that.

If you take a look at CommandLineOrUnsafeMethod::check we're explicitly denying cachability to any "unsafe" methods. The unsafe methods can be found in Request::isMethodSafe since only GET and HEAD are safe methods those are the only ones that will be cached. Unless a developer wants to explicitly override the cache policy (which they should be allowed to do).

I would totally agree in the case we would get the cacheability metadata right, but for example for POST requests one would have to vary by the POST data, but we don't yet.
Given that IMHO we should either implement it correctly, or not at all.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/ExceptionJsonSubscriber.php
   @@ -34,7 +34,7 @@ protected static function getPriority() {
       */
      public function on403(GetResponseForExceptionEvent $event) {
   -    $response = new JsonResponse(array('message' => $event->getException()->getMessage()), Response::HTTP_FORBIDDEN);
   +    $response = new CacheableJsonResponse(array('message' => $event->getException()->getMessage()), Response::HTTP_FORBIDDEN);
        $event->setResponse($response);
      }
    
   @@ -45,7 +45,7 @@ public function on403(GetResponseForExceptionEvent $event) {
   
   @@ -45,7 +45,7 @@ public function on403(GetResponseForExceptionEvent $event) {
       *   The event to process.
       */
      public function on404(GetResponseForExceptionEvent $event) {
   -    $response = new JsonResponse(array('message' => $event->getException()->getMessage()), Response::HTTP_NOT_FOUND);
   +    $response = new CacheableJsonResponse(array('message' => $event->getException()->getMessage()), Response::HTTP_NOT_FOUND);
        $event->setResponse($response);
      }
    
   @@ -56,7 +56,7 @@ public function on404(GetResponseForExceptionEvent $event) {
   
   @@ -56,7 +56,7 @@ public function on404(GetResponseForExceptionEvent $event) {
       *   The event to process.
       */
      public function on405(GetResponseForExceptionEvent $event) {
   -    $response = new JsonResponse(array('message' => $event->getException()->getMessage()), Response::HTTP_METHOD_NOT_ALLOWED);
   +    $response = new CacheableJsonResponse(array('message' => $event->getException()->getMessage()), Response::HTTP_METHOD_NOT_ALLOWED);
        $event->setResponse($response);
      }
    
   @@ -67,7 +67,7 @@ public function on405(GetResponseForExceptionEvent $event) {
   
   @@ -67,7 +67,7 @@ public function on405(GetResponseForExceptionEvent $event) {
       *   The event to process.
       */
      public function on406(GetResponseForExceptionEvent $event) {
   -    $response = new JsonResponse(['message' => $event->getException()->getMessage()], Response::HTTP_NOT_ACCEPTABLE);
   +    $response = new CacheableJsonResponse(['message' => $event->getException()->getMessage()], Response::HTTP_NOT_ACCEPTABLE);
        $event->setResponse($response);
      }
   

   Mh, so by what does this cacheable response varies at that point? All those responses vary by nothing, which, can't be right.

2. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -96,11 +105,19 @@ public function post(EntityInterface $entity = NULL) {
   +      $field_access = $entity->get($field_name)->access('edit', NULL, TRUE);
   +      if (!$field_access->isAllowed()) {
   +        $response = new ResourceResponse([
   +          'message' => "Access denied on creating field '$field_name'"
   +        ], 403);
   +        $response->addCacheableDependency($entity_access);
   +        $response->addCacheableDependency($field_access);
   +        return $response;
          }
   

   Don't we ideally have to check access of all fields and then combine that? In general this also feels like an API break to move away from the exceptions into directly returning the response.

I thought about that, but the error message is specific to that specific field. We also return a response as soon as we encounter a field that we don't have access to. If we want to add all fields, we'd first have to check all access, then loop through them again to determine if any of them should end the request. I'm fine with it either way, I wasn't sure which one was the more proper way to do it. If you want I can change that.

Well, this wasn't about the specific message. The problem is that when you want to cache something you need to know all factors by which this request could vary. Given that, we need to know all access result objects for every field access. You can beside of that still show just the error message of the first failed field.

I mean I think the problem is, is that if you don't provide cachable metadata, how could we possible know the cache contexts? I believe if there are no Contexts, Drupal's Dynamic Cache will not cache it at all (which I think is fine because there is nothing to cache it by). I suppose they would be different cache contexts depending on the error (i.e. a 404/403 would be url) where a 405 would be url headers:status)? I think the real issue is, is that you should not be using the HTTP Exceptions unless there is no cachable metadata (see #39)

Well, you know, throwing those exceptions are part of our API, and they have been longer than any render caching. If code cannot longer rely on being executed, I am quite convinced we add way more bugs, than we try to solve here.

Yeah, this would vary by url.path, as well as this would require something like a cache tag for watchdog entries. As you see, this all potential causes some BC issues.

BC breaks should still not be introduced as part of fixing bugs, as we basically replace one bug fix by another :)

I think making the changes just to the ResourceHandler would be way more safe, than changing the generic exception subscriber, which might be used by a lot of other code. Does that make sense?

Nice progress here!

1. +++ b/core/lib/Drupal/Core/Entity/Entity.php
   @@ -527,10 +527,8 @@ protected function invalidateTagsOnSave($update) {
   -    if ($this->hasLinkTemplate('canonical')) {
   -      // Creating or updating an entity may change a cached 403 or 404 response.
   -      $tags = Cache::mergeTags($tags, ['4xx-response']);
   -    }
   +    // Creating or updating an entity may change a cached 403 or 404 response.
   +    $tags = Cache::mergeTags($tags, ['4xx-response']);
   

   Why change this? Because the 4xx response can also happen for routes other than the canonical one?

2. +++ b/core/lib/Drupal/Core/EventSubscriber/AuthenticationSubscriber.php
   @@ -125,6 +126,29 @@ public function onExceptionSendChallenge(GetResponseForExceptionEvent $event) {
   +  public function onForbiddenResponse(FilterResponseEvent $event) {
   

   Why is this necessary? Why is the existing \Drupal\Core\EventSubscriber\AuthenticationSubscriber::onExceptionSendChallenge() insufficient?

   I have suspicions, but the documentation on this method doesn't explain it at all. Nor is there explicit test coverage being added.

3. +++ b/core/lib/Drupal/Core/EventSubscriber/AuthenticationSubscriber.php
   --- a/core/lib/Drupal/Core/EventSubscriber/ExceptionJsonSubscriber.php
   +++ b/core/lib/Drupal/Core/EventSubscriber/ExceptionJsonSubscriber.php
   

   These changes are effectively duplicating \Drupal\serialization\EventSubscriber\DefaultExceptionSubscriber.

   We should consolidate them, not add more such implementations.

   I'd be fine with moving \Drupal\serialization\EventSubscriber\DefaultExceptionSubscriber into core/lib/Drupal/Core/EventSubscriber and deprecating the original one, and this ExceptionJsonSubscriber.

4. +++ b/core/lib/Drupal/Core/EventSubscriber/ExceptionJsonSubscriber.php
   @@ -34,8 +37,19 @@ protected static function getPriority() {
   +    $cache->setCacheContexts(['headers:authorization']);
   

   Hm… this is going to cache a separate response for every possible value of the Authorization header. That seems problematic too.

   Also, this points to missing test coverage for cached 4xx responses to prove me wrong :)

5. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -106,7 +105,10 @@ public static function create(ContainerInterface $container, array $configuratio
   +      $response = new ResourceResponse(NULL, 403);
   +      $response->addCacheableDependency($entity);
   +      $response->addCacheableDependency($entity_access);
   

   Ah! This at least partially explains the changes in AuthenticationSubscriber.

   Related: #2808233: REST 403 responses don't tell the user *why* access is not granted: requires deep Drupal understanding to figure out.

   Finally: why this approach? Why not subclass AccessDeniedHttpException so that the exception can also carry cacheability metadata?
   That'd be a far less invasive change.

6. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -144,8 +146,11 @@ public function post(EntityInterface $entity = NULL) {
   -    if (!$entity->access('create')) {
   -      throw new AccessDeniedHttpException();
   +    $entity_access = $entity->access('create', NULL, TRUE);
   +    if (!$entity_access->isAllowed()) {
   
   @@ -199,8 +217,12 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
   -    if (!$original_entity->access('update')) {
   -      throw new AccessDeniedHttpException();
   +    $original_entity_access = $original_entity->access('update', NULL, TRUE);
   +    if (!$original_entity_access->isAllowed()) {
   
   @@ -256,8 +285,12 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
   -    if (!$entity->access('delete')) {
   -      throw new AccessDeniedHttpException();
   +    $entity_access = $entity->access('delete', NULL, TRUE);
   +    if (!$entity_access->isAllowed()) {
   

   Why these changes? PATCH/POST/DELETE responses are not cacheable by definition anyway.

7. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -256,8 +285,12 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
   diff --git a/core/modules/rest/src/Plugin/rest/resource/EntityResourceAccessTrait.php b/core/modules/rest/src/Plugin/rest/resource/EntityResourceAccessTrait.php
   
   diff --git a/core/modules/rest/src/Plugin/rest/resource/EntityResourceAccessTrait.php b/core/modules/rest/src/Plugin/rest/resource/EntityResourceAccessTrait.php
   deleted file mode 100644
   

   Why delete this trait?

8. +++ b/core/modules/rest/src/RequestHandler.php
   @@ -88,6 +90,9 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
   +      // Set the request format so an Exception is returned in the proper
   +      // format.
   +      $request->setRequestFormat($format);
   

   Interesting… this would definitely need explicit test coverage in \Drupal\Tests\rest\Kernel\RequestHandlerTest(). And it probably needs to happen in a separate issue.

9. +++ b/core/modules/rest/src/RequestHandler.php
   @@ -107,9 +112,7 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
   -          $error['error'] = $e->getMessage();
   -          $content = $serializer->serialize($error, $format);
   -          return new Response($content, 400, array('Content-Type' => $request->getMimeType($format)));
   +          throw new BadRequestHttpException($e->getMessage(), $e);
   

   This is already being fixed in #2813853: RequestHandler has its own error handling rather than leaving it to KernelEvents::EXCEPTION event subscribers.

EDIT: cross-posted with #106.

This is making me realize that more and more we need a CachableHttpException

+1. But please make it CacheableHttpExceptionInterface + CacheableHttpExceptionTrait. Then you can simply do:

class CacheableAccessDeniedHttpException extends AccessDeniedHttpException implements CacheableHttpExceptionInterface {
  use CacheableHttpExceptionTrait;
}

#108.3: yes that'd be better.

#108.6: no, caching for POST/PATCH/DELETE never makes sense. They must result in modifications to the stored data, or at least attempted modifications. Which means their responses can never be cached.

Retesting #110, because CI error.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/AuthenticationSubscriber.php
   @@ -97,7 +98,7 @@ public function onKernelRequestFilterProvider(GetResponseEvent $event) {
   -        throw new AccessDeniedHttpException();
   +        throw new CacheableAccessDeniedHttpException();
          }
        }
      }
   @@ -119,6 +120,9 @@ public function onExceptionSendChallenge(GetResponseForExceptionEvent $event) {
   
   @@ -119,6 +120,9 @@ public function onExceptionSendChallenge(GetResponseForExceptionEvent $event) {
          if ($exception instanceof AccessDeniedHttpException && !$this->authenticationProvider->applies($request) && (!isset($this->filter) || $this->filter->appliesToRoutedRequest($request, FALSE))) {
            $challenge_exception = $this->challengeProvider->challengeException($request, $exception);
            if ($challenge_exception) {
   +          if ($exception instanceof CacheableHttpExceptionInterface && $challenge_exception instanceof CacheableHttpExceptionInterface) {
   +            $challenge_exception->addCacheableDependency($exception->getCacheableMetadata());
   +          }
              $event->setException($challenge_exception);
            }
          }
   @@ -126,32 +130,6 @@ public function onExceptionSendChallenge(GetResponseForExceptionEvent $event) {
   
   @@ -126,32 +130,6 @@ public function onExceptionSendChallenge(GetResponseForExceptionEvent $event) {
      }
    
      /**
   -   * Respond with a challenge on a 403 response if appropriate.
   -   *
   -   * On a 403 (access denied), if there are no credentials on the request, some
   -   * authentication methods (e.g. basic auth) require that a challenge is sent
   -   * to the client.
   -   *
   -   * @param \Symfony\Component\HttpKernel\Event\FilterResponseEvent $event
   -   *   The response event.
   -   */
   -  public function onForbiddenResponse(FilterResponseEvent $event) {
   -    if (isset($this->challengeProvider) && $event->getRequestType() === HttpKernelInterface::MASTER_REQUEST) {
   -      $request = $event->getRequest();
   -      $response = $event->getResponse();
   -      if ($response->getStatusCode() === 403 && !$this->authenticationProvider->applies($request) && (!isset($this->filter) || $this->filter->appliesToRoutedRequest($request, FALSE))) {
   -        $challenge_exception = $this->challengeProvider->challengeException($request, new AccessDeniedHttpException());
   -        if ($challenge_exception) {
   -          // Add the exception data to the current response so cachable metadata
   -          // is not overridden.
   -          $response->headers->add($challenge_exception->getHeaders());
   -          $response->setStatusCode($challenge_exception->getStatusCode());
   -        }
   -      }
   -    }
   -  }
   

   So much better!

2. +++ b/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php
   @@ -158,6 +159,11 @@ protected function makeSubrequest(GetResponseForExceptionEvent $event, $url, $st
   +      // Persist any cachable metadata.
   

   s/cachable/cacheability/

3. +++ b/core/lib/Drupal/Core/Routing/Enhancer/ParamConversionEnhancer.php
   @@ -77,7 +77,7 @@ protected function copyRawVariables(array $defaults) {
   -      $event->setException(new NotFoundHttpException($exception->getMessage(), $exception));
   +      $event->setException(new CacheableNotFoundHttpException($exception->getMessage(), $exception));
   
   +++ b/core/lib/Drupal/Core/Routing/RequestFormatRouteFilter.php
   @@ -45,7 +45,7 @@ public function filter(RouteCollection $collection, Request $request) {
   -    throw new NotAcceptableHttpException("No route found for the specified format $format.");
   +    throw new CacheableNotAcceptableHttpException("No route found for the specified format $format.");
   
   +++ b/core/modules/basic_auth/src/Authentication/Provider/BasicAuth.php
   @@ -130,7 +130,7 @@ public function challengeException(Request $request, \Exception $previous) {
   -    return new UnauthorizedHttpException((string) $challenge, 'No authentication credentials provided.', $previous);
   +    return new CacheableUnauthorizedHttpException((string) $challenge, 'No authentication credentials provided.', $previous);
   

   Why modify these? They don't contain cacheability metadata.

   Perhaps you were trying to update all code throwing Symfony HTTP exceptions? I think that's not necessary; just updating the ones that do have cacheability metadata is sufficient.