Shred this code

hubbardude

Maybe this should go in the Newbies section, but I thought the coding forum more appropriate. It's a simple update page. Mainly, I'd like to know if I can make this any safer from potential hacks. Plus, if I have any unnecessary code or I can make it more efficient in any way I'd love to know how. So please, tear it apart. You won't hurt my feelings, I swear.

First part - top of page

<?php
include 'data.php';

if(isset($_POST["submit"])) {

if (empty($_POST['title']) || empty($_POST['body'])) {
	$messages[]="Please fill in all required fields";
}

else {
	$news_id=$_POST['news_id'];
	$title=$_POST['title'];
	$body=$_POST['body'];

	mysql_query(" UPDATE news SET title='$title' ,body='$body' WHERE news_id='$news_id'");
	header("Location: index.php");
}

}

?>

Code within <body> section:

<?php
if(!empty($messages)){
	displayMessages($messages);
}

$newssql = "SELECT * FROM news
WHERE news_id = '" . $_GET['news_id'] . "'";
$result = mysql_query($newssql)
or die("Invalid query: " . mysql_error());
$row = mysql_fetch_array($result);
$news_id = $row['news_id'];
$title = $row['title'];
$body = $row['body'];
?>

<form action="<?=$_SERVER["PHP_SELF"]?>" method="POST">
<input type="hidden" name="news_id" value="<? echo "$news_id" ?>">
News Title: <input type="text" name="title" value="<?php echo $title; ?>"><br />
News Body: <textarea name="body" cols="50" rows="20"><?php echo $body; ?></textarea>
<input type="submit" name="submit" value="Update">
</form>

NogDog

The first thing you need to do is not trust any data from external sources, including form data. (It's very easy to send fake form data, plus you never know what stupid things even non-malicious users may type into a form.) You should check each of the $_POST fields that you are going to use in your script to ensure that the values are valid in terms of content and length, rejecting the input with an error if not.

Secondly, you need to protect against SQL injection attacks with any such values used in your database queries. This is rather easily done by using the [man]mysql_real_escape_string/man function. (The linked page includes a number of useful examples.) As to why this is an issue, see this page.

PS: For an example and some comic relief, see http://xkcd.com/327/. 🙂

PPS: Also, please learn to use the

 [url=http://phpbuilder.com/board/misc.php?do=bbcode]BBCode tags[/url] around your code samples to make them much easier for us to read.

peytonrm

The above is very good advise. I would also recommend not echoing errors to the user as you are doing in your die statement. This results in unprofessional looking pages (weird output that nontechnical users dont get) as well as security holes (e.g. an error might contain database table names, usernames, etc. that an attacker could use in attempting an attack).

It doesn't really matter in a script this simple, but you should strive to always follow the MVC design pattern (google 'mvc pattern' if you aren't familiar). Doing so will make your code much more maintainable.

nebcode

peytonrm wrote:
The above is very good advise. I would also recommend not echoing errors to the user as you are doing in your die statement. This results in unprofessional looking pages (weird output that nontechnical users dont get) as well as security holes (e.g. an error might contain database table names, usernames, etc. that an attacker could use in attempting an attack).

It doesn't really matter in a script this simple, but you should strive to always follow the MVC design pattern (google 'mvc pattern' if you aren't familiar). Doing so will make your code much more maintainable.

The MVC pattern is very useful for large projects however it is not the end all and be all of PHP programming. It is true however that using die() for anything other than testing is a decidedly user-unfriendly and HTML non-compliant way of handling errors. MVC isn't about handling errors properly however. Use if constructs in your code to control the flow of your application and present friendly messages to your users. Don't reveal technical details about your system to visitors.

All MySQL functions return a value that evaluates to false if there is a problem. Use that to control flow and don't assume that even if your syntax for your quries has been debugged and determined to be correct that it will always execute.

As mentioned, escaping is always important, or use prepared statements.

laserlight

Read the PHP manual on [man]header/man. You will find that the URL for a location header should be an absolute URL. Also, you should use an exit (or die, which is equivalent) after sending a location header.

As mentioned, escaping is always important, or use prepared statements.

Indeed, I suggest that you switch to using the MySQLi extension or the more flexible PDO extension as they provide for prepared statements and an object based interface.