Trouble converting to parameterized queries

Question

So i'm trying to convert all of my SQL statements to prepared statements etc to prevent SQL injection attacks, but i'm having some issues fetching stuff etc

My code:

if($_GET["action"] == "ban"){

    if(isset($_GET["username"])){

        $username = $_GET["username"];

        $banMsg = $_GET["banMsg"];

        $email = "test@gmx.ch";

        $sql = "SELECT * FROM bans WHERE username = ?";
        $stmt = $db->prepare($sql);
        $stmt->bind_param("s", $username);
        $stmt->execute();
        $result = $stmt->fetch();
        $stmt->close();

        if($result->num_rows > 0){ //LINE 61
            die(json_encode(array("status" => 400, "message" => "User already banned")));
        }

        $result2 = $db->prepare("INSERT INTO bans (username, ip, email, message, expire, ban_creator) VALUES (?, ?, ?, ?, ?, ?)");
        $result2->bind_param("sssssd", $username, null, $email, $banMsg, null, 1); // LINE 72^^
        $result2->close();
        if($result2){
            updateBanCache();
            die(json_encode(array("status" => 200, "message" => "Successfully banned")));
        } else {

            die(json_encode(array("status" => 400, "message" => "SQL error")));
        }

    }

Also $result = $stmt->get_result(); doesn't wanna work for me, i do have mysqlnd driver installed in my php / cpanel though.

Any pointers would be helpful thanks!

ERROR LOG:

[11-Nov-2020 04:46:04 America/New_York] PHP Notice:  Trying to get property 'num_rows' of non-object in /home/public_html/index.php on line 61
[11-Nov-2020 04:46:04 America/New_York] PHP Fatal error:  Uncaught Error: Cannot pass parameter 3 by reference in /home/elysianmenu/public_html/index.php:72
Stack trace:
#0 {main}
  thrown in /home/public_html/index.php on line 72

SIDE NOTE: I also tried using $result = $stmt->get_result(); but I end up with error:

[11-Nov-2020 04:57:30 America/New_York] PHP Fatal error:  Uncaught Error: Call to undefined method mysqli_stmt::get_result() in /home/public_html/index.php:55
Stack trace:
#0 {main}
  thrown in /home/public_html/index.php on line 55

^^ Yes i do have the mysqlnd driver installed

Answer 1

From the docs: Fetch results from a prepared statement into the bound variables.

fetch() returns either TRUE, FALSE or NULL, but not the result set you expected. Instead, it sets the output to the variables you previously bound (using bind_param()) by reference. This is why you have to use variables, and not actual scalar types.

If your query did not return any rows, fetch() will return NULL. Update your code as follows:

$stmt = $db->prepare($sql);
$stmt->bind_param("s", $username);
$stmt->execute();
if ($stmt->fetch() === TRUE)
    die(json_encode(array("status" => 400, "message" => "User already banned")));
$stmt->close();

And to fix the error on line 72, you have to pass the values by reference, using variables. Something like this:

$ip = NULL;
$expire = NULL;
$ban_creator = 1;

$result2->bind_param("sssssd", $username, $ip, $email, $banMsg, $expire, $ban_creator);

Don't forget to execute the query! You're checking $result2 before anything actually happened.

Answer 2

1. The action of banning a user MUST NOT come from a GET request ($_GET["action"]). This would make it incredibly simple for a webcrawler to stumble upon your banning script and ban all of your users (if it somehow found a list of usernames). The whole payload should be coming in as $_POST. The bottomline is: use $_POST when you are writing data, use $_GET when you are reading data.

2. You MUST NOT blindly trust the user input. You should be validating the data even before connecting to the db. If the payload is invalid, no resources should be engaged.

3. When you are only interested in the row count of a result set (and not the values in the result set), write COUNT(1) in your query. This way you can check the lone value to be zero or a non-zero value with no unnecessary overheads. Use something like this https://stackoverflow.com/a/51259779/2943403

4. ip, expire, and ban_creator should have default settings in your table declaration of NULL, NULL, and 1. You should only mention those columns in an INSERT query if you wish to store a different value. Your INSERT query should only be binding 3 parameters. And of course check the outcome of the executed insert, like this: $stmt->execute() : How to know if db insert was successful?