Criticise my code!
Posted: Mon Sep 04, 2006 10:30 am
Hey ladies n gents.
I'm new to PHP.. basically, I've taught myself the language. I'm developing a website for a joint venture with two friends - a fishkeeping information resource.
I have a control panel which allows users to login, then depending on their user level, do various things - admins can do everything, authors can create profiles, guests can only view - and so on.
A friend of a friend was looking at my code, and has informed me that it's "woefully insecure". He also mentioned that my SQL statements would lead to very slow loading searches. Unfortunately, he hasn't told me how to rectify it!
Could you please give me some basic security and SQL tips? I've been looking up security on the net and realise that I should probably add a makesafe() function for variables which removes semi-colons, hashes and adds slashes to search variables. I realise, also, that I should turn off register global variables - but I'm not sure how to do that.
Said friend of a friend also told me that I should avoid using get/post variables from forms. I was using get methods, but because the "back" button stopped working, I changed the methods to post - apparently that's a bad idea.
The site needs to be secure. I have absolutely no doubt about that - I am not naive enough to think nobody will ever attempt to randomly access the site.
Excuse the coding please, it was one of the first PHP documents I wrote - with some bits borrowed from tutorials and the like.
Thanks in advance 
I'm new to PHP.. basically, I've taught myself the language. I'm developing a website for a joint venture with two friends - a fishkeeping information resource.
I have a control panel which allows users to login, then depending on their user level, do various things - admins can do everything, authors can create profiles, guests can only view - and so on.
A friend of a friend was looking at my code, and has informed me that it's "woefully insecure". He also mentioned that my SQL statements would lead to very slow loading searches. Unfortunately, he hasn't told me how to rectify it!
Could you please give me some basic security and SQL tips? I've been looking up security on the net and realise that I should probably add a makesafe() function for variables which removes semi-colons, hashes and adds slashes to search variables. I realise, also, that I should turn off register global variables - but I'm not sure how to do that.
Said friend of a friend also told me that I should avoid using get/post variables from forms. I was using get methods, but because the "back" button stopped working, I changed the methods to post - apparently that's a bad idea.
The site needs to be secure. I have absolutely no doubt about that - I am not naive enough to think nobody will ever attempt to randomly access the site.
Excuse the coding please, it was one of the first PHP documents I wrote - with some bits borrowed from tutorials and the like.
Code: Select all
<?php session_start(); ?>
<html>
<head>
<title>Search Results</title>
</head>
<body>
<?php
include("../config.php");
if(!$_SESSION['username'] && !$_SESSION['password']) {
echo '<center>Please log in</center>';
echo '
<form method="post" action="../log.php?act=login">
<table align="center" class="l_table">
<tr>
<td>Username:</td><td><input type="text" name="username" size="13" /></td></tr>
<tr>
<td>Password:</td> <td><input type="password" name="password" size="13" /></td></tr>
<tr>
<td> </td><td><input type="submit" value="Login" /></td></tr>
</table>
</form>';
}
else {
mysql_close($db);
$username = $_SESSION['username'];
$ulevel = $_SESSION['userlevel'];
$mid = $_SESSION['mid'];
include "dataLib.php";
// CONNECT TO DB (Global Function)
$dbConn = connectToDB();
// ^
// BEGIN MULTIPLE SEARCH PAGES
// >Start variables
if (!($limit))
{
$limit = 50;
} // Default results per-page.
if (!($page))
{
$page = 0;
} // Default page value.
$search = $_REQUEST['commonName'];
$searchSpecies = $_REQUEST['species'];
$searchGenus = $_REQUEST['genus'];
$searchCat = 0;
$searchCatInp = $_REQUEST['category'];
$searchAuthInp = $_REQUEST['author'];
$catRead = read_cat($_REQUEST['field']);
$authRead = "";
if ($searchAuthInp == "666")
{
$authRead = NULL;
$authAddition = NULL;
}
else
{
$authRead = read_auth($searchAuthInp);
$authAddition = "AND author LIKE '$searchAuthInp%'";
}
$sqlphp = "commonName=$search&species=$searchSpecies&genus=$searchGenus&category=$searchCatInp&author=$searchAuthInp";
$sql = "";
// CATEGORY
if ($searchCatInp == "")
{
$sql = "SELECT * FROM profile WHERE commonName LIKE '%$search%' AND species LIKE '%$searchSpecies%' AND genus LIKE '%$searchGenus%' $authAddition";
}
else if ($searchCatInp == "666")
{
$sql = "SELECT * FROM profile WHERE commonName LIKE '%$search%' AND species LIKE '%$searchSpecies%' AND genus LIKE '%$searchGenus%' $authAddition";
}
else
{
$sql = "SELECT * FROM profile WHERE commonName LIKE '%$search%' AND species LIKE '%$searchSpecies%' AND genus LIKE '%$searchGenus%' AND category LIKE '$searchCatInp%' $authAddition";
}
$result = mysql_query($sql, $dbConn);
$numrows = mysql_num_rows($result); // Number of rows returned from above query.
if ($numrows == 0)
{
echo("No results matching your query - ($search / $searchSpecies / $searchGenus / $catRead / $authRead)");
exit();
}
// >Finish variables
// >Start math
$pages = intval($numrows/$limit); // Number of results pages.
// $pages now contains int of pages, unless there is a remainder from division.
if ($numrows % $limit)
{
$pages++;
} // has remainder so add one page
$current = ($page/$limit) + 1; // Current page number.
if (($pages < 1) || ($pages == 0))
{
$total = 1;
} // If $pages is less than one or equal to 0, total pages is 1.
else
{
$total = $pages;
} // Else total pages is $pages value.
$first = $page + 1; // The first result.
if (!((($page + $limit) / $limit) >= $pages) && $pages != 1)
{
$last = $page + $limit;
} //If not last results page, last result equals $page plus $limit.
else
{
$last = $numrows;
} // If last results page, last result equals total number of results.
// >Finish math
$query = "$search // $searchSpecies // $searchGenus // $SearchAuthInp";
print <<<HERE
<title>Search Results</title>
</head>
<body><p>
<b><font size="3">Search Results</font></b><br>
<b>Common Name:</b> $search || <b>Genus:</b> $searchGenus || <b>Species:</b> $searchSpecies ||
<b>Category:</b> $catRead || <b>Author:</b> $authRead</p>
<table width="100%" border="0">
<tr>
<td align="left">
Page <b>$current</b> of <b>$total</b>
</td>
</tr>
<tr>
<td align="left">
Results <b>$first</b> - <b>$last</b> of <b>$numrows</b>
</td>
</tr>
<tr>
<td colspan="2" align="left">
</td>
</tr>
<tr>
<td colspan="2" align="left">
Results per-page: <a href="$PHP_SELF?$sqlphp&page=$page&limit=5">5</a> | <a href="$PHP_SELF?$sqlphp&page=$page&limit=10">10</a> | <a href="$PHP_SELF?$sqlphp&page=$page&limit=20">20</a> | <a href="$PHP_SELF?$sqlphp&page=$page&limit=50">50</a>
</td>
</tr>
</table>
HERE;
$sql = "";
// CATEGORY
if ($searchCatInp == "")
{
$sql = "SELECT * FROM profile WHERE commonName LIKE '%$search%' AND species LIKE '%$searchSpecies%' AND genus LIKE '%$searchGenus%' $authAddition ORDER BY genus ASC LIMIT $page, $limit";
}
else if ($searchCatInp == "666")
{
$sql = "SELECT * FROM profile WHERE commonName LIKE '%$search%' AND species LIKE '%$searchSpecies%' AND genus LIKE '%$searchGenus%' $authAddition ORDER BY genus ASC LIMIT $page, $limit";
}
else
{
$sql = "SELECT * FROM profile WHERE commonName LIKE '%$search%' AND species LIKE '%$searchSpecies%' AND genus LIKE '%$searchGenus%' AND category LIKE '$searchCatInp%' $authAddition ORDER BY genus ASC LIMIT $page, $limit";
}
$result = mysql_query($sql, $dbConn);
print "<p align=\"center\">";
if ($page != 0)
{ // Don't show back link if current page is first page.
$back_page = $page - $limit;
echo("<a href=\"$PHP_SELF?$sqlphp&page=$back_page&limit=$limit\">back</a><br> \n");
}
for ($i=1; $i <= $pages; $i++) // loop through each page and give link to it.
{
$ppage = $limit*($i - 1);
if ($ppage == $page){
echo("<b>$i</b> \n");} // If current page don't give link, just text.
else{
echo("<a href=\"$PHP_SELF?$sqlphp&page=$ppage&limit=$limit\">$i</a> \n");}
}
if (!((($page+$limit) / $limit) >= $pages) && $pages != 1)
{ // If last page don't give next link.
$next_page = $page + $limit;
echo(" <br><a href=\"$PHP_SELF?$sqlphp&page=$next_page&limit=$limit\">next</a>\n");
}
print "<p>";
while ($row = mysql_fetch_assoc($result))
{
$id = $row["id"];
$commonName = $row["commonName"];
$species = strtolower($row["species"]);
$genus = $row["genus"];
$authorID = $row["author"];
$count++;
print "<p><b>Common name: </b>$commonName\n";
print "<br><b>Scientific name: </b>$genus $species\n";
print "<br>[<a href=\"../../profile.php?id=$id\">view</a>]\n";
if ($ulevel == 1 || $ulevel == 2 && $mid == $authorID)
{
print "[<a href=\"edit.php?id=$id\">edit</a>]\n";
}
if ($ulevel == 1)
{
print "[<a href=\"delete.php?id=$id\">del</a>]</p>\n";
}
}
print "<p align=\"center\">";
if ($page != 0)
{ // Don't show back link if current page is first page.
$back_page = $page - $limit;
echo("<a href=\"$PHP_SELF?$sqlphp&page=$back_page&limit=$limit\">back</a><br> \n");
}
for ($i=1; $i <= $pages; $i++) // loop through each page and give link to it.
{
$ppage = $limit*($i - 1);
if ($ppage == $page){
echo("<b>$i</b> \n");} // If current page don't give link, just text.
else{
echo("<a href=\"$PHP_SELF?$sqlphp&page=$ppage&limit=$limit\">$i</a> \n");}
}
if (!((($page+$limit) / $limit) >= $pages) && $pages != 1)
{ // If last page don't give next link.
$next_page = $page + $limit;
echo(" <br><a href=\"$PHP_SELF?$sqlphp&page=$next_page&limit=$limit\">next</a>\n");
}
print "</p>";
print <<<HERE
<p align="left">
<p>> <a href="../index.php">Return to CP</a></p>
</p>
HERE;
}
?>
</body>
</html>