Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have the following snippet of code that redirects a user to a specific page on successful login to the application.

if(isset($this->params['url']['next'])) {
    $url = parse_url(urldecode($this->params['url']['next']), PHP_URL_HOST);
    if( $_SERVER['SERVER_NAME'] != $url || $url == false ) {
        $pathtoredirect = $this->Auth->redirect();
    } else {
        $pathtoredirect = urldecode($this->params['url']['next']);
    }
} else {
    $pathtoredirect = $this->Auth->redirect();
}
$this->redirect($pathtoredirect);

The idea is that if a query string named 'next' exists and has a valid hostname that matches the actual sever hostname then send the user to that location otherwise send them to the default location. It should also handle really bad formatted urls by the parse_url method returning false.

Can anyone see any issues with the above code? Or offer improvements?

Thanks

EDIT

The next parameter can be added manually for example on a login link, but is also created automatically on the redirect when a user tries to access a protected action.

The reason for adding the FULL_BASE_URL is because the session also contains the app url, so if your app is on a subdirectory e.g. domain.com/myapp/ and you try and access domain.com/myapp/protectedlocation you will get redirected to the login form with domain.com/myapp/login?next=/myapp/protectedlocation which will then send you off to: domain.com/myapp/myapp/protectedlocation adding /myapp twice! Doing the full url instead STOPS this from ever happening and doing the parse url prevents a malicious user from creating a login link with their own url as the return.

public function redirect($url, $status = null, $exit = true)
    {
        if($this->Session->read('Auth.redirect'))
        {
            $url.= '?next=' . urlencode(FULL_BASE_URL.$this->Session->read('Auth.redirect'));
        }
        $this->Session->delete('Auth.redirect');
        return parent::redirect($url, $status, $exit);
    }
share|improve this question
add comment

1 Answer

There is no need to urldecode() the url parameters, Cake does this for you. In fact, you are decoding twice, which is usually a bug.

$url actually contains the hostname. That is very confusing and you should rename that.

Also, that is really deep nesting for such a simple piece of code and a lot of code duplication.

I implemented pretty much the same functionality once.

If you only want to redirect to internal resources, maybe an option would be to only allow relative urls (e. g. http://example.com/login?next=/dashboard). That would save you a lot of trouble:

$defaultRedirect = $this->Auth->redirect();                                                                          

if(empty($this->params['url']['next'])) {                                                                           
    return $this->redirect($defaultRedirect);                                                                        
}                                                                                                                    

$path = $this->params['url']['next'];                                                                                 

if (strpos($path, '/') === 0) {                                                                                       
    return $this->redirect($path);                                                                        
}                                                                                                                    

$this->redirect($defaultRedirect);

This also works around the inconsistencies of $_SERVER['SERVER_NAME'].

Why are you expecting weird urls?

Consider matching the path to your defined routes if you want to redirect to actions of your application only.

share|improve this answer
    
The problem I had with using relative urls was that the next parameter can sometimes contain the app url itself, because it is populated from the session auth redirect. So if the app is a directory for example like: domain.com/myapp/login?next=/myapp/location`. And because $this->redirect() already assumes it's on the app and will prepend the passed return url with the app, so it would do /myapp/myapp/location and give a 404. Doing the FULL url for the return stops this from happening (see updated OP). And I do the check to stop someone trying to redirect a user to a bad location. –  Cameron Mar 6 '13 at 18:16
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.