PHP include() alternative?

Question

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; 
} ?>

Solution 1

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
• require_once
• include_once

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.

Solution 2

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 :)

Comments

[Tim Post]

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+)

[karim79]

@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.

[AndyS]

This doesn't actually match the supplied code - there is no default and test isn't handled. It is shorter, but isn't (arguably) more readable to novice programmers.