Page 1 of 1

How does this look (simple forum)

Posted: Tue Aug 21, 2007 1:01 pm
by psurrena
This is a very simple forum for a small website. Does it look secure enough?

Code: Select all

<?php
	ob_start();
	
	/*
		Connect
	*/
	require 'library/connect.php';	
	
	/*
		Handle Form
	*/

	if(isset($_POST['submit'])){
		$errors=array();
		
		//Check for empty fields
		if(!isset($_POST['name']) || empty($_POST['name'])){
			$errors[]='Name';
		}
		
		if (!eregi('^[_a-z0-9-]+(\.[_a-z0-9-]+)*@[_a-z0-9-]+(\.[_a-z0-9-]+)*(\.[a-z]{2,4})$',trim($_POST['email']))){
			$errors[]='Email';
		}
		
		if(!isset($_POST['comment']) || empty($_POST['comment'])){
			$errors[]='Comment';
		}
		
		//If all went ok
		if(empty($errors)){
			$name=mysql_real_escape_string(trim($_POST['name']));
			$email=mysql_real_escape_string(trim($_POST['email']));
			$comment=mysql_real_escape_string(trim(nl2br($_POST['comment'])));

			$query="INSERT INTO forum (name,email,comment,date) VALUES ('$name','$email','$comment',current_date)";
			mysql_query($query) or die (mysql_error());
			header('location:forum.php');
		} else {
			$error="<p>There is either missing or innacurate data in the following field(s):</p>\n";
			$error.="<ul>\n";
			foreach($errors as $value){
				$error.= "<li>$value</li>\n";
			}
			$error.="</ul>\n";
			$error.="<p>Please re-enter your comment.</p>\n";
		}
	}

	/*
		Start Content
	*/
	$pageTitle="Union County Comprehensive Plan - Forum";
	$banner='';
	
	$left='
	<p><strong>Comment</strong></p>
	<form method="post">
		Name<br />
		<input type="text" name="name" size="32" /><br />
		Email<br />
		<input type="text" name="email" size="32" /><br />
		Comment<br />
		<textarea name="comment" rows="5" cols="23"></textarea>
		<input type="submit" name="submit" value="Add Comment" />
	</form>
	';
	
	$content="<h3>Forum</h3>\n";	
		
	/*
		Display Comments
	*/	
	//pagination
	$rowsPerPage=5;
	$pageNum=1;
	if(isset($_GET['page'])){
		$pageNum=$_GET['page'];
	}
	$offset=($pageNum-1)*$rowsPerPage;
		
	$query="SELECT id FROM forum";
	$result=(mysql_query($query));
	$numrows=mysql_numrows($result);
	
	$maxPage = ceil($numrows/$rowsPerPage);

	$query="SELECT name, comment,email, date FROM forum ORDER BY id DESC LIMIT $offset, $rowsPerPage";
	$result=mysql_query($query) or die (mysql_error());
	
	if ($numrows < 1){
		$content.="<p>There are no comments.</p>\n";
	} else {
		while($row=mysql_fetch_assoc($result)){
			$date = date('F d, Y', strtotime($row['date']));
			
			$content.='<div class="comment">'."\n";
			$content.='<strong><a href="mailto:'.$row['email'].'">'.$row['name']."</a></strong><br />\n";
			$content.=$row['comment']."<br />\n";
			$content.='<span class="date">'.$date."</span>\n";
			$content.="</div>\n";
		}
	}

	if ($pageNum > 1){
		$page=$pageNum - 1;
		$prev='<a href="forum.php?page='.$page.'">Previous</a>';
	} else {
		$prev="Previous";
	}

	if ($pageNum < $maxPage){
		$page=$pageNum + 1;
		$next='<a href="forum.php?page='.$page.'">Next</a>';
	} else { $next="Next";}
	
	$content.="<p>$prev | $next<br />\n";
	$content.="Page $pageNum of $maxPage.</p>\n";
	
	require 'library/template.php';
?>

Posted: Tue Aug 21, 2007 1:17 pm
by feyd
Here's the quick list of things I see:
  • Use of eregi() over, preg_match(). Speed and future compatibility issue.
  • Assumption that $_POST['submit'] will always be present.. it doesn't have to be. $_SERVER['REQUEST_METHOD'].
  • Comments could contain HTML thereby allowing anyone to inject page code. Naughty!
  • header() based redirection without full URL, http:// and all. Standards compliance issue.
  • Appears to produce invalid, non-compliant HTML.
  • Selection for post count could be modified to use COUNT() reducing the database needs.
  • Page could be used as a spam harvester. Email addresses being shown.
  • The date could be shaped during selection instead of going through strtotime().
  • Potential reserved word collision in the use of "date" and "comment" for column names. It may not apply to you right now, but it probably will when you move this to another server.

Posted: Tue Aug 21, 2007 2:31 pm
by psurrena
Thanks for taking a look. I have a few questions based on your comments:

1) I don't follow:
Assumption that $_POST['submit'] will always be present.. it doesn't have to be. $_SERVER['REQUEST_METHOD']
2) In terms of using count(), are you referring to this piece of code?

Code: Select all

$query="SELECT id FROM forum";
$result=(mysql_query($query));
$numrows=mysql_numrows($result);
Are you saying to do this? Instead of using the PHP function? Let MySQL do the work?

Code: Select all

$query="COUNT(id) as numrows FROM forum";
3) This sounds great, could you give an example?
The date could be shaped during selection instead of going through strtotime().

Posted: Tue Aug 21, 2007 2:40 pm
by feyd
  1. Many browsers do not require interacting with the submit button to submit a form. Some don't send the submit button when this happens. Your code requires that someone interacts with your submit button. It's not a very good user experience.
  2. Ignoring the SQL syntax error, yes.
  3. Look up DATE_FORMAT() in the MySQL manual.

Posted: Tue Aug 21, 2007 3:00 pm
by psurrena
Thanks, I understand it all but #1. How else would someone interact with the form? Return Key? Isn't that what the submit button is for?

Posted: Tue Aug 21, 2007 3:08 pm
by feyd
Submit the form using IE through one of the text input elements. You'll get your errors.

Posted: Tue Aug 21, 2007 3:22 pm
by psurrena
Great. Thanks for all your help!