Problem/Motivation

Since #2375695: Condition plugins should provide cache contexts AND cacheability metadata needs to be exposed, ContextAwarePluginBase, which is the parent class for BlockBase, has implementations of getCache(Contexts|Tags|MaxAge)(). It returns the cacheability metadata of the contexts consumed by a block.

But, most existing BlockBase subclasses are just returning their cacheability metadata, rather than extending that of the parent, and hence the cacheability metadata of the contexts is being lost.

Proposed resolution

Fix the broken implementations.

Remaining tasks

Review.

User interface changes

None.

API changes

None.

Data model changes

None.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because wrong/incomplete cacheability metadata can lead to security problems (information disclosure due to missing cache contexts). And it's important that core sets the right example for module developers in contrib writing custom blocks by looking at the ones in core, and possibly copy/pasting code to start from.
Issue priority Major because does not change APIs, so does not need to block RC.
Prioritized changes The main goal of this issue is security/cacheability.
Disruption Zero disruption.
CommentFileSizeAuthor
#59 interdiff.txt932 bytesWim Leers
#59 blockbase_subclasses-2541344-59.patch4.48 KBWim Leers
#57 interdiff.txt2.94 KBWim Leers
#57 blockbase_subclasses-2541344-57.patch3.69 KBWim Leers
#50 interdiff.txt1 KBtim.plunkett
#50 blockbase_subclasses-2541344-50.patch5.05 KBtim.plunkett
#45 blockbase_subclasses-2541344-45.patch4.83 KBalvar0hurtad0
#36 interdiff.txt1015 bytesWim Leers
#36 block_context_cacheability_lost-2541344-36.patch4.83 KBWim Leers
#33 block_context_cacheability_lost-2541344-33.patch4.53 KBWim Leers
#30 drupal_2541344_30.patch4.8 KBXano
#30 interdiff.txt604 bytesXano
#28 interdiff-2541344-24-28.txt479 bytesdietr_ch
#28 2541344-blockbase_subclasses-28.patch4.21 KBdietr_ch
#24 2541344-blockbase_subclasses-24.patch4.15 KBdietr_ch
#14 blockbase_subclasses-2541344-14.patch4.14 KBborisson_
#14 interdiff.txt435 bytesborisson_
#10 interdiff.txt662 bytesborisson_
#10 blockbase_subclasses-2541344-10.patch4.13 KBborisson_
#7 2541344-7.patch3.48 KBjoshi.rohit100
#1 block_context_cacheability_lost-2541344-1.patch3.83 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Berdir’s picture

+++ b/core/modules/book/src/Plugin/Block/BookNavigationBlock.php
@@ -182,12 +183,7 @@ public function build() {
    */
   public function getCacheContexts() {
-    // The "Book navigation" block must be cached per role and book navigation
-    // context.
-    return [
-      'user.roles',
-      'route.book_navigation',
-    ];
+    return Cache::mergeContexts(parent::getCacheContexts(), ['route.book_navigation']);
   }

what happened to user.roles exactly?

Wim Leers’s picture

I removed it since I was touching those lines anyway; the user.roles bit is completely wrong. The overall cacheability bugs in the book navigation block are being fixed in #2483181: Make book navigation block cacheable.

legolasbo’s picture

Status: Needs review » Reviewed & tested by the community

+++ b/core/modules/book/src/Plugin/Block/BookNavigationBlock.php
@@ -182,12 +183,7 @@ public function build() {
- // The "Book navigation" block must be cached per role and book navigation
- // context.
- return [
- 'user.roles',
- 'route.book_navigation',
- ];

Could be considered scope creep, but given the simplicity of the patch I don't think that should be a problem. Other than this I can't find anything wrong with the patch.

Thank you.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1: block_context_cacheability_lost-2541344-1.patch, failed testing.

Wim Leers’s picture

Issue tags: +Novice, +php-novice, +Needs reroll

Two failures in a recently-changed test: NodeTranslationUITest. This patch causes the 'route' cache context to be added, which means it implies the route.menu_active_trails:* cache contexts, which means they're optimized away. The assertion's expectations simply need to be updated.

joshi.rohit100’s picture

Status: Needs work » Needs review
FileSize
3.48 KB

Just re-rolled.

Status: Needs review » Needs work

The last submitted patch, 7: 2541344-7.patch, failed testing.

Wim Leers’s picture

@joshi.rohit100: see #6 for how to fix those two last failures.

borisson_’s picture

Status: Needs work » Needs review
FileSize
4.13 KB
662 bytes

Attached patch should resolve the 2 remaining failures.

Wim Leers’s picture

Issue tags: -Needs reroll

I'd RTBC, but 90% of the patch is from my initial patch in #1.

I think we need e.g. Berdir to review/RTBC this.

Status: Needs review » Needs work

The last submitted patch, 10: blockbase_subclasses-2541344-10.patch, failed testing.

legolasbo’s picture

I'll fix the tests if we don't have a green patch by Thursday. No time until then.

borisson_’s picture

legolasbo’s picture

Status: Needs review » Reviewed & tested by the community

Seems good to me now.

dawehner’s picture

It is quite easy to make errors. Are we sure we cannot refactor the code to have a doGetCacheTags() or something like that so you as a plugin developer doesn't necessarily have to care about it?

catch’s picture

Status: Reviewed & tested by the community » Needs review

#16 is a good question.

Wim Leers’s picture

We have 75 occurrences of parent::buildForm(). We have parent::setUp(), parent::defineOptions(), parent::optionsSummary(), parent::buildOptionsForm(), parent::postSave(), parent::postDelete(), and probably many dozens more.

So, I don't see what's new here. It's easy to get wrong in those places too.

dawehner’s picture

So? Just because we are doing something wrong does not mean we should not fix that. Sane > consistency.

catch’s picture

The examples in #18 are just calling out to the parent which is extremely standard when extending a class.

With this you have to merge as well as call out to the parent, seems a bit different to me.

Berdir’s picture

We actually do have a pattern of protected block* methods in BlockBase for exactly that purpose but I don't like most of those because they IMHO add more confusion than anything else (e.g. blockForm() vs. buildConfigurationForm(), which is now the one you're supposed to implement?)

And the block* method name prefix is also strange :)

+++ b/core/modules/help/src/Plugin/Block/HelpBlock.php
@@ -120,9 +121,7 @@ public function build() {
+    return Cache::mergeContexts(parent::getCacheContexts(), ['route']);;

nitpick: ;;

Wim Leers’s picture

#21++

dietr_ch’s picture

Assigned: Unassigned » dietr_ch
dietr_ch’s picture

Status: Needs review » Needs work

The last submitted patch, 24: 2541344-blockbase_subclasses-24.patch, failed testing.

The last submitted patch, 14: blockbase_subclasses-2541344-14.patch, failed testing.

dietr_ch’s picture

Status: Needs work » Needs review
FileSize
4.21 KB
479 bytes

Fixing the re-rolled and failed patch by adding url.path and url.query_args:_wrapper_format to $defaultCacheContexts in Drupal\node\Tests\NodeTranslationUITest. I'm not sure whether this is correct but it makes the tests green again.

Status: Needs review » Needs work

The last submitted patch, 28: 2541344-blockbase_subclasses-28.patch, failed testing.

Xano’s picture

Status: Needs review » Needs work

The last submitted patch, 30: drupal_2541344_30.patch, failed testing.

Wim Leers’s picture

Issue tags: +Security improvements, +DX (Developer Experience), +rc target, +Needs committer feedback

Damn, completely lost track of this one. Re-testing.

This is important because it could lead to security problems otherwise (missing cache contexts). And it's important that core sets the right example. So tentatively tagging rc target and because I'm not actually allowed to do that, also Needs committer feedback.

Wim Leers’s picture

Assigned: dietr_ch » Unassigned
Status: Needs work » Needs review
FileSize
4.53 KB

Straight reroll, rebased by git.

Status: Needs review » Needs work

The last submitted patch, 33: block_context_cacheability_lost-2541344-33.patch, failed testing.

The last submitted patch, 33: block_context_cacheability_lost-2541344-33.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
4.83 KB
1015 bytes

Status: Needs review » Needs work

The last submitted patch, 36: block_context_cacheability_lost-2541344-36.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review

PIFR=drunk.

Wim Leers’s picture

Issue summary: View changes

Adding beta evaluation capturing impact vs disruption, extrapolated from #32.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC - a good one to get in to set the right example.

catch’s picture

Issue tags: -Needs committer feedback

This looks definitely fine for a patch release, and possibly fine during RC based on it being an entirely internal change to block module and security hardening against information disclosure. Still figuring exactly what we do and don't commit during RC (depends how it's all going probably), but untagging needs committer feedback for now.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a reroll.

I'm +1 on this being an rc target because I think block cacheability needs to set a good example for contrib and consequences of getting this wrong now that we are caching stuff for authenticated users are bad.

alvar0hurtad0’s picture

Assigned: Unassigned » alvar0hurtad0

Im doing it

alvar0hurtad0’s picture

Assigned: alvar0hurtad0 » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
4.83 KB

Patch rerolled

alexpott’s picture

Issue tags: -rc target +rc target triage

Putting into the triage queue

Status: Needs review » Needs work

The last submitted patch, 45: blockbase_subclasses-2541344-45.patch, failed testing.

GoZ’s picture

@alvar0hurtad0 Can you post interdiff ?

Wim Leers’s picture

Issue tags: -Novice, -php-novice

@GoZ No interdiff because it was a straight reroll. I manually verified that the patch is functionally equivalent with that in #36. Only the context is different.

But now there are test failures. So we'll need to investigate that.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
5.05 KB
1 KB

Missing use statement in the forum block, double semicolon in the help block. Scanning the fail list, I think that should cover it.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Thanks Tim <3

Back to RTBC then!

The last submitted patch, 45: blockbase_subclasses-2541344-45.patch, failed testing.

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -rc target triage +rc target

Discussed with @alexpott and yes to making this issue's title an RC target, but the following should be separate follow-ups so that they can be evaluated for RC inclusion separately:

  1. +++ b/core/modules/book/src/Plugin/Block/BookNavigationBlock.php
    @@ -182,12 +183,7 @@ public function build() {
    -    return [
    -      'user.roles',
    -      'route.book_navigation',
    -    ];
    +    return Cache::mergeContexts(parent::getCacheContexts(), ['route.book_navigation']);
    

    The removal of 'user.roles'.

  2. +++ b/core/modules/help/src/Plugin/Block/HelpBlock.php
    @@ -118,9 +119,7 @@ public function build() {
    -    // The "Help" block must be cached per URL: help is defined for a
    -    // given path, and does not come with any access restrictions.
    -    return array('url');
    +    return Cache::mergeContexts(parent::getCacheContexts(), ['route']);
    

    The change from 'url' to 'route'.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
  1. Nowhere in BookNavigationBlock do we do anything with roles, the current user, or even permissions. This is just a remnant from long ago that is incorrect. We're working on making this block cacheable in #2483181: Make book navigation block cacheable. Note that this block has max-age=0 explicitly anyway, so it will never be cached. This does not change that. Hence this cannot possibly create any security problems.
  2. url is wrong, route is correct, because HelpBlock does
    $help = $this->moduleHandler->invokeAll('help', array($this->routeMatch->getRouteName(), $this->routeMatch));
    

Both of these are plain riskfree bugfixes that IMO make sense to fix here because we're touch those lines anyway.

Hopefully this convinces committers. Back to RTBC with that. If committers still feel those two things should be in separate issues, I'll move them out.

The last submitted patch, 45: blockbase_subclasses-2541344-45.patch, failed testing.

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs work

I do still think they should be moved to separate issues. IMO, that we're touching those lines anyway doesn't matter. Fixing those bugs has nothing to do with this issue's title.

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs work

Thanks. Can we also add LocalActionsBlock to this patch? That's the only missing one I could find via PhpStorm.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
4.48 KB
932 bytes

Good catch. That one was added after this issue started :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 59: blockbase_subclasses-2541344-59.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

d.o is drunk. Test is green.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

I'm pretty sympathetic to @dawehner's idea in #16 but that is probably too big a change in RC. I definitely think we should consider the pattern suggested by #16 more often.

Committed b315084 and pushed to 8.0.x. Thanks!

  • alexpott committed b315084 on 8.0.x
    Issue #2541344 by Wim Leers, borisson_, DietrichM, Xano, tim.plunkett,...
effulgentsia’s picture

In light of #62, what about a follow-up to add test coverage? Something like for each block, add a test context/condition, and make sure the corresponding cache context and tag is returned?

Wim Leers’s picture

Yay, finally, that took more effort than expected :)

Unpostponed #2604092: BookNavigationBlock has the 'user.roles' cache context, is wrong and #2604098: HelpBlock has the 'url' cache context instead of 'route'.

#64: that's just busywork. It's asserting that a hardcoded string is indeed hardcoded.

effulgentsia’s picture

Category: Bug report » Task

that's just busywork. It's asserting that a hardcoded string is indeed hardcoded.

Ah, I was getting confused between block visibility conditions and block contexts. The cache contexts for block visibility conditions are managed separately by BlockAccessControlHandler::checkAccess(). Which means the patch committed here is not for that, but only for block contexts added via setContext() and that the block uses via getContext(). And none of these blocks have such contexts, so the patch committed here was I guess just to set a good example for contrib, not to actually fix anything. Therefore, recategorizing this as a task. If I'm wrong about this, then please reset to "bug report", at which point, we should also figure out what test to write to protect against that bug reappearing.

legolasbo’s picture

Even though adding test coverage can be considered busywork, we should actually check if every block plugin provides correct cacheability metadata. In this issue we've only made sure that blocks implementing getCache(Contexts|Tags|MaxAge)() provide correct cacheability metadata. It is however unknown if those Classes that don't implement these methods provide correct cacheability metadata.

#2596649: Exposed form does not save state when it is placed in a block and #2605038: Views Exposed Filters should provide more specific cache contexts. for example provide no/incorrect cache contexts and I believe the views exposed filter block should also provide cachetags (which it doesn't at the moment).

Status: Fixed » Closed (fixed)

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