...
PDA

Will A PHP Guru Please Critique My Code?

John_Saunders
01-09-2003, 02:39 AM
I wrote a PHP template script and was wondering if a PHP guru would look over my code and see if there's any security holes. If you see a better way to do something instead of how I coded it please point it out. :)

Also, how can I make a printer friendly page so a visitor can click on a link like: http://www.domain.com/index.php?section=about&page=resume&action=print and it will come up with a page with nothing but the content from the PHP file containing the main content and not the header and footer?

Here are the main files that contain the code.

index.php [template]

<?php
// format directory/page names for page title
$sectiontitle = str_replace("_", " ", $section);
$pagetitle = str_replace("_", " ", $page);
$st = ucwords($sectiontitle);
$pt = ucwords($pagetitle);
$title = "My Site | " . $st . " | " . $pt ."";
?>

<?php echo "<?xml version=\"1.0\" encoding=\"iso-8859-1\"?".">"; ?>
<!DOCTYPE html PUBLIC "-//w3c//dtd xhtml 1.0 transitional//en"
"http://www.w3.org/tr/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<title><?php print $title ?></title>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1" />
</head>

<body bgcolor="#FFFFFF" text="#000000">
<table width="700" cellpadding="0" cellspacing="0" align="center" border="1">
<tr>
<td width="700" colspan="2" align="center"><?php include "includes/header.php" ?></td>
</tr>
<tr>
<td width="700" colspan="2" align="center"><?php include "includes/navigation.php" ?></td>
</tr>
<tr>
<td width="150" valign="top">
<?php

/* This code is for the sub navigation links that change
based on what section of the site the visitor is viewing */

$subnav = Array('home','about','links','contact');

$sn = (!isset($_GET['section']) ) ? 'no section' : (!in_array($_GET['section'], $subnav)) ? 'home' : $_GET['section'] ;

$subnav = "includes/subnavigation/" . $sn . ".php";
include($subnav);

?>
</td>
<td width="550" valign="top">
<?php

/* This code pulls the main content for the page. */

$content = Array('overview','resume','references');

$c = (!isset($_GET['page']) ) ? 'no page' : (!in_array($_GET['page'], $content)) ? 'overview' : $_GET['page'] ;

$content = "includes/" . $sn . "/" . $c . ".php";
include($content);

?>
</td>
</tr>
<tr>
<td width="700" colspan="2" align="center"><?php include "includes/footer.php" ?></td>
</tr>
</table>
</body>
</html>

includes/navigation.php [main navigation links]

<?php
// home
if ($section == "" || $section == home) {
print "Home&nbsp;&nbsp;&nbsp;";
}
else {
print "<a href=\"index.php?section=home\">Home</a>&nbsp;&nbsp;&nbsp;";
}

// about
if ($section == about) {
print "About&nbsp;&nbsp;&nbsp;";
}
else {
print "<a href=\"index.php?section=about\">About</a>&nbsp;&nbsp;&nbsp;";
}

// links
if ($section == links) {
print "Links&nbsp;&nbsp;&nbsp;";
}
else {
print "<a href=\"index.php?section=links\">Links</a>&nbsp;&nbsp;&nbsp;";
}

// contact
if ($section == contact) {
print "Contact&nbsp;&nbsp;&nbsp;";
}
else {
print "<a href=\"index.php?section=contact\">Contact</a>&nbsp;&nbsp;&nbsp;";
}
?>

includes/subnavigation/about.php

<?php
// overview
if ($page == overview) {
print "Overview<br>";
}
else {
print "<a href=\"index.php?section=about&page=overview\">Overview</a><br>";
}

// resume
if ($page == resume) {
print "Resume<br>";
}
else {
print "<a href=\"index.php?section=about&page=resume\">Resume</a><br>";
}

// references
if ($page == references) {
print "References<br>";
}
else {
print "<a href=\"index.php?section=about&page=references\">References</a><br>";
}
?>


Any advice would be greatly appreciated.


Regards,

John
mordred
01-09-2003, 08:03 AM
The code looks quite secure, you did a IMO a good job to protect against an inclusion attack by looking up the GET parameter of "section" very thoroughly and comparing it against the values in those arrays.

You have however some slight errors in your if-statements that follow in the included files


if ($section == about) {


You are not comparing against a string, but against a constant. It may only work because you set your error level low enough, and then it doesn't report if a constant value is treated like a string. Include


error_reporting(E_ALL);


at the top of your script and see for yourself.... :)
John_Saunders
01-09-2003, 08:20 AM
Hello mordred,

Thanks for looking over my script. I added the error reporting script to the top of my page and it came up with the error below for each one of the links it printed on both the include pages.

Notice: Use of undefined constant home - assumed 'home' in /home/user22/htdocs/includes/navigation.php on line 3

Can you tell me how I can fix this and what exactly I'm doing wrong?

How about the print function? Is there a way I can easily add a section that will send just the main content include page to a printer friendly format?


Regards,

John
Ökii
01-09-2003, 10:32 AM
if($section == "about")

I posted a more in depth analysis over at sitepoint including a few hints about using $_GET and something on using arrays to simplify your code and make it more easily extendable.

On another subnote: using an xhtml doctype while keeping the align, width and height parameters in table and td tags defeats the purpose and will do you more harm than good.

print:

echo '<a href="print.php'.$_SERVER['QUERY_STRING'].'">printable version</a>';

EZ Archive Ads Plugin for vBulletin Copyright 2006 Computer Help Forum