-1

I have created this simple function to be able to delete a row from a database based on the username thats inputted. However i cannot get the row to actually delete, even if the username entered is correct. Does anyone have any suggestions as to why this isnt working? (I am new to php, so sorry if there is issues with injections and/or layout / coding)

<?php
    $uname = "xxxx";
    $password = "xxxx";
    $host = "xxxx";
    $db = $uname;
    $connection = mysqli_connect($host, $uname, $password, $db);
    if (mysqli_connect_error()) {
        echo "Failed to connect to MySQL: " . mysqli_connect_error();
    } else {
        echo "<h1> Delete User </h1>";
        if (isset($_POST["delSubmit"])) {
            if ((!empty($_POST["username"]))) {

                $query = "DELETE FROM login WHERE username = " . $_POST['username'] . ";";
                $result = mysqli_query($connection, $query);

                if ($result == false) {
                    echo "<p> Deleting user " . $_POST["username"] . " failed </p>";
                } else {
                    echo "<p> The user \"" . $_POST["username"] . "\" has been deleted";
                }
            }
        }
    }

?>
    <!DOCTYPE html>
    <html>
    <head>
    </head>
    <body>
    <form id="delForm" name="delForm" action="?" method="post">
        <div>
            <label for="delFormID">Please insert username to delete</label>
            <input id="delFormID" name="username">
        </div>
        <div>
            <input id="delSubmit" name="delSubmit" value="Delete User" type="submit">
        </div>
    </form>
    </body>
    </html>
<?php
    mysqli_close($connection);
?>
Cemal
  • 1,469
  • 1
  • 12
  • 19
Sam
  • 1
  • 1
  • 3
    That is basic sql syntax: You need to quote string values. However, you really need a prepared statement to avoid sql injection. – jeroen Apr 09 '18 at 12:23
  • Noooo @AshishPatel that's also wrong.. Prepared statements is to way to go here. – Raymond Nijland Apr 09 '18 at 12:24
  • Prevent SQL injection a must read https://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php – Raymond Nijland Apr 09 '18 at 12:26
  • You also need to escape your html output with [htmlspecialchars](http://php.net/manual/en/function.htmlspecialchars.php). – Peter Apr 09 '18 at 12:30

3 Answers3

3

use prepared statement like below:

$stmt = $mysqli->prepare("DELETE FROM login WHERE username = ?");
$stmt->bind_param("i", $_POST["username"]);
$stmt->execute();
Mahadeva Prasad
  • 709
  • 8
  • 19
  • This way will mix mysqli procedural api `mysqli_connect` (topic starters code) with the object api `$mysqli->prepare` (your code) which is not recommended to do within the php docs -> http://php.net/manual/en/mysqli.quickstart.dual-interface.php – Raymond Nijland Apr 09 '18 at 12:38
-1

Solution to your problem:

As others have mentioned, change

$query = "DELETE FROM login WHERE username = " . $_POST['username'] . ";";

to

$query = "DELETE FROM login WHERE username = '" . $_POST['username'] . "';";

Strings must be surrounded by " or ' in MySQL (actually, every SQL implementation I know).

Additional remarks

1) You may leave out the ; at the end of the string if the string contains only one statement. That means that you could change the above line to

$query = "DELETE FROM login WHERE username = '" . $_POST['username'] . "'";

2) NEVER SEND UNTRUSTED DATA TO ANY SQL SERVER THAT WAY since this makes your code prone to SQL injection. Instead, use prepared statements or stored procedures (in the latter case, make sure that your SP's code does not construct literal SQL strings from the SP's parameters again, or you will run into possible SQL injection again).

Binarus
  • 4,005
  • 3
  • 25
  • 41
  • 1
    So the OP should change it like in your answer but really should not do that. Strange answer. And it is not just about untrusted data; valid data can also lead to sql injection. – jeroen Apr 09 '18 at 12:43
  • 1
    "You may leave out the ; at the end of the string if the string contains only one statement" functions `mysqli_query` or `mysql_prepare` would wont execute multiple SQL statements separated with semicon (`;`) anyway.. – Raymond Nijland Apr 09 '18 at 12:44
  • "prone to SQL injection. Instead, use prepared statements or stored procedures." -> stored procedures can also be prone to SQL injections when you use MYSQL's PREPARE clause within them – Raymond Nijland Apr 09 '18 at 12:47
  • @RaymondNijland Regarding your first comment: I don't know PHP, but since it is true what I wrote in every other language I have seen, I was absolutely sure that it is the same in PHP. So leaving away the `;` is the right advice in any case. – Binarus Apr 09 '18 at 13:14
  • @jeroen No he shouldn't. What else could I do than using a bold font and capital letters to say DON'T DO THAT? And regarding "valid" data: No, if the validation is done the right way, it can't lead to SQL injection. In contrast, I personally even would *define* a correct validation to be a validation which prevents SQL injection in any case. The problem is that it is complicated (if not impossible for most people) to write a validation which is 100% error free. And additionally, it is just not necessary if we use parametrized queries. – Binarus Apr 09 '18 at 13:21
  • @RaymondNijland Regarding your second comment: Of course, I was relating to the wide-spread and correct design pattern where the SP takes all strings needed as parameters, and the code in the SP does not construct literal SQL strings from that parameters again. Nevertheless, edited my answer ... – Binarus Apr 09 '18 at 13:29
  • 2 of your 3 suggestions / the only 2 lines of codes that you suggest are wide open to sql injection. So either don't suggest that or simply replace the quoted POST variable with a question mark so that you have a query you can prepare. – jeroen Apr 09 '18 at 13:34
  • @jeroen Sorry, but no. The OP did not ask how to make his code immune against SQL injection. Instead, he asked why the respective row didn't get deleted. It is exactly this question what I have answered. And SO is all about answering questions, isn't it? Being responsible, I have additionally told the OP to not use that code in production. But if somebody would be evil, he could even tell me: "Hey, that SQL injection stuff is off-topic here. The OP didn't ask for such things." - and then downvote my answer because I have answered something which hasn't been asked. – Binarus Apr 09 '18 at 13:40
  • You should probably get used then to getting downvoted when you post answers that have sql injection problems... – jeroen Apr 09 '18 at 13:44
  • @jeroen This is exactly why I have put the section "Additional remarks" in my answer. I have answered the question, and I additionally have told the OP why he shouldn't use that sort of code (in fat capital letters). If it is unclear to him how to use prepared statements or SPs, he will ask a new question. And we should consider that everybody has different preferences when it comes to preventing SQL injection. So making a suggestion here would be opinion-based (somehow ...). – Binarus Apr 09 '18 at 13:49
  • @Binarus Thank you for the help! This now deletes, but will not give the error message when it fails? Also - thank you for the advice on SQL injection, i'm pretty new to all this so it's only for me to use at the moment, but i will be looking up more into what you have said, thank you again :) – Sam Apr 09 '18 at 14:00
-2

Define action="" instead of action="?".

change query to

$query = "DELETE FROM login WHERE username = '".$_POST['username']."'";

Ravi Thanki
  • 251
  • 2
  • 14
  • 2
    Don't do this. Use [parameterized queries](http://php.net/manual/en/mysqli.quickstart.prepared-statements.php), always. Always! – Peter Apr 09 '18 at 12:28
  • And `action=""` is invalid html 5.. in html 5 you need to remove the attibute `action=""` to post to the current page and keep the html5 valid. – Raymond Nijland Apr 09 '18 at 12:33
  • my username is : **UsePrepearedStatements'; Drop TABLE \`login\`;--** . Try this and see if it works. – Cemal Apr 10 '18 at 09:45