Problem/Motivation

See http://drupal.stackexchange.com/questions/175697/menu-callback-and-csrf/... and #2541166: Browse available tokens not shown.

The problem is that Twig template variables that implement either CacheableDependencyInterface (i.e. they carry cacheability metadata to bubble) or AttachmentsInterface (i.e. they carry attachments to bubble) don't have their metadata bubbled. Twig just uses their strings, and that's it.

Proposed resolution

Just before actually using/printing these Twig template variables, do the necessary bubbling, much like we do in ThemeManager.

Remaining tasks

  1. Patch.
  2. Manual testing.
  3. Test coverage.

User interface changes

None.

API changes

None.

Data model changes

None.

Comments

Berdir created an issue. See original summary.

mgifford’s picture

How would this be tested, just with Core?

David_Rothstein’s picture

#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.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Title: Drupal::url() and Drupal:l() seem to not bubble up their cacheability metadata (token placeholder) in some cases » Twig variables containing result of Drupal::url() and Drupal:l:() don't bubble up their cacheability and attachment metadata (e.g. token placeholder)
Component: routing system » theme system
Assigned: Unassigned » Wim Leers
Priority: Normal » Critical
Issue tags: +security

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.

Wim Leers’s picture

Wim Leers’s picture

Status: Active » Needs review
FileSize
2.24 KB
Wim Leers’s picture

Issue tags: +D8 cacheability, +Twig
FileSize
2.72 KB
1.56 KB

Cleanup.

dawehner’s picture

+++ 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

alexpott’s picture

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

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Issue tags: +Needs tests
FileSize
2.82 KB
1.91 KB

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.

Wim Leers’s picture

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

Now with updated issue summary.

alexpott’s picture

Unfortunately we have to fix this in more places :(

alexpott’s picture

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

dawehner’s picture

Working on some more test coverage.

dawehner’s picture

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

Status: Needs review » Needs work

The last submitted patch, 16: 2575519-16.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
9.48 KB
1.6 KB

There we go

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 18: 2575519-18.patch, failed testing.

Berdir’s picture

Could be a random fail:

fail: [Completion check] Line 26 of core/modules/system/src/Tests/Update/MenuTreeSerializationTitleTest.php:
The test did not complete due to a fatal error.

Didn't fail like that before.

Berdir’s picture

Status: Needs work » Reviewed & tested by the community

Requested a re-test.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to both branches, thanks!

  • catch committed fe6b686 on 8.2.x
    Issue #2575519 by Wim Leers, dawehner, alexpott: Twig template variables...
vorapoap’s picture

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

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.