Problem/Motivation

If you uninstall a module that provides filters to text formats, Drupal removes the text formats that used the filters, but should not.

Steps to Reproduce the Bug

  1. Enable a module that provides a text filter plugin.
  2. Enable the filter on a text format.
  3. Uninstall the module.

You will find that the text format has been removed.

Proposed resolution

  • Prevent uninstallation of modules that provide filter plugins that are enabled in text formats.
  • Implement FilterFormat::onDependencyRemoval(). Remove disabled filters from the text formats when called.

Remaining tasks

  • Code a solution.
  • Write tests.

User interface changes

None.

API changes

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Manually test the patch Novice Instructions
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards Instructions
Files: 
CommentFileSizeAuthor
#50 cant-enable-fitler-module-2348925-50.patch794 bytesGarrett Albright
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,430 pass(es), 3 fail(s), and 30,810 exception(s). View
#46 2348925.46.patch8.25 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,741 pass(es). View
#46 41-46-interdiff.txt2.56 KBalexpott
#41 2348925.40.patch7.99 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,718 pass(es). View
#40 34-40-interdiff.txt669 bytesalexpott
#34 2348925.34.patch7.98 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,060 pass(es). View
#34 17-34-interdiff.txt6.35 KBalexpott
#27 2348925.17.patch3.89 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,151 pass(es). View
#24 uninstalling_a_filter-2348925-24.patch1006 bytescilefen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,163 pass(es), 20 fail(s), and 0 exception(s). View
#23 uninstalling_a_filter-2348925-23.patch946 bytescilefen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,998 pass(es), 86 fail(s), and 1 exception(s). View
#17 2348925.17.patch3.89 KBWim Leers
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,038 pass(es). View

Comments

cilefen’s picture

Title: Filter plugin removes text formats » Uninstalling a filter plugin removes text formats
Issue summary: View changes
Related issues: +#2260457: Allow config entities to remove dependent configuration keys when dependencies are deleted due to module uninstall
cilefen’s picture

Drupal warns you:

Berdir’s picture

Priority: Normal » Major

We have an API for this now, see ConfigEntityInterface::onDependencyRemoval() and existing implemetations for it, most notably in EntityDisplayBase.

Fully agreed that this is a major if not critical bug, it should actually never be possible to delete a filter format.

alexpott’s picture

Priority: Major » Critical
Issue tags: +Configuration system, +Configurables

Definitely critical. Nice find.

cilefen’s picture

Status: Active » Needs review
FileSize
839 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,028 pass(es). View

This is fine except it doesn't work. ;-)

ashish_nirmohi’s picture

Issue summary: View changes
Berdir’s picture

Does not work how, does it still remove the filter?

Filter format uses EntityWithPluginCollectionInterface, so the dependencies are calculated based on getPluginCollections() -> filters(), which has a cache. I'm not sure if we re-load the objects or not.

I also don't know if those methods even care about the status?

cilefen’s picture

FileSize
1.46 KB
1.43 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,062 pass(es). View

This still does not work. removeFilter() is called in this situation but the code to remove the filter is wrong.

cilefen’s picture

Issue tags: +Needs tests
cilefen’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Novice
cilefen’s picture

Status: Needs work » Needs review
FileSize
1.56 KB
2.66 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,039 pass(es). View

I added a test that should not pass because the patch doesn't work. But, it passes

alexpott’s picture

FileSize
2.29 KB
1.46 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,939 pass(es), 1 fail(s), and 0 exception(s). View
3.65 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,043 pass(es). View

So the KernelTestBase::uninstallModule() does not actually fire module uninstall code - it is a special flower.
Also filter formats are a special flower too since the filter plugin collection will always return all available filter plugins since disabled configuration is preserved - unless it matches the default state - see \Drupal\filter\FilterPluginCollection::getConfiguration().

The last submitted patch, 12: 2348925.12.test-only.patch, failed testing.

cilefen’s picture

@alexpott Thank you. Manual testing with #12 works for me. +1 for RTBC

Wim Leers’s picture

Status: Needs review » Needs work

WOW!!!!!!

Definitely critical. Nice find.

+1

  1. +++ b/core/modules/filter/src/Tests/FilterAPITest.php
    @@ -386,4 +386,27 @@ public function assertFilterFormatViolation(ConstraintViolationListInterface $vi
    +    $filter_format->setFilterConfig('filter_test_restrict_tags_and_attributes', $filter_config);
    +    $filter_format->save();
    

    Can be chained?

  2. +++ b/core/modules/filter/src/Tests/FilterAPITest.php
    @@ -386,4 +386,27 @@ public function assertFilterFormatViolation(ConstraintViolationListInterface $vi
    +    \Drupal::entityManager()->getStorage('filter_format')->resetCache();
    

    I'd think that this would happen automatically, since uninstalling modules should invalidate the extension cache tag. But I'm guessing this is for the static cache, which doesn't use cache tags?

  3. +++ b/core/modules/filter/src/Tests/FilterAPITest.php
    @@ -386,4 +386,27 @@ public function assertFilterFormatViolation(ConstraintViolationListInterface $vi
    +  }
     }
    

    Missing trailing newline.

alexpott’s picture

Status: Needs work » Needs review
FileSize
971 bytes
3.63 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,038 pass(es). View

Tnaks for the review @Wim Leers - re #15

  1. Fixed
  2. Yes it is for the static cache - we're in a KernelTestBase test too
  3. Fixed
Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests
FileSize
3.89 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,038 pass(es). View
1.75 KB

Adding comments to clarify further.

cilefen’s picture

Issue summary: View changes
tstoeckler’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +security

Sorry, if I'm missing something, but at the very least this was not discussed here, so...

I don't see how this is even remotely viable. The specific configuration and order of filters in a text format is highly security relevant and it is in no case whatsoever safe to just willy-nilly delete some of them without explicit user confirmation.

If you uninstall PHP filter module for example, you will potentially reveal highly sensitive information to the world.

Surely the current solution of deleting the formats isn't good, but at least it hides the text wholesale, because check_markup() can't find the format. This patch makes things dramatically worse.

The correct solution is to disallow uninstalling any module which provides a filter to an existing text format.

Wim Leers’s picture

The correct solution is to disallow uninstalling any module which provides a filter to an existing text format.

This was my first thought also! :)

But consider this: you just installed a text filter. You gave it a try. You dislike it. You want to remove it. With what you propose, you can't.

Why is this warning not sufficient:

?

tstoeckler’s picture

That is not sufficient because quite simply we are putting the site in a potentially insecure state. And the user might be aware of that but she or he might not. A warning like that is nothing more than an excuse, when it comes to security. And it's a poor one too, because it is not at all explicit. Maybe I am completely aware of the security implications but I think that Drupal will simply delete all content which uses the given format, so I just go ahead. Who knows what

updated if possible, or deleted

means?!

alexpott’s picture

So I think @tstoeckler is correct. We need to implement hook_system_info_alter in the filter module to not allow a module to be uninstalled if it provides a filter format plugin that is enabled in any filter format configuration entity.

cilefen’s picture

Status: Needs work » Needs review
FileSize
946 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,998 pass(es), 86 fail(s), and 1 exception(s). View

This is not quite it (it makes all filter-providing modules required), but it's a start.

cilefen’s picture

FileSize
1006 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,163 pass(es), 20 fail(s), and 0 exception(s). View

The last submitted patch, 23: uninstalling_a_filter-2348925-23.patch, failed testing.

alexpott’s picture

Hang with #23 and #24. I was wrong. #21 now thinking about it - surely this is an existing issue. In Drupal 7 you can uninstall a module that provides the equivalent of a filter format plugin so this is an existing issue. I think the patch in #17 is good to go. We can fix this issue in the old issue created for it #562694: Text formats should throw an error and refuse to process if a plugin goes missing (which I have a patch for).

alexpott’s picture

FileSize
3.89 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,151 pass(es). View

Uploading the patch in #17 again to make it the latest and bring the issue back to being in scope.

The last submitted patch, 24: uninstalling_a_filter-2348925-24.patch, failed testing.

tstoeckler’s picture

Re #26: Ok, I agree that that issue is the one we need to fix properly. I still agree with this going in in the meantime, though. As stated above, the current behavior of deleting text formats is pretty bad, too, but at least it makes sure that text never gets filtered incorrectly, because check_markup() bails out in that case. I don't see why we should be purposely making things worse and introducing a known Security flaw just because we have another issue to fix that properly.

cilefen’s picture

@tstoeckler This is fixing a regression. Without #27, you have a broken site. #27 is making things better, not worse. Let's focus attention on #562694: Text formats should throw an error and refuse to process if a plugin goes missing after this goes in.

tstoeckler’s picture

@cilefen: Well it might be broken, but at least it is not insecure. In that tradeoff we should always choose security and I would consider it a terrible decision to commit this without #562694: Text formats should throw an error and refuse to process if a plugin goes missing but @alexpott is right that D7 suffers from similar behavior, so... In any case it's not up to me :-)

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

This patch does not make anything less secure. Also I think we're confusing filter plugins with filter formats. Filter plugins are provided be modules and can be configured to be part of a filter format. HEAD's current behaviour is totally ridiculous. If a module provides a filter plugin all filter formats will depend it on. When that module is uninstalled ALL filter formats will be deleted. This fixes that.

Separately we should address the issue that if a module provides a filter plugin and it is used in any filter format then we need to prevent that module being uninstalled. That is exactly what #562694: Text formats should throw an error and refuse to process if a plugin goes missing is (I've made this critical). I've made critical and will have a patch the moment this one is committed. There is a separate issue around what occurs when a whole filter format is removed and that WORKS - a missing filter format results in no output.

Given @tstoeckler says:

I still agree with this going in in the meantime, though

I'm re-rtbcing.

tstoeckler’s picture

Sorry, that was a really weird typo. I meant disagree.

And just want to clarify that this does make things less secure as explained above.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
6.35 KB
7.98 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,060 pass(es). View

Okey-dokey I'm not going to hold this up anymore especially since I've already done the work. Here is a patch that prevents uninstalling a module that provides a filter plugin that is currently used in any filter format configuration entity.

catch’s picture

fwiw I completely agree with tstoeckler here.

One case where we could delete the filter format entirely is when it's not used at all in any content, but that doesn't really seem compatible with the hook_system_info_alter() approach.

catch’s picture

chx’s picture

I really don't like all these issues being hanged from hook_system_info_alter ; it will make any rebuild quite slow and brittle . We are working on an uninstall in #2278017: When a content entity type providing module is uninstalled, the entities are not fully deleted, leaving broken reference

catch’s picture

If we can add a validation step to module uninstall and present the information then that would save doing it in hook_system_info_alter() - is worth a side issue to add that. I don't want to hold any of these issues up on that though - we can convert later using the same information and it'll be an API addition rather than change.

We'll need to make sure at least drush also respects this though (as it does required modules).

alexpott’s picture

ModuleHandler::uninstall() doesn't check if a module is required - if it did and we implement this extra step there then Drush would haven't to do anything too special. +1 to not holding this up.

alexpott’s picture

FileSize
669 bytes

Patch fixes test for new module_installer service.

alexpott’s picture

FileSize
7.99 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,718 pass(es). View

Now for the patch

alexpott’s picture

Issue tags: +D8 upgrade path

Tagging with the upgrade path as the current HEAD could get a site into an inconsistent state.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed the patch, tested it manually, stepped through the test coverage, tried to remove parts that I didn't understand at first sight/that didn't seem to be doing anything, hence gaining a full understanding. I couldn't break it. RTBC.

  1. +++ b/core/modules/filter/filter.module
    @@ -83,6 +84,38 @@ function filter_theme() {
    +  if ($type == 'module' && !defined('MAINTENANCE_MODE')) {
    

    Theoretically we'd want to exclude the filter module from getting this treatment, because that'd imply a nonsensical recursive dependency. However, it also has real-world — though rare — repercussions: it makes it impossible to uninstall the Filter module as long as any text format (FilterFormat) config entity is
    still using any filter plugin. To uninstall the Filter module, you'd have to disable every filter plugin on every text format.

    I'd almost say that this is a feature: it makes it exceedingly annoying/painful to remove the Filter system, and that's a good thing :)

  2. +++ b/core/modules/filter/src/Entity/FilterFormat.php
    @@ -387,4 +388,43 @@ public function getHtmlRestrictions() {
    +      // Only remove disabled filters.
    

    Nitpick: clearer comment would be "Remove disabled filters, so that this FilterFormat config entity can continue to exist."

tstoeckler’s picture

Awesome, thanks @alexpott for pushing this. This is a great solution to a non-trivial problem!

catch’s picture

Status: Reviewed & tested by the community » Needs review

I think the issue has diverged from the summary a bit, from the patch we seem to be making two changes:

1. Mark modules that provide plugins required if the filter plugin is used in a filter format.

2. Remove filter plugins from formats if the plugin provider is uninstalled, but only if the filter plugin is disabled.

I'm confused why if we do #1 how we'd ever get to #2 though?

Minor nits below:

  1. +++ b/core/modules/filter/filter.module
    @@ -83,6 +84,38 @@ function filter_theme() {
    +      foreach(filter_formats() as $filter_format) {
    +        foreach($filter_plugins as $filter_plugin) {
    

    Minor and can fix on commit but missing space after foreach x 2.

  2. +++ b/core/modules/filter/src/Tests/FilterAPITest.php
    @@ -386,4 +387,64 @@ public function assertFilterFormatViolation(ConstraintViolationListInterface $vi
    +   */
    

    Not sure this comment describes what the test does enough.

    What about:

    "Ensure that modules providing filter plugins are required when the plugin is in use, and that only disabled plugins are removed from format configuration entities rather than the configuration entities being deleted."

    Although if that's the correct description I'm still not sure if the behaviour is right.

alexpott’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
2.56 KB
8.25 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,741 pass(es). View

Updated issue summary.

I'm confused why if we do #1 how we'd ever get to #2 though?

This is because disabled filters are still stored in text formats if they have different settings from their defaults.

re #43

  1. Indeed filter module will be exceptionally hard to uninstall
  2. Fixed

re #45

  1. Fixed
  2. Fixed

Tentatively setting back to rtbc since the changes are very small.

  • catch committed 8b6c343 on 8.0.x
    Issue #2348925 by alexpott, cilefen, Wim Leers: Uninstalling a filter...
catch’s picture

Status: Reviewed & tested by the community » Fixed

OK that makes sense. Not storing anything for disabled filters is I think for #2312379: Bring filter format plugin configuration into line with other systems, I'd forgotten we'd not resolved that completely yet.

Garrett Albright’s picture

Title: Uninstalling a filter plugin removes text formats » REGRESSION: Uninstalling a filter plugin removes text formats
Priority: Critical » Major
Status: Fixed » Needs work

Commit 8b6c343 has made it so that one gets a PluginNotFoundException when enabling a module which provides a filter. Try the D8 branch of Pathologic, for example. (Despite the exception when the module is enabled, if you later go to the text format configuration form, the filter is available…)

I'm trying to look into it to discover why, but might not succeed before my bedtime.

Slate ~/Sites/d8.test/www> drush en pathologic -y
The following extensions will be enabled: pathologic
Do you really want to continue? (y/n): y
exception 'Drupal\Component\Plugin\Exception\PluginNotFoundException' with message 'The "filter_pathologic" plugin does not exist.' in                                 [error]
/Users/Albright/Sites/d8.test/www/core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php:57
Stack trace:
#0 /Users/Albright/Sites/d8.test/www/core/lib/Drupal/Component/Plugin/Discovery/DiscoveryCachedTrait.php(30):
Drupal\Core\Plugin\DefaultPluginManager->doGetDefinition(Array, 'filter_patholog...', true)
#1 /Users/Albright/Sites/d8.test/www/core/modules/filter/src/FilterPluginCollection.php(75):
Drupal\Core\Plugin\DefaultPluginManager->getDefinition('filter_patholog...')
#2 /Users/Albright/Sites/d8.test/www/core/lib/Drupal/Component/Plugin/LazyPluginCollection.php(85):
Drupal\filter\FilterPluginCollection->initializePlugin('filter_patholog...')
#3 /Users/Albright/Sites/d8.test/www/core/modules/filter/src/FilterPluginCollection.php(31): Drupal\Component\Plugin\LazyPluginCollection->get('filter_patholog...')
#4 /Users/Albright/Sites/d8.test/www/core/modules/filter/src/Entity/FilterFormat.php(138): Drupal\filter\FilterPluginCollection->get('filter_patholog...')
#5 /Users/Albright/Sites/d8.test/www/core/modules/filter/filter.module(104): Drupal\filter\Entity\FilterFormat->filters('filter_patholog...')
#6 /Users/Albright/Sites/d8.test/www/core/lib/Drupal/Core/Extension/ModuleHandler.php(496): filter_system_info_alter(Array, Object(Drupal\Core\Extension\Extension),
'module')
#7 /Users/Albright/Sites/d8.test/www/core/modules/system/system.module(913): Drupal\Core\Extension\ModuleHandler->alter('system_info', Array,
Object(Drupal\Core\Extension\Extension), 'module')
#8 /Users/Albright/Sites/d8.test/www/core/modules/system/system.module(970): _system_rebuild_module_data()
#9 /Users/Albright/Sites/d8.test/www/core/modules/user/src/PermissionHandler.php(219): system_rebuild_module_data()
#10 /Users/Albright/Sites/d8.test/www/core/modules/user/src/PermissionHandler.php(205): Drupal\user\PermissionHandler->systemRebuildModuleData()
#11 /Users/Albright/Sites/d8.test/www/core/modules/user/src/PermissionHandler.php(184): Drupal\user\PermissionHandler->getModuleNames()
#12 /Users/Albright/Sites/d8.test/www/core/modules/user/src/PermissionHandler.php(90): Drupal\user\PermissionHandler->sortPermissions(Array)
#13 /Users/Albright/Sites/d8.test/www/core/modules/user/user.module(1374): Drupal\user\PermissionHandler->getPermissions()
#14 [internal function]: user_modules_installed(Array)
#15 /Users/Albright/Sites/d8.test/www/core/lib/Drupal/Core/Extension/ModuleHandler.php(396): call_user_func_array('user_modules_in...', Array)
#16 /Users/Albright/Sites/d8.test/www/core/lib/Drupal/Core/Extension/ModuleInstaller.php(264): Drupal\Core\Extension\ModuleHandler->invokeAll('modules_install...',
Array)
#17 /Users/Albright/.composer/vendor/drush/drush/commands/core/drupal/environment.inc(129): Drupal\Core\Extension\ModuleInstaller->install(Array, true)
#18 /Users/Albright/.composer/vendor/drush/drush/commands/core/drupal/environment.inc(196): drush_module_install(Array)
#19 /Users/Albright/.composer/vendor/drush/drush/commands/pm/pm.drush.inc(1120): drush_module_enable(Array)
#20 [internal function]: drush_pm_enable('pathologic')
#21 /Users/Albright/.composer/vendor/drush/drush/includes/command.inc(359): call_user_func_array('drush_pm_enable', Array)
#22 /Users/Albright/.composer/vendor/drush/drush/includes/command.inc(210): _drush_invoke_hooks(Array, Array)
#23 [internal function]: drush_command('pathologic')
#24 /Users/Albright/.composer/vendor/drush/drush/includes/command.inc(178): call_user_func_array('drush_command', Array)
#25 /Users/Albright/.composer/vendor/drush/drush/lib/Drush/Boot/DrupalBoot.php(46): drush_dispatch(Array)
#26 /Users/Albright/.composer/vendor/drush/drush/drush.php(76): Drush\Boot\DrupalBoot->bootstrap_and_dispatch()
#27 /Users/Albright/.composer/vendor/drush/drush/drush.php(16): drush_main()
#28 {main}
Slate ~/Sites/d8.test/www> 
Garrett Albright’s picture

Status: Needs work » Needs review
FileSize
794 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,430 pass(es), 3 fail(s), and 30,810 exception(s). View

Looks like filter_system_info_alter() is trying to get info about a plugin which doesn't fully exist yet when a module is enabled.

Status: Needs review » Needs work

The last submitted patch, 50: cant-enable-fitler-module-2348925-50.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Fixed

Garrett: please open a new issue and link to this one. It's no longer accepted in core to reopen already committed issues. It's much easier for everyone to have a new issue. It's fine to link to the new one from the fixed one though!

Wim Leers’s picture

Title: REGRESSION: Uninstalling a filter plugin removes text formats » Uninstalling a filter plugin removes text formats

Status: Fixed » Closed (fixed)

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

slashrsm’s picture