Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#27 | drupal7.filter-default-settings.27.patch | 3.28 KB | rdickert |
#25 | drupal7.filter-default-settings.25.patch | 3.34 KB | rdickert |
#17 | drupal8.filter-default-settings.17.test-only.patch | 2.4 KB | sun |
#17 | drupal8.filter-default-settings.17.patch | 3.32 KB | sun |
#1 | filter_weight_5.patch | 880 bytes | Heine |
Comments
Comment #1
Heine CreditAttribution: Heine commentedPatches for 5, 6, 7 move new filters to the beginning of the chain.
Comment #2
Anonymous (not verified) CreditAttribution: Anonymous commentedThe last submitted patch failed testing.
Comment #3
sunHeine, 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.
Comment #4
sun.core CreditAttribution: sun.core commentedComment #5
sunSorry, 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.
Comment #6
Damien Tournoud CreditAttribution: Damien Tournoud commentedTotally agree that HTML filter should run first, it is meant to filter the *user input*, not the tags inserted by other filters.
Comment #7
sunhttp://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.
Comment #8
sunI 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.
Comment #9
hingo CreditAttribution: hingo commentedHi
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:
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.)
Comment #10
hingo CreditAttribution: hingo commentedI assume this is something that needs to be resolved before D7 goes out in the wild?
Comment #11
webchickNope.
The current behaviour has been that way since time immemorial. No reason for it to hold up release that I can see.
Comment #12
hingo CreditAttribution: hingo commented@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.)
Comment #13
sunNo, 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.
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.
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.
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.
Comment #14
hingo CreditAttribution: hingo commentedHi
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.
Comment #15
hingo CreditAttribution: hingo commentedEh, apparently our posts crossed somehow. I didn't intend to change back these settings.
Comment #16
sun#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.
Comment #17
sunAttached 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%
Comment #18
sunBetter title.
Comment #19
chx CreditAttribution: chx commentedLooks 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?
Comment #20
xjmI 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.
Comment #21
sunI 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.
Comment #22
xjmxpost?
Comment #23
catchLooks good to me. Committed/pushed to 8.x, moving to 7.x for backport.
Comment #24
rdickert CreditAttribution: rdickert commentedTaking this issue for backport (referred by zendoodle in office hours).
Comment #25
rdickert CreditAttribution: rdickert commentedBackported to D7.
Comment #26
xjmThanks @rdickert!
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.
Comment #27
rdickert CreditAttribution: rdickert commentedWell, 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!
Comment #28
sunGood to go if bot comes back green.
Comment #29
sun#27: drupal7.filter-default-settings.27.patch queued for re-testing.
Comment #30
webchickThis 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.