While reviewing #3033653: InvalidArgumentException when adding reference field without Media type, I discovered MediaLibraryWidget has:

  protected function getAllowedMediaTypeIdsSorted() {
    // Get the configured media types from the field storage.
    $handler_settings = $this->getFieldSetting('handler_settings');
    $allowed_media_type_ids = !empty($handler_settings['target_bundles']) ? $handler_settings['target_bundles'] : [];

    // When no target bundles are configured for the field, all are allowed.
    if (!$allowed_media_type_ids) {
      $allowed_media_type_ids = $this->entityTypeManager->getStorage('media_type')->getQuery()->execute();
    }

While I'm quite in favor of allowing all eligible bundles if no particular target bundle has been configured, this seems like a major inconsistency with how all other Core widgets handle this.

Generically, this has already been raised in #2348615: Clean up entity reference field settings anomalies (OP remark 3). Might this even be a regression?

IMO, we should generally allow all eligible bundles in this case, yet advertising this behaviour on the bundle settings ("If no target bundle is selected, all will be allowed" or so).

As pointed out in #2, this has been an explicit design decision based on perfectly reasonable security considerations.
So the inconsistency between MediaLibraryWidget and all others must be solved the other way around: if no media type has been explicitly allowed as target bundle, none is allowed, as is the case with all other Core widgets.
As a security-related bug, this is major and needs to be backported to D8.6.

Comments

Pancho created an issue. See original summary.

amateescu’s picture

IMO, we should generally allow all eligible bundles in this case, yet advertising this behaviour on the bundle settings ("If no target bundle is selected, all will be allowed" or so).

That's not possible, this behavior was an explicit design decision when the ER field was introduced in core :) The exact problem was that you could configure you field initially to be able to reference a specific bundle, then if you remove that bundle from the system the field would automatically fall back to "all bundles", which is a security problem.

That's why the target_bundles selection handler setting has three states:

- NULL: all bundles are allowed
- []: no bundles allowed
- ['some_bundle']: only some_bundle is allowed

pancho’s picture

Title: Allow all eligible bundles if target bundles aren't configured » Don't allow all media types if no target bundle is configured
Component: entity_reference.module » media system
Category: Task » Bug report
Issue summary: View changes
Priority: Normal » Major
Issue tags: +Security

That's not possible, this behavior was an explicit design decision when the ER field was introduced in core. [...] if you remove that bundle from the system the field would automatically fall back to "all bundles", which is a security problem.

Thanks for pointing out this perfectly reasonable security implication. To achieve consistency and fix a security hole, we therefore need to go the other way around: bringing MediaLibraryWidget in line with the rest of Core.

pancho’s picture

Title: Don't allow all media types if no target bundle is configured » [PP-1] Don't allow all media types if no target bundle is configured
Status: Active » Postponed
StatusFileSize
new1 KB

Here's a first one-liner that basically fixes the security bug and to a certain extent works, but may only be properly tested on top of #3033653: InvalidArgumentException when adding reference field without Media type, therefore postponing on that one. Some edge-cases need further consideration, though, so it's just a starter.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

seanb’s picture

seanb’s picture

seanb’s picture

I have integrated a solution and tests into the latest patch for #3033653-31: InvalidArgumentException when adding reference field without Media type.

phenaproxima’s picture

Status: Postponed » Closed (duplicate)

In that case, let's close this as a duplicate.