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
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. |
Comment | File | Size | Author |
---|---|---|---|
#59 | blockbase_subclasses-2541344-59.patch | 4.48 KB | Wim Leers |
Comments
Comment #1
Wim LeersComment #2
Berdirwhat happened to user.roles exactly?
Comment #3
Wim LeersI 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.Comment #4
legolasbo+++ 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.
Comment #6
Wim LeersTwo failures in a recently-changed test:
NodeTranslationUITest
. This patch causes the'route'
cache context to be added, which means it implies theroute.menu_active_trails:*
cache contexts, which means they're optimized away. The assertion's expectations simply need to be updated.Comment #7
joshi.rohit100Just re-rolled.
Comment #9
Wim Leers@joshi.rohit100: see #6 for how to fix those two last failures.
Comment #10
borisson_Attached patch should resolve the 2 remaining failures.
Comment #11
Wim LeersI'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.
Comment #13
legolasboI'll fix the tests if we don't have a green patch by Thursday. No time until then.
Comment #14
borisson_Comment #15
legolasboSeems good to me now.
Comment #16
dawehnerIt 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?Comment #17
catch#16 is a good question.
Comment #18
Wim LeersWe have 75 occurrences of
parent::buildForm()
. We haveparent::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.
Comment #19
dawehnerSo? Just because we are doing something wrong does not mean we should not fix that. Sane > consistency.
Comment #20
catchThe 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.
Comment #21
BerdirWe 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 :)
nitpick: ;;
Comment #22
Wim Leers#21++
Comment #23
dietr_ch CreditAttribution: dietr_ch commentedComment #24
dietr_ch CreditAttribution: dietr_ch as a volunteer commentedRe-roll patch #14.
Comment #28
dietr_ch CreditAttribution: dietr_ch as a volunteer commentedFixing the re-rolled and failed patch by adding
url.path
andurl.query_args:_wrapper_format
to$defaultCacheContexts
inDrupal\node\Tests\NodeTranslationUITest
. I'm not sure whether this is correct but it makes the tests green again.Comment #30
XanoComment #32
Wim LeersDamn, 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
and because I'm not actually allowed to do that, also .Comment #33
Wim LeersStraight reroll, rebased by git.
Comment #36
Wim LeersComment #39
Wim LeersPIFR=drunk.
Comment #40
Wim LeersAdding beta evaluation capturing impact vs disruption, extrapolated from #32.
Comment #41
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedRTBC - a good one to get in to set the right example.
Comment #42
catchThis 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.
Comment #43
alexpottNeeds 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.
Comment #44
alvar0hurtad0Im doing it
Comment #45
alvar0hurtad0Patch rerolled
Comment #46
alexpottPutting into the triage queue
Comment #48
GoZ CreditAttribution: GoZ at Centarro commented@alvar0hurtad0 Can you post interdiff ?
Comment #49
Wim Leers@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.
Comment #50
tim.plunkettMissing use statement in the forum block, double semicolon in the help block. Scanning the fail list, I think that should cover it.
Comment #51
Wim LeersThanks Tim <3
Back to RTBC then!
Comment #53
effulgentsia CreditAttribution: effulgentsia at Acquia commentedDiscussed 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:
The removal of 'user.roles'.
The change from 'url' to 'route'.
Comment #54
Wim LeersBookNavigationBlock
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 hasmax-age=0
explicitly anyway, so it will never be cached. This does not change that. Hence this cannot possibly create any security problems.url
is wrong,route
is correct, becauseHelpBlock
doesBoth 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.
Comment #56
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI 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.
Comment #57
Wim LeersAlright. Split them off:
Removed those parts. Back to RTBC.
Comment #58
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThanks. Can we also add LocalActionsBlock to this patch? That's the only missing one I could find via PhpStorm.
Comment #59
Wim LeersGood catch. That one was added after this issue started :)
Comment #61
Wim Leersd.o is drunk. Test is green.
Comment #62
alexpottI'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!
Comment #64
effulgentsia CreditAttribution: effulgentsia at Acquia commentedIn 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?
Comment #65
Wim LeersYay, 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.
Comment #66
effulgentsia CreditAttribution: effulgentsia at Acquia commentedAh, 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.
Comment #67
legolasboEven 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).