EntityAccessControlHandler::createAccess() and EntityAccessControlHandler::access() return false positive cache hits because it ignores context

This is at least Major if not Critical

We had converged on consensus on the solution in #36. But #48 makes this point:

including the $context that's in createAccess() isn't a complete fix for this bug: A module might implement hook_entity_create_access() and return an access value that's based on something else. It would correctly add a cache tag for that dependency into the returned AccessResult, only to have that ignored by the static caching.

I think this is a valid concern.

It seems therefore that in order to fix the bug here, we should use MemoryCache.

I think I'm wrong above in #55.
I don't see a way to get information back from the cacheable metadata added by hooks about what createAccess() $context they used in their logic. Therefore we cannot handle context intelligently even with MemoryCache.
So we either have to serialize the whole context, which has performance concerns, or we just don't do static caching when there is context. Which is the approach in #36 we mostly all like.

This is valid regardless of whether we use MemoryCache or not in the future. $context could contain anything, and so we shouldn't automatically be stuffing it into MemoryCache either.

MemoryCache would solve the issue identified in #48, but it's not the right solution for this, so let's rule #48 out of scope here and move it to another issue.

The Needs Review Queue Bot → tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request → . Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

#46 is wrong to say "this affects access() as well as createAccess()." access() does not use $context. It's true that cacheability metadata of hooks and handler results in both access() and createAccess() is not considered. This is a bug, and the correct solution is a memory cache. But it's a different bug to this issue, and solving it does not necessarily solve the context bug discussed in this issue because context may contain items that are not cacheable dependencies. I created 📌 Use a memory cache in EntityAccessControlHandler Active to address this other bug.

The MR is based on #33/36, which @hctom in #37 and @claudiu.cristea in #44 supported. We cease to use the cache if a context is provided. As discussed in #36/44 this may have a performance impact on some custom code, but that's better than the present situation of a bug with possible security implications. As @hctom said in #37:

Everything is better than getting wrongly cached results that might even result in security issues!

Improving performance by caching even if context is present is a task for the follow-up: ✨ EntityAccessControlHandler::createAccess() should use whole context array when caching Active .

Left some comments on the MR.

This is a blocker (not sure if soft or hard) for contrib in 📌 Add 'create * host permissions' Postponed

This needs a different approach.
The access result can vary by all sorts of context ($context, dereved from it, and external ones). This is the role of cache contexts to be returned with $accessResult (maybe from a hook_access_alter). These cache contexts can not be known in advance.
Sounds familiar? Yes, this is exactly what VariationCache is for.

This is a bug. And one with security implications. So in this issue probably we should fix it in the quickest, simplest way we can.

Creating a good modern-Drupal caching system for context is the subject of 📌 Use a memory cache in EntityAccessControlHandler Active . I'd be grateful if you could share the idea of using VariationCache there.

What things are passed in $context anyway?

We don't know, by design. The API is intentionally agnostic. That's a part of the challenge.

Thanks for the reply. You are right, mostly.

But what about this: the $context (and things derived from it) CAN be used in hook_entity_create_access_alter, with no way to broadcast that fact back to the AccessControlHandler.

???

But what about this: the $context (and things derived from it) CAN be used in hook_entity_create_access_alter, with no way to broadcast that fact back to the AccessControlHandler. (And it may add the need for additional caching contexts / $cid additions.)

Yes, hooks can use anything provided as by context and the handler doesn't know what they used. So everything in context needs to be considered when addressing caching, even if its not used by the handler. Things derived from context don't matter, they should be handled by caching context, and its up to hook implementations to add cacheable metadata themselves if they do anything more exotic.

But none of this matters in this issue. We already have a context. And we have caching. And caching ignores context, leading to false postives which are a bad thing, and so we should not use our current caching if we have context because no caching is much better than bad caching. And we should do this now, because its broken now and easy to fix, whereas making cool ways to cache context for edge cases is hard.

So it's up to the AccesControlHandler to add all "possible" $context variations to $cid. And if other things are passed in $context and used in an access hook, that is not supported (which may go to the docs...).

It's not my intent to bloat this, just to clarify.
Thanks a lot for pushing this forward!

Did a code review of the MR.

The factored out ::buildCreateAccessCid lgtm, it does what it announces, and defaulting complex cases to uncacheable seems reasonable.
Test coverage looks reasonable, though i did not think about each case. (Only found an Übernit in the code.)
Setting RTBC. You should revert though that if you think that more thorough test review is needed.

I read the IS and the comments. I didn't find any unanswered questions or other work to do. Reading the MR I found some coding standards to fix and I removed an unused variable from the test. These are minor, and I doubled checked, so I am leaving at RTBC, also updated credit a bit.

I think the approach of skipping caching if $context is non-standard is a good one. If someone runs into a problem with that, they might come up with a performant way to cache with arbitrary context.

Committed/pushed to 11.x, 11.1.x, and 10.5.x, thanks!

catch → closed merge request !9842

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