filter_xss() and Line break filter break HTML comments [#559584]

Here are the issues:
1. Using Filtered HTML input format comments are removed. I think it shouldn't do this or it should allow the !-- tag to be added (it doesn't do that either).
2. If the comments have some html tags inside, the result is even worse.  will result in comment -->. If my previous statement is arguable, now for sure something is wrong. It should either remove the comment or (ideally IMO) let it untouched.
3. Finally, using Full HTML will not strip the comment, but because of the line brake filter if you write

<!-- comment -->
<!-- comment <p>comment</p> -->

it will output in source view

<p><!-- comment --><br /><!-- comment
<p>comment</p>
<p> --></p>

. The problem is that on normal view you will see some empty lines and will not know why they are there.

The patch gives you the option to add <!--> as an allowed tags. If the tag is not allowed the comment is removed for good, with everything inside it, html code included (takes care of problem #1 and #2)
It ignores html comments when doing the autop processing (problem #3).

I removed the test because... it will not pass. DOMDocument treats comments as they are, which is text so it will not add a rel="nofollow" to a link inside a comment. That test now passes only because the comments tags are removed and the link is actually displayed (problem #2).

Comment	File	Size	Author
#12	drupal.filter-comments.12.patch	15.83 KB	sun
#12
#2	559584-3-html-filter-and-comments.patch	4.25 KB	tic2000
#2
	html-filter-and-comments.patch	4.24 KB	tic2000

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

28 August 2009 at 03:00

Status:

Needs review

» Needs work

The last submitted patch failed testing.

Comment #2

tic2000 CreditAttribution: tic2000 commented 28 August 2009 at 08:50

Status:

Needs work

» Needs review

File	Size
559584-3-html-filter-and-comments.patch	4.25 KB

Comment #3

Heine CreditAttribution: Heine commented 28 August 2009 at 09:00

Important backgrounds (duplicates?):

#69430-3: Comments containing HTML are broken (only leading tag is stripped)
#103563: HTML filter escaping html comments

Comment #4

31 August 2009 at 09:58

Status:

Needs review

» Needs work

The last submitted patch failed testing.

Comment #5

lilou CreditAttribution: lilou commented 31 August 2009 at 12:35

Status:

Needs work

» Needs review

HEAD is broken.

Comment #6

sun

German

Karlsruhe

CreditAttribution: sun commented 28 November 2009 at 15:05

Status:

Needs review

» Needs work

+++ includes/common.inc	28 Aug 2009 08:50:30 -0000
@@ -1503,11 +1505,20 @@
+  $comment = &$matches[4];
+  
+  if ($comment) {
+    $elem = '!--';
+  }
...
+  
+  if ($comment) {
+    return $comment;
+  }

Needs inline comments explaining what's being done here.

+++ modules/filter/filter.module	28 Aug 2009 08:50:30 -0000
@@ -954,7 +954,8 @@
       // Opening or closing tag?
-      $open = ($chunk[1] != '/');
+      $open = ($chunk[1] != '/' || $chunk[1] != '!');
+      $comment = (substr($chunk, 0, 4) == '<!--');

Likewise, inline comment needs to be updated.

+++ modules/filter/filter.module	28 Aug 2009 08:50:30 -0000
@@ -963,7 +964,7 @@
       // Only allow a matching tag to close it.
-      elseif (!$open && $ignoretag == $tag) {
+      elseif ((!$open && $ignoretag == $tag) || $comment) {

ditto, explaining special case of $comment

I'm on crack. Are you, too?

Comment #7

neochief CreditAttribution: neochief commented 7 December 2009 at 15:41

Status:

Needs work

» Closed (duplicate)

#222926: HTML Corrector filter escapes HTML comments

Comment #8

gpk CreditAttribution: gpk commented 1 February 2010 at 23:26

Status:

Closed (duplicate)

» Needs work

Re-opening per #222926-119: HTML Corrector filter escapes HTML comments. The point being that with the refactoring of the HTML corrector in #374441: Refactor Drupal HTML corrector (PHP5) it is a different beast in 7.x. Specifically,

1) in 6.x HTML comments get escaped, whereas in 7.x they get removed (i.e. #374441 partially fixed the problem)
2) this issue and #222926 need different approaches to fix them

Comment #9

rfay

English

Palisade, CO, USA

CreditAttribution: rfay commented 2 May 2010 at 23:27

Comment #10

markus_petrux CreditAttribution: markus_petrux commented 3 May 2010 at 07:34

Comment #11

sun

German

Karlsruhe

CreditAttribution: sun commented 19 August 2010 at 13:40

Title:	Handling of html comments in filter module	» Broken HTML comments due to filter_xss() and Filter module filters
Priority:	Normal	» Major

Better title. Major, as this is regression from D6.

Comment #12

sun

German

Karlsruhe

CreditAttribution: sun commented 2 September 2010 at 20:37

Title:	Broken HTML comments due to filter_xss() and Filter module filters	» filter_xss() and Line break filter break HTML comments
Status:	Needs work	» Needs review

File	Size
drupal.filter-comments.12.patch	15.83 KB

1) Apparently, there almost no tests for the Line break filter. Bad!

2) For filter_xss(), we have lots of XSS tests, but not a single assertion that verifies that non-XSS stuff stays as is. Bad!

3) FilterUnitTestCase contains old and duplicated tests for check_plain(), which is already tested elsewhere. If required, I'm going to move those clean-ups into a separate issue.