Inaccessible Media entities still have rows generated for them in the Media Library view, containing form wrapper markup *and* the media label

Created on 21 June 2019, over 5 years ago
Updated 12 September 2023, over 1 year ago

Problem/Motivation

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

<!-- See https://drupal.org/core-mentoring/novice-tasks for tips on identifying novice tasks. Delete or add "Novice" from the Novice? column in the table below as appropriate. Uncomment tasks as the issue advances. Update the Complete? column to indicate when they are done, and maybe reference the comment number where they were done. -->

Problem/Motivation

While responding to a review in #3038254: Delegate media library access to the "thing" that opened the library β†’ , @Wim Leers discovered that forbidding access to media entities like this:

diff --git a/core/modules/media/src/MediaAccessControlHandler.php b/core/modules/media/src/MediaAccessControlHandler.php
index a8fdea2f63..b8ebb0181f 100644
--- a/core/modules/media/src/MediaAccessControlHandler.php
+++ b/core/modules/media/src/MediaAccessControlHandler.php
@@ -16,6 +16,7 @@ class MediaAccessControlHandler extends EntityAccessControlHandler {
    * {@inheritdoc}
    */
   protected function checkAccess(EntityInterface $entity, $operation, AccountInterface $account) {
+    return AccessResult::forbidden();
     if ($account->hasPermission('administer media')) {
       return AccessResult::allowed()->cachePerPermissions();
     }

results in a media library selection dialog looking like this (with a test media library that contains 2 images):

Unfortunately you can see in that screenshot that some per-row (per inaccessible media entity) HTML is generated that should not be generated:

<div class="media-library-item media-library-item--grid js-media-library-item js-click-to-select views-row"><div class="views-field views-field-rendered-entity"><span class="field-content media-library-item__content"></span></div><div class="views-field views-field-media-library-select-form js-click-to-select-checkbox"><span class="field-content"><div class="js-form-item form-item js-form-type-checkbox form-type-checkbox js-form-item-media-library-select-form-0 form-item-media-library-select-form-0 form-no-label">
      <label for="edit-media-library-select-form-0--MFsPcY2kHNU" class="visually-hidden">Select llamaaww.PNG</label>
        <input data-drupal-selector="edit-media-library-select-form-0" type="checkbox" id="edit-media-library-select-form-0--MFsPcY2kHNU" name="media_library_select_form[0]" value="4" class="form-checkbox">

        </div>
</span></div></div>

an "empty result" message ideally, or at least no markup for each inaccessible media entity (every row in a view).

  1. Form wrapper markup for every inaccessible media entity, hence disclosing the number of media items in the library. The actual security risk of this seems minimal at best β€” Drupal's sequential identifiers for content entity in general already reveals this on many sites. We don't consider this sensitive information.
  2. That markup also contains a Select @label string, which could be highly sensitive information.

Proposed resolution

  1. Security hardening: ensure that no markup is generated at all for any row containing an inaccessible Media entity.
  2. An "empty result set" message or behavior, that also works when the user is denied access to all existing Media entities.

And needless to say: tests for both.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Views now adds a ENTITY_TYPE_ID_access query tag to all its queries by default. This means that any implementations of hook_query_TAG_alter(), where TAG is of the pattern ENTITY_TYPE_ID_access -- for example, user_access if user accounts are being queried -- will now run on Views queries as well. Modules implementing such hooks should ensure that this change does not result in unwanted side effects.

πŸ› Bug report
Status

Fixed

Version

8.8 ⚰️

Component
MediaΒ  β†’

Last updated about 20 hours ago

Created by

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Live updates comments and jobs are added and updated live.
  • Security improvements

    It makes Drupal less vulnerable to abuse or misuse. Note, this is the preferred tag, though the Security tag has a large body of issues tagged to it. Do NOT publicly disclose security vulnerabilities; contact the security team instead. Anyone (whether security team or not) can apply this tag to security improvements that do not directly present a vulnerability e.g. hardening an API to add filtering to reduce a common mistake in contributed modules.

  • VDC

    Related to the Views in Drupal Core initiative.

Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

Production build 0.71.5 2024