1
\$\begingroup\$

I have just written a PHP router, but I have some security concerns about this function. I am trying to serve all the content of a folder using this function. I am trying to prevent harmful URLs.

public static function set_up($reletive_path = true)
{
    self::$url = parse_url($_SERVER['REQUEST_URI'], PHP_URL_PATH);
    if ($reletive_path && self::$url !== "/") {
        $cut_of = strlen(dirname($_SERVER['PHP_SELF']));
        self::$url = substr(self::$url, $cut_of);
    }
    if (substr(self::$url, -1) == "/") {
        self::$url = substr(self::$url, 0, -1);
    }
    if (self::$url == "") {
        self::$url = "/";
    }
}

/**
 * Serves ONLY php and html files from a folder, the full path with extension must be provided
 * @param $url_path string Route to access the folder
 * @param $foldername string Path to the folder to serve
 * @param string|array $methods String or array of methods there will execute this route, all methods MUST be in UPPERCASE
 * @return bool True if route is executed
 */
public static function serve_folder($url_path, $foldername, $methods = "ALL")
{
    /** unrelated code */
    $foldername = rtrim($foldername, "/");

    $query_patten = $url_path;
    if (substr(self::$url, 0, strlen($query_patten)) === $query_patten) {
        $sub_path = substr(self::$url, strlen($query_patten));
        if ($sub_path === false) {
            if (file_exists($foldername . "/index.php") && is_file($foldername . "/index.php")) {
                require $foldername . "/index.php";
                self::$route_found = true;
                return true;
            } else if (file_exists($foldername . "/index.html") && is_file($foldername . "/index.html")) {
                require $foldername . "/index.html";
                self::$route_found = true;
                return true;
            }
        } else if (preg_match('/^(\/[a-zA-Z0-9-_]+)*\/[a-zA-Z0-9-_]+\.(html|php)$/', $sub_path)) {  // matches urls like /folder/subfolder/file.php /subfolder/file.html file.php
            $full_file_path = $foldername . $sub_path;
            if (file_exists($full_file_path) && is_file($full_file_path)) {
                require $full_file_path;
                self::$route_found = true;
                return true;
            }
        } else if (preg_match('/^(\/[a-zA-Z0-9-_]+)*\/[a-zA-Z0-9-_]+(\/)?$/', $sub_path)) { // matches urls like /folder/subfolder/ /folder/subfolder /folder
            $full_file_path = $foldername . rtrim($sub_path, "/");
            if (file_exists($full_file_path . "/index.php") && is_file($full_file_path . "/index.php")) {
                require $full_file_path . "/index.php";
                self::$route_found = true;
                return true;
            } else if (file_exists($full_file_path . "/index.html") && is_file($full_file_path . "/index.html")) {
                require $full_file_path . "/index.html";
                self::$route_found = true;
                return true;
            }
        }
    }
    return false;
}

EDIT

A usage example would be following:

You are having following folder structure:

/router.php (all traffic is routed to this file)

/homepage.php

/functions/signup.php

/functions/login.php

/functions/admin/add_article.php

// router.php
// the libery is called Highway
Highway::get("/", function () {
    include "homepage.php";
});
Highway::serve_folder("/actions", "functions/"); 

Following request will include following file:

website.com -> /homepage.php

website.com/actions/signup.php -> /functions/signup.php

website.com/actions/login.php -> /functions/login.php

website.com/actions/admin/add_article.php -> /functions/admin/add_article.php

\$\endgroup\$
1
\$\begingroup\$

Thanks for your example. It kind of validates my concern on how you are thinking about routing.

I like that you are thinking about the routing problem and trying to move away from legacy ways of having each URL going directly to a physical file for processing. I would however challenge you to take this a step further, as what you have now really only gets partially away from the legacy web application, a script-for-every controller/action line of thinking, in that you still have scripts that you are redirecting to rather then using your router to instantiate an appropriate controller object in order to satisfy the request.

Most modern frameworks that utilize a front-controller routing pattern like you are trying to achieve have gone the route of auto-loading a controller (ideally auto-loaded in a PSR-4 compliant way) to handle the request passed to it by the router, not having the router just include some specific script file.

So you might see a pattern like:

$controller = Router::getController($request);
$controller->execute();

That's it. The router is basically used on a single line in your main execution path.

Here all the information about the HTTP request (including URI) could be injected into a controller object, which then takes over application control. In this case, the router needs to know absolutely nothing about what dependencies that controller has. It typically just needs to know how to map the request (oftentimes just the first segment of the URI) to a controller to be instantiated and hand off the request information and application control over to the controller.

In your case, your router actually does the following:

  • It parses the request (something better handled in its own class such that a request object can be passed around as a dependency to consuming code).
  • It uses information from the request to map the request to an action to be performed (the only thing a router should truly do).
  • It has to understand where dependencies for a particular action are located in the filesystem.
  • It actually loads/executes those dependencies.

If you are striving to adhere to the single responsibility principle, you will quickly understand that the router has too much responsibility in your application. It should own just routing the request to an appropriate controller and then get out of the way.

\$\endgroup\$
  • \$\begingroup\$ Thank you for the feedback. I am going to use this router for a system there currently not have a routing system, and i need to be able to route a request to a folder. Most of the routes are going to use some other functions, those function will be routing to methods like Highway::get("/change-password", "UserController::changePassword"); But the router need to be able to handle files from a folder for some of the other files. Thats why i have made the function this way and i need to know about possible security flaws. \$\endgroup\$ – Benjaco May 3 '17 at 15:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.