PHP Action Routing Done Wrong



  • Hi, long time lurker, first time poster!

    I am maintaining a moderate-size php app, written by another co-worker. I came upon this in index.php:

     if ($_SESSION['section'] != '' or isset ($_GET['action'])){

        if (!isset ($_GET['action'])){
            if ($_SESSION['view'] == 'list')
                include 'getlist.php';
            else if ($_SESSION['view'] == 'thumbnails')
                include 'thumbnails.php';
            else if ($_SESSION['view'] == 'editors')
                include 'editorsview.php';
        } else if ($_GET ['action'] == 'addImage')
            include 'addimage.php';
        else if ($_GET ['action'] == 'doAddImage')
            include 'doaddimage.php';
        else if ($_GET ['action'] == 'editDesign')
            include 'editelement.php';
        else if ($_GET ['action'] == 'doEditElement')
            include 'doeditelement.php';
        else if ($_GET ['action'] == 'editElements')
            include 'editelements.php';
        else if ($_GET ['action'] == 'doEditelements')
            include 'doeditelements.php';
        else if ($_GET ['action'] == 'doEditManyelements')
            include 'doeditmanyelements.php';
        else if ($_GET ['action'] == 'deleteElements')
            include 'deleteelements.php';
        else if ($_GET ['action'] == 'editInstance')
            include 'editinstance.php';
        else if ($_GET ['action'] == 'doEditInstance')
            include 'doeditinstance.php';
        else if ($_GET ['action'] == 'deleteInstance')
            include 'deleteinstance.php';
        else if ($_GET ['action'] == 'deleteImage')
            include 'deleteimage.php';
        else if ($_GET ['action'] == 'viewInstances')
            include 'viewinstances.php';
        else if ($_GET ['action'] == 'viewElement')
            include 'viewelement.php';
        else if ($_GET ['action'] == 'addElementFolder')
            include 'addelementfolder.php';
        else if ($_GET ['action'] == 'addCatalogue')
            include 'addcatalogue.php';
        else if ($_GET ['action'] == 'doAddelementToCatalogue')
            include 'doaddelementtocatalogue.php';
        else if ($_GET ['action'] == 'deleteelementFromCatalogue')
            include 'deleteelementfromcatalogue.php';
        else if ($_GET ['action'] == 'addSelectedToCatalogue')
            include 'doaddselectedtocatalogue.php';
        else if ($_GET ['action'] == 'deleteSelectedFromCatalogue')
            include 'deleteselectedfromcatalogue.php';
        else if ($_GET ['action'] == 'doAddCatalogue')
            include 'doaddcatalogue.php';
        else if ($_GET ['action'] == 'editCatalogue')
            include 'editcatalogue.php';
        else if ($_GET ['action'] == 'doEditCatalogue')
            include 'doeditcatalogue.php';
        else if ($_GET ['action'] == 'sendCatalogue')
            include 'sendcatalogue.php';
        else if ($_GET ['action'] == 'doSendCatalogue')
            include 'dosendcatalogue.php';
        else if ($_GET ['action'] == 'makeAll')
            include 'makeall.php';
        else if ($_GET ['action'] == 'deleteCatalogue')
            include 'deletecatalogue.php';
        else if ($_GET ['action'] == 'addAllToCatalogue')
            include 'addalltocatalogue.php';
        else if ($_GET ['action'] == 'prices')
            include 'prices.php';
        else if ($_GET ['action'] == 'doPrices')
            include 'doprices.php';
        else if ($_GET ['action'] == 'versioninfo')
            include 'versioninfo.php';
    }

    Pretty fancy, eh?



  • I've see worse...

    include $_GET['action'] . '.php';



  • Well, if you're bound to use [code]include[/code], I can see the reasoning behind this code. At least it's better then the bad ol' [code]include "$_GET[action].php"[/code] where every script kiddie's grandma can hit you over the head with code injection.

    I agree however that there are far better methods of doing this, like, maybe, I dunno... venturing into the strange and exciting world of ... HTTP redirects?

    [edit] argh ninja'ed... by ... eleven minutes... I bow my head in shame...



  • @PSWorx said:

    [edit] argh ninja'ed... by ... eleven minutes... I bow my head in shame...
    Eleven minutes isn't getting ninja'd. That's not even getting pirate'd.



  •  Yes, I wonder... what would be the best solution in this case?

     I was wondering about something like:

     

    $action = $_GET('action');
    if (preg_match ('/\W/', $action)) {
       include('incorrectpage.php');
    } else {
       include("${action}.php");
    }
    

    Is it enough injection protection, or is it a soon-to-be WTF? :)



  • I thought something like:

    if (isset($arrayOfValidActions[$action])) include("$action.php");



  • @PSWorx said:

    I agree however that there are far better methods of doing this, like, maybe, I dunno... venturing into the strange and exciting world of ... HTTP redirects?

    Also, surprisingly many people get a moment of mysterious enlightenment when you tell them "you know, form action= parameter doesn't have to have value 'index.php'. If you're calling code in edit.php, why the hell don't you just refer to edit.php directly in the form?"

    Sometimes action routing like this is feasible, but not if it's just include()ing stuff, what's the point... If the code is already split in zillion separate scripts, one per action, why bother with action routing in the first place?

    If there's "god classes" in OO, well, I guess "god scripts" are almost as bad, especially if it's a man-behind-the-curtain kind of a god...



  • @dstozek said:

     Yes, I wonder... what would be the best solution in this case?

     I was wondering about something like:

     

    $action = $_GET('action');
    if (preg_match ('/\W/', $action)) {
       include('incorrectpage.php');
    } else {
       include("${action}.php");
    }
    

    Is it enough injection protection, or is it a soon-to-be WTF? :)

    This is a WTF.  Do not do this.  You should at the very least be checking against a list of known actions.  Also, cut it out with include().  Only use require(): the script should fatal if the file is not found. 



  • I use a more conventional web-MVC way of routing, following the concept of /module/controller/action.  The modules are high-level groupings contained in one directory, the controllers are classes and the actions are methods.  The framework I wrote is very light and just takes the URI and hostname, converts to a module/controller/action and checks for the proper class file which is then require()'d.  Then the existence of the action method is checked which is called if it exists.  If anything is missing, the error controller 404 action is invoked.  One nice thing about such a setup is that you can do lots of internal routing by calling back to the main router with a different module/controller/action.  This does away with a lot of extraneous HTTP redirects and makes the whole thing a bit easier to follow.



  •  another less  wtf-y replacement for this is having an interface for page classe and then action= the class name of the appropriate page class

     

    you could even have some class-autoload hooks in the background to load the appropriate php file containing the class as needed so you don't have a huge list of include/require/require_onces hiding around somewhere

     

    as for going to /foo.php instead of /index.php?pg=foo i think using mod_rewrites on a apache you go do with this a rule that to the effect of 

    /(*)\.php(.*)/i

    if \1 != index then rewrite with index.php?pg=\1&\2

    else don't modify



  • $actions = Array('foo', 'bar', 'baz');

    if(in_array($actions, $_GET['action'])) require("$_GET[action].php");

    else

    {

        //do something about it

    }



  • Hey, thanks for the feedback! I was interested in your opinions on this one.



  • $actions=array('end'=>'thesecodedumps.php');
    if(isset($actions[$_REQUEST['act']])) {
    require $actions[$_REQUEST['act']]);
    } else {
    require '404.php';
    }



  • I've tried the array thing (map values to files) way back and what I don't like about it is the maintenance every time you need to add/remove a fie. My current setup takes the action value, sanitizes it, sticks a php extension at the end of the name, checks that the file actually exists and serves the page. 



  • What henke37 said.

    You need an associative array because of

        else if ($_GET ['action'] == 'editDesign')
    include 'editelement.php';


  • @DOA said:

    I've tried the array thing (map values to files) way back and what I don't like about it is the maintenance every time you need to add/remove a fie. My current setup takes the action value, sanitizes it, sticks a php extension at the end of the name, checks that the file actually exists and serves the page. 

     

    if( file_exists( $_GET['action'] . ".php" ){

        include( $_GET['action'] . ".php" );

    }

    There is a slight risk when $_GET['action'] = "config" or something like it.

     

    "' Or '1' = '1" anyone ? 



  • @Monkios said:

    if( file_exists( $_GET['action'] . ".php" ){

        include( $_GET['action'] . ".php" );

    }

    There is a slight risk when $_GET['action'] = "config" or something like it.

     

    "' Or '1' = '1" anyone ? 

    Holy crap, people, stop suggesting garbage like this.   If you are not sanitizing user input before passing it to include() then you are asking to be raped by security holes.  Also, STOP USING include()!!  For fuck's sake, use require().



  • @Monkios said:

    @DOA said:
    I've tried the array thing (map values to files) way back and what I don't like about it is the maintenance every time you need to add/remove a fie. My current setup takes the action value, sanitizes it, sticks a php extension at the end of the name, checks that the file actually exists and serves the page.

    if( file_exists( $_GET['action'] . ".php" ){

        include( $_GET['action'] . ".php" );

    }

    There is a slight risk when $_GET['action'] = "config" or something like it. 

    Well this is an architecture thing. Your script should only look in a folder where your content and content alone resides. And of course you should have  a nice regex check that allows only say... alphanumeric characters (just an example) in the action value, which should keep people out of other places.



  • Why not just:

    switch($_GET['action']) {

     case 'blah':

        include('/inc/blah.php');

        break;

    case 'dildo':

        include('/inc/other_stuff.php');

        break;

    default:

         include('really_other_stuff.php');

        break; 

     

    Easier to read than "elseif"s, easy to maintain, no worrying about users changing things. 



  • @EJ_ said:

    Why not just:

    switch($_GET['action']) {

     case 'blah':

        include('/inc/blah.php');

        break;

    case 'dildo':

        include('/inc/other_stuff.php');

        break;

    default:

         include('really_other_stuff.php');

        break; 

     

    Easier to read than "elseif"s, easy to maintain, no worrying about users changing things. 

    Because it's a lost simpler to define the actions and their corresponding scripts in an associative array and do it that way. 



  • $action = isset($_GET["action"]) ? $_GET["action"] : "default";
    $action = preg_replace("[^a-z0-9_]", "", strtolower($action));
    $action = "action_$action.php";
    if (file_exists($action))
        require $action;
    else
        require "action_invalid.php";

Log in to reply