Twig template variables containing result of Drupal::url() and Drupal:l:() don't bubble up their cacheability and attachment metadata (e.g. token placeholder)

How would this be tested, just with Core?

#2646328: CSRF in update module manual check links might show an example of this.

Maybe it's by design that if you don't generate the link as a render array then it doesn't get the cacheability data correct? But that seems somewhat unsatisfying since there are multiple ways to generate URLs and links and it would be nice for them to work equally well.

Confirmed while I worked on #2646328-31: CSRF in update module manual check links.

Quoting my analysis from there:

+++ b/core/modules/update/update.module
@@ -567,7 +567,12 @@ function _update_project_status_sort($a, $b) {
-  $variables['link'] = \Drupal::l(t('Check manually'), new Url('update.manual_status', array(), array('query' => \Drupal::destination()->getAsArray())));
+  $variables['link'] = [
+    '#type' => 'link',
+    '#title' => t('Check manually'),
+    '#url' => Url::fromRoute('update.manual_status', [], ['query' => \Drupal::destination()->getAsArray()]),
+  ];

This should not be necessary. The code original code should work.

This is indeed related to #2575519: Twig template variables containing result of Drupal::url() and Drupal:l:() don't bubble up their cacheability and attachment metadata (e.g. token placeholder).

Rather than making this change, we should fix the root cause.

---

ThemeManager does this:

    if (isset($info['preprocess functions'])) {
      foreach ($info['preprocess functions'] as $preprocessor_function) {
        if (function_exists($preprocessor_function)) {
          $preprocessor_function($variables, $hook, $info);
        }
      }
      // Allow theme preprocess functions to set $variables['#attached'] and
      // $variables['#cache'] and use them like the corresponding element
      // properties on render arrays. In Drupal 8, this is the (only) officially
      // supported method of attaching bubbleable metadata from preprocess
      // functions. Assets attached here should be associated with the template
      // that we are preprocessing variables for.
      $preprocess_bubbleable = [];
      foreach (['#attached', '#cache'] as $key) {
        if (isset($variables[$key])) {
          $preprocess_bubbleable[$key] = $variables[$key];
        }
      }
      // We do not allow preprocess functions to define cacheable elements.
      unset($preprocess_bubbleable['#cache']['keys']);
      if ($preprocess_bubbleable) {
        // @todo Inject the Renderer in https://www.drupal.org/node/2529438.
        drupal_render($preprocess_bubbleable);
      }
    }

I.e. we allow preprocess functions to explicitly set #attached and #cache. In this case, our preprocess function is not doing that. Instead, it is assigning a value to one of the variables that itself contains cacheability metadata and attachments!

So, where these variables are being used, we must update Twig to do the necessary bubbling, just when those variables are actually printed.

Here's the patch from #2646328-31: CSRF in update module manual check links.

Cleanup.

+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -463,6 +467,38 @@ public function escapeFilter(\Twig_Environment $env, $arg, $strategy = 'html', $
+    if ($arg instanceof CacheableDependencyInterface || $arg instanceof AttachmentsInterface) {
+      $arg_bubbleable = [];
+      if ($arg instanceof CacheableDependencyInterface) {
+        $arg_bubbleable['#cache']['contexts'] = $arg->getCacheContexts();
+        $arg_bubbleable['#cache']['tags'] = $arg->getCacheTags();
+        $arg_bubbleable['#cache']['max-age'] = $arg->getCacheMaxAge();
+      }
+      if ($arg instanceof AttachmentsInterface) {
+        $arg_bubbleable['#attached'] = $arg->getAttachments();
+      }
+      if ($arg_bubbleable) {
+        $this->renderer->render($arg_bubbleable);
+      }
+    }

Can't we avoid one level of nesting here actually? Basically keep the exact same code, beside the first if

Re #9 we could do an early return... and then lose the last if.

Addressed #9.

Addressed Alex' feedback from IRC:

12:10:09 <alexpott> WimLeers: I don't think bubbling is necessary on objects that implement RenderableInterface
12:10:35 <alexpott> WimLeers: because that should be present in the render array - no?
12:13:54 <WimLeers> alexpott: fair
12:14:00 <WimLeers> alexpott: though I don't know of objects that implement both :P
12:14:07 <WimLeers> alexpott: but totally true we shouldn't do it in that case
12:14:07 <alexpott> WimLeers: yeah :)

I will write tests later today, unless somebody beats me to it.

Now with updated issue summary.

Unfortunately we have to fix this in more places :(

Beginning of some tests... we need more coverage for attachments and Renderable interface

Working on some more test coverage.

Here is some simplification of the code + more test coverage aka. also take care into account libraries.

There we go

This has tests. I've ran the tests manually and read through the code and it seems to be doing what it needs to achieve this fix.

TwigExtensionTest will fail before this fix and passess afterwards. ThemeRenderAndAutoescapeTest needs the datastructure that is provided to the RenderContext() via bubbling.

Committed/pushed to both branches, thanks!

I confirm that this patch also fix CSRF Token problem in this Drupal StackExchange Issue