Secure way to include page from GET parameter?

Question

I'm working on a set up where the URLs will be along the lines of:

http://example.com/index.php?page=about

In reality they will rewritten to that from a simpler URL. index.php will include another page, using this code:

if ( isset( $_GET['page'] ) )
{
 $page = $_SERVER['DOCUMENT_ROOT'] . '/pages/' . $_GET['page'] . '.php';
 if ( is_file( $page ) )
  include $page;
 else
  echo 'That page doesn\'t exist.';
}

Assuming everything in the pages folder is perfectly safe to be included, is this code secure? I've protected against the well-known directory hacks, i.e. using page=../../.passwd. Is there anything else I should be mindful of?

Answer 1

probably better to switch-case it

$page_name = $_GET['page'];

switch($page_name) {
case 'about':
 $page = $_SERVER['DOCUMENT_ROOT'] . '/pages/about.php';
 break;        
case 'home': //fall through to default
case default:
 $page = $_SERVER['DOCUMENT_ROOT'] . '/pages/home.php';
}

include $page;

This way, there isn't any injection problem.

Edit

Another solution would be to set up a class dedicated to handling the conversion of page name to address.

class Page {
  static private $pages = array ("about", "home");

  const DEFAULT_PAGE = "home";

  static public function includePage($page_name) {
    if (!in_array($page_name, self::$pages)) {
      $page_name = self::DEFAULT_PAGE;
    }
    include ($_SERVER['DOCUMENT_ROOT'] . '/pages/'.$page_name.'.php';);
  }
}

This way this is all managed inside a single class and future changes are easier to make without digging through other code

edited above to reflect request.

Answer 2

You can also switch to a framework like CodeIgniter that will do it all for you and force you into adopting some coding standards which is always a good thing.

Answer 3

your code is ok, except that you should validate the parameter before use:

if(!preg_match("~^\w+$~", $_GET['page']))
   die("page id must be alphanumeric!");

i won't recommend "switch" approach, because it decreases flexibility, which is the whole point of using dynamic includes.

Answer 4

When handling an arbitrary number of pages it might be best to ensure you have SEO friendly filenames. I would recommend alphanumeric filenames with hyphens or underscores:

define(DOCROOT, $_SERVER['DOCUMENT_ROOT']);

// assume you do not include file extensions in $_GET['page']
$page = trim(preg_replace('~[^\\pL\d]+~u', '-', $_GET['page']), '-');
if (is_file($page)) {
   include DOCROOT . $page;
}

Answer 5

A very secure way to do this would be to first construct a list of directory contents, then match the user input to that list and use the value from the list for the include. Something in the lines of:

$sdir = $_SERVER['DOCUMENT_ROOT'].'/pages/';
$targetfile = $_GET['page'].'.php';
$filenames = scandir($sdir); // returns an array of directory contents
foreach ($files as $filename) {
  if (($filename[0] != '.')
     && ($filename == $targetfile)
     && (is_file($sdir.$filename)) {
        include $sdir.$filename;
        break;
  }
}

Or you could do it simply by:

$targetfile = $_GET['page'].'.php';
$sdir = $_SERVER['DOCUMENT_ROOT'].'/pages/';
$filenames = scandir($sdir);
if (in_array($targetfile,$filenames)) {
   include $sdir.$filename;
}

But in the latter case you have to be really sure you get the check conditions right, and also use the check by suggested by user "stereofrog". In the first case, you're only including from a list constructed from the directory contents, so it'll be safe even if the user manages to get some weird input through your checks.