Problem
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?
Solution
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);
}
}
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 ofcore
- 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.