2
\$\begingroup\$

I know there are other ways to create a login script like Object Oriented, Prepared Statements and PDO, but how about those old MySQL PHP users like me trying to change from old style to the new ways? Here is a login script code I created for those like me changing from MySQL to MySQLI the easy way.

Please take a look at the follow code and help us improve it, make it shorter, give us ideas, review is the symbols used are safe or not. How about adding brute force check so users can only fail to login 3 times per hour?

session_start();
include 'database/connection.php';
// we clean username against sql injection
$username=mysqli_real_escape_string($database,$_POST['form_user']);
// we clean password against sql injection
$password=mysqli_real_escape_string($database,$_POST['form_password']);
// if we get empty username and password we redirect to error page
if (empty($username) || empty($password)){header('Location: index.php?error=1');exit();}
// we clean username and password against XS injection, 0 to 9 and a to z and A to Z only allowed or we redirect to error page
if (preg_match("/[^A-Za-z0-9]/", $username)){header('Location: index.php?error=2');exit();}
if (preg_match("/[^A-Za-z0-9]/", $password)){header('Location: index.php?error=3');exit();}
// now we check if user exist 
$members="SELECT * FROM `accounts` WHERE `member` = '$username'";
$result=mysqli_query($database,$members);
// if user exist we redirect to error page
if(mysqli_num_rows($result)==0){header('Location: index.php?error=4');exit;}
// we will now check if salted password is correct
$user=mysqli_fetch_array($result,MYSQL_ASSOC);
// encoding method
$encoding=hash('sha256',$user['codificacion'].hash('sha256',$password));
// if password is not correct we redirect to error page
if($encoding!=$user['password']){header('Location: index.php?error=5');exit;}
// now everything is ok so we should set session, the session name will be member username
$_SESSION['username']=$user['member'];
// because we have admins, mods and users we need to check user privilege and redirect
if ($user['privilege']=="USER"){header('Location: users/index.php');exit();}
else if($user['privilege']=="ADMIN"){header('Location: admins/index.php');exit();}
else if($user['privilege']=="MOD"){header('Location: mods/index.php');exit();}
// if user is not admin or user or mod there must be something wrong so we kick them
else {header('Location: kick.php?error=6');exit();}
\$\endgroup\$
  • \$\begingroup\$ sorry I just wanted other users to read the code. I am new here but thanks \$\endgroup\$ – coderminator Apr 16 '15 at 3:12
5
\$\begingroup\$
  1. You're not checking whether $_POST[..] exists at all or whether any data has been submitted via POST, this may cause Undefined index notices.
  2. You're not supposed to use empty on variables that you're explicitly declaring. empty is shorthand for !isset($var) || !$var. Since your variables are guaranteed to be set (you define them in the previous two lines), no need to suppress error reporting here with empty.
  3. You don't need to clean a username or password against any sort of injection. Injection issues must be mitigated by properly escaping data in the right context, not by restricting the allowed characters used. If you have a specific business rule for restricting allowed characters so be it, but injection prevention is not it.
  4. You're MySQL-escaping the values before you're checking for their validity. You need to do that in the opposite order. Escaping is always the last thing you do. There's no real issue in this case, since you're not modifying the string after escaping and your validity check isn't influenced either; but you should still stick to that rule.
  5. You're using an inadequate password hash. Use password_hash instead.
  6. You could be using a switch instead of the if..else chain.
  7. You really should be using prepared statements to avoid escaping altogether. I know you're trying to take it slow, but you (should) have to make that move eventually, so why not now?

You may want to read:

In summary, I'd write it like this (using procedural style for your benefit):

if (!isset($_POST['form_user'], $_POST['form_password'])) {
    header('Location: index.php?error=1');
    exit;
}

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

$stmt = mysqli_prepare($database, 'SELECT * FROM `accounts` WHERE `member` = ?');
mysqli_stmt_bind_param($stmt, 's', $_POST['form_user']);
mysqli_stmt_execute($stmt);
$result = mysqli_stmt_get_result($stmt);
$user = mysqli_fetch_assoc($result);

if (!$user) {
    header('Location: index.php?error=4');
    exit;
}

if (!password_verify($_POST['form_password'], $user['password'])) {
    header('Location: index.php?error=5');
    exit;
}

$_SESSION['username'] = $user['member'];

switch ($user['privilege']) {
    case 'USER':
        header('Location: users/index.php');
        exit;
    case 'ADMIN': 
        header('Location: admins/index.php');
        exit;
    case 'MOD': 
        header('Location: mods/index.php');
        exit;
    default:
        header('Location: kick.php?error=6');
        exit;
}
\$\endgroup\$
  • \$\begingroup\$ This was a really nice piece of code you wrote, is this prepared statements ? It is shorter, clean. Will this work on server with PHP 4 or only PHP 5+ ? \$\endgroup\$ – coderminator Apr 16 '15 at 13:22
  • \$\begingroup\$ In this example can session_start(); be there you put it or it must be always on top of php code? \$\endgroup\$ – coderminator Apr 16 '15 at 13:30
  • \$\begingroup\$ if (!password_verify($_POST['form_password'], $user['password'])) { header('Location: index.php?error=5'); exit; } will this check the hash just like that? \$\endgroup\$ – coderminator Apr 16 '15 at 13:36
  • \$\begingroup\$ Yes, that's prepared statements. No reason not to learn them, see? :) I'm putting session_start after the POST check because there's no reason to do anything if the POST isn't in order. Arguably you can even move it to after the password check. Yes, the password_hash API is that awesome. All this is PHP 5+ and in parts even newer, stop using the old stuff. \$\endgroup\$ – deceze Apr 16 '15 at 14:45
  • \$\begingroup\$ Nice m8, the only problem is my hosting provider runs on PHP 4 \$\endgroup\$ – coderminator Apr 16 '15 at 15:30
3
\$\begingroup\$

That code needs a breather, it's hard to read. As far as I know (I don't know much), runs server-side, so there's no point in cramming it all into a monolithic block like you've done.

Here's your code, with no changes other than added whitespace and indentation:

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

    $username = mysqli_real_escape_string($database,$_POST['form_user']);
    $password = mysqli_real_escape_string($database,$_POST['form_password']);

    if (empty($username) || empty($password)) {
        header('Location: index.php?error=1');
        exit();
    }

    // we clean username and password against XS injection, 0 to 0 and a to b and A to B only allowed or we redirect to error page
    if (preg_match("/[^A-Za-z0-9]/", $username)) {
        header('Location: index.php?error=2');
        exit();
    }

    if (preg_match("/[^A-Za-z0-9]/", $password)) {
        header('Location: index.php?error=3');
        exit();
    }

    $members = "SELECT * FROM `accounts` WHERE `member` = '$username'";
    $result = mysqli_query($database,$members);

    if (mysqli_num_rows($result) == 0) {
        header('Location: index.php?error=4');
        exit;
    }

    // we will now check if salted password is correct
    $user = mysqli_fetch_array($result,MYSQL_ASSOC);
    $encoding = hash('sha256',$user['codificacion'].hash('sha256',$password));

    if($encoding != $user['password']) {
        header('Location: index.php?error=5');
        exit;
    }

    $_SESSION['username'] = $user['member'];

    // because we have admins, mods and users we need to check user privilege and redirect
    if ($user['privilege'] == "USER") {
        header('Location: users/index.php');
        exit();
    }
    else if ($user['privilege'] == "ADMIN") {
        header('Location: admins/index.php');
        exit();
    }
    else if ($user['privilege'] == "MOD") {
        header('Location: mods/index.php');
        exit();
    }
    else {
        // if user is not admin or user or mod there must be something wrong, so we kick them
        header('Location: kick.php?error=6');
        exit();
    }

Sure, it's a bit longer. But spacing between if and ( is consistent, and whitespace around operators make them stand out. Having a single instruction per code line also makes it easier to read, because every line ends with a ; semicolon.

Some of the comments are useful - those that say why you're doing such or such check... but a lot of them are just clutter that say what the code is doing. I don't write PHP, but it seems to me that this script has mixed concerns that could benefit being broken down into two or three methods:

  • Getting the user record out of the supplied form data
  • Authentication (i.e. validating the password)
  • Authorization (i.e. validating the role /privilege level)
\$\endgroup\$
  • \$\begingroup\$ Any ideas or example to improve the code? \$\endgroup\$ – coderminator Apr 16 '15 at 3:07
  • \$\begingroup\$ As I said, I don't write PHP so I don't want to stick a foot in my mouth here... I'd go with prepared statements / parameterized queries - as for improving the code, separating concerns would be a step forward IMO ;) \$\endgroup\$ – Mathieu Guindon Apr 16 '15 at 3:10
2
\$\begingroup\$

Every time you put a comment like

// we clean username against sql injection

ask yourself "why do I need this comment? why my intention is not obvious?". Usually the answer is "I need to delegate this to a properly named method". Consider

$username = clean_against_sql_injection($_POST['form_user']);
$password = clean_against_sql_injection($_POST['form_password');

A perk benefit of such approach is that it eliminate multiple maintenance: should the cleaning code be buggy (or should you decide to change DB engine) there is only one place to fix.

\$\endgroup\$

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.