2
\$\begingroup\$

I have created a test login page that, as most do, takes in two values from the user (username and password) and the processes it.

For this I have opted to use jQuery and AJAX to pass the results to a separate page to be processed, checked and validated before allowing access.

My main front end uses Twitter Bootstrap v3.3.5 and jQuery v1.11.3 (I know there are newer but it's what I am used to until I learn the newer one).

The front end looks like this:

<!DOCTYPE html>
<html>
<head>
    <title>Login</title>
    <meta charset="utf-8" />
    <meta name="viewport" content="width=device-width, initial-scale=1" />
    <link rel="stylesheet" href="http://maxcdn.bootstrapcdn.com/bootstrap/3.3.5/css/bootstrap.min.css" />
    <script src="https://ajax.googleapis.com/ajax/libs/jquery/1.11.3/jquery.min.js"></script>
    <script src="http://maxcdn.bootstrapcdn.com/bootstrap/3.3.5/js/bootstrap.min.js"></script>
    <!-- Custom styles and scripts -->
    <link rel="stylesheet" href="style/gw_login.css" />
    <script src="script/jquery.ajax.login.js"></script>
    <!-- //Custom styles and scripts -->
</head>

<body class="container-fluid">
    <div class="row">
        <div class="container">
            <div class="row">
                <h1>Login</h1>

                <table class="table table-condesned table-striped">
                    <tr>
                        <td><p><label for="username">Username</label></p></td>
                        <td><input type="text" id="username" placeholder="Username" class="form-control" /></td>
                    </tr>

                    <tr>
                        <td><p><label for="password">Password</label></p></td>
                        <td><input type="password" id="password" placeholder="password" class="form-control" /></td>
                    </tr>

                    <tr>
                        <td colspan="2"><button class="btn btn-warning form-control" id="login">Login</button></td>
                    </tr>
                </table>
            </div>

            <div class="row" id="error"></div>
            <!-- //table -->
        </div>
        <!-- //container -->
    </div>
    <!-- //row -->
</body>
<!-- //Body -->
</html>

The jQuery code is as such:

$( document ).ready( function ()
{
    $( "#login" ).on( "click", function ()
    {
        $.ajax( {
            url: "script/ajax/login.php",
            type: "GET",
            success: function ( $res )
            {
                if ( $res != "t" )
                {
                    if ( $res == "f" )
                    {
                        $( "#error" ).html( "Username and/or password is incorrect." );
                    }
                    else
                    {
                        $( "#error" ).html( $res );
                        return;
                    }
                }
                else
                {
                    alert( "Login success." );
                }
            }
        } );
    } );
} );

And finally my PHP login script:

    <?php
$errors = [];

// Check that username and password exist
if (!isset($_REQUEST['username']) || empty($_REQUEST['username']))
    $errors[] = "Please supply a username.";
if (!isset($_REQUEST['password']) || empty($_REQUEST['password']))
    $errors[] = "Please supply a password.";

//Check if there have been errors
if (count($errors) > 0)
{
    foreach ($errors as $error)
    {
        print ($error . "<br />");
    }
    die();
}

// Create database link
$link = new mysqli("server_ip", "username", "password", "database_name");

// Set variables for easier use
$username_safe = $link->real_escape_string($_REQUEST['username']);
$password = ($_REQUEST['password']);

// Allow the user to use both email or chosen username
$query = "SELECT `password` FROM `users` WHERE `username` = '{$username_safe}' or `email` = '{$username_safe}';"; 

// Perform the query
$query = $link->query($query);

// Check rows are there
if ($query->num_rows > 0)
{
    // Fetch the result
    while ($res = mysqli_fetch_assoc($query))
    { 
        // Check password against value held in database
        print (password_verify($password, $res['password']) ? "t" : "f");
        die();
    }
}
?>

The expected behaviour changes depending on what it is I am doing, sometimes I want to see Login success when I user values that exist in the database and other times a single error on either the username/password or both.

The behaviours that I expect occur. I wanted to know if there is a better way of doing what I currently am?

Improve this question
\$\endgroup\$

1 Answer 1

Reset to default
1
\$\begingroup\$

I generally like your code. It's very clean, and easily readable, so just a couple of small points:

Security

  • Don't pass sensitive data via GET. It will show up in server logs, in your browser history, in referers, and so on.
  • Don't use REQUEST, but the more concrete POST or GET instead. REQUEST allows a fallback to the other method, making some attacks (such as CSRF) easier.
  • You don't seem to have CSRF protection. This is important even for a login page, as it allows an attacker to force-login a victim, hoping that the victim will perform actions under the attackers account, which may disclosure sensitive information.
  • Using escaping does prevent SQL injection, but it's really not recommended, as it's easier to make mistakes than with prepared statements and the code is less readable as well.
  • Is a while loop really what you want with the results of the query? I would not allow duplicate emails or usernames, as it could easily lead to authentication problems.

Nested if

Personally, I really don't like nested ifs. You could rewrite your JavaScript success function like this:

        function ( $res ) {
            if ( $res == "t" ) {
                alert( "Login success." );
            } else if ( $res == "f" ) {
                $( "#error" ).html( "Username and/or password is incorrect." );
            } else {
                $( "#error" ).html( $res );
            }
        }

Misc

  • You don't need isset with empty, empty will check if it is set.
  • Always uppercase SQL keywords (like or).
  • I would extract your PHP code to functions, so it's reusable and easier to read. For example, if you want to add actual functionality to your login - instead of just printing false or true - you would need to go into the login code. If however you had a function checkLogin($user, $pass), the code you would need to look at and change would be smaller, reducing the change for mistakes.
  • In production code, most of your comments are not actually needed; they just describe what the code already tells a reader. A reader will actually notice this, and ignore most of your comments, possibly missing the actually important ones.
Improve this answer
\$\endgroup\$
3
  • \$\begingroup\$ A huge thank you for this, I will action the points you have stated in your answer, but with one more question, (bit of a noob here...) but what is CSRF and how do I prevent it? \$\endgroup\$
    – Can O' Spam
    Jan 6, 2016 at 11:06
  • \$\begingroup\$ @SamSwift웃 Basically, an attacker can get a user to send a request to your page, when the user visits the attackers website. The request is then send in the context of the victim, meaning with their credentials, etc. There are lots of ways to prevent CSRF, the most common is to generate a token which is saved in the session. Whenever performing sensitive tasks (mostly this should be POST requests), you add a hidden field to the form with this token. When processing POST requests, you compare the recieved value with the stored value \$\endgroup\$
    – tim
    Jan 6, 2016 at 11:16
  • \$\begingroup\$ Once again, thank you very much, I will action this ASAP. You have been a true life saver here :D \$\endgroup\$
    – Can O' Spam
    Jan 6, 2016 at 11:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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