Ticket #4873 (closed defect: fixed)

Opened 1 year ago

Last modified 1 year ago

visitor redirected to 404 after logging in for comment

Reported by: eightize Assigned to: markjaquith
Priority: normal Milestone: 2.3
Component: General Version: 2.3
Severity: major Keywords: has-patch
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/ 2. go to post a comment (you must be logged out first). 3. 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

pluggable.diff (163 bytes) - added by eightize on 08/30/07 15:26:23.
4873-trunk.diff (0.5 kB) - added by Otto42 on 08/30/07 15:32:23.
Recreated diff for trunk
4873-222.diff (0.5 kB) - added by Otto42 on 08/30/07 17:07:06.
Backported patch for 2.2.2
4873-2.2.diff (0.6 kB) - added by westi on 08/30/07 17:09:30.
Backport for 2.2
4873.001.diff (1.6 kB) - added by markjaquith on 09/01/07 21:47:13.
urlencode() in the themes for login links.

Change History

08/30/07 15:26:23 changed by eightize

  • attachment pluggable.diff added.

08/30/07 15:31:36 changed by foolswisdom

  • milestone changed from 2.4 (next) to 2.3.

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

08/30/07 15:32:23 changed by Otto42

  • attachment 4873-trunk.diff added.

Recreated diff for trunk

08/30/07 15:35:03 changed by Otto42

  • keywords changed from comment login redirect error to has-patch needs-testing 2nd-opinion.
  • priority changed from normal to high.
  • version set to 2.3.
  • severity changed from normal to major.

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.

08/30/07 15:42:26 changed by ryan

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

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

08/30/07 17:01:25 changed by markjaquith

  • status changed from closed to reopened.
  • resolution deleted.
  • milestone changed from 2.3 to 2.2.3.

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

08/30/07 17:06:33 changed by westi

Backporting sounds very sensible in this case.

08/30/07 17:07:06 changed by Otto42

  • attachment 4873-222.diff added.

Backported patch for 2.2.2

08/30/07 17:07:30 changed by Otto42

Added patch for 2.2.2.

08/30/07 17:09:30 changed by westi

  • attachment 4873-2.2.diff added.

Backport for 2.2

08/30/07 17:09:59 changed by westi

Snap two matching patches ;-)

08/30/07 17:38:48 changed by markjaquith

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

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

08/30/07 17:43:28 changed by markjaquith

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

08/30/07 18:13:49 changed by markjaquith

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

08/30/07 18:19:28 changed by markjaquith

  • priority changed from high to normal.
  • status changed from closed to reopened.
  • resolution deleted.
  • milestone changed from 2.2.3 to 2.4 (next).

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.

08/30/07 20:43:36 changed by westi

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

08/30/07 20:44:10 changed by westi

That should read that I have tested with

FF 2.0.0.6 and IE 6.0

(follow-ups: ↓ 15 ↓ 16 ) 09/01/07 16:19:48 changed by 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

(in reply to: ↑ 14 ; follow-up: ↓ 17 ) 09/01/07 16:23:15 changed by westi

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

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

(in reply to: ↑ 14 ) 09/01/07 16:25:47 changed by eightize

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

(in reply to: ↑ 15 ; follow-up: ↓ 19 ) 09/01/07 16:32:45 changed by eightize

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/

09/01/07 16:44:40 changed by westi

  • 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.

(in reply to: ↑ 17 ) 09/01/07 16:49:27 changed by eightize

Replying to eightize:

the live version is on Apache/1.3.37 Server php 4.4.4

09/01/07 21:00:27 changed by markjaquith

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) "+"

09/01/07 21:01:10 changed by markjaquith

Sorry, make that:

<?php

$test = $test2 = '+';

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

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

?>

(follow-up: ↓ 24 ) 09/01/07 21:46:29 changed by markjaquith

  • 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.

09/01/07 21:47:13 changed by markjaquith

  • attachment 4873.001.diff added.

urlencode() in the themes for login links.

09/01/07 21:47:49 changed by markjaquith

  • keywords changed from reporter-feedback to has-patch.

(in reply to: ↑ 22 ) 09/01/07 22:25:30 changed by westi

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.

(follow-up: ↓ 26 ) 09/01/07 23:05:11 changed by markjaquith

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

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

(in reply to: ↑ 25 ) 09/02/07 04:37:54 changed by eightize

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.

09/14/07 07:48:06 changed by westi

  • milestone changed from 2.4 to 2.3.