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\$ Commented 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\$ Commented 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\$ Commented Jan 28, 2021 at 9:30

1 Answer 1

Reset to default
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\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.