Problem with some simple code

Question

<?php

// get all files from pages/ with .php extension
$pages = glob('pages/*.php');

foreach ($pages as $page) {

// remove path
$page_clean = str_replace('pages/', '', $page);

// put it in an array
$allowed_pages = array($page_clean);

// determine that the lank will be index.php?page=%
$page = $_GET['page'] . '.php';

// load page
if(in_array($page, $allowed_pages)) {
  include('pages/' . $page);
} else {
echo "Page not found.";
}

}

?>

It does include the page I call for but it echoes "Page not found" also. What am I doing wrong here?

One love

Answer 1

The if block shouldn't be in the loop. Also, you're constructing the array incorrectly. Try:

<?php

// get all files from pages/ with .php extension
$pages = glob('pages/*.php');

$allowed_pages = array();
foreach ($pages as $page) {
    // remove path
    $page_clean = str_replace('pages/', '', $page);

    // put it in an array
    $allowed_pages[] = $page_clean;
}

// determine that the lank will be index.php?page=%
$page = $_GET['page'] . '.php';

// load page
if(in_array($page, $allowed_pages)) {
    include('pages/' . $page);
} else {
    echo "Page not found.";
}

Answer 2

You shouldn’t browse the whole directory on every request just to see if a given file exists. Just check if that specific file exists:

if (strpos($page, '..') !== false || strpos($page, '/') !== false) {
    // invalid value, but you better use a whitelist than a blacklist like I did
} else {
    if (is_file('pages/'.$page.'.php')) {
        // file exists
    } else {
        // file doesn’t exist
    }
}

Answer 3

I'd do it like this:

if(!isset($_SESSION['allowed_pages'])) {
  $_SESSION['allowed_pages'] = array_map('basename', glob('pages/*.php'));
}
$page = $_GET['page'] . '.php';

if(in_array($page, $_SESSION['allowed_pages'])) {
    include("pages/$page");
}else {
    echo 'Page not found.';
}

That only loads the list of pages once per session and gets rid of the explicit loop for cleaning up the page names from the glob.