VariationCache needs to be more defensive about cache context manipulation to avoid broken redirects

Marked as major because we have a few sleeper bugs in core that this might uncover.

MR needs work, first item in the chain is always the stored item or a redirect, so we can't have this "we already checked initially" stuff in there. Working on it.

Okay tests have thorough coverage now. Let's see if this breaks core and why.

Oof, I was afraid we'd see this many test fails. Let's start digging, I suppose.

CommentNonNodeTest message was:
Trying to set a redirect at an address that already had a redirect with nothing in common, old redirect at address "languages:language_interface, theme, user.permissions" was pointing to "url.path.parent, url.path.is_front", new redirect at same address points to "route".

Sucks how browser tests obscure these, so far I always put the following in these tests so I'd happily learn about a better way of debugging this:

    $this->assertEquals('foo', $this->getSession()->getPage()->getContent());

Okay so figured out where these are coming from: CommentBreadcrumbBuilder::build() and PathBasedBreadcrumbBuilder::build(). It seems like either one or the other fires, but not both, leading to the exception we introduced here as the same cache ID now triggers a completely different redirect.

Yup, from CommentBreadcrumbBuilder:

  public function applies(RouteMatchInterface $route_match) {
    return $route_match->getRouteName() == 'comment.reply' && $route_match->getParameter('entity');
  }

This conditional applies return value is never represented by the 'route' cache context when the result is FALSE.

I'm sure we'll find plenty similar core bugs while combing over the test fails here.

Found an issue that reports this, moving on.

Seems like the other bug is with jsonapi:

Trying to set a redirect at an address that already had a redirect with nothing in common, old redirect at address "url.site, route, request_format" was pointing to "url.query_args", new redirect at same address points to "url.query_args:fields, url.query_args:include, user.permissions".

The data it was trying to set:

\Drupal\Core\Cache\CacheableResponse::__set_state(array(
   'headers' => 
  \Symfony\Component\HttpFoundation\ResponseHeaderBag::__set_state(array(
     'headers' => 
    array (
      'cache-control' => 
      array (
        0 => 'no-cache, private',
      ),
      'date' => 
      array (
        0 => 'Mon, 03 Jun 2024 16:26:31 GMT',
      ),
      'content-type' => 
      array (
        0 => 'application/vnd.api+json',
      ),
    ),
     'cacheControl' => 
    array (
    ),
     'computedCacheControl' => 
    array (
      'no-cache' => true,
      'private' => true,
    ),
     'cookies' => 
    array (
    ),
     'headerNames' => 
    array (
      'cache-control' => 'Cache-Control',
      'date' => 'Date',
      'content-type' => 'Content-Type',
    ),
  )),
   'content' => '{"jsonapi":{"version":"1.0","meta":{"links":{"self":{"href":"http:\\/\\/jsonapi.org\\/format\\/1.0\\/"}}}},"data":{"type":"entity_view_display--entity_view_display","id":"643f166a-a78e-4bad-941c-493d34d10dff","links":{"self":{"href":"http:\\/\\/web\\/jsonapi\\/entity_view_display\\/entity_view_display\\/643f166a-a78e-4bad-941c-493d34d10dff"}},"attributes":{"langcode":"en","status":true,"dependencies":{"config":["node.type.camelids"],"module":["layout_builder","user"]},"third_party_settings":{"layout_builder":{"enabled":true,"allow_custom":true}},"drupal_internal__id":"node.camelids.default","targetEntityType":"node","bundle":"camelids","mode":"default","content":{"links":{"settings":[],"third_party_settings":[],"weight":100,"region":"content"}},"hidden":{"layout_builder__layout":true}}},"links":{"self":{"href":"http:\\/\\/web\\/jsonapi\\/entity_view_display\\/entity_view_display\\/643f166a-a78e-4bad-941c-493d34d10dff"}}}',
   'version' => '1.0',
   'statusCode' => 200,
   'statusText' => 'OK',
   'charset' => NULL,
   'cacheabilityMetadata' => 
  \Drupal\Core\Cache\CacheableMetadata::__set_state(array(
     'cacheContexts' => 
    array (
      0 => 'url.query_args:fields',
      1 => 'url.query_args:include',
      2 => 'url.site',
      3 => 'user.permissions',
    ),
     'cacheTags' => 
    array (
      0 => 'config:core.entity_view_display.node.camelids.default',
    ),
     'cacheMaxAge' => -1,
  )),
))'

Will try to debug that one further when I have time, but that might be Thursday at the earliest.

These are coming from JsonApiRequestValidator::validateQueryParams() and EntityResource:buildWrappedResponse() in that order. So probably also some check not making it into the cache contexts.

... defensive by checking for all previously set cache contexts every step of a redirect chain. If one is missing, we should immediately throw an exception and bail out. This should catch a lot of insecurely written code that doesn't properly set the right cache contexts by letting people know there is something wrong with their code.

Could this be also achieved/enforced by PHPStan (Drupal) too? (cc @mglaman)

I think I've figured out (some of) the jsonapi module failures -- will work more on them tomorrow.

I got as far as Drupal\jsonapi\Access\EntityAccessChecker::checkEntityAccess() where we have this:

      $access = AccessResult::neutral()->addCacheContexts(['url.query_args:' . JsonApiSpec::VERSION_QUERY_PARAMETER])->orIf($access);

with no user.roles context.

Current findings:

1. DPC sets the initial cache contexts of 'route' and 'request_format'
2. This ends up at a redirect to 'url.site', set by JsonApiDocumentTopLevelNormalizer::normalize()

Then, the first time we try to store the return value of JsonApiRequestValidator::validateQueryParams(), leading to the creation of a second CacheRedirect, pointing to 'url.query_args'

But the second time, we try to store the return value of EntityResource::buildWrappedResponse(), which adds 'url.query_args:fields' and 'url.query_args:include' and somewhere along the line 'user.permissions' also makes it in there. Probably an access check, but have to figure that one out.

So the redirect at address response:[request_format]=api_json:[route]=jsonapi.entity_view_display--entity_view_display.individual_SOME_HASH_1:[url.site]=http://web used to point at 'url.query_args', but now points to those new cache contexts leading to the final cache ID of:

response:[request_format]=api_json:[route]=jsonapi.entity_view_display--entity_view_display.individual_SOME_HASH_1:[url.query_args:fields]=:[url.query_args:include]=:[url.site]=http://web:[user.permissions]=_SOME_HASH_2

This means that whatever code comes after JsonApiDocumentTopLevelNormalizer::normalize() is currently inconsistent in setting cache contexts.

Fixed it. JsonApiRequestValidator::validateQueryParams() only sets url.query_args when validation fails, but obviously it also needs to set that when validation succeeds. Because if the query args change, so can the validity.

Okay so this shows that there's only two detectable bugs in core right now:

CommentBreadcrumbBuilder::applies(), I "fixed" this the wrong way here to make tests go green. This caused StandardPerformanceTest to fail as there are now 2 more cache gets, which makes sense because we added a cache context. It also made phpunit tests report an unknown failure.

JsonApiRequestValidator::validateQueryParams(), I fixed this properly here but it needs a dedicated issue. Will create that one next.

There might be more core bugs, such as the one we just fixed in 🐛 Insufficient cacheability information bubbled up by UserAccessControlHandler Active , but we don't always have tests that are extensive enough for this VC fix to throw exceptions there. I.e.: They are flying under the radar and this exception will over time throw an exception on sites that have warmed their caches enough to run into the buggy code.

@kristiaanvandeneynde some tests are still failing as I can see, needs work? :-/

https://git.drupalcode.org/issue/drupal-3452181/-/pipelines/197002/test_...

some tests are still failing as I can see, needs work?

Those fails stem from my "fix" for breadcrumbs. The actual fails caused by the VC changes are all green now. If we split off the breadcrumb and jsonapi fix everything will be green.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

I know it doesn't apply, that's the whole point :)