ToolbarController::preRenderGetRenderedSubtrees() instantiates a new $cacheability variable which it populates in a loop and then applies to the parent render array. The problem is that this new instance does not have the parent's cacheability and therefore might delete crucial cache contexts.

Instead of using a blank CaheableMetadata, an instance should be created from the parent render array like so:

    $cacheability = CacheableMetadata::createFromRenderArray($data);

Marking as major because in some edge cases I can't come up with this might lead to data exposure. Not filing as a security issue because 8.8.0 isn't out yet.

CommentFileSizeAuthor
#2 drupal-3082032-2.patch1.15 KBkristiaanvandeneynde

Comments

kristiaanvandeneynde created an issue. See original summary.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
Assigned: kristiaanvandeneynde » Unassigned
Status: Active » Needs review
StatusFileSize
new drupal-3082032-2.patch1.15 KB

This was discovered as part of #2551419-149: Abstract RenderCache into a separate service that is capable of cache redirects in a non-render array-specific way where fixing this made 37 failing tests go green.

nickdickinsonwilde’s picture

nickdickinsonwilde
He/Him/His
Primary language English
Location Victoria, BC (T'So-uke lands)
commented
Status: Needs review » Reviewed & tested by the community

Looks good to me and the testbot.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

If this is a major bug fix then we need tests to prove we've fixed it and don't break it in the future.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Issue tags: +Needs tests

Tagged with needs tests to make it clear that we're waiting for tests (as mentioned in #4)

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

catch’s picture

catch
he/him
Primary language English
commented

If #2551419: Abstract RenderCache into a separate service that is capable of cache redirects in a non-render array-specific way causes test failures without this, then that might be enough for test coverage.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

Re #13: As #2 indicates, that seems to be the case.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs tests

Let's see what @alexpott thinks four years later.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Assigned: Unassigned » alexpott

Per #15

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 10.1.x-dev » 9.5.x-dev
Status: Reviewed & tested by the community » Fixed

Sure lets do this. There can't really be any harm.

Committed and pushed 2fe6ae3dd2 to 10.1.x and efc9b3f479 to 10.0.x and 31113fd6d0 to 9.5.x. Thanks!

  • alexpott committed 2fe6ae3d on 10.1.x 
    Issue #3082032 by kristiaanvandeneynde: ToolbarController::...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

  • alexpott committed efc9b3f4 on 10.0.x 
    Issue #3082032 by kristiaanvandeneynde: ToolbarController::...

  • alexpott committed 31113fd6 on 9.5.x 
    Issue #3082032 by kristiaanvandeneynde: ToolbarController::...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Assigned: alexpott » Unassigned
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

What an excellent find! 😵

Status: Fixed » Closed (fixed)

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