WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#4873 closed defect (bug) (fixed)

visitor redirected to 404 after logging in for comment

Reported by: eightize Owned by: 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 7 years ago.
4873-trunk.diff (536 bytes) - added by Otto42 7 years ago.
Recreated diff for trunk
4873-222.diff (536 bytes) - added by Otto42 7 years ago.
Backported patch for 2.2.2
4873-2.2.diff (570 bytes) - added by westi 7 years ago.
Backport for 2.2
4873.001.diff (1.6 KB) - added by markjaquith 7 years ago.
urlencode() in the themes for login links.

Download all attachments as: .zip

Change History (32)

eightize7 years ago

comment:1 foolswisdom7 years ago

  • Milestone changed from 2.4 (next) to 2.3

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

Otto427 years ago

Recreated diff for trunk

comment:2 Otto427 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.

comment:3 ryan7 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

comment:4 markjaquith7 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

comment:5 westi7 years ago

Backporting sounds very sensible in this case.

Otto427 years ago

Backported patch for 2.2.2

comment:6 Otto427 years ago

Added patch for 2.2.2.

westi7 years ago

Backport for 2.2

comment:7 westi7 years ago

Snap two matching patches ;-)

comment:8 markjaquith7 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

comment:9 markjaquith7 years ago

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

comment:10 markjaquith7 years ago

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

comment:11 markjaquith7 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.

comment:12 westi7 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

comment:13 westi7 years ago

That should read that I have tested with

FF 2.0.0.6 and IE 6.0

comment:14 follow-ups: eightize7 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

comment:15 in reply to: ↑ 14 ; follow-up: westi7 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

comment:16 in reply to: ↑ 14 eightize7 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

comment:17 in reply to: ↑ 15 ; follow-up: eightize7 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/

comment:18 westi7 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.

comment:19 in reply to: ↑ 17 eightize7 years ago

Replying to eightize:

the live version is on Apache/1.3.37 Server php 4.4.4

comment:20 markjaquith7 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) "+"

comment:21 markjaquith7 years ago

Sorry, make that:

<?php

$test = $test2 = '+';

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

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

?>

comment:22 follow-up: markjaquith7 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.

markjaquith7 years ago

urlencode() in the themes for login links.

comment:23 markjaquith7 years ago

  • Keywords has-patch added; reporter-feedback removed

comment:24 in reply to: ↑ 22 westi7 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.

comment:25 follow-up: markjaquith7 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

comment:26 in reply to: ↑ 25 eightize7 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.

comment:27 westi7 years ago

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