"Router" - Any issues? Comments?

[benanamen]

I would like feedback on this basic single site entry procedural code. Any issues, improvements or comments? (included in index.php)

<?php
/*
 * This file: display_pages.php
 * Acts as a Router to display pages
 * Restricts access to certain files
 *
 */

//------------------------------------------------------------------------
// Restrict access to these files
//------------------------------------------------------------------------

// Specify some disallowed paths
$restricted_files = array(
    'header',
    'footer',
    'navbar',
    'menu',
);

//----------------------------------------------------------------------------------------
// Display Pages
//----------------------------------------------------------------------------------------

if (isset($_GET['p']))
    {

    $page = basename($_GET['p']);

    // If it's not a disallowed path, and if the file exists
    if (!in_array($page, $restricted_files) && file_exists("./includes/$page.php"))
        {
        $include = "./includes/$page.php";
        }
    else
        {
        $include = './includes/404.php';
        }
    }
else
    {
    $include = './includes/default.php';
    }
?>

[requinix]

Whitelists are better than blacklists: you should specifically list which files are allowed rather than which files are not allowed. You could do that automatically by using scandir() to build an $allowed_files and excluding ".", "..", and the four you don't want (though really putting them in another directory would be better).

Alternatively, you could use ctype_alpha or _alnum on $page to validate that it is only letters/letters and digits.

 

Yes, I know you're basename-ing the page name. That's nice, but this is a matter of principle.

 

Oh. And for SEO you shouldn't use basename as that creates the opportunity for accessing a page through multiple URLs, which is not good. Just do the whitelist/ctype thing.

[benanamen]

Thanks for your feedback.

 

Oh. And for SEO you shouldn't use basename as that creates the opportunity for accessing a page through multiple URLs, which is not good.

 

This is only used in DB backend applications requiring logging in so SEO is of no concern.

Edited October 20, 2016 by benanamen

[Jacques1]

Definitely use a static(!) whitelist. PHP has a long history of fudging up even basic functions, so I wouldn't trust hacks like the basename() approach. For example, some file operations used to be vulnerable to null byte injections where the user could hide part of the path behind a \0.

 

Trying to fix user input (which is what your basename() ultimately does) is generally a bad idea, because it's error-prone and can cause a lot of confusion. Either accept the input as is, or reject it altogether.

Edited October 20, 2016 by Jacques1