2

I have a page that shows comments, "comments.php", and I include the page on any other page that I want comments to show. I am trying to implement a way to delete comments if needed. Each comment has an auto-increment "commentID". Right now I'm using an action, and then just using a link to call the action.

When I hover over the link, the URL looks correct, but when I click it, the page refreshes and nothing happens. Any ideas?

Action:

if ($_POST['action'] == 'delete') {
    $sql = "delete from " . $db_prefix . "comments where commentID = " . (int)$_GET['id'];
    mysql_query($sql) or die('error deleting user: ' . $sql);
header('Location: ' . $_SERVER['HTTP_REFERER']);
}

Show comments and show link to delete: (unnecessary code has been left out)

echo '<a href="/comments.php?action=delete&id=' . $result['commentID'] . '">delete</a>

What am I doing wrong?

Mark Jones
  • 193
  • 1
  • 9
  • Why are you typecasting in the query? – Script47 Aug 14 '15 at 01:34
  • Sorry, not sure I understand what you mean. – Mark Jones Aug 14 '15 at 01:36
  • 1
    you're using the wrong method as @Script47 said. `?xxx` is a GET method, not POST. – Funk Forty Niner Aug 14 '15 at 01:36
  • Also maybe I'm missing something but you check for `$_POST['action']` yet use `$_GET` in the query. – Script47 Aug 14 '15 at 01:36
  • 1
    `$_SERVER['HTTP_REFERER']` is unreliable http://stackoverflow.com/a/6023980/ so don't "rely" on that. – Funk Forty Niner Aug 14 '15 at 01:44
  • @Script47 The typecasting is so the value must be an int, a way to avoid injection. https://eval.in/416528 – chris85 Aug 14 '15 at 01:46
  • You should require some sort of authentication on the delete page. As is a malicious user could right a simple < 4 line script and delete a majority of your records, if not all of them. – chris85 Aug 14 '15 at 01:48
  • @chris85 he could use `intval();` but my issue was, why was he doing it ***in*** the query. Why not assign a variable outside of the query, it would make his code cleaner. – Script47 Aug 14 '15 at 01:51
  • Regarding your 404 below; you do realize that `/comments.php` goes to the root of your public folder. Make sure it exists in there and that it's not named "Comments.php" with an uppercase "P". On Linux, those are two different filenames. Check that that file doesn't have an include/require which contains a file that either doesn't exist, or is somewhere else on the server. Also check your form's action if there is one. – Funk Forty Niner Aug 14 '15 at 01:52
  • If you're using the same file for everything, just omit the file name `?action=delete&id` – Funk Forty Niner Aug 14 '15 at 01:55
  • `or die('error deleting user: ' . $sql);` doesn't help you, `or die(mysql_error())` does. Edit: I have to hit the hay, good luck. – Funk Forty Niner Aug 14 '15 at 01:56
  • 1
    Thank you! I took out `/comments.php` and it started working. Along with Script47's answer. Thanks to both of you! – Mark Jones Aug 14 '15 at 01:59

1 Answers1

4

In your code you're mixing $_POST with $_GET.

Try this,

 ?php
 if ($_GET['action'] == 'delete') {
    if (!ctype_digit($_GET['id'])) {
        exit("ID has to be an int.");
    }
    $id = intval($_GET['id']);

    $sql = "DELETE FROM `" . $db_prefix . "comments` WHERE `commentID` = " . $id;

    mysql_query($sql) or die('error deleting user: ' . $sql);

    header('Location: ' . $_SERVER['HTTP_REFERER']);
}
?>

Your link also shows action=delete so you should be checking if $_GET action equals delete.

Edit 1

Your code is prone to SQL injection, you are still using MySQL even though it has been deprecated, you should use either MySQLi or PDO with prepared statements.

Not to mention your $_GET data is being passed on to the query without being sanitized, you should start using intval it would make it better and prevent XSS. Please read up on the function intval and ctype_digit to get a better understanding at what it does.

Edit 2

Scrap $_SERVER['HTTP_REFERER']

How reliable is HTTP_REFERER?

Edit 3

As noted in comments:

"If you're using the same file for everything, just omit the file name ?action=delete&id"

which would explain the 404 you mentioned in comments.

Community
  • 1
  • 1
Script47
  • 14,230
  • 4
  • 45
  • 66