Problem with including files based on (non-defined) variable

Question

i have a PHP site with the following code in it:

<?php
$p = $_GET['p']
include("$p.inc");
?>

Whenever I send a visitor to a page like index.php?p=contact for example I want the file contact.inc to be included. This works fine.

Now I want a certain file to be included (e.g. start.inc) when the visitor is sent to index.php without any GET variables. However, an error message is returned which tells me that $p is undefined (which it logically is).

I tried fixing this problem by using the isset function like so:

<?php
if(!isset($p)) $p = "start";
else $p = $_GET['p'];
include("$p.inc");
?>

but this doesn't work because now $p always contains the string "start" and I can't send the visitor to index.php?p=contact anymore - it will still include start.inc

Can somebody please help me with this issue? Thanks in advance!

Alex · Accepted Answer · 2011-04-15 09:15:08Z

1

Explicitly specify the allowable values, obtained from outside.

<?php
    $allowed_pages = array(
        'home' => 'home.inc',
        'contact' => 'contact.inc',
    );

    $page = @$_GET['p'];

    $file = array_key_exists($page, $allowed_pages) ? $allowed_pages[$page] : $allowed_pages['home'];

    include($file);
?>

answered Apr 15, 2011 at 9:15

Alex

6411 gold badge5 silver badges10 bronze badges

Sign up to request clarification or add additional context in comments.

Comments

Nanne · Accepted Answer · 2011-04-15 08:59:42Z

1

You should white-list your pages anyway, for security. so:

<?php
   $p = $_GET['p']
   switch($p){
      case 'contact': 
         include("contact.inc");
         break;
      default:
         include("start.inc");
   }
?>

answered Apr 15, 2011 at 8:59

Nanne

64.4k16 gold badges122 silver badges166 bronze badges

Comments

Nemoden · Accepted Answer · 2011-04-15 09:09:48Z

1

Define your $p variable just like this:

$p = array_key_exists('p', $_GET) ? preg_replace('!\W!', '', $_GET['p']) : 'start';

answered Apr 15, 2011 at 9:09

Nemoden

9,0966 gold badges45 silver badges66 bronze badges

Comments

Your Common Sense · Accepted Answer · 2011-04-15 09:14:22Z

1

you're checking $p instead of $_GET['p'] so, as $p is never set, you always land at starting page.
anyway you have to sanitize this variable first.
good practice would be like this (assuming pages stored in a "pagedata" folder and have .php extension):

if(isset($_GET['p'])) {
  $p = basename($_GET['p']);
} else {
  $p = "start";
}
$fileName = "pagedata/$p.inc.php";
if(is_readable($fileName)) {
    include($fileName);
} else {
    include("pagedata/404.html");
}

answered Apr 15, 2011 at 9:14

Your Common Sense

158k42 gold badges226 silver badges374 bronze badges

Comments

mario · Accepted Answer · 2011-04-15 09:03:50Z

0

You should prefer an array-map or a switch like Nanne suggested.

At the very least use basename() if you want to keep using the $p variable directly in the include statement. And this is how you could avoid the "error" (which is a debug notice, btw):

<?php
   $p = @$_GET["p"]   or   $p = "start";

   $p = preg_replace("/\W+/", "", $p);    // minimum filtering
   include("./$p.inc");
?>

answered Apr 15, 2011 at 9:03

mario

146k20 gold badges243 silver badges293 bronze badges

4 Comments

fruitbooter Over a year ago

would you mind explaining the following piece? $p = preg_replace("/\W+/", "", $p);

mario Over a year ago

Yes. That's a regex filter. It removes any "non-word" characters. Meaning it only leaves in letters, numbers or underscores.

fruitbooter Over a year ago

Ah thanks! I guess this is particularly useful if the variable is manipulated by the visitor? If I am the only person in charge of the code I don't necesarrily need to filter my variables, correct?

mario Over a year ago

Better safe than sorry. If you were the only one using the website, then yes. But otherwise any user can manually enter the URL and a fake parameter.

fruitbooter · Accepted Answer · 2011-04-16 13:33:05Z

0

Thanks to you all!

I combined most of your suggestions to the following piece of code:

<?php
$pages = array(
'start'=>'Start.inc';
'contact'=>'Contact.inc';
'about'=>'About.inc';
};

$p = array_key_exists(@$_GET['p'], $pages) ? preg_replace('!\W!', '', $_GET['p'] : 'start';
$p = ucfirst($p);

$page = "./$p.inc";
if(is_readable($page)) include($page);
else include(./404.);
?>

I particularly like the array-map (as suggested by Alex and mario) for security reasons aswell as the error page idea by Col. Shrapnel.

edited Apr 16, 2011 at 13:33

answered Apr 16, 2011 at 10:40

fruitbooter

836 bronze badges

Collectives™ on Stack Overflow

Problem with including files based on (non-defined) variable

6 Answers 6

Comments

Comments

Comments

Comments

4 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

6 Answers 6

Comments

Comments

Comments

Comments

4 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related