Simple page generation from url passed variables

Megahertza

I'm preety new to php, I have this php script that includes other php files depending on what variables are passed in the url. You'll know what I mean when you see the code,

$id = $_GET['id'];
//- Includes For Main Links - Portal, Links and Tutorials ect -//
if ($id == 2) {include 'links.php';}
if ($id == 3) {include 'admin.php';}
if ($id == 4) {include 'forum.php';}
if ($id == 7) {include 'profile.php';}
//- Includes For fresh site index.php -//
if ( ($id == 1) || (empty($id)) ) {include 'news.php';}
//- Includes For the user registration page and user auth -//
if ($id == '6') {include 'users.php';}

What do you think, can I improve on this. thanks in advance

laserlight

Use a switch-case structure instead, since you're only comparing with 1 variable.

Shrike

Also with a switch you can provide a default case, which you don't seem to have done with your if/else (e.g. if $id is set but greater than 7)

Megahertza

I didn't really want a default like you said because I had origanly had more than seven and I figured that I might go above 7 again.
is there anything that might go wrong if i don't have a default

Shrike

Well imagine if someone fiddles with the URL and put ?id=8 ... currently your script wouldn't know what to do.

Megahertza

Thats ok be it would like entering in a url that doesn't exist, just shows a page with a menu minus the content. I'll put it anyway and change it later. thanks

PHPMagician

Another idea would be like:

<?php
$options = array('news','links','admin','forum','profile','users');
if ($_GET['id'] > count($options)) {
    $_GET['id'] = 0;
}
include($options[intval($_GET['id'])] . '.php'));
?>

And I could pull another few out of my.. hat. But basically, if you don't want to use a switch, I reccommend you at least fix you're if statements so that it is not:

if (condition1) {
    action1();
}
if (condition2) {
    action2();
}

but:

if (condition1) {
    action1();
}
elseif (condition2) {
    action2();
}

Basically, if you haven't seen it, make use of the else statement. There is no point checking if $_GET['id'] is all of them, simply because it can only be 1 of them or none of them.

anm

Hmm. Another possible way to do this that also includes the ability to have an error page if the page doesn't exist would be:

<?php
if (!isset($_GET['p'])) {
include("includes/index.php");
} else {
if (file_exists("includes/" . $_GET['p'] . ".php")) {
include("includes/" . $_GET['p'] . ".php");
} else {
echo ("<h2>That page doesn't exist</h2>");
}
}
?>

laserlight

The danger with your version, anm, is that since $_GET['p'] isnt verified as containing an acceptable value, there still is the chance for code injection.

anm

I don't get what you mean. . .

PHPMagician

Originally posted by anm
I don't get what you mean. . .

Well, for example, a user could use .. to escape the directory you specify and include other php scripts. This could result in the comprimisation of your system. Also, in some PHP versions (4.1.0 and less I believe) a user could pass %00 in the get parameter and PHP would convert it to a NULL, so then the .php at the end of the string would be ignore and any file could be open.

You need to validate input. Of course I am being a little hypocritical, in my version if you entered a number not in the arrays range in the negative scale then it still would still try to include a file. It is fixable by simply adding a check to see if the number is greater than or equal to zero, so I was a bit lazy. The problem with yours is the security problems which may arise from the script, which is more severe than my lack of validation.

anm

I still don't get it. If someone enters %00 as the p value it will attempt to include an includes/%00.php page, which won't work, so an error will be reached. If they set p to .. it will attempt to include ...php -- another error.

PHPMagician

Originally posted by anm
I still don't get it. If someone enters %00 as the p value it will attempt to include an includes/%00.php page, which won't work, so an error will be reached. If they set p to .. it will attempt to include ...php -- another error.

Not quite. Ignore the mention of %00 for the moment, and picture this.

p=../php_delete_all

This would then include includes/../php_delete_all.php

Now imagine that php_delete_all.php deletes everything.

Okay, kind of unrealistic, but is similar to what happens often.

Now, the %00 comes into play in PHP versions less than or equal to 4.1.0 I believe. In these versions, this will be reconised as a NULL and the end of the string - hence the .php will be ignored.

I hope that makes sense. Read the PHP manual section on filesystem security. http://ie.php.net/manual/en/security.filesystem.php

laserlight

Actually, PHPMagician, you should have checked for >= count($options) instead of > count($options)
It might be easier to use array_key_exists() instead, actually.

PHPMagician

True, I should have done a >=, I didn't test the code. The only reason I didn't use array_key_exists is because it was not around before PHP 4.1.0 and even though most people are not using that version or less, I have a habit of trying to right code to fit all PHP 4 versions for portability sakes (though, everyone should really be at least at PHP 4.2.0).