3

I was trying to create a commenting system like that uses facebook. I use php and jquery. My code works perfect. User writes something in textarea, comment_1 and post it. Comment_1 appears directly under the textarea successfuly. If i refresh the page I can see comment_1 posted at beginning. If I try to post a new comment (comment_2), then comment_2 appears under comment_1 and again comment_1 under comment_2. For example:

At the begining: comment_1

After refresh and post new comment: comment_1 / comment_2 comment_1 So as you can see, after refresh the page it places comment_2 and under it comment_1, but also keeps comment_1 above them (like keeping comment_1 in its "memory"). If I refresh the page I will get comment_2 comment_1 which is what I finaly want, but how can I do this without refresh? Here is my code:

wall.php

<?php

<script> 
$(document).ready(function(){                           
$("#comment_process").click(function(){
    if($("#comment_text").val() != ""){ 
        $.post("comments.php?action=post", { comment: $("#comment_text").val() }, function(data) {
            $(".comments").html(data);
            $("#comment_text").val("");
        });
    } 
});   
});   
</script>


<div class="comment_container">
<div class="comment_form">

<textarea id="comment_text" ></textarea>
<input type="button" id="comment_process" value="Post"/>

</div>
</div>

<?php include_once("comments.php");?>

<div class="comments">   </div>

?>

and this is comments.php

<?php
function getComments(){

$comments = "";
        // use desc order by date in order to display comments by date
        $sql = mysql_query("SELECT * FROM comments ORDER BY comment_date DESC ") or die (mysql_error());

        if(mysql_num_rows($sql) == 0){
                $comments = " <div class='each_comment'> There are no comments ...</div> ";
        }else{
            while ($row= mysql_fetch_assoc($sql)){          
                $comments .= "<fieldset> Stefanos Says : <div class='each_comment'>  <small><em> ".$row['comment_date']." </em></small><br />".$row['comment']."</div></fieldset>  </br>"; 
            }
        }

        return $comments;  

    }


function postComments($comment){

        $comment = mysql_real_escape_string(strip_tags($comment));
        $sql = mysql_query(" INSERT INTO `comments` (comment, comment_date) VALUES ('".$comment."', now()) ");
        return true;

    }



    if((isset($_GET['action'])) && ($_GET['action'] == "post")) {
        postComments($_POST['comment']);
    }

echo getComments();
?>
user2491321
  • 673
  • 10
  • 32
  • 1
    You are going to get hacked as soon as this thing goes on the internet. You can not stop a SQL injection by escaping and stripping tags. http://php.net/manual/en/book.pdo.php – Reactgular Aug 06 '13 at 11:49
  • 1
    @MathewFoscarini Incorrect, [mysql_real_escape_string](http://php.net/manual/en/function.mysql-real-escape-string.php) is safe for use in production environments. The reason for switching to PDO/mysqli is to use the superior prepared statements, not an intrinsic issue with mysql library. – Glitch Desire Aug 06 '13 at 11:52
  • 1
    @LightningDust Please don't tell me I'm wrong unless you know what you are talking about. http://stackoverflow.com/questions/5741187/sql-injection-that-gets-around-mysql-real-escape-string – Reactgular Aug 06 '13 at 11:59
  • 1
    @MathewFoscarini What kind of idiot doesn't use quotes around a string value such as a comment's contents? Personally I refuse to move away from the `mysql` extension, simply because I see no reason to. Escaping your MySQL inputs is just as second-nature to me as `htmlspecialchars` to prevent XSS. – Niet the Dark Absol Aug 06 '13 at 12:01
  • @MathewFoscarini If you don't know the difference between a `SELECT` statement and an `INSERT` statement, I'll leave that for you to figure out by yourself. Suffice to say, the worst case scenario here is that you get junk data in the database: a comment of `1=1` (probably a valid comment). – Glitch Desire Aug 06 '13 at 12:01
  • @Kolink I refuse to move away from `mysql` extension because PDO is slower and not any safer if you know what you're doing. – Glitch Desire Aug 06 '13 at 12:05
  • guys I am confused!... what do you think of this code? safe or not? What I should change to make it safe in your opinion? :) – user2491321 Aug 06 '13 at 12:07
  • @user2491321 This is safe, what Mathew pointed out is an edge case for `SELECT` statements. General rule for sanitizing inputs: If it's a string sanitize input with `$data = mysql_real_escape_string($_POST['somevalue']);` and put quotation marks around the string in the query: `mysql_query("SELECT * FROM something WHERE somefield = '$data'");`. If it's an int, sanitize input with `$data = (int) $_POST['somevalue']` and omit quotation marks around the variable in the query: `mysql_query("SELECT * FROM something WHERE somefield = $data");` – Glitch Desire Aug 06 '13 at 12:11
  • @user2491321 `strip_tags` will not prevent users from posting HTML to your comments. You need to prevent double encoded tags. `mysql_real_escape_string(strip_tags(strip_tags($comment)));`. Personally I'd drop all unimportant characters. – Reactgular Aug 06 '13 at 12:17
  • @Kolink Do you see anything wrong with the explanation in my updated answer? – Glitch Desire Aug 06 '13 at 12:23
  • 1
    @user2491321 I wouldn't use `strip_tags` in this instance because it will prevent harmless things being written in comments. You're better off using `$comment = mysql_real_escape_string(htmlspecialchars($comment))` ([documentation](http://php.net/manual/en/function.htmlspecialchars.php)) so that if users write tags in their comments, they just display as part of the comment when it's re-displayed. – Glitch Desire Aug 06 '13 at 12:35
  • 1
    @LightningDust yes, `htmlspecialchars` is much better! I stand corrected :). – Reactgular Aug 06 '13 at 12:58

1 Answers1

5

Issues with Comments Displaying Wrong

The issue here is that you're not clearing the original comments.

Move <?php include_once("comments.php");?> into this div:

<div class="comments"> </div>

This way, when you write comments to that block with your javascript, the original comments which were loaded when the page loaded will be replaced.

Apparent issues with MySQL injections and mysql extension

Correct Practices

How you SHOULD sanitize a string for a SELECT statement:

$data = mysql_real_escape_string($_POST['some_data']);
$query = mysql_query("SELECT * FROM some_table WHERE some_value = '$data'");

How you SHOULD sanitize an integer for a SELECT statement:

$data = (int) $_POST['some_data'];
$query = mysql_query("SELECT * FROM some_table WHERE some_value = $data");

Note that the integer does not have quotation marks around it.

The Apparent Vulnerability

Mathew correctly pointed out in the comments that this brand of select statement (demonstrated by ircmaxell) would not correctly prevent SQL injection attacks:

$data = mysql_real_escape_string($_POST['some_data']);
$query = mysql_query("SELECT * FROM some_table WHERE some_value = $data");

However, this isn't a vulnerability so much as a misuse of the function, by not putting quotations around $data we suggest that we're searching for an integer, but we've sanitized our input as if it were a string.

If it's an integer field, we should be casting the input value as an integer. If it's a string field, there should be quotes around it in the query. All that's been displayed here is that a function can be used incorrectly.

As your SELECT statement does not take any user inputs and just selects ALL comments, there is no opportunity for this kind of injection anyway.

Glitch Desire
  • 14,632
  • 7
  • 43
  • 55