This was originally reported a security issue and we came to an agreement with the Drupal Security Team to discuss/fix this in the public issue queue.
Problem/Motivation
Entity reference fields using \Drupal\Core\Entity\Plugin\EntityReferenceSelection\DefaultSelection and its subclasses (NodeSelection, UserSelection, etc.) do not properly respect entity access controls. Specifically, they bypass:
- entity->access('view label') checks
- entity->access('view') checks
This potentially allows users to see entity labels in entity reference fields they should not have access to.
Steps to reproduce
1. Add the following code to demonstrate the issue:
/**
* Implements hook_entity_access().
*/
function entity_ref_view_label_access_bypass_entity_access(\Drupal\Core\Entity\EntityInterface $entity, $operation, \Drupal\Core\Session\AccountInterface $account) {
if ($operation === 'view label') {
return \Drupal\Core\Access\AccessResult::forbidden();
}
return \Drupal\Core\Access\AccessResult::neutral();
}
2. Create an Article content or use the core/recipes/article_content_type recipe:
- Authored by field (user reference)
- Tags field (taxonomy term reference)
Current Behavior:
- Entity reference fields display entity labels even when access is forbidden
- Both 'view label' and 'view' access checks are bypassed
- hook_node_access() restrictions are not respected
Expected Behavior:
- Authored by field should not display usernames when access is forbidden
- Tags field should not display term references when access is forbidden
- Entity reference fields should respect both 'view label' and 'view' access controls
Comments
Comment #2
mxr576For historical reference, we had the following discussion in the security issue with mainly @berdir and @larowlan - please add your correction to my summary if necessary:
Originally, I have started the security thread with these three questions after opened #3500505: Fix filtering on Node's Authored by field (and on other entity reference fields):
hook_node_access()can override permissions granted byhook_node_access_grants()(Proof: https://git.drupalcode.org/project/view_usernames_node_author/-/blob/1.0.0/tests/src/Kernel/NodeAccessTest.php?ref_type=tags#L94-106)?TL;DR
\Drupal\user\Plugin\EntityReferenceSelection\UserSelection::buildEntityQuery()and\Drupal\node\Plugin\EntityReferenceSelection\NodeSelection::buildEntityQuery()$entity->access()performs access check on a single entity. When performed on a list of entities, it breaks pagination and could lead to performance issues.In addition: node/entity access is inconsistent. List and entity level access checking in Drupal core is complex, with several open issues addressing these concerns. Developers should be aware of how to use them properly.
Potential action items
During a security team triage meeting (@greggles, @mcdruid, @drumm, @larowlan), the following suggestions were made:
hook_entity_access()andhook_entity_access_alter()hook documentation, noting the gotchas around selection plugins.hook_entity_reference_selection_alter()can be used to modify the class for a given entity-type and add custom logic as needed.These documentation improvements would help developers, especially those new to Drupal, understand the potential pitfalls related to access controls.
My additional suggested documentation changes
Extend the documentation on
\Drupal\Core\Entity\EntityAccessControlHandler::$viewLabelOperationto properly explain that "view label" is a subset of the "view" operation. Clarify that even if it grants access to a field/property on an entity or sometimes a dynamically generated value (e.g.,$user->getDisplayName()), it is not equivalent to field access.Explain that when a user has "view" operation access to an entity, they also have "view label" access. However, when they only have "view label" access, they literally only have access to the entity label and nothing more. This also means that they may not have access to the entity's canonical URL (see The "Label" entity reference field formatter now restricts links for inaccessible destinations).
Comment #3
mxr576Comment #4
mxr576It was also concluded some type of access checking cannot be implemented as a query level access check, only as a single entity level access check which means the
$entity->access()calls remains a necessity even if they break pagination or may lead to performance issues.Comment #5
berdir> It was also concluded some type of access checking cannot be implemented as a query level access check, only as a single entity level access check which means the $entity->access() calls remains a necessity even if they break pagination or may lead to performance issues.
I don't agree with this. Core doesn't do it in hardcoded lists, views doesn't do it. Doing query access is expected to be sufficient. If that _were_ a necessity, then the fact that core doesn't do it would be a security issue.