Make WordPress Core

Opened 15 years ago

Closed 15 years ago

Last modified 15 years ago

#8587 closed defect (bug) (fixed)

Wrong redirect after deleting comment...

Reported by: caspie's profile Caspie Owned by:
Milestone: 2.8 Priority: normal
Severity: major Version: 2.7
Component: Comments Keywords: has-patch developer-feedback
Focuses: Cc:

Description

Only when there are cyrillic letters in the post slug.

  1. Go to single post and hit Edit link on some comment.
  2. Delete the comment with the button from the "Status" section in "Edit Comment"
  3. It redirects but removes all the cyrillic letters from the post slug.

Check out the images.

Attachments (4)

1.jpg (128.5 KB) - added by Caspie 15 years ago.
Step 1
2.jpg (90.8 KB) - added by Caspie 15 years ago.
Step 2
3.jpg (86.6 KB) - added by Caspie 15 years ago.
Step 3
8587.patch (1.9 KB) - added by hakre 15 years ago.
url parameter values should be urlencoded…

Download all attachments as: .zip

Change History (25)

@Caspie
15 years ago

Step 1

@Caspie
15 years ago

Step 2

@Caspie
15 years ago

Step 3

#1 @ryan
15 years ago

  • Component changed from General to Comments
  • Owner anonymous deleted

#2 @hakre
15 years ago

That might be related with #8446

#3 @Denis-de-Bernardy
15 years ago

  • Milestone changed from 2.7.2 to 2.8
  • Severity changed from normal to major

nah, his sounds more like something is getting over-sanitized. It's almost certainly caused by r9592.

#4 @hakre
15 years ago

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

Tested. This is not reproduceable with 2.8 bleeding. Looks Fixed to me.

#5 @Denis-de-Bernardy
15 years ago

I'm definitely getting issues with a post titled:

Quisque общественных Dolor

But it looks more related to international permalink problems (covered in a separate ticket) than anything.

#6 @hakre
15 years ago

my subject line was: the cyrillic І К Л М Н Ѯ О П Ч
text was: make me wonder. the cyrillic І К Л М Н Ѯ О П Ч

will try to reproduce with your code (testing against 2.8 trunk).

#7 @hakre
15 years ago

Reproduceable with your text. Will take a closer look.

#8 @hakre
15 years ago

r9592 is not the problem. testing with that referrer: http://192.168.2.106/worpress-trunk/2009/04/29/quisque-%d0%be%d0%b1%d1%89%d0%b5%d1%81%d1%82%d0%b2%d0%b5%d0%bd%d0%bd%d1%8b%d1%85-dolor/comment-page-1/

and cleanurl and stripslashes do not make any probs with that.

anyway an attr() should be added on that line.

#9 @hakre
15 years ago

Referrer is written correctly into the comment edit form.

@hakre
15 years ago

url parameter values should be urlencoded...

#10 @hakre
15 years ago

  • Keywords has-patch developer-feedback added

Found the Bug. Zapped. Tested. Fixed. Please Review.

#11 @hakre
15 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#12 @automattor
15 years ago

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

(In [11120]) urlencode referrer. Props hakre. fixes #8587

#13 follow-up: @Denis-de-Bernardy
15 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

aren't we double escaping with attr(clean_url($url)) in r11120?

#14 @Denis-de-Bernardy
15 years ago

I thought that clean_url was turning & into & already. Re-close as fixed if not, else I believe that part needs to be reverted.

#15 @ryan
15 years ago

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

(In [11127]) No need to attribute_escape a cleaned url. fixes #8587

#16 follow-up: @hakre
15 years ago

shure that clean_url properly sanitizes user input injected via the referrer header?

#17 in reply to: ↑ 16 @Denis-de-Bernardy
15 years ago

Replying to hakre:

shure that clean_url properly sanitizes user input injected via the referrer header?

clean_url outputs what it's name suggests: a cleaned up url. :-)

#18 @hakre
15 years ago

clean_url is a gemischtwarenladen* for "whatever URL". kinda filtering but it does not know for what exactly it is filtering. it even ignores the most basic things of an URL as describben in RFC1738.

within wordpress it is widely used for attribute values but might be unfitting there. normally that means, it needs additional processing, like done with attr(). anyway, it is common sense with wordpress developing not to do so (right now), so please leave this buggy but closed.

*gemischtwarenladen: german metaphor, no idea in english. "pick and mix" attitude maybe?

#19 follow-up: @Denis-de-Bernardy
15 years ago

Let's open a separate ticket for that. My personal take would be that it should *not* encode the &. (In which case the attr() call becomes valid.) But it's a personal taste...

#20 in reply to: ↑ 19 @hakre
15 years ago

Replying to Denis-de-Bernardy:

Let's open a separate ticket for that. My personal take would be that it should *not* encode the &. (In which case the attr() call becomes valid.) But it's a personal taste...

yeah nothing to get clear for 2.8. & is already encoded. that is xhtml encoding of an URL.

#21 in reply to: ↑ 13 @hakre
15 years ago

Replying to Denis-de-Bernardy:

aren't we double escaping with attr(clean_url($url)) in r11120?

for the &, yes.

Note: See TracTickets for help on using tickets.