Text formats should throw an error and refuse to process if a plugin goes missing

Subscribing.

We probably also want to provide some warning to the administrator when they disable a module whose filters are actually in use, so they understand what will happen.

This might also solve #161354: Disabling a module which provides a text format leaves the text format behind at the same time...

To clarify for anyone else who is reading this, @sun unpublished the issue in order to bring it to the attention of the security team.

After some discussion, the conclusion was that this qualifies as a definite bug and a security hardening patch, but one that can be posted publicly because it is essentially about helping close off an easy avenue for people to configure their text formats insecurely, rather than a directly exploitable flaw per se.

So, the issue is now published again, and a fix can be worked on here.

The following patch implements hook_modules_disabled() and hook_modules_uninstalled() in filter module. When modules that provide filters are disabled, the filters are disabled in all the text formats, but not removed from the database, to preserve the settings.

When modules are uninstalled, then all the filters are removed from the database.

I think the warning message is not so clear, need help on that!

Also, we need this fixed #562908: {filter}.module is not saved, in order to get this working as expected.

+

Rerolled against HEAD.

This patch seems to make sense.

However, I wonder if it is preferred to avoid people being able to disable a module until its filters have been reconfigured. It feels less damaging, and might be a better pattern to introduce and strive for.

Might also be a lot easier to code.

Just a thought.

Deleting any module can have severe damaging effects, and it is not something we are currently protecting people against. If you delete node.module, your site will be broken too. It is well understood that physically deleting a module can break your site, and that is why people disable and uninstall their modules first.

In other words, this patch introduces a new babysitting pattern. I'm not necessarily against that, because it is good for security, but it is a new babysitting pattern, and one that introduces some inconsistency and bloat.

Secondly, usability experts will tell you that a pro-active approach (i.e. prevent damage to begin with) is nicer than a retro-active approach (i.e. help people recover after they have done damage).

So while I'm cool with this "clean-up", I'd like to get some feedback from other core developers about this. Let's get some extra people to comment on this issue.

I had been meaning to take a look at this patch but hadn't had a chance yet. No time for a real review now, but a couple quick comments:

• I agree with Dries that we shouldn't bother worrying about cases where stuff gets randomly changed in the filesystem.... that seems beyond the scope of what Drupal should code for. However, Drupal has an API for disabling modules, and I do think we want to code for the situation where a filter module is disabled via that method. So blocking people from disabling modules via the UI doesn't seem like enough - we still would want to detect something after that, as this patch is doing.
• I definitely like the idea that this patch is detecting broken formats and warning about them in the UI. I'm less convinced about the part where it "zeroes out" all content in that format until the problem is fixed. It's tricky; for some filters, removing them definitely makes the content meaningless or even a security risk, but for others, this seems like huge overkill. If I disable the Smileys module, I really just want my smiley graphics to disappear, rather than having all my content disappear :) I don't have a great solution for this off the top of my head, though.
• I'm not entirely sure why the "missing text formats" thing is part of this patch. It seems like that might be better left to #358437: Filter system security fixes from SA-2008-073 not applied to Drupal 7.x - it's really a different problem. For one thing, when the user deletes a text format, we don't zero out existing content in that format but rather display it using the fallback format (e.g., plain text). It seems that whatever we do here should be consistent with that, since the most likely way that someone would try to filter text with a non-existent format is if the format was deleted but some module didn't update their database tables correctly.

Also note the relationship between this patch and #275811: Warn about potentially insecure filter configurations - there, we detect any text format that is configured insecurely. So, just thinking out loud.... perhaps (combining them) we could do something like "if the removal of the disabled module's filter changed the text format from being secure to insecure, that is the only time we zero out the content - otherwise just warn about it in the UI".

That wouldn't be perfect, of course - maybe the site admin is the only one who had permission to use that filter in the first place, so it doesn't matter. And it doesn't cover other, more rare types of security issues that we won't really be able to detect (e.g., exposed PHP code due to the removal of the PHP filter which reveals your company's secrets that were contained in the source code).

Overall, I'd suggest that we focus this patch on the "broken filter" API parts now and making sure they can be committed by October 15? It seems like the other stuff, including actually zeroing out the content, can wait a bit and leave more time to discuss it.

Personally, I don't think we should remove functional code from this patch. It's working, and it is an improvement over what we have.

The big question is: should it be proactive, reactive or both?

Answer: Ideally it would be both proactive and reactive, but if choosing one, reactive...

Is any part of this patch on an October 15 deadline? It's a bug, of course, but I assumed it was on that deadline because of the "API clean-up" tags -- however, looking at the code, the API changes are tiny (and perhaps not even necessary), which would suggest that it is not. If it doesn't need to be done by October 15, then no need to remove any code now.

@sun: I don't disagree with you, but I was just pointing out that whatever we do needs to be consistent. When the administrator deletes a text format intentionally, it doesn't make a whole lot of sense to automatically replace it with some random text format either, but that's what we do in D7 now (and that would be tricky to try to remove). We can't assume that just because D7 has a hook for deleted text formats, every module had a chance to respond to it: For example, a module that stores some data might have been disabled at the time the text format was deleted, so its hook never got called. When that module is enabled again, its data should not be treated any differently than the data stored by modules that were enabled all along.

It's not blocked by nostalgia. I'm just asking for the opinion of some more extra people. Nothing wrong with that.

Since #358437: Filter system security fixes from SA-2008-073 not applied to Drupal 7.x was marked a duplicate of this, for the time being I'm adding the "security.drupal.org" tag to keep track of it.

Please move the hook_filter_format_$op() implementations to text.module - that's where the caching is done, so that's where it should be cleared.

Hunks in this patch are a duplicate of #651086: Cache clearing is an ineffective mess in module_enable() and system_modules_submit(), needs sorting out.

#556022: When a text format is deleted, some modules (like text.module) don't update their data, even though we say they do was a duplicate of this issue.

I'm a bit concerned about watchdog flooding by the patch here - try to show ten text fields with broken formats and you're suddenly getting ten watchdog inserts per page, but then your site is going to look pretty broken too, so I guess it's OK.

#52: drupal.filter-fail.52.patch queued for re-testing.

Hm, if this patch fixes #556022: When a text format is deleted, some modules (like text.module) don't update their data, even though we say they do, it's not obvious to me why - we definitely need some code comments about this in text_filter_format_delete() explaining how it works :) We also should test this. Currently it looks like the filter module has tests for what happens when you delete the text format assigned to a custom block, but since the Field API part is so different (and currently is broken/nonexistent in HEAD), we need tests for that too.

Actually.... the tests that are added by this patch suggest that the expected behavior for deleting a text format is that the node body goes blank, but actually that's not what we usually want, right? Seems like we want to test two different things: If a text format is deleted by the site administrator, text.module properly acts to actually switch the format of the node body (like other modules do) to the new one, but in a different case (e.g., the node body somehow has a text format that never existed), that's when the node body should go blank. I had some old tests as part of the patch at #358437: Filter system security fixes from SA-2008-073 not applied to Drupal 7.x that might help here, although they are probably very out of date now.

In addition, I still don't understand how the "text format does not exist" part of this patch can work for all cases. As I commented in #30:

We can't assume that just because D7 has a hook for deleted text formats, every module had a chance to respond to it: For example, a module that stores some data might have been disabled at the time the text format was deleted, so its hook never got called. When that module is enabled again, its data should not be treated any differently than the data stored by modules that were enabled all along.

As I understand this patch, it would currently result in data loss in those cases (or at least "effective" data loss that requires you to manually resave a bunch of content to get it back) - and that is not acceptable. It would also result in data loss in the case of a module that does not properly implement hook_filter_format_delete(). Granted, that would be a bug in the other module, but it still seems like a tough price to make the site administrator pay for the bug in this way.

Maybe we can do something simple like store a record of deleted text formats somewhere (e.g., in a variable) so that we know what to do when we encounter one? Then, only content with an assigned text format that never existed on the site would get the "display an empty string" treatment, and all other cases would always have the behavior applied that the site administrator indicated when they deleted the format?

Well, regarding the text module stuff, something is not working right with the patch at least. Here are steps to reproduce:

1. Create an article with some content in the node body and Filtered HTML as the format.
2. Delete the filtered HTML format. The message on the screen says "If you have any content left in this text format, it will be switched to the Plain text text format."
3. Go back to the article you created. The content does not display as plain text - instead, it is completely gone.

Actually, why was #556022: When a text format is deleted, some modules (like text.module) don't update their data, even though we say they do closed as duplicate of this in the first place? I'm not sure why text.module's failure to implement these hooks has anything to do with the issue here - except that it's a useful test case for the rest of the patch :) If it's at all complicated (and it sounds like it might be), I'd suggest reopening that instead.

Re #5, I agree it's a more general problem. On the other hand, this seems way more serious than most of the other examples I can think of. If you're storing a reference to a deleted node or user, you probably don't expect it to display anyway, so it sounds like you'd just get a PHP warning if you tried to but otherwise no difference (and there are ways to code around that if you're careful). But here the content will not be displayed at all, even though the site administrator has every reason to expect it to.

I just looked through this again and I really dislike the idea of allowing people to do something unfortunate, then flashing big warnings at them and breaking all their site content once they've done it. I also fully agree with David and Dries that we shouldn't cater for people removing modules from their filesystem or disabling them with SQL, tough luck if you do that.

So here's an alternative approach, based on #23 from Dries:

However, I wonder if it is preferred to avoid people being able to disable a module until its filters have been reconfigured. It feels less damaging, and might be a better pattern to introduce and strive for.

Might also be a lot easier to code.

Yes, it's much easier - it's actually about 5 lines of real patch, the rest is indentation and another bug fix (see below...).

If a filter is enabled, filter module, in hook_system_info_alter() adds the providing module as a dependency. So if I enable the 'youtube_filter' module, add that filter to a text format, then go to the modules page, I can see that filter module now depends on 'youtube_filter', and that the checkbox is disabled. It's actually the case here that filter module depends on that module, because it has a text format which uses that's module's filter, so although it's a dynamic dependency, I believe it's a proper use of the dependency system and of this hook. It also happens to be the only way I can think of to do this since there's no other way to stop a module being disabled afaik.

There's one snag however. In system_modules() we unset() all required and hidden modules, in doing so, we also prevent dependency information from being shown for those modules, which breaks this patch. To fix that isn't hard, instead of unsetting those modules, just don't render them instead - this allows dependency information to be displayed properly.

However, there's an additional issue that "standard" install profile lists several optional core modules as dependencies, and suddenly these get greyed out checkboxes as well. See screenshot. I've fixed this in the patch by implementing hook_standard_system_info_alter() to reset all the optional modules it depends on to not be required (which will obviously only happen after install) - this way they can be disabled from the modules page again. standard.profile is a special case, since it doesn't really depend on these modules, just recommends them, so I think this is reasonable. I'm including these fixes here becuase they're quite small, and because this approach is fairly impossible without them.

I'm sure someone will complain about the magic dependency setting being a WTF, but compare this to the wtf of all your site content suddenly disappearing and having to go to admin/config/content/formats to get it back, I think it's a considerably smaller wtf.

Fix that test.

A text format being deleted is a completely different issue to a filter-providing module being disabled (while the filter is being used by an active text format). If this needs to be two separate issues, that's fine with me.

edit: this was a crosspost with #74

Looks like we cross posted a couple of times. In the case of major upgrades of Drupal, you should ensure your contrib modules are also up to date. If you don't update modules which your site depends on, then that's much the same as just removing them from the filesystem, and I don't think we need to add a tonne of code to core to support that case.

edit: and this was a crosspost with #75.

It doesn't matter in which way a text format or filter vanished. What matters is that we are not able to filter and output text that has to be filtered.

No, it does matter. If you completely bypass core documentation, UI and APIs, core doesn't protect you from your site being broken, whether that's potential security issues or a WSOD. I don't see any reason to change that pattern here.

There are potential security implications if you wipe a filter-providing module from your filesytem, or it might be that your text doesn't get filtered on pirate day any more. However in both cases there's not a reproducible exploit - it requires mis-administration to get into that state.

So while I fully agree we want to prevent people from getting doing this via the UI (especially without even a warning as it was prior to that SA going in), there's very little motivation to add code to core to support those not following best practices. That goes beyond a bit of babysitting to a full padded cell.

The one place I wouldn't mind that additional check is in filter_requirements() - we can easily check the contents of {filter} to see if there's an enabled filter for a disabled/missing module in there. That would be minimal runtime code and would warn sites upgrading from D6 with inconsistent data.

I took a look at the patch - overall the simplicity is very nice and it definitely hits the most common use case (a filter disappears because the module that provides it was disabled via the UI), but I'm not sure if it gets enough of them. What about if someone disables a module using Drush? What about someone playing around with http://drupal.org/project/flexifilter? Those are valid things for a site administrator to do and are not quite as edge-case-y as someone deleting files in the filesystem. So I think I'm somewhat in agreement with @sun that this isn't quite enough, but on the other hand, one could argue that it's up to those modules to handle those situations - or (as @catch eventually suggested) at least keep hook_requirements() around to inform the administrator about them.

Some other notes:

• The patch is maybe not as smart as would be ideal - e.g., if I turn on the PHP module and immediately want to disable it 5 seconds later, it doesn't let me because the filter is already "in use" (in the text format that the PHP module just created). I think the earlier patches had this kind of problem too, but it was less in-your-face since it didn't prevent disabling the module, just displayed warnings.
• The UI here is confusing since it doesn't indicate how you go about breaking the dependency - how do I know that I need to reconfigure my formats and remove all this module's filter before I can disable it?
• Finally, I don't understand why standard_system_info_alter() is special here... wouldn't any install profile have to do this (since they all now are supposed to declare dependencies in their .info file)?

***

Rereading this issue, I'm not sure I've changed my opinion much since #26. It feels like we need to define a middle ground of how paranoid to be, and then design the behavior around that. And we have to think somehow about different types of filters. For some of the filters that were mentioned above (Smileys, Pirate, etc) it is painful that we'd be requiring administrators to go and manually remove them from all formats by hand before they could (a) have their site work (@sun's approach), or (b) disable the module (@catch's approach). I'd really like to just be able to disable the Smileys module and have my smileys automatically go away :)

Have we considered doing this with a confirm form tacked on the module submission page? So maybe if I disable a module whose filters are in use, I could just be given an extra screen that asks me if I'm sure I want to do that, because removing the filter will change the behavior of text formats X, Y, and Z? And then assume they know what they're doing if they go ahead submit the form? That plus hook_requirements() to catch the edge cases seems like it might not be too bad - or beyond hook_requirements(), we could even go further and drupal_set_message() a specific message onto every admin page like update.module does, since this is a potential security issue just like that one is. I think this might be enough to be able to avoid actually zeroing out any content.

Re #78:

So can you explain why SA-2008-073 contained a security fix for this?

The security fix in SA-2008-073 was because Drupal told site administrators one thing but actually did another. When they went to delete a text format, it told them that all content using that format would be replaced with the default format, but in fact, any content using that format that was stored by a non-core module would actually be displayed with no filtering at all - i.e., it was essentially treated as Full HTML no matter what.

@sun. Please explain #79. I said here that we should not allow filter modules to be disabled which have filters in use. I said on the text format issue that we should not allow text formats to be deleted which are in use. These are entirely consistent positions. You may not like them, but they're consistent. In all cases there is no ideal behaviour, so I would prefer we did the simpler solution that doesn't involve completely breaking people's sites by either hiding all their content (here) or batch updating potentially millions of items (there) (especially when we have no way to differentiate between HTML escaping and smileys as David points out).

David is correct about the security issue. Core doesn't have to account for every possible eventually that could ever happen in relation to security, we still ship with PHP module ffs, it just has to not be completely misleading, and ideally make it somewhat difficult to end up in an insecure state.

@David. Given we've not prevented this in the past, hook_requirements() seems sensible. I'm less happy with drupal_set_message() on every admin page, update.module hacks that in via a hook_help()...

I also think it's fine to expect drush and/or flexifilter to sort this out themselves.

On PHP module being immediately a dependency issue when writing the patch. I'd prefer it if PHP module didn't automatically create its own text format, most filter modules don't do this - they allow you to apply their filter to existing text formats, I don't see why PHP module is any different. There are also modules which only show PHP textareas for certain functionality if PHP module is enabled, so it's possible to enable it without wanting the format at all.

# The UI here is confusing since it doesn't indicate how you go about breaking the dependency - how do I know that I need to reconfigure my formats and remove all this module's filter before I can disable it?

Currently you don't, and we have no mechanism for explain this. We could add to filter help text, if we have a hook_requirements(), we could also add a non-warning status showing the current filters in use. Not sure there's much else we can do within existing core APIs.

# Finally, I don't understand why standard_system_info_alter() is special here... wouldn't any install profile have to do this (since they all now are supposed to declare dependencies in their .info file)?

Well the question is whether they're actual dependencies or just suggestions, as far as I can see the only reason this works as it does currently is due to a bug. If you have a profile with hard dependencies, these are ignored due to the current bug. If you have a profile with suggestions/defaults, these will be enforced if the bug is fixed. Needs its own issue.

Well the question is whether they're actual dependencies or just suggestions, as far as I can see the only reason this works as it does currently is due to a bug. If you have a profile with hard dependencies, these are ignored due to the current bug. If you have a profile with suggestions/defaults, these will be enforced if the bug is fixed. Needs its own issue.

My understanding is that "dependencies" is the new way for a profile to list the modules it wants Drupal to automatically install... I think this was introduced in #509392: Introduce .info files for install profiles. It might be by design.

So in other words, "dependencies" sort of means different things for an install profile than it does a module :/ Profiles might not have any way to enforce a "hard" dependency except by altering in a 'required' flag on the module or some such.

Rolled. This one should pass (the previous one was trying to disable the filter_test module that is required by the filter module... through the UI).

Retrying.

#88: 562694-filter-varnish-security.patch queued for re-testing.

why did it say failed testing, looks like it passed testing?

+++ modules/system/system.module
@@ -2271,6 +2271,8 @@ function _system_rebuild_module_data() {
   // The install profile is required.
   $modules[$profile]->info['required'] = TRUE;
+  $type = 'profile';
+  drupal_alter('system_info', $modules[$profile]->info, $modules[$profile], $type);

The immediate thing I see here is that this is adding a second info alter on $modules[$profile], the first having type = 'module' (inside the foreach), the second with type = 'profile'. It seems redundant to process the profile as both.

If 'profile' is a possible value of $type, it's a minor API change and needs a hunk in system.api.php. However I think it points to an inconsistency though that we store {system}.type = 'module' for profiles but special case them like this, with $type being 'profile' here. Also, it looks like scope creep because erasing standard.profile's dependencies[] during runtime is pretty far from filter.module and the title of this issue.

I offer an alternative where $type is still 'module' but the filename is checked instead of $type inside the info alter implementation. Checking $file->filename extension is a convention we seem to have going for determining if a 'module' is a profile (see #808162: update.php fails when optional modules disabled). Hopefully in D8 we can find a better way.

lol @ filename of #86 patch, I admit that I read the same thing at first.

65 critical left. Go review some!

Hmm, and what if you're not using standard.profile? Instead of every profile having to info alter their dependencies[] out, can we just do it from system.module for the current profile?

I'm reviewing this, and the patch in it's current status seems to contradict itself. Why do we have the messages in hook_modules_disabled and hook_modules_uninstalled to warn about filter modules that have been disabled, when the patch tries to prevent module form being disabled? I.e., if the patch is working, we should never see those messages.

I agree with David that it's disconcerting to enable PHP module and then find that you can't disable it because it magically got a dependency, and doesn't tell me how to break that. In essences, if someone ignorantly turns on PHP module, we're making it harder for them to correct their mistake, so we're actually making Drupal MORE vulnerable to security errors in the hands of someone who doesn't know better. You can't argue that PHP module is too dangerous to ship with core on the one hand, and then make it impossible to turn it off on the other.

This also breaks convention, as I have never before seen a module gain a new dependency after being enabled. The hook_system_info_alter just feels like a hack. Perhaps we need a more general way to define a dependency for a module which is not another module (e.g., a setting/variable, or external library, etc), and let that dependency notice link to a relevant URL to guide the user in addressing the dependency.

I think the best compromise between user experience & security would be to present a confirmation form that says, 'Are you SURE you want to disable that, because you're going to break these text formats: PHP, etc,....' How difficult is that to do? Do any of the prior patch come close to that? (I only reviewed the newest patch.)

Since #820054: Add support for recommends[] and fix install profile dependency special casing has been moved to D8 by webchick, we should look at a centralised hack for this yes. System module doing an alter would be easier to rip out later than checking the filename somewhere in system_modules().

Also there are hunks in the current merged patch which are unnecessary if we do filter_system_info_alter(), for example:

 function filter_modules_disabled($modules) {
   // Reset the static cache of module-provided filters, in case any of the
   // newly disabled modules defined or altered any filters.
   drupal_static_reset('filter_get_filters');
+
+  if (_filter_get_broken_filters(NULL, $modules)) {
+    drupal_set_message(t('At least one text format contains a filter that was provided by a module that has been disabled. All content using one of these text formats will be hidden until <a href="@text-format-url">text formats have been re-configured</a>.', array('@text-format-url' => url('admin/config/content/formats'))), 'error');

Which once again makes me ask the question that if we're not going to run into this when disabling modules, only when they're deleted from the file system, do we really need all that code for something which happens once in a blue moon, and which only if you're very unlucky is going to cause a problem in the first place, and with the incredibly confusing side-effect of all your content disappearing.

cross-posted with matt2000

We can easily fix the PHP module issue by stopping it from creating a format when it's enabled. That's 4.6-era behaviour that should just stop.

Removing the PHP format is worth it in its own right, so opened #826550: PHP module should not create a text format.

Big +1 for this approach. I'm using the htmlpurifier module instead of the core html filter, and have lost a bit of sleep wondering what will happen if some future admin carelessly disables the purifier module.

Regarding the hook_requirements message, I think it would be much more helpful if it named the modules instead of just a generic warning (to do so easily may require keying _filter_get_broken_filters differently).

todo list?

I'm not sure what parts of #94 and #95 were reviews of the previous patch or descriptions of the new approach moving forward.

from #96:
We can easily fix the PHP module issue by stopping it from creating a format when it's enabled.

from #98:
Regarding the hook_requirements message, I think it would be much more helpful if it named the modules instead of just a generic warning (to do so easily may require keying _filter_get_broken_filters differently)

I'm pretty much -1 on the "add a dependency to the filter module for all the modules that provide a filter that is currently used". It's just:

1. confusing,
2. completely unnecessary: because we have other code in place that protects text formats from outputting sensitive information when on of the filter is not available anymore,
3. insufficient anyway, because filters can disappear for other reasons (a module is removed from the filesystem, a module stop providing a filter, etc.)

I strongly prefer the soft approach here, so here is a reroll of the patch without the parts that mess up with dependencies.

A summary of what this patch actually does:

• Implements a way to detect "broken" text formats (formats that rely on filters that are not available)
• Adds an entry to the status report when at least one text format is "broken"
• Display a warning to the user when a module that provided a filter that is used on one or more text formats is disabled or uninstalled
• Disable the output from any text format that is broken (ie. returns an empty string), to avoid exposing sensitive information

Some screenshots:

When disabling a module that provides a filter that is currently in use:

The status report:

The text format page:

We discussed that with catch today, and came to the conclusion that both this patch (which makes it more obvious when a filter vanishes) and the previous approach (adding a dependencies to the module that provide a filter) might be more harmful then the problem they are initially trying to solve.

Because I don't believe this is such an issue anyway, let's just downgrade this. If someone comes with a proper solution, let's keep consider this for Drupal 7, but this doesn't have any of the characteristics of a release blocker.

Clarifying in the tags that this is a security improvement, not an SA followup.

Clarifying in the tags that this is a security improvement, not an SA followup.

In short, fixing such a weird edge case while useful security hardening is not a critical issue and it takes an insane amount of fairly ugly code.

#358437: Filter system security fixes from SA-2008-073 not applied to Drupal 7.x was marked duplicate of this, so this can't be downgraded unless you also decide to "un-duplicficate" that one...

No, we already have the pieces needed to cover SA-2008-073:

• When a text format is deleted, all the content is changed to the fallback filter
• If check_markup() is called with an unknown text format, it just returns ''

tag

Removing tag, this is Drupal 8 now.

This is still posted on the initiative page:

http://drupal.org/community-initiatives/drupal-core

Can someone with permissions remove it?

#38: drupal.filter-fail.37.patch queued for re-testing.

Marking this Drupal 7, for the moment, in the hopes that this will make the test bot run, especially as #556022: When a text format is deleted, some modules (like text.module) don't update their data, even though we say they do might be simplified with this.

Tested with disabling the PHP module, nice alert message, and PHP text format content goes blank. All one has to do to bring the content back is save the text format, but, we've been warned, at least twice, by that point.

+    // When a text format is updated, we need to assume that its filter
+    // configuration has been verified and we are safe to remove any pointers
+    // to vanished filters.

Code comment should not use first person plural, just say what happens and why.

+        $filter->status = -1;

Should be a constant rather than a magic number.

function _filter_get_broken_filters()

+  // Compile a list of vanished filters.

Is the filter broken because it's missing, or would the text format the filter is assigned to be broken? Also did the filters really vanish? Or were they disabled intentionally? Both the code comments and function naming don't accurately convey what the potential issues are leading to this situation.

Also I still dislike strongly that we just let people get into a broken state here when it's a situation that could be more or less easily prevented, so would not want to see this go in without at least a Drupal 8 followup to fix the limitations on preventing modules being disabled in the first place (i.e. #67 onwards).

This is a bugfix. You do not help by pushing bugfixes to 8.x needlessly yet.

Good to go!

I'd still like to see some reviews here from people like Damien and David Rothstein who were involved in earlier incarnations of this patch but haven't spoken up for the current one.

This feels like too much of a behaviour change to insert this late (and even in October when this was first marked RTBC) to me, but if the consensus is the security improvement is worth it, we can discuss, as long as we wrap this up by RC1.

I re-read this issue and the latest patch. I'm mostly in favor (it is definitely a security improvement), but I don't think we've reached consensus on a couple issues yet.

1. If we're going to render their site broken after disabling one of these modules, we really need to warn people before they disable the module (as several of us suggested above). It's not enough to put a red "by the way your site is now broken" message after the fact; people will have no idea what hit them. We need a confirm form on the modules page. If it's really hard to have filter.module alter that in, I assume it would be fine to have the code live in system.module for D7, and improve it in D8.
2. I'm still suspicious of the part of this that has check_markup() refuse to display any content for the formats that have missing filters. That makes perfect sense for some filters, but no sense at all for others. It is still crazy that with this patch, disabling the Smileys or Pirate modules (the examples a couple of us brought up above) would potentially cause Drupal to stop displaying all of your site's content until you went through a ritual of clicking through a bunch of text format configuration forms. Maybe in D8 we can have a way to categorize filters that makes it possible to do this more granularly, but right now it's an extremely blunt tool to be applying to all of them.

   That said, I think I'm not totally opposed to it, provided we do the confirm form above - that would at least make it consistent with the behavior we now do when a text format is disabled/deleted (even if neither of those behaviors always make sense). However, I don't think we should be under the illusion that it really provides much security by itself. If they don't understand what's going on and simply re-save the text format without making any changes, they've then managed to convince check_markup() to start displaying their content again, but whatever security problem they might have had without this patch would start appearing right then anyway.

   It is more important that we educate people than try to directly protect them.

The patch itself has a couple issues, one major:

3. It seems to be checking for broken filters even on disabled text formats! (I discovered that because somehow I wound up in a scenario where it showed me the warning message even though none of my text formats had any problems; it seems to be because I had disabled the PHP code text format with the filter still in it, then disabled the PHP module.) That needs to be fixed.
4. Everywhere the patch uses "re-configured" it can be "reconfigured" instead.

5. @@ -262,6 +283,7 @@ function filter_format_save($format) {
        cache_clear_all($format->format . ':', 'cache_filter', TRUE);
      }
    
   +  // Clear the filter cache whenever a text format is deleted.
      filter_formats_reset();
   

   That should say "saved", not "deleted"?
   

，~

1. There is already a confirm form on the modules page that warns you when something unexpected (e.g., enabling modules you didn't ask for) is going to be done. With this patch, Drupal would actively stop displaying your content when certain modules are disabled, so that should be confirmed also. Since filter is a required module in D7, it shouldn't be a big deal if it can't be altered in; I think _filter_get_missing_filters() should probably be turned into a public function anyway, in which case system.module could simply call it.

3. Why do you think disabled text formats invalidate the entire patch? I think you just need to make sure _filter_get_missing_filters() ignores them (or at least ignores them by default). In that case, they will remain pristine in the database, with the original set of filters they last had. If someone wants to write a module to allow them to be reenabled, they could either deal with this issue themselves, or just let it be, and then in that case the newly re-enabled format won't start working until it's been verified/saved by the site administrator, which seems to be what we would want.

Why? Disable an existing text format having a filter enabled. Disable the module providing that filter. Try to update your text formats in order to ensure they are still secure. (Disabled text formats are still actively used.)

I am not sure what you mean by "actively used"...

The bug I reported above is that currently the patch will report disabled text formats as insecure when they contain missing filters. Since disabled text formats can't be accessed via the the UI to reconfigure them, and are protected by check_markup() which already returns an empty string whenever they are used (regardless of what filters they contain), we should just need to make the patch ignore them, so it doesn't display bogus warning messages in the above situation. Isn't that all we need to do to fix that - make _filter_get_missing_filters() ignore them by default?

#129: drupal.filter-fail.129.patch queued for re-testing.

Release blockers are by definition Critical.. so I dont think this is release blocking.

Retitling. This is critical since modules that provide filter plugins that are used in filter format should be able to be uninstalled. See discussions in #2348925: Uninstalling a filter plugin removes text formats

What's left here after #2348925: Uninstalling a filter plugin removes text formats?

AFAICT this problem from back in 2009 has indeed been resolved :)

@alexpott agreed with #149 and #150 in irc, I think we can mark this duplicate of #2348925: Uninstalling a filter plugin removes text formats.

Actually we missed a bit from here in #2348925: Uninstalling a filter plugin removes text formats.

I think we still need to resolve the behaviour in the following case:

1. A text format is configured to use a plugin

2. The plugin disappears.

There are now many less instances where a plugin can disappear, but not quite all of them:

1. The plugin could be removed or renamed in a contrib module with no upgrade path
2. The contrib module is removed from the system entirely without running uninstall

In either of those cases, when trying to run a text format, it should cause both an error and refuse to render the string.

Neither of those cases are 'allowed', but still seems worth warning people about it.

Neither of those cases are 'allowed', but still seems worth warning people about it.

I was going to say exactly that as a reason not do to this. But I think you're right; even in these unsupported edge cases, Drupal should remain safe.

So close! :)

I'd also be OK if the current behaviour was to throw any kind of fatal error due to the class not existing or similar - as long as we don't fail completely silently as if nothing happened.

I like that idea. Simple. And puts the burden on those doing the crazy things: modules changing filters without upgrade path or developers removing code without letting Drupal know.

gave a stab at an issue summary.
Could probably use some tightening up by someone more familiar with the issue.

Also started on beta evaluation. What would the disruption be?

html nit.

We already have this in ProcessedText::preRenderText():

    // If the requested text format does not exist, the text cannot be filtered.
    /** @var \Drupal\filter\Entity\FilterFormat $format **/
    if (!$format = FilterFormat::load($format_id)) {
      static::logger('filter')->alert('Missing text format: %format.', array('%format' => $format_id));
      $element['#markup'] = '';
      return $element;
    }

What if we should simply use very similar pattern when a filter plugin is missing. Log, drop text, move on. Could possibly result in a huge amount of log entries, but we can't change that unless you really want to break the whole page, which could be dangerous as it might be hard to get to the settings page.

#158 is a good plan.

Now that this is 'illegal' via configuration dependencies etc., I'm going to downgrade this issue to normal.

If a module has provided a filter plugin for some purpose during development, then wishes to remove that plugin entirely without uninstalling the module, is there any documentation on how to do so?

AFAICT #2015313: Missing filters result in Exception when the format is used fixed this many years ago?

Yes I think #2015313: Missing filters result in Exception when the format is used was even an improvement over a previous issue that did what this suggests. Closing as outdated.