CakePHP return url validator

Question

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);
    }

Answer 1

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.