Ticket #6642 (closed defect: fixed)

Opened 8 months ago

Last modified 4 months ago

Commenters can break page validation via HTML comments

Reported by: schiller Assigned to: anonymous
Priority: normal Milestone: 2.6.1
Component: General Version: 2.5
Severity: normal Keywords: has-patch 2nd-opinion
Cc: rubys@intertwingly.net

Description

As per http://www.w3.org/TR/REC-xml/#sec-comments, XML does not like two dashes (--) in comments, nor does it like comments ending in --->. This should be fixed in kses

Attachments

bug6642.patch (475 bytes) - added by schiller on 04/08/08 17:37:10.
Patch for kses, prevents adjacent hyphens in a HTML/XML comment

Change History

04/08/08 17:37:10 changed by schiller

  • attachment bug6642.patch added.

Patch for kses, prevents adjacent hyphens in a HTML/XML comment

04/08/08 17:37:36 changed by schiller

  • cc set to rubys@intertwingly.net.

04/09/08 02:04:11 changed by Viper007Bond

wptexturize() converts a double dash into –, so no problems there.

04/09/08 12:16:47 changed by schiller

Can you clarify this? When is wptexturize() called? Is this something that has changed since WP 2.3.3?

04/09/08 12:58:21 changed by Viper007Bond

  • status changed from new to closed.
  • resolution set to worksforme.
  • milestone deleted.

No, wptexturize() has been around since at least version 1.5. All comments and posts are run through it by default before being displayed.

Log out and make a comment like this on your blog: 

This is a -- test comment over here --->

It will display at this valid XHTML: 

This is a — test comment over here —>

Closing as worksforme.

04/09/08 13:00:16 changed by Viper007Bond

Oh, and to answer your "When is wptexturize() called?" question, look at /wp-includes/default-filters.php. You'll find this line in it: 

add_filter('comment_text', 'wptexturize');

04/09/08 13:18:10 changed by schiller

Actually I had already confirmed this was indeed a problem - someone was logged out and made the following comment on my WP 2.3.3 blog:

Comment: <!-- foo -- bar -->

And it resulted in a Yellow Page of Death when rendered as XHTML. That's why I dug through and came up with this 2-line patch for kses.

Note that the comment stays hidden i.e. it actually stays a HTML comment it doesn't get escaped to be

Comment: &lt;!-- foo -- bar --&gt;

I do not have the "WordPress should correct invalidly nested XHTML automatically" checkbox checked (Options > Writing). Can you describe the settings on your blog that relate to translating markup?

04/09/08 13:29:54 changed by Viper007Bond

  • keywords changed from xhtml, kses to needs-patch.
  • status changed from closed to reopened.
  • version set to 2.5.
  • resolution deleted.
  • milestone set to 2.7.

Okay, well that's an entirely different issue. ;)

Confirmed that no-access users can post HTML comments, something that they shouldn't be able to do IMO. It's specifically allowed in the code though, so then I guess we should just make sure it doesn't break validation.

04/09/08 13:32:27 changed by Viper007Bond

  • summary changed from kses should not allow multiple hyphens in comments to Commenters can break page validation via HTML comments.

(follow-up: ↓ 10 ) 04/09/08 15:35:58 changed by schiller

Ok, thanks - I should have clarified between the two different types of comments ;)

I did attach a patch for this bug - does it need to get reviewed or something? (Just curious about your addition of the 'needs-patch' keyword)

(in reply to: ↑ 9 ) 04/10/08 01:13:47 changed by Viper007Bond

  • keywords changed from needs-patch to has-patch 2nd-opinion.

Replying to schiller:

I did attach a patch for this bug - does it need to get reviewed or something? (Just curious about your addition of the 'needs-patch' keyword)

Sorry, force of habit and I thought your patch merely removed all double dashes. It was in the wee hours of the morning and I didn't realize your patch was specifically targeted at HTML comments. My apologies.

Switched to the "has-patch" tag. :)

07/21/08 00:32:43 changed by azaozz

  • status changed from reopened to closed.
  • resolution set to fixed.

(In [8382]) Prevent adjacent hyphens in a HTML/XML comment. Fixes #6642 for trunk. Props schiller.

07/21/08 00:46:13 changed by azaozz

  • status changed from closed to reopened.
  • resolution deleted.
  • milestone changed from 2.7 to 2.6.1.

Re-open for 2.6.1

07/21/08 00:47:08 changed by azaozz

  • status changed from reopened to closed.
  • resolution set to fixed.

(In [8383]) Prevent adjacent hyphens in a HTML/XML comment. Fixes #6642 for 2.6.1. Props schiller.