WordPress.org

Make WordPress Core

Opened 10 years ago

Closed 9 years ago

Last modified 9 years ago

#6992 closed defect (bug) (fixed)

Quotes in comment name prevent "awaiting moderation" display

Reported by: thomasperi Owned by:
Milestone: 2.8 Priority: normal
Severity: normal Version:
Component: Comments Keywords: has-patch commit
Focuses: Cc:

Description

If a double or single quote mark (not the "curly" or "smart" version of either) is entered in the Name field of the comment form, the comment does not appear to the user who posted it until it has been approved.

Attachments (4)

6992.diff (2.4 KB) - added by regulatethis 10 years ago.
PHP4 compatible fix
6992.2.diff (2.3 KB) - added by regulatethis 10 years ago.
PHP4 compatible fix, made to behave more like PHP5's htmlspecialchars_decode (thanks Hirvine)
6992-php4.diff (590 bytes) - added by regulatethis 9 years ago.
New patch, using html_entity_decode in the wp_get_current_commenter()
6992.3.diff (2.3 KB) - added by Denis-de-Bernardy 9 years ago.
using the cookie's original value

Download all attachments as: .zip

Change History (33)

#1 @regulatethis
10 years ago

  • Component changed from General to Comments
  • Owner anonymous deleted

Reproduced in r9508

#2 @regulatethis
10 years ago

  • Keywords has-patch added

For some reason this diff isn't showing up properly, oh well.

I fixed this by running $comment_author through htmlspecialchars_decode, but only for the purposes of that prepare() call... I wasn't sure if $comment_author itself was used anywhere else, so to be safe I didn't modify it's value directly.

#3 @regulatethis
10 years ago

BTW, this is happening because it grabs the comment author's name from the comment_author_COOKIEHASH cookie, where it is urlencoded, and then attempts to stick it directly into a query... At some point (not entirely sure where), quotes in the string are turned into htmlspecialchars. This is great for displaying this on a page but doesn't work so well in a SQL query.

#4 @regulatethis
10 years ago

Has anyone taken a look at this?

#5 @regulatethis
10 years ago

Finally got the diff cleared up! :)

#6 @markjaquith
10 years ago

  • Resolution set to fixed
  • Status changed from new to closed

(In [9773]) Show pending comments to users with quotes in their name. props regulatethis. fixes #6992

#7 @ryan
10 years ago

(In [9825]) Revert [9773]. htmlspecialchars_decode() requires PHP 5.1. see #6992

#8 @ryan
10 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

@regulatethis
10 years ago

PHP4 compatible fix

#10 @regulatethis
10 years ago

I just lifted the code and cleaned up some formatting. It's licensed under the LGPL. Can this go in as is?

#11 @Hirvine
10 years ago

Actually, 'ENT_COMPAT' does NOT translated single-quotes. PHP says "ENT_COMPAT, Will convert double-quotes and leave single-quotes alone."

Also I wouldn't set $quote_style = null) instead use $quote_style = ENT_COMPAT) which makes it clear, ENT_COMPAT is the default option.

@regulatethis
10 years ago

PHP4 compatible fix, made to behave more like PHP5's htmlspecialchars_decode (thanks Hirvine)

#12 @ryan
10 years ago

For 2.7 I'd rather use htmlspecialchars_decode() only when it exists if we do do this at all. We're too close to RC to introduce a new compat function.

#13 @regulatethis
10 years ago

So do we push this back to another release or should I start looking for another solution? The only thing I can think of is to go back to where this value is encoded in the first place and change that from happening, but it seems like its encoded for a good reason so I don't know if that's the best idea.

#14 @ryan
10 years ago

  • Keywords early added
  • Milestone changed from 2.7 to 2.8

Let's postpone to 2.8 so we have time to do a full fix.

#15 @regulatethis
10 years ago

Does a "full fix" imply the usage of the compat function? It seems like that value _should_ be encoded normally. I'm definitely open to working on another angle, I just don't know what it could be :)

#16 @mrmist
9 years ago

  • Keywords needs-patch added; has-patch removed

Since this seems to be awaiting a fuller solution, it's not really has-patch any more.

#17 @regulatethis
9 years ago

So again, what's a "fuller" solution? Anyone have any input here? I'd be glad to work on this, but I'd need some suggestions, obviously my previous fix was not used.

@regulatethis
9 years ago

New patch, using html_entity_decode in the wp_get_current_commenter()

#18 @regulatethis
9 years ago

  • Keywords has-patch added; needs-patch early removed

#19 @regulatethis
9 years ago

  • Keywords needs-patch early added; has-patch removed

Actually, this is bad because the values are obviously displayed on the page at some point. Doh. So that last patch is garbage, I'll see if I can figure out something else. I'm starting to think that a better solution would be to scrap the SELECTs as noted for comments_template() in comments-template.php.

#20 @regulatethis
9 years ago

  • Keywords early removed

#21 @janeforshort
9 years ago

  • Milestone changed from 2.8 to Future Release

Punting due to feature freeze. Reconsider with next release.

#22 follow-up: @Denis-de-Bernardy
9 years ago

the real issue here is sanitize_comment_cookies(), which is a bit too overzealous on the comment author name. we should store its original value (which is correctly set in the cookie) for use in the database query.

@Denis-de-Bernardy
9 years ago

using the cookie's original value

#23 @Denis-de-Bernardy
9 years ago

  • Keywords has-patch commit added; comments quotes needs-patch removed
  • Milestone changed from Future Release to 2.8

#24 in reply to: ↑ 22 ; follow-up: @westi
9 years ago

Replying to Denis-de-Bernardy:

the real issue here is sanitize_comment_cookies(), which is a bit too overzealous on the comment author name. we should store its original value (which is correctly set in the cookie) for use in the database query.

We can just reverse the work in the one place we need the un-sanitised version.

#25 @westi
9 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [11220]) Decode the commenters name why looking up un-moderated comments. Fixes #6992.

#26 in reply to: ↑ 24 ; follow-up: @Denis-de-Bernardy
9 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to westi:

Replying to Denis-de-Bernardy:

the real issue here is sanitize_comment_cookies(), which is a bit too overzealous on the comment author name. we should store its original value (which is correctly set in the cookie) for use in the database query.

We can just reverse the work in the one place we need the un-sanitised version.

Well, unless you know a means to reverse the work done by filters, you're in for big surprises with the patch you just committed.

#27 @regulatethis
9 years ago

So can someone explain the actual problem here? We have a value stored in a cookie and we need to decode html entities in it when using it in a query. Seems straightforward. It's so basic in fact, that it is essentially the exact same fix that I provided *6 months ago*, except the function used is named differently.

Why is this such a big issue?

#28 in reply to: ↑ 26 @westi
9 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

Replying to Denis-de-Bernardy:

Replying to westi:

Replying to Denis-de-Bernardy:

the real issue here is sanitize_comment_cookies(), which is a bit too overzealous on the comment author name. we should store its original value (which is correctly set in the cookie) for use in the database query.

We can just reverse the work in the one place we need the un-sanitised version.

Well, unless you know a means to reverse the work done by filters, you're in for big surprises with the patch you just committed.

Nope.

We are not reversing the work done by the filters.

The same work is done on the comment author on the way into the db so those filters have already been applied to the value before it was stored in the cookie so they should have no effect.

We are reversing the esc_attr call in sanitize_comment_cookies function and that is all.

#29 @Denis-de-Bernardy
9 years ago

esc_attr() call, which, incidentally, has a filter hiding in the background...

leaving this closed, but see also #9934

Note: See TracTickets for help on using tickets.