Guard against ../ in the uri_req parameter
I'm seeing a possible security hole. A big one. Consider the following URL:
http://yoursite.com/index.php?uri_req=../../hacking_scripts/dump_database.php
Now let's pretend for a moment that I'm criminal who has previously breached the web server that hosts your web application. Your app is stored at:
/www/himanshu/index.php
Now when I breached the web server, I created the following file:
/hacking_scripts/dump_database.php
Which contains PHP code to connect to your database as the same username and password as your web site, because after all, I can view the raw PHP code and found a database connection string with a username and password.
The dump_database.php script basically queries for all the tables your web site's user has available to it, does a SELECT * on the table and dumps out the raw data in the resulting HTML file. Upon hitting the URL above, your router gleefully loads that file and executes the PHP inside because I placed ../../ in the uri_req parameter.
Some other improvements:
Don't redirect to a 404 Not Found page, because the HTTP status will first be a 301 redirect to a GET request that results in a 200 OK response. Your intended response is 404 Not Found, so set the HTTP status to that value and simply include the 404.html file.
Web crawlers and search engines, when encountering a 404 Not Found, will not attempt (or should not attempt) to hit that URL again. In your setup, they will be back to the same "Not Found" URL over and over again.
The _PageCall function name doesn't really tell you what it does. It routes the request. Why not call it something like routeRequest?
Add a try-catch around the calls to other PHP files, then return a 500 Server Error response. This way you don't get ugly stack traces showing up in production, and you can create your own custom, prettier looking error page for unhandled catastrophic errors.
function _PageCall($file_name=null) {
try {
// route the request
} catch (Exception $ex) {
http_response_code(500);
require_once($pagedir . "500.html");
// Log the error message and stack trace
}
}
Remove duplicated 404 Not Found logic by separating the detecting of a file to load versus the actual executing of that file
function _PageCall($file_name=null) {
try {
$pagedir = ROOT.DIRECTORY_SEPARATOR.PAGEDIR;
$page = getFullFilePath($pagedir, $file_name);
if (!isset($page)) {
$page = $pagedir . '404.html';
http_response_code(404);
}
require_once($page);
} catch (Exception $ex) {
http_response_code(500);
require_once($pagedir . "500.html");
// Log the error message and stack trace
}
}
function getFullFilePath($pagedir, $file_name=null) {
/* Every File Request Must Be in HTML */
if(".html"===substr($file_name, -5)):
$file_name = trim($file_name,'.html');
if(file_exists($pagedir.$file_name.'.php')):
return $pagedir.$file_name.'.php';
else:
return null;
endif;
else:
if(NULL==$file_name||$file_name=='index.html'):
return $pagedir.'index.php';
elseif($file_name=='a'):
return $pagedir.'ajax.php';
else:
return null;
endif;
endif;
}
Whitespace is your friend. For readability put spaces around all operators and the conditions for if statements:
if(".html"===substr($file_name, -5)):
Becomes:
if (".html" === substr($file_name, -5)):
And put some empty lines between IF statements:
$file_name = trim($file_name,'.html');
if (file_exists($pagedir . $file_name . '.php')):
Add additional functions for detecting certain kinds of files so that:
if (".html" === substr($file_name, -5)):
Becomes:
if (isHtmlFile($file_name)):
Use a consistent variable naming convention. I see $pagedir and $file_name. In the very least avoid using all lower case letters. Separate words by an underscore or use camelCase.
This isn't a bad script to do simple routing. Really the most important change is to fix the security flaw allowing you to specify any file path in the URL parameter.
