Problem with wp_get_referer()

[tereshchenko]

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"

2. Go to "Writing Options"

3. Click "Update Options >>"

4. Redirected to "General Options", not the "Writing Options" as expected

Version I use is latest SVN of 2.0 branch.

[tereshchenko]

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

[tereshchenko]

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

[tereshchenko]

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

[ryan]

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

[ryan]

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

[tereshchenko]

Yep, that helped - thank you!

[markjaquith]

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.

[matt]

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

[ryan]

I'm reading up on it.

[masquerade]

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.

[ryan]

​http://www.webappsec.org/lists/websecurity/archive/2006-02/msg00040.html

[ryan]

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?

[matt]

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']);

[robmiller]

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

[ryan]

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

I think that regex is safe.

[ryan]

I think this one is fixed.

[(none)]

Milestone 2.0.4 deleted