Make WordPress Core

Opened 17 years ago

Closed 17 years ago

Last modified 17 years ago

#2858 closed defect (bug) (fixed)

Problem with wp_get_referer()

Reported by: tereshchenko's profile tereshchenko Owned by:
Milestone: Priority: normal
Severity: normal Version: 2.0.4
Component: Administration Keywords:
Focuses: Cc:

Description

It seems that because of new wp_get_referer() function I get redirected incorrectly in the admin "Options". When I save options, instead of going back to the same option page (like "General Options") it redirects back to the previous page.

  1. Go to "General Options"
  1. Go to "Writing Options"
  1. Click "Update Options >>"
  1. Redirected to "General Options", not the "Writing Options" as expected

Version I use is latest SVN of 2.0 branch.

Change History (17)

#1 @tereshchenko
17 years ago

As I look to the wp_get_referer() function, it looks like an obvious defect. It shouldn't be:

$_REQUEST['_wp_http_referer'], $_SERVER['HTTP_REFERER']

because if you will update several option pages one after another, _wp_http_referer will be kept the same, even if $_SERVERHTTP_REFERER? is valid and present. It should be:

$_SERVER['HTTP_REFERER'], $_REQUEST['_wp_http_referer']

In this case new referer will be preferred (if it exists).

#2 @tereshchenko
17 years ago

Actually it is even worse, check wp_referer_field() function:

$ref = ( false === wp_get_referer() ) ? $_SERVER['REQUEST_URI'] : wp_get_referer();

it will almost always output the return of wp_get_referer(), which in turn will be referer to the page that made the request. So even with my proposed fix it will redirect to the previous page if $_SERVERHTTP_REFERER? is not available (and will ignore $_SERVERREQUEST_URI?).

It is very confusing actually: the field _wp_http_referer should contain the referer, and probably wp_get_referer() should used some other first, which should contain $_SERVERREQUEST_URI? (since our goal is to get page URI, not the URI that referred to it).

#3 @tereshchenko
17 years ago

O.K., after reading the purpose of wp_get_referer() it seems to me that there is some mix up - when received by script:
_wp_http_referer will contain the referer to the page that made the request, not the referer to the script itself
_wp_original_http_referer - will containt the referer to the script itself, not the original referer to the requesting page

So probably _wp_http_referer and _wp_original_http_referer should be swapped or renamed (to avoid future confusion).

#4 @ryan
17 years ago

  • Version set to 2.0.4

[3919] should help. We definitely need to make more clear which functions should set the current page as the referrer and which should pass along the referrer.

#5 @ryan
17 years ago

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

(In [3920]) Have wp_referer_field() set the referer to the current page. fixes #2858

#6 @tereshchenko
17 years ago

Yep, that helped - thank you!

#7 @markjaquith
17 years ago

Yeah, my bad. That original patch wasn't quite ready for prime time. :-)

The intention was for _wp_http_referer field to contain the URL of the current page (the referer, to the page the form submits to). _wp_original_http_referer is supposed to be used for cases where you do something like this:

  • Start at A
  • Go to B
  • Do foo at B
  • Do bar at B
  • return to A

i.e. you're not going back one step, but back to a original step. So that's the one that should be passed along, and _wp_http_referer should always be the previous page.

#8 @matt
17 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Can someone confirm that these functions don't open us up to HTTP response splitting attacks?

#9 @ryan
17 years ago

I'm reading up on it.

#10 @masquerade
17 years ago

Nice call, Matt. For safety, we should strip out "\r\n" from $_REQUEST_wp_http_referer? just to be slightly paranoid. A link with _wp_http_referer with \r\n in it could be used to do an HTTP response splitting attack (although most likely anything that would refer is likely to be nonce protected anyways, but I haven't looked too deeply into the code to confirm that, and my bet is that the information would be used to refer backwards if a user was to hit "No", so yes, if my guess is correct (I'll dive into the code and check later), this does open us up. A simple return str_replace("\r\n", '', $ref) on line 864 of functions.php would solve the issue.

#12 @ryan
17 years ago

Note that wp_redirect already strip \r and \n. I think we also need to strip %0d and %0a.

Should we use wp_redirect() everywhere?

#13 @matt
17 years ago

Our approach in another part of the code is a whitelist of characters, which I believe is the safest approach as we never know what sort of weird unicode character + browser bug could make this an issue in the future.

This is the code we have in wp-login, I assume it is a comprehensive regex and I see no reason we shouldn't apply it to all places we send Location headers:

$redirect_to = preg_replace('|[^a-z0-9-~+_.?#=&;,/:]|i', '', $_REQUEST['redirect_to']);

#14 @robmiller
17 years ago

Doesn't the unescaped full-stop defeat the entire regex?

#15 @ryan
17 years ago

The only meta characters in a character class are : \ - and ]

I think that regex is safe.

#16 @ryan
17 years ago

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

I think this one is fixed.

#17 @(none)
17 years ago

  • Milestone 2.0.4 deleted

Milestone 2.0.4 deleted

Note: See TracTickets for help on using tickets.