In filter.admin.inc:

  foreach ($form_state['values']['filters'] as $id => $checked) {
    if ($checked) {
      list($module, $delta) = explode('/', $id);
      // Add new filters to the bottom.
      $weight = isset($current[$id]->weight) ? $current[$id]->weight : 10;
      db_query("INSERT INTO {filters} (format, module, delta, weight) VALUES (%d, '%s', %d, %d)", $format, $module, $delta, $weight);

      // Check if there are any 'no cache' filters.
      $cache &= !module_invoke($module, 'filter', 'no cache', $delta);
    }
  }

When a new filter is added to a Drupal installation, it is added after the HTML filter (weight of 1). When such a new filter emits HTML, this may lead to an insecure site when the user does not rearrange the filters.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Heine’s picture

Status: Active » Needs review
FileSize
877 bytes
886 bytes
880 bytes

Patches for 5, 6, 7 move new filters to the beginning of the chain.

Anonymous’s picture

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Heine, I'm not sure I agree with the thinking here, but you know I'm totally open to discuss.

In general, I differ between "security filters" and "markup/macro filters". Security filters should run first, because they sanitize the unsafe user input. If, after security filters, certain markup is left, and there are macro filters enabled that extend or convert certain things, then they need to run after. And, we need to trust those module-provided macro filters to produce safe HTML, because that markup does not originate from the real author but from the system.

Therefore, by default, security filters like "Allowed HTML tags" or "Escape all HTML" filter need to run first. If you add further filters, then they should run after. Hence, new filters are added with a weight of 10.

Please also note that filter status, weight, and configuration live on the same page in Drupal 7 now. Which "basically" means that this issue only applies to programmatically saved text formats and filters.

sun.core’s picture

Version: 7.x-dev » 8.x-dev
sun’s picture

Status: Needs work » Closed (works as designed)

Sorry, without further information this issue can only be marked as by design.

Feel free to re-open this issue if you want to provide further information. Thanks.

Damien Tournoud’s picture

Totally agree that HTML filter should run first, it is meant to filter the *user input*, not the tags inserted by other filters.

sun’s picture

Version: 8.x-dev » 7.x-dev
Status: Closed (works as designed) » Active

http://api.drupal.org/api/function/filter_filter_info/7 already declares a weight of -10 for HTML filter. This weight should be taken over as default weight, but I'm not sure whether that's covered by tests.

sun’s picture

Title: New filters run after the HTML filter by default » HTML filter should run first by default (tests only)
Category: bug » task
Issue tags: +Needs tests

I tested manually now. If you create a new text format and enable/disable/toggle filters - without re-ordering them - then HTML filter's default weight is stored accordingly to run first.

So all we need here are tests to ensure we don't break this behavior in the future.

hingo’s picture

Title: HTML filter should run first by default (tests only) » Should HTML filter run first or last by default?

Hi

Could we please reconsider this issue? I would like to support the original view by Heine that HTML filter should run last (at least in most cases).

The general ideology for this is that security filters should indeed run last, and no other filters should be trusted to run after them. This way you ensure that the list of tags set in HTML filter settings is exactly what is output, nothing can be added through a backdoor in some other module. (For instance, if I want to use img_assist or some similar module to add images to a post, I should explicitly add the img tag to HTML filter ...is my thinking.)

However, I also have a more particular objection. There are modules that will break if HTML filter runs before them. I'm the maintainer of the Footnotes module and I know the Biblio module (which is more popular) is in the same boat. The general category that will break are all modules that provide "extended" HTML/XML tags, which they then transform into some other texts with only valid HTML. (For instance in the case of Footnotes, I've taken care that I only output HTML which is compatible with the default setting of HTML filter, which must run *after*.)

The problem is as follows, using Biblio as example:

"The citation info of this citation is stored in my BibTex database, and I can easily insert it with a bib tag after this quote."<bib>12345</bib>

What the Biblio module does here is look up the work and author info for entry 12345 in your database, then insert a small footnote with that info. If HTML filter is run before this, all bib tags are just removed and this breaks.

It is true that both Biblio and Footnotes support also BBCode-like [fn]square brackets[/fn] instead of HTML-like <fn>angle brackets</fn>. The BBCode like brackets would obviously not have any issue with HTML filter. However we need to continue to support both, because:

1) It is advisable to be coherent across one site, so those that already use the <bib> or <fn> syntax should continue to do so.

2) The Footnotes plugin for CKEditor uses the <fn> variant only, since it uses the CKEditor FakeObject API to essentially render real-time footnotes. This is only possible with real XML tags, not any BBCode tags (I think, didn't write it). I think TinyMCE has something similar, but the TinyMCE Footnotes plugin isn't this fancy (yet).

***

Hmm... having written all of the above, I actually would like to advocate for general sanity either way: Core filters should use default weights that are close to zero, while still being correctly ordered amongst each other (possibly even leaving some "gaps" between each other). Core developers should not dictate that no addon module can ever run before HTML filter (even if you find your way "convenient", I suppose you admit that there is no problem if some other filter wants to go first? There is no absolute requirement, such as security reasons, to force HTML filter to have -10). And they should also not dictate that nobody can ever run after HTML filter for that matter (for this a security conscious person could argue that you could force it this way, but even here, you never know what needs some addon module may have).

So if you still feel HTML filter must be "early", just please don't set the weight to -10. Something like -5 should be enough, then those modules that need to fire earlier can use -10...-6. Similarly no core module should have a bigger weight than +5 so that there is ample room for addon filters to go after. (Ok, so HTML corrector by its nature probably can safely go a bit beyond that, but still not more than +8.)

hingo’s picture

Priority: Normal » Critical

I assume this is something that needs to be resolved before D7 goes out in the wild?

webchick’s picture

Priority: Critical » Normal

Nope.

The current behaviour has been that way since time immemorial. No reason for it to hold up release that I can see.

hingo’s picture

Priority: Normal » Critical

@webchick: What you say cannot possibly be correct, since the ability to set default weight is new in D7. Now would be a good time to set it wisely.

The closest you can get to compare with D6 is that HTML filter seems to have a default weight of 1, which is more than 0. This would leave 0 ... -10 for addon filters to run before it, if we only had some way to suggest a weight. If the default weight remained as 1 in D7, I would be quite ok with it. Setting it to -10 will break addon filters that work in D6 (ok so user can still manually enter the right setting).

(Same comment here as I've said in some other bug: If core developers want to release and not discuss this, that is of course your right to do so, but your argument for downgrading priority was not correct, so I'm reverting it for now. If you just say that this won't be fixed, period, then I will have no counterargument.)

sun’s picture

Title: Should HTML filter run first or last by default? » HTML filter should run first by default (tests only)
Priority: Critical » Normal

The general ideology for this is that security filters should indeed run last, and no other filters should be trusted to run after them.

No, there's a flaw in here. We trust other filters, because they have been setup by an site administrator. We don't trust user input, that's all.

This way you ensure that the list of tags set in HTML filter settings is exactly what is output, nothing can be added through a backdoor in some other module.

No, the HTML filter runs on user input, not on output. It is limiting the allowed HTML tags in the user input, not in the output.

(For instance, if I want to use img_assist or some similar module to add images to a post, I should explicitly add the img tag to HTML filter

No, and that's exactly the point: You don't want to allow your users to use IMG, DIV, and SPAN tags in their posts. Therefore, HTML filter strips those tags away and does this first. Afterwards, other filters may run that convert specially crafted text macros into rich markup content, such as an image with a caption in a container.

The general category that will break are all modules that provide "extended" HTML/XML tags

Exactly for this reason, I can't stop telling filter module authors that abusing an entirely made up/invented HTML tag for filtering purposes won't work. You will additionally face problems with client-side (WYSIWYG) editors, as most of them are simply stripping everything invalid away.

Even if there's one editor that hacks its own DOM doctype to allow for invalid markup, that doesn't mean the idea is right and commonly supported.

If your filter needs or wants to run before HTML filter for any reason, then you can assign a custom default weight (whichever you want).

--

In the end, we just need tests to ensure the default weight of -10 here.

--

Overall, you're very welcome to join the contrib efforts in Inline module.

hingo’s picture

Title: HTML filter should run first by default (tests only) » Should HTML filter run first or last by default?
Priority: Normal » Critical

Hi

I actually downloaded newest D7 in order to illustrate my comments with an actual patch. I see now that in D7 the weights for a filter can actually range from -50 to +50. The 'weight' attribute wasn't documented anywhere like on http://api.drupal.org/api/function/hook_filter_info/7, so I only saw this when I went to actually see the new UI.

My original objection was based on a belief that -10 would be the smallest weight possible, which was the case in D6. As things are now, I have absolutely no objection to what is in D7 filter.module now. In fact, the current code does exactly what I wanted to propose in the first place :-)That we seem to completely disagree on things that are just matters of personal taste is quite ok, as long as the core defaults to settings that allows addon modules to cleanly "do their own thing", whether you like what is being done or not.

Sorry for disturbing. Overall, the ability to set default weight and other improvements in filter api are very welcome, look forward to porting my own work to D7.

hingo’s picture

Title: Should HTML filter run first or last by default? » HTML filter should run first by default (tests only)
Priority: Critical » Normal

Eh, apparently our posts crossed somehow. I didn't intend to change back these settings.

sun’s picture

Title: HTML filter should run first by default (tests only) » HTML filter should run first by default
Version: 7.x-dev » 8.x-dev
Category: task » bug
Issue tags: +Needs backport to D7

#1373142: Use the Testing profile. Speed up testbot by 50% just revealed that filters may define a default weight, but it is not applied. Wonkydonkyponky.

EDIT: See http://drupalcode.org/sandbox/sun/1255586.git/commit/8670e706a9b681d4457... for the functional fix.

sun’s picture

Assigned: Unassigned » sun
Status: Active » Needs review
FileSize
3.32 KB
2.4 KB

Attached patch fixes the issue.

I'd love to get this out of the way ASAP for #1373142: Use the Testing profile. Speed up testbot by 50%

sun’s picture

Title: HTML filter should run first by default » HTML filter is not run first by default, despite default weight

Better title.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Looks good. I wonder about that D7 backport though. If I am not mistaken, enabling module foo implementing filters in 7.12 and the version this will come out with will have different behavior?

xjm’s picture

Issue tags: -Needs tests

I think it is OK for backport, since the weight is set when the module defining the filter is enabled only. Thereafter users can (and do) reorder them.

sun’s picture

Issue tags: +Needs tests

I considered to change the behavior first, but in light of the backport, I merely fixed the bug of defined filter default weights not getting applied.

The old code applies a default weight of 10, completely ignoring a filter's defined default weight. The default weight was supposed to improve security... that's why the HTML filter and the HTML Escape filter define a default of -10.

As @xjm mentioned, the default weights are only applied to new text formats, and only when saving them programmatically without specifying a weight. The Filter UI passes a weight for each filter when saving a text format.

xjm’s picture

Issue tags: -Needs tests

xpost?

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Looks good to me. Committed/pushed to 8.x, moving to 7.x for backport.

rdickert’s picture

Assigned: sun » rdickert

Taking this issue for backport (referred by zendoodle in office hours).

rdickert’s picture

Status: Patch (to be ported) » Needs review
FileSize
3.34 KB

Backported to D7.

xjm’s picture

Status: Needs review » Needs work

Thanks @rdickert!

+++ b/modules/filter/filter.moduleundefined
@@ -215,9 +215,11 @@ function filter_format_save($format) {
+    // If the format does not specify an explicit weight for a filter, assign    ¶
+    // a default weight, either defined in hook_filter_info(), or the default of    ¶
+    // 0 by filter_get_filters().    // Add new filters without weight to the bottom.

Something went funky here wit the comments. They're not wrapped properly (lines should be less than 80 chars, plus one seems to have two comments on one line). Also some trailing whitespace.

rdickert’s picture

Status: Needs work » Needs review
FileSize
3.28 KB

Well, that's embarrassing. Sorry for missing that! Posting this again with correct formatting and trailing whitespace removed. If I have fixed the problem with Windows inserting CRLF line endings, this should be done. As I understand it, it will fail testing if not. Thanks for your patience, xjm!

sun’s picture

Status: Needs review » Reviewed & tested by the community

Good to go if bot comes back green.

sun’s picture

webchick’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +7.13 release notes

This seems like a pretty obvious bug. Thanks for the fixes and tests.

Committed and pushed to 7.x. Seems worth calling out in the release notes maybe.

Status: Fixed » Closed (fixed)
Issue tags: -Needs backport to D7, -7.13 release notes

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