current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 2 down vote favorite

I found a lot of working codes to include page in page on Internet but I could not find a safe code. So I decided to create one myself. The pages will be only stored in folder /pages/ and whitelist seems to be a good option.

Is the follow code safe?

<?php
$unsafe = $_GET['pagename'];
$page = preg_replace('/[^A-Za-z0-9\-]/', '', $unsafe);
if (empty($page)){
include('pages/default.php');
}
$pages = array('default', 'pageone', 'pagetwo', 'another', 'last');
if (isset($pages[$page])) {
    include('pages/' . $page . '.php');
} else {
include('pages/error-404.php');
}
?>
share|improve this question
    
use file_exists() to check the file existence. – bekt Mar 9 '15 at 4:16
    
using file_Exists isn't really a safe option. You would need a lot of extra sanitation before passing something into file_exists. Just think of ../../et/password – Pinoniq Mar 14 '15 at 14:16
up vote 1 down vote

You are checking if a certain key exists in an array: isset($pages[$page]) but you define the $pages array without keys, so they default to numeric ones. A much better approach is to skip the preg_replace thing (it is useless anyway) and create a router array:

$router = array( 'url' => 'pages/foobar.php', 'default' => 'pages/default.php' );

This array maps urls to your internal pages. Then you check or the requested page exists:

if (isset($router[$_GET['page']]) {
    include $router[$_GET['page'];
} else {
    include $router['default'];
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.