#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)
Change History (33)
#2
@
15 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
@
15 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.
#9
@
15 years ago
htmlspecialchars_decode
There's a Compat version of the function here: http://pear.php.net/reference/PHP_Compat-1.6.0a1/__filesource/fsource_PHP_Compat__PHP_Compat-1.6.0a1CompatFunctionhtmlspecialchars_decode.php.html#a18
#10
@
15 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
@
15 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.
@
15 years ago
PHP4 compatible fix, made to behave more like PHP5's htmlspecialchars_decode (thanks Hirvine)
#12
@
15 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
@
15 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
@
15 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
@
15 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
@
15 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
@
15 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.
#19
@
15 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.
#21
@
15 years ago
- Milestone changed from 2.8 to Future Release
Punting due to feature freeze. Reconsider with next release.
#22
follow-up:
↓ 24
@
15 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.
#23
@
15 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:
↓ 26
@
15 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.
#26
in reply to:
↑ 24
;
follow-up:
↓ 28
@
15 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
@
15 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
@
15 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.
Reproduced in r9508