Don’t forget to exit after a redirect

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

14 Comments

  1. Posted August 28, 2006 at 5:48 pm | Permalink

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

  2. KesheR
    Posted August 28, 2006 at 7:31 pm | Permalink

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

  3. Posted August 28, 2006 at 7:51 pm | Permalink

    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…

  4. Posted August 28, 2006 at 7:56 pm | Permalink

    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.

  5. Posted August 28, 2006 at 8:07 pm | Permalink

    https://trac.cakephp.org/ticket/1358

  6. Markus Bertheau
    Posted August 29, 2006 at 4:05 am | Permalink

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

  7. Posted August 29, 2006 at 7:03 am | Permalink

    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.

  8. John Zimmerman
    Posted August 29, 2006 at 7:33 am | Permalink

    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.

  9. Martin Schapendonk
    Posted August 31, 2006 at 7:41 am | Permalink

    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.

  10. Posted August 31, 2006 at 2:58 pm | Permalink

    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.

  11. Posted August 31, 2006 at 3:52 pm | Permalink

    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”

  12. Posted August 31, 2006 at 5:40 pm | Permalink

    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.

  13. Evan Sagge
    Posted September 4, 2006 at 6:50 pm | Permalink

    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.

  14. Posted September 6, 2006 at 10:06 am | Permalink

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


2 Trackbacks/Pingbacks

  1. […] 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. […]

  2. […] 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 […]

%d bloggers like this: