0
\$\begingroup\$

I've been geeking up on MySQLi prepared statements to avoid potential SQL injection and I have managed to modify my legacy code (previously just using mysqli_real_escape_string).

I am looking for some advice regarding if this code template is safe enough to use as I will be looking to update the rest of my code across my site. I received some help from Fred-ii so thanks for that.

One issue I know is present is the use of $_SERVER['HTTP_REFERER'] but I will be creating a workaround via an array of acceptable URLs.

<?php 
// Starting Session

session_start(); 
include '../connection.php';

// Previous page variable

$previous = $_SERVER['HTTP_REFERER'];

//Acquire POST values Please note: Sanity checks will be added for these =)

$myusername = $_POST["username"];
$mypassword = $_POST["password"];

// Preparing the statement

$stmt = $connection-> prepare("SELECT username, password FROM $tbl_name WHERE username= ? ");
$stmt -> bind_param("s", $myusername);

// Sanity checks on the statement and binding

if($stmt ->execute() == true) {
    $stmt -> bind_result($user, $pass);
    $stmt -> fetch();

    //Checking hashed password

    if (password_verify($mypassword, $pass)) {

        $_SESSION['login_user']= $myusername;   
        header("location: ../member"); // Redirecting To Other Page         

    } else {

        $_SESSION["login_error"] = "<div id='loginAlert'> <p style='font-size: 1em; padding: 5px; text-align: center; background: #f2f2f2; 
        border: 1px solid #f2f2f2'>Incorrect username or password, please try again.</p> </div>";

        header('Location:'.$previous);

    }


    }

/* Close statement */
$connection-> close(); 

Looking for any feedback to improve this and any other recommendations.

Thanks. Stan.

\$\endgroup\$
  • 1
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Heslacher Dec 8 '17 at 16:07
1
\$\begingroup\$

There's no need for the $user variable (it's already in $myusername) so don't query for it either.

Please, deal with the $previous = $_SERVER['HTTP_REFERER']; now. You don't want to allow a malicious user to edit the HTTP headers you send.

What do you plan to do if any of the database calls fail? You don't seem to be checking for nor handling that. Also, it's best to have the exact same behavior on "invalid username" as well as "password supplied does not match". I'm not sure if your code does that or not.

I didn't think people usually put spaces around the -> method call operator, but at least be consistent with your spacing.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback Snowbody, appreciate it. I will resolve all of the issues you mentioned and re the spacing, it just makes it easier to read and from what I have experienced, doesn't effect processing/output. \$\endgroup\$ – Stan Howe Dec 8 '17 at 15:41
  • \$\begingroup\$ Thanks for the accept, but you really ought to wait 24-48 hours before accepting an answer, in case other people had things to say. \$\endgroup\$ – Snowbody Dec 8 '17 at 18:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.