HTML corrector filter has problems with unescaped CDATA and incorrectly closed tags

Without patch:

Hello! <script type="text/javascript">alert('hi');</script> Goodbye

No alert.

Hello! <script type="text/javascript">
alert('hi');
</script> Goodbye

No alert.

With patch:

Hello! <script type="text/javascript">alert('hi');</script> Goodbye

No alert.

Hello! <script type="text/javascript">
alert('hi');
</script> Goodbye

No alert.

Console log
..... I'm mostly getting this error in Firebug:

syntax error <![CDATA[\n

This in Chrome:

Uncaught SyntaxError: Unexpected token <

Note that it works fine with just a normal <script> tag.

If I read that correctly, it will only work on first level CDATA elements. We probably need/want to dive into the children.

Tidy is bundled with PHP5, but not enabled by default, which makes it unsuitable as a replacement.

That looks pretty much ok to me.

Why is any filtering run on a full HTML format anyway?

Works with both <script> and <script type="text/javascript"> now. Nicely done, Jacob.

Although "Full HTML" makes it seem like no filter is run on it, it seems sane to cleans the HTML and make sure it doesn't break the rest of the page. If we are to remove the feature, it should be in a seperate issue.

Patch review:

+++ modules/filter/filter.module
@@ -819,6 +819,15 @@ function filter_dom_load($text) {
 function filter_dom_serialize($dom_document) {

@@ -826,6 +835,38 @@ function filter_dom_serialize($dom_document) {
+ * Adds comments around <!CDATA section in a dom element

- This seems to be missing the word 'a'. That or 'section' should be 'sections' (plural).

- We should explain why/when this function is to be used.

+++ modules/filter/filter.module
@@ -826,6 +835,38 @@ function filter_dom_serialize($dom_document) {
+ * DocDocument::loadHTML makes CDATA sections from the contents of inline script
+ * and style tags.  This can cause HTML 4 browsers to throw exceptions.

DocDocument::loadHTML is related to this how? If this is a public API I think we should provide more context. Right now, the relation to DocDocument::loadHTML is unclear. I recommend we write: 'DocDocument::loadHTML in filter_dom_load()'.

+++ modules/filter/filter.module
@@ -826,6 +835,38 @@ function filter_dom_serialize($dom_document) {
+  foreach($dom_element->childNodes as $node) {

Missing space after 'foreach'.

+++ modules/filter/filter.module
@@ -826,6 +835,38 @@ function filter_dom_serialize($dom_document) {
+      // @see drupal_get_js()

Please be more specific.

+++ modules/filter/filter.module
@@ -826,6 +835,38 @@ function filter_dom_serialize($dom_document) {
+      $frag = $dom_document->createDocumentFragment();

We don't abbreviate variables. Please write $fragment.

Powered by Dreditor.

Reviewing this patch, I agree with Jacob that using saveXML() to clean-up broken HTML is a hack. To be honest, the proposed patch introduces a WTF but is necessary evil if we stick with saveXML(). I would be OK switching to Tidy, even if that is not always available. I have not investigated it deeply, but it will probably do a better and faster job.

I pinged sun, the filter system maintainer, to chime in.

(Off-topic: it seems like Tidy would also be useful for validating Drupal's output in SimpleTest. See tidy_error_count() for details.)

Looks good. I don't think that Tidy is an option - cheap/shared hosts may not have it, but HTMLCorrector filter should produce the same results on all Drupal sites. Bug reports would be insane to debug and test otherwise.

In addition to Dries' review:

+++ modules/filter/filter.module
@@ -819,6 +819,15 @@ function filter_dom_load($text) {
+  

@@ -826,6 +835,38 @@ function filter_dom_serialize($dom_document) {
+ * 

Trailing white-space here.

+++ modules/filter/filter.module
@@ -826,6 +835,38 @@ function filter_dom_serialize($dom_document) {
+ * DocDocument::loadHTML makes CDATA sections from the contents of inline script

DOMDocument::loadHTML

+++ modules/filter/filter.module
@@ -826,6 +835,38 @@ function filter_dom_serialize($dom_document) {
+ * @param $dom_document
+ * @param $dom_element

Missing @param description.

+++ modules/filter/filter.module
@@ -826,6 +835,38 @@ function filter_dom_serialize($dom_document) {
+ * @param $comment_start
+ *   String to use as a comment start marker to escape the CDATA declaration.
+ * @param $comment_end
+ *   String to use as a comment end marker to escape the CDATA declaration.

We should add a note about "Defaults to inline JS..." here.

+++ modules/filter/filter.module
@@ -826,6 +835,38 @@ function filter_dom_serialize($dom_document) {
+ * @return void

Please remove @return entirely.

+++ modules/filter/filter.module
@@ -826,6 +835,38 @@ function filter_dom_serialize($dom_document) {
+function filter_dom_serialize_fix_cdata_element($dom_document, $dom_element, $comment_start = '//', $comment_end = '') {

Could we replace "fix" with "escape" or similar?

+++ modules/filter/filter.module
@@ -826,6 +835,38 @@ function filter_dom_serialize($dom_document) {
+      // @see drupal_get_js()

Can we move this into the PHPDocblock of the function, please?

+++ modules/filter/filter.module
@@ -826,6 +835,38 @@ function filter_dom_serialize($dom_document) {
+      $embed_prefix = "\n<!--{$comment_start}--><![CDATA[{$comment_start} ><!--{$comment_end}\n";
+      $embed_suffix = "\n{$comment_start}--><!]]>{$comment_end}\n";

Actually it seems we don't do this yet for CSS, but it looks ok to do it here. Did you verify that this CDATA comment trickery really works the same for inline CSS?

The CDATA tag in drupal_get_js() doesn't use a space after $comment_start - would be good to remove it here as well.

+++ modules/filter/filter.test
@@ -1065,6 +1065,27 @@ class FilterUnitTestCase extends DrupalUnitTestCase {
+    $f = _filter_htmlcorrector('<script type="text/javascript">alert("test")</script>');

We should add at least 3 more tests:

1) SCRIPT without any attribute.

2) SCRIPT nested a bit deeper, e.g. DIV > P > A > SCRIPT

3) SCRIPT/STYLE with other nodes (text and/or inline) before and/or after it.

Powered by Dreditor.

I chatted with sun a bit and as the maintainer of some WYSIWYG modules, he feels it is an important filter. I'm not really in-depth with Drupal's WYSIWYG modules but apparently there are more support requests when this filter is not enabled/available. Based on that recommendation, I suggest we create a new issue: "Use PHP's Tidy component for the clean-up HTML filter feature" targetted for D8.

I committed #24 as it fixes a critical bug. Let's create a follow-up issue and mark this fixed. Settings to 'needs work' until that issue is created. Thanks Jacob.

I am still having this fail for anything other than trivial scripts. I don't think it's fixed.

The only way I can get it to work is removing the htmltidy from the filter chain. For example, try putting the twitter widget in a block: http://twitter.com/goodies/widget_profile

Without a proper doctype, the serialization to XML doesn't know that it cannot fold empty tags for some historical HTML tags (such as <script>).

Looks good.

As far as I can see, we're repeating this in various places (and we most probably also repeat that mistake all over again). Would it make sense to introduce a helper function? And perhaps even a variable for the doctype string, so users can eventually tweak this if required?

The fails in the test result:

Test name	Pass	Fail	Exception
CollapsedCore filters (FilterUnitTestCase) [Filter]	127	2	0
Message	Group	Filename	Line	Function	Status
HTML corrector -- tag closing.	Other	filter.test	1000	FilterUnitTestCase->testHtmlCorrectorFilter()	
HTML corrector -- Let proper XHTML pass thru.	Other	filter.test	1025	FilterUnitTestCase->testHtmlCorrectorFilter()	


CollapsedText summary (TextSummaryTestCase) [Field types]	106	10	0
Message	Group	Filename	Line	Function	Status
Generated summary "<p></p>" matches expected "<p />".	Other	text.test	363	TextSummaryTestCase->callTextSummary()	
Generated summary "<p></p>" matches expected "<p />".	Other	text.test	363	TextSummaryTestCase->callTextSummary()	
Generated summary "<p></p>" matches expected "<p />".	Other	text.test	363	TextSummaryTestCase->callTextSummary()	
Generated summary "<p></p>" matches expected "<p />".	Other	text.test	363	TextSummaryTestCase->callTextSummary()	
Generated summary "<p></p>" matches expected "<p />".	Other	text.test	363	TextSummaryTestCase->callTextSummary()	
Generated summary "<p></p>" matches expected "<p />".	Other	text.test	363	TextSummaryTestCase->callTextSummary()	
Generated summary "<p></p>" matches expected "<p />".	Other	text.test	363	TextSummaryTestCase->callTextSummary()	
Generated summary "<p></p>" matches expected "<p />".	Other	text.test	363	TextSummaryTestCase->callTextSummary()	
Generated summary "<p></p>" matches expected "<p />".	Other	text.test	363	TextSummaryTestCase->callTextSummary()	
Generated summary "<p></p>" matches expected "<p />".	Other	text.test	363	TextSummaryTestCase->callTextSummary()	

<p> is not a self-closing tag: refer to the XHTML DTD http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd

I've fixed the tests to not expect broken XHTML.

Sorry, guys, but <span> is not a self-closing tag either, in the XHTML DTD (Drupal serves up XHTML+RDFa 1.0, which is still XHTML).

Aha, found one last incorrect self-closing p tag.

With respect to the test with the self-closing span tag, I removed that test altogether, as it was testing for "Let proper XHTML pass thru", and as I do not know the reason for that test's inclusion, I could not come up with a suitable substitute for the test string "<span class="test" />". In general, there are too many possible proper XHTML strings, so we should not be testing for "Let proper XHTML pass thru" unless there was a bug that would not let it through and we are testing for regressions.

Ugh, I filed a duplicate issue at #766332: HTML corrector filter self-closes tags that should not be self-closed and worked on it for a while there - totally missed this one since the issue title was out of date :(

The approach here seems to be a lot better than the one I was working on; however, my patch has some more comprehensive tests that I think do a better job checking that this works. So, this patch takes the tests from my duplicate issue and combines them with the approach here.

As far as I understand we don't want that because it will convert e.g. even <br /> to <br></br>, but for those kinds of tags a single closed tag is preferred.

Look at the patch - you'll see I added a test to specifically make sure we don't let that happen :)

See also:
http://www.w3.org/TR/xhtml1/#C_2

Thanks, looks good.

Agreed. Committed to CVS HEAD. Thanks!

If this is committed, does that means this is fixed?

http://drupal.org/cvs?commit=352802