0

I am having a problem with my deleting comment form. I want to create somehow users to delete their own comments, but so far I only manage to create deleting for all comments. It doesn't matter if it's their own or not.

Here is my form:

<form action="<?php echo $editFormAction; ?>" method="POST" name="CommentForm">
<table>
<tr><td colspan="2"></td></tr><tr><td><input type="hidden" name="name" value="<?php     
    echo $_SESSION['MM_Username']; ?>" /></td></tr>  

 <tr><td colspan="2">Заглавие:</td></tr><tr><td><input type="text" name="title"  
 style="width:300px; height:20px;" /></td></tr>
 <tr><td colspan="2">Коментар</td></tr>
 <tr><td colspan="2"><textarea name="comment" style="width:450px; height:150px;"> 
   </textarea></td></tr>
  <tr><td colspan="2"><input type="submit" name="submit" value="Коментирай"                     
    style="padding:5px; color:#069"/></td></tr>

</table>
   <input type="hidden" name="MM_insert" value="CommentForm" />


   </form><br /><br  />
<?php

  $getquery=mysql_query("SELECT * FROM comments ORDER BY id DESC");

  while($rows=mysql_fetch_assoc($getquery))
   {
 $id=$rows['id'];
 $title=$rows['title'];
 $comment=$rows['comments'];
 $MM_Username=$rows['name'];
 $dellink="<a href=\"delete.php?id=" . $id . "\">ИЗТРИЙ</a>";

 echo '<b><p   style="background-color:#6CC; border-radius:10px; te"
     ;border:1px solid;"> &nbsp Потребител:</b> ' . $MM_Username . ' <br /><b>&nbsp   
     Тема:</b> ' . $title . '</b><br /><b>&nbsp Коментар:</b><br />&nbsp ' . $comment .
      '<br />' . $dellink .'</p><br />' ;

    }

  ?>

and this is my delete.php :

 <?php
  include 'Connections/localhost.php';
  mysql_query("DELETE FROM comments WHERE id = $_GET[id]")or die(msql_error()) ;
  header('location: komentari.php');
 ?>
gen_Eric
  • 223,194
  • 41
  • 299
  • 337
user3194814
  • 1
  • 1
  • 3
  • 4
    Well, for starters, you're not doing any check for whether or not the user is *allowed* to delete the comment. So any user can delete any comment. Your `delete.php` should have some kind of authorization check to ensure that the user making the request is allowed to delete the comment. Also, and this is ***very important***, your code is *wide open* to SQL Injection attacks. Users can not only delete comments, they can take full control of your server. Please read this: http://www.php.net/manual/en/security.database.sql-injection.php – David Jan 14 '14 at 16:50
  • 5
    **Danger**: You are using [an **obsolete** database API](http://stackoverflow.com/q/12859942/19068) and should use a [modern replacement](http://php.net/manual/en/mysqlinfo.api.choosing.php). You are also **vulnerable to [SQL injection attacks](http://bobby-tables.com/)** that a modern API would make it easier to [defend](http://stackoverflow.com/questions/60174/best-way-to-prevent-sql-injection-in-php) yourself from. – Quentin Jan 14 '14 at 16:51
  • Don't use links for operations that edit the database. You'll get bots (and precaching proxies) following them and deleting things wildly. Use a POST form. – Quentin Jan 14 '14 at 16:55
  • When showing the comments, send its `id` and put it somewhere you can retrieve to send with the form. This way, you don't need to delete all comments. – Minoru Jan 14 '14 at 16:56
  • Thank u very much guys I know there is a lot a lot a lot to study most of the things u said i didnt understand :D but i used this: IF ([owner] == $_SESSION['CurrentUser']) $dellink="ИЗТРИЙ"; ELSE $dellink="" probably it is not the right but so far it do the job. It simply hide the Delete link if the corect user who is logged is not the one who post the comment so it is good so far for me thanks again : ) and have a great day, week, month, year or maybe 100 years :D – user3194814 Jan 14 '14 at 18:23

3 Answers3

1

I would assume that a user is logged in and you have some information about that user stored in a session. If it is not there already, you should add the users ID so that you can do in your delete.php:

<?php
session_start();

// Here you should kick out any visitors that are not logged in


// proceed with the delete
include 'Connections/localhost.php';

$id = (int) $_GET[id];
$uid = (int) $_SESSION['user_id'];    // or something similar

mysql_query("DELETE FROM comments WHERE id = $id AND owner_id = $uid")or die(msql_error()) ;
                                                     ^^^^^^^^ or something similar
// etc.

And you should really switch to PDO or mysqli and prepared statements as you have an sql injection problem now and the mysql_* functions are deprecated.

jeroen
  • 91,079
  • 21
  • 114
  • 132
0

The simplest solution would be to add AND owner = ? to the end of the DELETE query. Populate the value from wherever you store the ID of the current user (probably the session). You will need to store the id of the user who made the comment in an owner column on the table.

You'll probably want to add some code to test if the delete was successful, and possibly to make some more queries to find out why if it isn't so you can give good error reporting.

You can avoid generating the delete link inappropriately by comparing the owner value you get out of the database with the one for the current user.

Quentin
  • 914,110
  • 126
  • 1,211
  • 1,335
  • The problem with this solution is that the forum will only display comments the currently logged in user owns.... So I'm not sure that's what the OP wants. – Dylan Brams Jan 14 '14 at 16:52
  • @DylanB — Err, the DELETE query, not the SELECT query. – Quentin Jan 14 '14 at 16:53
0

You're going to have to fix two problems here. First, your 'delete.php' should only process where the owner of the comment is equivalent to the logged in user. For this you're going to need to add

" AND [owner] = $_SESSION['CurrentUser']"

Note that this depents on your datastructure.

The second point you're going to need to put in some logic is around the definition of

$dellink="<a href=\"delete.php?id=" . $id . "\">ИЗТРИЙ</a>";

At this point, you're going to want to add:

IF ([owner] == $_SESSION['CurrentUser'])
   $dellink="<a href=\"delete.php?id=" . $id . "\">ИЗТРИЙ</a>";
ELSE
   $dellink=""

Or the PHP equivalent. Sorry, I haven't programmed in PHP in a while. But both the other current answers don't seem to fix both issues.

Dylan Brams
  • 2,089
  • 1
  • 19
  • 36
  • 1
    You do realize that `$_GET` comes from the user and can be anything the user wants it to be, right? – jeroen Jan 14 '14 at 16:56
  • Err... No. I changed it to $_SESSION, but the point remains. You shouldn't display the DELETE link if the user can't delete, so you should check whether the user can delete before displaying the link. – Dylan Brams Jan 14 '14 at 17:00