Forum Moderators: coopster
RammsteinNicCage has been big enough to provide our next code example for disection
thanks Jennifer :)
//check to see if a gallery was selected
$gallery = strip_tags($_GET['gallery']);
$type = strip_tags($_GET['type']);
$page = strip_tags($_GET['page']);
if (!$gallery ¦¦!$type ¦¦!$page) {
header("Location: gallery_list.php");
exit;
}
//make sure $page is a number and $type and $gallery are words
if (!preg_match("/^[0-9]*$/", $page) ¦¦!preg_match("/^[a-zA-Z]*$/", $type) ¦¦!preg_match("/^[a-zA-Z]*$/", $gallery)) {
exit;
}
//call to template
require_once($_SERVER["DOCUMENT_ROOT"]."/templates/template.php");
$template = new Template($_SERVER["DOCUMENT_ROOT"]."/templates/");
$template->set_filenames(array(
'gallery' => 'gallery.tpl')
);
$dir = $gallery."/images/".$type."/";
$d = dir($dir);
//output all files in $dir into an array, then exclude thumbnails and . directory
$files= array();
$files[] = 'junk'; //waste [0] element in array
$index = 1;
while (false!== ($entry = $d->read())) {
if (preg_match("/^_/", $entry) ¦¦ substr($entry, 0, 1)=='.') {
continue;
}
$files[]= $entry;
}
$d->close();
$count = count($files) - 1; //-1 to delete [0] element
$pics_per_page = 16;
sort($files, SORT_REGULAR);
unset($files[0]); //waste [0] element in array
//determine the number of pages needed and what image each page should start with
$num_pages = $count / $pics_per_page; // # of full pages
settype($num_pages,"integer"); // make $num_pages a whole number
// determine if an extra page is needed
if ($count % $pics_per_page!=0) {
$num_pages = $num_pages + 1;
}
$page = isset($_GET['page'])? $_GET['page'] : 1; // if page is set, get page, else page = 1
$start_value = ($page * $pics_per_page) - $pics_per_page + 1;
//begin displaying pics
$j = $start_value;
while ($j < ($start_value + $pics_per_page)) {
if ($j > ($count)) {
break;
} else {
if ($j%4 == 1) {
$template->assign_block_vars('row', array());
}
$template->assign_block_vars('row.cells', array(
'DIR' => $dir,
'FILE_NAME' => $files[$j])
);
$j++;
if ($j%4 == 1 ¦¦ $j > ($count)) {
}
}
}
/******************* BEGIN PAGINATION ****************/
$prev = $page - 1;
$next = $page + 1;
$pagination = "";
//previous page
if ($page > 1) {
$pagination .= "<a href=\"/galleries/gallery.php?gallery=".$gallery."&type=".$type."&page=".$prev."\">Previous</a> ";
}
//previous pages
$i = 1;
while ($i < $_GET['page']) {
$pagination .= "<a href=\"/galleries/gallery.php?gallery=".$gallery."&type=".$type."&page=".$i."\">".$i."</a> ";
$i++;
}
for($i = 1; $i <= $num_pages; $i++) {
if($i == $page) {
$pagination .= "<span style=\"font-weight:bold;\">".$i."</span> ";
}
}
//next pages
$i = $page + 1;
while ($i > $_GET['page'] && $i <= $num_pages) {
$pagination .= "<a href=\"/galleries/gallery.php?gallery=".$gallery."&type=".$type."&page=".$i."\">".$i."</a> ";
$i++;
}
//next page
if ($page < $num_pages) {
$pagination .= "<a href=\"/galleries/gallery.php?gallery=".$gallery."&type=".$type."&page=".$next."\">Next</a>";
}
$template->assign_block_vars('pagination', array(
'DIR' => $dir,
'PAGE' => $i,
'PAGINATION' => $pagination,
'GALLERY' => ucfirst($gallery)) //make first letter of string capital
);
$template->assign_vars(array(
'GALLERY' => ucfirst($gallery),
'TYPE' => ucfirst($type))
);
//*************** END PAGINATION **************
$template->pparse('gallery');
First a caveat - I'm looking at this quickly, so if I say "You don't check that it's an integer" what it really means is that I didn't see it in a quick look.
Anyway, these are more stability than security issues, but if you have errors reported and a hacker can generate errors, that's a minor security issue too.
//check to see if a gallery was selected
$gallery = strip_tags($_GET['gallery']);
$type = strip_tags($_GET['type']);
$page = strip_tags($_GET['page']);
if (!$gallery ¦¦!$type ¦¦!$page) {
I'd start here
- first, check whether or not these values are set
- then, where possible, instead of just stripping the tags and running through a regex, check these values against a whitelist. This probably isn't possible for some vars ($gallery I assume could be pretty much any name for a gallery), but it should be for others (presumably there is a short list of acceptable types?)
- the strip_tags isn't really necessary because in all cases, only alpha or numeric is allowed, so your regex is more restrictive. If someone is playing games and putting tags in the URL, just kick them back as you do if a key param is empty. Don't bother to try to salvage that request.
- On the other hand, be careful about checking with a simple a logical not. If page=0 or page is empty, presumably, you want to just give page 1 later on, not reject the request. Your regex already insures that it's a non-negative integer, since the - or . would fail the regex.
$dir = $gallery."/images/".$type."/";
$d = dir($dir);
These are all coming in as get parameters, so they need to be fully validated. There's no check that the dir in question is a valid dir and that it is an image directory, except insofar as it must have /images/ in the path somewhere. I'm not really sure what's happening inside your template, but I assume it's output only, so it doesn't appear that you are filtering for valid images. So basically, you're very liberal in terms of what you'll put into the $gallery and $files variables, and I can't tell what you're doing to make sure that it's all okay.
$page = isset($_GET['page'])? $_GET['page'] : 1;
Once again, what happens when page=0, which is allowed by your regex barring changes suggested above? If that isn't caught somewhere else, use empty() instead of isset(). Personally, I would check and clean the $_GET data near the top of the script so that you don't have to worry about it later.
Anybody else?
There are a couple of instances when $_GET['page'] is called further down in the code, when you already have that information cleaned and in a variable. Even though you are exiting out near the start if the data is bad, I agree that it is best to get all the data you need from your superglobals first off, sanitize them, and then forget about them. If nothing else it makes it easier to change your error-checking down the line, and ensures that data does not slip by.
I think it looks well written overall. I am still looking deeper at it, but I do not see any major noticeable problems :)
1. I would use + rather than * : * checks for 0 or more of the preceding, where + checks for 1 or more of the preceding.
Reason: You can accomplish two tasks at once: 1. check for correct string. 2. check to see if the variable is empty.
2. Rather than a location or a simple exit IOW 'there is nothing here' serve the proper headers: either a 301, if you are going to change locations, or a 404 if there is nothing found.
Reason: There is no need to confuse a SE (or take the chance) when sending them the right note is easily accomplished. I always opt for a proper 404 when there is no information, rather than redirecting.
Original:
$gallery = strip_tags($_GET['gallery']);
$type = strip_tags($_GET['type']);
$page = strip_tags($_GET['page']);
if (!$gallery ¦¦!$type ¦¦!$page) {
header("Location: gallery_list.php");
exit;
}
//make sure $page is a number and $type and $gallery are words
if (!preg_match("/^[0-9]*$/", $page) ¦¦!preg_match("/^[a-zA-Z]*$/", $type) ¦¦!preg_match("/^[a-zA-Z]*$/", $gallery)) {
exit;
}
Modified:
$gallery = strip_tags($_GET['gallery']);
$type = strip_tags($_GET['type']);
$page = strip_tags($_GET['page']);
if (!preg_match("/^[0-9]+$/", $page) ¦¦!preg_match("/^[a-zA-Z]+$/", $type) ¦¦!preg_match("/^[a-zA-Z]+$/", $gallery)) {
unset($gallery,$type,$page); // make sure anything incorrectly set is removed.
header("HTTP/1.0 404 Not Found");
exit;
}
Justin
