1
\$\begingroup\$

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)?

Also, when I attempt to log in, it currently takes about 1 second to process the login (on localhost). Is this long?

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?

\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – Duniyadnd Dec 4 '13 at 21:52
1
\$\begingroup\$

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);

would instead be:

password_hash($password, PASSWORD_BCRYPT, $cost);

The above is purely an FYI and not my opinion.

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

If you're using time() multiple times across the page, that's when you may want to put that against a variable. Even though it's a very quick function, it'll be faster to look it up against a variable that has a fixed number and is stored in a particular memory block, creates a consistent value across the board in case you have to do additional comparisons against each other (compare something about the expiration date or check the value that got inserted into the log table and since that's a salt, might as well have that as a reference etc.).

If the username/password verification feels like it takes a long time, then it is taking a long time, that's the way I see it. If you don't mind how long it takes, you might be ok (as saying 1 second is relative to the actual speed of the script). However, for bottlenecks, you'd have to test pieces of your code to see where is it slowest.

Hope this helps.

\$\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.