Is this page dispatcher secure/fast?

Question

I'm using a index system to say:

On index i use:

<?php core::doPage(); ?>

Core code is:

<?php
class core
{
    public static function doPage()
    {
        if(!isset($_GET["page"]) || empty($_GET) || !is_string($_GET["page"]))
        {
            if(!include_once('./_pages/index.php')) die("404 - File not found.");
        }
        else
        {
            $_GET["page"] = substr($_GET["page"], 0, -4);
            $pages = array_map(function($x) { return str_replace(".php", "", basename($x)); }, glob("./pages/*.php"));
            if(in_array($_GET["page"], $pages))
            {
                if(!include_once("./_pages/{$_GET["page"]}.php")) die("404 - File not found.");
            }
            else
                if(!include_once('./_pages/404.php')) die("404 - File not found.");
        }
    }
}
?>

Any tweak suggestions or is this code fairly secure? Not exploitable?

Answer 1

There are no glaring security issues I can see in the original code, but I felt it could be improved.

<?php

core::doPage();

class core
{
    public static function doPage()
    {

        // i hate having $_GET littered through my code, 
        // it is too easy to mishandle non-sanitized data
        $request_page = isset($_GET["page"]) ? $_GET["page"] : null;

        // why die here, why not use our standard page handling
        if(empty($request_page) || !is_string($request_page)) {
            $request_page = 'index.php';
        }

        // we take .php extenstion off, then tack it back on later, whats the point?,
        // just leave it on
        // if you have 1000's of files glob + in_array test will be a slow way to check
        // give your variables a more descriptive name other then $pages, 
        // so that when someone else edits your code, 
        // they can tell what you are trying to do
        $pages_whitelist = glob("*.php");

        // test and alter your request_page, just do one include at the end
        if(!in_array($request_page, $pages_whitelist)) {
            $request_page = '404.php';
        }

        // include_once triggers a php warning if the file doesn't exist, 
        // why not use file_exists?
        $file = "./_pages/$request_page";

        if (!file_exists($file)) {
            die("404 - File not found.");
        }

        // yay, no more nested if statements
        // a lot cleaner and easier to read (i hope)
        include_once($file);
    }

}

Answer 2

Be very careful with include_once("./_pages/{$_GET["page"]}.php"). What if someone passes in this URL:

/index.php?page=../../my/nefarious/file

You don't want the call to include_once to actually include that file. This is a glaring hole that needs to be shut immediately.

Some nitpicks:

• Classes should begin with capital letters: e.g. Core instead of core
• What happens if an error gets thrown in one of the included files? You want to hide stack traces from the end user.
• The "Core" class is doing too much. It is a router, file loader, and controller dispatcher all rolled into one. Consider breaking these into their own classes.