Make WordPress Core

Opened 17 years ago

Closed 17 years ago

Last modified 17 years ago

#4873 closed defect (bug) (fixed)

visitor redirected to 404 after logging in for comment

Reported by: eightize's profile eightize Owned by: markjaquith's profile markjaquith
Milestone: 2.3 Priority: normal
Severity: major Version: 2.3
Component: General Keywords: has-patch
Focuses: Cc:

Description

When a directory in the path to wordpress contains an unusual character, such as +, the redirect after a visitor logs in to post a comment strips that character out of the path name, so user ends up at the 404 not found page.

to reproduce the error:

  1. make the directory for wordpress contain a + character, and set wordpress to require visitors to register in order to post comments.:

www.site.com/test+folder/

  1. go to post a comment (you must be logged out first).
  2. after entering username and password, the page is redirected to www.site.com/testfolder/?p=xx (the + is stripped).

i believe i resolved the error on my site by correcting the regular expression in wp_redirect() in the file wp-includes/pluggable.php.

a diff is attached to this post.

Attachments (5)

pluggable.diff (163 bytes) - added by eightize 17 years ago.
4873-trunk.diff (536 bytes) - added by Otto42 17 years ago.
Recreated diff for trunk
4873-222.diff (536 bytes) - added by Otto42 17 years ago.
Backported patch for 2.2.2
4873-2.2.diff (570 bytes) - added by westi 17 years ago.
Backport for 2.2
4873.001.diff (1.6 KB) - added by markjaquith 17 years ago.
urlencode() in the themes for login links.

Download all attachments as: .zip

Change History (32)

@eightize
17 years ago

#1 @foolswisdom
17 years ago

  • Milestone changed from 2.4 (next) to 2.3

eightize, you didn't tell us what version of WordPress you are using.

@Otto42
17 years ago

Recreated diff for trunk

#2 @Otto42
17 years ago

  • Keywords has-patch needs-testing 2nd-opinion added; comment login redirect error removed
  • Priority changed from normal to high
  • Severity changed from normal to major
  • Version set to 2.3

His problem applies to all current versions of WordPress, I found the code he posted in both trunk and 2.2.2.

I believe that his fix is the correct one, that regular expression did not escape some special characters. Could use verification by somebody more skilled in regexp knowledge than myself, though.

Anyway, attached patch for trunk based on eightize's incomplete diff.

#3 @ryan
17 years ago

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

(In [5986]) Escape special chars in regex. Props eightize and Otto42. fixes #4873

#4 @markjaquith
17 years ago

  • Milestone changed from 2.3 to 2.2.3
  • Resolution fixed deleted
  • Status changed from closed to reopened

We should probably backport this to 2.2.x and 2.0.x

#5 @westi
17 years ago

Backporting sounds very sensible in this case.

@Otto42
17 years ago

Backported patch for 2.2.2

#6 @Otto42
17 years ago

Added patch for 2.2.2.

@westi
17 years ago

Backport for 2.2

#7 @westi
17 years ago

Snap two matching patches ;-)

#8 @markjaquith
17 years ago

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

(In [5988]) Escape special chars in regex. Props eightize and Otto42. fixes #4873 for 2.2.3

#9 @markjaquith
17 years ago

(In [5989]) Escape special chars in regex. Props eightize and Otto42. fixes #4873 for 2.0.12

#10 @markjaquith
17 years ago

(In [5993]) Roll back [5986], [5988], [5989]. We are in a char class, so no escaping needed. Props mdawaffe. see #4873

#11 @markjaquith
17 years ago

  • Milestone changed from 2.2.3 to 2.4 (next)
  • Priority changed from high to normal
  • Resolution fixed deleted
  • Status changed from closed to reopened

So... that fix did nothing. + doesn't need to be escaped in that context.

If someone finds a fix and it's really obvious, maybe we can slip this into 2.3 -- but it's very much an edge case, so not a huge priority.

#12 @westi
17 years ago

I have tested current trunk (r5996) with a + in the dir name and cannot reproduce this issue.

PHP 5.2.4_pre200708051230-pl2-gentoo with Suhosin-Patch 0.9.6.2
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.6) Gecko/20070725 Firefox/2.0.0.6
IE 6.0

#13 @westi
17 years ago

That should read that I have tested with

FF 2.0.0.6 and IE 6.0

#14 follow-ups: @eightize
17 years ago

i appologize for my slow response. i am testing with development version (2.3-beta1). The same problem also occurs under 2.2.2. a sample url for a page that would not work is:

http://localhost/wp/tr+unk/wp-login.php?redirect_to=http://localhost/wp/tr+unk/?p=1

which will redirect to

http://localhost/wp/trunk/?p=1

#15 in reply to: ↑ 14 ; follow-up: @westi
17 years ago

  • Keywords reporter-feedback added; has-patch needs-testing 2nd-opinion removed

Replying to eightize:

i appologize for my slow response. i am testing with development version (2.3-beta1). The same problem also occurs under 2.2.2. a sample url for a page that would not work is:

http://localhost/wp/tr+unk/wp-login.php?redirect_to=http://localhost/wp/tr+unk/?p=1

which will redirect to

http://localhost/wp/trunk/?p=1

Ok.

Can you detail what Browser / WebServer / WebServer Platform you are testing on?

i.e. IE/FF/Opera Apache2/whatever Linux/Windows/OS X

#16 in reply to: ↑ 14 @eightize
17 years ago

btw: i'm using Firefox 2.0.0.6 browser on the 2.3-beta1, with Apache/1.3.33 (Darwin) PHP/4.4.7

#17 in reply to: ↑ 15 ; follow-up: @eightize
17 years ago

Replying to westi:

i have removed my patch from this live version, so you can see it also here (version 2.2.2):

http://www.eightize.com/turtle+mole/

#18 @westi
17 years ago

  • Owner changed from anonymous to westi
  • Status changed from reopened to new

Ah ha.

Can reproduce on your site.

It seems to be something to do with the parsing of the redirect info out of the url rather than wp_redirect itself.

Note to self... read the description very carefully.

I will look into this further.

#19 in reply to: ↑ 17 @eightize
17 years ago

Replying to eightize:

the live version is on Apache/1.3.37 Server php 4.4.4

#20 @markjaquith
17 years ago

i have removed my patch from this live version

Your patch, in my testing, does nothing. "+" gets through either way.

Try this:

<?php

$test = $test2 = '+';

$test  = preg_replace('|[^+]|', '', $test);
$test2 = preg_replace('|[^\+]|', '', $test);

var_dump($test);
var_dump($test2);

?>

You should get:

string(1) "+" string(1) "+"

#21 @markjaquith
17 years ago

Sorry, make that:

<?php

$test = $test2 = '+';

$test  = preg_replace('|[^+]|', '', $test);
$test2 = preg_replace('|[^\+]|', '', $test2);

var_dump($test);
var_dump($test2);

?>

#22 follow-up: @markjaquith
17 years ago

  • Owner changed from westi to markjaquith
  • Status changed from new to assigned

Okay, found the issue. It's an issue with the default themes, of all things.

@markjaquith
17 years ago

urlencode() in the themes for login links.

#23 @markjaquith
17 years ago

  • Keywords has-patch added; reporter-feedback removed

#24 in reply to: ↑ 22 @westi
17 years ago

Replying to markjaquith:

Okay, found the issue. It's an issue with the default themes, of all things.

That sounds like it.

I go nearly that far earlier before I had to go out

+1 to marks patch.

Not tested yet.

#25 follow-up: @markjaquith
17 years ago

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

(In [6006]) urlencode() redirect_to param in login links for both themes. fixes #4873 for trunk

#26 in reply to: ↑ 25 @eightize
17 years ago

Replying to markjaquith:

Thank you all. I've applied markjaquith's patch to my live version and it works (and removed my - ahem - 'patch'.). Sorry about my ignorant red herring. The instructions for posting specifically warn about that too.

#27 @westi
17 years ago

  • Milestone changed from 2.4 to 2.3
Note: See TracTickets for help on using tickets.