8
\$\begingroup\$

This is a comment system that I wrote, is it secure?

<!doctype html>
<html>
<head>
<meta charset="utf-8">
<link rel="stylesheet" type="text/css" href="style.css">
<?PHP 

//Turn off error reporting (Not Necessary)
    error_reporting(E_ALL ^ E_NOTICE);

//Connect to the database
    if (!@mysql_connect('localhost', 'user', '') or !@mysql_select_db('comments')) {
        die('Could not connect, please check back later.'); 
    }

//Variables
    $guest_name = htmlentities(str_replace(' ', '',$_POST['guest_name']));
    $comment = htmlentities($_POST['comment']);
    $time = date('g:i A', time());
    $date = date('n/j/Y');
    $query = "INSERT INTO `user_comments` VALUES(
        '".mysql_real_escape_string('')."',
        '".mysql_real_escape_string($guest_name)."',
        '".mysql_real_escape_string($comment)."',
        '".mysql_real_escape_string($date)."',
        '".mysql_real_escape_string($time)."'
        )";
    $array_query = mysql_query("SELECT * FROM `comments` ORDER BY `ID` DESC");

//Insert comment into the database
    if (isset($_POST['submit_comment']) && !empty($comment) && !empty($guest_name)) {
        if(mysql_query($query)) {
            header('Location: index.php');  
        }
    } 

//Check if user has filled in all fields    
    if (isset($_POST['submit_comment']) && empty($guest_name)) {
        echo '<span style="color:red">*Please enter a Guest Name*</span><br>';  
    } 

    if (isset($_POST['submit_comment']) && empty($comment)) {
        echo '<span style="color:red">*Please enter a Comment*</span>'; 
    } 

//List comments
    function list_comments () {
        $query = "SELECT * FROM `user_comments` ORDER BY `ID` DESC";
        $query_run = mysql_query($query);
        while ($result = mysql_fetch_array($query_run)) {
            echo '<div id="comment_border">' . $result['Guest Name'] . ' - ' . $result['Date'] . ' - ' . $result['Time'] . '<br>' . $result['Comment'] . '<br></div><br>';  
        }
    };

?>
<title>Main_Page</title>
</head>

<body>
<!-- \/ COMMENT FORM \/ -->
<form method="post" action="index.php">

    <input type="text" name="guest_name" maxlength="15" placeholder="Guest Name"><br>
    <textarea cols="70" rows="5" name="comment" maxlength="512" placeholder="Comment"></textarea>
    <input type="submit" name="submit_comment">

</form>
<!-- /\ COMMENT FORM /\ -->
<hr><h1>Comments</h1><br>
<!-- Comments will list below here -->
<?PHP

    list_comments();

?>
</body>
</html>
\$\endgroup\$

2 Answers 2

Reset to default
13
\$\begingroup\$ 
<?PHP

Normally it's all lower case, just saying.

//Turn off error reporting (Not Necessary)
error_reporting(E_ALL ^ E_NOTICE);

This is one prime example of confusing and not helpful commenting. If this call is not necessary, why is it there at all?

if (!@mysql_connect('localhost', 'user', '') or !@mysql_select_db('comments')) {

Stop using the mysql_* functions, seriously.

$guest_name = htmlentities(str_replace(' ', '',$_POST['guest_name']));

Why are you replacing entities here? You should not replace stuff when saving it into your database. Ideally the data in your database would be as unbiased as possible. Only replace entities if you need to, f.e. you display it on a webpage.

$time = date('g:i A', time());
$date = date('n/j/Y');
$query = "INSERT INTO `user_comments` VALUES(
    '".mysql_real_escape_string('')."',
    '".mysql_real_escape_string($guest_name)."',
    '".mysql_real_escape_string($comment)."',
    '".mysql_real_escape_string($date)."',
    '".mysql_real_escape_string($time)."'
    )";

From this query I can tell you a few things:

  1. Your database layout is sub-optimal, it uses VARCHAR fields if it should use a DATETIME field.
  2. You like to copy and paste code, that's bad.
  3. You don't do things explicitly, consider adding a fields list to your queries.

First, you should be using a DATETIME type if you're gonna save date and time. MySQL supports also a 'pure' DATE and TIME format, there's absolutely no reason and no excuse to use use a VARCHAR field for that.

And then there's your missing fields list, I made a habit out of adding a fields list to my queries for two reasons:

  1. It let's the reader know exactly what is getting set to where.
  2. It makes your code more robust, the query will not fail if you add a field. Removing a field is also easier, as a simple grep for the field will yield all queries that use it.

So, your query should look like this:

$query = "
    INSERT INTO 
        user_comments
    SET
        name = :name,
        comment = :comment;
";

It's easy to read and you know exactly what ends where. Additionally the date/time stuff is here completely missing, because I changed the database layout:

CREATE TABLE user_comments
    name VARCHAR,
    comment VARCHAR,
    timestamp TIMESTAMP DEFAULT CURRENT_TIMESTAMP;

The TIMESTAMP type is optimal for this kind of operation, as it is able to save automatically the time when the row is created.

$array_query = mysql_query("SELECT * FROM `comments` ORDER BY `ID` DESC");

Same here, your usage of the * makes it easily broken by adding new fields. Additionally you don't know what fields are fetched until you look at the table declaration.

header('Location: index.php');

I'm 99% sure that this should not work. Headers can not be changed after data was send, you set the header in the middle of the file, but your php script should be moved to the top of the file. Additionally if you want to redirect people, it would be good to die() afterwards to make sure that your script stops executing at this point:

<?php
    if (condition) {
        header("Location: othersite.php");
        die("Redirect: <full address here>");
    }
?>
<html>
    <head>
    ...

This will also allow everyone who is not automatically redirected to see where to go to now.

echo '<span style="color:red">*Please enter a Guest Name*</span><br>';

You're outputting stuff while inside the <head>, that's bad practice, content should go into the <body>.

<!-- \/ COMMENT FORM \/ -->

What is this?!

And just to make sure:

Stop using the mysql_* functions and start using prepared queries!

\$\endgroup\$
11
\$\begingroup\$

One of the most common mistakes for PHP programmers is that they don't seem to have been looking at the big red box in the documentation for mysql_query, neither have they seen one of the many comments on Stack Overflow pointing to this question, and they have also missed the guide to choosing a database API in the PHP documentation.

Summary: (can't be emphasized enough, really...)

#DON'T USE THE MYSQL_* FUNCTIONS! THEY ARE DEPRECATED!

Instead, use PDO or mysqli. Also use prepared statements and you won't have to call as many functions to try and protect yourself from SQL injection attacks, the prepared statements will do a lot for you in preventing such attacks.

Another potential problem I see in your code is that you're not protecting yourself from spam-bots in any way. Once automatic bots find your form, they will use it. A lot. Use some PHP Captcha script to prevent bots from spamming you to oblivion.

\$\endgroup\$
7
  • 3
    \$\begingroup\$ I think your summary could be a bit bigger, just to make sure. \$\endgroup\$
    – Bobby
    Commented Feb 8, 2014 at 19:42
  • \$\begingroup\$ @Bobby I would make it bigger if I could. \$\endgroup\$
    – Simon Forsberg
    Commented Feb 8, 2014 at 19:45
  • 3
    \$\begingroup\$ Where is the blink tag if, for once, there's a legit usage for it?! \$\endgroup\$
    – Bobby
    Commented Feb 8, 2014 at 20:10
  • 1
    \$\begingroup\$ @Bobby In the future, we could use this link \$\endgroup\$
    – Simon Forsberg
    Commented Feb 8, 2014 at 20:24
  • 1
    \$\begingroup\$ I think the OP got the message for now... \$\endgroup\$
    – Simon Forsberg
    Commented Feb 8, 2014 at 20:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.