Don’t forget to exit after a redirect

Published on and tagged with cakephp  problem  tip

Last weekend I discovered something I was not aware that it works that way: that code defined after a redirect is executed. A simple example:

class UsersController extends AppController
{
    function beforeFilter()
    {
        // not logged in, so redirect to the login page
        $this->redirect('/login');
    }

    function delete($id)
    {
        $this->User->delete($id);
    }
}

This example redirects _and_ removes the specified user even if you are not logged in (you just have to know the url to delete users). To fix that potential security hole you have to add an exit() after the redirect:

function beforeFilter()
{
    // not logged in, so redirect to the login page
    $this->redirect('/login');
    exit();
}

19 comments baked

  • scott lewis

    Wow. That’s rather frightening. Thanks for the heads up!

  • KesheR

    I have always worked supossing that redirect() doesn’t end the execution, so I don’t have to review my code :P

  • Bret Kuhns

    I figured out this very same thing the hard way as well. Maybe a documentation ticket is in order so others don’t have to go through the same problems…

  • Luksha Zaryl

    I’ve consulted this with Phpnut_ and he told me, that this beaviour was made intentionally. I was not able to dig why, but we are manned to exit our scripts manually.

  • Bret Kuhns
  • Markus Bertheau

    If you exit right away, no cleanup can be done, which could be a bad thing.

  • Dirk Janßen

    Thanx for the hint. I had the same security lack in one of my applications thinking that code following redirect would bot be executed. Stay blogging.

  • John Zimmerman

    We have had this discussion in the CakePHP Google group.

    Basically, as stated above, any cleanup code or other code that would need to run at the completion of the action call would never get called if exit() were called after a redirect.

    Granted that most of the time an exit() is fine, making redirect explicitly exit would reduce flexibility.

    On another note, instead of calling exit, it would be in better form to just return; with no value.

    This way if you decided later that you needed some sort of code to run after an action call, even after a redirect call you can.

  • ThinkingPHP and beyond » Hacking a commercial airport WLAN

    […] Meanwhile I want to share a little hack I did when I was waiting at the Atlanta airport. As most airports do these days, they have a wireless network there. Unfortunatly, they try to make you pay $7 for 24h, no matter how long you actually get on there. Since I didn’t want to get ripped off, I started playing around with the network. Using LiveHTTPHeaders for firefox, I was able to see that they were redirecting me to their portal via a 302 whenever I tried to access a public site. So the first thing I tried was to deactivate redirects in the about:config, and hoped they would send me the site I wanted after their redirection header. This might sounds stupid, but checkout the post on cakebaker talking about it if you are unfamiliar with the problem. Anyway, it didn’t help, I wouldn’t see any page at all, and instead get a firefox error message. So back to the beginning. […]

  • Martin Schapendonk

    Imagine a very lengthy operation: you could redirect the user to another page immediately and finish the operation in the background.

    That would not be possible with an immediate exit.

  • kain

    hi.
    I noticed that too; at first glance, some times ago, I was thinking the same thing with $this->render().
    I was pretty sure that after the ->render the code execution stopped. NO.
    so be sure to give an exit(); before ->render where is necessary.

  • Joaquin

    A couple of day’s ago I proposed in the IRC channel to change the definition of the redirect function to:

    function redirect($url,$status=null,$autoExit=false) {
    […]
    if ($autoExit) exit();
    return;
    }

    I haven’t checked if that really works (an exit call on a function ends the main sript execution), but it whould be a good solution to this “problem”

  • Troy Schmidt

    Yes a redirect or render doesn’t stop CakePHP from processing. Which like John pointed out is a good thing. You just have to be careful with this power. I like the return false. I had never thought of that and went to the exit() right away. as well. Now I see the error of my ways.

    Gosh I wish there was a CakePHP convention. Some place where we could get together and share the best programming practices in CakePHP. Things like this would be covered. Also more on the $this->data array and the great uses it has that are sorely underused. Like to pass data in a requestaction via the $this->data and not a bunch of method variables.

  • Evan Sagge

    So, I suppose a ‘return false;’ at the end of beforeRender() doesn’t stop the rendering of the current page? Coz’ I always thought this was the case. I may have assumed too much that CakePHP was largely based on Rails’ implementation.

  • » cakePHP: debug oraz redirect mount /dev/normanos

    […] Druga rada to też wynik postów na forum. Pamiętajcie aby po $this->redirect(’/kontroler/akcja/’); zawsze wstawić exit();! Brak exita to musi być dość powszechny błąd skoro nawet było o tym na piekarence […]

  • cakebaker

    @Joaquin: Open an enhancement ticket on trac.

    @Evan Sagge: I think a “return false;” in beforeRender() does not stop the rendering, but you have to test it.

  • cakebaker » A small change in Controller::redirect()

    […] it does an exit() by default, you can no longer encounter a possible security hole I described in Don’t forget to exit after a redirect (well, theoretically you can still encounter it when setting the $exit parameter explicitly to […]

  • Billee D.

    Old post, I know. But for the benefit of others I am posting to clarify something.

    I am certain that what John Zimmerman was suggesting (and correct me if I am wrong here, John) is that you would just return, not return false. For example:

    function redirect($url) {
        [redirect code here...]
        return;
    }

    That way you aren’t returning a value (like true or false), but you are simply returning processing back over to the calling code. Here is the initial definition of return from the PHP docs:

    “If called from within a function, the return() statement immediately ends execution of the current function, and returns its argument as the value of the function call. return() will also end the execution of an eval() statement or script file.”

    So returning no value (just “return;”) ends the execution of the current script and returns a NULL argument — which is okay since you wouldn’t need an argument to be returned for this example.

    Hope this helps to clarify things a bit. It is okay to return NULL. :-)

  • cakebaker

    @Billee: Thanks for your explanations!

Bake a comment




(for code please use <code>...</code> [no escaping necessary])

© daniel hofstetter. Licensed under a Creative Commons License