Skip to content

Commit

Permalink
XWIKI-21803: Unescaped '<' (less than symbol) in in-line style breaks…
Browse files Browse the repository at this point in the history
… the WYSIWYG editor

* Escape '<' in the content of style tags both for the editor HTML input and output, to overcome a bug in CKEditor's HTML parser.
* Allow style tags (inside macro output), they were filtered out before by CKEditor's Advanced Content Filter
* Add functional test
* Rename some functional tests to be consisten.

(cherry picked from commit 0f794c9)
  • Loading branch information
mflorea committed Jan 30, 2024
1 parent f8affb6 commit 9a9718a
Show file tree
Hide file tree
Showing 6 changed files with 132 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
* <li>submits only the significant content</li>
* <li>converts the font tags into span tags</li>
* <li>unprotect allowed scripts</li>
* <li>escapes the content of style tags to avoid breaking the HTML parser</li>
* </ul>
*
* @see http://docs.cksource.com/CKEditor_3.x/Developers_Guide/Data_Processor
Expand Down Expand Up @@ -162,6 +163,12 @@
htmlFilter.addRules(submitOnlySignificantContent, {priority: 5, applyToAll: true});
}

// We have to filter both the input HTML (toHtml) and the output HTML (toDataFormat) because the CKEditor HTML
// parser is called in both cases. Priority 1 is needed to ensure our listener is called before the HTML parser
// (which has priority 5).
editor.on('toHtml', escapeStyleContent, null, null, 1);
editor.on('toDataFormat', escapeStyleContent, null, null, 1);

// Transform <font color="..." face="..."> into <span style="color: ...; font-family: ...">.
// See https://ckeditor.com/old//comment/125305#comment-125305
editor.filter.addTransformations([
Expand Down Expand Up @@ -310,4 +317,31 @@
}
}
});

const domParser = new DOMParser();
function escapeStyleContent(event) {
let html = event.data.dataValue;
// Depending on the editor type (inline or iframe-based) and configuration (fullPage true or false) the HTML
// string can be a full HTML document or just a fragment (e.g. the content of the BODY tag).
const isFragment = !html.trimEnd().endsWith('</html>');
try {
const doc = domParser.parseFromString(html, 'text/html');
// We want to modify the input HTML string only if there is a style tag that need escaping.
let modified = false;
doc.querySelectorAll('style').forEach(style => {
const styleContent = style.textContent;
// Escaping the '<' (less than) character in the style content is normally not required but unfortunately
// CKEditor's HTML parser relies heavily on regular expressions to match the start / end tags and considers
// the '<' character as the start of a new tag, even when '<' is used inside CSS (e.g. with the content
// property). See https://ckeditor.com/docs/ckeditor4/latest/api/CKEDITOR_htmlParser.html
style.textContent = styleContent.replaceAll('<', '\\3C');
modified = modified || (styleContent !== style.textContent);
});
if (modified) {
event.data.dataValue = isFragment ? doc.body.innerHTML : doc.documentElement.outerHTML;
}
} catch (e) {
console.warn('Failed to escape the style tags from the given HTML string: ' + html, e);
}
}
})();
Original file line number Diff line number Diff line change
Expand Up @@ -39,20 +39,20 @@ class NestedSaveIT extends SaveIT
}

@Nested
@DisplayName("Image Plugin")
class NestedImagePluginIT extends ImagePluginIT
@DisplayName("Image")
class NestedImageIT extends ImageIT
{
}

@Nested
@DisplayName("TextArea Editor")
@DisplayName("TextArea Property Editor")
class NestedTextAreaIT extends TextAreaIT
{
}

@Nested
@DisplayName("Link Plugin")
class NestedLinkPluginIT extends LinkPluginIT
@DisplayName("Link")
class NestedLinkIT extends LinkIT
{
}

Expand All @@ -73,5 +73,10 @@ class NestedUndoRedoIT extends UndoRedoIT
class NestedLocalizationIT extends LocalizationIT
{
}


@Nested
@DisplayName("Filter")
class NestedFilterIT extends FilterIT
{
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
/*
* See the NOTICE file distributed with this work for additional
* information regarding copyright ownership.
*
* This is free software; you can redistribute it and/or modify it
* under the terms of the GNU Lesser General Public License as
* published by the Free Software Foundation; either version 2.1 of
* the License, or (at your option) any later version.
*
* This software is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public
* License along with this software; if not, write to the Free
* Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
* 02110-1301 USA, or see the FSF site: http://www.fsf.org.
*/
package org.xwiki.ckeditor.test.ui;

import static org.junit.jupiter.api.Assertions.assertTrue;

import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Order;
import org.junit.jupiter.api.Test;
import org.xwiki.test.docker.junit5.TestReference;
import org.xwiki.test.docker.junit5.UITest;
import org.xwiki.test.ui.TestUtils;

/**
* Tests how CKEditor filters the content.
*
* @version $Id$
*/
@UITest
class FilterIT extends AbstractCKEditorIT
{
@BeforeEach
void beforeEach(TestUtils setup, TestReference testReference)
{
edit(setup, testReference);
}

@AfterEach
void afterEach(TestUtils setup, TestReference testReference)
{
maybeLeaveEditMode(setup, testReference);
}

@Test
@Order(1)
void escapeStyleContent()
{
StringBuilder source = new StringBuilder();
source.append("before\n");
source.append("\n");
source.append("{{html clean=\"false\"}}\n");
source.append("<style>\n");
source.append("p::before {\n");
source.append(" content: '<';\n");
source.append("}\n");
source.append("</style>\n");
source.append("<div>inside</div>\n");
source.append("<style id=\"second\">\n");
source.append("div::after {\n");
source.append(" content: '<';\n");
source.append("}\n");
source.append("</style>\n");
source.append("{{/html}}\n");
source.append("\n");
source.append("after");

setSource(source.toString());
String html = this.textArea.getContent();
assertTrue(html.contains("<style>"), "Unexpected content: " + html);
assertTrue(html.contains("content: '<'"), "Unexpected content: " + html);

this.textArea.sendKeys(" end");
// Verify that the origial style content is preserved.
assertSourceContains(source.toString() + " end");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@
* @since 14.7RC1
*/
@UITest
class ImagePluginIT extends AbstractCKEditorIT
class ImageIT extends AbstractCKEditorIT
{

@BeforeAll
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
@UITest(extraJARs = {
"org.xwiki.platform:xwiki-platform-search-solr-query"
})
class LinkPluginIT extends AbstractCKEditorIT
class LinkIT extends AbstractCKEditorIT
{
@BeforeAll
void beforeAll(TestUtils setup, TestConfiguration testConfiguration) throws Exception
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ CKEDITOR.editorConfig = function(config) {
'$1': {
elements: {
// Elements required because the editor input is a full HTML page.
html: true, head: true, link: true, script: true, body: true,
html: true, head: true, link: true, script: true, style: true, body: true,
// Headings
h1: true, h2: true, h3: true, h4: true, h5: true, h6: true,
// Lists
Expand Down

0 comments on commit 9a9718a

Please sign in to comment.