1
\$\begingroup\$

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?

\$\endgroup\$
3
  • 4
    \$\begingroup\$ Welcome to CR! It would be helpful if you edited your title to summarize what your code is doing. \$\endgroup\$ – Mathieu Guindon Sep 29 '14 at 2:54
  • \$\begingroup\$ Do you really have two directories named pages and _pages that contain files with the same name? If so you'd better give'em a proper name (at least one of them), because it looks like backup. \$\endgroup\$ – shudder Sep 29 '14 at 19:49
  • \$\begingroup\$ One obvious thing I am missing in your code are comments. Use comments to explain what you do and why. Sometimes code can speak for itself, but usually it's better for readers of the code, and yourself, when an explanation is present. \$\endgroup\$ – KIKO Software Sep 29 '14 at 20:56
2
\$\begingroup\$

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

}
\$\endgroup\$
2
\$\begingroup\$

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.
\$\endgroup\$
1
  • \$\begingroup\$ ../../my/nefarious/file shouldn't be a problem given he is checking it against a whitelist \$\endgroup\$ – bumperbox Sep 29 '14 at 20:16

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.