Forum Moderators: coopster
What the page does is, after someone clicks on a release name from a discography list, they are sent to this page and the data matching the release_id is extracted and displayed.
release.php
<?php//connect to database
include('database_connect.php');//check to see if a release was selected
if ((!$_GET[release_id])) {
header("Location: releases_list.php");
exit;
}//call to template
require_once('template.php');
$template = new Template();
$template->set_filenames(array(
'release_body' => 'release_body.tpl')
);//verify release exists, determine what release is being asked for
$verify = "SELECT release_id, release_name, release_date, release_type, release_image_path, release_info FROM releases WHERE release_id=$_GET[release_id]";
$verify_res = mysql_query($verify, $conn) or die(mysql_error());//fetch release's info
if (mysql_num_rows($verify_res) < 1) {
die("This release does not exist. Please return to the <a href=\"/releases_list.php\">releases list</a>.");
} else {
$release_id = $_GET[release_id];
while ($release = mysql_fetch_assoc($verify_res)) {
$release_name = $release['release_name'];
$release_date = $release['release_date'];
$release_type = $release['release_type'];
$release_image_path = $release['release_image_path'];
$release_info = nl2br($release['release_info']);//get tracklisting or summary
$sql = "SELECT track_number, track_name, track_time FROM tracks WHERE release_id=$_GET[release_id] ORDER BY track_number";
$sql_res = mysql_query($sql, $conn) or die(mysql_error());while ($content = mysql_fetch_assoc($sql_res)) {
if ($release_type == "Albums" ¦¦ $release_type == "Singles") {
$template->assign_block_vars('switch_release_type_list', array(
'TRACK_NUMBER' => $content['track_number'],
'TRACK_NAME' => $content['track_name'],
'TRACK_TIME' => $content['track_time'])
);
} else {
$template->assign_block_vars('switch_release_type_summary', array(
'SUMMARY' => $summary)
);
}
}
$template->assign_vars(array(
'RELEASE_NAME' => $release_name,
'RELEASE_DATE' => date('F j, Y', strtotime($release_date)),
'RELEASE_IMAGE_PATH' => $release_image_path,
'RELEASE_IMAGE_NAME' => $release_image_name,
'RELEASE_INFO' => $release_info)
);
}
}$template->pparse('release_body');
?>
release_body.tpl
<html>
<head>
<title>TEST SITE</title>
</head>
<body>
<div>
<p>{RELEASE_NAME}</p>
<!-- BEGIN switch_release_type_list -->
<table>
<tr>
<td>{switch_release_type_list.TRACK_NUMBER})</td>
<td>{switch_release_type_list.TRACK_NAME}</td>
<td>{switch_release_type_list.TRACK_TIME}</td>
</tr>
</table>
<!-- END switch_release_type_list -->
<!-- BEGIN switch_release_type_summary -->
<p>{switch_release_type_summary.SUMMARY}</p>
<!-- END switch_release_type_summary -->
<p>{RELEASE_DATE}</p>
<p>{RELEASE_INFO}</p>
<p><img src="{RELEASE_IMAGE_PATH}" alt="{RELEASE_NAME}" height="180" width="180" /></p>
</div>
</body>
</html>
Any help is appreciated, thank you!
Jennifer
SQL injection can also happen by GET stuff in the url (often called url parameters).
What you can do to protect this code:
- check to see if your GET variable is what you are expecting, an integer; if not, don't use it in a query - do something else, maybe even just die() on the spot or exit() gracefully, depending on your sense of style
This is quite strict checking, so should be ok for this script, unless you do anything fancy later that I didn't see (just read the first few lines, assuming this is your only query, and that your one GET var is always numeric).
In general, and if your script goes on to do fancier stuff later that I didn't bother reading, you should do this - do it whenever you can't do the ultra-strict check of whether it's an integer or not, like when you expect text and that needs to go in a query:
- check to see if magic_quotes_gpc are on with magic_quotes_gpc();
- if so, strip slashes with strip_slashes() from everything coming in from the request - every GET and POST variable
- Add slashes to every string which goes into the query with mysql_escape_string() or mysql_real_escape_string()
- put quotes around every string in your query.
So your query wouldn't be like you have it (yours is ok, since you will check that the input value is an integer), but like this:
$verify = "SELECT release_id, release_name, release_date, release_type, release_image_path, release_info FROM releases WHERE release_id='$_GET[release_id]'";
Kudos for your interest in security, if all PHP webmasters were similarly interested we'd have a much better-working net.
What you can do to protect this code:
- check to see if your GET variable is what you are expecting, an integer; if not, don't use it in a query - do something else, maybe even just die() on the spot or exit() gracefully, depending on your sense of style
Ok, I tried doing this:
if (gettype($_GET[release_id]) == "integer") {
$verify = "SELECT release_id, release_name, release_date, release_type, release_image_path, release_info FROM releases WHERE release_id='$_GET[release_id]'";
$verify_res = mysql_query($verify, $conn) or die(mysql_error());
} else {
die("This release does not exist. Please return to the <a href=\"/releases_list.php\">releases list</a>.");
}
After about an hour of trying to figure out why the die message kept coming up, I realized it was because it thinks the type is a string, not an integer. The release_id is a number, so what am I doing wrong?
- check to see if magic_quotes_gpc are on with magic_quotes_gpc();
- if so, strip slashes with strip_slashes() from everything coming in from the request - every GET and POST variable
- Add slashes to every string which goes into the query with mysql_escape_string() or mysql_real_escape_string()
- put quotes around every string in your query.
magic_quotes_gpc is not on, so I skip the first step, I also skip the second step, right? And the only time you have to do strip_slashes is when there is user input, right?
Jennifer<--such a noob :p
if (gettype($_GET[release_id]) == "integer")
I'm just lazy and use preg_match(), not caring about the four microseconds more it might take than some other method. Anyways, preg_match() uses regular expressions which can look pretty hairy, but it's simple enough for your purpose:
if(preg_match('#^[0-9]*$#', $_GET['release_id']) {
About magic quotes: no, strictly speaking, you don't *have* to check if magic quotes are on, and stripslashes if they are. Only problem is, what if your host turns them on without you knowing, because someone tells them it's better for security? Or someone changes the setting locally since they want to install some script that wants this setting on? The "best practice" is always to check and always to strip slashes if on.
However, here, you've already checked for sure that this is an integer, so it can't have quotes in it, or any of the other things that need to be escaped with slashes. It's just plain numbers. So magic quotes don't matter either way here and you certainly don't have to strip slashes for this. But consider it when you're dealing with other types of queries that contain text from user input.
And the only time you have to do strip_slashes is when there is user input, right?Yup, or input that's not your own and trusted - so stuff coming in via a url, the referrer, even cookie names.
Happy coding!
