HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

PHP Login Check

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
phpchecklogin

Problem

I use this block of code to check if the user is logged in (login stored in cookies as a plain text and pass MD5 crypted). As I'm a novice in PHP, I'm not sure that this is a correct way. Are there other easier ways of checking that you use?

if(isset($_COOKIE['login']) AND isset($_COOKIE['pass'])){
    $login = $_COOKIE['login'];
    $pass =  $_COOKIE['pass'];
    if($qw->query("SELECT count(id) FROM users where password = '".$pass."' and  email = '".$login."';")->fetchColumn() == 0){
        setcookie('login','');
        setcookie('pass','');
        header('Location:auth.php');
    }

} else {
    setcookie('login','');
    setcookie('pass','');
    header('Location:auth.php');
    die();

}

Solution

Are there other easier ways of checking that you use?

Any particular reason why you're using cookies to persist a user's authentication through every request?

I recommend using sessions to keep track of user authentications by storing its identifier and checking for its presence to know whether a user is already authenticated.

Using the MD5 algorithm for passwords is a bad idea, not because of its cryptographic weaknesses, but because it's fast. This means that an attacker can try billions of candidate passwords per second on a single GPU.

Did you know that PHP 5.5+ has the password_hash() function that takes care of all the troublesome details for you when it comes to creating secure password hashes?

-
I would use && as a convention instead of and if you're not utilizing its lower precedence. Better yet, you can do isset($_COOKIE['login'], $_COOKIE['pass']);. See the isset() manual.

-
When using PDO::query($statement) it requires you to properly escape all data to avoid SQL Injections and other issues. Use prepared statements to get rid of that.

-
Be consistent when writing code. If you like your SQL keywords uppercased, then uppercase them all, not just "most of them". It prevents potential confusion and it makes for code that's easier to read.

-
header(): "HTTP/1.1 requires an absolute URI as argument to » Location: including the scheme, hostname and absolute path, but some clients accept relative URIs.". So if you want to be safe you should use absolute paths.

-
Early exits will more often then not make your code cleaner and avoid nested if-statements that might hurt your code's readability.

Why are you setting the login and password cookies to an empty string regardless if the authentication has been successful or not? I would keep the cookies if the authentication was successful and ignore them if otherwise.

My 2 cents for now.

Context

StackExchange Code Review Q#64063, answer score: 3

Revisions (0)

No revisions yet.