Problem/Motivation
- Drupal 8 core outputs
<!DOCTYPE html>
at the top ofhtml.html.twig
, which tells the browser that the contents are HTML5. - http://www.w3.org/TR/html5/single-page.html#non-conforming-features lists many tags and attributes from older versions of HTML that the HTML5 specification defines as: Elements in [this] list are entirely obsolete, and must not be used by authors.
- Install Standard Profile, and go to
/node/add/article
. Decide that you can't think of anything original to write so you go searching for interesting content on other web pages to copy and paste. You stumble upon https://www.cs.tut.fi/~jkorpela/www/justify.html, which is a perfectly valid HTML 4 page and find that you like the section "What is justification?", so you select that whole section and copy it to your clipboard. You then switch back to your node form and paste that into the body field, click Save, and voila, you now have a page on your site with some lovely justified and right-aligned paragraphs. - Problem is,
align
is not a valid HTML5 attributes. Current browsers still respect it, but there's nothing stopping a future browser from ignoring it. And meanwhile, you now have a site that will be failing strict html validation (if you care about that sort of thing). - If instead, non-html5-valid attributes were filtered out, you'd have the opportunity to discover a more correct (i.e., standards-compliant, and therefore future-proof) way of aligning your paragraphs, such as going to your format configuration and adding the alignment/justification buttons to your toolbar.
Proposed resolution
Fortunately, we already have all of the tooling necessary to fix this. FilterInterface::getHTMLRestrictions() provides the API for expressing attribute (and other) restrictions and Html::load() gives us a DOM document that we can use in order to apply those rules. So the proposed resolution consists of changing the allowed_html
setting of html_filter
to include a whitelist of attributes, and for each attribute, an optional whitelist of allowed attribute values.
Remaining tasks
Review.
User interface changes
The format configuration UI.
API changes
None.
Data model changes
The allowed_html
key of the filter_html
filter's configurations is expanded to allow for whitelisting attributes. So, e.g. <a>
to <a href hreflang dir>
, <h4>
to <h4 id>
, et cetera.
Beta phase evaluation
Issue category | Bug because content authors shouldn't be required to know the HTML5 specification in order to avoid generating invalid web pages. |
---|---|
Issue priority | Major because we should not change the format configurations of existing sites, so if we don't prioritize this for 8.0.0, then sites built between 8.0.0 and when this is fixed will have invalid html content potentially for a long time. |
Prioritized changes | The main goal of this issue is usability, in the sense of helping content authors generate content that is standards-compliant and therefore future-proof. |
Disruption | Requires an upgrade path and is disruptive to some existing D8 sites (depending on which attributes their content depends on, they may need to manually add additional attributes and attribute values to be whitelisted). Potentially disruptive for contributed and custom modules that implement filters that run after 'filter_html' but use internal attributes that 'filter_html' will now be filtering out. However, these modules have ways to resolve that, such as instructing site builders on the required configuration for compatibility and/or using the FilterInterface::prepare() method. |
Comment | File | Size | Author |
---|---|---|---|
#149 | increment.txt | 4.11 KB | pwolanin |
#149 | 2549077-149.patch | 63.99 KB | pwolanin |
#147 | interdiff.txt | 776 bytes | Wim Leers |
#147 | 2549077-147.patch | 63.48 KB | Wim Leers |
#145 | interdiff.txt | 1.21 KB | Wim Leers |
Comments
Comment #2
effulgentsia CreditAttribution: effulgentsia at Acquia commentedComment #3
effulgentsia CreditAttribution: effulgentsia at Acquia commentedComment #4
effulgentsia CreditAttribution: effulgentsia at Acquia commentedWim has already put together a first draft patch. Assigning this to him to post that work.
Comment #5
Wim LeersThis patch was created using
$ git format-patch origin/8.0.x...HEAD --stdout > patchfile
, so it can be applied usinggit am
. This ensures you continue to see all individual commits, which should make the review process significantly easier.The 8 commits contained in this patch, in commit order, are:
Commits 1-4 affect the PHP code. Commits 5–8 affect the JS code for filter setting syncing (i.e. the stuff that #1894644: Unidirectional editor configuration -> filter settings syncing brought to core). And point 5 specifically is a fix to pre-existing bugs in HEAD, the patch for which has been filed in the corresponding issue.
Comment #6
Wim LeersThe IS was wrong about BC.
While any existing value for the
allowed_html
key of thefilter_html
filter's configurations remains valid (and is BC-compatible in that sense), keeping the exact same value means that all attributes of those allowed tags are stripped. Because this patch only allows whitelisted attributes and attribute values to remain.Also, if this truly were BC-compatible, I wouldn't be working on this now, but I'd do it in 8.1.0. Doing this in 8.1.0 would cause an extremely painful upgrade path, one that would by definition require manual intervention. It'd be better to do that before 8.0.0 for that reason.
The whole point of this issue is that no attributes are allowed by default. The only thing we can do in terms of upgrade path (and this patch already updates the D6 upgrade path in that way too), is assigning the attribute whitelist for each tag that D8 ships with to the tags that are allowed in D6 (and D7, once we have a D7 upgrade path). That sounds very complex, but it simply means mapping
<a>
to<a href hreflang dir>
,<h4>
to<h4 id>
, et cetera.The beta evaluation got it right though:
IS updated accordingly.
Comment #7
Dave ReidI'm assuming this will affect Entity Embed and URL Embed?
Comment #8
Wim LeersThe attributes they need would need to be whitelisted, but if their CKEditor plugins mark those attributes as required (as they should), then they'll be whitelisted automatically when their corresponding CKEditor buttons are enabled. Which is just like enabling the image button for CKEditor, that causes the
data-align
anddata-caption
attributes to be whitelisted on the<img>
tag.Comment #9
RainbowArrayWhat this boils down to me is how simple is it to whitelist attributes? Where does that happen?
Is there a way to whitelist an attribute so it can be used on every allowed tag? Or do you need to manually whitelist every attribute for every single allowed tag? If the format I'm seeing in the patch is how that will be done, if somebody wants to whitelist a number of attributes for every tag, that could get very long quickly.
The initial set of attributes looks like it's a pretty small subset of the valid html5 attribute list. And this doesn't prevent anybody from whitelisting an attribute that isn't valid in html5. It might be more difficult than right now, but it's certainly still possible.
JS frameworks all too often use non-valid attributes to do their magic. You'd hope an editor wouldn't need to use those attributes within a rich text field, but I'm betting that will happen.
From my personal experience, all too often I've gotten really frustrated when a wysiwyg like ckeditor wrecks my markup by stripping out returns and tabs and classes and, heaven forbid it is necessary (but sometimes it is), inline styles. And while it may be possible to then dig into the innards of the configuration, hopefully in the UI, to prevent that from happening again, in the meantime a bunch of markup that I wrote now has to be rewritten.
So I'm not sure that when somebody tries to use a non-html5 attribute they'll instantly realize, hey, I can only use valid html5, and then they'll behave from here on out. Because that's not true, there's a whole lot of html5 attributes they can't use either. So the more likely outcome is that if they really wanted to do something, they dig into the config and add in the align attribute so things work again.
I don't know, maybe I'm wrong. I'm just not sure that if the explicit goals is to teach content editors not to use attributes that are no longer valid in html5, that this will do that. It will do other things, and maybe those things are good too.
Overall, if tags can be whitelisted, it probably makes sense that attributes can be whitelisted too.
Comment #10
Dave Reid@WimLeers The problem with Entity Embed is that we do not know in advance what all attributes could be used. We make it very possible for any arbitrary attributes to be added to the
<drupal-entity>
element. For example the image field formatters add in alt and title attributes. Another use case could be that something adds indata-field-name-override="Overridden value"
. I guess we could basically whitelist anything that starts with data-* in Entity Embed, but I'm not sure if that's possible.Comment #11
Wim Leers#10: But those formatters are run in a filter, right? So that filter should just run after
filter_html
.Comment #12
effulgentsia CreditAttribution: effulgentsia at Acquia commented@mdrummond, @Dave Reid: thanks for joining the conversation on this issue. It's really necessary to bring in multiple perspectives for the outcome to be a successful one.
Exactly. That's the bottom line. On D6 and D7, attribute whitelisting requires a contrib module (there are several popular ones to choose from), but my point in this issue is that with D8 core outputting HTML5, attribute whitelisting needs to be supported by core.
Right. Just like the default tag list is a small subset of valid html tags. If you want to add a
<table>
tag to the Restricted HTML format, you need to edit the format and do that. Similarly, if you want to add adraggable
attribute, edit the format and do it. Standard profile's default configuration should only include what we consider to be very broadly necessary.The difference is that for the person to do this, they need permission to edit that format's configuration. Which means they're acting in a "site building" capacity rather than a "content authoring" capacity. We can't stop site builders from making poor configuration choices. But we can help content authors generate content that complies with technical standards that we shouldn't require them to know.
HTML5 is the example I wrote the IS with, because that's the specification that a fresh core installation claims to comply with (via the doctype output in html.html.twig). But it's not just about that: it's about site builders being able to define the specifications of the site they build (HTML5 being one component of that). We shouldn't have to teach content editors about those specifications: the tools should automatically ensure that the content complies.
Comment #13
RainbowArrayI think this issue title better reflects how this is working.
Comment #14
effulgentsia CreditAttribution: effulgentsia at Acquia commentedOr even this.
Comment #15
davidhernandezReading through this, I agree with mdrummond's concerns. They are not unfounded. The high level goal here is a worth one, but this does seem to be some amount of babysitting. The example in the IS is a good one. I can imagine that exact scenario happening, and that editor not having any idea how to fix it. That's the big concern for me.
I do agree with effulgentsia in #12:
We have to always side with allowing the flexibility to do something, even if it isn't the default. Listen to the person, not the machine.
Did we get an answer as to why the whitelist has to be per tag? That seems cumbersome. And looking briefly at the patch, there are a bunch of tags that don't have any attributes whitelisted, and class is not allowed on anything?
Comment #16
Wim LeersBecause many attributes only make sense on specific tags. And perhaps you e.g. only want to allow images to be aligned, and not blockquotes.
We could support a
*
tag whose attribute whitelist would apply to all tags. E.g.<* id>
would allow any ID attribute on any tag.First, the specific default whitelist in the patch is just an example, it definitely needs scrutiny (aka bikeshedding).
Second, the
class
attribute is not whitelisted because many sites don't want to use classes. If you want to use classes, enable CKEditor's "Styles" button and add additional styles there; they'll become whitelisted automatically then. I.e. if you configurein CKEditor's Styles button configuration, then you'll effectively be whitelisting
<p class="callout"> <img class="hero">
. Allowing any class to be used is undesirable. If you specifically make classes available for the content editor, then you have to tell the text editor configuration about it, which well then automatically cause the text format (filter) configuration to be kept in sync.Comment #17
davidhernandezI think that would be an improvement.
Undesirable for whom? Why make these kinds of value judgements? If the editor is this restrictive out of the box, it would be a fail for users, especially new ones. People will not understand what is happening, and it could potentially increase the perception that Drupal is just too hard to use.
I understand that some sites want a very restricted editing experience, ones run by teams and with more structure, but not every site is like that. (Do we need this to look even more enterprise-like, with the perception that you really need a team of experts to even run a Drupal site?) I support having the ability for sites to create that level of restriction, to control the things their editors can do, but I would not force it upon people.
Just thinking about myself, I use one-off CSS classes all the time for something simple. Having to add them to the styles drop-down of the editor just to make sure they don't get filtered would be terrible. Those styles are only for things editors use frequently, and need easy access to. I would not want everything to appear there. And I don't want to have to change the editor configuration every time I need to use a class. The styles drop down also doesn't support multiple selections, as far as I know. I can't choose "Special subhead" and "Red" together. But, in any case, how would I whitelist that? Does every combination of class names have to be whitelisted?
....
After ranting to myself a bit I'm looking at this patch some more. I don't really understand enough about the filters, but I'm trying to figure some of it out. This looks like it is tied to the filter itself, and not the text format? So you are proposing adding these restrictions to the "Limit allowed HTML tags" filter, which is active for "Basic HTML" and "Restricted HTML", but not "Full HTML". Basic HTML being the default format used after install. That means changing your format from Basic to Full would turn off the filtering completely? Is that right?
If that is the case, that makes this less controversial. And it is a lot easier to tell people to change their text format than to figure out how to change their CKEditor configuration to support exact specifics of what they are doing. What I would prefer then is to see if this could be a separate filter instead of it being part of full_html, or an option of some kind. A separate "Limit allowed HTML attributes". That would make it easier to turn off and on by itself. Then we could argue about whether it should be on by default, but it would available for use.
Comment #18
gregglesI think the need here really depends on the site. We've seen cases on drupal.org where people abuse classes and ids to get styling on pages to make things look very different from the reality. For example, there was an installation profile a few years ago that manually made a download area that looked like the real one and was "green" and had download links on a completely different site that contained insecure versions of core/contrib. The real download area is (or was) smart enough to turn yellow if core was out of date, but this manually created table kept being green for long after.
I believe this would be part of the filter so advanced users who are trusted with more advanced filters could be trusted with more permissive sets of attributes and attribute values.
Further, some sites could get by with a wildcard (if only trusted users are inputting content), while others will want to strictly limit the attributes. That feels like a very Drupally solution to me.
Comment #19
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedBetter title, I think, based on the above discussion. Definitely if you're using Full HTML you should not have any restrictions, but for formats that use the HTML filter it makes sense to have this as an option -- in general it is a good principle to only give people access to what they need.
For example, if you paste some HTML from somewhere on the internet and it has class names that overlap with class names used on your site, you could accidentally get some styling that you don't want, etc.
I believe WordPress, for example, only allows unprivileged users (e.g. people leaving comments) access to a limited set of HTML attributes by default. In Drupal you currently need to install something like HTML Purifier or WYSIWYG Filter to accomplish this, which are good but a fair amount more complex than the filters in core.
We should consider this for backport to Drupal 7 also (although without the change in default behavior, especially for existing sites).
Comment #20
Wim Leers#17:
Correct.
Correct. And for a good reason: we don't want to actively encourage people from creating all sorts of crazy HTML and having articles output that.
Yes, that's right. And it's nothing new.
This has already been the case in Drupal 8 for a few years, since shortly after we have CKEditor. When using "Basic HTML", CKEditor will not allow you to use (or paste)
<table>
(just an example) unless the "Limit allowed HTML tags" filter allows that tag. In other words: CKEditor helps see content creators immediately what to expect, rather than happily showing any HTML and then filtering it away on output, which is extremely frustrating.Since Full HTML does not use the "Limit allowed HTML tags" filter, there are no restrictions, and hence you can do anything, including
<script>
tags. (Which is another reason why you should not ever allow non-admin users to use "Full HTML").Finally, this actually also is the case in Drupal 7 and before. The only new thing here is to also whitelist attributes. But the same exemptions in case of "Full HTML" apply: anything goes when using Full HTML.
#18: Thanks, great anecdotal examples of why this is useful.
#19:
Exactly!
Indeed, in that regard we are lagging WordPress. (Though WordPress still does its filter-on-input thing, see https://www.drupal.org/node/263002 for why Drupal does not.)
Now, let's look at things from a different perspective. Overall, I would personally say that Drupal really is about data modeling. You create content types, you add fields to them. You configure the fields' settings. This issue is following those same lines, and is addressing something we've overlooked for quite some time.
We currently allow any sort of crazy attributes to be set on tags (only
style
andon*
are disallowed). Which means you still can't reason properly about the data that Drupal stores, because the semantic vocabulary of markup that this particular site, with this particular use case, and this particular audience… is simply not defined. It's only defined coarsely, at the tag level.Comment #21
davidhernandezBut what you are proposing doesn't have much of a middle ground. It is either all or nothing. You are untrusted and have no access to tags or attributes or you are an admin user and can do anything. The patch doesn't apply any more so I can't check, but does this give us a configuration to choose the allowed attributes like the option we have to choose the allowed tags? I assume not based on the prior discussion of the whitelisting and wildcard. At a bare minimum that would be needed, in my opinion.
I don't see why that is a concern. It may be for some sites, but it certainly isn't for every site. Again, I'm not against functionality to give a site owner the ability, but we should be careful about forcing it down everyone's throat and making the interface more difficult to use.
Comment #22
Wim LeersDragging in the "Image" button would cause all attributes needed for that button to function correctly to be whitelisted, so it'd add
<img src alt data-align data-caption data-entity-type data-entity-uuid>
to be whitelisted.Adding additional styles to CKEditor (e.g.
p.callout|Callout
,p.intro|Introduction
andblockquote.choice-quote|Choice quote
) also cause the corresponding attributes to be whitelisted (in the examples given:<p class="callout|intro">
and<blockquote class="choice-quote">
.In other words: as you change the text editor configuration, the corresponding attributes that need to be whitelisted are updated automatically.
Yep, I totally agree with that sentiment, and I'm glad you're voicing these concerns :) I think we just may be doing a poor job of explaining this right now.
I'll get a green patch again.
Comment #23
davidhernandezI don't think this is flexible enough. I wouldn't want all classes, especially all block level ones, in the CKEditor config. I also wouldn't want to have to constantly update the configuration. The WYSIWYG is a tool to help build markup. It shouldn't be the gatekeeper. Not to mention some of the limitations it has doing something more than plain vanilla.
I presume that if the editor is turned off all the whitelisting goes with it? Adding the image button would whitelist the img tag and those attributes, so not having the button or the editor means those go away?
I'd like to also see some opinions from more content focused people. I know there are many circles that are trending away from being wysiwyg dependent. That could actually play either way.
Comment #24
pwolanin CreditAttribution: pwolanin at Acquia commentedDisucussed with Wim that we should try to allow specifying allowed attribute values using a space separated list - in other words just like you would write a class on html like:
to allow an A tag with any/all of the foo, bar, baz class values.
Comment #25
pwolanin CreditAttribution: pwolanin at Acquia commentedWe need to check that html entities in attribute values are correctly handled in filtering and restored in the output
I think it tends to decode entities when you fetch an attribute value from the DOM node.
Comment #26
pwolanin CreditAttribution: pwolanin at Acquia commentedPerhaps the conversion in attribute values of the basic XML entites like
&
isn't a problem, since it looks like the patch is also using DOM to extract the allowed values.We do need to fix the check for allowed attribute values sice it looks like the patch noly supports a single allowed value per attribute, but I expect things like class need to support multiple?
I think we also need a way to specify attribute names and values allowed for all tags? maybe something like
<global >
or<_attr >
or even<_ >
which seems to get through the parser?From the comments on the PHP manual page "if your DOMDocument was loaded from HTML, where element and attribute names are case-insensitive, the DOM parser converts them all to lower-case"
This seems to be true, and I see that reflected in the output of Html::serialize(), so that behavior is not changed by the patch, but something worth documenting perhaps?
Comment #27
pwolanin CreditAttribution: pwolanin as a volunteer commentedAttempting to reroll. This is missing some changes from HEAD to add H2 and H3 to the allowed tags list.
Comment #29
pwolanin CreditAttribution: pwolanin as a volunteer commentedoops - double "use" statement.
Comment #31
pwolanin CreditAttribution: pwolanin as a volunteer commentedAdd h2 and h3 back, and fix up test.
Comment #32
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThis was also mentioned in #9, #15, #16, and maybe some other comments. I agree with that being useful for some attributes (especially
id
anddir
), but let's also keep in mind that the list of global html5 attributes is very small.Comment #33
effulgentsia CreditAttribution: effulgentsia at Acquia commentedSpeaking of
dir
(#32), let's also change these to enumerate the allowed values of it (e.g.,dir="ltr rtl"
) per http://www.w3.org/TR/html5/dom.html#the-dir-attribute. Not sure if we want to add "auto" to that value list in our default configuration, given that spec's statement of "Authors are urged to only use this value as a last resort".Comment #34
effulgentsia CreditAttribution: effulgentsia at Acquia commentedAlso, wondering if it make any sense to include
dir
in our default config without also includinglang
on the same elements?Comment #35
pwolanin CreditAttribution: pwolanin as a volunteer commentedMaybe we should just add id, lang, and dir to the globally allowed list of attributes?
Comment #36
pwolanin CreditAttribution: pwolanin as a volunteer commentedHere's a start on globally allowing those 3.
Also fixes a logic bug in adding rel=nofollow
Clearly there is missing test coverage for that.
Comment #38
Wim LeersI rebased my local branch, which is identical to the patch in #5. The advantage is that I'm now able to still roll patches that contain multiple commits, which make this much, much easier to review.
This is identical to #5, but rebased, and with two additional commits: to import that changes in #31 and #37. One commit of #5 became obsolete: commit 5, because that was just a straight bugfix being included that was necessary here, and that bugfix was since committed: #2510138: Unidirectional editor configuration -> filter settings syncing breaks when button name does not equal the command name.
#31 looks fine, I have some questions for #37.
@pwolanin, you can apply these commits locally, so that you have the exact same branch! :)
Comment #39
pwolanin CreditAttribution: pwolanin as a volunteer commentedFixing test fails.
Comment #40
Wim Leers@pwolanin and I are now collaborating in a sandbox, easier than multi-commit patches like #5 and #38. See http://cgit.drupalcode.org/sandbox-dereine-2031809/log/?h=2549077-attrib....
This addresses #34.
Comment #42
pwolanin CreditAttribution: pwolanin as a volunteer commentedtest and logic fixes
Comment #43
Wim LeersAdded a small bit of missing test coverage.
Comment #44
Wim LeersGiven that id/lang/dir are now globally allowed and hardcoded attributes, update the migration process plugin accordingly. D7 now also has a filter format migration template, add a @todo to update that too.
Comment #45
pwolanin CreditAttribution: pwolanin as a volunteer commentedAdding more test coverage and test fixes.
Comment #46
phenaproximaFixed the @todo in the d7_filter_format migration.
Comment #47
pwolanin CreditAttribution: pwolanin at Acquia commentedMoving the real code to a method on the filter class, and starting a unit test specifically for the attribute code.
Also, tried to improve the performance of FilterHtml by saving the calculated restrictions array - this might make a difference if you are e.g. rendering a long comment thread and calling it multiple times. Also allows for the new method to be more stand-alone.
Comment #48
pwolanin CreditAttribution: pwolanin at Acquia commentedI think we can also remove some of the special casing for the rel=nofollow setting in FilterHtml::getHTMLRestrictions(), since that now is added after the other filtering.
Comment #49
pwolanin CreditAttribution: pwolanin at Acquia commentedHere's a little change to getHTMLRestrictions() that makes the settings parsing code 5x faster.
Quick microbench script and example output also attached.
Comment #50
dawehnerThis looks great!
Let's not use short names, there is just no reason to do so, unlike we are in fortran
I'm curious whether it would make sense to save some of the xpath queries and instead do the logic in php?
Should we better we strict here?
Any reason to not just go with str_replace, which is IMHO easier to read for simple usecases and a bit faster
It would be nice to document why id, lang and dir are always allowed. Why for example are classes not always allowed but IDs are? So yeah I wonder why this attributes are not just part of the global allowed attributes
Comment #51
pwolanin CreditAttribution: pwolanin at Acquia commentedper feedback from dawehner
Comment #52
pwolanin CreditAttribution: pwolanin at Acquia commentedAdditional tweak to use an xpath selector for any attribute to make the code a little simpler and reduce the number of loops.
Comment #53
regilero CreditAttribution: regilero commentedWe are talking of filtered content. I don't understand why we should allow 'id' attribute for contributed contents.
"id" should be a unique attribute in a final page. A regular website will usually use "id" on the layout composition, css theming for main blocs. But having some "id" in the dynamic content may lead to duplicates on the page (i know that browsers all kind enough with this, but by definition it is bad). Some bad usages of ids are replacing regular usage of classes, but forbidding classes and allowing ids will maybe push users to replace "class" classical usages with "id", that would be worst.
What are the real world usage of "id" in contributed contents (I'm maybe wrong on that, just that I do not see real examples) ?
Why would you need to whitelist this attribute?
Comment #54
pwolanin CreditAttribution: pwolanin at Acquia commentedI think we have some mismatches now with the docs on \Drupal\filter\Plugin\FilterInterface::getHTMLRestrictions()
Comment #55
pwolanin CreditAttribution: pwolanin at Acquia commented@regilero - it's very commonly used to make jump links - take a look at drupal.org handbook pages. Also, I discussed with @alexpott and Drupal 8 has removed most or all use of ID in CSS selectors.
However, I had been debating myself if there should be a setting to disallow ID as part of the filter.
Comment #56
effulgentsia CreditAttribution: effulgentsia at Acquia commentedFrom #19:
I think this applies to ID as well. If you copy content from another web page, what it used for an element's ID might overlap with an ID you already have on your page, and then you end up with duplicate IDs and thereby invalid HTML. So at a minimum I think it should be possible for site builders to opt-out of allowing ID attributes to their content authors. As to whether the default configuration should allow ID, ...
I think this is an area where WordPress might have already done a lot of good thinking on this. So, do they allow ID by default in their content inputs?
Comment #58
effulgentsia CreditAttribution: effulgentsia at Acquia commentedOne thought might be to allow ID by default in basic_html, but not in restricted_html, but again, I think we should research what WordPress does, just to have that as a data input to this decision.
Comment #59
regilero CreditAttribution: regilero commented@pwolanin, so a default filter policy allowing "id" for "a" tags only would make sense. But this 'withelist id for anchor' elements should be removable. In a perfect world I would like a filter allowing "id" only on anchors having no other attributes like "href", very sad that html is using anchors for both external linking and internal maps, that's a very different type of anchor.
Also about id usage in css, you cannot expect all themers to forbid usage of ids in css, even if D8 core is doing so, as "id" seems a pretty nice tool for layouts.
Comment #60
RainbowArrayYou can put id on any element to create an in-page anchor. Yes, ideally an id should not be used for CSS selectors, but there are other legitimate uses of id.
Comment #61
Wim LeersSo, I've been working on the JS. The challenge here is to keep #2510138: Unidirectional editor configuration -> filter settings syncing breaks when button name does not equal the command name working. The code that that issue introduced is very complex, because it needs to parse generic filter settings (in core's case only
filter_html
's) and generic text editor (in core's case only CKEditor) settings. And then compare the two, and smartly determine which tags, attributes and attribute values are missing.The problem is that this was never designed to work with attribute values in general, but only classes and styles. Making that work is a huge undertaking.
On the bright side: this allows us to make the JS match
\Drupal\filter\Plugin\FilterInterface::getHTMLRestrictions()
in all cases: far fewer data structures are necessary.But this is such a massive amount of changes on the JS side, after having worked on this for >20 hours in the past few days, the end is still not in sight. Due to the lack of JS tests, I'm also continuously breaking things, and so progress is very slow and EXTREMELY frustrating.
Hopefully somebody else is willing to take this over. Here's a patch that contains multiple commits on top of #44, but it should apply just as well to the latest patch.
Or, perhaps we can make it work with FAR less changes and just have it work for styles and classes initially? And break BC for this in 8.1, with a better, more generic JS API to do the editor->filter setting syncing?
My JS-fu is not strong enough anymore, this would take me several more DAYS, fulltime, to get it all to work again at the current pace. Not to mention I just want to kick my computer after having worked on this the entire day. I need a break from this, and I won't have the energy to work on this again tomorrow.
Comment #62
pwolanin CreditAttribution: pwolanin at Acquia commented@Wimleers - I think just supporting styles and classes in 8.0.x would be fine, as long as the editor JS will ignore other attributes.
Comment #64
RainbowArrayWait, so you're saying removing the ability for anything except classes and styles as attributes without the possibility of whitelisting other attributes?
If that's the case, I'd say that it's better to not do this at all. I certainly understand that this is complex and time-consuming. But I think this would be a huge frustration point to remove the ability to add attributes with no recourse except for Full HTML.
Comment #65
Wim Leers(Sorry for the unclear, emotional comment in #61 — it was a very frustrating day working on the JS side of this.)
No, I'm saying that we'll only do the automatic
of attribute values for theclass
attribute. Tag syncing and allowed attribute syncing will continue to work. But only the class attribute will get the attribute value syncing.That still allows you to whitelist specific attribute values for any attribute, it just won't happen automatically. But the 99% use case is whitelisting specific
class
attribute values anyway.Sorry for being so unclear!
Comment #66
effulgentsia CreditAttribution: effulgentsia at Acquia commentedSo as an example, you could either add
lang
as an attribute without listing values (which would mean, any value is allowed), or you could list values, likelang="en de"
if you only wanted those languages allowed. But, if you did the latter, and then used a CKEditor plugin that allowed adding a language selector to the toolbar, and that plugin had a UI for adding languages, this patch would not handle the automatic addition of those languages to your filter configuration: you'd need to edit the filter settings manually in addition to configuring the CKEditor plugin. I think that's acceptable. If at some point, someone with the JS chops wants to take on improving that, I think that can be a normal priority issue for 8.1, or maybe even 8.0.1.Comment #67
RainbowArrayI think based on my understanding, sounds good. As long as the allowed attribute list is configurable, which it sounds like it is, +1. Hope tomorrow is a better day Wim!
Comment #68
pwolanin CreditAttribution: pwolanin as a volunteer commentedLooking at \Drupal\filter\Plugin\FilterInterface it seems that the data structure for attribute values wasn't following the described structure - each value should be a key, and the value TRUE or FALSE.
Comment #69
pwolanin CreditAttribution: pwolanin as a volunteer commentedgrr
Comment #72
pwolanin CreditAttribution: pwolanin as a volunteer commentedOk, in response to comments above, taking out ID as globally allowed and going back to Wim's original approach of putting it just on H2 - H6 tags by default.
trying to figure out what's going with the lang attribute, chx pointed me to #1333730: [Meta] PHP DOM (libxml2) misinterprets HTML5
A couple points after looking around.
We seem to not be using the html5 library except in one test, and I can only assume it's much slower or has other problems.
This patch makes the html corrector filter pointless (redundant) if you are using the html filter, so we should possibly remove it if they are both, and change the UI text. from "Limit allowed HTML tags" to e.g. "Limit allowed HTML tags and Correct faulty HTML"
Comment #73
Dave ReidI'm a little uncertain about modules like Entity Embed which use data attributes on a
<drupal-entity>
element, but we don't know what attributes will be actually used at run-time, as individual display options/formatters can inject additional attributes to the element. Are all attributes starting with data- whitelisted by default? Is there any way to wildcard all attributes of a tag? Must we absolutely know what attributes are supported at all times?Comment #74
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThat sounds like something that's done during the Entity Embed filter (i.e., render the
<drupal-entity>
element into whatever the HTML display of that entity is, which is where formatters would come into play). Per #11, don't you need thefilter_html
filter running before the Entity Embed filter anyway? Since the rendered entity can end up being output with all sorts of tags not allowed by filter_html as well.If I'm right about the above, then what's the use case where you don't know ahead of time all of the data attributes that are possible on the
<drupal-entity>
element itself (prior to its rendering by Entity Embed)?Comment #75
Dave ReidYes, we definitely have filter_html running before the Entity Embed rendering filter.
Use case: I want to override a field on a node when embedding it. I form alter the Entity Embed dialog form to add:
This form value is automatically saved as
<drupal-entity ... data-my-field-override="Submitted value"></drupal-entity>
. While I believe whoever form alters this could also alter our filter plugin definition or anything else alterable, the same attribute is definitely *not* going to be added to our CKEditor plugin JS, since that's hard-coded to certain attributes only by Entity Embed (and has no way to alter them).Comment #76
Dave ReidWe *need* this flexibility to permit any additional arbitrary attributes to be added to entity embeds. It's critical to making sure that our project is successful in real-world use. We would need to be able to whitelist all attributes on an element (if not just anything that starts with data-*), and not just certain predefined ones.
Comment #77
Gábor Hojtsy@pwolanin asked me to look at this issue specifically re id support. I agree that supporting id, lang and other common attributes by default or making it very easy to configure their support would be important.
Comment #78
Wim Leers#68:
Yep, this was wrong, also fixed in #61.
#73:
Not in the current patch.
No, that'd largely defeat the purpose: it's likely that most sites would then end up allowing all attributes on some or all tags. That'd then make this entire patch pointless.
I think
data-my-field-override
is a very, very bad idea, because it can easily conflict with other HTML. IMO it should be namespaced to the Entity Embed module. So:data-entity-embed-my-field-override
. If we'd then specify<drupal-entity data-entity-embed-*>
, then it should work. On the CKEditor side it will already work. But on the PHP side (in the HTML filter), wildcards are not yet supported.The current installation instructions of Entity Embed say:
then that could be changed to:
… and in fact all that could happen automatically if you updated Entity Embed's CKEditor plugin to not specify
allowedContent: 'drupal-entity[*]'
andrequiredContent: 'drupal-entity[*]'
, both of which are very generic. Specify the necessary attributes explicitly and they'll end up being whitelisted. Seecore/modules/ckeditor/js/plugins/drupalimage/plugin.js
for an example.Hopefully this addresses your concerns :)
@pwolanin Thoughts about supporting
prefix-*
support for attribute whitelisting?Comment #79
pwolanin CreditAttribution: pwolanin as a volunteer commented@WimLeers - I was thinking about
prefix-*
as an important use case, thoguh clearly you can't really integrate that with CKEditor?In terms of the PHP code, it would be a little ugly or less performant, but certainly doable. XPATH seems to support
@*
but not@prefix-*
, so we need to use strpos or preg_matchComment #80
pwolanin CreditAttribution: pwolanin at Acquia commentedHere's with a first pass addition of * support for attribute names and values. Also change the name in the UI and removed htmlcorrector as a default filter.
Comment #81
Wim LeersLooks awesome at first sight!
I'll work on the JS tomorrow, as agreed in #61, #62, #66.
Comment #84
Wim LeersNow the JS is done!
I did a lot of manual testing.
My testing data set for the "Styles" dropdown settings:
Starting from Basic HTML, I added the "Styles" dropdown and then pasted in the above settings. Removed again. Then saved it with the first, second and fifth line. Adding just the third line and removing it again works as expected. Adding the fourth line and removing it again works as expected.
(Remember: once the text format is saved, any styles dropdown setting that is removed — or generally, any button that is removed — does NOT result in the HTML filter's setting being modified. Because in the mean time, content may have been created that depends on those tags/attributes/attribute values being allowed, and therefore we should always only *add* to the filter setting, not *remove*. Site builders can still remove things manually of course, but the automatic updating of the setting should never cause any problems. Hence this design decision, made a long time ago in #2510138: Unidirectional editor configuration -> filter settings syncing breaks when button name does not equal the command name, and still respected now.)
Comment #86
Wim LeersDrupalCI is green, PIFR is too slow (HEAD has already changed by the time it began testing).
Back to NR.
Comment #87
pwolanin CreditAttribution: pwolanin as a volunteer commentedNeeded a re-roll though for conflicts in D6 and d7 migration files. (new branch in sandbox: 2549077-attribute-filtering-rebase)
not sure I got it right - let's see about the tests.
Comment #90
Wim LeersFixed the incorrectly resolved conflicts (interdiff is relative to #87). Should be green again.
This is identical to #84, and just slightly smaller, because core already introduced part of the changes this needed in #2562073: Filter format migrations need to handle php_code filter.
Comment #91
Wim LeersTo address #61: #2567801: Deprecate core/modules/editor/js/editor.admin.js JS APIs in Drupal 10, for removal in Drupal 11. Also added
@todo
s.Comment #93
Wim LeersUnused.
This currently feels kinda mysterious. I think it'd be great to explain:
- that the first two lines do tag filtering, and for that we reuse the well-tested, battle-hardened
Xss::filter()
; the asterisk tag does not exist, so there's no point in passing that toXss::filter()
- after we've done tag filtering, we do attribute and attribute value filtering
The comment also applies to this if-statement, so I think it should come first.
I think we want a comment explaining this mysterious question mark :)
Nit: s/filter the attributes/filter the attributes and attribute values/
s/white list/whitelist/ ?
This exists mainly (solely?) because of whitelisted attribute values supporting wildcards. Would be great to have that mentioned in the docblock?
Nit: in earlier patches (e.g. #4), this also took into account the
filter_html_nofollow
setting. Also not the case in HEAD though, so it's fine to consider that out of scope.Nit: s/star/asterisk/Although this makes it sound much more epic, so scratch that :D
Nit: 80 cols violation.
Needs to be updated to match the current defaults (which includes e.g.
<h2 id>
).This test no longer makes sense.
The comment doesn't match the configuration.
There is a lot of duplication.
Seems like the duplication is a copy/paste error that removed the additional test coverage that is now missing?
See the test coverage I added in #43.
Comment #97
pwolanin CreditAttribution: pwolanin as a volunteer commentedFix for the 1 test fail.
Comment #98
pwolanin CreditAttribution: pwolanin as a volunteer commentedFix #93.1-10
Comment #99
pwolanin CreditAttribution: pwolanin as a volunteer commentedTrying to address the last 2 points.
Some of the test coverage (the CODE tag) was moved to the phpunit test.
Comment #102
pwolanin CreditAttribution: pwolanin as a volunteer commentedfix up format strings
Comment #104
pwolanin CreditAttribution: pwolanin at Acquia commentedRebase for minor conflicts in HEAD in core/modules/ckeditor/src/Tests/CKEditorTest.php
Comment #107
Wim LeersNW only for the first point, the rest is nits (and happy observations)!
Missing
id
attribute on the headers. We do add theid
attribute everywhere else: in the default text formats for the Standard install profile, and in the migration path for 6 and 7. So we should do so here as well, in the default settings for this filter.Nit: Two blank lines, should be one.
<3 wooly alpaca!
@pwolanin++++++++++++++++
ROFL
Nit: missing trailing period.
Comment #108
pwolanin CreditAttribution: pwolanin at Acquia commentedThanks - easy fixes.
Comment #109
pwolanin CreditAttribution: pwolanin at Acquia commentedWim asked me about hist comment in #6. In the patch I think we already have that in core/modules/filter/src/Plugin/migrate/process/FilterSettings.php?
Comment #110
Wim LeersThat's a migration path, not an upgrade path. I'm specifically referring to the
Needs upgrade path
issue tag, and what we need to do to be able to remove that.This also still needs a change record.
Comment #111
pwolanin CreditAttribution: pwolanin at Acquia commented@Wim Leers - ok, yes. I see what you mean. We should basically be able to use the same code? Or copy/paste it into an update hook?
Comment #112
pwolanin CreditAttribution: pwolanin at Acquia commentedAdds update function, test, and fixes debug change to a test class I left in the patch.
Comment #113
Wim LeersYes! Almost:
If the caption or alignment filters are enabled, we'll need their
data-
attributes to be allowed as well.Comment #114
pwolanin CreditAttribution: pwolanin at Acquia commentedok, adding handling for those too
Comment #115
pwolanin CreditAttribution: pwolanin at Acquia commentedoops, duplicate code from local rebase.
Comment #118
Wim LeersManually tested the upgrade path with content. Works great.
I've thoroughly reviewed the PHP. The PHP is ~90% different compared to the initial patch I wrote in #4. Peter has thoroughly tested the JavaScript and was not able to break it.
Even though I'd very much like more reviews, I do feel this is very important for Drupal 8 going forward, and it must land before RC, so: marking RTBC.
Nit:
Can be fixed on commit.Comment #119
Wim LeersComment #120
Gábor Hojtsy@pwolanin asked me to look at this patch. The concept makes sense. I spot-checked a few areas and things look fine. Even though the change is backwards compatible, I think it would be great to have a change record (as the existing tag says so as well :) for notifying people of the change.
Comment #121
susanb CreditAttribution: susanb commentedI am starting to write the change record.
Comment #122
susanb CreditAttribution: susanb commentedI wrote a draft change record, pls check it!
Comment #123
znerol CreditAttribution: znerol commentedI like that patch a lot, this makes HTML filtering a lot more robust. I Verified the new behavior manually in the browser. While reading the code I stumbled across some questions / nitpicks.
Wondering how risky this might be. I guess that if someone manages to poison `Drupal.settings`, then we have more serious problems?
Is there a specific reason why this work cannot be done on the server-side? This looks a lot like `FilterFormat::getHtmlRestrictions()`.
Great!
The list
type
attribute (for bothul
andol
) sometimes is restricted, sometimes not, sometimes has a value whitelist. Not sure whether or not it is important to be consistent here.Fourth?
s/the the/the
Maybe s/intentionally/explicitly/
Comment #124
droplet CreditAttribution: droplet commentedIn D8, we set CKEditor to use Class for alignment. Is it possible to config it to allow 4 classes only ?
Comment #125
Wim LeersMany thanks, @znerol and @droplet, for your excellent reviews!
<ul type>
AFAICT? I agree that it should be consistent.data-align
attribute is set. TheFilterAlign
filter then adds those classes upon filtering. But, you're right that those classes are used when specifically enabling CKEditor's alignment buttons (which are disabled by default because that is a layout decision that ideally a content creator should not be able to make, but specific sites can choose to opt in to that). In that case, dragging the alignment buttons into the toolbar on the CKEditor toolbar configuration page should already cause those classes to become whitelisted :) And if that is not the case, that's IMO a follow-up, because it doesn't impact default functionality.NW just for #123.3.
Comment #126
znerol CreditAttribution: znerol commentedIt looks a bit weird if Fourth comes out of nothing, I cannot see First, Second and Third anywhere.
Comment #127
droplet CreditAttribution: droplet commentedfew more:
1. Config doesn't sync in /admin/config/content/formats/manage/basic_html. Drag img icon out and still remain a `img` tag.
2. ESLint warning :)
3.
Assumed they used ID in h2 ~ h6 before this patch ?
Comment #128
pwolanin CreditAttribution: pwolanin at Acquia commented@droplet re: #127.1 this is by design (but maybe bneeds to be better documented). The JS only adds more allowed tags it never removes them from the format since you might have existing content with IMG tags (in this case)
#127.2 what's the warning?
#127.3 all attributes (including ID) were allowed before, so it makes sense to at least allow ID the same as the new default.
Comment #129
znerol CreditAttribution: znerol commentedRegarding #123.1: I realize that this piece of JavaScript code is used in the filter / editor admin screen only and not on content editing forms. No concerns from my side anymore.
Comment #130
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedre: #123.1 - we should possibly create a follow-up to also make sure the values are aligned in the form submit processing, instead of only relying on JS, but yes, it's not used on the node edit form only on the configuration form
laurii ran ESLint for me and found:
Comment #131
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedComment #132
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commented1 more JS lint fix.
Comment #133
lauriiiThis fixes all the remaining eslint errors.
Comment #138
znerol CreditAttribution: znerol commentedReading through the interdiff #123, #124 and #127 are addressed. Back to RTBC.
Comment #140
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedsimple conflict in core/profiles/standard/config/install/filter.format.restricted_html.yml
Comment #141
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedfor catch
Comment #145
Wim Leers#2573667: Entity uninstallation does not clean up field schema data in state properly already added
system_update_8008()
, rerolling with an incremented update ID.Comment #146
catchMostly nits:
Why would !featureName be false - could use an inline comment.
Three nested for loops is a lot - can we factor some out to a method and reduce the nesting?
This could also possibly be factored out to a helper - takes an HTML node and filters the attributes on it.
I'm imagining the space opera with llamas that someone is slowly writing into Drupal 8 source code.
Not sure what that comment means. If attributes were allowed it'd be an array instead of FALSE?
If style and on* are always disallowed, why are they specified here? It looks like you could flip the FALSE to TRUE and allow them but that's not the case.
Is this going to need a follow-up issue for the filter migration from 6.x/7.x too?
Comment #147
Wim LeersOnly addressing the JS & llama bits:
<p>
and<br>
). Basically, anything that doesn't have an associated command does not have a feature name by default. Documented.Comment #148
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedre:
#146.6 This is an expression of what the filter does that other APIs can read, so even if it's suggestive that it could be changed I don't think it's wrong. You also cannot save '*' settings, and if you tried the code should overwrite it here:
#146.7 There is already a new class in the patch to handle migrations of filter settings:
\Drupal\migrate\Plugin\migrate\process\FilterSettings
Comment #149
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedre:
#146.3 split this code into a protected helper method and renamed $node to $element to reflect the actual class we are working with.
#146.5 added a comment to the allowed section in the IF branch. In that case it's either TRUE or an array
Comment #150
catch#146 and #149 interdiffs both look good to me.
Comment #151
Wim Leers@pwolanin's changes in #149 look great, AFAICT all feedback is addressed, and my changes in #147 are only comments.
Back to RTBC.
Comment #153
catchOK. It looks like mdrummond's concerns were addressed, and while the code is complex, I'll take Wim's point that there are existing issues to simplify what's there in HEAD which would then simplify things here.
Since this is security hardening, and we may have a beta tomorrow, seems better to get this in earlier and tested rather than later.
I forgot to add effulgentsia to the attribution when committing, but added to the issue.
Committed/pushed to 8.0.x, thanks!
Comment #154
effulgentsia CreditAttribution: effulgentsia at Acquia commentedMoving to the 7.x queue per #19.
@David_Rothstein: the backport for this won't be entirely trivial, given the need to not break BC for existing 7.x sites. And this issue is already >150 comments. Therefore, would you prefer a new issue or keep using this one?
Comment #156
DuaelFrThat fix introduced a regression in 8.0.x
See #2578957: (regression) "Open in new window" checkbox does not work anymore
Comment #157
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI think it might be a (somewhat) straightforward backport actually? Mostly we'd just need to leave out a large part of the patch (including the update function) which wouldn't be relevant for Drupal 7.
The main actual difference is that for backwards compatibility we don't want to change the meaning of the 'allowed_html' setting in Drupal 7. So we'd need to leave that as is, and add a new setting ('allowed_html_attributes'?) that has the same syntax as Drupal 8's 'allowed_html'. The duplication would be a little unfortunate there, but not a huge deal since this is already a very advanced area of the admin UI anyway.
So if a tag appears in 'allowed_html' but not 'allowed_html_attributes', it's allowed with all non-style/on* attributes (backwards compatibility). If a tag appears in both, attributes are filtered. If it appears in 'allowed_html_attributes' but not 'allowed_html', it gets ignored (the tag is still forbidden).
I hope that makes sense and covers everything.
Comment #158
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedBy the way, did the Drupal 8 version of this patch miss changing the admin UI?
I'm looking at an up-to-date version of Drupal 8 right now, and the setting is still titled "Allowed HTML tags" with the description set to "A list of HTML tags that can be used. JavaScript event attributes, JavaScript URLs, and CSS are always stripped." That doesn't seem right anymore...
(for example, at admin/config/content/formats/manage/basic_html)
Comment #159
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commented@David_Rothstein for Drupal 7 I'd instead think you'd want a checkbox to enable attribute filtering. Then you could keep the same list of tags you have now, but if you enable attribute filtering the attributes are parsed from them as in this patch. I don't think it makes any sense to have 2 lists of allowed tags.
Comment #160
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedIf we stored it that way we'd still be changing the meaning of the 'allowed_html' setting - for example, you could no longer use explode() to get a list of the allowed tags.
But maybe we could use a checkbox like that in the UI, then change it to two separate settings on save.
Comment #161
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedcreated a follow-up issue to fix the UI text: #2579357: Fix text for "Limit allowed HTML tags" filter to also indicate it restricts HTML attributes
Comment #162
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commented@David_Rothstein - since the meaning of the setting is internal to the filter I don't understand why it matter that explode() would no longer work and you'd need ot use the technique that D8 has.
You think code external to the filter is trying to read this setting in D7?
Comment #163
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI don't see why it wouldn't. It's part of the public API, e.g. as returned by filter_list_format().
Comment #165
bneil CreditAttribution: bneil at The University of Iowa commentedI'm interested in working on the patch for D7, but would like more insight on which direction to go: Create a separate field in the UI for attributes, or go with the checkbox? Personally a checkbox seems like the best UX to me.
Comment #166
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented@bneil, that would be great if you're still interested!
My vote is for what I wrote in #160: A checkbox in the UI (since that's the best user experience), but then stored as two separate tag lists in the database (for backwards compatibility at the API level).
Comment #167
stefan.r CreditAttribution: stefan.r commentedJust bumping this as this would be good to get into 7.x
Comment #170
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedComment #171
Wim LeersPer changed policies, this needs a separate issue for D7.
Comment #172
Wim LeersComment #173
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedYes, but it is left open until that issue has been created and linked here.
Comment #174
pwolanin CreditAttribution: pwolanin as a volunteer and commentedHere it is: #2826458: Allow the "Limit allowed HTML tags" filter to also restrict HTML attributes, and only allow a small list of attributes by default
Leaving this open for now until we can capture any key comments about D7 into the issue summary there.
Comment #175
Wim LeersThanks for opening that issue, @pwolanin! I think that issue can still find those key comments while this issue is marked fixed. To ensure it, I left a comment: #2826458-1: Allow the "Limit allowed HTML tags" filter to also restrict HTML attributes, and only allow a small list of attributes by default.
Now finally closing this issue again.
Comment #177
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThis seemed harmless enough, but it broke an assumption in
text_summary()
. See #3067116: text_summary() returns malformed (not normalized) HTML for basic_html and other formats that use filter_html instead of filter_htmlcorrector.Comment #178
Mêlée CreditAttribution: Mêlée commentedCan users be given the choice of which tags are filtered from their code when using Ckeditor please.
Seems strange to remove a useful feature that was included with previous versions.