1
\$\begingroup\$

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');
}
?>
\$\endgroup\$
3
  • \$\begingroup\$ use file_exists() to check the file existence. \$\endgroup\$
    – bekt
    Mar 9, 2015 at 4:16
  • \$\begingroup\$ 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 \$\endgroup\$
    – Pinoniq
    Mar 14, 2015 at 14:16
  • \$\begingroup\$ Why aren't all nondigital characters stripped by preg_replace(). It seems obvious to me that $pages is an indexed array. Is this code working as expected? \$\endgroup\$ Jan 28, 2021 at 9:30

1 Answer 1

1
\$\begingroup\$

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'];
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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