How does this look (simple forum)

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

Post Reply
User avatar
psurrena
Forum Contributor
Posts: 355
Joined: Thu Nov 10, 2005 12:31 pm
Location: Broolyn, NY

How does this look (simple forum)

Post 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';
?>
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post 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.
User avatar
psurrena
Forum Contributor
Posts: 355
Joined: Thu Nov 10, 2005 12:31 pm
Location: Broolyn, NY

Post 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().
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post 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.
User avatar
psurrena
Forum Contributor
Posts: 355
Joined: Thu Nov 10, 2005 12:31 pm
Location: Broolyn, NY

Post 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?
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

Submit the form using IE through one of the text input elements. You'll get your errors.
User avatar
psurrena
Forum Contributor
Posts: 355
Joined: Thu Nov 10, 2005 12:31 pm
Location: Broolyn, NY

Post by psurrena »

Great. Thanks for all your help!
Post Reply