how to build a good router for php mvc

Question

I'm experimenting with php mvc and I'm stucked with the following issue. My request and router classes are really simple and I would like to extend theme to can handle controller calls from sub folders and to controller classes functions should be able to pick up url variables send it threw get and post.

my router looks as it follows

class Router{

    public static function route(Request $request){


        $controller = $request->getController().'Controller';

        $method = $request->getMethod();

        $args = $request->getArgs();


        $controllerFile = __SITE_PATH.'/controllers/'.$controller.'.php';


        if(is_readable($controllerFile)){
            require_once $controllerFile;

            $controller = new $controller;


            if(!empty($args)){
                call_user_func_array(array($controller,$method),$args);
            }else{  
                call_user_func(array($controller,$method));
            }   
            return;
        }

        throw new Exception('404 - '.$request->getController().'--Controller not found');
    }
}

and Request class

    private $_controller;


    private $_method;

    private $_args;

    public function __construct(){

        $parts = explode('/',$_SERVER['REQUEST_URI']);


        $this->_controller = ($c = array_shift($parts))? $c: 'index';
        $this->_method = ($c = array_shift($parts))? $c: 'index';

        $this->_args = (isset($parts[0])) ? $parts : array();

    }

    public function getController(){

        return $this->_controller;

    }
    public function getMethod(){

        return $this->_method;

    }
    public function getArgs(){

        return $this->_args;
    }
}

The problem is:when I try to send threw ajax, variables to a controller method this are not recognized because of its url structure. For example

index/ajax?mod_title=shop+marks&domain=example

is accepted just if it look

index/ajax/shop+mark/example

Answer 1

Your code contains what is known as an LFI vulnerability and is dangerous in its current state.
You should whitelist your what can be used as your $controller, as otherwise an attacker could try to specify something using NUL bytes and possibly going up a directory to include files that SHOULD NOT be ever included, such as /etc/passwd, a config file, whatever.

Your router is not safe for use; beware!

edit: example on whitelisting

$safe = array(
    'ajax',
    'somecontroller',
    'foo',
    'bar',
);
if(!in_array($this->_controller, $safe))
{
    throw new Exception(); // replace me with your own error 404 stuff
}

Answer 2

Since your Request class uses a URI segments approach for identifying controller, action and arguments, global variables such as $_GET or $_REQUEST are not taken into account from within your Request.

What you need to do is to make some additions to your Request code. Specifically:

Remove the line:

$this->_args = (isset($parts[0])) ? $parts : array();

And add the following:

$all_parts = (isset($parts[0])) ? $parts : array();
$all_parts['get'] = $_GET;
$this->_args = $all_parts;

This way, $_GET (ie variables passed via the url) variables will be available in the actions called, as they will be in $args (they will be available as $args['get'] actually, which is the array that holds the $_GET vars, so you will be able to have access to domain=example by using $args['get']['domain']).

Ofcourse, you can add one more method in your Request class (e.g. query) that might look like that:

public function query($var = null)
{
    if ($var === null)
    {
        return $_GET;
    }
    if ( ! isset($_GET[$var]) )
    {
        return FALSE;
    }
    return $_GET[$var];
}

This way, you can get a single variable from the url (e.g. $request->query('domain')) or the whole $_GET array ($request->query()).