This module may have an access cacheability metadata bug.

Te code includes multiple access checks but there is a chance the cacheability metadata from the access check isn't bubbled up to the resultant render array.

Steps to test and verify if there is an issue or not.

  • Create a twig template that uses drupal_entity()
  • Implement hook_entity_access and add some access logic that doesn't rely solely on permissions (As the user.permissions context is always applied to every render array)
  • Verify if the output is cached without the cache metadata and can be bypassed by another user (via cache-retrieval)

The issue was originally submitted on security.drupal.org by @larowlan and moved here with the permission of security team.

Comments

Chi created an issue. See original summary.

chi’s picture

The issue can arise when implementing a custom access workflow based on something different from user permissions. For instance, the access control may be based on some field values in user profile and implemented via hook_node_access(). Anyway, I believe it's up for developers to ensure bubbling access cacheability metadata when implementing such workflow. Currently it can be done like follows.

{{ drupal_entity('node', 123) }}
{{ {'#cache': some_cache_metadata} }}

Note that's not specific to Twig templates. Access cachebility metatdata needs to be respected in PHP code as well.
The following code has the same problem.

if ($node->access('view')) {
  $build = \Drupal::entityTypeManager()
    ->getViewBuilder('node')
    ->view($node);
}

The correct approach would be like this.

$build = [];
$access = $node->access('view', NULL, TRUE);
if ($access->isAllowed()) {
  $build = \Drupal::entityTypeManager()
    ->getViewBuilder('node')
    ->view($node);
}
CacheableMetadata::createFromRenderArray($build)
  ->merge(CacheableMetadata::createFromObject($access))
  ->applyTo($build);

I think Twig Tweak should merge those metadata itself to make the result more secure by default.

Chi credited larowlan.

chi’s picture

  • Chi committed f1b0efd on 8.x-2.x
    Issue #3116410 by larowlan: The module may not bubble access...

  • Chi committed f135078 on 8.x-1.x
    Issue #3116410 by larowlan: The module may not bubble access...
chi’s picture

The fix was pushed in 3.x branch and backported to 2.x and 1.x branches.
In 2.x and 1.x branches Twig Tweak functions does not return cache metadata when access is not allowed. That was done to preserve backward compatibility. Returning not NULL value in this case would break conditions in Twig templates like this one.

{% set content = drupal_entity('node', 123) %}
{% if content %}
  {{ content }}
{% endif %}

That's not perfect but it fixes the security problems with access control. 3.x branch of the module uses more correct approach.

chi’s picture

Status: Active » Fixed

Status: Fixed » Closed (fixed)

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