# How can I optimize my login script?

I've created an Ajax login for my website but I feel like I can optimize it, but I'm not sure how and where.

Questions:

• How can I optimize my code?
• Is the code secure? Any ways to break it (injection, etc)?

Here's my Ajax call:

$(document).ready(function() {$(document).on("submit", "form", function(event) {
event.preventDefault();
$.ajax({ url: 'assets/php/login_script.php', type: 'POST', data:$(this).serialize(),
success: function(data) {
if (data == true) {
window.location.href = "index.php";
} else {
$("input[name=password_field]").focus();$(".error").html(data);
}
}
});
});
});


Here's the PHP script:

<?php

include_once("access.php");

$cxn = mysqli_connect($host, $user,$pass, $db) or die ("Couldn't connect to the server. Please try again.");$username = $_POST["username"];$password = $_POST["password"];$date = date('Y-m-d h:i:s', time());
$ip_address = get_ip_address();$expire = time() + 86400 * 365;

$options = array('cost' => 12);$hash_password = password_hash($password, PASSWORD_BCRYPT,$options);

/* Log the login request. */
$stmt =$cxn->prepare("INSERT INTO login_logs (log_id, username, password, datetime, ip_address) VALUES ('', ?, ?, ?, ?)");
$stmt->bind_param('ssss',$username, $hash_password,$date, $ip_address);$stmt->execute();

/* Get user information from database. */
$stmt =$cxn->prepare('SELECT * FROM users WHERE username = ?');
$stmt->bind_param('s',$username);
$stmt->execute();$result = $stmt->get_result(); /* If a result exists, continue. */ if ($result->num_rows > 0) {
while ($row =$result->fetch_assoc()) {
$db_username =$row['username'];
$db_password =$row['password'];
$random_hash = password_hash(time() .$db_username . time(), PASSWORD_BCRYPT, $options); /* Password matches. */ if (password_verify($password, $db_password)) { /* Get user's cookie information in database. */$stmt2 = $cxn->prepare("SELECT * FROM cookies WHERE username = ?");$stmt2->bind_param('s', $db_username);$stmt2->execute();
$result2 =$stmt2->get_result();

/* If a result exists, update the cookie. */
if ($result2->num_rows > 0) {$stmt = $cxn->prepare("UPDATE cookies SET hash = ? WHERE username = ?");$stmt->bind_param('ss', $random_hash,$db_username);
$stmt->execute(); setcookie("user",$db_username, $expire, "/"); setcookie("hash",$random_hash, $expire, "/"); } else {$stmt = $cxn->prepare("INSERT INTO cookies (cookie_id, username, hash) VALUES ('', ?, ?)");$stmt->bind_param('ss', $db_username,$random_hash);
$stmt->execute(); setcookie("user",$db_username, $expire, "/"); setcookie("hash",$random_hash, $expire, "/"); } echo true; } else { echo "Incorrect credentials."; } } } else { echo "Incorrect credentials."; } function get_ip_address() {$ip_address = '';
if (getenv('HTTP_CLIENT_IP'))
$ip_address = getenv('HTTP_CLIENT_IP'); else if(getenv('HTTP_X_FORWARDED_FOR'))$ip_address = getenv('HTTP_X_FORWARDED_FOR');
else if(getenv('HTTP_X_FORWARDED'))
$ip_address = getenv('HTTP_X_FORWARDED'); else if(getenv('HTTP_FORWARDED_FOR'))$ip_address = getenv('HTTP_FORWARDED_FOR');
else if(getenv('HTTP_FORWARDED'))
$ip_address = getenv('HTTP_FORWARDED'); else if(getenv('REMOTE_ADDR'))$ip_address = getenv('REMOTE_ADDR');
else
$ip_address = 'UNKNOWN'; return$ip_address;
}

?>


How can I optimize my script to look better, be faster, etc?

Before I begin I just wanted to correct/add to a couple of things from Duniyadnd's post.

I've seen quite a few programmers who don't enjoy seeing arrays coming in as arguments

Why? This is a rather vague statement that is misleading without context. There is a time and a place for everything. Are you using an indexed array, named keys or position reliant data, instead of/in addition to the argument list? Then yes, that array is most likely bad (APIs with flag arrays might get a pass here). Are you performing some generic repetitive task on a bunch of different elements? Well then, that array is plausible. Does using an array make your code less reusable? There are a number of factors to take into consideration and each situation will have to be evaluated for viability independently. Just saying don't pass arrays as arguments is horribly limiting.

Is there a purpose of creating additional variables that are essentially the same values of the initial variables?

Yes. Yes there is. This makes code cleaner, thus easier to read. The extra memory blocks this takes up is negligible, especially on today's computers, and is a type of micro optimization that qualifies for premature optimization. PHP may be built on C, but it does not require manual memory management. Additionally, in this instance, creating $username could be setting up to sanitize $_POST[ 'username' ], which is a crucial step missing from this code (I'll get to this later).

Now that I'm done with the previous answer, I'll move on to the question.

jQuery

In the below snippet document is a jQuery selector. Selectors tell jQuery what to include in the jQuery object. The more specific the selector, the smaller the object; The smaller the object, the easier and faster it is to find what you're looking for. When you tell jQuery you want to search document for a submit event you have to search the whole document. Unless the content on the page is loaded dynamically you don't have to query the whole document. Instead, limit your selector to search for your form. Additionally, querying jQuery for the same selector multiple times forces jQuery to build multiple identical objects. When you are going to be performing multiple tasks on the same jQuery selector you should properly reference it with a variable, or in this case you would have used $( this ) as you were already in the $( document ).ready() scope. Finally, jQuery provides a submit() shortcut that does the same thing as on( 'submit' ).

$(document).on("submit", "form", function(event) { //becomes$( '#formID' ).submit( function( event ) {


Since we are in the scope of our form now we can reference its action and method attributes to populate our url and type indices. Now if either of these attributes ever change you wont have to update your code to match. Its better to write your code so that you only have to make a change once rather than search for every use.

$this =$( this );
url: $this.attr( 'action' ), type:$this.attr( 'method' ),


This goes for PHP and jQuery. If you are performing a loose comparison == and not an absolute comparison ===, then there is no need to compare to an actual boolean. The following are equivalent.

if( data == true ) {
if( data ) {

if( data != true ) {
if( ! data ) {


PHP

The *_once() versions of include and require are slightly more demanding on PHP. When its just one include this is trivial, but you should get in the habit writing your code so that there is no circular logic that would make this necessary. That's not to say you shouldn't ever use the *_once() versions, just that they are typically unnecessary. This is dangerously close to violating premature optimization, but figured I'd point it out none-the-less.

include_once("access.php");
//change to
include 'access.php';//you can add parenthesis if you wish, but they aren't necessary here


I noted above that I would be coming back to $username. Well, here it is. You should always sanitize user input, especially before uploading it to your database. There are many different ways to go about this, but for brevity I will use filter_input(). $username = filter_input( INPUT_POST, 'username', FILTER_SANITIZE_STRING );


Get in the habit of returning early. Doing so will reduce your indentation, which in turn increases legibility. This kind of code is a prime example of the arrow anti-pattern. With the arrow anti-pattern you typically have a really large if statement, followed by a small else statement. If you reverse the if logic to match your else logic then you can perform the else block first and return early, thus allowing the rest of the code to continue execution at normal indentation.

if( ! password_verify( $password,$db_password ) ) {
echo "Incorrect credentials.";
return;//we returned early rest of code is pseudo else statement
}

//rest of original if statement


I don't pretend to be a security expert, but I don't imagine storing even a hashed password client-side is acceptable. What would this even be used for? I'm assuming you're using it as some sort of credentials check. In which case you should really look into using sessions instead.

setcookie("hash", $random_hash,$expire, "/");
//becomes
$_SESSION[ 'hash' ] =$random_hash;
//or better yet
$_SESSION[ 'authenticated' ] = TRUE;  There are a number of things wrong with your get_ip_address() function. First and foremost is its repetition. First, you perform a boolean check on the return value of a function. Then, if it returns true, you query the same function again and set the results to a variable. That's two function calls when only one is necessary. Set it first then check it. Then you proceed to do the same thing for numerous other parameters. An array would be useful here. Create an array of all of your flags then loop over them to perform your search. Finally, once found there is no need to continue executing the script, you should return early again. $flags = array(
'HTTP_CLIENT_IP',
'HTTP_X_FORWARDED_FOR',
//etc...
);

foreach( $flags AS$flag ) {
$temp = getenv($flag );
if( $temp ) { return$temp;//you could also use break here if you needed to do something else
}
}

return 'UNKNOWN';


The last issue I saw was with your if statement syntax. PHP is not a braceless syntax language. Like many non-braceless languages, PHP promotes bad coding practices by continuing to allow semi-braceless syntax. For languages like Python, where you don't use braces, this is OK, but PHP inherently requires them, otherwise you wouldn't be limited to one line statements. And what does this save you really? Two bytes? Four or five if you add spacing? This is really a style preference, but it can easily become an issue if a more novice programmer takes over your code and doesn't understand this syntax.

• I'm glad you brought up your opinion about my post, but believe it should have been placed as a comment under mine so a discussion could place. I thought the my responses were clear about arguments-considering the context of this specific question. I'm glad you said it is quite vague without context. I do personally disagree that we should create additional variables that are essentially the same as $_POST to keep consistency and also keep track of where that information has come from. This allows the user to set rules for himself that they would never change the value of$_POST. – Duniyadnd Dec 4 '13 at 21:52

I've seen quite a few programmers who don't enjoy seeing arrays coming in as arguments into the code (me personally, I'm fine with it as long as there is some sort of documentation that goes with it so you don't forget what the function can and cannot take).

password_hash($password, PASSWORD_BCRYPT,$options);


password_hash($password, PASSWORD_BCRYPT,$cost);

When it comes to accepting username/passwords, you want to be sure you're on a secure location. Based on your current code, if you're only a http:// page (typically port 80) then a malicious bot can potentially sniff the username/password. Easiest way to do this, is run a check against $_SERVER['HTTPS'] but if that fails, you do want to redirect the user to another page or something like that. You shouldn't have more than one user with the same username, so create a LIMIT 1 at the end of the SQL query, otherwise it'll scan the entire table no matter how many results you get. $stmt = $cxn->prepare('SELECT * FROM users WHERE username = ? LIMIT 1 ');  Same concept for your cookies table lookup: $stmt2 = $cxn->prepare("SELECT * FROM cookies WHERE username = ? LIMIT 1");  Is there a purpose of creating additional variables that are essentially the same values of the initial variables? e.g. $username = $_POST['username'];  What that ends up doing is you will use up additional memory blocks when it's not really needed to happen. Also, you could lose track of what$username is down the road, and re-initialize it with a different value. Same idea when you're looking at your $row data. PASSWORD_BCRYPT is not meant to be fast, but it is meant to be secure. You're running it twice, with different values, is there a reason you're doing that for your logs considering you can't reverse the encryption? I'd just run it once and put it against$hash_password