Adding any of the following to the 'Allowed HTML tags' produces a JS error and breaks functionality:

<thead>, <tr>, <tbody>, <td>, <tfoot>

This is due to the following in /core/modules/filter/filter.filter_html.admin.js:

sandbox.innerHTML = allowedTags[t];
node = sandbox.firstChild;

Any of these tags return null instead of a Dom element.

Files: 

Comments

b_sharpe created an issue. See original summary.

b_sharpe’s picture

Forgot to add <caption> to the list

I actually can't find any documentation as to why this is, and why only table elements, here is a simple example.

I've marked this as major as without these elements in the list, you cannot add a table using the WYSIWYG, but with them you cannot rely on the admin page JS functions (i.e. Custom styles won't auto populate)

phenaproxima’s picture

What is the JS error you get?

b_sharpe’s picture

Basically both just due to calling tagName on null:

Chrome - Uncaught TypeError: Cannot read property 'tagName' of null

FF - TypeError: node is null

phenaproxima’s picture

Status: Active » Postponed (maintainer needs more info)

I think this needs explicit steps to reproduce...

b_sharpe’s picture

Status: Postponed (maintainer needs more info) » Active
FileSize
135.08 KB

Steps to reproduce:

  • Navigate to text formats - /admin/config/content/formats
  • Manage Any format
  • Make sure the "Limit allowed HTML tags and correct faulty HTML" is enabled under filters
  • Delete everything in the "Allowed HTML tags" under "Filter Settings"
  • Open a browser inspector console
  • Put in any of the aforementioned tags (i.e. <tr>)
  • Remove focus from that field and note the error in the console

If you save that field, it will also now error anytime you manage that field. Screenshot also attached.

Wim Leers’s picture

Priority: Major » Normal

Patches welcome :)

maxocub’s picture

I don't know how to solve this yet, but I did some digging and I think I found the reason.

var allowedTags = setting.match(/(<[^>]+>)/g);
var sandbox = document.createElement('div');
var rules = {};
for (var t = 0; t < allowedTags.length; t++) {
  sandbox.innerHTML = allowedTags[t];
  node = sandbox.firstChild;
  tag = node.tagName.toLowerCase();
  ...
}

If we take <tbody> as an example, the code above is trying to set the <tbody> as the inner HTML of a <div>, which is not valid, so the node variable is then null.

I'm no JavaScript expert, but one way to avoid this error for table elements would be to have the sandbox be set as a <table> for table elements, but there must be a better way to avoid this kind of conditional treatment.

Wim Leers’s picture

#8: interesting! Good find — that seems plausible. I wrote that code, and @nod_ reviewed/RTBC'd it, but apparently there are edge cases where this fails. So… we'll need to come up with a different way to achieve what that part of the code does, since it apparently doesn't work in all cases.

Although I wonder if this is something that is browser-specific?

Wim Leers’s picture

Title: Allowed HTML tags Breaks with table elements » Parsing of allowed HTML tags (for integration with Text Editor configuration) throws JS error when adding <tbody> element
Issue tags: +JavaScript, +Needs JS testing
b_sharpe’s picture

@wim it's not browser specific, check out my fiddle from the #2. That's why I hadn't patched yet, I haven't been able to come up with a way short of a rewrite as you suggest.

I did test omitting them and just leaving <table> and can confirm this disallows tables, so ckeditor does need the tags to work.

b_sharpe’s picture

FileSize
3.05 KB

For reference, here is every tag that will fail based off of the list here:

"<!DOCTYPE>",
"<body>",
"<caption>",
"<col>",
"<colgroup>",
"<frame>",
"<frameset>",
"<head>",
"<html>",
"<tbody>",
"<td>",
"<tfoot>",
"<th>",
"<thead>",
"<tr>",

So the question is do we want to create a list of exceptions for tables and create them as children of a table node, then omit the non-table elements (is there a reason to allow frame/frameset?) or should it be rewritten to support all tags?

A simple work-around is to create the element itself, rather than create a div and then use the firstChild, but since we're dealing with only a few of many, it may not warrant creating a new DOM node for every tag rather than re-using the original div, so maybe something like the attached patch would suffice.

Wim Leers’s picture

Well… <!DOCTYPE>, <body>, <head>, <html> cannot possibly ever make sense in a text format. And I'd argue the same is true for <frame> and <frameset>.

So that then pretty much just leaves <table>-related elements. Which… maybe means we can special-case just those elements? We could make the parent <table> instead of <div> for those :)

droplet’s picture

It's a Public Security issue involved these functions: #2725255: Unfiltered data in "Allowed HTML tags".

b_sharpe’s picture

Status: Active » Postponed
Related issues: +#2725255: Unfiltered data in "Allowed HTML tags"

Adding related issue and postponing as this issue may not be relevant once the related issue is fixed.