Suggestions?

[DanUK]

Hi again. Sorry for yet another thread!

Can you suggest whether I have made "decent" changes to the following code, the old is below, and the newer below:


<?php
if(isset($_GET['p'])){
if(!eregi("/\.php$/",$_GET['p'])){
$p = $_GET['p'] . ".php";
}else{
exit;
}

if(file_exists($p)){
include($p);
exit;
}
}

include("index.php");
exit;
?>


New:


<?php
if (isset($_GET['p'])) {
$p = (substr($_GET['p'], -4) == '.php') ? $_GET['p'] : $_GET['p']
.
'.php';
if (file_exists($p))
include $p;
}
else
include 'index.php';
?>


Any suggesitons or comments on how to improve this would be much appreciated.
This code allows me to load other pages, i.e. faq.php via page.php?p=faq.

Thanks.

[pyro]

You could clean it up even further, like so:

<?php
if (isset($_GET['p'])) {
$p = (strstr($_GET['p'], ".php") ? $_GET['p'] : $_GET['p'].".php";
if (file_exists($p)) {
include $p;
}
}
else {
include "index.php";
}
?>

[DanUK]

Thanks pyro.
Security wise, do you think it's ok?

I did as much reading as poss to create the first, and then went back over everything I did with the second - and your one looks even better.

Thanks.

OOhh update -- Error on line 5.

[pyro]

My mistake. Try this one:

<?php
if (isset($_GET['p'])) {
$p = (strstr($_GET['p'], ".php")) ? $_GET['p'] : $_GET['p'].".php";
if (file_exists($p)) {
include $p;
}
}
else {
include "index.php";
}
?>As far as security goes, they will be able to display any page they want, but they could aslo do that by typing it in their location bar of their browser.

[DanUK]

That's brilliany pryo thanks.

One quick qu. Before, if a page didn't exist, say for example page.php?p=sasjka was loaded, and it didn't exist, it re-loaded index.php. Now, it's just a white screen. Is this intentional?

I realsie that anyone could load any file, but doesn't that only load files ending in .php? I've tried it with other things, and it won't load them. So indeed, it's probably no less secure than anything else - as you could load something directly from the address bar anyway.

Thanks a lot pyro.

[pyro]

Ok, maybe this then:


<?php
if (isset($_GET['p'])) {
$p = (strstr($_GET['p'], ".php")) ? $_GET['p'] : $_GET['p'].".php";
if (file_exists($p)) {
include $p;
}
else {
include "index.php";
}
}
?>Or perhaps even better:


<?php
if (isset($_GET['p'])) {
$p = (strstr($_GET['p'], ".php")) ? $_GET['p'] : $_GET['p'].".php";
if (file_exists($p)) {
include $p;
}
else {
include "index.php";
}
}
else {
include "index.php";
}
?>

[DanUK]

Originally posted by pyro

<?php
if (isset($_GET['p'])) {
$p = (strstr($_GET['p'], ".php")) ? $_GET['p'] : $_GET['p'].".php";
if (file_exists($p)) {
include $p;
}
else {
include "index.php";
}
}
else {
include "index.php";
}
?>

Pyro hi again.
With regard to this, is there a way to secure it a little further, i.e. to only load .php files and to specify a specific directory to only load from, so stop ../ pages for example.
I.e. /home/LAN/www/public_html

Thanks!

[pyro]

If you only want to dissallow them moving up directories, you could use this:

$p = (strstr($_GET['p'], ".php")) ? $_GET['p'] : $_GET['p'].".php";
$p = str_replace("../", "", $p); #remove any ../
if (file_exists($p)) {

If you want to be sure it comes from a spedific directory, you could use something like this:

$dir = "/root/path/to/dir/"; #root path to directory
$p = (strstr($_GET['p'], ".php")) ? $_GET['p'] : $_GET['p'].".php";
$p = str_replace("../", "", $p); #remove any ../
if (file_exists($dir.$p)) {

[DanUK]

Wonderful thanks!

So it'd look like:


<?php
if (isset($_GET['p'])) {
$dir = "/root/path/to/dir/"; #root path to directory
$p = (strstr($_GET['p'], ".php")) ? $_GET['p'] : $_GET['p'].".php";
$p = str_replace("../", "", $p); #remove any ../
if (file_exists($dir.$p)) {
include $p;
}
else {
include "index.php";
}
}
else {
include "index.php";
}
?>

[pyro]

Yep, looks good.

[DanUK]

pyro hi again.
It seems to do nothing but load, none of the pages will load, it just sits on index and loads...
I changed the directory - what could this be down to?

thanks!

[pyro]

Did you change this line?

$dir = "/root/path/to/dir/"; #root path to directory

[DanUK]

It's working now thanks pyro.
I don't think i had a "/" after public_html, would this of made a difference?
It's working now I had /home/LAN/www/public_html/

Furthermore, at the top of each page I have:


<?php
if (!eregi("page.php", $_SERVER['PHP_SELF'])) {
die ("Sorry, You cannot access this file directly...");
}

$index = 0;
include("header.php");
?>


at the top of each page.

For footer.php:


<?php
if (eregi("footer.php",$_SERVER['PHP_SELF'])) {
Header("Location: page.php");
die();
}

$footer = 1;
?>


header.php:


<?php
if (eregi("header.php",$_SERVER['PHP_SELF'])) {
Header("Location: page.php");
die();
}

$header = 1;
?>


Is that ok? :) thanks.

[pyro]

Originally posted by skydan
Is that ok?How do you mean that? Ok as far as what?

[DanUK]

Sorry, I meant is the code I've used in the pages/header/footer etc okay?