Including a page specified in query string

Question

This is how I include a page depending by name.

Example URL:

http://www.page.com/index.php?page=admin

where admin.inc.php lays inside /modules/ directory.

The files are located in a directory I specified. The included files should not render when connected to them directly.

• Are there ANY issues with the code?
• Are there any security flaws, such as remote file inclusion?
• What could I improve in this approach?
• Is this the right approach to modularize a page?
• Are included pages secure enough to not be rendered by themselves?

<?php
 class cms { 
 private $config_module_name;
 private $config_module_extention = '.inc.php'; // ex: .inc.php
 private $config_module_directory = 'modules'; // ex: modules
 private $config_module_list;
 function __construct() {
 $this->SetModuleList();
 }
 public function GetModuleExtention() {
 return $this->config_module_extention;
 }
 private function GetModuleDirectory() {
 return $this->config_module_directory;
 } 
 public function GetModuleList() {
 $moduleList = array();
 foreach($this->config_module_list as $module) {
 if($this->CheckModule($module) == 1) { // 1 for pure module result
 $moduleNoExt = str_replace($this->GetModuleExtention(), '', $module);
 $moduleList[] = str_replace($this->GetModuleExtention(), '', $moduleNoExt);
 }
 }
 return $moduleList;
 }
 private function SetModuleList() {
 $this->config_module_list = array_diff(scandir($this->GetModuleDirectory(), 1), array('..', '.'));
 }
 // 0 - invalid; 1 - correct module; 2 - folder
 private function CheckModule($module) {
 $moduleNameArray = explode(".", $module);
 $countParts = count($moduleNameArray);
 if($countParts == 1) { // folder return 2
 return 2;
 }
 else if($countParts == 3) { // module must count 3
 $moduleNameExtention = '.' . $moduleNameArray[1] . '.' . $moduleNameArray[2];
 // module must match set module extention
 if($this->GetModuleExtention() == $moduleNameExtention) {
 return 1;
 }
 }
 return 0; // invalid module - if reached
 } 
 public function IncludeModule($name) {
 if(in_array($name, $this->GetModuleList())) { 
 include($this->GetModuleDirectory() . '/' . $name . $this->GetModuleExtention());
 }
 else { // if module doesn't exist - throw 404
 header('HTTP/1.0 404 Not Found');
 $this->ConcatContent('<h2>Error 404</h2><br>This page doesn\'t exist.');
 }
 }
 }
?>

Index page where all pages get included:

<?php
 define('CMS_TOKEN', '');
 include('config/config.inc.php');
 $cms = new cms()
 if(($_SERVER['REQUEST_METHOD'] === 'GET' || $_SERVER['REQUEST_METHOD'] === 'POST') && !empty($_GET['page'])) {
 if($_GET['page'] == '404') {
 header('HTTP/1.0 404 Not Found');
 $cms->ConcatContent('<h2>Error 404</h2><br>This page doesn\'t exist.');
 }
 else {
 print_r($_GET);
 $cms->IncludeModule($_GET['page']);
 }
 }
?>

This is how included module looks like. The file must be included by index, or else it should redirect.

<?php
 $module_name = 'Register';
 if(!defined('CMS_TOKEN')) {
 // page cannot be rendered
 header('Location: ../');
 }
 else {
 // render page
 }
?>

• Elias Van Ootegem: In a word: symlinks, another word would be hidden files (like .htaccess). A slightly less dangerous issue coding standards, they matter look into them. Just never trust the network, never trust $_GET or $_REQUEST variables when it comes to require-ing files

Answer 1

• Common practice is to check all GET/POST/SERVER/ENV parameter values against a whitelist or regex before using it for any purpose. You are checking it with a list derived from scandir, but that makes the assumption that the modules directory hasn't been compromised.
• Running scandir on every request is a likely performance bottleneck. One possible solution is to cache the results of the scandir and subsequent filtering to a file above the document root and only re-run the scan if the mod time of the modules directory is more recent than the mod time of the cache file (check out the filemtime() docs).
• You have some duplicate code for handling 404s. This could be combined into one method if you want to adhere to DRY principles.
• A more widely accepted way to prevent a file from being executed is to compare the __FILE__ constant to the SCRIPT_FILENAME which has been set by the server (if available in the $_SERVER global). It's not fool proof, but more portable than your solution.

• HelpNeeder: Thank you for your comment. I'm thinking of having a token in module through SERVER global to assure the correct module has been included for some extra security. But that's still not good if, like you said, the module dir hasn't been compromised. I also like your point about cashing the directory modification time to reduce the compilation time.
• HelpNeeder: Duplicate 404 handling has double purpose, one's for HTACCESS 404 and other for non-existing module 404. Not fully finished with this.