[upstream] Cannot set cell width or height because inline styles are stripped

Thanks for the bug report! It looks like CKEditor's Table plugin is not fully integrated with its Advanced Content Filter. The Basic HTML text format doesn't allow those attributes, so the CKEditor Table plugin shouldn't show those in its dialogs.

Assigning to the CKEditor team.

Trying to reproduce this bug I created my own text format(since Basic HTML doesn't have tables, and Full HTML does not reproduce it) which differed from Basic HTML only by having the table plugin.

I received a notification, that based on the plugin set I had chosen a few entries in the Allowed HTML tags were added. This configuration reproduced the bug, but when I modified the list of allowed tags so that it included <td style> everything worked fine.

How are these additional automatic entries determined?

#5: it looks at the requiredContent of a CKEditor plugin, and then whitelists that automatically.

And that's the whole point: if only <td> is whitelisted, then it'll mean that allowedContent will only allow <td>, not any classes, not any styles, not any attributes at all. Then the CKEditor Table plugin should respect that, but it does not.

See #1894644-2: Unidirectional editor configuration -> filter settings syncing for the history of how this was built, in tandem with the CKEditor team :)

Does anyone have any solution for this? I need to set width for td.

@Tade0 I added style to allowed tags attribute list for td. After that Ckeditor doesn't strip it out if you switch between source and preview mode. But after saving the content and editing it strips it out.

@saikatewu Perhaps the upstream bug has been fixed. Did you whitelist the necessary attributes in the "Allowed HTML tags" filter on the Drupal side?

Yes, I did. Still same issue.

I am having the same issue.

Having updated to 8.3.4 and whitelisted "style" in the "Limit allowed HTML tags and correct faulty HTML", I do observe that the style is no longer stripped when I toggle from source to wysiwyg, which was an issue before.

However, when I save, the style attribute is still stripped.

As an addendum, I changed the allowance in "Limit allowed HTML tags and correct faulty HTML" to <td border cellpadding cellspacing colspan rowspan style> and confirmed that only "style" is still stripped. The other 5 attributes maintain integrity.

Fix issue landed to our code base, and is scheduled to be release with version 4.8.0 🎉.

Great news! 👍 ❤️

Awesome!

Would love to see :)

This issue was reportedly fixed in CKEditor 4.8.0 in https://github.com/ckeditor/ckeditor-dev/issues/1014

CKEditor 4.8.0 was added to Drupal in #2926932: Update CKEditor library to 4.8.0

However, style properties on table cells (th, td) are still not preserved when saving. Should investigate if this is because of the implementation in Drupal or in CKEditor.

Screencapture:

Filter settings 'Allowed HTML tags':
<a href hreflang id name data-entity-type data-entity-uuid data-entity-substitution> <em> <strong> <cite> <blockquote cite> <code> <ul type> <ol start type> <li> <dl> <dt> <dd> <h2 id> <h3 id> <h4 id> <h5 id> <h6 id> <br> <span> <img src alt height width data-entity-type data-entity-uuid data-entity-substitution data-align data-caption> <p> <table><summary> <caption> <tbody> <thead> <tfoot> <th colspan rowspan style> <td colspan rowspan style> <tr> <p class></p>

It turns out this issue consists of bugs in both Drupal and CKEditor:

1. Drupal tells the CKEditor the style attribute is allowed in allowedContent, while these are always filtered when the 'Limit allowed HTML tags and correct faulty HTML' filter is enabled.
2. If Drupal tells CKEditor the style attribute is now allowed, several table 'Cell properties' can still be added through the UI in the CKEditor such as 'Vertical-align' and 'Background color'.

I will file a new issue for #1, so the scope of this issue is contained to #2.

Filed a CKEditor github issue: https://github.com/ckeditor/ckeditor-dev/issues/1986

I think the scope of this issue is actually #1, no?

Looks like \Drupal\filter\Plugin\Filter\FilterHtml will always strip out style attributes, possibly as a result of passing the filtered text through Xss::filter().

This problem also affects floating the cell content... really all cell property settings, I think.

How to allow td and th tag to have style attributes in Xss::filter?

I'm thinking to copy the Limit allowed HTML tags and correct faulty HTML filter into my custom module and modify to allow td and th to have style tag. But I'm not sure what to pass to Xss::filter.

I tried to add

$restrictions['allowed']['td'] = [
      'style' => true,
      'on*' => false,
      'lang' => true,
      'dir' => ['ltr' => true, 'rtl' => true],
    ];
    $restrictions['allowed']['th'] = [
      'style' => true,
      'on*' => false,
      'lang' => true,
      'dir' => ['ltr' => true, 'rtl' => true],
    ];

below $restrictions['allowed']['*'] = [ .. but doesn't seems to work.

@permanaj: If you allow style attributes you allow XSS attacks. You can switch off the XSS filter completely then, because it will become almost useless. See e.g.: https://stackoverflow.com/questions/4546591/xss-attacks-and-style-attrib...

CKEditor has a way of saying which style properties are allowed:

https://ckeditor.com/docs/ckeditor4/latest/guide/dev_allowed_content_rul...

It just seems that Drupal only allows us to say attributes. If we could do something like td{width,height} (notice the curly braces rather than square brackets) then it should only allow those particular CSS properties.

We'd also need to be able to tell the Drupal input filter that those style attributes are allowed as well.

@dsnopek: Yeah, I think your solution is the way to go. Of course it's quite difficult to fully and securely support all CSS. But I think the naive approach (explode by ";" and ":") should already be sufficient. We then get a key-value array we can filter by the allowed keys mentioned in the curly braces. Values should strictly be alphanumerical only. No parentheses, no calc(), no nothing. Anything else will be deleted - even if we allowed the key in the first place. The final result will be concatenated together. I think this method will be quite safe because you cannot do monkey-business with it.

The issue with allowing style tags has been noted above for XSS reasons. If you were to allow style tags within CKEditor and then saved the node, then the style attributes could still be stripped when editing again since Standard::filterXss() would be called if you were using the filter_html filter ("Limit allowed HTML tags and correct faulty HTML").

While I appreciate the difficulty of exhaustively filtering all possibly-safe CSS attributes, there is a significant subset of simple properties and values (e.g. width: 25%) that could be safely allowed without risk of XSS. I've added #3109650: Refactor Xss::attributes() to allow filtering of style attribute values to refactor the filtering code enough to allow filtering to be easier to implement as right now the attribute filtering is tough to extend. My proof-of-concept may be sufficient to block XSS attacks, but certainly it could be expanded upon to white-list just certain properties and a limited set of values for those properties.

thanks @adamfranco!

I'm seeing the originally reported behavior (style attributes getting stripped from table cells), as well, right now (D8.8.5) and I don't even have "Limit allowed HTML tags and correct faulty HTML" enabled.

Has anyone really figured out what the true cause(s) of this is/are? I can't identify what still remains as the source of the problem by reading through all these comments in this issue.

I'd really appreciate some guidance on how to actually add size attributes to a table cell via WYSIWYG and NOT have them get stripped by something.

@jptillman: at present, you can't using stock Drupal, because Drupal strips all style attributes for security, to prevent XSS attacks. See #3109650: Refactor Xss::attributes() to allow filtering of style attribute values for a possible patch and custom module solution.

sounds like there should be official drupal approved css classes to handle this

I've tried the patch as mentioned in #36. This works in ckeditor, but my style attributes are still stripped from the output unfortunately.

CKEditor has been removed from core, CKEditor 4 is removed from Drupal Core in 10.0.0