5
\$\begingroup\$

I would like to ask someone if the code i will put here is Secured for SQL injection and my password hashing and checking while user is logging in is secure and done well.

My Database connection :

$hostname='localhost';
$username='root';
$password='';

try {
    $dbh = new PDO("mysql:host=$hostname;dbname=myshop",$username,$password);
    $dbh->query ('SET NAMES utf8');
    $dbh->query ('SET CHARACTER_SET utf8_unicode_ci');
    $dbh->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION); // <== add this line
} catch (PDOException $e) {
      echo $e->getMessage();
    }

My registration script :

include_once 'config.php';

$data = file_get_contents("php://input");
$data = json_decode($data, true);

try {
    $stmt = $dbh->prepare("INSERT INTO `accounts` (`account_id`, `firstname`, `lastname`, `username`, `password`, `email`, `city`, `postalcode`, `adress`, `country`, `role`)
      VALUES (NULL, :firstname, :lastname, :username, :password, :email, :city, :postalcode, :adress, :country, 0) ");
    $stmt->bindParam(':firstname', $data['firstname']);
    $stmt->bindParam(':lastname', $data['lastname']);
    $stmt->bindParam(':username', $data['username']);
    $stmt->bindParam(':password', password_hash($data['password'], PASSWORD_DEFAULT));
    $stmt->bindParam(':email', $data['email']);
    $stmt->bindParam(':city', $data['city']);
    $stmt->bindParam(':postalcode', $data['postalcode']);
    $stmt->bindParam(':adress', $data['adress']);
    $stmt->bindParam(':country', $data['country']);
    $stmt->execute();
  } catch (PDOException $e) {
      if (strpos($e->getMessage(), "for key 'login'") !== false) {
          echo 'Duplicate login entry';
          exit;
      } if (strpos($e->getMessage(), "for key 'email'") !== false) {
          echo 'Duplicate e-mail entry';
          exit;
      }
      else {
          throw $e;
      }
  }
  echo 'Success';

My login script :

include_once 'config.php';

$data = file_get_contents("php://input");
$data = json_decode($data, true);

try {
  $stmt = $dbh->prepare("SELECT * from accounts WHERE
    username=:username");
  $stmt->bindParam(':username', $data['username']);
  $stmt->execute();
  } catch (PDOException $e) {
          throw $e;
      }

  $rows = $stmt->rowCount();
  $result = $stmt->fetchAll(PDO::FETCH_ASSOC);
  $json = json_encode($result);

  if ($rows === 1){
    if (password_verify($data['password'], $result[0]['password'])) {
      echo $json;
      exit;
    }
    else{
      echo 'Wrong password';
      exit;
    }}
  if ($rows === 0) {
    echo 'No account';
    exit;
  }
  else {
    echo 'Something went wrong';
    exit;
\$\endgroup\$
2
\$\begingroup\$

Your code is secure against SQL injection but it's largely inefficient. And this army of bindParams's takes the largest part in it. All you need is just to send an array you already have directly into execute():

$stmt->execute($data);

Registration

On the other hand, your use of try and catch operator for insert is proper, which is a rare sight. The only objection is error messages that shouldn't be bluntly echoed out. Better to collect them and show nicely along with a form. So I would make the whole registration code as

$error = '';
$data['password'] = password_hash($data['password'], PASSWORD_DEFAULT);
try {
    $stmt = $dbh->prepare("INSERT INTO `accounts` (`account_id`, `firstname`, `lastname`, `username`, `password`, `email`, `city`, `postalcode`, `adress`, `country`, `role`)
      VALUES (NULL, :firstname, :lastname, :username, :password, :email, :city, :postalcode, :adress, :country, 0) ");
    $stmt->execute($data);
} catch (PDOException $e) {
    if (strpos($e->getMessage(), "for key 'login'")) {
        $error =  'Duplicate login entry';
    } elseif (strpos($e->getMessage(), "for key 'email'")) {
        $error = 'Duplicate e-mail entry';
    } else {
        throw $e;
    }
}
if ($error) {
    echo $error;
} else {
    header("Location: ...");
    exit;
}  

Login

The login code could be improved as well. I've got a ready made example, Authenticating a user using PDO and password_verify(), you may check it out for the actual code. Here I will just comment on some parts

} catch (PDOException $e) {
    throw $e;
}

I fancy you just copy-pasted this part from the insert example. But if you think on it, it will become clear that this code will behave exactly the same without try, catch and throw operators! So they should be simple taken out.

$rows = $stmt->rowCount();

is essentially useless, as you have the fetch result that can be used instead.

$result = $stmt->fetchAll(PDO::FETCH_ASSOC);

fetchAll() doesn't seem to be a wise choice, as it adds unnecessary dimension to the resulting array. fetch() should be used instead.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you for your review of my code and a very good response. I will try to workout my code with all your tips and give u feedback about how it went ! :) \$\endgroup\$
    – SupremeDEV
    Apr 3 '18 at 12:59

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.