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.
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | only_allowed_media_types_3038245-4.patch | 1 KB | pancho |
Comments
Comment #2
amateescu commentedThat'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_bundlesselection handler setting has three states:- NULL: all bundles are allowed
- []: no bundles allowed
- ['some_bundle']: only
some_bundleis allowedComment #3
panchoThanks 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.
Comment #4
panchoHere'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.
Comment #6
seanbAdded this to the must-haves in #2834729: [META] Roadmap to stabilize Media Library
Comment #7
seanbComment #8
seanbI have integrated a solution and tests into the latest patch for #3033653-31: InvalidArgumentException when adding reference field without Media type.
Comment #9
phenaproximaIn that case, let's close this as a duplicate.