markf
markf

Reputation: 83

Simple PHP Controller, any issues with this code?

I'm trying to build a really simple php controller page for a small site. Here is what I have so far. It seems to work well. Are there any issues I might be missing with doing it this way?

$page = $_GET['p'];

switch ($page)
{
case "":
    ob_start();
    include "inc/home.php";
    $content = ob_get_contents();
    ob_end_clean();
    break;
case $page:
    $page = str_replace("/", "", $page);
    if (file_exists("inc/".$page.".php"))
    {
       ob_start();
       include "inc/".$page.".php";
       $content = ob_get_contents();
       ob_end_clean();
    }
    else
       include "inc/404.php";
    break;
}

include("inc/header.php");

echo $content;

include("inc/footer.php");

UPDATE: Here is the final code based on comments that works well.

<?php

$page = (isset( $_GET['p']) && !empty($_GET['p'])) ? $_GET['p'] : 'home';

if( preg_match( '/[^a-z]/i', $page))
{
    $page = '404';
}

if( !file_exists( "inc/".$page.".php"))
{
    $page = '404';
}

ob_start();
include("inc/header.php");
include("inc/".$page.".php");
include("inc/footer.php");

?>

Upvotes: 0

Views: 516

Answers (1)

nickb
nickb

Reputation: 59699

Your entire script can be rewritten as follows:

$page = ( isset( $_GET['p']) && !empty( $_GET['p'])) ? $_GET['p'] : 'home';

// Only allow alphabetic characters in a user supplied page
if( preg_match( '/[^a-z]/i', $page))
{
    $page = '404';
}

if( !file_exists( "inc/".$page.".php"))
{
    $page = '404';
}

include("inc/header.php");
include("inc/".$page.".php");
include("inc/footer.php");

However, this is also no longer susceptible to Local File Inclusion, as $page is restricted to only alphabetic characters, and the script will show the 404 page if anything else is submitted.

It's also more efficient as its not using output buffering.

Upvotes: 2

Related Questions