Safe Dynamic Include

Question

I been using this php dynamic include code on my site. But I think it not safe, how can write safer and better code to replace this:

$page = (empty($_GET['page'])) ? '' : $_GET['page'].".html";

if (empty($page))
{ 
 $page = 'index.html';
}

else
 {
 $page = $page;

 }

 include($page);

Thank you very much

Answer 1

In terms of security, always allow policy is a bad policy. Do not assume that the request is valid and safe. Always deny upfront, and use a whitelist:

switch($_GET['page']): 
   case 'page-a': case 'page-b': case 'other-page':
      include $_GET['page'] . '.html';
      break;
    default: 
      include 'index.php';
endswitch;

If the whitelist is hard to maintain, try to narrow the possibilities to a single path, use basename:

$name = basename($_GET['page']);
include 'includes/' . $name . '.html';

This way you don’t have to worry to much about security, as long as you keep all the contents of this directory (and all include paths) safe (notice that someone could upload a compromised file to that directory).

If the above fails, try to use realpath() and make sure that the file is in your specified directory tree.

Answer 2

You can use preg_replace() to make sure that the page name doesn't contain anything harmful:

// $page can only contain letters, numbers and '_'
$page = preg_replace('/[^a-zA-Z0-9_]/', '', $_GET['page']);
include "pages/$page.html";