3

I have a Delete link listed beside all $rows, when I mouse over them they reflect the correct id for deletion, however, when I click DELETE I get redirected to phpfile.php?id=4, for example, and nothing is deleted, no errors are posted.

while ($row = mysqli_fetch_array($r,MYSQLI_ASSOC))
{
    echo '<tr><td align="left">' .
    $row['title'] . '</td><td align="left">'
    . $row['genre'] . '</td><td align="left">'
    . $row['length'] . '</td><td align="left">'
    . $row['created'] . '</td><td align="left">'
    . $row['views'] . '</td><td align="left">'
    . "<a href='newwriter_profile.php?id={$row['upload_id']}'>Delete</a></td>" .      '</tr>';
}
echo '</table>'; // Close the table

The remainder of the code, existing on the same page:

if(isset($_GET['id'])) {
// Get the ID
$id = intval($_GET['upload_id']);


require_once ('../mysqli_connect.php'); //Connect to the db




    $delquery = "
        DELETE 
        FROM upload
        WHERE upload_id = {$id}";
    $done = @mysqli_query ($dbc, $delquery); // Run the query

    if($done) {
        // Make sure the result is valid
        if (mysqli_num_rows($done)==1) {
        echo 'Record Deleted';
        }
        else {
            echo 'error - delete failed';
        }

        // Free the mysqli resources
        @mysqli_free_result($result);
    }
    else {
        echo "Error! Query failed:" .$mysqli_error($dbc);
    }
    mysqli_free_result($done);
    mysqli_close($dbc);
}

If I can solve this bug I will solve a similar bug except with a Download function.

Unihedron
  • 10,902
  • 13
  • 62
  • 72
V1GG3N
  • 309
  • 1
  • 6
  • 22

1 Answers1

2

You are pulling $id from the non-existent $_GET['upload_id'] when you intend to use $_GET['id']. Since $_GET['upload_id'] is not set, its value is NULL, which gets interpreted as 0. Your query ends up as: DELETE FROM upload WHERE upload_id = 0

$id = intval($_GET['upload_id']);
// Should be
$id = intval($_GET['id']);

Instead of using intval(), I would suggest using more extensive bounds checking on $id. If for example, a string like "abc" were passed in ?id=abc, intval("abc") would cast it to 0 and you would pass 0 into your query. If id needs to be a positive integer, use something like:

if (ctype_digit($_GET['id'])) {
  // ok, do your query
}
else {
  // invalid input, report error to user and don't touch your database.
}

Finally, we don't see the rest of your script, but it is usually crucial when using a hyperlink to perform a delete action (or any action for that matter) that you check ownership of the row you are attempting to delete before completing the action. Make sure that the logged-in user has permission to delete the row, and if not, don't perform any database action. Otherwise, any user could pass any value into the URL to modify others' data. Suggested reading: The Spider of Doom

Michael Berkowski
  • 267,341
  • 46
  • 444
  • 390
  • Thanks for the info, Michael. What then would be the best way to go about a delete action? How should I validate ownership of the row? – V1GG3N May 11 '12 at 02:35
  • 1
    @V1GG3N Your way of using a link is fine, as long as you check ownership. Assuming you have a logged-in user of some kind, and assuming the upload row has some sort of user identifier on it, use the delete query `WHERE` clause to make sure the user can delete it, like `DELETE FROM upload WHERE upload_id={$id} AND userid=$logged_in_userid`. – Michael Berkowski May 11 '12 at 02:41
  • Thank you Michael, I made the adjustment according to my session variables to check for ownership. Would this still allow a spider to keep hitting my query though? In any case, my delete is still broken and not giving me any errors. – V1GG3N May 11 '12 at 02:54
  • @V1GG3N If the spider doesn't have a logged-in user session (which it shouldn't), the links could not succeed. – Michael Berkowski May 11 '12 at 02:56
  • I am going to rethink this and try using an associated checkbox for each row, I will still validate ownership on SESSION. Thanks again. – V1GG3N May 11 '12 at 03:12