views:

377

answers:

6

Hello,

I want to know if my code is safe and if there are other safer alternatives to include external files..

So this is my code example, is it safe? How can I make it safer? Thanks!

<?php switch($_GET['p']){
   case 'test1':
      include 'test1.php';
      break;
   case 'test2':
      include 'test2.php';
      break;
   case 'test':
                echo 'something';
      include 'pages/test.php';
                echo 'something';
      break;
   default: 
      include 'main.php';
      break; 
} ?>
A: 

Seeing as you only include those you've hardcoded, I don't see why this wouldn't be safe. These aren't external files though, but I see what you mean. External would mean on a different server.

As for your question, the only alternative to include is require but that isn't necessarily safer, it just works differently.

Arda Xi
+5  A: 

You code is fine. There is no issue conditionally including files like you are doing as the file names are hardcoded. The issue occurs when a the file included is based on an un-sanitized value from the user. E.g

include $_GET['p'];

Which can include whatever the user wants (depending on PHP settings it may also include files on other domains)

The other options are variations on what you are doing

require and require_once will fail if the file doesn't exist. inlucde_once and require_once ensure that the file is only included once, so it that file has been inlucded elsewhere in the program it won't be included.

include_once 'myfile.php';
include_once 'myfile.php'; //does nothing as the file is already included

If you have use classes, there is also the option of the autoloader. From the looks of your application you would have to re-structure it to be able to use it though.

Yacoby
A: 

Yes, perfectly safe.

You're including files you know the contents of, and not doing it based on variables coming from outside sources. an include wont cause your script to fail if it can not load, if that is the result you'd want, choose require('filename');.

Stomped
A: 

You could make that more readable, as follows:

$safeIncludes = array('test1', 'test2', 'test3');
$p = $_GET['p'];
if(in_array($p, $safeIncludes)) {
    $scriptName = $p . '.php';
    include($scriptName);
}

Other than that it is safe as others have pointed out.

karim79
But if its only a few pages, is it really worth allocating an array and searching it if a simple switch() could do? I don't see why its not readable as-is? (unless the cases grew significantly, ie 20+)
Tim Post
@Tim Post Maybe so - I did think it worth suggesting, as any further modification need only happen on the array. Furthermore, if he decides to store the list of allowed scripts in the DB, he would *need* something like the above.
karim79
A: 

It is safe as it is and the switch statement made the logic clearer. Just to make it more safer maybe you can use $__POST just to hide the switch variable data source to make it little bit safer. :D

Hanseh
+1  A: 

You might consider examining the contents of $_GET['p'] prior to even entering the switch. If it contains special characters, rubbish or something else, your program may want to log the incident (and not waste time trying to render the page).

At the least, a nice and polite "Sorry, we could not process your request" page would be in order.

This still allows the switch to fall through to the main page, provided that p contained something worthy of the switch evaluating in the first place.

This is especially true if the main page does any amount of queries in order to render. Sooner or later, someone will notice your URI structure and decide that it might be fun to play with it, don't burn CPU cycles on idiots :)

Tim Post