Dynamic include

Question

<?php
// Default page
if (!$_SERVER['QUERY_STRING']) $Page = "news";

// View
elseif (isset($_GET['newsID'])) $Page = "newsView";
elseif (isset($_GET['userID'])) $Page = "profile";
elseif (isset($_GET['messageID'])) $Page = "message";
elseif (isset($_GET['threadID'])) $Page = "thread";
elseif (isset($_GET['forumID'])) $Page = "forum";
elseif (isset($_GET['imgID'])) $Page = "imageView";

// Pages
elseif ($_GET['content'] == "search") $Page = "search";
elseif ($_GET['content'] == "gallery") $Page = "gallery";
elseif ($_GET['content'] == "forums") $Page = "forums";
elseif ($_GET['content'] == "messages") $Page = "messages";
many more...

// If page don't exist
else $Page = "error";

// Output page
include($config['PAGE_PATH'].$Page.'.php');
include($config['TEMPLATE_PATH'].$Page.'.html');
?>

This is some code my friend wrote years ago...
I'm wondering how safe this is and if I could make it a little cleaner?

Thanks.

Answer 1

Well it's safe in the sense that the code sanitizes the parameter. People often do that (to disastrous results usually).

I'm not a big fan of this pattern however. By that I mean a single controller that includes files passed on a parameter. I much prefer to have a script per page and just include what's needed. Structurally I think it's better.

That being said, there's nothing fundamentally wrong with the above approach.

Just make sure you treat any data that comes from the user with extreme paranoia.

Answer 2

I'd be very cautious about cleaning this up any more than it is. Using a variable provided by user input in an include is a major security flaw. I would leave this as it is.

Answer 3

You could make an array with GET options as key and pages as values.. then use switch() statement. This should make the code cleaner. But as Cletus said, this isn't the best way to make a controller.

Answer 4

As it is you who defines what pages are allowed to be included (white list), I cannot see any way to poison the $Page variable. So this seems pretty safe.

But you could clean it up using arrays such as:

$argToPage = array(
    'newsID'    => 'newsView',
    'userID'    => 'profile',
    'messageID' => 'message',
    'threadID'  => 'thread',
    'forumID'   => 'forum',
    'imgID'     => 'imageView'
);
$contents = array(
    'search',
    'gallery',
    'forums',
    'messages'
);

$Page = null;
if (trim($_SERVER['QUERY_STRING']) == '') {
    $Page = 'news';
} else {
    foreach ($_GET as $key => $val) {
        if (isset($argToPage[$key])) {
            $Page = $argToPage[$key];
            break;
        }
    }
}
if (is_null($Page) && isset($_GET['content']) && in_array($_GET['content'], $contents)) {
    $Page = $contents[$_GET['content']];
} else {
    $Page = 'error';
}

But that’s not much cleaner.