Secure way to include page from GET parameter?

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?

13.10.2009 23:58:36
I think setting $_GET['page'] to '../../another/directory/file' might cause issues. I will leave it to someone else to verify and show you ways around this.
MitMaro 14.10.2009 00:05:11
AFAIK that won't cause any problems. $page would get set to something like /var/www/pages/../directory/file which doesn't resolve to /var/www/directory/file, right?
DisgruntledGoat 14.10.2009 00:24:06
Actually I just tried this and ../ in the middle of the path does indeed make it go up a directory. I never expected that...
DisgruntledGoat 14.10.2009 00:31:38
I was going to post an example to show you but since you beat me to it, I will leave things as are. Jonathan Fingland method is your best and safest bet.
MitMaro 14.10.2009 00:38:59
5 ОТВЕТОВ
РЕШЕНИЕ

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.

4
14.10.2009 00:16:43

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.

9
14.10.2009 00:34:00
For this, I think it be much simpler to have a flat array of {'home', 'about'} and then if the request matches one of those, just include it (wrapped in the path etc). The file names to include will always match the page present in the URL.
DisgruntledGoat 14.10.2009 00:29:39
the key point about the second one is that the pubic method signature didn't change at all. If down the road you want to add more complexity, or add a security layer, etc, it is easier to do so.
Jonathan Fingland 14.10.2009 00:35:30

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.

1
14.10.2009 00:09:58

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;
}
0
14.10.2009 00:51:47
Isn't that actually bad for SEO, since you're converting a messy URL to a neater one? In other words something like my;page;name would resolve to my-page-name, meaning duplicate URLs. If the URL is messed up, we don't display the page.
DisgruntledGoat 14.10.2009 01:00:55
In this case it's used as a security precaution because it whitelists available chars, which would disallow characters such as periods for changing directories. This would assume, however, that you would be appending a standard file extension to the end of your include call (i.e. ".php")
Corey Ballou 14.10.2009 10:17:22

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 regex check suggested in another answer. 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.

1
23.05.2017 10:27:38