Configure CKEditor's "Advanced Content Filter" (ACF) to match Drupal's text filters settings

Given what is being specified in the callback, 'allowed_tags' is a misnomer. It's 'allowed_html'.

Aside from that, I need to think some more through this.

According to http://ckeditor.com/blog/CKEditor-4.1-RC-Released and http://docs.ckeditor.com/#!/guide/dev_advanced_content_filter, if allowedContent isn't specified, which is currently the case in Drupal HEAD, then CKEditor automatically defaults it to whatever is allowed by the enabled buttons and plugins. Therefore, I'm not immediately able to see how this issue is needed on security grounds. I tried the iframe and script attacks listed in the issue summary for all formats installed by Standard profile, and was not able to get CKEditor to run that XSS code. If someone is able to come up with an attack that works, please post it. Otherwise, I suggest we remove "security" from the tags and title, and downgrade this to a major.

In terms of the usability part of this issue (helping wys really be wyg), why do the approach taken here (setting allowedContent based on what the Drupal filter settings are) rather than enforcing consistency between the buttons/plugins and the Drupal filter settings in the first place? Wouldn't the latter be better UI in terms of consistency between how the administrator configures the toolbar and the toolbar that actually gets displayed?

@effulgentsia - were you testing in chrome or firefox? Some browsers will block scripts that appear to come "from the user".

I was testing in Firefox, but what I found wasn't just that CKEditor didn't run the code, it also stripped it from the content, so CKEditor was filtering, not the browser.

In terms of the usability part of this issue (helping wys really be wyg), why do the approach taken here (setting allowedContent based on what the Drupal filter settings are) rather than enforcing consistency between the buttons/plugins and the Drupal filter settings in the first place?

Our syncing between the Editor settings and the Filter settings is currently CKEditor-specific and will likely remain that way in Drupal 8. There is a layer of abstraction in there to allow other modules to update filter settings in the same way CKEditor does, but right now there's no such thing as a generic "Editor plugin" that would work with every WYSIWYG out there. Instead right now we only have CKEditor plugins, and any other module that provides an editor (or multiple editors) would handle its own plugins in its own way. This means that if we wanted to enforce filtering consistency between buttons/plugins and filter settings, each filter would need to integrate with each individual editor because we don't have a generic API for handling plugins across all editors. By using the approach Wim proposes here, each filter doesn't need to directly provide integration with any editor at all. The editor is responsible for utilizing the information provided by the filters and making its own configuration match. So basically, this shifts responsibility of integrating with a particular editor from the filter to the editor, and saves filter authors the need from needing to write an editor-specific integration.

Updated issue summary.

Since no one identified how this patch improves security relative to letting CKEditor use its default ACF, I'm adjusting the issue title, tags, and priority accordingly. I also updated the summary to reflect my current understanding of the problem space (in particular, that we're now discussing custom ACF vs. default ACF rather than the original summary that was discussing custom ACF vs. no ACF).

@quicksketch: thanks for #10; that was helpful. It helped inform the changes I made to the summary, but I left the consideration of supporting other editors out of the summary for now, for simplicity. If we end up needing those considerations to resolve this issue properly, we can update the summary again at that time.

+++ b/core/modules/filter/filter.module
@@ -1302,10 +1388,25 @@ function _filter_html_settings($form, &$form_state, $filter, $format, $defaults)
+function _filter_html_allowed_html($filter) {
+  $allowed_html = array();
+  $tags = preg_split('/\s+|<|>/', $filter->settings['allowed_html'], -1, PREG_SPLIT_NO_EMPTY);
+  foreach ($tags as $tag) {
+    $allowed_html[$tag] = TRUE;
+  }
+  return $allowed_html;

This is lessening the accuracy relative to CKEditor's default. _filter_xss_attributes() filters out all "on*" attributes, but that's not reflected here. As a result, this patch allows onclick in through the editor (go into source mode, type that, then switch back), whereas HEAD doesn't. It seems that CKEditor doesn't run onclick events on its content anyway, so this isn't necessarily a security problem, but it still seems to be going backwards, and I'm not clear how we fix that, since CKEditor's ACF doesn't support regex rules, from what I can tell.

+++ b/core/modules/filter/filter.module
@@ -495,6 +495,90 @@ function filter_get_filter_types_by_format($format_id) {
+    // From the set of remaining filters (they were filtered by array_filter()
+    // above), collect the list of tags and attributes that are allowed by all
+    // filters, i.e. the intersection of all allowed tags and attributes.
+    $allowed_html = array_reduce($filters, function($intersection, $filter) {

How would this work with something like Media module (and transformation filters in general)? A common Media configuration is to have the "Limit allowed HTML tags" filter not include <img>. But for the Media filter to run later, and convert the inline JSON codes into <img> tags. Meanwhile, on the CKEditor side, the plugin inserts <img> tags, which only on detach() does it turn back into the JSON codes. Wouldn't the above code disallow <img> tags from allowedContent (since it's computing the intersection) and prevent the plugin from working? Or does CKEditor now support a transformation API that could allow the content to always contain the JSON codes only, and therefore, the transformation to <img> would happen in a different part of its pipeline and be immune from allowedContent?

That could be a reason for wanting to postpone this issue until after ACF has blacklist support.

I'd like to comment more on #13, but for now, setting to "needs work", because I think we need more from the ACF in general. Not postponing though, because we can use this issue to clarify what Drupal needs, and help get that info to the CKEditor devs.

I've only read through about half the patch so far. Here are two things that stuck out:

- I don't think we need to be so elaborate in describing our tag blacklist as a "hack". I think it's a perfectly valid solution to provide a blacklist of our own by using a "instanceLoaded" event until CKEditor provides one natively. However maybe we should simply make it explicit rather than assuming filtered_html is being added. Couldn't we add a simple check server-side if "filtered_html" is present in the filter list and pass that to the editor's JS settings? I think it's only the assumption that it must be added that's the hacky part.

- filter_get_html_restrictions_by_format() has a return value of TRUE here, but the return value for what happens with TRUE is not documented:

+    if ($filter->getType() === FILTER_TYPE_HTML_RESTRICTOR && $filter->getHTMLRestrictions() !== FALSE) {
+      return TRUE;
+    }

+++ b/core/modules/ckeditor/js/ckeditor.jsundefined
@@ -98,8 +100,81 @@ Drupal.editors.ckeditor = {
+   * Drupal's default HTML filtering (the filter_html filter) also blacklists
...
+   * So this function hacks in explicit support for Drupal's filter_html's need
+   * to blacklisting specifically those attributes, until ACF supports

Small comment fix: 'to blacklisting' to 'to blacklist'.

After reading a couple times through this issue, the approach taken here seems correct. We do not want the filter/format system in Drupal to bend to the needs of editors (like CKEditor). Instead, Drupal should produce a standard, predictable representation of the content output and that editors will need to consume. If there is information loss in the exchange, we need to correct that loss at the editor level, as Wim has been pushing for with the CKEditor folks.

I walked through the JavaScript portion of the patch. It looks good. I added restrictive filters to each of the basic text formats in Drupal and walked through the a content item edit page load for each format. I'm able to add attributes to HTML elements except the attributes style and on*.

I don't feel I can give a sufficient review of the PHP portion of the patch that would allow me to RTBC this issue, but from the perspective the JavaScript, it's ready to go.

+++ b/core/modules/ckeditor/lib/Drupal/ckeditor/Plugin/CKEditorPlugin/Internal.phpundefined
@@ -265,4 +270,134 @@ protected function generateFormatTagsSetting(Editor $editor) {
+      $get_allowed_attribute_values = function() {
+        $values = array_keys(array_filter($attribute_values, $is_not_forbidden));
+        if (count($values)) {
+          return implode(',', $values);
+        }
+        else {
+          return NULL;
+        }
+      };
...
+                $allowed_styles = $get_allowed_attribute_values($attributes['style']);

Where does $attribtue_values come from here?! What am I missing?

Ie. how does $attributes['style'] used in the function?! It does not take an argument.

+++ b/core/modules/ckeditor/lib/Drupal/ckeditor/Plugin/CKEditorPlugin/Internal.phpundefined
@@ -265,4 +270,134 @@ protected function generateFormatTagsSetting(Editor $editor) {
+          // CKEditor only allows specific values for the "class" and "style"
+          // attributes; so ignore any other restrictions.

In contrast does Drupal allow for more restrictions? Its not clear here at all.

+++ b/core/modules/ckeditor/lib/Drupal/ckeditor/Tests/CKEditorTest.phpundefined
@@ -104,13 +108,22 @@ function testGetJSSettings() {
+    // Change the allowed HTML tags; the "allowedContent" and format_tags"
+    // settings for CKEditor should automatically be updated as well.

"format_tags"

+++ b/core/modules/filter/filter.moduleundefined
@@ -416,6 +416,148 @@ function filter_get_filter_types_by_format($format_id) {
+            // The tag is in the intersection, but now we most calculate the
+            // intersection of the allowed attributes.

most => must

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/FilterInterface.phpundefined
@@ -176,6 +176,74 @@ public function prepare($text, $langcode, $cache, $cache_id);
+   *     array(
+   *       'allowedTags' => array(
+   *         // Allows any attribute with any value on the <div> tag.
+   *         'div' => TRUE,
...
+   *   A simpler example, for a very coarse filter:
+   *     array(
+   *       'forbiddenTags' => array('iframe', 'script')
+   *     )
+   *
+   *   The simplest example possible: a filter that doesn't allow any HTML:
+   *     array(
+   *       'allowedTags' => array()
+   *     )

These are supposed to be wrapped in @code tags no?

+++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterAPITest.phpundefined
@@ -2,17 +2,19 @@
-use Drupal\simpletest\WebTestBase;
+use Drupal\simpletest\DrupalUnitTestBase;

Woot :)

+++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterAPITest.phpundefined
@@ -100,10 +109,78 @@ function testFilterFormatAPI() {
+    // Test on stupid_filtered_html, where nothing is disallowed.
+    $stupid_filtered_html_format = entity_create('filter_format', array(
+      'format' => 'stupid_filtered_html',
+      'name' => 'Stupid Filtered HTML',
+      'filters' => array(
+        'filter_html' => array(
+          'status' => 1,
+          'settings' => array(
+            'allowed_html' => '', // Nothing is allowed.

Nothing is allowed vs. nothing is disallowed :)

+++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterAPITest.phpundefined
@@ -100,10 +109,78 @@ function testFilterFormatAPI() {
+          'status' => 1,
+          'settings' => array(
+            'allowed_html' => 'p br a strong',

Wouldn't these be

,
, etc. Not just the names of the tags?

+++ b/core/modules/filter/tests/filter_test/lib/Drupal/filter_test/Plugin/Filter/FilterTestRestrictTagsAndAttributes.phpundefined
@@ -0,0 +1,41 @@
+ *   title = @Translation("Tag & attribute restricting filter"),

I don't think we use & *anywhere* like this(?) I'd just use good old "and" :)

+++ b/core/modules/filter/tests/filter_test/lib/Drupal/filter_test/Plugin/Filter/FilterTestRestrictTagsAndAttributes.phpundefined
@@ -0,0 +1,41 @@
+ *   description = @Translation("Replaces all content with filter and text format information."),

What? Replaces all content with information(?) it seems to return the text as-is passed in with no replacement, so does not seem like a correct description?

Thanks for the expanded test coverage and the separate interdiffs especially. Looks good to me!

Wim asked me to test this here and it solves the problem that I had. Thanks guys!

Relatively minor nits, but "needs work" at least on the incorrect checking of a never defined $allowed_values (see below).

+++ b/core/modules/ckeditor/lib/Drupal/ckeditor/Plugin/CKEditorPlugin/Internal.php
@@ -264,4 +269,145 @@ protected function generateFormatTagsSetting(Editor $editor) {
+        if (count($values)) {
+          return implode(',', $values);
+        }

Just a stylistic suggestion, but since the need to implode is an artifact of CKEditor's data format, I think it would be better to move it out of the closure and into where you actually set the variable further down in the function. Feel free to disagree though.

+++ b/core/modules/ckeditor/lib/Drupal/ckeditor/Plugin/CKEditorPlugin/Internal.php
@@ -264,4 +269,145 @@ protected function generateFormatTagsSetting(Editor $editor) {
+        else if ($attributes === TRUE) {

Though it makes no functional difference, Drupal's convention for PHP code is to use elseif as one word.

+++ b/core/modules/ckeditor/lib/Drupal/ckeditor/Plugin/CKEditorPlugin/Internal.php
@@ -264,4 +269,145 @@ protected function generateFormatTagsSetting(Editor $editor) {
+            'styles' => TRUE,
+            'classes' => TRUE,

I assume the reason this is needed is that CKEditor essentially ignores 'attributes' for 'style' and 'class'? In other words, if 'attributes' => TRUE is set, but 'styles' and 'classes' are left unspecified, then CKEditor removes all styles and classes? If so, then a comment to that effect would be helpful. If not, then a comment as to why this is actually needed would be helpful.

+++ b/core/modules/ckeditor/lib/Drupal/ckeditor/Plugin/CKEditorPlugin/Internal.php
@@ -264,4 +269,145 @@ protected function generateFormatTagsSetting(Editor $editor) {
+                $allowed_styles = $get_allowed_attribute_values($attributes['style']);
+                if (isset($allowed_values)) {

s/$allowed_values/$allowed_styles/. We need test coverage for this as well.

+++ b/core/modules/ckeditor/lib/Drupal/ckeditor/Plugin/CKEditorPlugin/Internal.php
@@ -264,4 +269,145 @@ protected function generateFormatTagsSetting(Editor $editor) {
+                $allowed_styles = $get_allowed_attribute_values($attributes['class']);
+                if (isset($allowed_values)) {

s/$allowed_styles/$allowed_classes/.
s/$allowed_values/$allowed_classes/. + test coverage.

+++ b/core/modules/ckeditor/lib/Drupal/ckeditor/Plugin/CKEditorPlugin/Internal.php
@@ -264,4 +269,145 @@ protected function generateFormatTagsSetting(Editor $editor) {
+            $allowed_styles = $get_allowed_attribute_values($attributes['style']);
+            if (isset($allowed_values)) {

And once more. (another hunk from the one 2 above).

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/FilterInterface.php
@@ -176,6 +176,79 @@ public function prepare($text, $langcode, $cache, $cache_id);
+   * @return array|FALSE
+   *   A nested array with the allowed tags as keys, and for each of those tags

Docs needs fixing for the fact that the first level of the array is 'allowedTags' or 'forbiddenTags'. And the 2nd level of the array varies between these two.

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/FilterInterface.php
@@ -176,6 +176,79 @@ public function prepare($text, $langcode, $cache, $cache_id);
+   *       'allowedTags' => array(

AFAIK, in Drupal PHP code, array keys are always lowercase (i.e., 'allowed_tags'). Perhaps except when we transfer them directly to JS, but that's not what we're doing here. Unless you can find a counter example, I suggest making that change here to match our conventions.

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/FilterInterface.php
@@ -176,6 +176,79 @@ public function prepare($text, $langcode, $cache, $cache_id);
+   *       'forbiddenTags' => array('iframe', 'script')

Ditto.

Thanks.

Committed to 8.x. Thanks!

+++ b/core/modules/ckeditor/js/ckeditor.jsundefined
@@ -98,8 +100,81 @@ Drupal.editors.ckeditor = {
+   * This is the only way we could get https://drupal.org/node/1936392 committed
+   * before Drupal 8 code freeze on July 1, 2013. CKEditor has committed to
+   * explicitly supporting this in some way.
+   *
+   * @todo D8 remove this once http://dev.ckeditor.com/ticket/10276 is done.

Do we have a d.o issue to do our part once the CK ticket is resolved? If no, can we file it, and if yes, can we link it here and in the summary? Thanks!